Skip to content

Detect task directive references in dynamic config values - #7507

Closed
pditommaso wants to merge 1 commit into
directive-refs-valrefsfrom
directive-refs-config
Closed

Detect task directive references in dynamic config values#7507
pditommaso wants to merge 1 commit into
directive-refs-valrefsfrom
directive-refs-config

Conversation

@pditommaso

Copy link
Copy Markdown
Member

Stacked on #7505 (directive-refs-valrefs). Review this PR as a delta on top of it; the base must be merged first.

Problem

#7505 makes TaskRun#isDirectiveReferenced(String) work off the script variable references, so nf-seqera can disable resource prediction for a process whose script bakes in task.memory. It does not see a reference made by a dynamic directive value defined in the config file:

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

The script only shows task.ext.args; the closure is resolved by LazyMap, never by the template engine, and a config closure carries no source text at runtime (ClosureToStringVisitor only runs for nextflow config / kuberun; TaskClosure is only applied to when / stub). So the reference has to be collected at compile time.

This is the common shape in nf-core pipelines, so without it the safety net of #7505 misses most real occurrences.

Approach

Ported from the config half of #7483, with two refinements.

  1. DirectiveRefCollector (nf-lang) — walks an AST expression and reports the task.<name> property reads whose name is a process directive. The name set is derived from ProcessDsl.DirectiveDsl rather than hard-coded, so task.attempt / task.exitStatus are correctly ignored while task.memory / task.time are collected, and a new directive needs no second place to update.
  2. ConfigToGroovyVisitor#transformDirectiveRefs — wraps a dynamic directive value that references a directive into a DirectiveRefsClosure constructor call carrying the names (recursing into list/map literals, mirroring ClosureToStringVisitor#replaceClosures). Only closures are wrapped: task is not defined in the config scope, so a plain interpolated string referencing it is already a parse error, which makes a closure the only shape that can carry a reference.
  3. DirectiveRefsClosure (nextflow) — delegates every Closure method to the closure it wraps, so the value keeps behaving exactly as written when LazyMap#resolveImpl clones and calls it. Follows the pattern of TaskClosure.
  4. TaskConfig#isDirectiveReferenced(directive, fromDirectives = null) — inspects the raw config entries (nothing is resolved as a side effect of the check) and reports whether any value declares the reference.

The process-definition side is untouched: it stays exactly as #7505 implements it, via valRefs. TaskRun#isDirectiveReferenced becomes the union of the two.

Not ported from #7483: BodyDef.directiveRefs and its 5th constructor argument, the ProcessToGroovyVisitorV1/V2 changes, its own ProcessDirectiveRefsTest (#7505's is kept), and the ClosureToStringVisitor change (see refinement 1).

Refinement 1 — guard on !renderClosureAsString instead of patching ClosureToStringVisitor

#7483 wrapped closures unconditionally, which left nextflow config and kuberun rendering a DirectiveRefsClosure constructor call where a source-text placeholder belongs — corrupting the output. It fixed that by teaching ClosureToStringVisitor to unwrap the constructor call again.

The rendering path and the running path are mutually exclusive: nextflow config / kuberun never need directive refs, and a run never renders closures as strings. So ConfigCompiler now threads its renderClosureAsString flag into ConfigToGroovyVisitor, which skips the wrapping entirely when it is set. The renderer therefore always sees the plain closure the user wrote, and ClosureToStringVisitor needs no knowledge of this feature at all.

Covered by should render a dynamic directive as a string placeholder, which asserts both the ConfigClosurePlaceholder and the original source text surviving into ConfigHelper.toCanonicalString.

Refinement 2 — scope the check by source directive

#7483 flattened every reference into one union, which over-triggers. The nf-core institutional profiles routinely carry:

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

clusterOptions is read only by the grid executors (AbstractGridExecutor, Lsf/Pbs/PbsPro, TaskArrayCollector) and is put in the job header, not in the command. plugins/nf-seqera/src/main never touches clusterOptions, queue or penv. Under the flattened union, that one line at the top of the process scope disables resource prediction for every process in the pipeline, on the strength of a directive the executor ignores — which defeats the feature.

So isDirectiveReferenced takes an optional collection of source directives to inspect; null keeps the broad behaviour. No keying on the closure is needed — the config entry key already is the source directive (ext for ext.args = {...}), so the filter is on the entry key.

TaskRun passes COMMAND_DIRECTIVES = ['ext', 'beforeScript', 'afterScript', 'containerOptions']: the directives whose value is user-authored text baked verbatim into the rendered command or the container run spec, verified against TaskBean / BashWrapperBuilder:

directive reaches the command
ext yes — interpolated by the script as ${task.ext.args}
beforeScript / afterScript yes — emitted verbatim into .command.run (BashWrapperBuilder:405,543)
containerOptions yes — builder.addRunOptions(containerOptions) (BashWrapperBuilder:763)
clusterOptions, queue, penv no — grid submit header only

The other directives TaskBean feeds to the wrapper (module, shell, scratch, stageInMode, …) take enumerated or structured values that cannot meaningfully carry a resource figure, and are left out.

Note containerOptions is inert on the Seqera path specifically — SeqeraExecutor#isContainerNative() is true, so runWithContainer is false and nf-seqera never forwards it — but it is a genuine command dependency for the container-managed executors.

Where the set lives

In core (TaskRun), not nf-seqera. The set answers "which config directives are rendered into the task command", which is a property of TaskBean / BashWrapperBuilder — the same knowledge that would go stale if a directive were added to the wrapper. nf-seqera's question is the executor-specific one ("does this command depend on task.memory"), and it already asks exactly that through TaskRun#isDirectiveReferenced(String); nf-seqera needs no change in this PR. Putting the set in the plugin would duplicate it into every executor that later wants the same check.

Known limitation

NXF_SYNTAX_PARSER=v1 selects ConfigParserV1 (ConfigParserFactory:37), which has no ConfigToGroovyVisitor — so this is v2-config-parser only and degrades silently to the #7505 behaviour (script references only). Same limitation as #7483. Documented in the nf-seqera README.

Test evidence

New ConfigDirectiveRefsTest (13 cases): detection in ext / plain directives / nested list+map values; task.attempt and task.exitStatus correctly ignored; the wrapped value still resolving normally through LazyMap (beforeScriptecho -Xmx8g); source-directive scoping in both directions; the withName: selector skip for a matching and a non-matching process; the nextflow config placeholder rendering; and the TaskRun union.

The selector skip is a real bug found in review of #7483: ProcessConfigBuilder#applyConfigDefaults copies every key of the process scope into each process config, including the withName: / withLabel: blocks that were not applied — so without the skip a single selector makes every process report a reference. A selector that does match is already merged into the top-level directives, so true positives are unaffected.

$ ./gradlew :nextflow:compileGroovy :nf-lang:compileJava
BUILD SUCCESSFUL

$ ./gradlew :nextflow:test --tests "*DirectiveRefs*" --tests "*TaskConfig*" \
    --tests "*ConfigParser*" --tests "*ClosureToString*" --tests "*ConfigHelper*" \
    --tests "*TaskHasher*" --tests "*TaskRun*"
BUILD SUCCESSFUL  -- 8 classes, 228 tests, 0 failures
  ConfigDirectiveRefsTest, ProcessDirectiveRefsTest, ConfigParserV1Test,
  ConfigParserV2Test, TaskConfigTest, TaskHasherTest, TaskRunTest, ConfigHelperTest
  (no class matches *ClosureToString*; that path is covered by ConfigDirectiveRefsTest
   and ConfigParserV2Test)

$ ./gradlew :plugins:nf-seqera:test --tests "*SeqeraTaskHandler*"
BUILD SUCCESSFUL  -- 71 tests, 0 failures

$ ./gradlew :nf-lang:test :nextflow:test --tests "*Config*" --tests "*Task*" --tests "*Process*"
BUILD SUCCESSFUL  -- nextflow: 67 classes / 971 tests / 0 failures
                     nf-lang:  14 classes / 149 tests / 0 failures

TaskHasherTest and TaskRunTest are included to show no hash/resume regression: the collected names never enter the hash — they ride on the config value, and TaskRun#getGlobalVars skips task.* as task-local anyway.

🤖 Generated with Claude Code

A dynamic directive value defined in the config file, e.g.

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

is a closure resolved by LazyMap, never by the template engine, so the script
variable references collected by #7505 only ever show `task.ext.args` -- the
`task.memory` dependency is invisible. A closure carries no source text at
runtime, so the names are collected from the config AST at compile time and
attached to the value by DirectiveRefsClosure.

The wrapping is skipped when `renderClosureAsString` is set: `nextflow config`
and `kuberun` replace every closure with its source text, so the two paths are
mutually exclusive and the renderer always sees the plain closure.

TaskConfig#isDirectiveReferenced takes the source directives to inspect, so a
caller only counts a reference made by a directive it actually consumes.
TaskRun passes the directives rendered into the task command -- `ext`,
`beforeScript`, `afterScript`, `containerOptions` -- which keeps a common
`clusterOptions = { "-l h_vmem=${task.memory}" }` in an institutional profile
from reporting every process of the pipeline as memory-dependent.

Limitation: NXF_SYNTAX_PARSER=v1 selects ConfigParserV1, which has no
ConfigToGroovyVisitor, so config references are not collected there.

Assisted-by: Claude Opus 5 (Claude Code)
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso

Copy link
Copy Markdown
Member Author

Closing 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.

1 participant