feat(slurm): add profile setup and image lifecycle wiring

[andreatnvidia]

Summary

• add package-owned profile init and profile validate services with deterministic mode-0600 no-overwrite creation, ordinary catalog selection, effective workspace checks, and Slurm GPU resolution
• replace the production image add unavailable stub with the existing structured lifecycle prepare, render, submit, scheduler-observation, inspection, publication, and atomic registry path
• fail closed on missing or non-terminal accounting evidence, preserve ambiguous submissions, and clean definitive failures, cancellations, interruptions, and successful lifecycle state safely
• extend public CLI, service, source-package, and installed-wheel coverage without consuming retry or benchmark work from adjacent PRs

Validation

• .venv/bin/ruff check --fix .
• .venv/bin/ruff format .
• make check-slurm
• make test-slurm (1,572 tests)
• make test-slurm-wheel-install
• Slurm public-artifact audit
• Claude independent review plus remediation follow-up
• real PDX profile init/validate: hostname selection, writable Lustre workspace, and gpus_per_node: auto resolved to 8
• real PDX CPU prebuilt-SQSH inspection: job 6924629 completed 0:0; a fresh process resolved the published alias and found zero residual lifecycle jobs

Part of #874

[greptile-apps]

Confidence Score: 4/5

The implementation appears behaviorally sound, but the explicit repository naming requirement must be satisfied before merging.

Summary

• Adds profile init and profile validate CLI and service paths.
• Resolves automatic GPU counts against the selected or default Slurm partition.
• Connects image preparation, submission, observation, verification, publication, and registry updates.
• Retains lifecycle artifacts when cancellation cannot be confirmed safely.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[image add] --> B[Resolve Slurm profile]
    B --> C[Prepare lifecycle directory and script]
    C --> D[Submit Slurm job]
    D --> E[Observe queue and accounting]
    E -->|Successful terminal accounting| F[Inspect and publish image]
    F --> G[Atomically update registry]
    E -->|Definitive failure| H[Clean lifecycle state]
    E -->|Unknown submission or unconfirmed cancellation| I[Retain state for recovery]

Loading

_{Reviews (3) · Last reviewed commit: "fix(slurm): require terminal cancellatio..."}

[nabinchha]

Thanks for putting this together, @andreatnvidia!

Summary

This adds production profile initialization/validation and wires the existing structured image lifecycle into the public service and CLI, matching the PR's stated intent. I independently confirmed the three existing Greptile findings around partitionless GPU discovery, cancellation/cleanup ordering, and dangling-symlink overwrite protection, so I have not duplicated them below; they remain blocking.

Findings

Warnings — Worth addressing

packages/data-designer-slurm/src/data_designer/slurm/services/profiles.py:176 — Malformed Slurm output is reported as invalid profile input

• What: SlurmCommandOutputError inherits from both SlurmLauncherError and ValueError, but the ValueError handler appears before the SlurmLauncherError handler. If sinfo returns malformed GPU data, validate() therefore returns INVALID_REQUEST with "profile configuration cannot be resolved" instead of the intended UNAVAILABLE/"Slurm is unavailable" result.
• Why: The profile can be perfectly valid while the scheduler response is broken. Misclassifying that infrastructure failure as caller input produces the wrong CLI exit code and can make callers edit configuration instead of retrying or investigating Slurm.
• Suggestion: Catch SlurmLauncherError before the tuple containing ValueError, and add a test whose launcher raises SlurmCommandOutputError from query_gpu_counts() and expects UNAVAILABLE.

Suggestions — Take it or leave it

packages/data-designer-slurm/tests/services/test_image_wiring.py:288,305 — Keep the new test helpers flat and fully typed

• What: The interrupt test introduces a local interrupt() function even though it captures no local state, and the new _service() helper omits its return annotation.
• Why: These are both avoidable departures from the project style guide: helpers should live at module scope unless closure capture materially helps, and annotations are required in tests as well as production code.
• Suggestion: Move the interrupting sleeper to a module-level private helper and annotate _service() as returning SlurmImageService.

What Looks Good

• The image flow composes the existing prepare, submit, observe, verify, publish, and registry boundaries instead of introducing a parallel lifecycle path.
• Ambiguous submission outcomes retain their lifecycle state, while terminal evidence and exit status are checked before publication.
• Validation was broad: changed-file lint/format checks passed, 54 focused service/CLI tests passed, and the full Slurm suite produced 1,571 source-tree passes plus the environment-only entry-point test passing after installing the PR package (1,572 effective passes).

Residual Risk

The real-cluster checks in the PR are valuable, but failure and cancellation timing remains scheduler-dependent; the existing cleanup-race finding should be covered with a terminal-confirmation test as part of its fix.

Verdict

Needs changes — Please resolve the three existing P1 findings and correct the launcher-error classification before merge. The test-helper cleanup is non-blocking.

---

This review was generated by an AI assistant.

[nabinchha]

Thanks for the follow-up, @andreatnvidia. I re-reviewed the complete PR at 18035e2390a140020d0f9edba0488be8697277b3, with particular attention to scheduler discovery, cancellation cleanup, profile error handling, filesystem safety, and the updated tests.

Summary

The current head resolves all previously blocking behavior: implicit GPU discovery is restricted to the default partition, cancellation cleanup waits for terminal accounting evidence, dangling destination symlinks cannot bypass no-overwrite behavior, and malformed Slurm output is classified as an availability failure. The avoidable nested test helper and missing return annotation were also cleaned up.

Findings

No new blocking or warning-level findings.

What Looks Good

• Partitionless GPU discovery now parses only the default partition while retaining explicit-partition behavior and rejecting ambiguous/malformed output.
• Cancellation retains lifecycle artifacts unless terminal accounting evidence confirms the job has stopped; this closes the cleanup race without hiding uncertain state.
• Resolving only the destination parent preserves a dangling final symlink for the atomic no-overwrite check.
• SlurmLauncherError is handled before ValueError, so malformed scheduler output maps to UNAVAILABLE as intended.
• The new test helper is flat and typed, and the regression coverage exercises the corrected error and lifecycle paths.
• Validation is strong: lint and formatting checks pass; 206 focused tests pass; the broader Slurm suite produced 1,578 passes with only the isolated-worktree package-entry-point metadata check failing because that worktree was not installed as a distribution. The visible GitHub checks are green.

Residual Risk

The five-minute cancellation/accounting behavior remains scheduler-dependent and should still receive the documented real-cluster acceptance coverage.

Verdict

Ship it. The previously blocking findings are fixed and the uncertain cancellation path now fails safely by retaining state.

---

This review was generated by an AI assistant.

[andreatnvidia]

@nabinchha Thanks for the re-review and for confirming the fixes. Agreed that the five-minute cancellation/accounting path remains the main real-cluster acceptance case.