Add fillDescription to CommonTools/UtilAlgos/interface/Merger.h #43728

silviodonato · 2024-01-17T10:38:14Z

PR description:

This PR addd fillDescription to CommonTools/UtilAlgos/interface/Merger.h in order to make it available in ConfDB.
No "_cfi" was available previeusly.

PR validation:

[sdonato@lxplus713 src]$ edmPluginHelp  -p TrackSimpleMerger
1  TrackSimpleMerger  (global::EDProducer)  "pluginCommonToolsRecoAlgos_plugins.so"

This plugin has 1 PSet description. This description is always used to validate configurations. The label below is used when generating the cfi file.

  1.1 module label: simpleMergedTracks

    src
                        type: VInputTag 
                        default: (vector size = 2)
                          [0]: 'collection1'
                          [1]: 'collection2'

    mightGet
                        type: untracked vstring optional
                        default: none
                        List contains the branch names for the EDProducts which might be requested by the module.
                        The format for identifying the EDProduct is the same as the one used for OutputModules, except no wild cards are allowed. E.g.
                        Foos_foomodule_whichFoo_RECO

Without the PR the output is

[sdonato@lxplus713 src]$ edmPluginHelp  -p TrackSimpleMerger
1  TrackSimpleMerger  (global::EDProducer)  "pluginCommonToolsRecoAlgos_plugins.so"

  This plugin has not implemented the function which defines its
  configuration descriptions yet. No descriptions are available.
  Its PSets will not be validated, and no cfi files will be generated.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Yes, I will need a backport to 13_3_X.

cc: @mmusich

cmsbuild · 2024-01-17T10:38:41Z

cms-bot internal usage

cmsbuild · 2024-01-17T10:47:20Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43728/38432

This PR adds an extra 20KB to repository

cmsbuild · 2024-01-17T10:47:41Z

A new Pull Request was created by @silviodonato (Silvio Donato) for master.

It involves the following packages:

CommonTools/UtilAlgos (reconstruction)

@mandrenguyen, @jfernan2, @cmsbuild can you please review it and eventually sign? Thanks.
@missirol this is something you requested to watch as well.
@antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

mmusich · 2024-01-17T10:47:56Z

@cmsbuild, please test

cmsbuild · 2024-01-17T10:59:31Z

-1

Failed Tests: Build
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-de28bf/36882/summary.html
COMMIT: df5ba00
CMSSW: CMSSW_14_0_X_2024-01-16-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43728/36882/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

function for each template instantiation.
3. This can also happen when there is an inheritance heirarchy and multiple plugin modules
are defined using derived classes and the base class which share the same fillDescriptions
function. Fix these by redefining the fillDescriptions function in each derived class.
----- End Fatal Exception -------------------------------------------------
gmake: *** [lib/el8_amd64_gcc12/CommonToolsCandAlgos_plugins.edmplugin] Error 1
Entering library rule at CommonTools/CandAlgos
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_X_2024-01-16-2300/src/CommonTools/CandAlgos/src/GenJetParticleSelector.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_X_2024-01-16-2300/src/CommonTools/CandAlgos/src/ModifyObjectValueBase.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_X_2024-01-16-2300/src/CommonTools/CandAlgos/src/decayParser.cc
>> Building shared library tmp/el8_amd64_gcc12/src/CommonTools/CandAlgos/src/CommonToolsCandAlgos/libCommonToolsCandAlgos.so

cmsbuild · 2024-01-17T11:27:42Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43728/38433

This PR adds an extra 20KB to repository

cmsbuild · 2024-01-17T11:28:07Z

Pull request #43728 was updated. @jfernan2, @cmsbuild, @mandrenguyen can you please check and sign again.

silviodonato · 2024-01-17T11:29:28Z

It is still crashing, I need to implement the fillDescription for all classes derived from Merged.h

…r PFClusterCandidateMerger SuperClusterMerger TrackSimpleMerger

mmusich

minor suggestion.

mmusich · 2024-01-17T11:40:05Z

CommonTools/RecoAlgos/plugins/BasicClusterMerger.cc

+                                           edm::InputTag("collection1"),
+                                           edm::InputTag("collection2"),
+                                       });
+  descriptions.add("basicClusterMerger", desc);


Suggested change

descriptions.add("basicClusterMerger", desc);

descriptions.addWithDefaultLabel(desc);

?

mmusich · 2024-01-17T11:40:08Z

