-
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
Migration of pixel code to esConsumes #32093
Migration of pixel code to esConsumes #32093
Conversation
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32093/19739
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
@@ -96,6 +96,7 @@ SiPixelEDAClient::SiPixelEDAClient(const edm::ParameterSet &ps) { | |||
sipixelDataQuality_ = new SiPixelDataQuality(offlineXMLfile_); | |||
|
|||
inputSourceToken_ = consumes<FEDRawDataCollection>(ps.getUntrackedParameter<string>("inputSource", "source")); | |||
cablingMapToken_ = esConsumes<SiPixelFedCablingMap, SiPixelFedCablingMapRcd, edm::Transition::EndLuminosityBlock>(); |
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.
@makortel, here the token is defined with the EndLuminosityBlock transition because the event setup is accessed in SiPixelEDAClient::dqmEndLuminosityBlock(...)
. However, the edm::ESHandle
assigned in the SiPixelEDAClient::dqmEndLuminosityBlock(...)
is later also used in SiPixelEDAClient::dqmEndJob(...)
. I am not sure if this could have any unwanted implications.
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.
Unfortunately use of EventSetup products at endJob()
(or at endProcessBlockProduce()
to which dqmEndJob()
translates now) is not allowed (framework may clean up the object before reaching that point).
Best option would be to move all computations using SiPixelFedCablingMap
to dqmEndLuminosityBlock()
. If that is not possible (or feasible), I believe the best workaround would be to copy the SiPixelFedCablingMap
(or necessary parts of it if the full object is no not needed) from dqmEndLuminosityBlock()
to dqmEndJob()
.
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.
Here is my attempt at implementing a workaround 6c41889
@makortel, how to handle |
Correct, |
…accessing EventSetup products in dqmEndJob() which is not allowed
OK, I implemented this in 226be17 |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32093/19771
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
Unfortunately yes, my bad. Hopefully they know how to unwatch a thread ;)
This would require a more extensive audit of the pixel code so perhaps something to think about.
I would add for the future in an ideal world ;) |
If the class is not used, and not intended to be used in the future, the easiest "fix" would be to remove it. If the class needs to stay for some reason, passing the ESProduct in the function is a viable option. But I'd suggest to leave the |
@mmusich, @tsusa, @tvami, do you know who might be the best person to ask about removing this class. Perhaps @dkotlins knows since he commented out this class in |
Just reporting the outcome of the private discussion within the DPG. |
And just to add, the PR is therefore complete and ready for review and sign-off by the relevant code responsibles. |
+reconstruction
|
+1 |
+1 |
+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 |
PR description:
This PR addresses the esConsumes migration (#31061) for the SiPixel code. This is a purely framework-related update so no changes in the output are expected. In the process some limited cleanup of header file includes was done. As a by-product, the code in
RecoLocalTracker/SiPixelClusterizer/test/
was fully migrated to consumes (there were still a lot ofgetByLabel
calls which were migrated togetByToken
). Code touched in #31697 intentionally skipped in this PR.PR validation:
Code compiles and explicitly tested by running workflow 4.53.
if this PR is a backport please specify the original PR and why you need to backport that PR:
No backport needed.