Update DT DQM, calibration scripts, and local reco code to allow using "new" DB format #34612

namapane · 2021-07-24T11:31:51Z

PR description:

New DT payload formats were introduced with #5977, but so far have been adopted only for uncertainties (#9883).
A first bucnch of updates to DT vdrift calibration code/scripts to enable to optionally read/write constants in the new format was published with #31808.
This PR updates all remaining code. The default behaviour is unchanged (ie use the legacy format); it is one step towards enabling a complete migration.

PR validation:

all modifications checked to provide identical results with the default (old) format as well as when reading identical constants in the new format.

cmsbuild · 2021-07-24T11:41:55Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34612/24185

This PR adds an extra 216KB to repository

cmsbuild · 2021-07-24T11:42:14Z

A new Pull Request was created by @namapane (Nicola Amapane) for master.

It involves the following packages:

CalibMuon/DTCalibration (alca)
CondTools/DT (db)
DQM/DTMonitorClient (dqm)
DQM/DTMonitorModule (dqm)
RecoLocalMuon/DTRecHit (reconstruction)

@perrotta, @malbouis, @andrius-k, @yuanchao, @kmaeshima, @tlampen, @ErnestaP, @ahmad3213, @rvenditti, @cmsbuild, @jpata, @jfernan2, @slava77, @ggovi, @pohsun, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@bellan, @mmusich, @jhgoh, @tocheng, @battibass this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy, @perrotta you are the release manager for this.

cms-bot commands are listed here

slava77 · 2021-07-24T12:44:20Z

RecoLocalMuon/DTRecHit/plugins/DTLinearDriftFromDBAlgo.cc

@@ -34,19 +36,37 @@ DTLinearDriftFromDBAlgo::DTLinearDriftFromDBAlgo(const ParameterSet& config)
      // Option to force going back to digi time at Step 2
      stepTwoFromDigi(config.getParameter<bool>("stepTwoFromDigi")),
      useUncertDB(config.getParameter<bool>("useUncertDB")),
