LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows (back port) #35341

bainbrid · 2021-09-20T11:03:30Z

PR description:

UL mini v2 workflows for FastSim are broken due to an exception thrown by the LowPtGsfElectronIDProducer when attempting to access an edm::Ref to the missing generalTracksBeforeMixing collection in AOD.
All details can be found in this issue: Mini-v2 can not be produced in FastSim UL scenario #34774.
For re-miniaod (and re-nanoaod) workflows, this PR also provides an acceptable workaround that allows the ID producer to evaluate the BDT model without throwing an exception, while maintaining the expected physics performance. This is achieved by extracting features from the KF track used in the ElectronSeed step (obtained from the available generalTracks collection in AOD), instead of from the KF track embedded in the GsfElectronCore object (which is chosen based on number of shared hits with the GsfTrack and obtained from the missing generalTracksBeforeMixing collection in AOD).
All development was performed in the 10_6_X cycle (see the compare here), tested with the example recipes provided in the issue #34774, and then ported to master (this PR).
Also tested is the behaviour for the nano v9 workflow, which is similarly affected.
This work was presented to RECO/AT here: https://indico.cern.ch/event/1071803/. The final slide compares physics performance b/w the 10_6_X and 12_1_X cycles (essentially identical).
Physics and computing performances are ~unaffected.

PR validation:

This PR was validated using cmsDriver commands that performed the GEN->AOD and mini v2 steps, based on those provided here, for tests with the 10_6_X cycle, and modified versions for tests with the 12_1_X release (this PR).

if this PR is a backport please specify the original PR and why you need to backport that PR:

This PR is a back port of #35181.

@slava77 @jordan-martins @crovelli

…ni v2, issue cms-sw#34774

cmsbuild · 2021-09-20T11:03:55Z

A new Pull Request was created by @bainbrid for CMSSW_10_6_X.

It involves the following packages:

RecoEgamma/EgammaElectronProducers (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @jainshilpi, @rovere, @lgray, @sobhatta, @lecriste, @afiqaize, @wrtabb, @varuns23, @ram1123 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

slava77 · 2021-09-20T12:34:49Z

@cmsbuild please test

cmsbuild · 2021-09-20T15:40:28Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-449f17/18757/summary.html
COMMIT: 6437a79
CMSSW: CMSSW_10_6_X_2021-09-19-0000/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35341/18757/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: 119 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 3215686
DQMHistoTests: Total failures: 29
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3215323
DQMHistoTests: Total skipped: 334
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 143 log files, 29 edm output root files, 35 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-20T22:10:26Z

Reco comparison results: 119 differences found in the comparisons

I was expecting that there is no visible difference in 10_6_X in the workflows tested by the bot.

slava77 · 2021-09-20T22:11:50Z

RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronID_cff.py

@@ -28,3 +28,9 @@
    lowPtGsfElectronID,
    ModelWeights = ["RecoEgamma/ElectronIdentification/data/LowPtElectrons/LowPtElectrons_ID_2021May17.root"],
 )
