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
Fix EcalDQMStatusWriter to watch runs and load the input file #35466
Fix EcalDQMStatusWriter to watch runs and load the input file #35466
Conversation
… to date. Remove config file for non-existing EcalDQMStatusReader plugin.
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35466/25621
|
A new Pull Request was created by @thomreis (Thomas Reis) for master. It involves the following packages:
@emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d1676c/19230/summary.html Comparison SummarySummary:
|
+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) |
hold Do not merge this yet please. I need to make one more check. Probably unrelated to this PR but I want to be sure. |
Pull request has been put on hold by @thomreis |
unhold |
please test |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35466/25632
|
Pull request #35466 was updated. @emanueleusai, @ahmad3213, @jfernan2, @rvenditti, @pbo0, @pmandrik can you please check and sign again. |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d1676c/19244/summary.html Comparison SummarySummary:
|
@thomreis apparently the PR is fine, but since the real validation comes from you in the form of the db check, I will wait for your green light before approving. |
Hi @jfernan2 in the current state the xml dump of the created payloads looks reasonable. From my side I can give the green light. Thanks for approving. |
+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
|
#process.CondDBCommon.connect = 'oracle://cms_orcon_prod/CMS_COND_34X_ECAL' | ||
process.load("CondCore.CondDB.CondDB_cfi") | ||
process.CondDB.DBParameters.authenticationPath = '/nfshome0/popcondev/conddb' | ||
process.CondDB.connect = 'sqlite_file:mask-ECAL.db' |
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.
@thomreis This is the change producing a valid sqlite... nothing to do with conddb dump
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.
Without changing the writeOne()
functions in DQM/EcalMonitorDbModule/plugins/EcalDQMStatusWriter.cc from pointer based to reference based the produced sqlite file was not valid. Or at least the payload in it could not be printed.
void EcalDQMStatusWriter::analyze(edm::Event const &, edm::EventSetup const &_es) { | ||
cond::service::PoolDBOutputService &dbOutput(*edm::Service<cond::service::PoolDBOutputService>()); | ||
if (firstRun_ == dbOutput.endOfTime()) | ||
return; | ||
|
||
dbOutput.writeOne(&channelStatus_, firstRun_, "EcalDQMChannelStatusRcd"); | ||
dbOutput.writeOne(&towerStatus_, firstRun_, "EcalDQMTowerStatusRcd"); | ||
dbOutput.writeOne(channelStatus_, firstRun_, "EcalDQMChannelStatusRcd"); |
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.
Hi @ggovi
The changes that I needed to make in order to get conddb dump
to work are these two lines.
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.
@thomreis Can you please dump the error that you get with conddb dump?
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.
Sure. I have also sent it in an email to you last week already.
$ conddb --db mask-ECAL.db dump a68ed854e09eb04addd68ec350b820a4e171ca66
[2021-09-29 16:03:38,098] INFO: Connecting to mask-ECAL.db [sqlite:///mask-ECAL.db]
[2021-09-29 16:03:38,150] INFO: Found payload of type EcalCondTowerObjectContainer<EcalDQMStatusCode>*
[2021-09-29 16:03:39,303] WARNING: No XML converter for payload class EcalCondTowerObjectContainer<EcalDQMStatusCode>* found in the built-in library.
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.
@thomreis thanks, i'll investigate.
PR description:
After the esConsumes migration of the EcalDQMStatusWriter in #34963 the
beginRun()
function was not called anymore by the framework because the module has been made anedm::one::EDAnalyzer<>
. This means that the input file was not loaded and the module would have added only empty objects to the records. This PR changes the module type to aedm::one::EDAnalyzer<edm::one::WatchRuns>
in order to call thebeginRun()
function. The configuration file, which had been outdated, was updated to send the correct parameters to the module.In addition, the config file for readEcalDQMStatus was removed since the EcalDQMStatusReader module it is supposed to configure does not exist in CMSSW.
PR validation:
This module is run manually only and it has been tested by running the
DQM/EcalCommon/data/writeEcalDQMStatus.py
configuration, which is configured to produce an sqlite file with the two records.