Move MSLayerKeeper* objects from thread-local into EventSetup #35269

makortel · 2021-09-14T15:53:51Z

PR description:

This PR is part of #31061 and focuses on the MultipleScatteringParametrisation helper. That class relied an old hack to cache EventSetup data

cmssw/RecoTracker/TkMSParametrization/src/MultipleScatteringParametrisation.cc

Lines 45 to 49 in 919a747

    
           thread_local Keepers keepers; 
        
           //NOTE: This is being used to globally cache information from the EventSetup 
        
           // this should not be done so we need this code changed. 
        
           //NOTE; the thread_local only works in this case because MultipleScateringParametrisation 
        
           // instances are only ever created on the stack and not the heap.

that this PR suggests to finally fix properly by moving the MSLayersKeeperX0DetLayer, MSLayersKeeperX0AtEta, and MSLayersKeeperX0Averaged from thread-locals into EventSetup itself. In the EventSetup they are abstracted into MultipleScatteringParametrisationMaker class that is then used to make MultipleScatteringParametrisation objects. The corresponding ESProducer is added into the customizeHLTforCMSSW.py and in various cff fragments within reconstruction configuration (I can't exclude missing some places).

This PR also completes the esConsumes migration for touched helper classes where that was feasible.

As in #35081, despite of being part of #31061 this PR adds some new calls to the deprecated EventSetupRecord::get() function without esConsumes(). All these cases have some relation to TrackingRegion classes, and will be addressed in a subsequent PR (I have the work done already, but I though they would be easier to review separately).

Resolves cms-sw/framework-team#251

PR validation:

Tested on earlier IB (on top of #35081) that limited matrix runs. After rebase checked that the code compiles.

cmsbuild · 2021-09-14T16:00:43Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35269/25261

This PR adds an extra 104KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/Configuration/python/customizeHLTforCMSSW.py modified in PR(s): Add support for the gpu modifier at HLT #34978, Add GEM unpacker in L1Repack FullMC and temporary load of GEM geometry to hltGetConfiguration #34788
- File RecoTracker/IterativeTracking/python/MixedTripletStep_cff.py modified in PR(s): Migrate module config in RecoTracker{IterativeTracking} to use default cfipython v3 #35194

cmsbuild · 2021-09-14T16:01:09Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

HLTrigger/Configuration (hlt)
RecoHI/HiTracking (reconstruction)
RecoPixelVertexing/PixelLowPtUtilities (reconstruction)
RecoPixelVertexing/PixelTrackFitting (reconstruction)
RecoPixelVertexing/PixelTriplets (reconstruction)
RecoTracker/IterativeTracking (reconstruction)
RecoTracker/TkHitPairs (reconstruction)
RecoTracker/TkMSParametrization (reconstruction)
RecoTracker/TkSeedGenerator (reconstruction)
RecoTracker/TkTrackingRegions (reconstruction)

@Martin-Grunewald, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mtosi, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @jazzitup, @VinInn, @Martin-Grunewald, @yenjie, @ebrondol, @rovere, @kurtejung, @gpetruc, @mmusich, @mandrenguyen, @dgulhan, @yetkinyilmaz 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

makortel · 2021-09-14T16:13:27Z

@cmsbuild, please test

makortel · 2021-09-14T16:15:52Z

RecoTracker/TkMSParametrization/plugins/MultipleScatteringParametrisationMakerESProducer.cc

+  // Using NavigationSchoolRecord here sounds a bit confusing, that
+  // that depends only on TrackerRecoGeometryRecord and
+  // IdealMagneticFieldRecord
+  std::unique_ptr<MultipleScatteringParametrisationMaker> produce(const NavigationSchoolRecord& iRecord);


Note that I blatantly reused the NavigationSchoolRecord for this ESProduct, as that record depends exactly on the two records (TrackerRecoGeometryRecord and IdealMagneticFieldRecord) needed for the MultipleScatteringParametrisation). If that is considered too confusing, I can create a new record (or put it to some other existing record like CkfComponentsRecord, but those may have smaller IOVs than the intersection of TrackerRecoGeometryRecord and IdealMagneticFieldRecord).

cmsbuild · 2021-09-14T19:35:24Z

-1

Failed Tests: AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c6a8f8/18601/summary.html
COMMIT: fa1e4e0
CMSSW: CMSSW_12_1_X_2021-09-13-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35269/18601/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 10 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c6a8f8/18601/llvm-analysis/esrget-sa.txt for details.

AddOn Tests

hlt_mc_Fake

----- Begin Fatal Exception 14-Sep-2021 19:02:39 CEST-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="MultipleScatteringParametrisationMaker" label=""
 from record NavigationSchoolRecord. The two providers are 
1) type="MultipleScatteringParametrisationMakerESProducer" label="hltMultipleScatteringParametrisationMakerESProducer"
2) type="MultipleScatteringParametrisationMakerESProducer" label="multipleScatteringParametrisationMakerESProducer"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

