Disable resource prediction for processes referencing task.memory - #7483
Disable resource prediction for processes referencing task.memory#7483pditommaso wants to merge 4 commits into
Conversation
Explore an AST-based alternative to detect the processes whose rendered command depends on a directive that an executor can adjust at schedule time, such as `memory` under the Seqera scheduler prediction model. Traverse the process AST when it is transformed to Groovy, collecting the `task.<directive>` property references from the script, the stub and the process directives, and carry the resulting names to runtime through a new `BodyDef.directiveRefs` field. Being a compile-time fact it needs no value to be evaluated, so it also covers the directives resolved independently of the task script e.g. `beforeScript`, `ext`. The names are filtered against the `DirectiveDsl` declarations so that the `task` properties that are not directives e.g. `task.attempt` are not reported. Unlike `valRefs` these names do not contribute to the task hash. Note: a dynamic `ext.args` defined in the config file is compiled by the config parser, hence it is not covered by this approach. Assisted-by: Claude Opus 5 (1M context) Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Extend the compile-time collection of the `task` directive references to
the dynamic values defined in the config file, which is where the common
`ext.args` idiom lives:
process.ext.args = { "-Xmx${task.memory.toGiga()}g" }
A config value can only reference `task` from within a closure, since
`task` is not defined in the config scope -- the interpolated string form
is rejected at parse time. Closures retain no source at run time, so the
names are collected from the config AST and attached to the value through
the new `DirectiveRefsClosure` wrapper. An executor can then tell whether
a task depends on a directive without evaluating any value.
`TaskRun.isDirectiveReferenced` now reports the references coming from
both the process definition and the config file, and nf-seqera uses it to
disable the resource prediction model for the affected tasks.
Assisted-by: Claude Opus 5 (1M context)
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
✅ Deploy Preview for nextflow-docs canceled.
|
- Skip the config selector blocks when inspecting the task config. The
`withName:` / `withLabel:` entries are copied verbatim into every
process config by ProcessConfigBuilder#applyConfigDefaults, so a single
selector using a dynamic directive made *every* process report the
reference.
- Unwrap the directive-refs closure when rendering the config as text.
ClosureToStringVisitor runs after ConfigToGroovyVisitor and only knew
about a plain closure expression, so the wrapped values were left in
place, breaking `nextflow config` and the config generated by `kuberun`.
- Report the references made by a `shell` block or a `template` file,
which are rendered by the template engine and therefore are not visible
in the process AST.
- Descend into a list literal when attaching the references, as done by
ClosureToStringVisitor for `publishDir = [[path: {..}], ..]`.
- Sort the collected names to keep the generated bytecode reproducible.
Assisted-by: Claude Opus 5 (1M context)
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Expand the javadoc and add inline comments explaining the decisions and the non-obvious branches: - why the names are collected from the AST rather than recovered at runtime, and why only a closure is wrapped in the config file - why the directive references are kept apart from the value references, which feed the task hash - why the config selector blocks are skipped when inspecting the task config, and what each branch of the value walk corresponds to - why the three reference sources are unioned, none seeing the other two - why the wrapper is unwrapped when rendering the config as text - why the directive names come from the DSL declaration, and how a nested `task.ext.args` access is reported - why only `memory` gates the Seqera prediction model, why an explicit hint wins, and why the warning is reported once per process Assisted-by: Claude Opus 5 (1M context) Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
|
My initial reading -- this PR seems like overkill, and a mixture of concerns that I'd prefer to avoid right now (we are trying to stabilize the strict parser with all the new language features) I would just document it as a pitfall for now -- resource prediction doesn't mix well with task scripts that encode specific memory requests -- and see what solutions emerge before adding more compiler magic. It may be possible to write the process script in a better way (e.g. use memory % instead of memory GB) or disable optimization manually for specific processes |
|
Fair concerns, thanks for the read. A few thoughts: I did first try to solve it entirely inside You're right that it's broader than the immediate problem. That's somewhat deliberate, but I take the point about timing given the parser work. Also worth noting most of the diff is config introspection, which we can simplify once On documenting it as a pitfall: I'd rather not, at least not as the only answer. For Intelligent Compute the bar is that users get a good result without having to know about extra settings or rewrite their scripts — that part isn't negotiable for me. Happy to discuss scoping this down or staging it, though, if that helps land it more safely. |
|
I have created an alternative PR: #7505 It keeps the nf-seqera consumer logic and tests, but implements the directive-ref collection in a much simpler way. It just reuses the existing We can make this simplification because we don't need to cover directive-refs in process directives or config. I have not seen examples of Since this is a stop-gap fix anyway and the ultimate goal is to find a way to re-enable the optimization, I opted for a targeted fix |
|
Closing in favour of #7506 |
Summary
Detect at compile time which
taskdirectives a process depends on, so that an executor whichadjusts the compute resources after the script has been rendered can react.
The motivating case is the Seqera scheduler resource prediction model: it may allocate less memory
than the task requested, but the script is rendered before the task is scheduled, so a script
referencing
task.memorycarries the value that was requested:-Xmx8gis baked into the command even when less is allocated, and the task fails with anout-of-memory error.
How
DirectiveRefCollectorwalks the AST and the collected names are carried to run time, so nothinghas to be evaluated:
process is transformed to Groovy; the names travel on the new
BodyDef.directiveRefs.taskis not defined in theconfig scope, so the interpolated form is rejected at parse time), and closures retain no source
at run time. The names are therefore attached to the value itself via
DirectiveRefsClosure.shellblocks andtemplatefiles — rendered by the template engine, so they are reportedthrough the existing
TaskRun.templateVars.TaskRun.isDirectiveReferenced(name)returns the union.nf-seqerauses it to submit the affectedtasks with prediction model
noneand warn once per process; an explicitseqera/predictionModelhint still takes precedence.
Covered
task.memoryis referencedscript:/exec:blockshell:blocktemplatefilestub:blockext,beforeScript,containerOptions, …)process.ext.args = { … }, incl.withName:/withLabel:selectorsNames are matched against the
DirectiveDsldeclarations, so thetaskproperties that are notdirectives (
task.attempt,task.name,task.exitStatus, …) are correctly ignored.directiveRefsdeliberately does not contribute to the task hash, unlikevalRefs, so resumecaches are unaffected.
Remaining gap
NXF_SYNTAX_PARSER=v1— the legacy parsers do not go throughProcessToGroovyVisitor*/ConfigToGroovyVisitor, so the set stays empty and the check never fires. It degrades silently tothe current behaviour rather than failing.
Test plan
3759 tests across
:nextflow,:nf-langand:plugins:nf-seqera, 0 failures.New:
ProcessDirectiveRefsTest,ConfigDirectiveRefsTest, and the prediction cases inSeqeraTaskHandlerTest.Two defects found in review are fixed in c02168b and pinned by tests — neither was caught by the
first (fully green) CI run:
withName:/withLabel:blocks are copied verbatim into every process config byProcessConfigBuilder#applyConfigDefaults, so a single selector using a dynamic directive madeevery process report the reference.
ClosureToStringVisitorruns afterConfigToGroovyVisitorand only knew about a plain closureexpression, so wrapped values broke
nextflow configand the config generated bykuberun.