Skip to content

feat: define Slurm runtime and state record contracts - #878

Merged
nabinchha merged 4 commits into
feat/slurm-executionfrom
codex/865-slurm-state-contracts
Aug 20, 2026
Merged

feat: define Slurm runtime and state record contracts#878
nabinchha merged 4 commits into
feat/slurm-executionfrom
codex/865-slurm-state-contracts

Conversation

@nabinchha

Copy link
Copy Markdown
Contributor

📋 Summary

Defines strict, versioned records shared across Slurm submission, allocation-local execution, fresh-process observation, retry, and collection. This establishes the deterministic serialization and validation boundary required by later Slurm implementation work without coupling consumers to scheduler commands or runtime services.

🔗 Related Issue

Closes #865

Related to #850. Builds on #851, #852, and #853.

🔄 Changes

✨ Added

  • Add immutable Pydantic records for run, shard, attempt, scheduler, readiness, candidate-output, winner, and collection state.
  • Add deterministic canonical and human-readable JSON serialization with SHA-256 content digests.
  • Add pure cross-record validators for identity, chronology, shard ownership, winner publication, collection inputs, and readiness reconciliation.
  • Add checked-in golden records covering successful, failed, stale-readiness, accounting-lag, single-node, and multi-node scenarios.
  • Add comprehensive positive and negative tests for record and cross-record invariants.

🔧 Changed

  • Declare Pydantic as a direct dependency of the optional Slurm package.
  • Extend isolated wheel verification to validate the state API and packaged dependency metadata.

🐛 Fixed

  • Require explicit schema versions and reject unknown or malformed record data.
  • Reject overlapping shard ranges, shared resume workspaces, partial winners, regressive probe evidence, and stale or future reconciliation evidence.
  • Give terminal attempt, terminal scheduler, and failed-readiness evidence deterministic precedence.

🔍 Attention Areas

⚠️ Reviewers: Please pay special attention to the following:

🧪 Testing

  • make check-slurm
  • .venv/bin/ruff format --check scripts/test_slurm_package_install.py
  • .venv/bin/ruff check scripts/test_slurm_package_install.py
  • make test-slurm (80 passed)
  • State tests with 100% statement coverage (510/510 statements)
  • make test-slurm-wheel-install (0.456 s base median, 0.461 s extension median, 0.005 s overhead)
  • make test — not run; validation was scoped to the optional Slurm package and its isolated wheel boundary.
  • Unit tests added and updated
  • E2E tests — N/A for pure serialized record contracts

✅ Checklist

  • Follows commit message conventions
  • DCO is covered by the repository author-level sign-off workflow
  • Architecture docs — N/A; broader runtime architecture remains outside this contract-only task

Define strict, versioned execution, readiness, output, reconciliation, and scheduler records. Enforce cross-record identity, digest, shard-set, and terminal-state invariants with deterministic golden fixtures and built-wheel coverage.

Closes #865
Reject partial winners and conflicting shard ownership. Require explicit versions, enforce evidence chronology and readiness precedence, and cover every state-contract path with validated fixtures.

Refs #865
@nabinchha
nabinchha requested a review from a team as a code owner August 18, 2026 21:07
@greptile-apps

greptile-apps Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR establishes immutable, versioned Slurm records and deterministic serialization as the shared boundary between planning, execution, observation, retry, and collection.

  • Adds run, shard, attempt, scheduler, readiness, candidate-output, winner, and collection records.
  • Adds cross-record lifecycle, identity, chronology, ownership, publication, and reconciliation validation.
  • Adds golden fixtures and focused contract tests.
  • Declares Pydantic as a direct dependency of the optional Slurm package and extends isolated-wheel verification.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/data-designer-slurm/src/data_designer/slurm/contracts.py Defines strict immutable contract primitives, path constraints, deterministic JSON serialization, and content digests.
