Fix of the L1CaloTowerTreeProducer and L1uGTTreeProducer to avoid an exception MustUseESGetToken #36756

elfontan · 2022-01-20T09:13:10Z

PR description:

The PR is needed to prevent the L1 Trigger emulation to crash when running in CMSSW_12_3_X using the cmsDriver command.

PR validation:

Basic tests performed successfully starting from CMSSW_12_3_0_pre3.
From CMSSW_12_3_0_pre3/src:

scram b distclean
git cms-checkdeps -a -A
scram b -j 8
scram b runtests
runTheMatrix.py -l limited -i all --ibeos

Checks that the following cmsDriver commands (for MC and data) are running properly:

MC:

cmsDriver.py l1Ntuple --python_filename mc.py -s L1REPACK:uGT,RAW2DIGI --mc -n -1 --conditions auto:phase1_2021_realistic --era Run3 --no_output --customise=L1Trigger/L1TNtuples/customiseL1Ntuple.L1NtupleRAWEMU --customise=L1Trigger/Configuration/customiseUtils.L1TGlobalMenuXML --filein /store/relval/CMSSW_12_0_1/RelValTTbar_14TeV/GEN-SIM-DIGI-RAW/120X_mcRun3_2021_realistic_v7-v2/10000/b67e121b-b29f-4eb0-8628-b3aa1cb76720.root
cmsDriver.py l1Ntuple --python_filename mc.py -s L1REPACK:FullMC,RAW2DIGI --mc -n -1 --conditions auto:phase1_2021_realistic --era Run3 --no_output --customise=L1Trigger/L1TNtuples/customiseL1Ntuple.L1NtupleRAWEMU --customise=L1Trigger/Configuration/customiseUtils.L1TGlobalMenuXML --filein /store/relval/CMSSW_12_0_1/RelValTTbar_14TeV/GEN-SIM-DIGI-RAW/120X_mcRun3_2021_realistic_v7-v2/10000/b67e121b-b29f-4eb0-8628-b3aa1cb76720.root

Data:

cmsDriver.py l1Ntuple -s L1REPACK:uGT,RAW2DIGI --python_filename=data.py -n -1 --no_output --era=Run2_2018 --data --conditions=120X_dataRun2_v2 --customise=L1Trigger/Configuration/customiseReEmul.L1TReEmulFromRAW --customise=L1Trigger/L1TNtuples/customiseL1Ntuple.L1NtupleRAWEMU --customise=L1Trigger/Configuration/customiseUtils.L1TGlobalMenuXML --filein=root://xrootd-cms.infn.it//store/data/Run2018D/EphemeralZeroBias8/RAW/v1/000/320/497/00000/04E0297E-C893-E811-B0AA-FA163EADBE2E.root

…cer.cc

cmsbuild · 2022-01-20T09:20:29Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36756/27878

This PR adds an extra 16KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36756/27878/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36756/27878/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

…atic tests

cmsbuild · 2022-01-20T10:47:38Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36756/27879

This PR adds an extra 16KB to repository

cmsbuild · 2022-01-20T10:48:01Z

A new Pull Request was created by @elfontan (Elisa Fontanesi) for master.

It involves the following packages:

L1Trigger/L1TNtuples (l1)

@epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please review it and eventually sign? Thanks.
@dinyar, @missirol, @kreczko, @thomreis, @Martin-Grunewald this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

cecilecaillol · 2022-01-20T10:51:40Z

please test

perrotta

I understand that these plugins are meant to be used to produce private ntuples...

Nevertheless, since they are located in the plugin area, and not in test, I'd further fix the fact that they are legacy modules, and not thread safe ones. The simplest fix could be to make them edm::one::EDAnalyzer<>, but probably they could even become global modules as well with little effort

perrotta · 2022-01-20T10:54:18Z

L1Trigger/L1TNtuples/plugins/L1uGTTreeProducer.cc

