Added ConditionalTask #37305
Added ConditionalTask #37305
Conversation
- added fillDescriptions - fixed a bug when filter not supposed to produce a data product - allow testing for missing data products
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37305/28942
|
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages:
@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
| @@ -21,6 +22,7 @@ | |||
| #include "FWCore/ParameterSet/interface/ParameterSetDescription.h" | |||
| #include "FWCore/ParameterSet/interface/Registry.h" | |||
| #include "FWCore/ServiceRegistry/interface/PathContext.h" | |||
| #include "FWCore/Reflection/interface/DictionaryTools.h" | |||
There was a problem hiding this comment.
probably not needed
| @@ -36,6 +38,7 @@ | |||
| #include <list> | |||
| #include <map> | |||
| #include <exception> | |||
| #include <unordered_set> | |||
| (ci.process().empty() or ci.process() == processConfiguration->processName())) { | ||
| auto productModuleLabel = ci.label(); | ||
| if (productModuleLabel.empty()) { | ||
| for (auto const& branch : conditionalModuleBranches) { |
There was a problem hiding this comment.
probably should add a comment saying this is a consumesMany
| auto itCondBegin = std::find(modnames.begin(), modnames.end(), "#"); | ||
|
|
||
| std::unordered_set<std::string> conditionalmods; | ||
| //need to capture |
There was a problem hiding this comment.
Either complete this thought or remove.
| @@ -442,6 +645,17 @@ namespace edm { | |||
| if (runConcurrently && worker->moduleType() == Worker::kFilter and filterAction != WorkerInPath::Ignore) { | |||
| runConcurrently = false; | |||
| } | |||
|
|
|||
| //TODO: call consumesInfo and see if need any modules from conditionalmods | |||
There was a problem hiding this comment.
Remove this comment as all this was done.
|
I plan to address my own comments once the intitial tests finish. |
|
I'm still digesting, but I think having a test with |
| @@ -238,13 +238,19 @@ def findDirectDependencies(element, collection,sortByType=True): | |||
| continue | |||
| t = 'sequences' | |||
| # cms.Task | |||
| elif isinstance(item, Task): | |||
| elif isinstance(item, _Task): | |||
There was a problem hiding this comment.
Should this be
| elif isinstance(item, _Task): | |
| elif isinstance(item, _TaskBase): |
?
There was a problem hiding this comment.
Directly below the line is the check on ConditionalTask so this should be Task.
| @@ -444,7 +450,7 @@ def replace(self, original, replacement): | |||
| # where objects that contain other objects are involved. See the comments | |||
| # for the _MutatingSequenceVisitor. | |||
|
|
|||
| if isinstance(original,Task) != isinstance(replacement,Task): | |||
| if (isinstance(original,Task) != isinstance(replacement,Task)) or (isinstance(original,ConditionalTask) != isinstance(replacement,ConditionalTask)): | |||
| raise TypeError("replace only works if both arguments are Tasks or neither") | |||
There was a problem hiding this comment.
Should this error message be updated?
| " replace was called with original type = " + str(type(original)) + "\n" + \ | ||
| " and replacement type = " + str(type(replacement)) + "\n") | ||
| if not self._allowedInTask(original) or (not replacement is None and not self._allowedInTask(replacement)): | ||
| raise TypeError("The Task replace function only works with objects that can be placed on a {}\n".format(self._taskType()) + \ |
There was a problem hiding this comment.
How about
| raise TypeError("The Task replace function only works with objects that can be placed on a {}\n".format(self._taskType()) + \ | |
| raise TypeError("The {0} replace function only works with objects that can be placed on a {0}\n".format(self._taskType()) + \ |
?
| An EDProducer or EDFilter will be added to a Path or EndPath based on which other | ||
| modules on the Path consumes its data products. If that ConditionalTask assigned module | ||
| is placed after an EDFilter, the module will only run if the EDFilter passes. If no module | ||
| on the Path needs the module's data products, the module will be removed from the job. |
There was a problem hiding this comment.
This comment is inconsistent with the PR description.
| if options.isCfg: | ||
| result += 'process.' | ||
| result += self._name+'\")\n' | ||
| return result | ||
|
|
||
| class Task(_TaskBase) : | ||
| """Holds EDProducers, EDFilters, ESProducers, ESSources, Services, and Tasks. | ||
| A Task can be associated with Sequences, Paths, EndPaths and the Schedule. |
There was a problem hiding this comment.
Should this list include ConditionalTask now?
There was a problem hiding this comment.
Tasks can't hold ConditionalTasks
There was a problem hiding this comment.
Right, but it can be associated with ConditionalTask. Or would some other wording than "associated" be more precise for the relationship between Task and ConditionalTask?
| s = Sequence(e, ct4) | ||
| p16 = Path(a+b+s+c,ct1) | ||
| self.assertEqual(p16.dumpPython(),"cms.Path(process.a+process.b+cms.Sequence(process.e, cms.ConditionalTask(process.d, process.f))+process.c, cms.ConditionalTask(process.a))\n") | ||
|
|
There was a problem hiding this comment.
Could you add a test that Task(ConditionalTask(...)) gives an error?
| @@ -410,6 +415,9 @@ def __setattr__(self,name,value): | |||
| +"an instance of "+str(type(value))+" will not work - requested label is "+name) | |||
| if not isinstance(value,_Labelable) and not isinstance(value,Source) and not isinstance(value,Looper) and not isinstance(value,Schedule): | |||
| if name == value.type_(): | |||
| if hasattr(self,name) and (getattr(self,name)!=value): | |||
| self._replaceInTasks(name, value) | |||
| self._replaceInConditionalTasks(name, value) | |||
There was a problem hiding this comment.
Is this the "replace a Service that was held in a Task" case you had mentioned privately?
There was a problem hiding this comment.
Maybe? I only vaguely remember our conversation about that.
FWCore/ParameterSet/python/Config.py
Outdated
| constTaskVistor = ModuleNodeOnConditionalTaskVisitor(condTaskModules) | ||
| pathCompositeVisitor = CompositeVisitor(pathValidator, nodeVisitor, lister, constTaskVistor) |
There was a problem hiding this comment.
Typo(s?)
| constTaskVistor = ModuleNodeOnConditionalTaskVisitor(condTaskModules) | |
| pathCompositeVisitor = CompositeVisitor(pathValidator, nodeVisitor, lister, constTaskVistor) | |
| constTaskVisitor = ModuleNodeOnConditionalTaskVisitor(condTaskModules) | |
| pathCompositeVisitor = CompositeVisitor(pathValidator, nodeVisitor, lister, constTaskVisitor) |
or
| constTaskVistor = ModuleNodeOnConditionalTaskVisitor(condTaskModules) | |
| pathCompositeVisitor = CompositeVisitor(pathValidator, nodeVisitor, lister, constTaskVistor) | |
| condTaskVisitor = ModuleNodeOnConditionalTaskVisitor(condTaskModules) | |
| pathCompositeVisitor = CompositeVisitor(pathValidator, nodeVisitor, lister, condTaskVisitor) |
FWCore/ParameterSet/python/Config.py
Outdated
| testTask1.visit(visitor) | ||
| self.assertEqual(l, set(['mesproducer', 'mproducer', 'mproducer2', 'mfilter', 'd', 'messource'])) | ||
| l2 = testTask1.moduleNames | ||
| self.assertEqual(l, set(['mesproducer', 'mproducer', 'mproducer2', 'mfilter', 'd', 'messource'])) |
There was a problem hiding this comment.
Should this be
| self.assertEqual(l, set(['mesproducer', 'mproducer', 'mproducer2', 'mfilter', 'd', 'messource'])) | |
| self.assertEqual(l2, set(['mesproducer', 'mproducer', 'mproducer2', 'mfilter', 'd', 'messource'])) |
?
FWCore/ParameterSet/python/Config.py
Outdated
| testTask1.add(testTask3) | ||
| process.myTask1 = testTask1 | ||
|
|
||
| # test the validation that occurs when attaching a Task to a Process |
There was a problem hiding this comment.
| # test the validation that occurs when attaching a Task to a Process | |
| # test the validation that occurs when attaching a ConditionalTask to a Process |
|
-1 Failed Tests: UnitTests Unit TestsI found errors in the following unit tests: ---> test materialBudgetTrackerPlots had ERRORS Comparison SummaryThere are some workflows for which there are errors in the baseline: The workflows 1001.0, 1000.0, 136.88811, 136.874, 136.8311, 136.793, 136.7611, 136.731, 4.22 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons @slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
|
@Dr15Jones thanks for this. |
|
@Dr15Jones ? |
Because it would be very difficult to make it work without that. So for a first pass we added that behavior. |
|
+1 |
cmsbuild
added
fully-signed
hlt-approved
and removed
hlt-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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
hold based on #36938 (comment) |
|
Pull request has been put on hold by @makortel |
|
unhold based on #36938 (comment) |
|
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) |
|
@Dr15Jones there are a couple of comments from @makortel in FWCore/ParameterSet/python/SequenceTypes.py that don't look having been ever addressed or answered. Could any of you confirm that they do not require further fixes here, and that this PR can be merged as it is? |
|
@perrotta I checked again and my comments #37305 (comment) and #37305 (comment) (that are the only ones in |
Thank you @makortel for the confirmation |
|
+1 |
PR description:
A ConditionalTask will automatically add modules in the ConditionalTask to the associated Path in the proper order based on consumes/produce calls of the modules in the Path and in the ConditionalTask. If no module on the Path directly or indirectly uses a module in the ConditionalTask, that module is set to unscheduled execution.
PR validation:
All framework unit tests pass and newly added unit tests also pass.