-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Add a mechanism to set the chosen case for SwitchProducer #35510
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35510/25705
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages:
@makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild, please test |
|
||
The decision can be "forced" with 'foo.setCase_("case1")' and | ||
unset with 'foo.setCase_(None)'. This setting persists through | ||
dumpPython() and picking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dumpPython() and picking. | |
dumpPython() and pickling. |
c = cms.int32(2) | ||
) | ||
) | ||
).setCase_('test1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumping the "set case" like this looks a bit weird (setCase_()
returns self
so it should work), but doing it along
foo = SwitchProducerTest(
...
)
foo.setCase_(...)
would require knowing the module label (which is not available here), or extending the functionality of specialImportRegistry
(would need to think more what exactly that would imply).
I also considered
SwitchProducerTest(
test1 = ...,
test2 = ....,
setCase_ = cms.untracked.string('test1')
)
but that appeared to require adding special cases for setCase_
in many places complicating the implementation even more. Also given that this setting should(?) be somewhat special case, and that it is not passed on to C++, making it a function call felt better choice than a "real parameter".
test parameters:
|
@cmsbuild, please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a9008a/19328/summary.html Comparison SummarySummary:
|
Closing in favor of #36699 |
PR description:
This PR attempts to resolve to add a mechanism to "force a chosen case" for a SwitchProducer following the idea in #31760 (comment).
Opening now as a RFC, because I'm not fully satisfied with the naming I came up, and I'm not fully convinced of the approach (not that I would have really better ideas).
PR validation:
Framework unit tests run.