cms.allowed now accepts a dict when assigning value #44002
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44002/38922
|
|
A new Pull Request was created by @Dr15Jones for master. It involves the following packages:
@cmsbuild, @smuzaffar, @makortel, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
FWCore/ParameterSet/python/Types.py
Outdated
| @@ -109,7 +109,7 @@ def dumpPython(self, options=PrintOptions()): | |||
| v+=".untracked" | |||
| if hasattr(self.__type, "__name__"): | |||
| return v+'.'+self.__type.__name__ | |||
| return v+'.'+self.__type.dumpPython(options) | |||
| return v+'.'+self.__type.dumpPython() | |||
There was a problem hiding this comment.
Why not passing options anymore? Or was this causing the extra indentation that is now removed (as visible in the tests below)?
There was a problem hiding this comment.
It removes the extra indention.
There was a problem hiding this comment.
After seeing the additional failures of unit tests, I determined we do want the extra indents.
| self.assertEqual(p2.aValue.b.value(),3) | ||
| self.assertEqual(p2.dumpPython(), 'cms.PSet(\n aValue = cms.PSet(\n b = cms.int32(3)\n )\n)') | ||
| p2 = PSet(aValue=optional.allowed(int32,PSetTemplate(b=optional.int32))) | ||
| self.assertRaises(TypeError, lambda: setattr(p2, "aValue", dict(i = 1)) ) |
There was a problem hiding this comment.
Could you add a tests for
# Assign something not int32 nor PSet, e.g.
p2.aValue = double(3.14)
# Assign non-int32 inside the PSet, e.g.
p2.aValue = PSet(b = double(3.14))?
Such assignments should raise an Exception, right?
There was a problem hiding this comment.
The assertRaises() could be also
| self.assertRaises(TypeError, lambda: setattr(p2, "aValue", dict(i = 1)) ) | |
| self.assertRaises(TypeError, setattr, p2, "aValue", dict(i = 1) ) |
(I don't have strong preferences though)
There was a problem hiding this comment.
p2.aValue = PSet(b=double(3.14))does not throw. I think that is expected for a PSetTemplate since the code explicitly states the type and we assume that if you do that you know what you intended (e.g. the C++ code might allow multiple types).
p2.aValue = dict(b = 3.14)does throw as the value doesn't match for an int type.
There was a problem hiding this comment.
```python p2.aValue = PSet(b=double(3.14))does not throw. I think that is expected for a PSetTemplate since the code explicitly states the type and we assume that if you do that you know what you intended (e.g. the C++ code might allow multiple types).
Ok, makes sense. Would it be worth of adding such test anyway to demonstrate the behavior in that case?
Dr15Jones
force-pushed
the
allowedWithDict
branch
from
1b62b95
to
016d0c5
Compare
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-started
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44002/38924
|
|
Pull request #44002 was updated. @Dr15Jones, @cmsbuild, @smuzaffar, @makortel can you please check and sign again. |
|
please test |
|
-1 Failed Tests: UnitTests The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see more details here: Unit TestsI found 2 errors in the following unit tests: ---> test TestFWCoreParameterSetDriver had ERRORS ---> test testFWCoreParameterSetEdmConfigDump had ERRORS Comparison SummarySummary:
|
Extended code to also allow PSetTemplate in a cms.allowed statement.
Dr15Jones
force-pushed
the
allowedWithDict
branch
from
016d0c5
to
04b0d48
Compare
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-rejected
code-checks-approved
labels
|
please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44002/38937
|
|
Pull request #44002 was updated. @Dr15Jones, @smuzaffar, @makortel can you please check and sign again. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fd2edc/37550/summary.html Comparison SummarySummary:
|
|
+core |
cmsbuild
added
core-approved
fully-signed
and removed
core-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 now be reviewed by the release team before it's merged. @sextonkennedy, @rappoccio, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
Extended code to also allow PSetTemplate in a cms.allowed statement.
PR validation:
Python unit test passes.