CommonTools/RecoAlgos/plugins/ElectronCollectionMerger.cc

+                                           edm::InputTag("collection1"),
+                                           edm::InputTag("collection2"),
+                                       });
+  descriptions.add("gsfElectronCollectionMerger", desc);


Suggested change

descriptions.add("gsfElectronCollectionMerger", desc);

descriptions.addWithDefaultLabel(desc);

mmusich · 2024-01-17T11:40:12Z

CommonTools/RecoAlgos/plugins/ElectronCollectionMerger.cc

+                                           edm::InputTag("collection1"),
+                                           edm::InputTag("collection2"),
+                                       });
+  descriptions.add("patElectronCollectionMerger", desc);


Suggested change

descriptions.add("patElectronCollectionMerger", desc);

descriptions.addWithDefaultLabel(desc);

mmusich · 2024-01-17T11:40:14Z

CommonTools/RecoAlgos/plugins/PFClusterCandidateMerger.cc

+                                           edm::InputTag("collection1"),
+                                           edm::InputTag("collection2"),
+                                       });
+  descriptions.add("pfClusterRefCandidateMerger", desc);


Suggested change

descriptions.add("pfClusterRefCandidateMerger", desc);

descriptions.addWithDefaultLabel(desc);

mmusich · 2024-01-17T11:40:17Z

CommonTools/RecoAlgos/plugins/SuperClusterMerger.cc

+                                           edm::InputTag("collection1"),
+                                           edm::InputTag("collection2"),
+                                       });
+  descriptions.add("superClusterMerger", desc);


Suggested change

descriptions.add("superClusterMerger", desc);

descriptions.addWithDefaultLabel(desc);

cmsbuild · 2024-01-17T11:42:53Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43728/38435

This PR adds an extra 24KB to repository

cmsbuild · 2024-01-17T11:43:16Z

Pull request #43728 was updated. @cmsbuild, @jfernan2, @mandrenguyen can you please check and sign again.

…tedPFCandidateMerger L1TPFCandMerger EgammaIsolationAlgos PFProducer

cmsbuild · 2024-01-17T11:56:59Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43728/38436

This PR adds an extra 32KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43728/38436/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43728/38436/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

missirol · 2024-01-17T12:11:30Z

CommonTools/UtilAlgos/interface/Merger.h

@@ -34,19 +36,20 @@ class Merger : public edm::global::EDProducer<> {
  explicit Merger(const edm::ParameterSet&);
  /// destructor
  ~Merger() override;
+  static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);


I'm completely guessing, but I thought it would be enough to call addWithDefaultLabel from the fillDescriptions of the template class (here, Merger), and then different cfis would be generated for the different instances automatically. Something like this seemed to work in #37039. (but again, I'm only guessing)

That was my first attempt, but I got

