Converted ObjectSelector system to use edm::stream::EDFilter<> #35575

Dr15Jones · 2021-10-07T16:15:49Z

PR description:

Looking at all uses of ObjectSelector showed they were all implemented in a thread-friendly manner
Created a ObjectSelectorLegacy class in case an analysis user has their own code which can't be immediately migrated to be thread-friendly.

NOTE: this includes the commits from #35562. It is OK if that PR is merged first (to get rid of the initial false positives) or this one is merged first.

PR validation:

Code compiles.

This is a step towards removing use of the deprecated edm::EDFilter - removed implicit use of edm::EDFilter from many template arguments - Changed NullPostProcessor to not need to know module type - Made GsfElectron modules always use consistent base class

- All non-templated classes inheriting from ObjectSelector were found to be thread-friendly - The templated classes were moved to ObjectSelectorLegacy for now

They are now identical so only need one

The only user of ObjectShallowCloneSelector can safely be used in a thread-friendly manner.

The only user of SingleObjectRefSelector is thread-friendly.

cmsbuild · 2021-10-07T16:23:28Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35575/25821

This PR adds an extra 40KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CommonTools/CandAlgos/interface/SingleObjectShallowCloneSelector.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/CandAlgos/plugins/AssociatedVariableMaxCutCandSelectorNew.cc modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/RecoAlgos/interface/GsfElectronSelector.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/RecoAlgos/plugins/EtaPtMinGsfElectronFullCloneSelector.cc modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/RecoAlgos/plugins/EtaPtMinPixelMatchGsfElectronFullCloneSelector.cc modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/NullPostProcessor.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/ObjectSelector.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/ObjectSelectorBase.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/ObjectSelectorStream.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/ObjectSelectorStreamProducer.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/SingleObjectSelector.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/StoreManagerTrait.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562

cmsbuild · 2021-10-07T16:23:49Z

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

It involves the following packages:

Alignment/CommonAlignmentProducer (alca)
Calibration/TkAlCaRecoProducers (alca)
CommonTools/CandAlgos (reconstruction)
CommonTools/ParticleFlow (reconstruction)
CommonTools/RecoAlgos (reconstruction)
CommonTools/UtilAlgos (reconstruction)
PhysicsTools/PatAlgos (reconstruction)
RecoHI/HiTracking (reconstruction)
RecoTauTag/RecoTau (reconstruction)
Validation/RecoHI (dqm)

@malbouis, @yuanchao, @pmandrik, @emanueleusai, @ahmad3213, @tvami, @cmsbuild, @jfernan2, @slava77, @jpata, @francescobrivio, @pbo0, @rvenditti can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @hatakeyamak, @emilbols, @tlampen, @mbluj, @demuller, @threus, @seemasharmafnal, @pakhotin, @mmarionncern, @ahinzmann, @jhgoh, @jdolen, @mandrenguyen, @yetkinyilmaz, @abbiendi, @kkrajczar, @cbernet, @tocheng, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @mmusich, @dgulhan, @clelange, @AlexDeMoor, @adewit, @swozniewski, @jazzitup, @JyothsnaKomaragiri, @yenjie, @RylanC24, @kurtejung, @gpetruc, @mariadalfonso, @andrzejnovak 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

Dr15Jones · 2021-10-07T16:24:24Z

please test

cmsbuild · 2021-10-07T20:05:35Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55d328/19478/summary.html
COMMIT: ab32884
CMSSW: CMSSW_12_1_X_2021-10-07-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35575/19478/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: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 3219394
DQMHistoTests: Total failures: 5
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3219366
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 39 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 169 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

tvami · 2021-10-07T23:54:56Z

@Dr15Jones there seems to be a coding rule violation: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55d328/19478/codeRules/cmsCodeRule1.txt

Dr15Jones · 2021-10-08T01:34:40Z

The code violation is a false positive. The using namespace happens internally to a member function. The true code violations is when the using namespace happens at global scope within a header as that pollutes the global namespace.

tvami · 2021-10-08T01:41:15Z

+alca

cmsbuild · 2021-10-13T15:05:10Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35575/25926

This PR adds an extra 88KB to repository
Found files with invalid states:
- CommonTools/UtilAlgos/interface/ObjectSelectorLegacy.h:
  - Added: 2b51130
  - Deleted: 40a49a8
There are other open Pull requests which might conflict with changes you have proposed:
- File CommonTools/CandAlgos/interface/SingleObjectShallowCloneSelector.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/CandAlgos/plugins/AssociatedVariableMaxCutCandSelectorNew.cc modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/RecoAlgos/interface/GsfElectronSelector.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/RecoAlgos/plugins/EtaPtMinGsfElectronFullCloneSelector.cc modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/RecoAlgos/plugins/EtaPtMinPixelMatchGsfElectronFullCloneSelector.cc modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/NullPostProcessor.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/ObjectSelector.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/ObjectSelectorBase.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/ObjectSelectorStream.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/ObjectSelectorStreamProducer.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/SingleObjectSelector.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562
- File CommonTools/UtilAlgos/interface/StoreManagerTrait.h modified in PR(s): Avoid deprecated false positive warnings for ObjectSelector variant #35562

