Deprecate convertToUnscheduled() customize function and --runUnscheduled argument of cmsDriver #29630
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29630/15008
|
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages: Configuration/Applications @andrius-k, @lveldere, @chayanit, @wajidalikhan, @sbein, @schneiml, @ianna, @kpedro88, @Martin-Grunewald, @fioriNTU, @santocch, @perrotta, @civanch, @makortel, @cmsbuild, @fwyzard, @davidlange6, @smuzaffar, @Dr15Jones, @cvuosalo, @mdhildreth, @jfernan2, @slava77, @fabiocos, @ssekmen, @kmaeshima, @pgunnell, @silviodonato, @franzoni can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
| # there are customize functions given by users (in our unit | ||
| # tests) that need to be run before the "unscheduled customise | ||
| # functions" | ||
| self.pythonCfgCode += self.addCustomise(1) |
There was a problem hiding this comment.
I tried first to remove the customisation_file_unsch altogether, but then found out that (at least) this test
https://github.com/cms-sw/cmssw/blob/master/TauAnalysis/MCEmbeddingTools/test/runtests.sh
depending on customize functions being called before the customize function added with PAT step.
| @@ -1645,8 +1645,6 @@ def prepare_PAT(self, sequence = "miniAOD"): | |||
| self.prepare_PATFILTER(self) | |||
| self.loadDefaultOrSpecifiedCFF(sequence,self.PATDefaultCFF) | |||
| self.labelsToAssociate.append('patTask') | |||
| if not self._options.runUnscheduled: | |||
| raise Exception("MiniAOD production can only run in unscheduled mode, please run cmsDriver with --runUnscheduled") | |||
There was a problem hiding this comment.
I'd be curious to learn more details on why MiniAOD requires "unscheduled mode" (to understand if not moving all EDProducers to Tasks could cause any problems).
There was a problem hiding this comment.
PAT was moved to unscheduled-only support a long time ago (was it 2013 or so), hence this check.
There was a problem hiding this comment.
PAT was moved to unscheduled-only support a long time ago (was it 2013 or so), hence this check.
Something along that was my recollection as well. Do I understand correctly that it means that as long as PAT is run unscheduled, all is fine (i.e. PAT does not require anything else in the job to be run unscheduled)?
There was a problem hiding this comment.
PAT does not require anything else in the job to be run unscheduled)?
yes
|
-1 Tested at: 3f47162 CMSSW: CMSSW_11_1_X_2020-05-04-1100 I found follow errors while testing this PR Failed tests: UnitTests
I found errors in the following unit tests: ---> test TestDQMOnlineClient-dt4ml_dqm_sourceclient had ERRORS |
|
Comparison job queued. |
|
+operations |
|
Do you have any comments? |
|
+1 |
|
+1 |
cmsbuild
added
fully-signed
pdmv-approved
and removed
pdmv-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. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
This broke the latest IB. It appears that we now have a case of a module in a Path depending on a module in the EndPath. From the log
In theory that could work, except we still have the assumption that EndPaths run after Paths in the dependency checker
|
|
Being followed-up in an issue #30074 |
PR description:
This PR proposes to deprecate the
convertToUnscheduled()andcleanUnscheduled()customize functions that were first introduced in 2012 as a "temporary aid" to help in moving to "unscheduled execution", and the--runUnscheduledcommand line argument tocmsDriver.py. In both cases the function/argument are left there, but they only print a deprecation message (to minimize breakingThe current behavior is to take all EDProducers and ignored EDFilters in Paths (either directly or via Sequences) and move those modules into Tasks associated to the same Paths. This transformation was useful to expose the module-level intra-event concurrency to the framework in the maximal way.
Since then, the framework has evolved, and with #29024 all modules (except EDFilters) in Paths are now run concurrently (still according to the filtering logic). Therefore the
convertToUnscheduled()is no longer strictly needed to maximize the intra-event concurrency between modules. In addition, #29553 makes the distinction between EDProducers in Tasks and in Paths more important:Accumulatorextension), they are, however, run for lumi, run, and job transitionsTherefore I'm starting to feel uneasy to move EDProducers from Paths to Tasks behind people's back (ok, in principle the use of
convertToUnscheduled()can be argued to be a "deliberate choice"), and am proposing to remove that functionality.It may be that some more modules get run now in the RECO step (e.g. DQM, VALIDATION, ALCA are still mostly using Sequences, whereas e.g. RAW2DIGI and RECO use Tasks).
Moving producers from Paths/Sequences to Tasks is recommended to be continued (a bit less overhead, and unnecessary producers do not get run).
I had to modify VALIDATION sequences in two places to avoid circular dependencies (modules in Paths/Sequences still need to be declared in the order of their data dependencies). I decided to just move modules to Tasks to make it explicit that their execution order does not matter. It may be that further testing (e.g. full matrix) reveals more such cases.
Resolves #20363. Resolves #20203.
PR validation:
Limited matrix and (relevant) unit tests run.