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
handle inverted filters in TriggerObjectStandAlone
#37181
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37181/28749
|
A new Pull Request was created by @missirol (Marino Missiroli) for master. It involves the following packages:
@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
if (pset->existsAs<vector<string>>(triggerNames.triggerName(i), true)) { | ||
auto const &modules = pset->getParameter<vector<string>>(triggerNames.triggerName(i)); | ||
for (auto const &module : modules) { | ||
auto const moduleStrip = module.front() != '-' and module.front() != '!' ? module : module.substr(1); |
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.
Naive comment: shouldn't this be a tilde, instead (or in addition)?
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.
So, what I did was simply print all these strings and it was clear that cms.ignore(process.foo)
resolves to -foo
, while ~process.foo
resolves to !foo
.
I don't actually know where the creation of these strings happens: it would be useful to know, so that we can figure out if other special cases/characters should be kept into account for EDFilters.
A naive look at the code base with git grep "'\!'"
did not give me certainty that -
and !
are the only two cases (maybe they are), e.g.
cmssw/FWCore/ParameterSet/python/SequenceTypes.py
Lines 576 to 628 in 6abbda5
class _SequenceNegation(_UnarySequenceOperator): | |
"""Used in the expression tree for a sequence as a stand in for the '!' operator""" | |
def __init__(self, operand): | |
super(_SequenceNegation,self).__init__(operand) | |
def __str__(self): | |
return '~%s' %self._operand | |
def dumpSequenceConfig(self): | |
return '!%s' %self._operand.dumpSequenceConfig() | |
def dumpSequencePython(self, options=PrintOptions()): | |
if self._operand.isOperation(): | |
return '~(%s)' %self._operand.dumpSequencePython(options) | |
return '~%s' %self._operand.dumpSequencePython(options) | |
def decoration(self): | |
return '!' | |
class _SequenceIgnore(_UnarySequenceOperator): | |
"""Used in the expression tree for a sequence as a stand in for the '-' operator""" | |
def __init__(self, operand): | |
super(_SequenceIgnore,self).__init__(operand) | |
def __str__(self): | |
return 'ignore(%s)' %self._operand | |
def dumpSequenceConfig(self): | |
return '-%s' %self._operand.dumpSequenceConfig() | |
def dumpSequencePython(self, options=PrintOptions()): | |
return 'cms.ignore(%s)' %self._operand.dumpSequencePython(options) | |
def decoration(self): | |
return '-' | |
class _SequenceWait(_UnarySequenceOperator): | |
"""Used in the expression tree for a sequence as a stand in for the '|' operator""" | |
def __init__(self, operand): | |
super(_SequenceWait,self).__init__(operand) | |
def __str__(self): | |
return 'wait(%s)' %self._operand | |
def dumpSequenceConfig(self): | |
return '|%s' %self._operand.dumpSequenceConfig() | |
def dumpSequencePython(self, options=PrintOptions()): | |
return 'cms.wait(%s)' %self._operand.dumpSequencePython(options) | |
def decoration(self): | |
return '|' | |
class _SequenceWaitAndIgnore(_UnarySequenceOperator): | |
"""Used in the expression tree for a sequence as a stand in for the '+' operator""" | |
def __init__(self, operand): | |
super(_SequenceWaitAndIgnore,self).__init__(operand) | |
def __str__(self): | |
return 'wait(ignore(%s))' %self._operand | |
def dumpSequenceConfig(self): | |
return '+%s' %self._operand.dumpSequenceConfig() | |
def dumpSequencePython(self, options=PrintOptions()): | |
return 'cms.wait(cms.ignore(%s))' %self._operand.dumpSequencePython(options) | |
def decoration(self): | |
return '+' |
cmssw/FWCore/Framework/src/StreamSchedule.cc
Lines 397 to 405 in 6abbda5
if (name[0] == '!') { | |
filterAction = WorkerInPath::Veto; | |
} else if (name[0] == '-' or name[0] == '+') { | |
filterAction = WorkerInPath::Ignore; | |
} | |
if (name[0] == '|' or name[0] == '+') { | |
//cms.wait was specified so do not run concurrently | |
doNotRunConcurrently = true; | |
} |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2311fa/22960/summary.html Comparison SummarySummary:
|
type bugfix Looks like the warnings in question have disappeared as expected. @makortel , it's not urgent but could you please have a look at #37181 (comment) , and let me know if I'm missing something obvious? Can we have EDFilters whose names are prepended by a special character other than
Edit : I guess it became urgent.. #37181 (comment) |
urgent |
I'd think these "decorations" to be more or less implementation details of the framework. The aim here appears to be stripping these off in order to get the ParameterSet of the module. Instead of extending the "stripping" logic here, perhaps we should provide a function to do that? @Dr15Jones, what do you think? |
+reconstruction
|
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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1
|
PR description:
This PR improves
TriggerObjectStandAlone
to handle cases where inverted filters (e.g.~process.fooFilter
, which returns the opposite ofprocess.fooFilter
) are used in the configuration file.Addresses #37152 (comment) by @perrotta, and possibly solves #37157.
Note: I don't know
TriggerObjectStandAlone
well, and I would encourage PAT experts to have a look at the suggested fix.FYI: @Martin-Grunewald
PR validation:
Ran
runTheMatrix.py -l 11634.0
, and checked that the warnings in question disappeared.If this PR is a backport, please specify the original PR and why you need to backport that PR:
N/A