Skip to content
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

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

Merged
merged 2 commits into from Sep 23, 2021

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Sep 14, 2021

PR description:

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

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 makortel/framework#251

PR validation:

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

@cmsbuild
Copy link
Contributor

+code-checks

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

@cmsbuild
Copy link
Contributor

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
Copy link
Contributor Author

@cmsbuild, please test

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

-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

----- 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 -------------------------------------------------
----- 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 -------------------------------------------------
----- 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
Copy link
Contributor Author

@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
Copy link
Contributor

+code-checks

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

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

+code-checks

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

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

+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
Copy link
Contributor

slava77 commented Sep 22, 2021

+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

@tvami
Copy link
Contributor

tvami commented Sep 22, 2021

+alca

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

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor

I see. All clear now. Thanks, Matti.

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit ad02572 into cms-sw:master Sep 23, 2021
@makortel makortel deleted the esconsumesMultipleScattering branch September 23, 2021 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework MultipleScatteringParametrisation to esConsumes
7 participants