From 13f351673297fbec2874ed25d8c2057901c0d51f Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Mon, 17 Apr 2023 12:39:17 -0500 Subject: [PATCH 1/7] Avoid unnecessary calls when assigning initial values from defaults --- FWCore/ParameterSet/python/Config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/FWCore/ParameterSet/python/Config.py b/FWCore/ParameterSet/python/Config.py index 300bd3eb76284..e911d8ec798df 100644 --- a/FWCore/ParameterSet/python/Config.py +++ b/FWCore/ParameterSet/python/Config.py @@ -422,9 +422,9 @@ def __setattr__(self,name,value): if not name.replace('_','').isalnum(): raise ValueError('The label '+name+' contains forbiden characters') - if name == 'options': + if name == 'options' and hasattr(self,name): value = self.__updateOptions(value) - if name == 'maxEvents': + if name == 'maxEvents' and hasattr(self,name): value = self.__updateMaxEvents(value) # private variable exempt from all this From 63b50d9484ef21dc1c2e2ef740b9280c7bbfcd6d Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Mon, 17 Apr 2023 12:47:59 -0500 Subject: [PATCH 2/7] Support container method forwarding in _ProxyParameter Needed to handle cms.[vV]* classes properly. Modifying the tests for InputTag search and replace uncovered this missing feature. --- FWCore/ParameterSet/python/MassReplace.py | 9 ++++++-- FWCore/ParameterSet/python/Types.py | 26 +++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/FWCore/ParameterSet/python/MassReplace.py b/FWCore/ParameterSet/python/MassReplace.py index a51e607aae2a7..8fceb80f51868 100644 --- a/FWCore/ParameterSet/python/MassReplace.py +++ b/FWCore/ParameterSet/python/MassReplace.py @@ -173,9 +173,12 @@ def testMassSearchReplaceAnyInputTag(self): ), ) p.op = cms.EDProducer("op", src = cms.optional.InputTag, unset = cms.optional.InputTag, vsrc = cms.optional.VInputTag, vunset = cms.optional.VInputTag) + p.op2 = cms.EDProducer("op2", src = cms.optional.InputTag, unset = cms.optional.InputTag, vsrc = cms.optional.VInputTag, vunset = cms.optional.VInputTag) p.op.src="b" - p.op.vsrc=cms.VInputTag("b") - p.s = cms.Sequence(p.a*p.b*p.c*p.sp*p.op) + p.op.vsrc = ["b"] + p.op2.src=cms.InputTag("b") + p.op2.vsrc = cms.VInputTag("b") + p.s = cms.Sequence(p.a*p.b*p.c*p.sp*p.op*p.op2) massSearchReplaceAnyInputTag(p.s, cms.InputTag("b"), cms.InputTag("new")) self.assertNotEqual(cms.InputTag("new"), p.b.src) self.assertEqual(cms.InputTag("new"), p.c.src) @@ -210,6 +213,8 @@ def testMassSearchReplaceAnyInputTag(self): self.assertEqual(cms.untracked.InputTag("new"), p.sp.test2.nested.usrc) self.assertEqual(cms.InputTag("new"), p.op.src) self.assertEqual(cms.InputTag("new"), p.op.vsrc[0]) + self.assertEqual(cms.InputTag("new"), p.op2.src) + self.assertEqual(cms.InputTag("new"), p.op2.vsrc[0]) def testMassReplaceInputTag(self): process1 = cms.Process("test") diff --git a/FWCore/ParameterSet/python/Types.py b/FWCore/ParameterSet/python/Types.py index e052b6c4ace59..bc65fab3fbac0 100644 --- a/FWCore/ParameterSet/python/Types.py +++ b/FWCore/ParameterSet/python/Types.py @@ -61,6 +61,25 @@ def __setattr__(self,name, value): if not name.startswith('_'): raise AttributeError("%r object has no attribute %r" % (self.__class__.__name__, name)) return object.__setattr__(self, name, value) + #support container like behavior + def __iter__(self): + v =self.__dict__.get('_ProxyParameter__value', None) + if v is not None: + return v.__iter__() + else: + raise TypeError("'_OptionalParameter' object is not iterable") + def __setitem__(self, key, value): + v =self.__dict__.get('_ProxyParameter__value', None) + if v is not None: + return v.__setitem__(key,value) + else: + raise TypeError("'_OptionalParameter' object does not support item assignment") + def __getitem__(self, key): + v =self.__dict__.get('_ProxyParameter__value', None) + if v is not None: + return v.__getitem__(key) + else: + raise TypeError("'_OptionalParameter' object is not subscriptable") def __bool__(self): v = self.__dict__.get('_ProxyParameter__value',None) return _builtin_bool(v) @@ -140,6 +159,13 @@ def __call__(self,value): if chosenType is None: raise RuntimeError("Cannot convert "+str(value)+" to 'allowed' type") return chosenType(value) + def _setValueWithType(self, valueWithType): + for t in self.__types: + if isinstance(valueWithType, t): + return valueWithType + raise TypeError("type {bad} is not one of 'allowed' types {types}".format(bad=str(type(valueWithType)), types = ",".join( (str(t) for t in self__types))) ) + + class _PSetTemplate(object): def __init__(self, *args, **kargs): From e7ecea1912d217cfe6655483de99edc4bb3afe86 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Mon, 17 Apr 2023 12:50:52 -0500 Subject: [PATCH 3/7] Enforce assignment with explicit type matches previous type In the python configuration, if a PSet parameter already has been assigned, then reassignment using an explicit type will only work if the types match. --- FWCore/ParameterSet/python/Mixins.py | 21 ++++++++++++++++++++- FWCore/ParameterSet/python/Types.py | 25 ++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/FWCore/ParameterSet/python/Mixins.py b/FWCore/ParameterSet/python/Mixins.py index 6f4eef29e3608..d55257bd299b5 100644 --- a/FWCore/ParameterSet/python/Mixins.py +++ b/FWCore/ParameterSet/python/Mixins.py @@ -85,6 +85,11 @@ def setIsFrozen(self): self._isFrozen = True def isCompatibleCMSType(self,aType): return isinstance(self,aType) + def _returnTypeWithValue(self, valueWithType): + if isinstance(valueWithType, type(self)): + return valueWithType + raise TypeError("Attempted to assign type {from_} to type {to}".format(from_ = str(type(valueWithType)), to = str(type(self))) ) + class _SimpleParameterTypeBase(_ParameterTypeBase): """base class for parameter classes which only hold a single value""" @@ -277,7 +282,7 @@ def __setattr__(self,name,value): else: # handle the case where users just replace with a value, a = 12, rather than a = cms.int32(12) if isinstance(value,_ParameterTypeBase): - self.__dict__[name] = value + self.__dict__[name] = self.__dict__[name]._returnTypeWithValue(value) else: self.__dict__[name].setValue(value) self._isModified = True @@ -938,5 +943,19 @@ def testSpecialImportRegistry(self): self.assertEqual(reg.getSpecialImports(), ["import foo"]) reg.registerUse("a") self.assertEqual(reg.getSpecialImports(), ["import bar", "import foo"]) + def testInvalidTypeChange(self): + class __Test(_TypedParameterizable): + pass + class __TestTypeA(_SimpleParameterTypeBase): + def _isValid(self,value): + return True + class __TestTypeB(_SimpleParameterTypeBase): + def _isValid(self,value): + return True + pass + a = __Test("MyType", + t=__TestTypeA(1)) + self.assertRaises(TypeError, lambda : setattr(a,'t',__TestTypeB(2))) + unittest.main() diff --git a/FWCore/ParameterSet/python/Types.py b/FWCore/ParameterSet/python/Types.py index bc65fab3fbac0..c91a2b3aab09c 100644 --- a/FWCore/ParameterSet/python/Types.py +++ b/FWCore/ParameterSet/python/Types.py @@ -45,6 +45,20 @@ def setValue(self, value): if not _ParameterTypeBase.isTracked(self): v = untracked(v) self.__dict__["_ProxyParameter__value"] = v + def _returnTypeWithValue(self, valueWithType): + if isinstance(valueWithType, type(self)): + return valueWithType + if isinstance(self.__type, type): + if isinstance(valueWithType, self.__type): + self.__dict__["_ProxyParameter__value"] = valueWithType + return self + else: + raise TypeError("type {bad} does not match {expected}".format(bad=str(type(valueWithType)), expected = str(self.__type))) + v = self.__type._setValueWithType(valueWithType) + if not _ParameterTypeBase.isTracked(self): + v = untracked(v) + self.__dict__["_ProxyParameter__value"] = v + return self def __getattr__(self, name): v =self.__dict__.get('_ProxyParameter__value', None) if name == '_ProxyParameter__value': @@ -163,7 +177,7 @@ def _setValueWithType(self, valueWithType): for t in self.__types: if isinstance(valueWithType, t): return valueWithType - raise TypeError("type {bad} is not one of 'allowed' types {types}".format(bad=str(type(valueWithType)), types = ",".join( (str(t) for t in self__types))) ) + raise TypeError("type {bad} is not one of 'allowed' types {types}".format(bad=str(type(valueWithType)), types = ",".join( (str(t) for t in self.__types))) ) @@ -1945,6 +1959,8 @@ def testRequired(self): p1.foo = dict(a=5) self.assertEqual(p1.dumpPython(),'cms.PSet(\n foo = cms.PSet(\n a = cms.int32(5)\n ),\n allowAnyLabel_=cms.required.PSetTemplate(\n a = cms.required.int32\n )\n)') self.assertEqual(p1.foo.a.value(), 5) + p1 = PSet(anInt = required.int32) + self.assertRaises(TypeError, lambda : setattr(p1,'anInt', uint32(2))) def testOptional(self): p1 = PSet(anInt = optional.int32) @@ -1996,6 +2012,9 @@ def testOptional(self): p1.foo = dict(a=5) self.assertEqual(p1.dumpPython(),'cms.PSet(\n foo = cms.PSet(\n a = cms.int32(5)\n ),\n allowAnyLabel_=cms.optional.PSetTemplate(\n a = cms.optional.int32\n )\n)') self.assertEqual(p1.foo.a.value(), 5) + #check wrong type failure + p1 = PSet(anInt = optional.int32) + self.assertRaises(TypeError, lambda : setattr(p1,'anInt', uint32(2))) def testAllowed(self): @@ -2063,6 +2082,10 @@ def testAllowed(self): p3 = PSet(aValue=optional.untracked.allowed(int32,uint32)) self.assertRaises(RuntimeError, lambda: setattr(p3, "aValue", 42)) + p1 = PSet(aValue = required.allowed(int32, string)) + self.assertRaises(TypeError, lambda : setattr(p1,'aValue', uint32(2))) + + def testVPSet(self): p1 = VPSet(PSet(anInt = int32(1)), PSet(anInt=int32(2))) self.assertEqual(len(p1),2) From 90a05d876492c08aee212dc20cc02bcace223f72 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Tue, 18 Apr 2023 13:56:05 -0500 Subject: [PATCH 4/7] Can specify a default for `allowed` configuration parameter In addition to specify the allowed types, one can now use the `default` keyword to specify a starting value. --- FWCore/ParameterSet/python/Mixins.py | 10 +- FWCore/ParameterSet/python/Types.py | 91 +++++++++++-------- .../producersLayer1/muonProducer_cfi.py | 2 +- 3 files changed, 59 insertions(+), 44 deletions(-) diff --git a/FWCore/ParameterSet/python/Mixins.py b/FWCore/ParameterSet/python/Mixins.py index d55257bd299b5..baec7d3c80807 100644 --- a/FWCore/ParameterSet/python/Mixins.py +++ b/FWCore/ParameterSet/python/Mixins.py @@ -584,7 +584,7 @@ def __init__(self,*arg,**args): super(_ValidatingListBase,self).__init__(arg) if 0 != len(args): raise SyntaxError("named arguments ("+','.join([x for x in args])+") passsed to "+str(type(self))) - if not self._isValid(iter(self)): + if not type(self)._isValid(iter(self)): raise TypeError("wrong types ("+','.join([str(type(value)) for value in iter(self)])+ ") added to "+str(type(self))) def __setitem__(self,key,value): @@ -595,12 +595,13 @@ def __setitem__(self,key,value): if not self._itemIsValid(value): raise TypeError("can not insert the type "+str(type(value))+" in container "+self._labelIfAny()) super(_ValidatingListBase,self).__setitem__(key,value) - def _isValid(self,seq): + @classmethod + def _isValid(cls,seq): # see if strings get reinterpreted as lists if isinstance(seq, str): return False for item in seq: - if not self._itemIsValid(item): + if not cls._itemIsValid(item): return False return True def _itemFromArgument(self, x): @@ -758,7 +759,8 @@ def _modifyParametersFromDict(params, newParams, errorRaiser, keyDepth=""): import unittest class TestList(_ValidatingParameterListBase): - def _itemIsValid(self,item): + @classmethod + def _itemIsValid(cls,item): return True class testMixins(unittest.TestCase): def testListConstruction(self): diff --git a/FWCore/ParameterSet/python/Types.py b/FWCore/ParameterSet/python/Types.py index c91a2b3aab09c..096575856e121 100644 --- a/FWCore/ParameterSet/python/Types.py +++ b/FWCore/ParameterSet/python/Types.py @@ -40,6 +40,8 @@ def __init__(self,type): super(_ProxyParameter,self).__init__() self.__dict__["_ProxyParameter__type"] = type self.__dict__["_ProxyParameter__value"] = None + if hasattr(self.__type,"_default") and self.__type._default is not None: + self.setValue(self.__type._default) def setValue(self, value): v = self.__type(value) if not _ParameterTypeBase.isTracked(self): @@ -81,19 +83,19 @@ def __iter__(self): if v is not None: return v.__iter__() else: - raise TypeError("'_OptionalParameter' object is not iterable") + raise TypeError("'_ProxyParameter' object is not iterable") def __setitem__(self, key, value): v =self.__dict__.get('_ProxyParameter__value', None) if v is not None: return v.__setitem__(key,value) else: - raise TypeError("'_OptionalParameter' object does not support item assignment") + raise TypeError("'_ProxyParameter' object does not support item assignment") def __getitem__(self, key): v =self.__dict__.get('_ProxyParameter__value', None) if v is not None: return v.__getitem__(key) else: - raise TypeError("'_OptionalParameter' object is not subscriptable") + raise TypeError("'_ProxyParameter' object is not subscriptable") def __bool__(self): v = self.__dict__.get('_ProxyParameter__value',None) return _builtin_bool(v) @@ -151,14 +153,13 @@ def _dumpPythonName(): return 'obsolete' class _AllowedParameterTypes(object): - def __init__(self, *args): + def __init__(self, *args, **kargs): self.__dict__['_AllowedParameterTypes__types'] = args - self.__dict__['_AllowedParameterTypes__value'] = None + self.__dict__['_default'] = None self.__dict__['__name__'] = self.dumpPython() + if len(kargs): + self.__dict__['_default'] = self._setValueWithType(kargs['default']) def dumpPython(self, options=PrintOptions()): - v =self.__dict__.get('_ProxyParameter__value',None) - if v is not None: - return v.dumpPython(options) specialImportRegistry.registerUse(self) return "allowed("+','.join( ("cms."+t.__name__ for t in self.__types))+')' def __call__(self,value): @@ -210,10 +211,10 @@ class _AllowedWrapper(object): def __init__(self, untracked, type): self.untracked = untracked self.type = type - def __call__(self, *args): + def __call__(self, *args, **kargs): if self.untracked: - return untracked(self.type(_AllowedParameterTypes(*args))) - return self.type(_AllowedParameterTypes(*args)) + return untracked(self.type(_AllowedParameterTypes(*args, **kargs))) + return self.type(_AllowedParameterTypes(*args, **kargs)) return _AllowedWrapper(self.__isUntracked, self.__type) if name == 'PSetTemplate': @@ -969,8 +970,8 @@ class vint32(_ValidatingParameterListBase): def __init__(self,*arg,**args): super(vint32,self).__init__(*arg,**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return int32._isValid(item) @staticmethod def _valueFromString(value): @@ -983,8 +984,8 @@ def insertInto(self, parameterSet, myname): class vuint32(_ValidatingParameterListBase): def __init__(self,*arg,**args): super(vuint32,self).__init__(*arg,**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return uint32._isValid(item) @staticmethod def _valueFromString(value): @@ -997,8 +998,8 @@ def insertInto(self, parameterSet, myname): class vint64(_ValidatingParameterListBase): def __init__(self,*arg,**args): super(vint64,self).__init__(*arg,**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return int64._isValid(item) @staticmethod def _valueFromString(value): @@ -1011,8 +1012,8 @@ def insertInto(self, parameterSet, myname): class vuint64(_ValidatingParameterListBase): def __init__(self,*arg,**args): super(vuint64,self).__init__(*arg,**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return uint64._isValid(item) @staticmethod def _valueFromString(value): @@ -1025,8 +1026,8 @@ def insertInto(self, parameterSet, myname): class vdouble(_ValidatingParameterListBase): def __init__(self,*arg,**args): super(vdouble,self).__init__(*arg,**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return double._isValid(item) @staticmethod def _valueFromString(value): @@ -1042,8 +1043,8 @@ def pythonValueForItem(self,item, options): class vbool(_ValidatingParameterListBase): def __init__(self,*arg,**args): super(vbool,self).__init__(*arg,**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return bool._isValid(item) @staticmethod def _valueFromString(value): @@ -1057,8 +1058,8 @@ class vstring(_ValidatingParameterListBase): def __init__(self,*arg,**args): super(vstring,self).__init__(*arg,**args) self._nPerLine = 1 - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return string._isValid(item) def configValueForItem(self,item,options): return string.formatValueForConfig(item) @@ -1071,8 +1072,8 @@ def insertInto(self, parameterSet, myname): class VLuminosityBlockID(_ValidatingParameterListBase): def __init__(self,*arg,**args): super(VLuminosityBlockID,self).__init__(*arg,**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return LuminosityBlockID._isValid(item) def configValueForItem(self,item,options): return LuminosityBlockID.formatValueForConfig(item) @@ -1099,8 +1100,8 @@ def __init__(self,*arg,**args): except TypeError: pass super(VInputTag,self).__init__((InputTag._stringFromArgument(x) for x in arg),**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return InputTag._isValid(item) def configValueForItem(self,item,options): # we tolerate strings as members @@ -1136,8 +1137,8 @@ def __init__(self,*arg,**args): except TypeError: pass super(VESInputTag,self).__init__((ESInputTag._stringFromArgument(x) for x in arg),**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return ESInputTag._isValid(item) def configValueForItem(self,item,options): # we tolerate strings as members @@ -1168,8 +1169,8 @@ def insertInto(self, parameterSet, myname): class VEventID(_ValidatingParameterListBase): def __init__(self,*arg,**args): super(VEventID,self).__init__(*arg,**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return EventID._isValid(item) def configValueForItem(self,item,options): return EventID.formatValueForConfig(item) @@ -1195,8 +1196,8 @@ def insertInto(self, parameterSet, myname): class VLuminosityBlockRange(_ValidatingParameterListBase): def __init__(self,*arg,**args): super(VLuminosityBlockRange,self).__init__(*arg,**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return LuminosityBlockRange._isValid(item) def configValueForItem(self,item,options): return LuminosityBlockRange.formatValueForConfig(item) @@ -1221,8 +1222,8 @@ def insertInto(self, parameterSet, myname): class VEventRange(_ValidatingParameterListBase): def __init__(self,*arg,**args): super(VEventRange,self).__init__(*arg,**args) - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return EventRange._isValid(item) def configValueForItem(self,item,options): return EventRange.formatValueForConfig(item) @@ -1248,8 +1249,8 @@ class VPSet(_ValidatingParameterListBase,_ConfigureComponent,_Labelable): def __init__(self,*arg,**args): super(VPSet,self).__init__(*arg,**args) self._nPerLine = 1 - @staticmethod - def _itemIsValid(item): + @classmethod + def _itemIsValid(cls,item): return isinstance(item, PSet) and PSet._isValid(item) def configValueForItem(self,item, options): return PSet.configValue(item, options) @@ -2032,6 +2033,14 @@ def testAllowed(self): self.assertTrue(p1.aValue.isCompatibleCMSType(string)) self.assertFalse(p1.aValue.isCompatibleCMSType(uint32)) + p1 = PSet(aValue = required.allowed(int32, string, default=int32(3))) + self.assertEqual(p1.dumpPython(),'cms.PSet(\n aValue = cms.int32(3)\n)') + p1.aValue = "foo" + self.assertEqual(p1.dumpPython(),"cms.PSet(\n aValue = cms.string('foo')\n)") + + p1 = PSet(aValue = required.untracked.allowed(int32, string, default=int32(3))) + self.assertEqual(p1.dumpPython(),'cms.PSet(\n aValue = cms.untracked.int32(3)\n)') + p1 = PSet(aValue = required.untracked.allowed(int32, string)) self.assertEqual(p1.dumpPython(),'cms.PSet(\n aValue = cms.required.untracked.allowed(cms.int32,cms.string)\n)') p1.aValue = 1 @@ -2085,7 +2094,11 @@ def testAllowed(self): p1 = PSet(aValue = required.allowed(int32, string)) self.assertRaises(TypeError, lambda : setattr(p1,'aValue', uint32(2))) + p1 = PSet(aValue = required.allowed(InputTag, VInputTag, default=InputTag("blah"))) + p1.aValue.setValue("foo") + self.assertEqual(p1.aValue.value(), "foo") + def testVPSet(self): p1 = VPSet(PSet(anInt = int32(1)), PSet(anInt=int32(2))) self.assertEqual(len(p1),2) diff --git a/PhysicsTools/PatAlgos/python/producersLayer1/muonProducer_cfi.py b/PhysicsTools/PatAlgos/python/producersLayer1/muonProducer_cfi.py index eb562b4b51340..b2772de198649 100644 --- a/PhysicsTools/PatAlgos/python/producersLayer1/muonProducer_cfi.py +++ b/PhysicsTools/PatAlgos/python/producersLayer1/muonProducer_cfi.py @@ -77,7 +77,7 @@ # mc matching addGenMatch = cms.bool(True), embedGenMatch = cms.bool(True), - genParticleMatch = cms.InputTag("muonMatch"), ## particles source to be used for the matching + genParticleMatch = cms.required.allowed(cms.InputTag, cms.VInputTag, default = cms.InputTag("muonMatch")), ## particles source to be used for the matching # efficiencies addEfficiencies = cms.bool(False), From 3cd4a9bc2e049bf4effe112814484defccae47ed Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Tue, 18 Apr 2023 14:45:25 -0500 Subject: [PATCH 5/7] Remove explicit use of type when resetting parameter in ECALHCAL.py The type used was incorrect (a cms.string instead of a cms.InputTag). Removing the type altogether should help long term maintenance. --- Validation/Configuration/python/ECALHCAL.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Validation/Configuration/python/ECALHCAL.py b/Validation/Configuration/python/ECALHCAL.py index c23cd4db1433e..4f7d073fb02c0 100644 --- a/Validation/Configuration/python/ECALHCAL.py +++ b/Validation/Configuration/python/ECALHCAL.py @@ -24,7 +24,7 @@ def customise(process): # use directly the generator output, no Hector - process.g4SimHits.Generator.HepMCProductLabel = cms.string('generatorSmeared') + process.g4SimHits.Generator.HepMCProductLabel = 'generatorSmeared' # modify the content From b4e33be482c31cd8b45e1749207e72b62ed19aed Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 19 Apr 2023 08:03:10 -0500 Subject: [PATCH 6/7] Force check in C++ for wrong type unit test Removing the parameter then adding it back avoids checking the new values type in python. --- FWCore/Framework/test/run_wrongOptionsType.sh | 1 - FWCore/Framework/test/test_wrongOptionsType_cfg.py | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/FWCore/Framework/test/run_wrongOptionsType.sh b/FWCore/Framework/test/run_wrongOptionsType.sh index dffe4a34c84b5..962fcb93d3fa0 100755 --- a/FWCore/Framework/test/run_wrongOptionsType.sh +++ b/FWCore/Framework/test/run_wrongOptionsType.sh @@ -7,4 +7,3 @@ function die { echo $1: status $2 ; echo === Log file === ; cat ${3:-/dev/null} cmsRun ${SCRAM_TEST_PATH}/test_wrongOptionsType_cfg.py -- --name=${NAME} --value="$VALUE" > ${NAME}.log 2>&1 && die "cmsRun for ${NAME} succeeded, should have failed" 1 ${NAME}.log grep -E "(The type in the configuration is incorrect)|(ValueError type of .* is expected to be .* but declared as)" ${NAME}.log > /dev/null || die "cmsRun for ${NAME} failed for other reason than incorrect configuration type" $? ${NAME}.log - diff --git a/FWCore/Framework/test/test_wrongOptionsType_cfg.py b/FWCore/Framework/test/test_wrongOptionsType_cfg.py index e2bcaad4c6134..4bd2c25a8c9c6 100644 --- a/FWCore/Framework/test/test_wrongOptionsType_cfg.py +++ b/FWCore/Framework/test/test_wrongOptionsType_cfg.py @@ -17,5 +17,6 @@ process.source = cms.Source("EmptySource") process.maxEvents.input = 2 - +#avoid type check in python to force check in C++ +delattr(process.options, args.name) setattr(process.options, args.name, eval(str(args.value))) From e081f54665aec442407075778edebe8c7cd39e8d Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 24 Apr 2023 14:45:16 -0500 Subject: [PATCH 7/7] Changed based on review comments - explicitly used named parameter - simplify a unit test --- FWCore/ParameterSet/python/Config.py | 21 ++++++++++++++++----- FWCore/ParameterSet/python/Mixins.py | 4 ++-- FWCore/ParameterSet/python/Types.py | 10 +++++----- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/FWCore/ParameterSet/python/Config.py b/FWCore/ParameterSet/python/Config.py index e911d8ec798df..9ff23bfb8d755 100644 --- a/FWCore/ParameterSet/python/Config.py +++ b/FWCore/ParameterSet/python/Config.py @@ -150,8 +150,8 @@ def __init__(self,name,*Mods): self.__isStrict = False self.__dict__['_Process__modifiers'] = Mods self.__dict__['_Process__accelerators'] = {} - self.options = Process.defaultOptions_() - self.maxEvents = Process.defaultMaxEvents_() + self.__injectValidValue('options', Process.defaultOptions_()) + self.__injectValidValue('maxEvents', Process.defaultMaxEvents_()) self.maxLuminosityBlocks = Process.defaultMaxLuminosityBlocks_() # intentionally not cloned to ensure that everyone taking # MessageLogger still via @@ -422,9 +422,9 @@ def __setattr__(self,name,value): if not name.replace('_','').isalnum(): raise ValueError('The label '+name+' contains forbiden characters') - if name == 'options' and hasattr(self,name): + if name == 'options': value = self.__updateOptions(value) - if name == 'maxEvents' and hasattr(self,name): + if name == 'maxEvents': value = self.__updateMaxEvents(value) # private variable exempt from all this @@ -557,6 +557,10 @@ def __setattr__(self,name,value): self._replaceInScheduleDirectly(name, newValue) self._delattrFromSetattr(name) + self.__injectValidValue(name, value, newValue) + def __injectValidValue(self, name, value, newValue = None): + if newValue is None: + newValue = value self.__dict__[name]=newValue if isinstance(newValue,_Labelable): self.__setObjectLabel(newValue, name) @@ -3600,6 +3604,10 @@ def testOptions(self): numberOfThreads =2) self.assertEqual(p.options.numberOfThreads.value(),2) self.assertEqual(p.options.numberOfStreams.value(),2) + del p.options + self.assertRaises(TypeError, setattr, p, 'options', untracked.PSet(numberOfThreads = int32(-1))) + p.options = untracked.PSet(numberOfThreads = untracked.uint32(4)) + self.assertEqual(p.options.numberOfThreads.value(), 4) def testMaxEvents(self): p = Process("Test") @@ -3614,7 +3622,10 @@ def testMaxEvents(self): p = Process("Test") p.maxEvents = untracked.PSet(input = untracked.int32(5)) self.assertEqual(p.maxEvents.input.value(), 5) - + del p.maxEvents + self.assertRaises(TypeError, setattr, p, 'maxEvents', untracked.PSet(input = untracked.uint32(1))) + p.maxEvents = untracked.PSet(input = untracked.int32(1)) + self.assertEqual(p.maxEvents.input.value(), 1) def testExamples(self): p = Process("Test") diff --git a/FWCore/ParameterSet/python/Mixins.py b/FWCore/ParameterSet/python/Mixins.py index baec7d3c80807..a09fa7ea0d792 100644 --- a/FWCore/ParameterSet/python/Mixins.py +++ b/FWCore/ParameterSet/python/Mixins.py @@ -85,7 +85,7 @@ def setIsFrozen(self): self._isFrozen = True def isCompatibleCMSType(self,aType): return isinstance(self,aType) - def _returnTypeWithValue(self, valueWithType): + def _checkAndReturnValueWithType(self, valueWithType): if isinstance(valueWithType, type(self)): return valueWithType raise TypeError("Attempted to assign type {from_} to type {to}".format(from_ = str(type(valueWithType)), to = str(type(self))) ) @@ -282,7 +282,7 @@ def __setattr__(self,name,value): else: # handle the case where users just replace with a value, a = 12, rather than a = cms.int32(12) if isinstance(value,_ParameterTypeBase): - self.__dict__[name] = self.__dict__[name]._returnTypeWithValue(value) + self.__dict__[name] = self.__dict__[name]._checkAndReturnValueWithType(value) else: self.__dict__[name].setValue(value) self._isModified = True diff --git a/FWCore/ParameterSet/python/Types.py b/FWCore/ParameterSet/python/Types.py index 096575856e121..7b3aff64efb9d 100644 --- a/FWCore/ParameterSet/python/Types.py +++ b/FWCore/ParameterSet/python/Types.py @@ -47,7 +47,7 @@ def setValue(self, value): if not _ParameterTypeBase.isTracked(self): v = untracked(v) self.__dict__["_ProxyParameter__value"] = v - def _returnTypeWithValue(self, valueWithType): + def _checkAndReturnValueWithType(self, valueWithType): if isinstance(valueWithType, type(self)): return valueWithType if isinstance(self.__type, type): @@ -153,12 +153,12 @@ def _dumpPythonName(): return 'obsolete' class _AllowedParameterTypes(object): - def __init__(self, *args, **kargs): + def __init__(self, *args, default=None): self.__dict__['_AllowedParameterTypes__types'] = args self.__dict__['_default'] = None self.__dict__['__name__'] = self.dumpPython() - if len(kargs): - self.__dict__['_default'] = self._setValueWithType(kargs['default']) + if default is not None: + self.__dict__['_default'] = self._setValueWithType(default) def dumpPython(self, options=PrintOptions()): specialImportRegistry.registerUse(self) return "allowed("+','.join( ("cms."+t.__name__ for t in self.__types))+')' @@ -1961,7 +1961,7 @@ def testRequired(self): self.assertEqual(p1.dumpPython(),'cms.PSet(\n foo = cms.PSet(\n a = cms.int32(5)\n ),\n allowAnyLabel_=cms.required.PSetTemplate(\n a = cms.required.int32\n )\n)') self.assertEqual(p1.foo.a.value(), 5) p1 = PSet(anInt = required.int32) - self.assertRaises(TypeError, lambda : setattr(p1,'anInt', uint32(2))) + self.assertRaises(TypeError, setattr, p1,'anInt', uint32(2)) def testOptional(self): p1 = PSet(anInt = optional.int32)