@@ -47,6 +47,7 @@ class L1uGTTreeProducer : public edm::EDAnalyzer {

  // EDM input tokens
  const edm::EDGetTokenT<GlobalAlgBlkBxCollection> ugt_token_;
+  edm::ESGetToken<L1TUtmTriggerMenu, L1TUtmTriggerMenuRcd> l1GtMenuToken_;


Make it const, and initialize in the class initializer list

perrotta · 2022-01-20T11:00:16Z

L1Trigger/L1TNtuples/plugins/L1CaloTowerTreeProducer.cc

@@ -99,6 +100,7 @@ L1CaloTowerTreeProducer::L1CaloTowerTreeProducer(const edm::ParameterSet& iConfi
  ecalToken_ = consumes<EcalTrigPrimDigiCollection>(iConfig.getUntrackedParameter<edm::InputTag>("ecalToken"));
  hcalToken_ = consumes<HcalTrigPrimDigiCollection>(iConfig.getUntrackedParameter<edm::InputTag>("hcalToken"));
  l1TowerToken_ = consumes<l1t::CaloTowerBxCollection>(iConfig.getUntrackedParameter<edm::InputTag>("l1TowerToken"));
+  decoderToken_ = esConsumes<CaloTPGTranscoder, CaloTPGRecord>();


Certainly all these tokens, but I'd say also most (if not all) class parameters can be made const and initialized in the class initializer list

cmsbuild · 2022-01-20T14:30:39Z

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22ebae/21841/summary.html
COMMIT: 25fd42a
CMSSW: CMSSW_12_3_X_2022-01-19-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36756/21841/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 2 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22ebae/21841/llvm-analysis/legacy-mod-sa.txt for details.

RelVals-INPUT

4.764.76_ZMuSkim2012D+ZMuSkim2012D+HLTDSKIM2+RECODR1reHLT2+HARVESTDR1reHLT/step2_ZMuSkim2012D+ZMuSkim2012D+HLTDSKIM2+RECODR1reHLT2+HARVESTDR1reHLT.log

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 43
DQMHistoTests: Total histograms compared: 3464860
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3464832
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
Checked 181 log files, 42 edm output root files, 43 DQM output files
TriggerResults: no differences found

… initializer list)

elfontan · 2022-01-21T12:37:57Z

@perrotta changes mentioned above should be correctly included, thank you very much for the suggestions!

cecilecaillol · 2022-01-21T13:20:37Z

@perrotta The relvals validation failed for a file open error, similarly as in other of our PRs today and yesterday. Is there a central problem? What can we do?

perrotta · 2022-01-21T13:25:55Z

@perrotta The relvals validation failed for a file open error, similarly as in other of our PRs today and yesterday. Is there a central problem? What can we do?

Yes, it is there since a few days already. The workflow must be updated with a reachable input file, ppd was informed. See also #36771

In the meanwhile, just ignore that single workflow, and consider the validation as passed if all other succeed

cecilecaillol · 2022-01-21T13:36:58Z

+l1

cmsbuild · 2022-01-21T13:37:20Z

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). 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)

perrotta · 2022-01-21T13:59:42Z

+1

The fix applies to a couple of plugins only used to privately produce some L1 Trigger DPG ntuples used for L1T developments: as such it cannot affect the output of any workflow, and therefore it is suitable to be merged now for 12_3_0_pre4, even if that pre-release is only intended to integrate the updates/fixes to the Run3 geometry on top of pre3
(There is other code in the same area that also needs to be updated at some point)

perrotta · 2022-01-21T14:00:55Z

merge

Dr15Jones · 2022-01-21T19:22:54Z

L1Trigger/L1TNtuples/plugins/L1CaloTowerTreeProducer.cc

@@ -59,10 +59,10 @@ Description: Produce L1 Extra tree
 // class declaration
 //

