Remove inappropriate use of provenance in EcalDeadCellTriggerPrimitiveFilter #24387

Dr15Jones · 2018-08-24T22:27:09Z

The code was calling edm::Event::getAllStableProvenance to try to determine if some data products were available in the Event. This did not help performance since the consumes calls would already prefetch all the data. To try to keep the same functionality, I switched to using callWhenNewProductsRegistered.

The logic which changed the movule's behavior for data older than CMSSW_4_2 was also removed.

It is best to avoid holding onto information from the Event.

Removed use of getAllStableProvenance. The consumes calls were already forcing the prefetching of the different data products. Switched to using callWhenNewProductsRegistered and only call 'consumes' if the alternate data product is actually available and the prefered product has not been seen.

cmsbuild · 2018-08-24T22:27:28Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-08-24T22:29:37Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24387/6160

cmsbuild · 2018-08-24T22:29:54Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

RecoMET/METFilters

@perrotta, @monttj, @cmsbuild, @slava77, @gpetruc, @arizzi can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @ahinzmann, @mmarionncern, @jdamgov, @jdolen, @nhanvtran, @gkasieczka, @clelange, @schoef, @mariadalfonso, @seemasharmafnal this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2018-08-24T22:31:24Z

please test

cmsbuild · 2018-08-24T22:31:40Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30050/console Started: 2018/08/25 01:27

cmsbuild · 2018-08-25T01:55:54Z

+1
Tested at: 19d098b
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24387/30050/summary.html

cmsbuild · 2018-08-25T01:55:57Z

Comparison job queued.

cmsbuild · 2018-08-25T03:28:01Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24387/30050/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 3 differences found in the comparisons
DQMHistoTests: Total files compared: 31
DQMHistoTests: Total histograms compared: 2987459
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2987267
DQMHistoTests: Total skipped: 190
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
Checked 129 log files, 14 edm output root files, 31 DQM output files

slava77 · 2018-08-27T17:57:09Z

RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc

-  void loadEcalRecHits(edm::Event& iEvent, const edm::EventSetup& iSetup);
+  void loadEcalDigis(edm::Event& iEvent,
+                     edm::Handle<EcalTrigPrimDigiCollection>& pTPDigis
+);


please close this on the previous line to preserve indentation

slava77 · 2018-08-27T18:20:38Z

NOTE: This functionality to get an alternative data product appears to only be run if the data file was generated in a CMSSW version before 4_2. Perhaps this entire code path could now be removed?

