Evaluate CWL when conditionals before scheduling to avoid wasted batch-system submissions (#3990) - #5569
Evaluate CWL when conditionals before scheduling to avoid wasted batch-system submissions (#3990)#5569annagiroti wants to merge 2 commits into
when conditionals before scheduling to avoid wasted batch-system submissions (#3990)#5569Conversation
…tch-system submissions Steps with a `when` conditional were always scheduled onto the real batch system as a fully-resourced job, even when the condition was going to evaluate false and the step would be skipped. On a busy HPC system, a skipped step that asks for large resources can sit queued for hours before finding out it had nothing to do. CWLJobWrapper already solves this problem for dynamic resource requirements: it runs as a cheap local job that resolves inputs before spawning the real job. Route `when`-conditional steps through the same wrapper, so the condition is checked with resolved inputs before ever creating the real job, and skipped steps never touch the batch system.
|
This closes #5507 by replacing it, I think. |
|
@annagiroti Can you fill in the changelog entry ( |
adamnovak
left a comment
adamnovak
left a comment
There was a problem hiding this comment.
This looks like it should work OK, but:
- The test has a
sleepinput, presumably copied from a different test and not changed, which does not control a sleep - The docstrings are following the Way of Claude rather than PEP 257, and in particular are not leveraging a theory of mind to make the reader reach true conclusions about the code.
| steps: | ||
| produce: | ||
| in: | ||
| sleep: sleep |
There was a problem hiding this comment.
We should rename this input to something other than sleep (maybe number?), because other test workflows will have sleep inputs that make them actually wait, and it's weird that this one doesn't.
| """ | ||
| Wrap a CWL job that uses dynamic resources requirement. | ||
| Wrap a CWL job that uses a dynamic resources requirement, or that may be |
There was a problem hiding this comment.
Ideally we want a 1-line summary at the top of our docstring. Maybe say that this job determines how and whether to run the wrapped job?
| """ | ||
| Wrap a CWL job that uses dynamic resources requirement. | ||
| Wrap a CWL job that uses a dynamic resources requirement, or that may be | ||
| skipped by a `when` conditional that can't be safely evaluated until the |
There was a problem hiding this comment.
Saying it that way makes it sound like there are some conditionals that can be safely evaluated without the step's inputs, and others that can't, and our code actually knows the difference and acts on it to evaluate only the conditionals that need the inputs with this mechanism.
That's not true, so we can probably just say "a conditional" instead of the long mis-explanatory phrase.
| When executed, this creates a new child job which has the correct resource | ||
| requirement set. | ||
| When executed, this runs on the leader with minimal resources, resolves | ||
| the job's inputs, and then either reports the step as skipped or creates | ||
| a new child job which has the correct resource requirement set. |
There was a problem hiding this comment.
This whole piece should probably be cut, because it's a summary rather than an explanation.
If we do want to write down the responsibility, we should say something like "This job is responsible for creating a CWLJob child with the right resource requirements, when the job should not be skipped."
| jobobj, | ||
| runtime_context, | ||
| parent_name=parent_name, | ||
| conditional=conditional, |
There was a problem hiding this comment.
We're still passing the conditional to CWLJob, and CWLJob hasn't had any conditional-checking code removed from it. If CWLJobWrapper is now responsible for evaluating all the conditionals on jobs, CWLJob should probably forget how to do it itself.
| if has_dynamic_resource_requirement or ( | ||
| conditional is not None and conditional.expression is not None | ||
| ): | ||
| # Resource requirements and the `when` conditional can depend on |
There was a problem hiding this comment.
It probably makes more sense to talk about conditionals in the code's terms, where we have a Conditional type, rather than in CWL language terms where we have a when field in various places in YAML.
2 participants
Changelog Entry
Steps with a
whenconditional were always scheduled onto the real batch system as a fully-resourced job, even when the condition evaluated false and the step would be skipped. On a busy HPC system, a skipped step that requests large resources can sit queued for hours before discovering it had nothing to do.CWLJobWrapperalready solves this for dynamic resource requirements: it runs as a cheap local job that resolves inputs before spawning the real job. This routeswhen-conditional steps through the same wrapper, so the condition is checked with fully-resolved inputs — including promises from upstream step outputs — before the real job is ever created. Skipped steps never touch the batch system.Resolves #3990
To be copied to the draft changelog by merger:
Reviewer Checklist
issues/XXXX-fix-the-thingin the Toil repo, or from an external repo.camelCasethat want to be insnake_case.docs/running/{cliOptions,cwl,wdl}.rstMerger Checklist