Fix in SiPixelQualityESProducer to support two labelled records #40061
Conversation
|
@cmsbuild , please test
|
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40061/33038
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
ferencek
force-pushed
the
SiPixelQualityESProducer_fix-12_6_X
branch
from
e16b8c2 to
c61a662
Compare
cmsbuild
added
code-checks-pending
tests-pending
and removed
tests-started
code-checks-rejected
labels
|
urgent
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40061/33039
|
|
A new Pull Request was created by @ferencek (Dinko F.) for master. It involves the following packages:
@malbouis, @yuanchao, @cmsbuild, @saumyaphor4252, @francescobrivio, @ChrisMisan, @tvami can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
type bug-fix |
|
please test |
| labelTokens_ = | ||
| Tokens(setWhatProduced(this, &SiPixelQualityESProducer::produceWithLabel, edm::es::Label(label)), label); | ||
| } | ||
|
|
||
| label = conf_.exists("siPixelQualityLabel_RawToDigi") |
There was a problem hiding this comment.
exists is a deprecated API. The replacement is the use of fillDescriptions.
There was a problem hiding this comment.
Sure, I was actually surprised that there was not even an empty fillDescriptions but then I thought that rules were different for EDProducers and ESProducers. So thank you for pointing out that fillDescriptions should be used. However, I would leave this for a follow-up PR as this one deals with a very specific problem and, as far as I understood, needs to be backported to 12_4_X rather urgently. Any additions to this PR will just cause further delays.
There was a problem hiding this comment.
Hi @ferencek!
How long would it take to implement the fillDescriptions?
If it's a matter of delaying this PR of 1 day, I believe we can afford to wait and have it correctly implemented in one go, rhather than splitting it into 2 PRs I think.
There was a problem hiding this comment.
Well, not really sure, I haven't really dealt with fillDescriptions in a while. For a complete implementation one also needs to switch to an automatically generated cfi file which potentially leads to some knock-on effects in other parts of the code. In any case, this is likely at least 1 day of delay assuming I get to it tomorrow. On the other hand, I really don't see much point in backporting fillDescriptions to already closed release cycles. However, since I already have a working area set up, it would be a pity not to implement fillDescriptions for the currently open release cycle. I would just do it in a separate PR.
There was a problem hiding this comment.
@ferencek ok, from my (alca) side, we can then have the backports to 12_4_X and 12_5_X identical to this PR, and later a followup PR to add fillDescriptions in master only.
@cms-sw/orp-l2 @Dr15Jones do you think this plan is acceptable?
There was a problem hiding this comment.
@ferencek while we wait for feedback from you and release managers on the plan I detailed detailed above I will open tha backports of this PR based on what is implemented now. I hope that is fine with you and everyone!
There was a problem hiding this comment.
@francescobrivio, the backport branches are already in place (https://github.com/CMSTrackerDPG/cmssw/tree/SiPixelQualityESProducer_fix-12_5_X and https://github.com/CMSTrackerDPG/cmssw/tree/SiPixelQualityESProducer_fix-12_4_X). Feel free to make PRs. Thanks
|
@smuzaffar the tests of this PR have been running for 23hours and the DQM bin-by-bin comparison is still missing. |
|
Hi @francescobrivio, Comparison job is now running, I will keep an eye on the build. We have had some delays in the testing due to failures in the IB infrastructure. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-437665/28997/summary.html Comparison SummarySummary:
|
|
+alca
|
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 Backports are eventually needed for next 12_4 |
PR description:
This PR addresses the problem with the Run3Summer22 MC campaign reported on Jira where production jobs were crashing when the SiPixelRawToDigi module tried to load a labelled SiPixelQuality payload.
PR validation:
The code compiles and fixes the problem which was occurring in step2.
If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
If the tests are successful, the PR will be backported to 12_5_X and 12_4_X.