-
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
fractional prescale propagation into L1-O2O in 112 x #30886
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30886/17269
|
A new Pull Request was created by @hjkwon260 (Hyejin Kwon) for master. It involves the following packages: CondFormats/L1TObjects @benkrikler, @christopheralanwest, @tocheng, @cmsbuild, @rekovic, @tlampen, @ggovi, @pohsun can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@@ -18,7 +18,7 @@ class L1TGlobalPrescalesVetos { | |||
} | |||
|
|||
unsigned int version_; | |||
std::vector<std::vector<int> > prescale_table_; | |||
std::vector<std::vector<double> > prescale_table_; |
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.
@hjkwon260 You can't change data attributes types/names, since the back-ward compatibility in reading existing data would be broken.
If this change is required, you should create a new class.
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.
@ggovi thank you very much for the comment! I see, I'll create a new class and propagate it to o2o producer
Dear @hjkwon260 , is this something that would be needed for the next MWGR ? (scheduled first week of September) - Thanks |
Dear @boudoul Yes, we are planning so, thank you |
please test |
do you have any news @hjkwon260 ? |
The tests are being triggered in jenkins.
|
+1 |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 |
@hjkwon260 @gekobs @panoskatsoulis @cavana @zhenbinwu @BenjaminRS @bundocka @rekovic : What is the plan for testing this PR prior to the next MWGR ? Adding aslo @tapio, @christopheralanwest as they are presumably part of builing this plan for testing . Thanks |
HI @boudoul -- we are also currently discussing with TSG and FOG to see what else is needed for this to go in. But it still needs sign off by various groups. |
Very good, thanks @BenjaminRS |
@cms-sw/db-l2 @cms-sw/l1-l2 |
So as requested we did a little bit of investigating in the HLT, using PrintEventSetupDataRetrieval, we can see which es products the HLT is actually consuming. This should be a sufficient test. The conclusions are: the HLT as run online does not consume the prescale record and therefore this change can have no effect. This has been tested on both a full GRun menu offline and the current cosmics menu on hilton However when run for development, one of our analyser paths does consume this via L1TGlobalUtil. Additionally L1TGlobalProducer may be optionally configured to read the prescale information. Both of these code are L1 owned and therefore the L1 group should ideally update this code before progressing as without doing will limit our ability to analyse the prescale information of runs taken with this offline. In the interests of expediency and given the current effects are minor, I am willing to instead take the promise of the L1 group to submit this PR within a month after the MGWR. When this is done there is the requirement that inside a CMSSW job we should be able to access the prescale information of any data we run over, thus the solutions should ensure that we can still read the precale information of the old data. I would also encourage you to find a solution that ensures that this is as transparent and robust as possible. For example, I would encourage you to write an iov in the existing record which is set to a dumy value (all 0 prescales or something) so it is absolutely clear to a user that they are now picking up the wrong record. I also think the simpliest approach would be to convert all the existing records for the data into the new format, this I strongly suspect could be easily achieved with a day or two worth of effort with a simple script. We would therefore only have one record to worry about rather than having to fall back to the old one via some error prone mechanism. I assume you are in full consultation with the OMS developers on this change. |
+1 |
Kind reminder @cms-sw/l1-l2 |
@rekovic this PR is waiting for a review/signature for a long time. We need to merge this soon in order to have the backport ready for the next MWGR. Do you have any comments? Thank you |
+1 |
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
@hjkwon260 please make a backport for |
Hi @rekovic , we were expecting a bit more than just a '+1' . There are comments from Sam which would be good to answer/follow up . Thanks |
Hi @boudoul |
Hi, I now get a missing L1GtPrescaleFactorsAlgoTrigTrivialProducer but I do not see it removed in this PR:
|
Solved by #31593 |
PR description:
To adapt fractional prescale into the L1 O2O payload, data type of the prescale is changed from the integer.
This update involves change in CondFormats, so everywhere fetch the prescale in L1T codes is also updated to success the compilation.
We (L1 O2O team, @gekobs @panoskatsoulis ) understand the changes would involve the needed change for uGT emulator. So I ping @cavana @zhenbinwu of this PR.
PR validation:
The code has been tested running the runTheMatrix workflows, code-checks and code-format.
Local test on O2O was done using tsc/rs key containing fractional prescale, checked the payload contains the fractional vaules.
Backport 11_1_X: #31545