Optional python parameters #27191
Optional python parameters #27191
Conversation
In a PSet one can now associate a label and a type without specifying a default value. - required: if the value is never set, an exception will be thrown at conversion to C++ time - optional: nothing happens if the value is never set This ability is now used to set values for the Process options, maxEvents and maxLuminosityBlock top level PSets The new options are injected by fillDescriptions. If more than one type is allowed for a given label, the .allowed mechanism can be used with optional or required. If any label would be allowed, used the new PSet label 'allowAnyLabel_' and associate a required or optional parameter.
The standard top level PSets options, maxEvents and maxLuminosityBlocks are now properly injected into the Process.
A dict can now be passed to PSet.setValue (which is also called during assignment). The key, value pairs in the dict are then iterated over and used to call assignment on the corresponding parameter labels within the PSet. If the PSet contains other parameters than those with keys in the dictionary, those other parameters are not changed.
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27191/10355
|
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages: FWCore/Integration @cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
What do the generated files look like ? |
Only used if no default given. Else they do exactly what we've done in the past.
eventIDFilter = cms.EDFilter('ModuloEventIDFilter',
modulo = cms.required.uint32,
offset = cms.required.uint32,
mightGet = cms.optional.untracked.vstring
)where previously it was eventIDFilter = cms.EDFilter('ModuloEventIDFilter'
)and the edmOutput = cms.OutputModule('PoolOutputModule',
fileName = cms.required.untracked.string,
logicalFileName = cms.untracked.string(''),
catalog = cms.untracked.string(''),
maxSize = cms.untracked.int32(2130706432),
compressionLevel = cms.untracked.int32(9),
compressionAlgorithm = cms.untracked.string('ZLIB'),
basketSize = cms.untracked.int32(16384),
eventAutoFlushCompressedSize = cms.untracked.int32(20971520),
splitLevel = cms.untracked.int32(99),
sortBaskets = cms.untracked.string('sortbasketsbyoffset'),
treeMaxVirtualSize = cms.untracked.int32(-1),
fastCloning = cms.untracked.bool(True),
overrideInputFileSplitLevels = cms.untracked.bool(False),
writeStatusFile = cms.untracked.bool(False),
dropMetaData = cms.untracked.string(''),
dataset = cms.untracked.PSet(),
overrideBranchesSplitLevel = cms.untracked.VPSet(
),
outputCommands = cms.untracked.vstring('keep *'),
SelectEvents = cms.untracked.PSet(
SelectEvents = cms.optional.vstring
)
)where |
|
-1 Tested at: bdee71c You can see the results of the tests here: I found follow errors while testing this PR Failed tests: UnitTests RelVals AddOn
I found errors in the following unit tests: ---> test testSSTGainPCL_fromRECO had ERRORS
When I ran the RelVals I found an error in the following workflows: runTheMatrix-results/4.53_RunPhoton2012B+RunPhoton2012B+HLTD+RECODR1reHLT+HARVESTDR1reHLT/step2_RunPhoton2012B+RunPhoton2012B+HLTD+RECODR1reHLT+HARVESTDR1reHLT.log7.3 step1 runTheMatrix-results/7.3_CosmicsSPLoose_UP18+CosmicsSPLoose_UP18+DIGICOS_UP18+RECOCOS_UP18+ALCACOS_UP18+HARVESTCOS_UP18/step1_CosmicsSPLoose_UP18+CosmicsSPLoose_UP18+DIGICOS_UP18+RECOCOS_UP18+ALCACOS_UP18+HARVESTCOS_UP18.log8.0 step1 runTheMatrix-results/8.0_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS/step1_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS.log9.0 step1 runTheMatrix-results/9.0_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST/step1_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST.log25.0 step1 runTheMatrix-results/25.0_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT/step1_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT.log136.731 step2 runTheMatrix-results/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2/step2_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2.log136.788 step2 runTheMatrix-results/136.788_RunSinglePh2017B+RunSinglePh2017B+HLTDR2_2017+RECODR2_2017reHLT_skimSinglePh_Prompt+HARVEST2017/step2_RunSinglePh2017B+RunSinglePh2017B+HLTDR2_2017+RECODR2_2017reHLT_skimSinglePh_Prompt+HARVEST2017.log4.22 step2 runTheMatrix-results/4.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC/step2_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC.log158.0 step1 runTheMatrix-results/158.0_HydjetQ_B12_5020GeV_2018_ppReco+HydjetQ_B12_5020GeV_2018_ppReco+DIGIHI2018PPRECO+RECOHI2018PPRECO+ALCARECOHI2018PPRECO+HARVESTHI2018PPRECO/step1_HydjetQ_B12_5020GeV_2018_ppReco+HydjetQ_B12_5020GeV_2018_ppReco+DIGIHI2018PPRECO+RECOHI2018PPRECO+ALCARECOHI2018PPRECO+HARVESTHI2018PPRECO.log1306.0 step1 runTheMatrix-results/1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15/step1_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15.log5.1 step1 runTheMatrix-results/5.1_TTbar+TTbarFS+HARVESTFS/step1_TTbar+TTbarFS+HARVESTFS.log1330.0 step1 runTheMatrix-results/1330.0_ZMM_13+ZMM_13+DIGIUP15+RECOUP15_L1TMuDQM+HARVESTUP15_L1TMuDQM/step1_ZMM_13+ZMM_13+DIGIUP15+RECOUP15_L1TMuDQM+HARVESTUP15_L1TMuDQM.log25202.0 step1 runTheMatrix-results/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/step1_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25.log135.4 step1 runTheMatrix-results/135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step1_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log10024.0 step1 runTheMatrix-results/10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017/step1_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017.log10042.0 step1 runTheMatrix-results/10042.0_ZMM_13+ZMM_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017/step1_ZMM_13+ZMM_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017.log10224.0 step1 runTheMatrix-results/10224.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017PU_GenSimFull+DigiFullPU_2017PU+RecoFullPU_2017PU+HARVESTFullPU_2017PU/step1_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017PU_GenSimFull+DigiFullPU_2017PU+RecoFullPU_2017PU+HARVESTFullPU_2017PU.log10824.0 step1 runTheMatrix-results/10824.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+HARVESTFull_2018+ALCAFull_2018+NanoFull_2018/step1_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+HARVESTFull_2018+ALCAFull_2018+NanoFull_2018.log101.0 step1 runTheMatrix-results/101.0_SingleElectronE120EHCAL+SingleElectronE120EHCAL/step1_SingleElectronE120EHCAL+SingleElectronE120EHCAL.log136.85 step2 runTheMatrix-results/136.85_RunEGamma2018A+RunEGamma2018A+HLTDR2_2018+RECODR2_2018reHLT_skimEGamma_Offline_L1TEgDQM+HARVEST2018_L1TEgDQM/step2_RunEGamma2018A+RunEGamma2018A+HLTDR2_2018+RECODR2_2018reHLT_skimEGamma_Offline_L1TEgDQM+HARVEST2018_L1TEgDQM.log11624.0 step1 runTheMatrix-results/11624.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2021_GenSimFull+DigiFull_2021+RecoFull_2021+HARVESTFull_2021+ALCAFull_2021/step1_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2021_GenSimFull+DigiFull_2021+RecoFull_2021+HARVESTFull_2021+ALCAFull_2021.log20034.0 step1 runTheMatrix-results/20034.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D17_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D17+RecoFullGlobal_2023D17+HARVESTFullGlobal_2023D17/step1_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D17_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D17+RecoFullGlobal_2023D17+HARVESTFullGlobal_2023D17.log21234.0 step1 runTheMatrix-results/21234.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D21_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D21+RecoFullGlobal_2023D21+HARVESTFullGlobal_2023D21/step1_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D21_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D21+RecoFullGlobal_2023D21+HARVESTFullGlobal_2023D21.log27434.0 step1 runTheMatrix-results/27434.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D35_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D35+RecoFullGlobal_2023D35+HARVESTFullGlobal_2023D35/step1_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D35_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D35+RecoFullGlobal_2023D35+HARVESTFullGlobal_2023D35.log29034.0 step1 runTheMatrix-results/29034.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D41_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D41+RecoFullGlobal_2023D41+HARVESTFullGlobal_2023D41/step1_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D41_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D41+RecoFullGlobal_2023D41+HARVESTFullGlobal_2023D41.log250202.181 step1 runTheMatrix-results/250202.181_TTbar_13UP18+TTbar_13UP18+PREMIXUP18_PU25+DIGIPRMXLOCALUP18_PU25+RECOPRMXUP18_PU25+HARVESTUP18_PU25/step1_TTbar_13UP18+TTbar_13UP18+PREMIXUP18_PU25+DIGIPRMXLOCALUP18_PU25+RECOPRMXUP18_PU25+HARVESTUP18_PU25.log140.53 step2 runTheMatrix-results/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/step2_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI.log140.56 step2 runTheMatrix-results/140.56_RunHI2018+RunHI2018+RECOHID18+HARVESTDHI18/step2_RunHI2018+RunHI2018+RECOHID18+HARVESTDHI18.log1000.0 step2 runTheMatrix-results/1000.0_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT/step2_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT.log1001.0 step2 runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVDSIPIXELCALRUN1+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4+ALCAHARVD5/step2_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVDSIPIXELCALRUN1+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4+ALCAHARVD5.log1325.7 step2 runTheMatrix-results/1325.7_TTbar_13_94Xv2NanoAODINPUT+TTbar_13_94Xv2NanoAODINPUT+NANOEDMMC2017_94XMiniAODv2+HARVESTNANOAODMC2017_94XMiniAODv2/step2_TTbar_13_94Xv2NanoAODINPUT+TTbar_13_94Xv2NanoAODINPUT+NANOEDMMC2017_94XMiniAODv2+HARVESTNANOAODMC2017_94XMiniAODv2.log136.8311 step2 runTheMatrix-results/136.8311_RunJetHT2017F_reminiaod+RunJetHT2017F_reminiaod+REMINIAOD_data2017+HARVEST2017_REMINIAOD_data2017/step2_RunJetHT2017F_reminiaod+RunJetHT2017F_reminiaod+REMINIAOD_data2017+HARVEST2017_REMINIAOD_data2017.log136.7611 step2 runTheMatrix-results/136.7611_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM/step2_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM.log
I found errors in the following addon tests: cmsDriver.py TTbar_8TeV_TuneCUETP8M1_cfi --conditions auto:run1_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot Realistic8TeVCollision : FAILED - time: date Wed Jun 12 20:45:22 2019-date Wed Jun 12 20:45:14 2019 s - exit: 256 |
|
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
cmsbuild
added
comparison-notrun
tests-rejected
and removed
comparison-pending
tests-started
labels
|
I wonder why cmsDriver now generates process.maxEvents = cms.untracked.PSet(
input = cms.optional.untracked.int32,
output = cms.optional.untracked..allowed(cms.int32,cms.PSet)
)( and process.options = cms.untracked.PSet(
FailPath = cms.untracked.vstring(),
IgnoreCompletely = cms.untracked.vstring(),
Rethrow = cms.untracked.vstring(),
SkipEvent = cms.untracked.vstring(),
allowUnscheduled = cms.obsolete.untracked.bool,
canDeleteEarly = cms.untracked.vstring(),
emptyRunLumiMode = cms.obsolete.untracked.string,
fileMode = cms.untracked.string('FULLMERGE'),
forceEventSetupCacheClearOnNewRun = cms.untracked.bool(False),
makeTriggerResults = cms.obsolete.untracked.bool,
numberOfConcurrentLuminosityBlocks = cms.untracked.uint32(1),
numberOfConcurrentRuns = cms.untracked.uint32(1),
numberOfStreams = cms.untracked.uint32(0),
numberOfThreads = cms.untracked.uint32(1),
printDependencies = cms.untracked.bool(False),
sizeOfStackForThreadsInKB = cms.optional.untracked.uint32,
throwIfIllegalParameter = cms.untracked.bool(True),
wantSummary = cms.untracked.bool(False)
)Maybe related to ConfigBuilder building both string and python object presentations of the config and somehow using pieces of the latter? |
|
@makortel I've now extended the tests in FWCore.ParameterSet.Types to also test untracked.allowed. It now sees the problem. Working on a fix. |
|
+operations |
|
+1 |
|
merge @santocch the PhysicsTools part looks ok to me, it basically update the definition of an assertEqual. Please check and comment in case for further updates |
|
+1 |
cmsbuild
added
analysis-approved
fully-signed
and removed
analysis-pending
pending-signatures
labels
|
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 be automatically merged. |
|
@Dr15Jones |
|
@slava77 https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideAboutPythonConfigFile needs to be updated with the changes. |
|
@slava77 I've updated the documentation on that page |
|
hi @Dr15Jones - looks like this creates problems w/ python3 An exception of category 'ConfigFileReadError' occurred while At: [i'm not sure I understand the actual cause yet] |
|
@davidlange6 I believe this is the underlying cause https://stackoverflow.com/questions/1087135/boolean-value-of-objects-in-python/1087166#1087166 _OptionalParameter defines nonzero which is used by python 2, but python 3 renamed the function to bool. |
|
@davidlange6 fixed in #27399 |
|
Sorry but I need to come back to this as it seems the added functionality is incompatible with ConfDB. I assume OBSOLETE parameters could simply be ignored and not recorded with the module during parsing, REQUIRED parameters should always have an accompanied default value emitted into the cfi file. As we know, parameters without a value do not get emitted (via fillDescriptions into cfi files) and hence ConfDB will not know about them (as it uses cfi files to discover them) and hence they are simply not available for module configs in HLT menus in ConfDB. OPTIONAL parameters also seem completely unhandlebar in ConfDB parsing in case the C++ code I am unclear how to proceed and what concrete problem is actually solved by this PR, thus whether it could be done differently and compatible with ConfDB parsing. |
|
On Wed, 17 Jul 2019, 23:15 Martin Grunewald, ***@***.***> wrote:
@Dr15Jones <https://github.com/Dr15Jones> @fabiocos
<https://github.com/fabiocos>
Sorry but I need to come back to this as it seems the added functionality
is incompatible with ConfDB.
In ConfDB, we store one fixed set of top-level plugin parameters for each
plugin class. Each parameter needs an example value (even if it is a fake
value) in cfi files used to parse this during ConfDB parsing of a new
release.
I assume OBSOLETE parameters could simply be ignored and not recorded with
the module during parsing,
REQUIRED parameters should always have an accompanied default value
emitted into the cfi file. As we know, parameters without a value do not
get emitted (via fillDescriptions into cfi files) and hence ConfDB will not
know about them (as it uses cfi files to discover them) and hence they are
simply not available for module configs in HLT menus in ConfDB.
OPTIONAL parameters also seem completely unhandlebar in ConfDB parsing in
case the C++ code
checks the existence and then does different things depending on the
existance of the parameter
(and what else would be the use case??)
This is because for ConfDB the number/type/list of parameters is fixed, so
we can not allow
for varying number of parameters (at least for top-level parameters,
varying sets of parameters are
allowed inside PSet parameters).
I am unclear how to proceed and what concrete problem is actually solved
by this PR, thus whether it could be done differently and compatible with
ConfDB parsing.
I guess it's finally time for ConfDB parsing to stop being based on the
generated python files, and move to something more robust.
For example, the parameter set validation code could outout a
machine-readable (e.g json ?) representation of the allowed rules and
values; a parser could read those a populate the database accordingly.
Of course this requires support in the parser, database, gui, and
extraction tool.
So, good news that we may have a dedicated developer again... bad news that
it will be only towards the end of the year.
.A
… |
If all the parameters for all HLT used modules already have default values, then you will see no difference with this change. Also, hand written cfi files shouldn't contain any of these new types, just ones autogenerated by
Yes. There is no facility to add_ obsolete_ parameters using fillDescriptions so they can only be used in handwritten configurations. Therefore no module descriptions are making use of this. It was only added to accomodate top level PSets used by the framework where we want to validate at python time but some parameters (like
Now you have a way to discover if defaults were never given since a required will appear in the python. You now have a way of checking which you didn't before.
Again, you can now discover cases where this is being done and report the problem to the developers. If ConfDB had not previously known about these parameter values it can just skip them since they were always optional and that is what ConfDB was doing before.
If module writes already followed the HLT policy of having default values in the fillDescriptions, nothing changes. If they had not, now you can catch the problems. This pull request did not change how developers make use of the ParameterSet system nor gave any new abilities in the C++. Developers have always been able to do required and optional parameters on the C++ side. What this pull request does is expose those on the python side in order to allow
|
|
But I assume there is nothing to prevent someone from handwriting a cfi file with these new variants? Yes, we discover it, but how? How does a REQUIRED without value look in the cfi file? And if it is there it needs to be fixed, so why allow it in the first place if it creates problems later? Also for OPTIONAL: we spend a long time educating HLT authors not to change physics on the presence or not of parameters, and now you put back an explicit accommodation for this, so |
PR description:
Added the ability to declare parameters in PSets, EDProducers, etc without giving actual values to those parameters. The three types are
required: if not set will throw a python exception during serialization to C++optional: if not set the parameter will not be serialized to C++obsolete: parameter is never serialzed to C++This allows the following code
This facility is now used to define the standard top level PSets of
cms.Process: options, maxEvents and maxLuminosityBlocks.If more than one type is allowed for a given PSet label, one can use the
allowedmechanism to specify the types:To make it easier to modify multiple parameters within a PSet, one can now use a
dictto modify a given set of parameters within a PSet:On the C++ side, the
fillDescriptionssystem was updated to generatecms.requiredandcms.optionalentries in the_cfi.pyfiles.PR validation:
All framework related tests pass.