feat(slurm): wire retry and merge commands

[andreatnvidia]

Summary

• expose typed retry and collection operations through SlurmRunService
• add thin retry and merge CLI commands with stable results, errors, confirmation, and Big Iron-compatible flags
• preserve sparse retry identity, safe if_possible resume selection, idempotent recovery, and read-only dry runs
• submit winner-driven collection through the existing zero-GPU state coordinator with authorized destinations and persisted partition intent
• extend source and built-wheel command-surface coverage without adding benchmark work

Validation

• .venv/bin/ruff check --fix .
• .venv/bin/ruff format .
• make check-slurm
• make test-slurm (1,575 tests)
• make test-slurm-wheel-install
• exact-head Claude review at 768cb9d: no actionable findings

Part of #874

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge; no actionable new defect or outstanding previous finding remains.

Summary

• Adds typed retry and collection results and service operations.
• Supports sparse retry previews and submissions with explicit resume behavior.
• Submits winner-driven collection through the existing state coordinator.
• Adds profile initialization/validation and synchronous image lifecycle wiring.
• Extends source and installed-wheel command-surface coverage.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    CLI[Slurm CLI] --> Service[SlurmRunService]
    Service --> Retry[Retry coordinator]
    Service --> Collection[Collection coordinator]
    Retry --> State[Persisted run state]
    Retry --> Scheduler[Slurm scheduler]
    Collection --> State
    Collection --> Scheduler
    CLI --> Profile[Profile service]
    CLI --> Image[Image service]
    Profile --> Scheduler
    Image --> Scheduler

Loading

_{Reviews (3) · Last reviewed commit: "merge Slurm profile and image wiring"}

[nabinchha]

Thanks for wiring the retry and merge flows through the public Slurm service boundary—there is a lot of careful recovery and contract work here.

Summary

I reviewed the full PR at 768cb9d4984ceb54ff357c28aec501b9849aa438, including the CLI/service wiring, retry and collection state transitions, destination handling, packaging surface, and tests. I also independently confirmed the existing read-only preview lock-file finding, so I am not duplicating it below; it remains blocking.

Findings

Critical — Let's fix these before merge

packages/data-designer-slurm/src/data_designer/slurm/services/retry_collection.py:140 — Explicit merge destinations can overlap package-managed state

• What: collect() authorizes an arbitrary --output-path with CollectionDestinationResolver.resolve(), but that resolver only checks writable-mount coverage. It does not apply the planning invariant that rejects overlap with images, runtime, benchmarks, managed assets, or another path below runs outside the current run's output subtree. For example, /workspace/primary/runs/run-single/collections resolves successfully today.
• Why it matters: A destination equal to the run's collections directory passes this preflight, but submission then creates that state directory; the accepted collection worker can therefore never atomically publish its dataset there. Other currently accepted destinations, such as a not-yet-created retries directory, can publish user output into a namespace that retry state later expects to own. The new public merge --output-path makes these deterministic failures/state collisions user-triggerable.
• Suggestion: Apply the same non-overlap rules used by _validate_output_destination() to explicit collection destinations before persisting or submitting them. Add negative tests for package state roots and per-run collections/retries, while retaining a positive test for the current run's intended output subtree or another explicitly safe export mount.

Warnings — Worth addressing

packages/data-designer-slurm/src/data_designer/slurm/cli.py:124 and packages/data-designer-slurm/src/data_designer/slurm/services/run.py:250,284 — Avoid the newly added nested helpers

• What: The new retry/merge paths add retry(), retry_run(), and collect_run() as nested functions.
• Why it matters: Our style guide asks us to keep helpers at module or class scope unless a closure genuinely needs local capture; nesting this logic hides independently useful validation/forwarding behavior, makes focused tests harder, and adds closure frames to tracebacks.
• Suggestion: Use direct calls where the helper only forwards arguments, and move result validation/error-wrapping behavior into module-level private helpers or private methods with explicit parameters. This request is limited to the functions introduced by this PR; the nearby legacy closures do not need to be cleaned up here.

What Looks Good

• The retry identities, sparse shard ordering, and persisted recovery paths are modeled explicitly and covered across fresh, active, and resumed attempts.
• The public result contracts provide a stable boundary for both CLI JSON and Python callers.
• Validation was broad: changed-file and full-package lint/format checks passed, 1,574 source-tree tests passed, and the sole environment-only entry-point metadata test passed after installing the PR package (1,575 effective passes).

Verdict

Needs changes. Please fix the existing read-only preview mutation and prevent merge destinations from overlapping package-managed workspace/run state before merging. The newly introduced nested helpers should also be refactored where practical to follow the repository style guide.

---

This review was generated by an AI assistant.

[nabinchha]

Thanks for the follow-up, @andreatnvidia. I re-reviewed the complete PR at 44158adf98148b53126966ada0285832c05fec10, with particular attention to the changes since my previous review.

Summary

The current head resolves the prior merge-destination safety issue, the read-only preview mutation, and the avoidable nested retry/merge helpers. I also rechecked the surrounding retry, collection, observer, persistence, CLI, and service behavior against the full PR diff.

Findings

No new blocking or warning-level findings.

What Looks Good

• Explicit collection destinations now reuse the planner's package-state overlap rules, while still permitting the current run's intended output subtree and safe authorized export mounts.
• Persisted collection intent is reauthorized against the exact normalized host destination, preventing a stored path from drifting during resume.
• Read-only observation uses a non-mutating context, so previews no longer create shard lock files or scheduler evidence.
• The retry and collection callables were flattened into typed module-level helpers/partials, addressing the nested-function feedback cleanly.
• Validation is strong: lint and formatting checks pass; 77 focused tests pass; the broader Slurm suite produced 1,583 passes with only the isolated-worktree package-entry-point metadata check failing because that worktree was not installed as a distribution.

Residual Risk

The local suite cannot substitute for the documented real-cluster retry/merge acceptance checks, especially around scheduler timing and filesystem visibility.

Verdict

Ship it. The previously blocking findings are fixed and covered by focused regression tests.

---

This review was generated by an AI assistant.

[andreatnvidia]

@nabinchha Thanks, Nabin. I addressed all three points in 44158adf. Read-only retry previews now skip shard-lock acquisition, so they do not create lock files or persist scheduler evidence. Explicit collection destinations now reject overlap with workspace state, managed assets, and run-owned paths outside the current run's output subtree, with coverage for collections, retries, and safe output paths. I also replaced the new nested CLI and service helpers with direct calls and module-level validation helpers.