packages/data-designer-slurm/src/data_designer/slurm/state/execution.py Defines versioned run, shard, and attempt records with lifecycle consistency constraints.
packages/data-designer-slurm/src/data_designer/slurm/state/outputs.py Defines candidate-output, winner, and collection records with record-count and uniqueness invariants.
packages/data-designer-slurm/src/data_designer/slurm/state/readiness.py Defines deployment and attempt readiness snapshots with aggregate-state consistency checks.
packages/data-designer-slurm/src/data_designer/slurm/state/reconciliation.py Implements readiness transition validation and deterministic precedence for attempt observations.
packages/data-designer-slurm/src/data_designer/slurm/state/validation.py Implements cross-record identity, chronology, ownership, winner-publication, and collection validation.
packages/data-designer-slurm/pyproject.toml Declares Pydantic as a direct dependency of the optional Slurm distribution.
scripts/test_slurm_package_install.py Extends isolated-wheel verification to cover the packaged state API and dependency metadata.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Run[Run manifest] --> Shards[Shard manifests]
    Shards --> Attempts[Attempt manifests]
    Attempts --> Scheduler[Scheduler observations]
    Attempts --> Readiness[Readiness snapshots]
    Scheduler --> Reconcile[Observation reconciliation]
    Readiness --> Reconcile
    Attempts --> Candidates[Candidate outputs]
    Candidates --> Winners[Shard winners]
    Winners --> Collection[Collection plan]
Loading

Reviews (3): Last reviewed commit: "refactor(slurm): share contract primitiv..." | Re-trigger Greptile

Comment thread packages/data-designer-slurm/src/data_designer/slurm/state/base.py Outdated
shard_indices == tuple(range(run.shard_count)),
"shards must be ordered by a complete zero-based shard index",
)
for previous, current in zip(shards, shards[1:]):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This accepts [0, 100) and [200, 300) as an exact two-shard set. Because RunManifest has no total-record bound and this validator never consults #873's resolved shard intent, all winners can validate while collection omits the missing range. Can we add the total record count to the run contract and require the ranges to form a contiguous [0, total_records) partition, with a gapped-range test?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is a cleaner answer to this now than the extra field I originally suggested. The resolved plan owns the requested total and complete ordered shard set, so RunManifest can stay minimal and the integration validator can compare every state shard directly with its planned shard. How does that ownership split sound?

@nabinchha nabinchha Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed on the ownership split. The sequence is: #878 lands the canonical ShardId, RecordRange, ResumeWorkspace, and state-local checks; #879 then rebases, imports those exact public types, and removes its duplicate definitions; after that, a plan-aware integration validator compares the ordered state shards directly with ResolvedPlan.shards, including identities, indices, ranges, input partitions, and workspace paths. I am intentionally not duplicating the requested total in RunManifest. I am leaving this thread open until that integration check lands.

The follow-up integration work is tracked in #880: #880