@Dr15Jones
I was not able to follow this comment.
IIUC, the comment is related to the following part of the code https://github.com/cms-sw/cmssw/pull/24387/files#diff-896c2d58ecf157fdf90ebb82ce236a3eR226

   if( !hastpDigiCollection_ && !hasReducedRecHits_ ){ useTPmethod_ = false; useHITmethod_ = false;  }
   else if( hastpDigiCollection_ ){ useTPmethod_ = true; useHITmethod_ = false; }
   else if( majorV >=5 || (majorV==4 && minorV >=2) ){ useTPmethod_ = false; useHITmethod_ = true; }
   else{ useTPmethod_ = false; useHITmethod_ = false;
// kind of fail here

Was your comment to remove the last else because logically this code will never get there assuming there are no more inputs ? Or was your comment to drop the logic of the version detection because it has no effect (assuming all inputs are already after version 4_2)?

Dr15Jones · 2018-08-28T14:29:57Z

@slava77

Was your comment to remove the last else because logically this code will never get there assuming there are no more inputs ? Or was your comment to drop the logic of the version detection because it has no effect (assuming all inputs are already after version 4_2)?

I was asking if we can drop the entire version detection because CMSSW_10_3 will never be used to run on reconstruction from before CMSSW_4_2.

cmsbuild · 2018-08-29T18:12:53Z

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

perrotta · 2018-08-30T05:16:13Z

please test

cmsbuild · 2018-08-30T05:16:31Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30139/console Started: 2018/08/30 07:17

cmsbuild · 2018-08-30T08:00:40Z

+1
Tested at: 53fbe20
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24387/30139/summary.html

cmsbuild · 2018-08-30T08:00:43Z

Comparison job queued.

cmsbuild · 2018-08-30T10:48:36Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24387/30139/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3143879
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3143681
DQMHistoTests: Total skipped: 197
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 133 log files, 14 edm output root files, 32 DQM output files

slava77 · 2018-08-30T21:15:00Z

+1

for #24387 53fbe20

somewhat technical change in the provenance access, in line with the PR description; drop the logic used with inputs created before CMSSW_4_2
jenkins tests pass

@Dr15Jones please update the PR description to clarify that the old version detection logic was removed.

Dr15Jones · 2018-08-30T21:27:42Z

@slava77 comment updated

kpedro88 · 2018-08-31T14:56:31Z

+1

kpedro88 · 2018-08-31T14:57:22Z

merge

Dr15Jones added 3 commits August 24, 2018 16:09

Do not use edm::Handle as member data

80bfa6a

It is best to avoid holding onto information from the Event.

Use edm::Event::emplace

19d098b

cmsbuild added this to the CMSSW_10_3_X milestone Aug 24, 2018

cmsbuild added analysis-pending code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 24, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 24, 2018

cmsbuild added tests-started and removed tests-pending labels Aug 24, 2018

cmsbuild added tests-approved and removed tests-started labels Aug 25, 2018

cmsbuild added comparison-available and removed comparison-pending labels Aug 25, 2018

slava77 reviewed Aug 27, 2018

View reviewed changes

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Aug 29, 2018

cmsbuild added comparison-pending tests-pending tests-started and removed comparison-notrun tests-rejected tests-pending labels Aug 30, 2018

cmsbuild added tests-approved and removed tests-started labels Aug 30, 2018

cmsbuild added comparison-available and removed comparison-pending labels Aug 30, 2018

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Aug 30, 2018

cmsbuild added orp-approved and removed orp-pending labels Aug 31, 2018

cmsbuild merged commit edb9f98 into cms-sw:master Aug 31, 2018

Dr15Jones deleted the fixEcalDeadCellTriggerPrimitiveFilter branch August 31, 2018 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove inappropriate use of provenance in EcalDeadCellTriggerPrimitiveFilter #24387

Remove inappropriate use of provenance in EcalDeadCellTriggerPrimitiveFilter #24387

Dr15Jones commented Aug 24, 2018 •

edited

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

Dr15Jones commented Aug 24, 2018

cmsbuild commented Aug 24, 2018 •

edited

cmsbuild commented Aug 25, 2018

cmsbuild commented Aug 25, 2018

cmsbuild commented Aug 25, 2018

slava77 Aug 27, 2018

slava77 commented Aug 27, 2018

Dr15Jones commented Aug 28, 2018

cmsbuild commented Aug 29, 2018

perrotta commented Aug 30, 2018

cmsbuild commented Aug 30, 2018 •

edited

cmsbuild commented Aug 30, 2018

cmsbuild commented Aug 30, 2018

cmsbuild commented Aug 30, 2018

slava77 commented Aug 30, 2018

Dr15Jones commented Aug 30, 2018

kpedro88 commented Aug 31, 2018

kpedro88 commented Aug 31, 2018

Remove inappropriate use of provenance in EcalDeadCellTriggerPrimitiveFilter #24387

Remove inappropriate use of provenance in EcalDeadCellTriggerPrimitiveFilter #24387

Conversation

Dr15Jones commented Aug 24, 2018 • edited

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

Dr15Jones commented Aug 24, 2018

cmsbuild commented Aug 24, 2018 • edited

cmsbuild commented Aug 25, 2018

cmsbuild commented Aug 25, 2018

cmsbuild commented Aug 25, 2018

slava77 Aug 27, 2018

Choose a reason for hiding this comment

slava77 commented Aug 27, 2018

Dr15Jones commented Aug 28, 2018

cmsbuild commented Aug 29, 2018

perrotta commented Aug 30, 2018

cmsbuild commented Aug 30, 2018 • edited

cmsbuild commented Aug 30, 2018

cmsbuild commented Aug 30, 2018

cmsbuild commented Aug 30, 2018

slava77 commented Aug 30, 2018

Dr15Jones commented Aug 30, 2018

kpedro88 commented Aug 31, 2018

kpedro88 commented Aug 31, 2018

Dr15Jones commented Aug 24, 2018 •

edited

cmsbuild commented Aug 24, 2018 •

edited

cmsbuild commented Aug 30, 2018 •

edited