hlt_mc_Fake1

----- Begin Fatal Exception 14-Sep-2021 19:02:45 CEST-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="MultipleScatteringParametrisationMaker" label=""
 from record NavigationSchoolRecord. The two providers are 
1) type="MultipleScatteringParametrisationMakerESProducer" label="hltMultipleScatteringParametrisationMakerESProducer"
2) type="MultipleScatteringParametrisationMakerESProducer" label="multipleScatteringParametrisationMakerESProducer"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

hlt_mc_Fake2

----- Begin Fatal Exception 14-Sep-2021 19:02:51 CEST-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="MultipleScatteringParametrisationMaker" label=""
 from record NavigationSchoolRecord. The two providers are 
1) type="MultipleScatteringParametrisationMakerESProducer" label="hltMultipleScatteringParametrisationMakerESProducer"
2) type="MultipleScatteringParametrisationMakerESProducer" label="multipleScatteringParametrisationMakerESProducer"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

Expand to see more addon errors ...

Comparison Summary

The workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

Summary:

No significant changes to the logs found
Reco comparison results: 1299 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3001001
DQMHistoTests: Total failures: 3691
DQMHistoTests: Total nulls: 19
DQMHistoTests: Total successes: 2997269
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -45.703 KiB( 38 files compared)
DQMHistoSizes: changed ( 140.53 ): -44.531 KiB Hcal/DigiRunHarvesting
DQMHistoSizes: changed ( 140.53 ): -1.172 KiB RPC/DCSInfo
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

makortel · 2021-09-14T20:19:53Z

@Martin-Grunewald I see the addOn tests that run HLT and RECO in the same job. Should I then use the same label for the ESProducer in both HLT and RECO so those jobs would have only one instance of it? I.e. in customizeHLTforCMSSW.py just do process.load('RecoTracker.TkMSParametrization.multipleScatteringParametrisationMakerESProducer_cfi')?

cmsbuild · 2021-09-14T20:25:52Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35269/25271

This PR adds an extra 104KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/Configuration/python/customizeHLTforCMSSW.py modified in PR(s): Add support for the gpu modifier at HLT #34978, Add GEM unpacker in L1Repack FullMC and temporary load of GEM geometry to hltGetConfiguration #34788
- File RecoTracker/IterativeTracking/python/MixedTripletStep_cff.py modified in PR(s): Migrate module config in RecoTracker{IterativeTracking} to use default cfipython v3 #35194

cmsbuild · 2021-09-14T20:26:15Z

Pull request #35269 was updated. @Martin-Grunewald, @jpata, @cmsbuild, @slava77 can you please check and sign again.

cmsbuild · 2021-09-22T03:38:57Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35269/25441

This PR adds an extra 68KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/Configuration/python/customizeHLTforCMSSW.py modified in PR(s): [EGM] Eta-Extended-Electrons (EEE) #35309, Update GPU workflows #35331

cmsbuild · 2021-09-22T03:39:19Z

Pull request #35269 was updated. @malbouis, @yuanchao, @Martin-Grunewald, @missirol, @slava77, @jpata, @francescobrivio, @tvami can you please check and sign again.

cmsbuild · 2021-09-22T08:19:23Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c6a8f8/18811/summary.html
COMMIT: 7cf94fc
CMSSW: CMSSW_12_1_X_2021-09-21-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35269/18811/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 10 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c6a8f8/18811/llvm-analysis/esrget-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: 40
DQMHistoTests: Total histograms compared: 3211080
DQMHistoTests: Total failures: 20
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3211038
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 39 files compared)
Checked 169 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-22T15:30:53Z

+reconstruction

for #35269 7cf94fc

code changes are somewhat technical in line with the PR description and the follow up review
- a few newly added es.get cases will be addressed in a followup PR
jenkins tests pass and comparisons with the baseline show no particularly relevant differences
- as noted earlier Move MSLayerKeeper* objects from thread-local into EventSetup #35269 (comment) (and repeated in the following tests) there is one more seed in the detachedTripletStep, but it does not become a track, so the persisted data reco comparisons show no differences

tvami · 2021-09-22T15:37:33Z

+alca

the only AlCa related change is the include of the multiple scattering parametrization in the tracker dependent cff

Martin-Grunewald · 2021-09-22T17:04:21Z

+1

cmsbuild · 2021-09-22T17:04:41Z

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)

missirol · 2021-09-22T17:14:19Z