cmsbuild · 2021-10-13T15:05:35Z

Pull request #35575 was updated. @malbouis, @yuanchao, @pmandrik, @emanueleusai, @ahmad3213, @tvami, @cmsbuild, @jfernan2, @slava77, @jpata, @francescobrivio, @pbo0, @rvenditti can you please check and sign again.

Dr15Jones · 2021-10-13T15:07:21Z

please test

Dr15Jones · 2021-10-13T15:07:53Z

@tvami @jfernan2 please resign this PR as the only change was to remove an unused header file.

tvami · 2021-10-13T15:08:31Z

+alca

jfernan2 · 2021-10-13T15:14:55Z

+1

cmsbuild · 2021-10-13T19:02:33Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55d328/19601/summary.html
COMMIT: 40a49a8
CMSSW: CMSSW_12_1_X_2021-10-13-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35575/19601/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: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 2796791
DQMHistoTests: Total failures: 5
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2796764
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-10-14T13:09:21Z

+reconstruction

for #35575 40a49a8

code changes are essentially technical, in line with the PR description and the follow up review
jenkins tests pass and comparisons with the baseline show no (relevant) differences

cmsbuild · 2021-10-14T13:09:44Z

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)

perrotta · 2021-10-14T14:07:02Z

+1

Technical
@Dr15Jones could you please update the PR description field and cancel the reference to the ObjectSelectorLegacy class (now removed from this PR)?

Dr15Jones added 8 commits October 6, 2021 15:54

Include correct header

bc97888

Removed unnecessary includes

11e4c4c

Removed unnecessary include

46be563

Moved ObjectSelector to use edm::stream::EDFilter<>

2b51130

- All non-templated classes inheriting from ObjectSelector were found to be thread-friendly - The templated classes were moved to ObjectSelectorLegacy for now

Moved ObjectSelectorStream to ObjectSelector

524fd9b

They are now identical so only need one

ObjectShallowCloneSelector uses ObjectSelector

8203dd1

The only user of ObjectShallowCloneSelector can safely be used in a thread-friendly manner.

Move SingleObjectRefSelector to ObjectSelector

ab32884

The only user of SingleObjectRefSelector is thread-friendly.

cmsbuild added this to the CMSSW_12_1_X milestone Oct 7, 2021

cmsbuild added alca-pending code-checks-pending dqm-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 7, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 7, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 7, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 7, 2021

cmsbuild removed the alca-pending label Oct 8, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 13, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 13, 2021

cmsbuild added alca-approved and removed alca-pending labels Oct 13, 2021

cmsbuild added dqm-approved and removed dqm-pending labels Oct 13, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 13, 2021

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Oct 14, 2021

cmsbuild added orp-approved and removed orp-pending labels Oct 14, 2021

cmsbuild merged commit e0a8074 into cms-sw:master Oct 14, 2021

This was referenced Oct 14, 2021

Add DQM Matrix to RelVals #35678

Merged

DAQ fix spurious luminosityBlockAuxiliary assertion in HLT (12_1_X) #35682

Merged

Dr15Jones deleted the updateObjectSelectorToStream branch October 20, 2021 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converted ObjectSelector system to use edm::stream::EDFilter<> #35575

Converted ObjectSelector system to use edm::stream::EDFilter<> #35575

Dr15Jones commented Oct 7, 2021

cmsbuild commented Oct 7, 2021

cmsbuild commented Oct 7, 2021

Dr15Jones commented Oct 7, 2021

cmsbuild commented Oct 7, 2021

tvami commented Oct 7, 2021

Dr15Jones commented Oct 8, 2021

tvami commented Oct 8, 2021

cmsbuild commented Oct 13, 2021

cmsbuild commented Oct 13, 2021

Dr15Jones commented Oct 13, 2021

Dr15Jones commented Oct 13, 2021

tvami commented Oct 13, 2021

jfernan2 commented Oct 13, 2021

cmsbuild commented Oct 13, 2021

slava77 commented Oct 14, 2021

cmsbuild commented Oct 14, 2021

perrotta commented Oct 14, 2021

Converted ObjectSelector system to use edm::stream::EDFilter<> #35575

Converted ObjectSelector system to use edm::stream::EDFilter<> #35575

Conversation

Dr15Jones commented Oct 7, 2021

PR description:

PR validation:

cmsbuild commented Oct 7, 2021

cmsbuild commented Oct 7, 2021

Dr15Jones commented Oct 7, 2021

cmsbuild commented Oct 7, 2021

Comparison Summary

tvami commented Oct 7, 2021

Dr15Jones commented Oct 8, 2021

tvami commented Oct 8, 2021

cmsbuild commented Oct 13, 2021

cmsbuild commented Oct 13, 2021

Dr15Jones commented Oct 13, 2021

Dr15Jones commented Oct 13, 2021

tvami commented Oct 13, 2021

jfernan2 commented Oct 13, 2021

cmsbuild commented Oct 13, 2021

Comparison Summary

slava77 commented Oct 14, 2021

cmsbuild commented Oct 14, 2021

perrotta commented Oct 14, 2021