Restructure VID Python/FWLite code and avoid generating dictionaries for `Reco/Identification` packages #35953

guitargeek · 2021-11-02T11:28:44Z

PR description:

I took issue reported in #35754 as an opportunity to simplify the VID code a bit on the Python side.

The key class for the Python/FWLite support of VID is the VersionedSelector<> in PhysicsTools/SeletorUtils. The dictionaries for the different template instances of this class were split over the Electron/Muon/Photon identification packages. Now, they are all in the PhysicsTools/SeletorUtils library.

Then, there were dictionaries in the 3 identification packages for almost trivial classes used in VIDSelectorBase.py. However, these classes are tiny template classes that are only used in said python scipt, so I don't think we need dictionaries for them. The classes are small enough such that including them via gInterpreter.Declare is very fast.

Finally, there was a dictionary produced for the ElectronMVAEstimatorRun2 class, which I suggest to also not do anymore. There is only one use case for this: the class is used in the ElectronIdentification/python/FWLite.py utility file. But again, declaring the ElectronMVAEstimatorRun2 is very quick (after avoiding some includes), so it's not necessary to generate dictionaries in CMSSW just for this one usecase (which is outdated anyway because FWLite usage is going down since NanoAOD I guess).

More information can also be found in the commit messages.

PR validation:

CMSSW compiles
test_eleid_fwlite.py runs
it is still possible to instantiate the Python classes VIDElectronSelector, VIDPhotonSelector, VIDMuonSelector

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

No backport intended.

cmsbuild · 2021-11-02T11:37:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35953/26361

This PR adds an extra 24KB to repository

cmsbuild · 2021-11-02T11:37:41Z

A new Pull Request was created by @guitargeek (Jonas Rembser) for master.

It involves the following packages:

RecoEgamma/EgammaTools (reconstruction)
RecoEgamma/ElectronIdentification (reconstruction)
RecoEgamma/PhotonIdentification (reconstruction)

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

cmsbuild · 2021-11-02T13:05:20Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35953/26362

This PR adds an extra 20KB to repository

cmsbuild · 2021-11-02T13:05:42Z

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

cmsbuild · 2021-11-02T15:32:31Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35953/26367

This PR adds an extra 24KB to repository

cmsbuild · 2021-11-02T15:32:51Z

Pull request #35953 was updated. @smuzaffar, @Dr15Jones, @makortel, @cmsbuild, @slava77, @jpata can you please check and sign again.

guitargeek · 2021-11-02T15:38:28Z

Sorry, I should have made the PR a draft before. I'm done with the force pushes now

makortel · 2021-11-02T16:04:09Z