----- Begin Fatal Exception 17-Jan-2024 12:20:25 CET----------------------- An exception of category 'LogicError' occurred while [0] Running executable "edmWriteConfigs" [1] Processing library /afs/cern.ch/work/s/sdonato/CMSSW_14_0_X_2024-01-16-2300/tmp/el8_amd64_gcc12/src/CommonTools/RecoAlgos/plugins/CommonToolsRecoAlgos_plugins/libCommonToolsRecoAlgos_plugins.so [2] Writing cfi files using ParameterSetDescriptions for module of base type EDProducer with plugin name 'SuperClusterMerger' [3] Executing function ConfigurationDescriptions::writeCfiForLabel Exception Message: Two cfi files are being generated with the same name in the same directory. The cfi file name is 'simpleMergedTracks_cfi.py' and the module label is 'simpleMergedTracks'. This error is probably caused by an error in one or more fillDescriptions functions where duplicate module labels are being passed to the ConfigurationDescriptions::add function. All such module labels must be unique within a package. If you do not want the generated cfi file and do not need more than one description for a plugin, then a way to fix this is to use the addDefault function instead of the add function. There are 3 common ways this problem can happen. 1. This can happen when a module label is explicitly duplicated in one or more fillDescriptions functions. Fix these by changing the module labels to be unique. 2. This can also happen when a module class is a template class and plugins are defined by instantiations with differing template parameters and these plugins share the same fillDescriptions function. Fix these by specializing the fillDescriptions function for each template instantiation. 3. This can also happen when there is an inheritance heirarchy and multiple plugin modules are defined using derived classes and the base class which share the same fillDescriptions function. Fix these by redefining the fillDescriptions function in each derived class. ----- End Fatal Exception ------------------------------------------------- ``` (see above). But probably it is because I used `add` instead of `addWithDefaultLabel`

Indeed using addWithDefaultLabel seems to solve the problem. Moved to #43731

silviodonato · 2024-01-17T13:24:36Z

Moved to #43731

add fillDescription in CommonTools/UtilAlgos/interface/Merger.h

df5ba00

cmsbuild added this to the CMSSW_14_0_X milestone Jan 17, 2024

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending code-checks-pending labels Jan 17, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 17, 2024

cmsbuild added tests-started and removed tests-pending labels Jan 17, 2024

cmsbuild added tests-rejected and removed tests-started labels Jan 17, 2024

restrict fillDescription to TrackSimpleMerger

98c8a53

cmsbuild added tests-pending code-checks-pending and removed tests-rejected code-checks-approved labels Jan 17, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 17, 2024

define fill description in BasicClusterMerger ElectronCollectionMerge…

ff45c90

…r PFClusterCandidateMerger SuperClusterMerger TrackSimpleMerger

cmsbuild removed the code-checks-approved label Jan 17, 2024

cmsbuild added the code-checks-pending label Jan 17, 2024

mmusich reviewed Jan 17, 2024

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 17, 2024

add filldescription for CandMerger CandRefMerger CandViewMerger Isola…

6d18eaf

…tedPFCandidateMerger L1TPFCandMerger EgammaIsolationAlgos PFProducer

cmsbuild added l1-pending upgrade-pending code-checks-pending and removed code-checks-approved labels Jan 17, 2024

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jan 17, 2024

missirol reviewed Jan 17, 2024

View reviewed changes

silviodonato closed this Jan 17, 2024

silviodonato mentioned this pull request Jan 17, 2024

Add fillDescription to CommonTools/UtilAlgos/interface/Merger.h - 140X #43731

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fillDescription to CommonTools/UtilAlgos/interface/Merger.h #43728

Add fillDescription to CommonTools/UtilAlgos/interface/Merger.h #43728

silviodonato commented Jan 17, 2024

cmsbuild commented Jan 17, 2024 •

edited

Loading

cmsbuild commented Jan 17, 2024

cmsbuild commented Jan 17, 2024

mmusich commented Jan 17, 2024

cmsbuild commented Jan 17, 2024

cmsbuild commented Jan 17, 2024

cmsbuild commented Jan 17, 2024

silviodonato commented Jan 17, 2024

mmusich left a comment

mmusich Jan 17, 2024

mmusich Jan 17, 2024

mmusich Jan 17, 2024

mmusich Jan 17, 2024

mmusich Jan 17, 2024

cmsbuild commented Jan 17, 2024

cmsbuild commented Jan 17, 2024

cmsbuild commented Jan 17, 2024

missirol Jan 17, 2024

silviodonato Jan 17, 2024

silviodonato Jan 17, 2024

silviodonato commented Jan 17, 2024

	descriptions.add("basicClusterMerger", desc);
	descriptions.addWithDefaultLabel(desc);

	descriptions.add("gsfElectronCollectionMerger", desc);
	descriptions.addWithDefaultLabel(desc);

	descriptions.add("patElectronCollectionMerger", desc);
	descriptions.addWithDefaultLabel(desc);

	descriptions.add("pfClusterRefCandidateMerger", desc);
	descriptions.addWithDefaultLabel(desc);

	descriptions.add("superClusterMerger", desc);
	descriptions.addWithDefaultLabel(desc);

Add fillDescription to CommonTools/UtilAlgos/interface/Merger.h #43728

Add fillDescription to CommonTools/UtilAlgos/interface/Merger.h #43728

Conversation

silviodonato commented Jan 17, 2024

PR description:

PR validation:

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

cmsbuild commented Jan 17, 2024 • edited Loading

cmsbuild commented Jan 17, 2024

cmsbuild commented Jan 17, 2024

mmusich commented Jan 17, 2024

cmsbuild commented Jan 17, 2024

Build

cmsbuild commented Jan 17, 2024

cmsbuild commented Jan 17, 2024

silviodonato commented Jan 17, 2024

mmusich left a 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

cmsbuild commented Jan 17, 2024

cmsbuild commented Jan 17, 2024

cmsbuild commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silviodonato commented Jan 17, 2024

cmsbuild commented Jan 17, 2024 •

edited

Loading