revision: PositiveInt
updated_at: datetime
state: ReadinessState
deployments: tuple[DeploymentReadiness, ...] = Field(min_length=1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When #873 adds the resolved deployment contract, could we add a plan-aware initial-snapshot validator that checks this tuple's identity, order, and expected backend counts? Transition validation preserves these values after the first snapshot, but nothing currently anchors the initial snapshot to authored or resolved intent.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The resolved deployment shape now has a stable deployment_id, the unrestricted authored model_alias, and topology.replica_count, all in deployment order. Those seem like the right fields for anchoring the initial readiness snapshot, while the existing transition checks preserve them afterward. Let me know if that lines up with how you intended these entries to identify deployments.

@nabinchha nabinchha Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, those are the intended anchor fields. Following the same sequence, #878 lands the canonical state and shared primitives first, then #879 rebases and reuses those exact classes. Once both contracts are on the same base, the initial-snapshot validator should compare deployment order, deployment_id, the unrestricted model_alias, and topology.replica_count; the existing transition validator preserves them afterward. I am leaving this open for that post-rebase integration.

The follow-up integration work is tracked in #880: #880

@andreatnvidia andreatnvidia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @nabinchha. The state boundary is well scoped. I found two contract issues that need to be fixed before the schema is frozen: referenced record digests do not match persisted file bytes, and exact shard sets can omit record ranges because the run contract has no total. I also left a non-blocking #873 integration note for anchoring the initial readiness snapshot to the resolved deployment list.

return shards


def validate_attempt_manifest(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we need attempt-set and transition validation alongside this single-manifest check. Two shards can currently claim the same (array_job_id, array_task_id), and a later manifest can change its scheduler identity or move from SUCCEEDED back to RUNNING. Adding scheduler-identity uniqueness and terminal-immutability checks would preserve the contracts required by #865.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Each planned shard now persists its array_task_index, along with the shard-NNNNN identity. When we connect the contracts, I think each attempt should match that task index and use attempt-NNNN derived from its ordinal. That would also catch mappings where task IDs are unique but assigned to the wrong shards.

@nabinchha nabinchha Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The state-owned portion is fixed in 7a4b1e0: validate_attempt_set enforces unique attempt identities, ordinals, and scheduler identities, while validate_attempt_transition preserves scheduler and candidate identity, enforces monotonic lifecycle, and makes terminal records immutable. The plan-owned mapping follows the agreed sequence: #878 first, #879 rebase and shared-type adoption second, then the integration validator checks array_task_index and the ordinal-derived attempt ID against the planned shard. I am leaving this open until that join lands.

The follow-up integration work is tracked in #880: #880

@andreatnvidia

Copy link
Copy Markdown
Contributor

A few non-blocking follow-ups seem worth folding in while these contracts are fresh:

  • validate_absolute_path() accepts // and //host/path, including the root-equivalent //, despite rejecting /. Rejecting multiple leading slashes would keep the normalization and uniqueness checks consistent.
  • The golden records use shard-0000 and candidate-output.json, while F3 uses shard-00000 and output-manifest.json. Aligning the fixture spelling now would prevent dependent lanes from copying conflicting conventions.
  • If DeploymentReadiness.model_alias is intended to preserve the authored ModelConfig.alias, its Identifier type narrows the current public contract by rejecting spaces, slashes, Unicode, and aliases over 128 characters. It would be good to either retain the authored alias as a string or make Define shared Slurm configuration and execution-plan contracts #873’s alias-to-deployment-ID mapping explicit.

Comment thread packages/data-designer-slurm/src/data_designer/slurm/state/base.py Outdated
Hash persisted record bytes, align state identities with the resolved plan vocabulary, and preserve public model aliases.

Add monotonic attempt and scheduler transition checks, including fixed accounting-lag deadlines and UNKNOWN precedence.

Refs #865
Make planning and runtime state consume one public immutable model family. Align shard, attempt, range, artifact, and resume workspace shapes so the configuration-plan branch can rebase without preserving duplicate Pydantic types.\n\nRefs #865
@nabinchha

nabinchha commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — the standalone follow-ups are addressed in 7a4b1e0 and 2ecc3f7:

  • absolute paths reject repeated leading slashes;
  • the golden records use shard-NNNNN and output-manifest.json;
  • model_alias preserves the unrestricted authored value while deployment_id remains the stable identifier;
  • record digests hash the exact persisted serialize_json bytes;
  • attempt-set, transition, and accounting-lag invariants are covered.

For the cross-PR work, the intended sequence is:

  1. feat: define Slurm runtime and state record contracts #878 lands the canonical public contract primitives and runtime-state records.
  2. feat: define Slurm config and plan contracts #879 rebases onto that base, imports data_designer.slurm.contracts, and removes its duplicate shared definitions.
  3. Plan-aware joins then validate state shards against planned shards, initial readiness against resolved deployments, attempts against planned task identities, and ClientResult through candidate/winner finalization.

I left those four integration threads open until step 3 lands. Local validation is green: Slurm lint, 90 tests, 100% contract/state coverage, and isolated wheel installation.

The four plan-aware joins and their tests are tracked in #880: #880

@andreatnvidia andreatnvidia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for working through the feedback. With the remaining integration covered by #880, this looks good to me.

@nabinchha
nabinchha merged commit 3d152b2 into feat/slurm-execution Aug 20, 2026
7 checks passed
@nabinchha
nabinchha deleted the codex/865-slurm-state-contracts branch August 20, 2026 17:31
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.

2 participants