-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added PPSAlignmentConfiguration - a new conditions format for PPS global alignment #35174
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35174/25100
|
A new Pull Request was created by @MatiXOfficial (Mateusz Kocot) for master. It involves the following packages:
@malbouis, @yuanchao, @cmsbuild, @ggovi, @francescobrivio, @tvami can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild , please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-374bc0/18346/summary.html Comparison SummarySummary:
|
+alca
|
@@ -10,6 +10,6 @@ | |||
<use name="CondFormats/PPSObjects"/> | |||
</bin> | |||
|
|||
<bin file="testSerializationPPSAlignmentConfig.cc"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PPSAlignmentConfig class is left in the code, or I did not notice the delete line? In the first case, why you remove its test ( and the above entries in the FETCH/IMPORT/PAYLOAD_2XML macros )?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I did not remove the PPSAlignmentConfig
class. Even though it's obsolete, as far as I understand, I can't remove it (and the record) since it is a conditions format (with some payloads already in the Conditions DB). Because it won't be used any more, I removed the test, the write/retrieve modules and the entries in CondCore/Utilities
(772b3e4), but I can revert these changes. What do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no workflow code is consuming the old payloads, in principle we could remove it. However, it is a choice that presents some risks... I would propose to consider its deletion in a future PR. In this one, please revert the changes related to the old CondFormats (tests and FETC/IMPORT/PAYLOAD_2XML). Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - now the files/lines of code related to the PPSAlignmentConfig class are not modified in this PR.
This reverts commit 772b3e4.
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35174/25250
|
@cmsbuild , please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-374bc0/18589/summary.html Comparison SummaryThe workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons Summary:
|
+1 |
+alca |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This PR adds a new conditions format -
PPSAlignmentConfiguration
- which will be used in PPS global alignment running in PCL. An older version of this format -PPSAlignmentConfig
- was uploaded to CMSSW in #31728 and fixed in #34844, but since it required some modifications, a new format had to be created. Although the obsoletePPSAlignmentConfig
class and its record cannot be removed, this PR removes its ESProducer and serialization tests.Main modifications:
PPSAlignmentHarvester
config,extraParams
that for now will be empty, but will be used in case any new parameters need to be added to the class. It is not planned, but still might be necessary.This is not the last PR in PPS global alignment roadmap for Run 3. The next PR will include updates to
PPSAlignmentWorker
andPPSAlignmentHarvester
, define a new matrix test and add a few files required by PCL. The whole strategy was presented in the AlCaDB meeting on 30 August. Note that the naming was changed fromPPSAlignmentConfigRun3v1
toPPSAlignmentConfiguration
after the presentation.PR validation:
This PR adds a serialization test (
testSerializationPPSAlignmentConfiguration
). It also adds modules that store aPPSAlignmentConfiguration
object in an SQLite file, and then retrieve the data from it. I was able to run these modules and retrieve correct objects.