Skip to content

Propagate machine info to array-child tasks in Google Batch executor - #7629

Open
pditommaso wants to merge 2 commits into
masterfrom
fix-array-child-machine-info
Open

pditommaso wants to merge 2 commits into
masterfrom
fix-array-child-machine-info

Conversation

@pditommaso

Copy link
Copy Markdown
Member

Summary

Fixes null machineType/cost reporting for array-batched tasks on the Google Batch executor.

When a process uses the array directive, submit() runs only on the array-parent handler, which resolves and caches the job's CloudMachineInfo (machine type, zone, price model) as an instance field. updateStatus() then cascades jobId/taskId/uid down to each array-child handler, but never propagated machineInfo — so every child's trace record reported a null machine type, and any downstream consumer that prices a task from its machine type (e.g. Seqera Platform) could not compute a cost for it.

Approach

Rather than reaching into each child's private field from the parent, the resolved CloudMachineInfo is threaded through the updateStatus() recursion so each child stores it in its own field, alongside the jobId/taskId/uid it already receives. Every handler mutates only its own state, mirroring the existing propagation pattern in the same method — no cross-instance field access (.@) is introduced.

This is safe because Google Batch enforces a uniform InstancePolicy (machine type, CPU, memory, disk) across every element of one array job — array-eligible processes already require identical resource directives — so the parent's resolved CloudMachineInfo reflects what each child actually ran on.

Changes

  • GoogleBatchTaskHandler.groovy — add an optional machineInfo parameter to updateStatus(); the array branch forwards the parent's resolved value to each child, which stores it in the else branch (guarded so the non-array path is unchanged).
  • GoogleBatchTaskHandlerTest.groovy — new test asserting every array-child handler ends up reporting the same machineInfo as the parent after updateStatus().

Notes

Supersedes #7606, which fixed the same bug by writing the child's private field directly via the Groovy .@ field-access operator. This PR resolves the same root cause without breaking encapsulation. Credit to @markp for the original diagnosis and fix.

Test plan

  • New test: should propagate machine info to array child tasks on update status
  • nf-google test suite passes (./gradlew :plugins:nf-google:test — 109 tests, 0 failures)

🤖 Generated with Claude Code

For a process using the `array` directive, `submit()` runs only on the
array-parent handler, which resolves and caches the job's machine
type/zone/price model as an instance field. `updateStatus()` cascades
the submission results (jobId/taskId/uid) down to each array-child
handler but never propagated that machine info, so every array-child
trace record reported a null machine type and its cost could not be
computed downstream (e.g. Seqera Platform).

Propagate the resolved `CloudMachineInfo` through the `updateStatus()`
recursion so each child stores it in its own field, alongside the
jobId/taskId/uid it already receives. This keeps each handler mutating
only its own state — no cross-instance field access is required.

This is safe because Google Batch enforces a uniform instance policy
(machine type, CPU, memory, disk) across every element of one array
job, so the parent's resolved machine info reflects what each child
actually ran on.

Assisted-by: Claude Opus 4.8
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@netlify

netlify Bot commented Sep 16, 2026

Copy link
Copy Markdown

Deploy Preview for nextflow-docs canceled.

Name Link
🔨 Latest commit 3d951a0
🔍 Latest deploy log https://app.netlify.com/projects/nextflow-docs/deploys/6aac28556299520008066f56

@bentsherman bentsherman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Confirmed the mechanism: machineInfo is only ever set as a side effect of buildInstancePolicyOrTemplate() while building the submit request, and since only the array parent calls submit(), children were left with a null machine type in their trace records.

Checked the new test is a real regression guard rather than a tautology. Reverting just the three production edits and keeping the test makes it fail exactly as predicted:

child0.getMachineInfo() == machineInfo
|      null             |  CloudMachineInfo(n2-standard-4, europe-west2, spot)

Restoring the fix returns the suite to green at 109 tests.

Also verified the subclass concern, since Groovy compiles the default parameter into a four-arg method plus a three-arg bridge and the recursion now targets the four-arg form. An xpack subclass overriding the three-arg updateStatus() would have been silently skipped. There is no such override: GoogleBatchTaskHandlerPro is the only subclass and it overrides only createTaskWrapper() and newSubmitRequest(). Its newSubmitRequest() calls super first, so machineInfo is still populated before the cascade runs.

One non-blocking note worth recording. Children now hold a non-null machineInfo, so they reach updateZoneFromEvents() for the first time. That is an improvement, since each child resolves its own actual zone rather than inheriting the parent's planned one, but it adds a getTaskStatus() call per child on top of the one getNumSpotInterruptions() already makes. On a large array that doubles the per-task API calls at trace time. A follow-up could share a single status lookup between the two in getTraceRecord().

Approach is also the right call over #7606. Threading the value through the recursion keeps each handler mutating only its own state, matching how jobId, taskId and uid already flow.

@pditommaso

Copy link
Copy Markdown
Member Author

Thanks for the thorough review — especially for verifying the test actually fails on a revert, and for chasing down the Groovy default-parameter bridge concern with the GoogleBatchTaskHandlerPro subclass. That was the one thing I wasn't fully sure about.

Good catch on the extra getTaskStatus() call per child now that updateZoneFromEvents() is reachable. Opened #7636 to track sharing a single status lookup between the zone resolution and the spot-interruption count in getTraceRecord().

Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants