esConsumes migration for EDModules in SiStrip packages, part 2 #32677

pieterdavid · 2021-01-18T13:54:40Z

PR description:

esConsumes migration #31061 for modules in packages with SiStrip in the name not done in #31826 (mostly DQM-related).
I tried to keep other changes minimal, but did some refactoring in SiStripMonitorCondData (making it use the helper class SiStripClasToMonitorCondData because they had a lot of duplicate code, and the esConsumes-related changes would have added even more); @sroychow @arossi83 could you have a look and comment on whether this is covered by the tests, or how to validate the changes otherwise?

PR validation:

compiles, runTheMatrix.py -l limited --useInput all passes and only shows differences in TimerService, but that may not cover all modified plugins.

…sioningSources

…sk, CalibrationTask, and CalibrationScanTask)

…ctionality (minimal changes to support both)

cmsbuild · 2021-01-18T14:03:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32677/20778

This PR adds an extra 356KB to repository

cmsbuild · 2021-01-18T14:03:54Z

A new Pull Request was created by @pieterdavid (Pieter David) for master.

It involves the following packages:

CalibTracker/SiStripQuality
DQM/SiStripCommissioningDbClients
DQM/SiStripCommissioningSources
DQM/SiStripMonitorClient
DQM/SiStripMonitorHardware
DQM/SiStripMonitorPedestals
DQM/SiStripMonitorSummary
DQMOffline/CalibTracker

@andrius-k, @yuanchao, @kmaeshima, @christopheralanwest, @ErnestaP, @cmsbuild, @jfernan2, @fioriNTU, @tlampen, @pohsun can you please review it and eventually sign? Thanks.
@echabert, @hdelanno, @erikbutz, @sroychow, @robervalwalsh, @gbenelli, @tocheng, @fioriNTU, @jandrea, @idebruyn, @mmusich, @venturia, @threus, @arossi83, @rociovilar this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

mmusich · 2021-01-18T14:06:52Z

@cmsbuild, please test

cmsbuild · 2021-01-18T16:49:30Z

-1

Failed Tests: HeaderConsistency
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d6f8ca/12323/summary.html
COMMIT: 03b1409
CMSSW: CMSSW_11_3_X_2021-01-17-2300/slc7_amd64_gcc900

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2716961
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2716938
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 156 log files, 37 edm output root files, 37 DQM output files

cmsbuild · 2021-01-28T00:23:35Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d6f8ca/12573/summary.html
COMMIT: 4389691
CMSSW: CMSSW_11_3_X_2021-01-27-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32677/12573/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2716596
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2716573
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 156 log files, 37 edm output root files, 37 DQM output files

yuanchao · 2021-01-28T09:48:08Z

@pieterdavid Generally "RunInfoRcd" is needed for sub-systems to run. But sorry, not knowing about your use case so not be able to comment here.

yuanchao · 2021-01-28T09:50:37Z

+1
Thanks a lot for the update.

jfernan2 · 2021-01-28T12:53:48Z

+1

cmsbuild · 2021-01-28T12:54:17Z

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)

qliphy · 2021-01-28T15:34:25Z

+1

makortel · 2021-01-29T15:34:33Z

By quick look it seems that this PR could be causing a failure of TestDQMOnlineClient-sistrip_dqm_sourceclient unit test, see #32770

makortel · 2021-01-29T15:36:33Z

DQM/SiStripMonitorClient/plugins/SiStripAnalyser.cc

