-
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
Migrate some modules in SimPPS to esConsumes #34989
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34989/24834
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages:
@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild, please test |
@@ -53,7 +52,7 @@ namespace CLHEP { | |||
class RPDigiProducer : public edm::EDProducer { |
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.
This module needs to be migrated to some thread-efficient base class.
Is this module run in runTheMatrix
? It is included in a list of modules being run that I extracted in last spring, but I don't see it being flagged as one preventing efficient use of concurrent lumis (that legacy EDModules imply) in #25090.
And now I noticed the migration of cmssw/SimPPS/RPDigiProducer/plugins/RPDigiProducer.cc Lines 174 to 176 in 9b19580
that uses RPDisplacementGenerator in
that gets data from EventSetup , .e.gcmssw/SimPPS/RPDigiProducer/plugins/RPDisplacementGenerator.cc Lines 26 to 29 in 9b19580
) This needs to be changed to something that allows declaring the consumed EventSetup products to be declared in the |
FYI @cms-sw/ctpps-dpg-l2 (since these packages do not appear to have any watchers), see my comments above. |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b1da21/17983/summary.html CMS StaticAnalyzer warnings: There are 1 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b1da21/17983/llvm-analysis/legacy-mod-sa.txt for details. Comparison SummarySummary:
|
@mundim : can you check? Help in the RP digitizers is needed too. Thanks! |
Hello, @mundim asked me to take care of this so he can focus on the fullsim |
@cms-sw/simulation-l2 Could you review and sign this PR even if it not complete for esConsumes in |
@makortel , @fabferro , @clemencia , I would propose that tomorrow morning I will sign this PR and make issue for PPS development. If you disagree, please, let me know. |
+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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Part of #31061.
PR validation:
Code compiles.