Skip to content

Report the task directives referenced by the task script - #7505

Closed
bentsherman wants to merge 1 commit into
masterfrom
directive-refs-valrefs
Closed

Report the task directives referenced by the task script#7505
bentsherman wants to merge 1 commit into
masterfrom
directive-refs-valrefs

Conversation

@bentsherman

Copy link
Copy Markdown
Member

Alternative to #7483

Collect the whole task. namespace in the script variable references and report them through TaskRun#isDirectiveReferenced() so that an executor adjusting the resources at schedule time can tell whether the rendered command carries a value it is about to change.

The names do not reach the task hash: task is a local variable of the task context, so TaskRun#getGlobalVars() skips them.

nf-seqera submits the affected tasks with prediction model none.

This PR covers references in script:, shell:, stub:, and template scripts. It does not cover references in process directives or config because I have not seen examples of such usage.

Collect the whole `task.` namespace in the script variable references, matching
the legacy parser, and report them through TaskRun#isDirectiveReferenced so that
an executor adjusting the resources at schedule time can tell whether the
rendered command carries a value it is about to change.

The names do not reach the task hash: `task` is a local variable of the task
context, so TaskRun#getGlobalVars skips them.

nf-seqera submits the affected tasks with prediction model `none`.

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman
bentsherman requested a review from pditommaso August 19, 2026 17:49
@bentsherman
bentsherman requested a review from a team as a code owner August 19, 2026 17:49
@netlify

netlify Bot commented Aug 19, 2026

Copy link
Copy Markdown

Deploy Preview for nextflow-docs canceled.

Name Link
🔨 Latest commit 14910fd
🔍 Latest deploy log https://app.netlify.com/projects/nextflow-docs/deploys/6a85ecbc8524e20008a3e32a

@pditommaso

Copy link
Copy Markdown
Member

Thanks for putting this together — I like the direction, and I agree the valRefs reuse is a much lighter mechanism than what I had in #7483.

Pros over #7483, as I see it:

  • Two lines of production change in the core, vs. a new AST collector, a Closure subclass, and a BodyDef constructor arg.
  • Works with the v1 parser for free — nextflow.ast.VariableVisitor already collects full property chains, so this actually removes a v1/v2 inconsistency rather than adding machinery.
  • The no-hash-impact argument holds: TaskConfig#setContext puts task in the context holder, so getGlobalVars skips every task.* as a local var, and getTaskExtensionDirectiveVars re-adds only task.ext.*.
  • Doesn't touch config compilation or closure rendering, which is your main objection to Disable resource prediction for processes referencing task.memory #7483 and a fair one.

Blocker: no config coverage. This shape is common in nf-core/configs institutional profiles:

process {
    clusterOptions = { "-l h_vmem=${task.memory.toString().replaceAll(/[\sB]/, '')}" }
}

clusterOptions itself is grid-only and never reaches the Seqera path, so this exact line is harmless — but it shows the pattern is standard practice, not hypothetical. The same shape on a command-affecting directive does reach us:

process { withName: FOO { ext.args = { "-Xmx${task.memory.toGiga()}g" } } }

Here the script only shows task.ext.args; the closure is resolved by LazyMap, never by the template engine, so getVariableNames() can't see it. And config closures carry no source at runtime — ClosureToStringVisitor only runs under renderClosureAsString, and TaskClosure is only applied to when/stub. So there's nothing to inspect at submit time; this needs a compile-time hook.

Two smaller things: the stub isn't actually covered — TaskContext.variableNames comes from processor.getTaskBody().getValNames(), and the stub is a separate TaskClosure whose refs are never collected. And matching the whole task. namespace means isDirectiveReferenced('workDir') answers for a non-directive; #7483 filtered against DirectiveDsl for that reason.

@pditommaso

Copy link
Copy Markdown
Member

I'm attempting to cover the config reference extending this PR.

@bentsherman

Copy link
Copy Markdown
Member Author

I have another option that we can try. Let me submit another PR

@bentsherman

Copy link
Copy Markdown
Member Author

Have you actually encountered the ext.args example in a real pipeline?

@bentsherman

Copy link
Copy Markdown
Member Author

#7506 applies a fix at the runtime layer by simply recording accesses to task properties. That way we don't have to play whack-a-mole in the compiler

@pditommaso

Copy link
Copy Markdown
Member

The main config concern as pattern like this

process {
    clusterOptions = { "-l h_vmem=${task.memory.toString().replaceAll(/[\sB]/, '')}" }
}

there are plenty in nf-core/configs

@pditommaso

Copy link
Copy Markdown
Member

Comparing the five approaches against the requirement: detect task.memory in script/shell/template and config, direct or via closure.

One fact collapses the matrix: a config value cannot reference task unless it is a closure. ext.args = "-Xmx${task.memory}g" fails at parse time — v2 raises ConfigParseException, v1 degrades task.memory to an empty ConfigObject and throws MissingMethodException: No signature of method: groovy.util.ConfigObject.toGiga(). So "direct reference in config" is not a case to cover.

script / shell / template config closure directives resolved after render (beforeScript…) parsers delta
#7505 both +257
#7506 both +337
#7483 v2 config only +908
#7507 (#7505 + config AST) v2 config only ~+907 total
#7508 (#7505 + heuristic) ⚠️ over-broad both not viable

#7506 is the one I'd take. Observing the reads rather than inferring them gets the config ext.args case for free — no compiler hook, no Closure subclass, no hash exposure, and it works on both parsers. I checked the funnel claim holds: getMemory()getMemory0()get('memory'), getProperty falls through to get(), and the nested ext map is re-resolved on every read (only TaskConfig caches, and the read is recorded before the cache check), so the closure is genuinely re-entered during the window.

Its one gap is the one it already documents: beforeScript/afterScript are resolved by BashWrapperBuilder after the window closes. That looks fixable by widening the recording window over the wrapper build, rather than by adding a parser.

Two results from trying the alternatives:

One thing to carry into whichever lands: scope by source directive. clusterOptions = { "-l h_vmem=${task.memory...}" } is standard in nf-core institutional configs and never reaches the Seqera path, so #7483 as written would disable prediction pipeline-wide on it. #7506 sidesteps this for free, since it only observes what the command actually reads.

@bentsherman

Copy link
Copy Markdown
Member Author

But nf-core/configs will never apply to SIC because they are for other compute environments

The ext.args example might be a problem if you can find actual usages. Otherwise it's not worth chasing, since we are just applying a quick fix now and a long-term solution later.

But if you want to be extra sure then I would just go with #7506 . It should cover everything: script, config, v1/v2 parser

@bentsherman

Copy link
Copy Markdown
Member Author

Just saw your analysis. #7506 works for me. Feel free to test it / clean it up / merge it 👍

@pditommaso

Copy link
Copy Markdown
Member

Cool, took your advice. Closing this in favour of #7506

@pditommaso pditommaso closed this Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants