improve fillDescriptions section in cms coding rules

[mmusich]

This might help in getting developers producing code that results in configurations parsable in confDB.
@cms-sw/hlt-l2 @fwyzard @missirol FYI

[fwyzard]

I would go further and simply require that all modules have a working fill/descriptions() that generates at least one cfi file - irrespective of their potential use in ConfDB.

[mmusich]

I would go further and simply require that all modules have a working fill/descriptions() that generates at least one cfi file - irrespective of their potential use in ConfDB.

In a sense the code rules already say that:

The _cfi file should be left to be generated automatically with the fillDescriptions().

[makortel]

I would go further and simply require that all modules have a working fill/descriptions() that generates at least one cfi file - irrespective of their potential use in ConfDB.

In a sense the code rules already say that:

The _cfi file should be left to be generated automatically with the fillDescriptions().

I think there is a difference. The current wording allows a _cfi file not to exist (i.e. only when a _cfi exists, it should be generated automatically), whereas @fwyzard's suggestion would require every module to have _cfi file that is generated from fillDescriptions().

If would be serious about requiring every module to have a fillDescriptions() that generates a _cfi file, we should enforce it, and make all existing code compliant. The latter would imply non-trivial amount of work (although if achieved it would be great).

[mmusich]

If would be serious about requiring every module to have a fillDescriptions() that generates a _cfi file, we should enforce it, and make all existing code compliant.

I agree that this would be great. On the other hand that would not be sufficient: all the other conditions still apply.

[makortel]

We can in any case discuss about this in Core Software meeting tomorrow (although I would not make conclusions yet).

[mmusich]

We can in any case discuss about this in Core Software meeting tomorrow (although I would not make conclusions yet).

is there any showstopper in merging this as is?

[fwyzard]

If would be serious about requiring every module to have a fillDescriptions() that generates a _cfi file, we should enforce it, and make all existing code compliant. The latter would imply non-trivial amount of work (although if achieved it would be great).

To begin with, we could use a similar approach to the coding rules: we keep the current code as it is, but we require all new modules to have a proper fillDescriptions().

In addition, changing all addDefault() to addWithDefaultLabel() is trivial to try, and maybe it would not be too hard to figure out and fix what breaks.

By the way, for the Modifier case maybe we can introduce a different function (e.g. addWithModifierLabel()) and tell people to use that one instead ?

[mmusich]

In addition, changing all addDefault() to addWithDefaultLabel() is trivial to try.