@makortel I was having a look for HLT, and wanted to ask a clarification (maybe I'm missing something obvious).

IIuc, in #35269 (comment) you mention that hlt_mc_GRun (from the addOnTests) needs the HLT customisation function to run, but I understood from #35269 (comment) that in that wf (and others) HLT and RECO run in the same job, and the new ES module would already be provided by RECO. What am I missing?

Maybe the HLT customisation is really needed, but not for that specific addOnTest?

makortel · 2021-09-22T19:10:46Z

@missirol Looking closer, the step in hlt_mc_GRun that failed in #35269 (comment) was not the HLT+RECO job, but the following step that runs just the HLT menu. I tested with the current version of the PR commenting out the customiseFor35269(), and it continues to fail in similar way (while the RECO+HLT job runs fine because the ESProducer is provided via RECO, well now via FrontierConditions_GlobalTag_cff).

A consequence of the HLT+RECO jobs in the addOnTests that I referred to in #35269 (comment) was the use of process.load() wrt. creating a second ESProducer with a different label (that would be technically possible, but would be more cumbersome to do).

missirol · 2021-09-22T19:39:21Z

I see. All clear now. Thanks, Matti.

perrotta · 2021-09-23T17:20:05Z

+1

cmsbuild added this to the CMSSW_12_1_X milestone Sep 14, 2021

makortel mentioned this pull request Sep 14, 2021

Rework MultipleScatteringParametrisation to esConsumes cms-sw/framework-team#251

Closed

cmsbuild added code-checks-pending hlt-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 14, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 14, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 14, 2021

makortel commented Sep 14, 2021

View reviewed changes

makortel mentioned this pull request Sep 14, 2021

Consumes migration of Alignment/ReferenceTrajectories #35259

Merged

cmsbuild added tests-rejected and removed tests-started labels Sep 14, 2021

makortel force-pushed the esconsumesMultipleScattering branch from fa1e4e0 to 3f99a0d Compare September 14, 2021 20:19

cmsbuild added code-checks-pending tests-pending and removed tests-rejected code-checks-approved labels Sep 14, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 14, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 22, 2021

cmsbuild added tests-approved and removed tests-started labels Sep 22, 2021

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Sep 22, 2021

cmsbuild added alca-approved and removed alca-pending labels Sep 22, 2021

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Sep 22, 2021

cmsbuild mentioned this pull request Sep 23, 2021

Fixes for PPS geometry on Pixel sensors and materials #35346

Closed

cmsbuild added orp-approved and removed orp-pending labels Sep 23, 2021

cmsbuild merged commit ad02572 into cms-sw:master Sep 23, 2021

makortel deleted the esconsumesMultipleScattering branch September 23, 2021 18:54

makortel mentioned this pull request Sep 24, 2021

Migrate TrackingRegions to esConsumes #35393

Merged

cmsbuild mentioned this pull request Sep 24, 2021

fix offline BS swap for express Run3 #35373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move MSLayerKeeper* objects from thread-local into EventSetup #35269

Move MSLayerKeeper* objects from thread-local into EventSetup #35269

makortel commented Sep 14, 2021 •

edited

Loading

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

makortel commented Sep 14, 2021

makortel Sep 14, 2021

cmsbuild commented Sep 14, 2021

makortel commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 22, 2021

cmsbuild commented Sep 22, 2021

cmsbuild commented Sep 22, 2021

slava77 commented Sep 22, 2021 •

edited

Loading

tvami commented Sep 22, 2021

Martin-Grunewald commented Sep 22, 2021

cmsbuild commented Sep 22, 2021

missirol commented Sep 22, 2021

makortel commented Sep 22, 2021

missirol commented Sep 22, 2021

perrotta commented Sep 23, 2021

	thread_local Keepers keepers;
	//NOTE: This is being used to globally cache information from the EventSetup
	// this should not be done so we need this code changed.
	//NOTE; the thread_local only works in this case because MultipleScateringParametrisation
	// instances are only ever created on the stack and not the heap.

Move MSLayerKeeper* objects from thread-local into EventSetup #35269

Move MSLayerKeeper* objects from thread-local into EventSetup #35269

Conversation

makortel commented Sep 14, 2021 • edited Loading

PR description:

PR validation:

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

makortel commented Sep 14, 2021

makortel Sep 14, 2021

Choose a reason for hiding this comment

cmsbuild commented Sep 14, 2021

AddOn Tests

Comparison Summary

makortel commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 22, 2021

cmsbuild commented Sep 22, 2021

cmsbuild commented Sep 22, 2021

Comparison Summary

slava77 commented Sep 22, 2021 • edited Loading

tvami commented Sep 22, 2021

Martin-Grunewald commented Sep 22, 2021

cmsbuild commented Sep 22, 2021

missirol commented Sep 22, 2021

makortel commented Sep 22, 2021

missirol commented Sep 22, 2021

perrotta commented Sep 23, 2021

makortel commented Sep 14, 2021 •

edited

Loading

slava77 commented Sep 22, 2021 •

edited

Loading