feat: add one-node Slurm runtime - #909
Conversation
Greptile SummaryThe PR adds an allocation-local one-node Slurm runtime and completes the previously requested restart recovery behavior.
|
| Filename | Overview |
|---|---|
| packages/data-designer-slurm/src/data_designer/slurm/runtime/controller.py | Adds allocation lifecycle orchestration and now reloads persisted readiness before publishing a monotonic restart epoch for requeued running attempts. |
| packages/data-designer-slurm/src/data_designer/slurm/state/readiness.py | Adds the explicit RESTARTING state and validates reset deployment snapshots consistently. |
| packages/data-designer-slurm/src/data_designer/slurm/state/reconciliation.py | Allows nonterminal readiness states to enter a reset restart epoch while preserving terminal-state restrictions and revision monotonicity. |
| packages/data-designer-slurm/src/data_designer/slurm/runtime/supervisor.py | Centralizes shell-free process launch, required-process checks, termination, and cleanup confirmation. |
| packages/data-designer-slurm/src/data_designer/slurm/runtime/bundle.py | Builds and atomically publishes a deterministic, restrictive allocation runtime archive. |
| packages/data-designer-slurm/src/data_designer/slurm/integration.py | Splits candidate and winner validation while retaining identity, count, path, digest, and chronology checks. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Load persisted attempt] --> B{Attempt state}
B -->|SUBMITTED or PENDING| C[Run allocation preflight]
B -->|RUNNING| D[Load persisted readiness]
D --> E[Publish next-revision RESTARTING state]
E --> C
C --> F[Mark attempt RUNNING]
F --> G[Launch servers]
G --> H[Probe backends]
H --> I[Launch and probe endpoints]
I --> J[Publish READY]
J --> K[Run client and validate candidate]
K --> L[Clean up managed processes]
L --> M[Publish STOPPED]
M --> N[Persist terminal attempt]
C -->|failure| O[Publish failure state]
G -->|failure| O
H -->|failure| O
I -->|failure| O
K -->|failure| O
O --> L
Reviews (11): Last reviewed commit: "fix: make allocation restarts explicit" | Re-trigger Greptile
| def _publish_readiness(self, state: ReadinessState) -> None: | ||
| revision = 1 if self._readiness is None else self._readiness.revision + 1 |
There was a problem hiding this comment.
Restart resets readiness revision
When the runtime restarts an attempt already in RUNNING state with a persisted readiness snapshot, _readiness remains unset and this code publishes revision 1 again. SlurmStateWriter.write_readiness rejects that non-monotonic update, causing the otherwise executable attempt to enter the failure path instead of resuming.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-slurm/src/data_designer/slurm/runtime/controller.py
Line: 335-336
Comment:
**Restart resets readiness revision**
When the runtime restarts an attempt already in `RUNNING` state with a persisted readiness snapshot, `_readiness` remains unset and this code publishes revision 1 again. `SlurmStateWriter.write_readiness` rejects that non-monotonic update, causing the otherwise executable attempt to enter the failure path instead of resuming.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in a5ece02. A requeued RUNNING attempt now loads its persisted readiness snapshot before launching, continues at the next revision, and suppresses PENDING/STARTING publications that would regress an already advanced readiness state. Regressions cover persisted STARTING and READY snapshots and verify revision 7 resumes at revision 8.
d819d5f to
c01a2cb
Compare
79c89e3 to
286a81e
Compare
| _add_archive_file(archive, _ENTRYPOINT_NAME, _ENTRYPOINT, mode=_ENTRYPOINT_MODE) | ||
| _add_archive_file(archive, _SLURM_PACKAGE_NAME, _SLURM_PACKAGE_SHIM, mode=_SOURCE_MODE) | ||
| source_root = Path(__file__).parent | ||
| for source_path in sorted(source_root.glob("*.py")): |
There was a problem hiding this comment.
[P3] One thing that may get awkward as this runtime grows: glob("*.py") only includes modules directly under runtime/. Moving code into a subpackage for the multi-node work would behave normally in the checkout but leave those files out of the allocation bundle. It might be worth packaging a dedicated runtime package, or recursively collecting an explicit manifest and checking that the extracted bundle imports cleanly.
There was a problem hiding this comment.
Fixed in e9d254a. Runtime source collection is now recursive and deterministic, and the archive contains an explicit runtime-sources.txt manifest. Tests stage a synthetic nested package, import it from the extracted bundle, and import every module listed by the real bundle manifest. The isolated wheel-install check also passes.
| client_result: ClientResult, | ||
| candidate: CandidateOutputManifest, | ||
| ) -> None: | ||
| _validate_candidate_identities(context, client_result, candidate) |
There was a problem hiding this comment.
[P3] We now have the candidate rules in a few places: here, ClientResult, and PlanStateValidator.validate_finalization_chain. That makes resume or winner-policy changes easy to update on one side but miss on another, so allocation success and winner finalization could drift apart. Could PlanStateValidator expose a shared validate_client_candidate(...) step that both paths call?
There was a problem hiding this comment.
Fixed in e9d254a. PlanStateValidator.validate_client_candidate(...) is now the authoritative cross-record check for identities, counts, resume policy, dataset/manifest location, digest, and timestamps. Both runtime result loading and validate_finalization_chain(...) call it; the runtime boundary still classifies partial/failed client outcomes as CLIENT_FAILED before candidate loading. Added direct pre-terminal and boundary-order regressions.
|
|
||
|
|
||
| @contextmanager | ||
| def _defer_termination_signals() -> Iterator[None]: |
There was a problem hiding this comment.
[P3] The signal handling is spread across the entrypoint and supervisor now: one installs handlers, another temporarily replaces them, and cleanup masks them again. It works for the current main-thread flow, but start() fails if launch work later moves to a background thread, and future signal changes would need coordinated edits in several places. A small signal coordinator owned by the entrypoint and passed into the supervisor would keep this easier to reason about.
There was a problem hiding this comment.
Fixed in e9d254a. TerminationSignalCoordinator is created by the entrypoint and passed explicitly into StepSupervisor. It now owns handler installation, deferred delivery, and cleanup-time blocking; the supervisor no longer installs or replaces handlers itself. A regression launches start() on a worker thread, and the spawn-race test confirms deferred termination is replayed only after registration and cleanup owns the child.
| tarfile.open(fileobj=compressed, mode="w", format=tarfile.PAX_FORMAT) as archive, | ||
| ): | ||
| _add_archive_file(archive, _ENTRYPOINT_NAME, _ENTRYPOINT, mode=_ENTRYPOINT_MODE) | ||
| _add_archive_file(archive, _SLURM_PACKAGE_NAME, _SLURM_PACKAGE_SHIM, mode=_SOURCE_MODE) |
There was a problem hiding this comment.
[P2] I think there’s still a deployment dependency hiding here. The archive contains only the runtime modules, while the package shim and entrypoint load SlurmStateWriter and the other contracts from whatever data-designer-slurm installation happens to be available on the compute node. An older or missing installation will fail before the runtime starts, and the same bundle digest could execute against different state code. Do we guarantee the exact wheel on every node? Otherwise, could the bundle include or verify the required package version?
There was a problem hiding this comment.
Fixed in 823c77d. The content-addressed archive now carries the complete data_designer.slurm Python package—including state, contracts, planning, client, and runtime—instead of extending an ambient installation. The bundle digest therefore pins the exact Slurm implementation used on the compute node. Tests assert the package contents and import every module from the extracted manifest; make test-slurm (1,173 tests), make check-slurm, and the isolated wheel-install suite all pass.
| if self._attempt.state not in { | ||
| AttemptLifecycleState.SUBMITTED, | ||
| AttemptLifecycleState.PENDING, | ||
| }: | ||
| raise SlurmRuntimeError( | ||
| SlurmRuntimeErrorCode.INVALID_CONTEXT, | ||
| "allocation attempt is not executable", | ||
| ) |
There was a problem hiding this comment.
Requeued attempts are terminally failed
When Slurm reinvokes an array task whose persisted attempt is already RUNNING, the deterministic attempt directory causes the same manifest to be loaded, but this validation rejects it before launching any steps and the controller persists FAILED. No production path in this package creates the replacement attempt required for recovery, so the shard remains unexecuted.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-slurm/src/data_designer/slurm/runtime/controller.py
Line: 282-289
Comment:
**Requeued attempts are terminally failed**
When Slurm reinvokes an array task whose persisted attempt is already `RUNNING`, the deterministic attempt directory causes the same manifest to be loaded, but this validation rejects it before launching any steps and the controller persists `FAILED`. No production path in this package creates the replacement attempt required for recovery, so the shard remains unexecuted.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in a5ece02. RUNNING is now an executable recovery state for the same scheduler-owned attempt: the controller reloads nonterminal readiness, restarts the allocation-owned preflight/server/endpoint/client process sequence, and preserves monotonic readiness through completion. FAILED or STOPPED readiness remains non-restartable. The STARTING and READY requeue regressions complete successfully and the full Slurm suite passes (1,178 tests).
823c77d to
a5ece02
Compare
| if self._attempt.state not in { | ||
| AttemptLifecycleState.SUBMITTED, | ||
| AttemptLifecycleState.PENDING, | ||
| AttemptLifecycleState.RUNNING, |
There was a problem hiding this comment.
[P1] One recovery edge still looks like it will block real requeues. A RUNNING attempt keeps the same attempt directory, and SubprocessStepRunner creates fixed log names with O_EXCL. Once a step has created its .out or .err files, restarting it hits FileExistsError before anything can relaunch. The requeue test uses the fake runner, so it misses this path. Could each execution use its own log directory, or could the existing artifacts be handled explicitly before restarting?
There was a problem hiding this comment.
Fixed in 69574fa. Every controller execution now binds all preflight, server, endpoint, and generation steps to logs/execution-<initial-readiness-revision>/, while the restrictive O_EXCL creation rule remains intact. Requeues therefore preserve prior logs and create a fresh namespace. The production subprocess regression now launches the same step ID successfully in two execution directories.
| self._clock.sleep(self._poll_interval_seconds) | ||
|
|
||
| def _publish_readiness(self, state: ReadinessState) -> None: | ||
| if self._readiness is not None and not _can_advance_readiness(self._readiness.state, state): |
There was a problem hiding this comment.
[P2] There’s also a window where the persisted status can be misleading. If the previous snapshot is READY, the PENDING and STARTING updates are skipped while the old processes are gone and the replacement services are still booting. Anyone reading state continues to see READY/PUBLISHED during that restart. Could recovery use an execution epoch or another transition that keeps readiness accurate without losing monotonic revisions?
There was a problem hiding this comment.
Fixed in 69574fa. A requeued RUNNING attempt now publishes an explicit next-revision RESTARTING snapshot before allocation preflight. That snapshot resets ready backends to zero and endpoint publication to pending, so observers no longer see stale READY/PUBLISHED state while services are absent. Transition validation allows this reset only from nonterminal readiness through RESTARTING, and subsequent revisions remain strictly monotonic.
69574fa to
e73b016
Compare
andreatnvidia
left a comment
There was a problem hiding this comment.
Nice work tightening this up. The explicit restart state and execution-scoped logs cleanly handle the requeue cases we discussed, and the overall runtime flow looks solid now. LGTM!
- stage and verify a deterministic allocation runtime bundle - launch structured server, endpoint, and client steps with one cleanup owner - publish readiness and validate complete attempt-local candidates - cover preflight, failure, signal, backpressure, and filesystem boundaries Part of #868 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Do not relaunch a RUNNING attempt without persisted ownership of its prior process groups. Fail it before starting new steps so later retry can create a fresh attempt without resetting readiness revisions. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Contain interruptions from failed-readiness bookkeeping so the controller still cleans owned process groups and persists terminal failure state. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Confirm each artifact path still names the descriptor-verified inode after hashing so later launch steps cannot reopen a concurrently replaced file. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Limit buffered backend response bodies and return a bounded proxy error when a local model server exceeds the reviewed response size. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Repair a content-addressed runtime archive when interruption leaves its package-owned temporary hard link behind, while preserving immutable inode checks. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Avoid buffering readiness response bodies when only the HTTP status is required, while still closing every probe connection. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Register each allocation port socket for cleanup before socket configuration and bind can fail. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Strip Connection-nominated and Proxy-Connection headers on both sides of the allocation-local HTTP proxy. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Recursively manifest and import-check bundled runtime packages. Reuse plan-aware client/candidate validation across runtime and finalization, and coordinate termination handling through one entrypoint-owned collaborator. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Include the complete Slurm package in the content-addressed archive so compute nodes execute the exact state, contracts, planning, client, and runtime sources represented by the bundle digest. This removes the runtime's dependency on an ambient data-designer-slurm installation. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Allow a scheduler-requeued RUNNING attempt to restart its owned processes while continuing any persisted readiness revision. Suppress readiness regressions during recovery and reject restart only after terminal readiness. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
e73b016 to
6127fa4
Compare
Summary
Adds the one-node allocation-local runtime for resolved Slurm plans. It verifies persisted launch intent, launches server/endpoint/client steps, publishes monotonic state, proxies model traffic, and owns complete cleanup through focused runtime collaborators.
Related Issue
Part of #868 (868#2).
Merge Order and Dependencies
feat/slurm-execution. This branch is rebased directly onto that merged base.Changes
Added
srunconstruction, process-group supervision, monotonic state publication, and a bounded loopback least-connections proxy.TerminationSignalCoordinatorfor handler installation, spawn deferral, cleanup blocking, and deferred delivery.RESTARTINGreadiness and execution-scoped restrictive log directories keyed by the restart revision.Changed
data_designer.slurmpackage in a deterministic source manifest and import-checked every manifested module after extraction.Fixed
RUNNINGattempts through an explicit restart epoch while preserving monotonic readiness revisions.READY/PUBLISHEDstate before restart preflight so observers do not see unavailable services as ready.O_EXCLlog filenames by isolating every execution's logs.mask_gpucovering its assigned GPUs without manually forwardingCUDA_VISIBLE_DEVICES.Retry-Afterresponse to the caller.STOPPEDonly after every managed process was confirmed exited.CLIENT_FAILEDclassification for partial or failed client results before candidate-manifest loading.data-designer-slurminstallation.Attention Areas
runtime/controller.py— lifecycle ordering, explicit restart publication, and terminal-state publication.runtime/logs.py— execution-scoped path binding and restrictive descriptor-safe creation.runtime/signals.py— centralized signal ownership and deferred delivery.runtime/supervisor.py— process ownership and cleanup confirmation.runtime/bundle.py— deterministic complete-package source packaging.integration.py— shared plan-aware candidate validation.Testing
make check-slurmmake test-slurm— 1,183 passed after rebasing onto merged feat: add Slurm state writer #908make test-slurm-wheel-install— CLI overhead -0.001sChecklist
Description updated with AI