-    if (!actionExecutor_.readTkMapConfiguration(eSetup)) {
+    detCabling_ = &eSetup.getData(detCablingToken_);
+    if (!actionExecutor_.readTkMapConfiguration(
+            detCabling_, &eSetup.getData(tkDetMapTokenER_), &eSetup.getData(tTopoTokenER_))) {


The token was declared to be consumed in EndRun, but is being used in beginRun.

makortel · 2021-01-29T15:37:09Z

DQM/SiStripMonitorClient/plugins/SiStripAnalyser.cc

@@ -103,7 +101,8 @@ void SiStripAnalyser::analyze(edm::Event const& e, edm::EventSetup const& eSetup
      actionExecutor_.createDummyShiftReport();
    } else {
      auto& dqm_store = *edm::Service<DQMStore>{};
-      actionExecutor_.fillStatus(dqm_store, detCabling_, eSetup);
+      actionExecutor_.fillStatus(
+          dqm_store, detCabling_, &eSetup.getData(tkDetMapTokenELB_), &eSetup.getData(tTopoTokenELB_));


The token was declared to be consumed in EndLuminosityBlock, but is being used in analyze.

mmusich · 2021-01-29T16:44:09Z

I gave a try to fix the issue #32770 at #32771.
Wondering why the DQM online client unit tests did not run in the PR tests here?
Can someone clarify how the unit tests to be run in a PR are chosen? Via dependencies?

jfernan2 · 2021-01-29T17:27:06Z

I guess it is via dependencies yes, changes in DQM/Integration package in this case
Not sure if experts want to modify the jenkins settings to run always for every PR unit tests which are targeting Online DQM world only
Thanks for the fix

makortel · 2021-01-29T17:30:26Z

I gave a try to fix the issue #32770 at #32771.

Thanks!

Can someone clarify how the unit tests to be run in a PR are chosen? Via dependencies?

Essentially the code of the PR is checked out, then git cms-checkdeps -a (i.e. all C++ and python source dependencies), and the unit tests of those packages are run. This misses unit tests in a package outside of the aforementioned set of packages that e.g. runs a configuration that loads a plugin defined in the aforementioned set of packages.

This is a known deficiency in the test system that has been discussed a few times. It is possible to ask the bot to run all unit tests, but IIUC the full(_cmssw|) would would lead to also building full CMSSW (@smuzaffar please clarify). So far such failures have been rare-enough for catching them in IBs to be sufficient (even if suboptimal).

smuzaffar · 2021-01-29T17:59:57Z

For now, one can explicilty ask to run tests for a package by setting the addpkg = CMSSW/Package test parameter. I am looking in to enabling full unit tests for each PR that should avoid such issues.

makortel · 2021-01-29T18:53:18Z

For now, one can explicilty ask to run tests for a package by setting the addpkg = CMSSW/Package test parameter.

Thanks Shahzad. I remembered something was done, but didn't remember what exactly, and somehow missed the addpkg in http://cms-sw.github.io/cms-bot-cmssw-cmds.html.

pieterdavid · 2021-02-01T12:55:53Z

I gave a try to fix the issue #32770 at #32771.

Thanks @mmusich !

silviodonato · 2021-05-31T15:29:20Z

see dmwm/T0#4574

pieterdavid added 14 commits January 16, 2021 11:02

esConsumes for DQMOffline/CalibTracker

243e7c9

use code-checks suggestion (character overload for single-char string)

120f637

esConsumes migration for DQM/SiStripMonitorPedestals

a5cf605

esConsumes for DQm/SiStripMonitorHardware

7c50a84

DQM/SiStripCommissioningSources: FineDelayHit, direct use from Commis…

901a16b

…sioningSources

esConsumes for pedestals and noise used in CommissioninTasks (NoiseTa…

9cb2b14

…sk, CalibrationTask, and CalibrationScanTask)

esConsumes migration for DQM/SiStripMonitorClient

0236379

soem plugins in DQM/SiStripMonitorSummary

7176acd

SiStripMonitorCondData plugin: use the helper class with the same fun…

438f91b

…ctionality (minimal changes to support both)

Rest of DQM/SiStripMonitorSummary

f0ad25b

DQM/SiStripCommissioningDbClients esConsumes migration

c769e38

fix for SiStripAnalyser

754d868

Code-checks fixes

3ba23a5

Add forgotten ESWatcher.check calls

03b1409

cmsbuild added this to the CMSSW_11_3_X milestone Jan 18, 2021

cmsbuild added alca-pending code-checks-pending dqm-pending orp-pending pending-signatures tests-pending labels Jan 18, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 18, 2021

cmsbuild added tests-started and removed tests-pending labels Jan 18, 2021

cmsbuild removed the tests-started label Jan 18, 2021

cmsbuild added the tests-started label Jan 27, 2021

cmsbuild added tests-approved and removed tests-started labels Jan 28, 2021

cmsbuild added alca-approved and removed alca-pending labels Jan 28, 2021

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Jan 28, 2021

cmsbuild added orp-approved and removed orp-pending labels Jan 28, 2021

cmsbuild merged commit 24b65fa into cms-sw:master Jan 28, 2021

makortel mentioned this pull request Jan 29, 2021

Failure in TestDQMOnlineClient-sistrip_dqm_sourceclient unit test #32770

Closed

makortel reviewed Jan 29, 2021

View reviewed changes

pieterdavid mentioned this pull request May 5, 2021

esConsumes migration for SiStripFedZeroSuppression, SiStripHitEffFromCalibTree, and SiStripClusterToDigiProducer #33632

Merged

silviodonato mentioned this pull request May 31, 2021

Replay test with CMSSW_11_3_1 dmwm/T0#4574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esConsumes migration for EDModules in SiStrip packages, part 2 #32677

esConsumes migration for EDModules in SiStrip packages, part 2 #32677

pieterdavid commented Jan 18, 2021

cmsbuild commented Jan 18, 2021

cmsbuild commented Jan 18, 2021

mmusich commented Jan 18, 2021

cmsbuild commented Jan 18, 2021

cmsbuild commented Jan 28, 2021

yuanchao commented Jan 28, 2021 •

edited

yuanchao commented Jan 28, 2021

jfernan2 commented Jan 28, 2021

cmsbuild commented Jan 28, 2021

qliphy commented Jan 28, 2021

makortel commented Jan 29, 2021

makortel Jan 29, 2021

makortel Jan 29, 2021 •

edited

mmusich commented Jan 29, 2021

jfernan2 commented Jan 29, 2021

makortel commented Jan 29, 2021

smuzaffar commented Jan 29, 2021

makortel commented Jan 29, 2021

pieterdavid commented Feb 1, 2021

silviodonato commented May 31, 2021

esConsumes migration for EDModules in SiStrip packages, part 2 #32677

esConsumes migration for EDModules in SiStrip packages, part 2 #32677

Conversation

pieterdavid commented Jan 18, 2021

PR description:

PR validation:

cmsbuild commented Jan 18, 2021

cmsbuild commented Jan 18, 2021

mmusich commented Jan 18, 2021

cmsbuild commented Jan 18, 2021

Comparison Summary

cmsbuild commented Jan 28, 2021

Comparison Summary

yuanchao commented Jan 28, 2021 • edited

yuanchao commented Jan 28, 2021

jfernan2 commented Jan 28, 2021

cmsbuild commented Jan 28, 2021

qliphy commented Jan 28, 2021

makortel commented Jan 29, 2021

makortel Jan 29, 2021

Choose a reason for hiding this comment

makortel Jan 29, 2021 • edited

Choose a reason for hiding this comment

mmusich commented Jan 29, 2021

jfernan2 commented Jan 29, 2021

makortel commented Jan 29, 2021

smuzaffar commented Jan 29, 2021

makortel commented Jan 29, 2021

pieterdavid commented Feb 1, 2021

silviodonato commented May 31, 2021

yuanchao commented Jan 28, 2021 •

edited

makortel Jan 29, 2021 •

edited