+      readLegacyTTrigDB(config.existsAs<bool>("readLegacyTTrigDB") ? config.getParameter<bool>("readLegacyTTrigDB")


::exists method is rather an exception left for cases for rather old and complex pre-7X software.
Define a ::fillDescriptions method https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp
If a fillDescriptions implementation is too complicated due to the tools/plugins coding pattern, please modify the existing configs appropriately and set the value to true/false there.

I discussed this with @battibass before PR-ing and our opinion is that ,oving to fillDescriptions consistently would be too complicated since his module is re-used in several places upstream, cf:
https://github.com/cms-sw/cmssw/search?l=Python&q=DTLinearDriftFromDBAlgo

OTHO, all existing configs have already been modified in this same PR. The reason we added existsAs is not because of cfgs, but because this module is used in HLT and we want to allow for a smooth transition while the new cards are added to confDB.

For HLT, a change can be inserted in https://github.com/cms-sw/cmssw/blob/master/HLTrigger/Configuration/python/customizeHLTforCMSSW.py

Uhm the clean solution is obviously to update confDB which (in my understanding) is trivial once the card is present in the relevant cfi.py. At that point the temporary existsAs can be removed. I think that editing customizeHLTforCMSSW.py would be an equally temporary solution, and it seem less clean potentially and more error-prone..
What about open an issue to re-check and clean-up existAs in DT code instead, so that we don't forget to remove the temporary existAs()? (I think there are a few to be removed elsewhere as well)

customizeHLTforCMSSW.py has been the preferred solution since a while

I see but as I mentioned this module is reused in a number of places, cf:
https://github.com/cms-sw/cmssw/search?l=Python&q=DTLinearDriftFromDBAlgo
which means we would have to add quite a number of temporary changes in customizeHLTforCMSSW.py instead than a single line in the code...
OTOH we have other existsAs to cleanup in the subsystem, so my proposal of coupling this with an issue would also go in the direction of a full cleanup and sync.
If that is not fine with you, my preference is to postpone modification of this class to a future PR, after the new cards have been added to confDB manually (and proceed with the rest of the PR now).

If that is not fine with you, my preference is to postpone modification of this class to a future PR, after the new cards have been added to confDB manually (and proceed with the rest of the PR now).

My preference in the current context (no dillDescriptions) is to have a customizeHLTforCMSSW.py and no existsAs

I understand but I really have a hard time figuring out how this would be done.
The new cards should be added to these psets:

hltDt1DRecHits.recAlgoConfig

hltDt4DSegments.Reco4DAlgoConfig.recAlgoConfig

hltDt4DSegments.Reco4DAlgoConfig.Reco2DAlgoConfig.recAlgoConfig

hltDt4DSegmentsMeanTimer.Reco4DAlgoConfig.recAlgoConfig

hltDt4DSegmentsMeanTimer.Reco4DAlgoConfig.Reco2DAlgoConfig.recAlgoConfig

How should that be coded in customizeHLTforCMSSW.py?
is there a way to add only the new cards, or should the entire pset be replaced?

there are relevant examples in customizeHLTforCMSSW.py already for replacement/parameter insertions by the module type.

IIUC, DTRecHitProducer is the first kind

for producer in producers_by_type(process, "DTRecHitProducer"): producer.recAlgoConfig.readLegacyTTrigDB = cms.bool(False)

the other cases are apparently DTRecSegment4DProducer

Should be done now. As far as I can tell, HLT runs correctly with the latest commit.

slava77 · 2021-07-24T12:46:01Z

RecoLocalMuon/DTRecHit/plugins/DTLinearDriftFromDBAlgo.cc

+    ESHandle<DTRecoConditions> hVdrift;
+    setup.get<DTRecoConditionsVdriftRcd>().get(hVdrift);


this additional es.get goes against the current campaign to define esConsumes.
Please consider adding esConsumes in this PR already.

OK will have a look on this on Monday (other get's will have to be migrated in the same class as well)

Uhm, this class is an helper that is called by the actual EDProducer, but is not an EDProducer itself.
So I cannot directly call esConsumes from here. ("error: 'esConsumes' was not declared in this scope")
@slava77, do you know how should I handle this use case? I checked the doc but found nothing apparently related.

the consumesCollector will need to be passed to the algorithm constructor.
a (somewhat random) example is available in isolation

cmssw/RecoMuon/MuonIsolationProducers/plugins/MuIsoDepositProducer.cc

Line 74 in 49cd0eb

theExtractor = IsoDepositExtractorFactory::get()->create(extractorName, extractorPSet, consumesCollector());

Mmhh this would mean updating every implementation under RecoLocalMuon/DTRecHit/plugins, don't you think it would make sense as a separate migration rather than being coupled to this specific PR?

it's OK to do in a follow up PR.

One thing I could do is to add this in the algo interface with cc=0 as a default and leave it dummy in other implementations not affected by this PR. Do you like the idea?

without compiling this locally, I'm not sure that this would help much.

We did something along the lines in the MuonServiceProxy; there the solution was to add an interface (constructor and related calls) with ConsumesCollector. But then in this case it will probably just complicate the implementation with something artificially temporary.

slava77 · 2021-07-24T12:46:52Z

@cmsbuild please test

qliphy · 2021-07-26T02:46:01Z

please abort

qliphy · 2021-07-26T02:46:37Z

please test

cmsbuild · 2021-07-26T06:02:48Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5be359/17188/summary.html
COMMIT: dce7665
CMSSW: CMSSW_12_0_X_2021-07-25-0000/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34612/17188/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 8 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5be359/17188/llvm-analysis/esrget-sa.txt for details.
CMS StaticAnalyzer warnings: There are 1 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5be359/17188/llvm-analysis/legacy-mod-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 2998564
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2998529
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 38 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

cmsbuild · 2021-08-07T01:43:32Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5be359/17624/summary.html
COMMIT: 8fa7b72
CMSSW: CMSSW_12_1_X_2021-08-06-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34612/17624/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 8 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5be359/17624/llvm-analysis/esrget-sa.txt for details.
CMS StaticAnalyzer warnings: There are 1 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5be359/17624/llvm-analysis/legacy-mod-sa.txt for details.

Comparison Summary

Summary:

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

slava77 · 2021-08-09T18:14:07Z

+reconstruction

for #34612 8fa7b72

code changes since the last reco signoff for e98692d were technical, to rebase after the context lines changed in HLTrigger/Configuration/python/customizeHLTforCMSSW.py
jenkins tests pass

jfernan2 · 2021-08-10T07:10:54Z

+1

Martin-Grunewald · 2021-08-10T12:04:11Z

+1

ggovi · 2021-08-10T14:47:17Z

+1

yuanchao · 2021-08-11T19:14:00Z

+1

enable new DT payload formats and validation
due to refactoring needed, esConsume mirgration to be done later
matrix tests and unit tests passed

cmsbuild · 2021-08-11T19:14:26Z

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)

qliphy · 2021-08-12T06:10:16Z

+1

namapane added 2 commits July 21, 2021 15:19

migrate DQM/DTMonitor* packages to new DTDB format

dd41f56

migrate DTVDriftAnalyzer to new DTDB format

9a304e2

cmsbuild added this to the CMSSW_12_0_X milestone Jul 24, 2021

cmsbuild added alca-pending code-checks-pending db-pending dqm-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jul 24, 2021

namapane changed the title ~~Update DT DQM, calibration scripts, and local reco code yo allow using "new" DB format~~ Update DT DQM, calibration scripts, and local reco code to allow using "new" DB format Jul 24, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 24, 2021

slava77 reviewed Jul 24, 2021

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Jul 24, 2021

cmsbuild added tests-pending and removed tests-started labels Jul 26, 2021

cmsbuild added tests-started and removed tests-pending labels Jul 26, 2021

cmsbuild added tests-approved and removed tests-started labels Jul 26, 2021

cmsbuild added tests-started and removed tests-rejected labels Aug 6, 2021

cmsbuild added tests-approved and removed tests-started labels Aug 7, 2021

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Aug 9, 2021

cmsbuild added dqm-approved and removed dqm-pending labels Aug 10, 2021

cmsbuild mentioned this pull request Aug 10, 2021

Enable boosted tau reco to run at miniAOD #34576

Merged

cmsbuild added hlt-approved and removed hlt-pending labels Aug 10, 2021

cmsbuild added db-approved and removed db-pending labels Aug 10, 2021

cmsbuild mentioned this pull request Aug 11, 2021

Unify vertex association between CHS and PUPPI #33885

Merged

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Aug 11, 2021

cmsbuild added orp-approved and removed orp-pending labels Aug 12, 2021

cmsbuild merged commit efd0017 into cms-sw:master Aug 12, 2021

cmsbuild mentioned this pull request Aug 12, 2021

[SimCalorimetry] Avoid using private CondFormats/EcalObjects/src/classes.h #34849

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update DT DQM, calibration scripts, and local reco code to allow using "new" DB format #34612

Update DT DQM, calibration scripts, and local reco code to allow using "new" DB format #34612

namapane commented Jul 24, 2021

cmsbuild commented Jul 24, 2021

cmsbuild commented Jul 24, 2021

slava77 Jul 24, 2021

namapane Jul 24, 2021

slava77 Jul 26, 2021

namapane Jul 26, 2021

slava77 Jul 26, 2021

namapane Jul 26, 2021

slava77 Jul 26, 2021

namapane Jul 27, 2021

slava77 Jul 27, 2021

namapane Jul 28, 2021

slava77 Jul 24, 2021

namapane Jul 24, 2021

namapane Jul 26, 2021

slava77 Jul 26, 2021

namapane Jul 26, 2021

slava77 Jul 26, 2021

namapane Jul 26, 2021

slava77 Jul 26, 2021

slava77 commented Jul 24, 2021

qliphy commented Jul 26, 2021

qliphy commented Jul 26, 2021

cmsbuild commented Jul 26, 2021

cmsbuild commented Aug 7, 2021

slava77 commented Aug 9, 2021

jfernan2 commented Aug 10, 2021

Martin-Grunewald commented Aug 10, 2021

ggovi commented Aug 10, 2021

yuanchao commented Aug 11, 2021

cmsbuild commented Aug 11, 2021

qliphy commented Aug 12, 2021

		ESHandle<DTRecoConditions> hVdrift;
		setup.get<DTRecoConditionsVdriftRcd>().get(hVdrift);

Update DT DQM, calibration scripts, and local reco code to allow using "new" DB format #34612

Update DT DQM, calibration scripts, and local reco code to allow using "new" DB format #34612

Conversation

namapane commented Jul 24, 2021

PR description:

PR validation:

cmsbuild commented Jul 24, 2021

cmsbuild commented Jul 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 commented Jul 24, 2021

qliphy commented Jul 26, 2021

qliphy commented Jul 26, 2021

cmsbuild commented Jul 26, 2021

Comparison Summary

cmsbuild commented Aug 7, 2021

Comparison Summary

slava77 commented Aug 9, 2021

jfernan2 commented Aug 10, 2021

Martin-Grunewald commented Aug 10, 2021

ggovi commented Aug 10, 2021

yuanchao commented Aug 11, 2021

cmsbuild commented Aug 11, 2021

qliphy commented Aug 12, 2021