Skip to content
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

Added ConditionalTask #37305

Merged
merged 10 commits into from
Apr 7, 2022
Merged

Added ConditionalTask #37305

merged 10 commits into from
Apr 7, 2022

Conversation

Dr15Jones
Copy link
Contributor

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.

- added fillDescriptions
- fixed a bug when filter not supposed to produce a data product
- allow testing for missing data products
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37305/28942

  • This PR adds an extra 132KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

  • DataFormats/Provenance (core)
  • FWCore/Framework (core)
  • FWCore/ParameterSet (core)

@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks.
@makortel, @wddgit, @rovere this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not needed

@@ -36,6 +38,7 @@
#include <list>
#include <map>
#include <exception>
#include <unordered_set>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be in .h

(ci.process().empty() or ci.process() == processConfiguration->processName())) {
auto productModuleLabel = ci.label();
if (productModuleLabel.empty()) {
for (auto const& branch : conditionalModuleBranches) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this comment as all this was done.

@Dr15Jones
Copy link
Contributor Author

I plan to address my own comments once the intitial tests finish.

@makortel
Copy link
Contributor

I'm still digesting, but I think having a test with SwitchProducer in the ConditionalTask would be useful.

Copy link
Contributor

@makortel makortel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through the python part

@@ -238,13 +238,19 @@ def findDirectDependencies(element, collection,sortByType=True):
continue
t = 'sequences'
# cms.Task
elif isinstance(item, Task):
elif isinstance(item, _Task):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be

Suggested change
elif isinstance(item, _Task):
elif isinstance(item, _TaskBase):

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()) + \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

Suggested change
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this list include ConditionalTask now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tasks can't hold ConditionalTasks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the "replace a Service that was held in a Task" case you had mentioned privately?

Copy link
Contributor Author

@Dr15Jones Dr15Jones Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? I only vaguely remember our conversation about that.

Comment on lines 1285 to 1286
constTaskVistor = ModuleNodeOnConditionalTaskVisitor(condTaskModules)
pathCompositeVisitor = CompositeVisitor(pathValidator, nodeVisitor, lister, constTaskVistor)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo(s?)

Suggested change
constTaskVistor = ModuleNodeOnConditionalTaskVisitor(condTaskModules)
pathCompositeVisitor = CompositeVisitor(pathValidator, nodeVisitor, lister, constTaskVistor)
constTaskVisitor = ModuleNodeOnConditionalTaskVisitor(condTaskModules)
pathCompositeVisitor = CompositeVisitor(pathValidator, nodeVisitor, lister, constTaskVisitor)

or

Suggested change
constTaskVistor = ModuleNodeOnConditionalTaskVisitor(condTaskModules)
pathCompositeVisitor = CompositeVisitor(pathValidator, nodeVisitor, lister, constTaskVistor)
condTaskVisitor = ModuleNodeOnConditionalTaskVisitor(condTaskModules)
pathCompositeVisitor = CompositeVisitor(pathValidator, nodeVisitor, lister, condTaskVisitor)

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']))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be

Suggested change
self.assertEqual(l, set(['mesproducer', 'mproducer', 'mproducer2', 'mfilter', 'd', 'messource']))
self.assertEqual(l2, set(['mesproducer', 'mproducer', 'mproducer2', 'mfilter', 'd', 'messource']))

?

testTask1.add(testTask3)
process.myTask1 = testTask1

# test the validation that occurs when attaching a Task to a Process
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# test the validation that occurs when attaching a Task to a Process
# test the validation that occurs when attaching a ConditionalTask to a Process

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ec433d/23282/summary.html
COMMIT: 76938a5
CMSSW: CMSSW_12_4_X_2022-03-22-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37305/23282/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test materialBudgetTrackerPlots had ERRORS

Comparison Summary

There are some workflows for which there are errors in the baseline:
534.0 step 1
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

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:

  • /pool/condor/dir_63795/jenkins/workspace/compare-root-files-short-matrix/data/PR-ec433d/39434.911_TTbar_14TeV+2026D88_DD4hep+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3695650
  • DQMHistoTests: Total failures: 52023
  • DQMHistoTests: Total nulls: 62
  • DQMHistoTests: Total successes: 3643543
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 48 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@Martin-Grunewald
Copy link
Contributor

@Dr15Jones thanks for this.
I assume we need a backport to 12_3 as this is the release used for initial data taking.
@silviodonato

@Martin-Grunewald
Copy link
Contributor

@Dr15Jones
Why is this needed - as per your PR description:

If no module on the Path directly or indirectly uses a module in the ConditionalTask, that module is set to unscheduled execution.

?

@Dr15Jones
Copy link
Contributor Author

If no module on the Path directly or indirectly uses a module in the ConditionalTask, that module is set to unscheduled execution.

Because it would be very difficult to make it work without that. So for a first pass we added that behavior.

Copy link
Contributor

@makortel makortel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through the C++ code

FWCore/Framework/src/StreamSchedule.cc Outdated Show resolved Hide resolved
FWCore/Framework/src/StreamSchedule.cc Outdated Show resolved Hide resolved
FWCore/Framework/src/StreamSchedule.cc Outdated Show resolved Hide resolved
FWCore/Framework/src/StreamSchedule.cc Outdated Show resolved Hide resolved
FWCore/Framework/src/StreamSchedule.cc Show resolved Hide resolved
@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2022

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)

@makortel
Copy link
Contributor

makortel commented Apr 6, 2022

hold

based on #36938 (comment)

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 6, 2022

Pull request has been put on hold by @makortel
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@makortel
Copy link
Contributor

makortel commented Apr 6, 2022

unhold

based on #36938 (comment)

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 6, 2022

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)

@perrotta
Copy link
Contributor

perrotta commented Apr 7, 2022

@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?

@makortel
Copy link
Contributor

makortel commented Apr 7, 2022

@perrotta I checked again and my comments #37305 (comment) and #37305 (comment) (that are the only ones in SequenceTypes.py that are not "Outaded" AFAICT) have been addressed.

@perrotta
Copy link
Contributor

perrotta commented Apr 7, 2022

@perrotta I checked again and my comments #37305 (comment) and #37305 (comment) (that are the only ones in SequenceTypes.py that are not "Outaded" AFAICT) have been addressed.

Thank you @makortel for the confirmation

@perrotta
Copy link
Contributor

perrotta commented Apr 7, 2022

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants