Alternative module configuration syntax #43955
Conversation
|
cms-bot internal usage |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43955/38840
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
Dr15Jones
force-pushed
the
altModuleConfig
branch
from
c687847
to
dbd80eb
Compare
|
please test |
cmsbuild
added
tests-started
code-checks-pending
and removed
tests-pending
code-checks-rejected
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43955/38841
|
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages:
@Dr15Jones, @makortel, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| process.source = cms.Source("DelayedReaderThrowingSource", labels = cms.untracked.vstring("test", "test2", "test3")) | ||
| process.source = DelayedReaderThrowingSource( labels = ["test", "test2", "test3"]) | ||
|
|
||
| process.getter = cms.EDProducer("AddIntsProducer", labels = cms.VInputTag(cms.InputTag("test","","INPUTTEST"))) | ||
| process.onPath = cms.EDProducer("AddIntsProducer", labels = cms.VInputTag(cms.InputTag("test2", "", "INPUTTEST"), cms.InputTag("getter", "other"))) | ||
| process.f1 = cms.EDFilter("IntProductFilter", label = cms.InputTag("onPath"), shouldProduce = cms.bool(True)) | ||
| process.f2 = cms.EDFilter("IntProductFilter", label = cms.InputTag("onPath"), shouldProduce = cms.bool(True)) | ||
| process.inFront = cms.EDFilter("IntProductFilter", label = cms.InputTag("test3")) | ||
| process.getter = AddIntsProducer(labels = [("test","","INPUTTEST")]) | ||
| process.onPath = AddIntsProducer(labels = [("test2", "", "INPUTTEST"), ("getter", "other")]) | ||
| process.f1 = IntProductFilter( label = "onPath", shouldProduce = True) | ||
| process.f2 = IntProductFilter( label = "onPath", shouldProduce = True) | ||
| process.inFront = IntProductFilter( label = "test3") |
There was a problem hiding this comment.
This is something I was thinking about ages ago, when I was spending too much of my time looking after the HLT configurations, and I'm glad to see it being implemented !
Looking at the example, the main concern I have is for InputTags and VInputTags: writing or reading
labels = [("test2", "", "INPUTTEST"), ("getter", "other")]I think it's very easy to get confused about getter and other being two components on an InputTag instead of two separate InputTags.
Some other confusing exmples:
this is a VInputTag with only one InputTag ?
labels = [("getter", "other")]this is a VInputTag with two InputTags ?
labels = ["getter", "other"]this is a VInputTag with two InputTags ?
labels = ["getter"]this is a VInputTag with two InputTags ?
labels = [("getter")]this is a VInputTag with two InputTags ?
labels = [("getter",)]There was a problem hiding this comment.
@fwyzard Thanks for the feedback! All the examples are already possible since this code just reuses what is already available. The reduced syntax can already be used in
- Modifiers
- clone statements
- explicit changing of existing established parameters
E.g.
>>> import FWCore.ParameterSet.Config as cms
>>> test = cms.EDProducer("FOO", srcs = cms.VInputTag())
>>> test.srcs = ["getter"]
>>> print(test.dumpPython())
cms.EDProducer("FOO",
srcs = cms.VInputTag("getter")
)That being said, the reduced syntax also excepts the explicit type information as well
>>> test.srcs = [cms.InputTag("getter")]
>>> print(test.dumpPython())
cms.EDProducer("FOO",
srcs = cms.VInputTag(cms.InputTag("getter"))
)As for some of your explicit examples, I'm curious why you ask if the following are VInputTags with two entries, rather than (the correct) a VInputTags with 1 entry.
labels = ["getter"]
labels = [("getter")]
or even
labels = [("getter",)]
which does require one to understand that (...,) in python is just a way to express a tuple with a single entry.
All the above are standard python for a container with 1 entry.
There was a problem hiding this comment.
These two cases could be interpreted as ambiguous for understanding the type of labels (InputTag vs VInputTag) only from the assignment.
this is a
VInputTagwith twoInputTags ?labels = ["getter", "other"]
The same syntax would work for InputTag (with module and instance labels) and for 2-element VInputTag
this is a
VInputTagwith twoInputTags ?labels = ["getter"]
The same syntax would work for InputTag (with module labels) and for 1-element VInputTag.
(for reference, the sequence-syntax for assining InputTag was added in #29992, #29846)
|
-1 Failed Tests: Build BuildI found compilation error when building: >> Done generating edm plugin poisoned information gmake[1]: *** [config/SCRAM/GMake/Makefile.rules:1958: CompilePython] Error 1 >> Plugins of all types refreshed. gmake[1]: Target 'PostBuild' not remade because of errors. gmake[1]: Leaving directory '/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_1_X_2024-02-13-1100' gmake: *** [config/SCRAM/GMake/Makefile.rules:1846: src] Error 2 gmake: Target 'all' not remade because of errors. gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2 + eval scram build outputlog '&&' '(python3' /data/cmsbld/jenkins/workspace/ib-run-pr-tests/cms-bot/buildLogAnalyzer.py --logDir /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_1_X_2024-02-13-1100/tmp/el8_amd64_gcc12/cache/log/src '||' 'true)' ++ scram build outputlog >> Entering Package FWCore/Integration |
Dr15Jones
force-pushed
the
altModuleConfig
branch
from
dbd80eb
to
a8b1925
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-43955/39287
|
|
Pull request #43955 was updated. @makortel, @smuzaffar, @Dr15Jones can you please check and sign again. |
|
As a test, I found that the following could be a different way to implement the _cfi.py files from fillDescriptions from .FooProd import FooProd
foo = FooProd(aValue = 5)The relative python module path would avoid needing to know into which SubSystem.Package the file is generated. I hand created one such _cfi.py file in the correct cfipython sub-directory and everything (including calling cms.Process.load) worked just fine. NOTE: Using |
This only works for auto-generated cfis, or also for hand written ones ? |
I did a quick test and it does work with hand-written ones as well (surprisingly). |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4771e4/37822/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, @antoniovilela, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
@Dr15Jones , looks like this change now creates [a] cms-sw/cmsdist#9046 (comment) |
Correct. We think it would be best if scram would create the |
PR description:
Uses the default C++ configuration description to generate a new python file that knows the module
By importing the file (directly or indirectly through the new 'modules' python module) the developer no longer needs
to specify those information with declaring a module for use.
PR validation:
Framework unit tests all pass, including the two that were modified to use the new syntax.