what happens when there is nothing added to the descriptions (e.g. in the plugins that have been written via the skeleton and get the boiler-plate:

  //The following says we do not know what parameters are allowed so do no validation
  // Please change this to state exactly what you do use, even if it is no parameters
  edm::ParameterSetDescription desc;
  desc.setUnknown();
  descriptions.addDefault(desc);

?
Do we really want to generate hundreds of empty (?) cfi files?

[makortel]

In addition, changing all addDefault() to addWithDefaultLabel() is trivial to try.

what happens when there is nothing added to the descriptions (e.g. in the plugins that have been written via the skeleton and get the boiler-plate:

  //The following says we do not know what parameters are allowed so do no validation
  // Please change this to state exactly what you do use, even if it is no parameters
  edm::ParameterSetDescription desc;
  desc.setUnknown();
  descriptions.addDefault(desc);

?

I'd expect replacing the addDefault() with addWithDefaultLabel() to lead to a generated _cfi that contains effectively

fooModule = cms.EDProducer('FooModule')

[mmusich]

I'd expect replacing the addDefault() with addWithDefaultLabel() to lead to a generated _cfi that contains effectively

I just checked and it contains actually:

import FWCore.ParameterSet.Config as cms

fooModule = cms.EDProducer('FooModule',
  mightGet = cms.optional.untracked.vstring
)

[makortel]

We can in any case discuss about this in Core Software meeting tomorrow (although I would not make conclusions yet).

is there any showstopper in merging this as is?

In absence of wider discussion I would organize the content a bit differently, e.g. along

14. The `_cfi` file should be left to be generated automatically with the `fillDescriptions()`. When Modifier customizations are needed, the auto-generated label should have e.g. "Default" postfix and be imported+cloned to the desired name.
  Whenever the plugin needs to be included in a HLT configuration, it is mandatory (for confDB parsing purposes) that a default value is provided for all the configurable parameters. In general it is preferable to use `addWithDefaultLabel()`, such that the auto-generated cfi file bears the same name as the plugin, starting with a lower case character.

and leave out the comment about addDefault() (whose use currently is not generally discouraged).

[makortel]

I'd expect replacing the addDefault() with addWithDefaultLabel() to lead to a generated _cfi that contains effectively

I just checked and it contains actually:

import FWCore.ParameterSet.Config as cms

fooModule = cms.EDProducer('FooModule',
  mightGet = cms.optional.untracked.vstring
)

Ah right, I forgot the mightGet (that we could clean up by now).

[mmusich]

and leave out the comment about addDefault() (whose use currently is not generally discouraged).

we certainly do want to discourage it for HLT purposes.

[fwyzard]

Do we really want to generate hundreds of empty (?) cfi files?

No, but about 150 cases seem to have a non-empty description (at least, they do do not call desc.setUnknown()) and still use descriptions.addDefault(desc).

[makortel]

If would be serious about requiring every module to have a fillDescriptions() that generates a _cfi file, we should enforce it, and make all existing code compliant. The latter would imply non-trivial amount of work (although if achieved it would be great).

To begin with, we could use a similar approach to the coding rules: we keep the current code as it is, but we require all new modules to have a proper fillDescriptions().

We could. That would likely leave the enforcement to the PR reviewers (which means likely something will slip through).

In addition, changing all addDefault() to addWithDefaultLabel() is trivial to try, and maybe it would not be too hard to figure out and fix what breaks.

True. I was thinking more the cases that don't implement a meaningful fillDescriptions(), or where implementing one would be impossible today.

[makortel]

and leave out the comment about addDefault() (whose use currently is not generally discouraged).

we certainly do want to discourage it for HLT purposes.

I'd be fine if the comment on addDefault() being discouraged would be worded to be specific to HLT. (for more general statement I want wider discussion)

[mmusich]

No, but about 150 cases seem to have a non-empty description (at least, they do do not call desc.setUnknown()) and still use descriptions.addDefault(desc)

I opened cms-sw/cmssw#44372 to play with this.

[mmusich]

In absence of wider discussion I would organize the content a bit differently

I'd be fine if the comment on addDefault() being discouraged would be worded to be specific to HLT.

this is implemented in the last push.

[Martin-Grunewald]

No, but about 150 cases seem to have a non-empty description (at least, they do do not call desc.setUnknown()) and still use descriptions.addDefault(desc)

I opened cms-sw/cmssw#44372 to play with this.

Hmm, I am not so keen on this automatic replacement/change: ConfDB parsing gives precedence to the generated cfi files in ../cfipython, but if now with this change there are newly generated cfi files which are only rudimentary/incomplete/partially complete (compared to explicit python files in ../python), because fillDescriptions is incomplete/permissive, then ConfDb parsing will pick up the wrong templates.

For existing usage, I am afraid this would have to be looked at case by case. For new plugins, however, it could be enforced.

[mmusich]

For existing usage, I am afraid this would have to be looked at case by case.

Indeed, let's do that. But first let's see if change breaks something else.

[mmusich]

For existing usage, I am afraid this would have to be looked at case by case.

for the record (and for lack of a better place where to put it), here's the list of all the EDModules that we're using in the HLT combined table and do not implement a fillDescriptions() such that it produces also a _cfi file (e.g. it's either unimplemented or the author left an addDefault()).
This list needs to be cross-checked with the one in the PR cms-sw/cmssw#44372, and the intersection needs to be studied.

List of modules

• AnalyticalTrackSelector
• BTagProbabilityToDiscriminator
• CSCHaloDataProducer
• CSCSegmentProducer
• CaloJetSelector
• CaloMETProducer
• CaloTowersCreator
• CandIPProducer
• CandPtrSelector
• CandSecondaryVertexProducer
• CandViewSelector
• CandidateVertexMerger
• ChainedJetCorrectorProducer
• ChargedRefCandidateProducer
• ConcreteChargedCandidateProducer
• CorrectedCaloJetProducer
• CorrectedPFJetProducer
• CosmicMuonSeedGenerator
• DTRecHitProducer
• DTRecSegment4DProducer
• DTuROSRawToDigi
• DeepFlavourJetTagsProducer
• DeepNNTagInfoProducer
• ESRecHitProducer
• EcalHaloDataProducer
• EcalRecalibRecHitProducer
• EtMinCaloJetSelector
• EtaRangeCaloJetSelector
• FixedGridRhoProducerFastjet
• GenericPFCandidateSelector
• GlobalHaloDataProducer
• HBHEPhase1Reconstructor
• HFPhase1Reconstructor
• HFPreReconstructor
• HcalHaloDataProducer
• HcalHitReconstructor
• JetCoreClusterSplitter
• JetTagProducer
• JetTracksAssociatorAtVertex
• L1FastjetCorrectorProducer
• L2MuonCandidateProducer
• L3MuonCandidateProducer
• L3MuonTrajectorySeedCombiner
• L3TrackCandCombiner
• L3TrackCombiner
• L3TrackLinksCombiner
• LXXXCorrectorProducer
• LargestEtCaloJetSelector
• LargestEtPFJetSelector
• LightPFTrackProducer
• MaskedMeasurementTrackerEventProducer
• MuonIdProducer
• MuonLinksProducer
• MuonLinksProducerForHLT
• PFBlockProducer
• PFClusterProducer
• PFJetSelector
• PFMultiDepthClusterProducer
• PFRecHitProducer
• ParameterSetBlobProducer
• PixelVertexProducer
• PrimaryVertexObjectFilter
• PrimaryVertexProducer
• RPCRecHitProducer
• RawDataCollectorByLabel
• RecoTrackRefSelector
• SecondaryVertexProducer
• SeedCombiner
• SeedGeneratorFromRegionHitsEDProducer
• SiPixelRecHitConverter
• SiStripClusterizer
• SiStripRawToDigiModule
• SiStripZeroSuppression
• SoftLepton
• TSGFromL2Muon
• TrackDeepNNTagInfoProducer
• TrackIPProducer
• TrackListMerger
• TrackProducer
• TrackWithVertexSelector
• VertexMerger
• VertexSelector

[makortel]

I'm fine with merging this PR.

[mmusich]

@Martin-Grunewald @fwyzard do you agree with the current wording as well?

[Martin-Grunewald]

It's ok with me!

[mmusich]

given the consensus above can this PR be merged?
I am not sure who is actually managing this repo? @cms-sw/orp-l2 ? @smuzaffar ?

[antoniovilela]

given the consensus above can this PR be merged?
I am not sure who is actually managing this repo? @cms-sw/orp-l2 ? @smuzaffar ?

@makortel @smuzaffar
Better for Matti & Shahzad to merge this.

[smuzaffar]

ok, merging it now

[Dr15Jones]

In addition, changing all addDefault() to addWithDefaultLabel() is trivial to try, and maybe it would not be too hard to figure out and fix what breaks.

I disagree with this idea, especially since we now have constructors in python for each module that uses a fillDescription (see cms-sw/cmssw#43955 )

The addDefault explicitly states that this is the validation to use for the module, with the other add calls allowing alternative default values. In fact, my recent PR uses the description in addDefault to be the basis for any other add call (i.e. the description in the other add will eventually be required to be compatible with addDefault). If there is one and only one add call then that is promoted to be default description. I'd like to change the code so that if there were multiple add calls than we'd require the addition of an explicit addDefault as well.

As for the ConfDB problem, I have a [RFC] PR which demonstrates how we can generated dummy CFI files based on the new constructor files which has the added bonus of injecting dummy values for types ConfDB can't handle. cms-sw/cmssw#44424. In this way we do not inject dummy default values into the C++ code which could lead to problems if those values are forgotten to be overridden in the python code. [Now if you forget to override them we have a failure during python processing rather than failure after we've run the job and someone notices the output of the job is wrong].

In this way we only generate the dummy cfi files temporarily when ConfDB wants to be updated and we do not keep those around (as the addWithDefaultLabel() would cause).

[fwyzard]

the description in the other add will eventually be required to be compatible with addDefault

This is simply wrong.

[fwyzard]

I disagree with this idea, especially since we now have constructors in python for each module that uses a fillDescription

I don't see why - in fact, everything you wrote seems completely orthogonal with respect to writing a cfi file or not.

[Dr15Jones]

the description in the other add will eventually be required to be compatible with addDefault

This is simply wrong.

As the designer of the system, I can unequivocally state that was always the design intent. The reason is as follows:

Say you have a generated cfi.py (e.g. foo_cfi.py) which creates a description incompatible with the default (regardless on if the default is from addDefault or is implicitly the case where the first add is always used as the default ) and then one does a clone from the cfi

from .foo_cfi import foo
bar = foo.clone()

as the label bar does not correspond to a specific add call in fillDescriptions, the configuration will be validated by the default and will fail. While using the label foo would pass because the explicit add related to foo is used to validate. This will happen even if the configuration information for foo and bar are identical!

This inconsistency in validation would be a very bad behavior. Hence why we want to move to a system where the default must be able to validate all autogenerated cfis.

[fwyzard]

This eliminates the possibility of having different, incompatible configurations - which the current system supports.

[Dr15Jones]

This eliminates the possibility of having different, incompatible configurations - which the current system supports.

I'm afraid I don't see how. Please elaborate?

[fwyzard]

As you wrote, the current system supports using a non-default validation for a specific label.
If all labels will need to be validated by the default function, this will stop working.

So, IIUC, you plan to change the design of the system from "support different configurations with different validators" to "support different configurations with a single validator".

[Dr15Jones]

Ah, I was confused by what the This in your sentence referenced in this long discussion, sorry about that.

So, IIUC, you plan to change the design of the system from "support different configurations with different validators" to "support different configurations with a single validator".

Nearly so. We still want to be able to allow add to change default values for parameters based on the module label, we just want to enforce that such changes are inline with the default validator. What we'd really like is to make it easy to base all explicitly labelled configuration as derivatives of the default one. That could enforce in the API a way to guarantee the proper relationship.

Now for the vast majority of modules, this point is mote as they declare 1 and only 1 entry to edm::ConfigurationDescriptions.