+
+from Configuration.Eras.Modifier_fastSim_cff import fastSim
+fastSim.toModify(


I think that here we should be coupled with run2_miniAOD_UL

Yes. Also coupled with run2_nanoAOD_106Xv2, as done here

cmsbuild · 2021-09-21T07:44:41Z

Pull request #35341 was updated. @jpata, @cmsbuild, @slava77 can you please check and sign again.

slava77 · 2021-09-21T12:16:26Z

@cmsbuild please test

cmsbuild · 2021-09-21T16:13:38Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-449f17/18787/summary.html
COMMIT: 4f8e226
CMSSW: CMSSW_10_6_X_2021-09-19-0000/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35341/18787/install.sh to create a dev area with all the needed externals and cmssw changes.

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: 1365 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 3215686
DQMHistoTests: Total failures: 2069
DQMHistoTests: Total nulls: 22
DQMHistoTests: Total successes: 3213261
DQMHistoTests: Total skipped: 334
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -56.705 KiB( 34 files compared)
DQMHistoSizes: changed ( 140.53 ): -44.531 KiB Hcal/DigiRunHarvesting
DQMHistoSizes: changed ( 140.53 ): -10.938 KiB Info/EventInfo
DQMHistoSizes: changed ( 140.53 ): -1.172 KiB RPC/DCSInfo
DQMHistoSizes: changed ( 140.53 ): -0.064 KiB SiStrip/MechanicalView
Checked 143 log files, 29 edm output root files, 35 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-21T16:41:15Z

Reco comparison results: 1365 differences found in the comparisons

ignoring the glitch in 140.53 which accounts for 1240, the remaining 125 differences are still not expected , in particular the UL reminiAOD wf 136.88811 with 103

cmsbuild · 2021-09-22T16:03:52Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-449f17/18833/summary.html
COMMIT: 0052747
CMSSW: CMSSW_10_6_X_2021-09-19-0000/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35341/18833/install.sh to create a dev area with all the needed externals and cmssw changes.

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: 1246 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 3215686
DQMHistoTests: Total failures: 2042
DQMHistoTests: Total nulls: 22
DQMHistoTests: Total successes: 3213288
DQMHistoTests: Total skipped: 334
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -56.705 KiB( 34 files compared)
DQMHistoSizes: changed ( 140.53 ): -44.531 KiB Hcal/DigiRunHarvesting
DQMHistoSizes: changed ( 140.53 ): -10.938 KiB Info/EventInfo
DQMHistoSizes: changed ( 140.53 ): -1.172 KiB RPC/DCSInfo
DQMHistoSizes: changed ( 140.53 ): -0.064 KiB SiStrip/MechanicalView
Checked 143 log files, 29 edm output root files, 35 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-22T17:42:11Z

Reco comparison results: 1246 differences found in the comparisons

OK now; these are all false-positives (from 140.53, mainly, which again glitched in DAS)

slava77 · 2021-09-23T01:14:13Z

RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronIDProducer.cc

+      // Extract Track
+      const reco::Track* trk = nullptr;
+      if (useGsfToTrack_) {
+        trk = (*gsf2trk)[gsf].get();


the master version has more checks before doing .get, in particular isAvailable is present.
@makortel does the Ref::get throw if the product is not available or will it return a nullptr?

It throws an exception if the product is not available.

@bainbrid
please update this with isAvailable to match the master version so that we don't potentially get more crashes here than would be expected in 12X.

Done. I add the check within here and I do not (on purpose touch the lines here)

cmsbuild · 2021-09-23T14:44:23Z

Pull request #35341 was updated. @jpata, @cmsbuild, @slava77 can you please check and sign again.

slava77 · 2021-09-23T15:30:48Z

@cmsbuild please test

cmsbuild · 2021-09-23T19:01:48Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-449f17/18864/summary.html
COMMIT: 0f64359
CMSSW: CMSSW_10_6_X_2021-09-19-0000/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35341/18864/install.sh to create a dev area with all the needed externals and cmssw changes.

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: 1242 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 3215686
DQMHistoTests: Total failures: 2041
DQMHistoTests: Total nulls: 22
DQMHistoTests: Total successes: 3213289
DQMHistoTests: Total skipped: 334
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -56.705 KiB( 34 files compared)
DQMHistoSizes: changed ( 140.53 ): -44.531 KiB Hcal/DigiRunHarvesting
DQMHistoSizes: changed ( 140.53 ): -10.938 KiB Info/EventInfo
DQMHistoSizes: changed ( 140.53 ): -1.172 KiB RPC/DCSInfo
DQMHistoSizes: changed ( 140.53 ): -0.064 KiB SiStrip/MechanicalView
Checked 143 log files, 29 edm output root files, 35 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-23T20:24:49Z

+reconstruction

for #35341 0f64359

code changes are consistent with LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows #35181 (merged a week ago)
jenkins tests pass and comparisons with the baseline show no relevant differences
- the reported differences are false-positives; primarily from 140.53 which had different files in input between the baseline and this PR tests
I x-checked locally that the fastsim miniAOD with UL setup runs OK

cmsbuild · 2021-09-23T20:25:15Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_10_6_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_1_X is complete. 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)

slava77 · 2021-09-23T20:26:12Z

@qliphy @perrotta
after this PR is merged please consider to build a new 10_6_X release at your earliest so that the fastsim campaigns with mini-v2 can (re)start.
Thank you.

qliphy · 2021-09-24T00:44:48Z

@qliphy @perrotta
after this PR is merged please consider to build a new 10_6_X release at your earliest so that the fastsim campaigns with mini-v2 can (re)start.
Thank you.

@slava77 Sure we will.

perrotta · 2021-09-24T09:36:54Z

+1

The new features that allow the fix for fastsim are correctly included steered by a era modifier: no changes expected for the configurations of the already started productions

sbein · 2021-10-08T12:59:13Z

+1

bainbrid added 4 commits September 20, 2021 12:29

ID producer now uses seed track instead of closest, for UL FastSim mi…

bdfb132

…ni v2, issue cms-sw#34774

bug fix to identify correct packed cand

9862798

slava review (back port of dd17b74 and 0212d1d

51145db

scram build code-format

6437a79

cmsbuild added this to the CMSSW_10_6_X milestone Sep 20, 2021

cmsbuild added orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 20, 2021

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

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

slava77 reviewed Sep 20, 2021

View reviewed changes

use seed tracks for (fastSim AND (re-mini OR re-nano))

4f8e226

cmsbuild removed the tests-approved label Sep 21, 2021

cmsbuild added the tests-pending label Sep 21, 2021

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

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

bug fix: remove continue if null Track ptr

0052747

cmsbuild added the tests-started label Sep 22, 2021

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

slava77 reviewed Sep 23, 2021

View reviewed changes

slava review: add isAvailable check

0f64359

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

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

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

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Sep 23, 2021

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

cmsbuild merged commit 39546b2 into cms-sw:CMSSW_10_6_X Sep 24, 2021

slava77 mentioned this pull request Oct 7, 2021

Mini-v2 can not be produced in FastSim UL scenario #34774

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows (back port) #35341

LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows (back port) #35341

bainbrid commented Sep 20, 2021

cmsbuild commented Sep 20, 2021

slava77 commented Sep 20, 2021

cmsbuild commented Sep 20, 2021

slava77 commented Sep 20, 2021

slava77 Sep 20, 2021

bainbrid Sep 21, 2021

cmsbuild commented Sep 21, 2021

slava77 commented Sep 21, 2021

cmsbuild commented Sep 21, 2021

slava77 commented Sep 21, 2021

cmsbuild commented Sep 22, 2021

slava77 commented Sep 22, 2021

slava77 Sep 23, 2021

makortel Sep 23, 2021

slava77 Sep 23, 2021

bainbrid Sep 23, 2021

cmsbuild commented Sep 23, 2021

slava77 commented Sep 23, 2021

cmsbuild commented Sep 23, 2021

slava77 commented Sep 23, 2021

cmsbuild commented Sep 23, 2021

slava77 commented Sep 23, 2021

qliphy commented Sep 24, 2021

perrotta commented Sep 24, 2021

sbein commented Oct 8, 2021

LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows (back port) #35341

LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows (back port) #35341

Conversation

bainbrid commented Sep 20, 2021

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Sep 20, 2021

slava77 commented Sep 20, 2021

cmsbuild commented Sep 20, 2021

Comparison Summary

slava77 commented Sep 20, 2021

slava77 Sep 20, 2021

Choose a reason for hiding this comment

bainbrid Sep 21, 2021

Choose a reason for hiding this comment

cmsbuild commented Sep 21, 2021

slava77 commented Sep 21, 2021

cmsbuild commented Sep 21, 2021

Comparison Summary

slava77 commented Sep 21, 2021

cmsbuild commented Sep 22, 2021

Comparison Summary

slava77 commented Sep 22, 2021

slava77 Sep 23, 2021

Choose a reason for hiding this comment

makortel Sep 23, 2021

Choose a reason for hiding this comment

slava77 Sep 23, 2021

Choose a reason for hiding this comment

bainbrid Sep 23, 2021

Choose a reason for hiding this comment

cmsbuild commented Sep 23, 2021

slava77 commented Sep 23, 2021

cmsbuild commented Sep 23, 2021

Comparison Summary

slava77 commented Sep 23, 2021

cmsbuild commented Sep 23, 2021

slava77 commented Sep 23, 2021

qliphy commented Sep 24, 2021

perrotta commented Sep 24, 2021

sbein commented Oct 8, 2021