[WIP] add `fillPSetDescription` to `CkfTrackCandidateMakerBase` and propagate it to `CkfTrackCandidateMaker` #35385

mmusich · 2021-09-23T15:02:43Z

resolves #35344

PR description:

Address #35344 by adding a fillPSetDescription method to CkfTrackCandidateMakerBase and propagate it to CkfTrackCandidateMaker.

PR validation:

It compiles, not yet run a full integration tests matrix.

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

N/A

cmsbuild · 2021-09-23T15:08:12Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35385/25478

This PR adds an extra 20KB to repository

cmsbuild · 2021-09-23T15:08:34Z

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

RecoTracker/CkfPattern (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @ebrondol, @gpetruc, @mmusich, @mtosi, @dgulhan 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

makortel · 2021-09-23T15:36:20Z

RecoTracker/CkfPattern/src/CkfTrackCandidateMakerBase.cc

+    desc.add<edm::InputTag>("src", edm::InputTag("globalMixedSeeds"));
+
+    edm::ParameterSetDescription psd0;
+    desc.add<edm::ParameterSetDescription>("TrajectoryBuilderPSet", psd0);


I'm afraid this states that the TrajectoryBuilderPSet can be only an empty PSet.

In the CkfTrackCandidates_cfi the TrajectoryBuilderPSet = cms.PSet(refToPSet_ = cms.string('GroupedCkfTrajectoryBuilder')) means that the contents of TrajectoryBuilderPSet are taken from process.GroupedCkfTrajectoryBuilder PSet at the time the python configuration is "serialized" to be shipped to the C++ code, where the configuration validation, based on the ConfigurationDescriptions, is run. The configuration validation should then fail because the PSet has unexpected parameters.

A complication here is that the parameters of this PSet depend (in general, at least, I didn't check this particular case) on the helper plugin. But, in this case the helper plugin type appears to be defined within the PSet, in which case it might be straightforward to use
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp#A_Plugin_Module_Using_Another_He

@makortel

The configuration validation should then fail because the PSet has unexpected parameters.

indeed it fails at runtime with:

----- Begin Fatal Exception 23-Sep-2021 18:17:08 CEST----------------------- An exception of category 'Configuration' occurred while [0] Constructing the EventProcessor [1] Validating configuration of module: class=CkfTrackCandidateMaker label='convTrackCandidates' Exception Message: Illegal parameters found in configuration. The parameters are named: 'ComponentType' 'MeasurementTrackerName' 'TTRHBuilder' 'alwaysUseInvalidHits' 'bestHitOnly' 'estimator' 'foundHitBonus' 'inOutTrajectoryFilter' 'intermediateCleaning' 'keepOriginalIfRebuildFails' 'lockHits' 'lostHitPenalty' 'maxCand' 'minNrOfHitsForRebuild' 'propagatorAlong' 'propagatorOpposite' 'requireSeedHitsInRebuild' 'seedAs5DHit' 'trajectoryFilter' 'updator' 'useSameTrajFilter' You could be trying to use parameter names that are not allowed for this plugin or they could be misspelled. ----- End Fatal Exception -------------------------------------------------

I'm afraid this states that the TrajectoryBuilderPSet can be only an empty PSet.

now, this is quite unfortunate because in turn process.GroupedCkfTrajectoryBuilder depends on other refToPSet_s, e.g.:

cmssw/RecoTracker/CkfPattern/python/GroupedCkfTrajectoryBuilder_cfi.py

Line 12 in 1a6b75d

trajectoryFilter = cms.PSet(refToPSet_ = cms.string('CkfBaseTrajectoryFilter_block')),

and

cmssw/RecoTracker/CkfPattern/python/GroupedCkfTrajectoryBuilder_cfi.py

Line 14 in 1a6b75d

inOutTrajectoryFilter = cms.PSet(refToPSet_ = cms.string('CkfBaseTrajectoryFilter_block')),

. But, in this case the helper plugin type appears to be defined within the PSet, in which case it might be straightforward to use https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp#A_Plugin_Module_Using_Another_He

Let me get this straight, where should I define the descriptions for the configurations that are just contained in a PSet which doesn't seem to be directly linked with any EDModule ?

Let me get this straight, where should I define the descriptions for the configurations that are just contained in a PSet which doesn't seem to be directly linked with any EDModule ?

Strictly speaking there is no mechanism to define the fillDescriptions() for such "global PSets". But such global PSets can still be used via cms.PSet(refToPSet_ = cms.string(...)) construct in an EDModule that uses the "helper plugin mechanism" in its fillDescriptions().

In the "helper plugin mechanism" the static void fillPSetDescription(edm::ParameterSetDescription& iDesc) would be defined in all the plugin classes that are part of BaseCkfTrajectoryBuilderFactory factory, the factory registration macro

cmssw/RecoTracker/CkfPattern/src/BaseCkfTrajectoryBuilderFactory.cc

Line 3 in 1a6b75d

EDM_REGISTER_PLUGINFACTORY(BaseCkfTrajectoryBuilderFactory, "BaseCkfTrajectoryBuilderFactory");

needs to be changed to EDM_REGISTER_VALIDATED_PLUGINFACTORY, and the plugin registration macro e.g. in

cmssw/RecoTracker/CkfPattern/plugins/SealModules.cc

Lines 21 to 22 in 1a6b75d

DEFINE_EDM_PLUGIN(BaseCkfTrajectoryBuilderFactory, CkfTrajectoryBuilder, "CkfTrajectoryBuilder");

DEFINE_EDM_PLUGIN(BaseCkfTrajectoryBuilderFactory, GroupedCkfTrajectoryBuilder, "GroupedCkfTrajectoryBuilder");

need to be changed to DEFINE_EDM_VALIDATED_PLUGIN.

When using a global PSet via refToPSet_ in this setup the PSet is missing a parameter, that parameter is inserted into "a local copy of the PSet" during the configuration validation phase.

But a new parameter does not get added into the "global PSet" in the cfi generation phase. A new parameter gets added only to the cfi generated for the EDModule.

I see, thanks. I will take a stab at it tomorrow.

@mmusich
In case it helps, I started to follow @makortel's instructions in 4352178.
It does not work yet, but maybe it saves you some typing.

Thanks, @missirol I merged your commit, though I am a bit lost on what still needs to be done.
If I merge your changes on top of mine and I run for example wf. 4.53 I get:

----- Begin Fatal Exception 24-Sep-2021 17:15:28 CEST----------------------- An exception of category 'Configuration' occurred while [0] Constructing the EventProcessor [1] Validating configuration of module: class=CkfTrackCandidateMaker label='convTrackCandidates' Exception Message: Illegal parameter found in configuration. The parameter is named: 'minGoodStripCharge' You could be trying to use a parameter name that is not allowed for this plugin or it could be misspelled. ----- End Fatal Exception -------------------------------------------------

while I would have naively expected that to be taken care from here:

4352178#diff-3a57ce7e190928d1e542d59a175b5be0b6b5d0c6bb0a664aa909e96778f92c6eR42

When I last tested, I encountered a similar complaint (in a different wf, from a different module, about different parameters), so something must be off in how I filled the FillDescriptions methods. I'll try to have a look today/tomorrow.

cmsbuild · 2021-09-24T17:46:35Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35385/25519

This PR adds an extra 60KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File TrackingTools/TrajectoryFiltering/interface/MinHitsTrajectoryFilter.h modified in PR(s): [EGM] Eta-Extended-Electrons (EEE) #35309

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-35385/25519/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35385/25519/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

missirol · 2021-09-24T17:52:44Z

TrackingTools/TrajectoryFiltering/interface/CompositeTrajectoryFilter.h

@@ -26,6 +27,10 @@ class CompositeTrajectoryFilter : public TrajectoryFilter {

  ~CompositeTrajectoryFilter() override {}

+  static void fillPSetDescription(edm::ParameterSetDescription& iDesc){
+    // !!! MISSING


Just highlighting this function, because it needs to be implemented.

yeah, I noticed that, but I was more curious about the ones that I don't understand, before tackling the one I do understand :)

cmsbuild · 2021-09-24T18:02:48Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35385/25520

This PR adds an extra 60KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File TrackingTools/TrajectoryFiltering/interface/MinHitsTrajectoryFilter.h modified in PR(s): [EGM] Eta-Extended-Electrons (EEE) #35309

cmsbuild · 2021-09-24T18:03:08Z

Pull request #35385 was updated. @Martin-Grunewald, @jpata, @cmsbuild, @missirol, @slava77 can you please check and sign again.

missirol · 2021-09-25T16:23:37Z

@mmusich missirol@7d7525f includes some more fixes, but is still work-in-progress.

With that, wf 11634.0 runs up to the RECO step, and there it fails due to an invalid configuration
(didn't figure out yet what the solution is..):

An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Validating configuration of module: class=CkfTrackCandidateMaker label='initialStepTrackCandidatesPreSplitting'
Exception Message:
Illegal parameters found in configuration.  The parameters are named:
 'layerMask'
 'maxNSat'
 'maxTrimmedSizeDiffNeg'
 'maxTrimmedSizeDiffPos'
 'seedCutMIPs'
 'seedCutSN'
 'subclusterCutMIPs'
 'subclusterCutSN'
 'subclusterWindow'
 'trimMaxADC'
 'trimMaxFracNeigh'
 'trimMaxFracTotal'
You could be trying to use parameter names that are not
allowed for this plugin or they could be misspelled.

These parameters should be provided by StripSubClusterShapeFilterBase::fillPSetDescription, but somehow that's not working atm.

One problem might be that initialStepTrajectoryFilterShapePreSplitting comes in via the CompositeTrajectoryFilter named initialStepTrajectoryFilterPreSplitting, and I don't know if my current implementation of CompositeTrajectoryFilter::fillPSetDescription is correct (unlikely).

cmsbuild · 2021-09-27T10:11:17Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35385/25548

This PR adds an extra 220KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/Configuration/python/customizeHLTforCMSSW.py modified in PR(s): [EGM] Eta-Extended-Electrons (EEE) #35309
- File TrackingTools/GsfTracking/python/CkfElectronCandidateMaker_cff.py modified in PR(s): [EGM] Eta-Extended-Electrons (EEE) #35309
- File TrackingTools/TrajectoryFiltering/interface/MinHitsTrajectoryFilter.h modified in PR(s): [EGM] Eta-Extended-Electrons (EEE) #35309

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

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

jpata · 2021-10-11T13:41:21Z

kind ping on this, let us know if/how you plan to take this forward.

missirol · 2021-10-11T13:45:47Z

Hi, I identified a couple of fixes, and I was planning to have an update in 1/2 days from now (assuming this works for @mmusich).

missirol · 2021-10-12T14:31:41Z

missirol@cecd514 contains a new attempt from scratch (with all changes squashed, and rebased on top of 12_1_0_pre4).

runTheMatrix.py -l limited and addOnTests passed. What I haven't checked is differences in the outputs.

Details:
aside from fixing the addOnTests, I undid the addition of the flags skipClusters and skipPhase2Clusters in CkfTrackCandidateMakerBase (I suspect this might be the reason for the many differences in the outputs of the last tests); trying to configure these two new flags correctly in all places seems error-prone, and probably too invasive for this technical PR. The pre-PR logic in CkfTrackCandidateMakerBase leads to using clustersToSkip or phase2ClustersToSkip only if those InputTags exist; in this new attempt, I tried the minimal change of making CkfTrackCandidateMakerBase use them only if their labels are non-empty (with ~~that~~ empty being the default).

jpata · 2021-10-25T13:54:47Z

kind ping on this

mmusich · 2021-10-26T11:32:17Z

@jpata sorry for the delay, I plan to come back to this later this week.

jpata · 2021-11-08T12:30:01Z

kind ping on this

jpata · 2021-11-15T14:40:45Z

kind 7-day ping

jpata · 2021-12-09T15:33:54Z

-1

for now

mmusich · 2021-12-13T09:47:06Z

closing in favor of #36459.
Thanks @missirol

cmsbuild added this to the CMSSW_12_1_X milestone Sep 23, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 23, 2021

mmusich mentioned this pull request Sep 23, 2021

CkfTrackCandidateMaker lacks fillDescriptions method #35344

Closed

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

makortel reviewed Sep 23, 2021

View reviewed changes

cmsbuild added code-checks-pending hlt-pending and removed code-checks-approved labels Sep 24, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Sep 24, 2021

missirol reviewed Sep 24, 2021

View reviewed changes

mmusich force-pushed the fillDescriptionsInCkfTrackCandidateMaker branch from be36ebb to 97ab884 Compare September 24, 2021 17:55

cmsbuild added code-checks-pending and removed code-checks-rejected labels Sep 24, 2021

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

cmsbuild added code-checks-pending and removed code-checks-approved labels Sep 27, 2021

cmsbuild added tests-rejected and removed tests-started labels Oct 2, 2021

This was referenced Oct 4, 2021

Increase SiPixel L1 cluster threshold from 2000 to 4000e, synchronize channel threshold between HTL and offline #35518

Merged

Migrate module config in RecoTracker to use default seedingLayersEDProducer in cfipython v2 #35555

Merged

cmsbuild mentioned this pull request Oct 11, 2021

HLT migration to CMSSW_12_1_0_pre4 template #35608

Merged

This was referenced Oct 15, 2021

Track DNN update on mkFit tracks #35686

Merged

esConsumes migration of DT calibration codes #35695

Merged

Disable looper track reconstruction for >= Run2_2016 #35721

Merged

cmsbuild mentioned this pull request Oct 25, 2021

Improve various Patatrack Kernels #35598

Closed

cmsbuild mentioned this pull request Oct 28, 2021

[EGM] Undo relaxing of GSF nHit cuts for phase2 #35887

Merged

smuzaffar modified the milestones: CMSSW_12_1_X, CMSSW_12_2_X Oct 29, 2021

This was referenced Oct 29, 2021

Update customisePixelL1ClusterThresholdForRun2Input to adjust also the GPU producer #35901

Merged

Phase2-hgx293X Extend the possibility of new cells needed for rotated full wafers #35864

Closed

smuzaffar modified the milestones: CMSSW_12_2_X, CMSSW_12_3_X Dec 6, 2021

cmsbuild added reconstruction-rejected and removed reconstruction-pending labels Dec 9, 2021

missirol mentioned this pull request Dec 11, 2021

add fillDescriptions to CkfTrackCandidateMaker (and its dependencies) #36459

Merged

mmusich closed this Dec 13, 2021

mmusich deleted the fillDescriptionsInCkfTrackCandidateMaker branch December 13, 2021 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] add `fillPSetDescription` to `CkfTrackCandidateMakerBase` and propagate it to `CkfTrackCandidateMaker` #35385

[WIP] add `fillPSetDescription` to `CkfTrackCandidateMakerBase` and propagate it to `CkfTrackCandidateMaker` #35385

mmusich commented Sep 23, 2021

cmsbuild commented Sep 23, 2021

cmsbuild commented Sep 23, 2021

makortel Sep 23, 2021

mmusich Sep 23, 2021

makortel Sep 23, 2021

mmusich Sep 23, 2021

missirol Sep 23, 2021

mmusich Sep 24, 2021 •

edited

Loading

missirol Sep 24, 2021

cmsbuild commented Sep 24, 2021

missirol Sep 24, 2021

mmusich Sep 24, 2021

cmsbuild commented Sep 24, 2021

cmsbuild commented Sep 24, 2021

missirol commented Sep 25, 2021

cmsbuild commented Sep 27, 2021

jpata commented Oct 11, 2021

missirol commented Oct 11, 2021

missirol commented Oct 12, 2021 •

edited

Loading

jpata commented Oct 25, 2021

mmusich commented Oct 26, 2021

jpata commented Nov 8, 2021

jpata commented Nov 15, 2021

jpata commented Dec 9, 2021

mmusich commented Dec 13, 2021

	DEFINE_EDM_PLUGIN(BaseCkfTrajectoryBuilderFactory, CkfTrajectoryBuilder, "CkfTrajectoryBuilder");
	DEFINE_EDM_PLUGIN(BaseCkfTrajectoryBuilderFactory, GroupedCkfTrajectoryBuilder, "GroupedCkfTrajectoryBuilder");

[WIP] add fillPSetDescription to CkfTrackCandidateMakerBase and propagate it to CkfTrackCandidateMaker #35385

[WIP] add fillPSetDescription to CkfTrackCandidateMakerBase and propagate it to CkfTrackCandidateMaker #35385

Conversation

mmusich commented Sep 23, 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 23, 2021

cmsbuild commented Sep 23, 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

mmusich Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Sep 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Sep 24, 2021

cmsbuild commented Sep 24, 2021

missirol commented Sep 25, 2021

cmsbuild commented Sep 27, 2021

jpata commented Oct 11, 2021

missirol commented Oct 11, 2021

missirol commented Oct 12, 2021 • edited Loading

jpata commented Oct 25, 2021

mmusich commented Oct 26, 2021

jpata commented Nov 8, 2021

jpata commented Nov 15, 2021

jpata commented Dec 9, 2021

mmusich commented Dec 13, 2021

[WIP] add `fillPSetDescription` to `CkfTrackCandidateMakerBase` and propagate it to `CkfTrackCandidateMaker` #35385

[WIP] add `fillPSetDescription` to `CkfTrackCandidateMakerBase` and propagate it to `CkfTrackCandidateMaker` #35385

mmusich Sep 24, 2021 •

edited

Loading

missirol commented Oct 12, 2021 •

edited

Loading