-class L1CaloTowerTreeProducer : public edm::EDAnalyzer {
+class L1CaloTowerTreeProducer : public edm::one::EDAnalyzer<> {


Since this is using the TFileService and only one module at a time can use that Service, this needs to be

class L1CaloTowerTreeProducer : public edm::one::EDAnalyzer<edm::one::SharedResources> {

Dr15Jones · 2022-01-21T19:23:26Z

L1Trigger/L1TNtuples/plugins/L1CaloTowerTreeProducer.cc

+      ecalToken_(consumes<EcalTrigPrimDigiCollection>(iConfig.getUntrackedParameter<edm::InputTag>("ecalToken"))),
+      hcalToken_(consumes<HcalTrigPrimDigiCollection>(iConfig.getUntrackedParameter<edm::InputTag>("hcalToken"))),
+      l1TowerToken_(consumes<l1t::CaloTowerBxCollection>(iConfig.getUntrackedParameter<edm::InputTag>("l1TowerToken"))),
+      decoderToken_(esConsumes<CaloTPGTranscoder, CaloTPGRecord>()) {
  edm::InputTag clusterTag = iConfig.getUntrackedParameter<edm::InputTag>("l1ClusterToken");


One also needs to add in the constructor

usesResource(TFileService::kSharedResource);

perrotta · 2022-01-22T07:25:05Z

Thank you @Dr15Jones , indeed we forgot it
A fix following your recipe for the two files touched here is now submitted in #36777
@elfontan (or whoever will take care of migrating the remaining files in this package) will take care of it in the next updates.

elfontan · 2022-01-23T09:49:41Z

@Dr15Jones and @perrotta, thank you very much for the fix and the explanation!
Can you please further clarify how I can distinguish when I am using a TFileService (so that only one module at a time can use the service) and when not and the difference with other cases (e.g. here [1] I see WatchRun and here [2] nothing)?
Thanks a lot,
--Elisa

[1]

cmssw/L1Trigger/L1TMuonOverlapPhase1/plugins/L1MuonOverlapPhase1ParamsDBProducer.h

Line 16 in ffe0bbf

    
           class L1MuonOverlapPhase1ParamsDBProducer : public edm::one::EDAnalyzer<edm::one::WatchRuns> {

[2]

cmssw/L1Trigger/GlobalCaloTrigger/test/L1GctTestAnalyzer.h

Line 34 in c33bb6c

class L1GctTestAnalyzer : public edm::one::EDAnalyzer<> {

perrotta · 2022-01-23T21:24:53Z

Uhm... In [1] I don't see any TFileService used in the code, and I would say that WatchRun was not actually needed even there, in fact. @Dr15Jones, what do you think? Am I wrong? Elisa Fontanesi ***@***.***> ha scritto:

…

@Dr15Jones and @perrotta, thank you very much for the fix and the explanation! Can you please further clarify how I can distinguish when I am using a TFileService (so that only one module at a time can use the service) and when not and the difference with other cases (e.g. here [1] I see WatchRun and here [2] nothing)? Thanks a lot, --Elisa [1] https://github.com/cms-sw/cmssw/blob/ffe0bbf1e59edade2a85c26c30dd3d49c393fedb/L1Trigger/L1TMuonOverlapPhase1/plugins/L1MuonOverlapPhase1ParamsDBProducer.h#L16 [2] https://github.com/cms-sw/cmssw/blob/c33bb6c199ba2b7d6eb8ebfa97b091a32c2c6333/L1Trigger/GlobalCaloTrigger/test/L1GctTestAnalyzer.h#L34 -- Reply to this email directly or view it on GitHub: #36756 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

Dr15Jones · 2022-01-24T14:28:47Z

@elfontan here is the link to the Twiki about the different kinds of thread same module types: https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedFrameworkModuleTypes

In particular, the full interface for the one module can be found here
https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedFrameworkOneModuleInterface

The WatchRuns is described here
https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedFrameworkOneModuleInterface#edm_one_WatchRuns

It is only needed if the module in question needs to see beginRun or endRun transitions.

Dr15Jones · 2022-01-24T14:33:39Z

@perrotta [1] does have a beginRun method

cmssw/L1Trigger/L1TMuonOverlapPhase1/plugins/L1MuonOverlapPhase1ParamsDBProducer.cc

Line 16 in ffe0bbf

    
           void L1MuonOverlapPhase1ParamsDBProducer::beginRun(edm::Run const& run, edm::EventSetup const& iSetup) {

although it is so trivial as to have all its contents be easily moved to the analyze method instead and then the use of edm::one::WatchRuns could be removed.

Dr15Jones · 2022-01-24T14:36:03Z

@elfontan the only way I know it find TFileService is to check the module itself and any helper classes used by the module and see if they use the TFileService. Luckily, having helpers use the TFileService is not all that common so just looking at the body of the module often shows if it is used or not.

elfontan · 2022-01-24T16:06:12Z

@Dr15Jones many many thanks for your help and your comments!

elfontan added 3 commits January 17, 2022 15:41

fix L1 producers to use ESGetToken

5e3dd21

coherent name for the menu token with the one used in L1TExtCondProdu…

9d2afae

…cer.cc

Updated README including references to produce L1 ntuples

a1f54ed

cmsbuild added this to the CMSSW_12_3_X milestone Jan 20, 2022

cmsbuild added code-checks-pending l1-pending orp-pending pending-signatures tests-pending labels Jan 20, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jan 20, 2022

After run scram build code-checks because of the failure of the autom…

25fd42a

…atic tests

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jan 20, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 20, 2022

cmsbuild added tests-started and removed tests-pending labels Jan 20, 2022

perrotta reviewed Jan 20, 2022

View reviewed changes

cmsbuild added tests-rejected and removed tests-started labels Jan 20, 2022

Fix for legacy modules and const parameters (initialized in the class…

2a3b82c

… initializer list)

cmsbuild added code-checks-pending and removed tests-rejected code-checks-approved labels Jan 20, 2022

cmsbuild added the tests-rejected label Jan 21, 2022

cmsbuild added fully-signed l1-approved and removed l1-pending pending-signatures labels Jan 21, 2022

cmsbuild added orp-approved and removed orp-pending labels Jan 21, 2022

cmsbuild merged commit 131e8b4 into cms-sw:master Jan 21, 2022

Dr15Jones reviewed Jan 21, 2022

View reviewed changes

This was referenced Jan 21, 2022

move pixelTopology.h from Geometry/TrackerGeometryBuilder to Geometry/CommonTopologies #36773

Merged

[CXXMODULE] Updated root to tip of branch master cms-sw/cmsdist#7557

Merged

Update Geant4 to 10.7ref09 cms-sw/cmsdist#7436

Merged

perrotta mentioned this pull request Jan 22, 2022

Add forgotten SharedResources for TFileService in L1Trigger/L1TNtuples #36777

Merged

cmsbuild mentioned this pull request Jan 22, 2022

mkFit external integration #36546

Merged

epalencia mentioned this pull request Feb 7, 2022

Fix of the L1CaloTowerTreeProducer and L1uGTTreeProducer... (backport from master) cms-l1t-offline/cmssw#980

Merged

elfontan deleted the EF_L1Producers_UseESGetToken branch January 19, 2023 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix of the L1CaloTowerTreeProducer and L1uGTTreeProducer to avoid an exception MustUseESGetToken #36756

Fix of the L1CaloTowerTreeProducer and L1uGTTreeProducer to avoid an exception MustUseESGetToken #36756

elfontan commented Jan 20, 2022 •

edited

cmsbuild commented Jan 20, 2022

cmsbuild commented Jan 20, 2022

cmsbuild commented Jan 20, 2022

cecilecaillol commented Jan 20, 2022

perrotta left a comment

perrotta Jan 20, 2022

perrotta Jan 20, 2022

cmsbuild commented Jan 20, 2022

elfontan commented Jan 21, 2022

cecilecaillol commented Jan 21, 2022

perrotta commented Jan 21, 2022 •

edited

cecilecaillol commented Jan 21, 2022

cmsbuild commented Jan 21, 2022

perrotta commented Jan 21, 2022

perrotta commented Jan 21, 2022

Dr15Jones Jan 21, 2022

Dr15Jones Jan 21, 2022

perrotta commented Jan 22, 2022

elfontan commented Jan 23, 2022

perrotta commented Jan 23, 2022 via email

Dr15Jones commented Jan 24, 2022

Dr15Jones commented Jan 24, 2022

Dr15Jones commented Jan 24, 2022

elfontan commented Jan 24, 2022

Fix of the L1CaloTowerTreeProducer and L1uGTTreeProducer to avoid an exception MustUseESGetToken #36756

Fix of the L1CaloTowerTreeProducer and L1uGTTreeProducer to avoid an exception MustUseESGetToken #36756

Conversation

elfontan commented Jan 20, 2022 • edited

PR description:

PR validation:

cmsbuild commented Jan 20, 2022

cmsbuild commented Jan 20, 2022

cmsbuild commented Jan 20, 2022

cecilecaillol commented Jan 20, 2022

perrotta left a comment

Choose a reason for hiding this comment

perrotta Jan 20, 2022

Choose a reason for hiding this comment

perrotta Jan 20, 2022

Choose a reason for hiding this comment

cmsbuild commented Jan 20, 2022

RelVals-INPUT

Comparison Summary

elfontan commented Jan 21, 2022

cecilecaillol commented Jan 21, 2022

perrotta commented Jan 21, 2022 • edited

cecilecaillol commented Jan 21, 2022

cmsbuild commented Jan 21, 2022

perrotta commented Jan 21, 2022

perrotta commented Jan 21, 2022

Dr15Jones Jan 21, 2022

Choose a reason for hiding this comment

Dr15Jones Jan 21, 2022

Choose a reason for hiding this comment

perrotta commented Jan 22, 2022

elfontan commented Jan 23, 2022

perrotta commented Jan 23, 2022 via email

Dr15Jones commented Jan 24, 2022

Dr15Jones commented Jan 24, 2022

Dr15Jones commented Jan 24, 2022

elfontan commented Jan 24, 2022

elfontan commented Jan 20, 2022 •

edited

perrotta commented Jan 21, 2022 •

edited