-
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
Moved creating SiPixelTemplateStore from DB to an ESProducer #42514
Conversation
please test |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42514/36519
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages:
@perrotta, @ssekmen, @tvami, @nothingface0, @civanch, @emanueleusai, @consuegs, @mdhildreth, @mandrenguyen, @pmandrik, @saumyaphor4252, @clacaputo, @syuvivida, @sbein, @tjavaid, @micsucmed, @francescobrivio, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
a37977f
to
3839ad6
Compare
please test |
please test |
|
||
// user include files | ||
#include "CondFormats/SiPixelTransient/interface/SiPixelTemplate.h" | ||
#include "FWCore/Framework/interface/eventsetuprecord_registration_macro.h" |
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.
I believe this should be
#include "FWCore/Framework/interface/eventsetuprecord_registration_macro.h" | |
#include "FWCore/Utilities/interface/typelookup.h" |
which would then avoid adding the dependence on FWCore/Framework
to this package.
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42514/36520
|
+db |
@cms-sw/fastsim-l2 , could you please review this PR ? There are upcoming HLT PRs that require this PR to be merged first (changes to |
+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, @antoniovilela, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
Is it worth backporting this (at least to 13.2.X)? |
This appears to have broken some tests in the IB. I'm in the process of doing a fix. |
Let backport only what is necessary, and/or not too complicate (see also #42514 (comment)) |
AFAIK this makes a sizeable performance improvement in DQM memory requirement, so that's why I am asking if we want to make use of this or not (e.g. for the HI data-taking or for a second take on the pp 2023 reprocessing?) |
The fix for the unit test is in #42597 |
@cms-sw/ppd-l2 asked in the O&C today to backport this PR (+ #42597) down to 13_0_X. |
thanks @makortel ! Indeed, this will come in hand with the most likely rereco of the 2023 DATA to happen in 130X, in case it happens |
PR description:
All DB construction was switched to using EventSetup. Cases using reading from files were kept local to module.
The new ESProducer was added as a Task to all needed Sequences.
PR validation:
Code compiles. All changed python files are readable by python3. No cmsRun jobs were run.