Migrate DQMEDAnalyzers from stream to one modules
#22218
Conversation
|
The code-checks are being triggered in jenkins. |
|
First attempt at a simpler, partial migration for #22157 . |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22218/3364 |
|
A new Pull Request was created by @fwyzard (Andrea Bocci) for master. It involves the following packages: DQMServices/Core @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @vanbesien can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
relative to #22157, I see
|
cmsbuild
added
code-checks-pending
tests-pending
and removed
code-checks-approved
tests-started
labels
|
The code-checks are being triggered in jenkins. |
fwyzard
force-pushed
the
migrate_DQM_to_one_modules
branch
from
54278b6
to
df16684
Compare
|
@cmsbuild please test |
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22218/3366 |
|
Pull request #22218 was updated. @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @vanbesien can you please check and sign again. |
|
Comparison job queued. |
|
+1 |
cmsbuild
added
dqm-approved
fully-signed
and removed
dqm-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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
Comparison is ready Comparison Summary:
|
|
+1 |
| @@ -856,7 +707,7 @@ DQMStore::book_(const std::string &dir, const std::string &name, | |||
| { | |||
| // Create and initialise core object. | |||
| assert(dirs_.count(dir)); | |||
| MonitorElement proto(&*dirs_.find(dir), name, run_, streamId_, moduleId_); | |||
| MonitorElement proto(&*dirs_.find(dir), name, run_, moduleId_); | |||
There was a problem hiding this comment.
@fwyzard ugh... What have you done there? Did you also remove the streamId from the MonitorElement constructor? It should be removed, but the change is not in this PR, so now streamIds and moduleIds are messed up!
|
I thought I did fix all the constructors as well... I can have a look
tomorrow.
|
|
@schneiml from a cursory look, I think I have updated all the calls to the If you can point me where you see an inconsistency between the |
|
@fwyzard streamId is still in the constructor declaration, so you are passing moduleId instead of streamId. Which would be sort of fine if it was used consistently, but some places do nontrival queries on the moduleId (e.g., |
|
Ah, I see... and it was not spot before by the compiler because of all the default parameters. |
Migrate all DQMEDAnalyzer modules from
edm::stream::EDAnalyzer<...>toedm::one::EDAnalyzer<...>.This should significantly reduce the memory usage of a multithreaded DQM job, at the price of preventing the framework from analysing multiple runs or lumisections at the same time.
Lumisection-based histograms are handled in the
postModuleGlobalEndLumisignal of theDQMStore.If
DQMEDanalyzermodules are migrated fromEDAnalyzertoEDProduce<Accumulator>, that signal should be removed and the handling moved to the modules'endLuminosityBlockProducemethod.