PhysicsTools/SelectorUtils/src/classes.h

 namespace PhysicsTools_SelectorUtils {
  struct dictionary {
    pat::strbitset strbitset;
    edm::Wrapper<pat::strbitset> wstrbitset;
    std::vector<pat::strbitset> vstrbitset;
    edm::Wrapper<std::vector<pat::strbitset> > wvstrbitset;
+
+    VersionedSelector<edm::Ptr<reco::Photon> > vPhotonSelector;
+    VersionedSelector<edm::Ptr<pat::Photon> > vPatPhotonSelector;
+    VersionedSelector<edm::Ptr<reco::Muon> > vMuonSelector;
+    VersionedSelector<edm::Ptr<reco::GsfElectron> > vGsfElectronSelector;
+    VersionedSelector<edm::Ptr<pat::Electron> > vPatElectronSelector;
  };


This struct is not needed anymore (for a long time).

Suggested change

namespace PhysicsTools_SelectorUtils {

struct dictionary {

pat::strbitset strbitset;

edm::Wrapper<pat::strbitset> wstrbitset;

std::vector<pat::strbitset> vstrbitset;

edm::Wrapper<std::vector<pat::strbitset> > wvstrbitset;

VersionedSelector<edm::Ptr<reco::Photon> > vPhotonSelector;

VersionedSelector<edm::Ptr<pat::Photon> > vPatPhotonSelector;

VersionedSelector<edm::Ptr<reco::Muon> > vMuonSelector;

VersionedSelector<edm::Ptr<reco::GsfElectron> > vGsfElectronSelector;

VersionedSelector<edm::Ptr<pat::Electron> > vPatElectronSelector;

};

Okay, thanks for the suggestion!

makortel · 2021-11-02T16:05:54Z

PhysicsTools/SelectorUtils/src/classes_def.xml

@@ -9,4 +9,5 @@
  <class name="MuonVPlusJetsIDSelectionFunctor"/>
  <class name="PVSelector"/>
  <class name="RunLumiSelector"/>
+  <class pattern="VersionedSelector<*>"/>


The classes_def.xml should list all types explicitly (we had not so long ago a campaign to remove all patterns).

Alright! I fixed this

makortel · 2021-11-02T16:06:58Z

RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Base.h

+namespace edmplugin {
+  template <class T>
+  class PluginFactory;
+}
 typedef edmplugin::PluginFactory<AnyMVAEstimatorRun2Base*(const edm::ParameterSet&)> AnyMVAEstimatorRun2Factory;


To me this sounds more of a reason to move the plugin factory definition out from this file.

Well, I don't see why that's better, but it's also fine for me :) I have now put it in a separate file called AnyMVAEstimatorRun2Factory.h. Are you fine with that?

Well, I don't see why that's better, but it's also fine for me :)

The users of AnyMVAEstimatorRun2Base do not need to know about the factory (only the code creating the object needs the factory). This separation is actually a common pattern in CMSSW (I'd even say vast majority of plugin factories are defined that way, but I didn't do a thorough analysis). The split also avoids the need to explicitly forward-declare types from the framework (code rule 4.7).

I have now put it in a separate file called AnyMVAEstimatorRun2Factory.h. Are you fine with that?

That's good, thanks!

Ok, got it!

makortel · 2021-11-02T16:12:23Z

RecoEgamma/ElectronIdentification/interface/ElectronMVAEstimatorRun2.h

@@ -47,13 +71,13 @@ class ElectronMVAEstimatorRun2 : public AnyMVAEstimatorRun2Base {
  std::vector<int> nVariables_;

  // Data members
-  std::vector<std::unique_ptr<const GBRForest>> gbrForests_;
+  std::vector<const GBRForest*> gbrForests_;


Why is the smart pointer removed? (was not clear to me in the commit message)

Because you can't use a unique_ptr<T> if T is only forward declared. But that was an over-optimization anyway, I included now GBRForest.h again, and generating the dictionaries for the ElectronMVAEstimatorRun2 is still very fast.

guitargeek · 2021-11-03T15:27:11Z

Hi @makortel, thanks for the review! I have addressed your comments, let me know if I should follow up on anything.

cmsbuild · 2021-11-03T15:29:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35953/26396

This PR adds an extra 24KB to repository

cmsbuild · 2022-01-21T06:13:34Z

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49ad7a/21869/summary.html
COMMIT: d4a53bb
CMSSW: CMSSW_12_3_X_2022-01-20-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35953/21869/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

4.764.76_ZMuSkim2012D+ZMuSkim2012D+HLTDSKIM2+RECODR1reHLT2+HARVESTDR1reHLT/step2_ZMuSkim2012D+ZMuSkim2012D+HLTDSKIM2+RECODR1reHLT2+HARVESTDR1reHLT.log

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 5 differences found in the comparisons
DQMHistoTests: Total files compared: 43
DQMHistoTests: Total histograms compared: 3464860
DQMHistoTests: Total failures: 5
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3464833
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
Checked 181 log files, 42 edm output root files, 43 DQM output files
TriggerResults: no differences found

jpata · 2022-01-21T11:50:47Z

All the unit tests pass, and there are no relevant reco differences. 🚀

makortel · 2022-01-21T14:17:31Z

+core

Thanks @guitargeek!

jpata · 2022-01-25T08:34:04Z

+reconstruction

removes unneeded dictionaries related to the electron MVA ID
no reco changes, no DQM changes
addresses the dependency issue highlighted in Module IB broken because of tensorflow/core/framework/tensor.h #35754

cmsbuild · 2022-01-25T08:34:27Z

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). 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 · 2022-01-25T09:38:11Z

+1

perrotta · 2022-01-25T09:39:02Z

merge

qliphy · 2022-01-26T01:42:56Z

@guitargeek There appear in IB (and also PR tests) some warnings about duplicated dictionaries or dictionaries defined in the wrong place. Would you please have a look?:

https://cmssdt.cern.ch/SDT/cgi-bin/showDupDict.py/slc7_amd64_gcc10/www/tue/12.3-tue-11/CMSSW_12_3_X_2022-01-25-1100/testLogs/dupDict-lostDefs.log
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49ad7a/21869/dupDict/lostDefs.txt

Searching for 'classes_def.xml' in '/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_3_X_2022-01-20-1100/src'.

./PhysicsTools/SelectorUtils/src/classes_def.xml
EgammaCandidates : VersionedSelector< edm::Ptr< reco::Photon > >
MuonReco : VersionedSelector< edm::Ptr< reco::Muon > >
EgammaCandidates : VersionedSelector< edm::Ptr< reco::GsfElectron > >

slava77 · 2022-01-26T01:58:46Z

@guitargeek There appear in IB (and also PR tests) some warnings about duplicated dictionaries or dictionaries defined in the wrong place. Would you please have a look?:

CMSSW_12_3_X_2022-01-25-1100 appears to be a patch release. IIUC, we need a full build for the duplicate dictionaries to go away.

makortel · 2022-01-26T02:40:29Z

CMSSW_12_3_X_2022-01-25-1700 is a full build, and the complaint of dictionaries in wrong package is still there
https://cmssdt.cern.ch/SDT/cgi-bin/showDupDict.py/slc7_amd64_gcc10/www/tue/12.3-tue-17/CMSSW_12_3_X_2022-01-25-1700/testLogs/dupDict-lostDefs.log

I think in this case we should update the checker script instead

cmssw/Utilities/ReleaseScripts/scripts/duplicateReflexLibrarySearch.py

Line 62 in ec200b2

{'EgammaCandidates' : ['reco::GsfElectron.*','reco::Photon.*']},

cmssw/Utilities/ReleaseScripts/scripts/duplicateReflexLibrarySearch.py

Line 57 in ec200b2

{'MuonReco' : ['reco::Muon(Ref|)(Vector|)']},

In general all dictionaries of DataFormat classes should be declared in the package that defines the class, but in this case PhysicsTools/SelectorUtils (that defines VersionedSelector<T> already depends on DataFormats/EgammaCandidates and DataFormats/MuonReco, and therefore it is the correct place to define these three dicrionaries.

@smuzaffar

smuzaffar · 2022-01-26T06:45:34Z

@makortel , I will update the dict rule defined here https://github.com/cms-sw/cmssw/blob/master/Utilities/ReleaseScripts/scripts/duplicateReflexLibrarySearch.py#L32

cmsbuild added this to the CMSSW_12_2_X milestone Nov 2, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Nov 2, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 2, 2021

cmsbuild added code-checks-pending and removed code-checks-approved labels Nov 2, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 2, 2021

cmsbuild added code-checks-pending core-pending and removed code-checks-approved labels Nov 2, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 2, 2021

guitargeek changed the title ~~Support interactive dictionary generation for ElectronMVAEstimatorRun2~~ Restructure VID Python/FWLite code and avoid generation dictionaries for Reco*/*Identification packages Nov 2, 2021

makortel reviewed Nov 2, 2021

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-approved labels Nov 3, 2021

cmsbuild added requires-external tests-started and removed tests-rejected labels Jan 20, 2022

cmsbuild added tests-rejected and removed tests-started labels Jan 21, 2022

cmsbuild added core-approved and removed core-pending labels Jan 21, 2022

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Jan 25, 2022

cmsbuild added orp-approved and removed orp-pending labels Jan 25, 2022

cmsbuild merged commit f88f508 into cms-sw:master Jan 25, 2022

guitargeek deleted the ElectronMVAEstimatorRun2_fwlite branch January 25, 2022 09:44

smuzaffar mentioned this pull request Jan 26, 2022

added VersionedSelector dict mapping to PhysicsTools/SelectorUtils #36805

Merged

cmsbuild mentioned this pull request Jan 26, 2022

Try a second time if pop fails in ReusableObjectHolder #36813

Closed

jpata mentioned this pull request Mar 14, 2022

MiniAOD warnings in 12_4_0_pre1 using 2021 data #37227

Closed

jpata mentioned this pull request Apr 27, 2022

EL9: Fix OpenSSL warnings #37629

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure VID Python/FWLite code and avoid generating dictionaries for `Reco/Identification` packages #35953

Restructure VID Python/FWLite code and avoid generating dictionaries for `Reco/Identification` packages #35953

guitargeek commented Nov 2, 2021 •

edited

cmsbuild commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

guitargeek commented Nov 2, 2021

makortel Nov 2, 2021

guitargeek Nov 3, 2021

makortel Nov 2, 2021

guitargeek Nov 3, 2021

makortel Nov 2, 2021

guitargeek Nov 3, 2021

makortel Nov 3, 2021

guitargeek Nov 3, 2021

makortel Nov 2, 2021

guitargeek Nov 3, 2021

guitargeek commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

cmsbuild commented Jan 21, 2022

jpata commented Jan 21, 2022

makortel commented Jan 21, 2022

jpata commented Jan 25, 2022

cmsbuild commented Jan 25, 2022

perrotta commented Jan 25, 2022

perrotta commented Jan 25, 2022

qliphy commented Jan 26, 2022

slava77 commented Jan 26, 2022 •

edited

makortel commented Jan 26, 2022

smuzaffar commented Jan 26, 2022

Restructure VID Python/FWLite code and avoid generating dictionaries for Reco*/*Identification packages #35953

Restructure VID Python/FWLite code and avoid generating dictionaries for Reco*/*Identification packages #35953

Conversation

guitargeek commented Nov 2, 2021 • edited

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 Nov 2, 2021

cmsbuild commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

guitargeek commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guitargeek commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

cmsbuild commented Jan 21, 2022

RelVals-INPUT

Comparison Summary

jpata commented Jan 21, 2022

makortel commented Jan 21, 2022

jpata commented Jan 25, 2022

cmsbuild commented Jan 25, 2022

perrotta commented Jan 25, 2022

perrotta commented Jan 25, 2022

qliphy commented Jan 26, 2022

slava77 commented Jan 26, 2022 • edited

makortel commented Jan 26, 2022

smuzaffar commented Jan 26, 2022

Restructure VID Python/FWLite code and avoid generating dictionaries for `Reco/Identification` packages #35953

Restructure VID Python/FWLite code and avoid generating dictionaries for `Reco/Identification` packages #35953

guitargeek commented Nov 2, 2021 •

edited

slava77 commented Jan 26, 2022 •

edited