feat(slurm): add deterministic benchmark workflows

[andreatnvidia]

Summary

• Implements Run, observe, and analyze Slurm benchmarks #877 first: deterministic benchmark compilation, immutable benchmark/run identity, ordinary child submission, fresh-process observation, and stable analysis.
• Consumes that core in Expose Slurm services, CLI, packaging, and documentation #874 slice 4: public benchmark service wiring, thin run/analyze CLI commands, installed-package coverage, and Fern workflow documentation.
• Preserves explicit pending, accounting-lag, failed, incomplete, missing, stale, and scheduler-inconsistent outcomes without a resident controller.
• Adds safe force resume for initialized children lacking submission evidence while never overwriting submitted or partial state.

Validation

• make check-slurm
• make test-slurm: 1584 passed
• make test-slurm-wheel-install
• make check-fern-links
• repository-wide ruff check --fix and ruff format
• Claude independent review plus two remediation follow-ups

Real-cluster acceptance remains a release gate because no cluster profile or Slurm environment is available in this workspace.

Closes #877
Part of #874

[github-actions]

Fern preview: https://nvidia-preview-pr-932.docs.buildwithfern.com/nemo/datadesigner

Fern previews include the docs-website version archive with PR changes synced into latest. Notebook tutorials are rendered without execution outputs in previews.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge; no outstanding correctness, security, or repository-rule violation was identified.

Summary

• Derives immutable benchmark and child-run identities from canonical configuration.
• Persists benchmark manifests before submitting ordinary child runs and supports safe resumptions.
• Reconstructs per-shard measurements from durable state and produces stable recommendations.
• Adds benchmark run/analyze CLI commands and public service construction.
• Adds retry, collection, profile, and image-lifecycle production adapters with state reconciliation.
• Documents benchmark configuration, execution, observation, and analysis.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Benchmark configuration] --> B[Deterministic compiler]
    B --> C[Immutable benchmark manifest]
    B --> D[Ordinary child-run configurations]
    C --> E[Benchmark store]
    D --> F[Slurm run service]
    F --> G[Persisted child state]
    G --> H[Fresh-process observer]
    E --> I[Benchmark analyzer]
    H --> I
    I --> J[Ordered report and recommendations]

Loading

_{Reviews (4) · Last reviewed commit: "merge Slurm M3 public workflows"}

[nabinchha]

Thanks for putting this together, @andreatnvidia!

Summary

This adds deterministic benchmark expansion, ordinary child-run submission, persisted observation, analysis, CLI wiring, and user documentation, which matches the stated intent of the PR. I also independently confirmed the cross-shard timing and aggregate GPU-cost findings already posted by Greptile, so I have not duplicated them below.

Findings

Critical — Let's fix these before merge

packages/data-designer-slurm/src/data_designer/slurm/benchmark/execution.py:118 — Concurrent benchmark invocations can corrupt a submitted child

• What: Child idempotency is implemented as an unlocked check followed by submission. Two processes running the same deterministic benchmark can both observe that a child has no submission and both call the ordinary run service. After one process records attempt-0001, the other process's record conflicts; its cleanup then calls record_submission_failure, which loads that shared attempt and marks it cancelled even though it belongs to the first process's still-valid job.
• Why: A harmless retry or two operators starting the same benchmark concurrently can submit duplicate jobs and, worse, persist the surviving job's attempt as failed/cancelled. That makes the benchmark state disagree with Slurm and defeats the deterministic/idempotent contract this feature is meant to provide.
• Suggestion: Serialize the child-exists check and submission with a benchmark/child submission lock, or atomically claim ownership before submitting. Cleanup should also only mutate an attempt after verifying that its scheduler identity belongs to the job being cancelled. Please add a two-run concurrency test that forces both callers past the existence check.

Warnings — Worth addressing

packages/data-designer-slurm/src/data_designer/slurm/services/wiring.py:755 — Benchmark children lose selected-profile provenance

• What: The benchmark factory resolves a full SelectedSlurmProfile, but creates each child service with only selected.profile. create_slurm_run_service() resolves that value again as an injected profile, discarding the original cluster name, selection source, catalog path/digest, and hostname match.
• Why: Child plans launched from --profile-file/--cluster will claim the profile was injected and cannot be audited back to the catalog selection that actually drove the benchmark.
• Suggestion: Add an internal construction path that accepts and preserves the existing SelectedSlurmProfile (or share the already-built preparer/dependencies), and test that a catalog-selected benchmark child plan retains the original selection provenance.

Suggestions — Take it or leave it

packages/data-designer-slurm/src/data_designer/slurm/cli_benchmark.py:35 — Avoid the extra nested command callbacks

• What: Both CLI commands define another local run()/analyze() function inside an already nested Typer command function.
• Why: These extra levels make the command flow and unit testing harder to follow, and the project style guide asks us to avoid nested functions where practical.
• Suggestion: Move the actual run/analyze operations to typed module-level private helpers and pass them with functools.partial (or another small callable). The decorated command closures can remain where Typer dependency capture makes them useful.

What Looks Good

• The compiler gives cases and child runs stable identities, and the persisted benchmark manifest makes repeat analysis reproducible.
• Reusing the ordinary run service keeps benchmark submission on the same artifact and state boundaries as normal Slurm execution.
• The new workflow coverage exercises compilation, resumable submission, observation failures, analysis, and CLI forwarding across the main feature path.

Residual Risk

Real-cluster acceptance remains deferred, so scheduler/accounting behavior and the reported measurements still need the release-gate validation described in the PR.

Verdict

Needs changes — Please resolve the concurrent child-submission race and profile-provenance loss, along with the two already-open timing/cost findings, before merge. The nested CLI callbacks are non-blocking cleanup.

---

This review was generated by an AI assistant.

[nabinchha]

Thanks for the thorough follow-up, @andreatnvidia. I re-reviewed the complete PR at 4221757a64177f1f04e4d78bcd5602611008f9d9, including the concurrency, accounting, provenance, persistence, CLI, and documentation paths.

Summary

The current head resolves the previously blocking child-submission race, prevents one failed submission from cancelling another job's attempt, preserves selected-profile provenance, and corrects cross-shard timing and target-cost accounting.

Findings

No new blocking or warning-level findings. The previously noted nested CLI callback cleanup remains an optional style nit, so I have not repeated it as a new finding.

What Looks Good

• The per-child submission lock makes the existence check and ordinary-run submission one serialized decision, and the new concurrent-run test exercises the race directly.
• Submission-failure recording is now scoped to the scheduler job ID it owns, so cleanup cannot corrupt a different caller's successful attempt.
• Each shard is measured from its own allocation/readiness timeline, avoiding queue-stagger distortion; aggregate throughput and target task-allocation costs are now derived from those per-shard measurements.
• Benchmark child services retain the exact SelectedSlurmProfile, including catalog and selection provenance.
• Immutable manifests/reports and ordinary child-run boundaries continue to preserve deterministic replay and auditable analysis.
• Validation is strong: lint and formatting checks pass; 79 focused tests pass; the broader Slurm suite produced 1,587 passes with only the isolated-worktree package-entry-point metadata check failing because that worktree was not installed as a distribution. Fern preview and the visible GitHub checks are green.

Residual Risk

Real multi-node scheduler/accounting timing and reported benchmark measurements still need the PR's documented cluster acceptance run.

Verdict

Ship it with nits. The correctness and provenance findings are fixed; the remaining previously raised callback-flattening suggestion is non-blocking.

---

This review was generated by an AI assistant.

[andreatnvidia]

@nabinchha Thanks, these were valid. Commit 4221757a now serializes the child existence check and submission per deterministic run ID, while compensating cleanup only updates attempts owned by the cancelled scheduler job. Regression coverage exercises concurrent invocations and verifies the surviving scheduler state.

Benchmark child construction also reuses the original SelectedSlurmProfile, preserving catalog selection provenance. I corrected the two confirmed array-accounting issues by measuring shards independently and projecting job and GPU cost per task allocation.

I left the nested Typer callbacks unchanged since that is non-blocking cleanup and would be an unrelated CLI refactor.

[andreatnvidia]

@nabinchha I addressed the remaining CLI callback nit in a59f0bfc. The benchmark run and analyze operations are now typed module-level helpers, with the Typer closures passing them through functools.partial.

[nabinchha]

Thanks for addressing the remaining callback cleanup, @andreatnvidia. I re-reviewed PR #932 at a59f0bfc2361c42b4aa0230ce332f92287687908, including the full current benchmark flow and the focused delta from the previously reviewed head.

Summary

The latest commit matches its stated intent: the avoidable inner run/analyze callbacks are now typed module-level helpers, while the Typer command closures that materially capture the app dependencies remain in place. The refactor preserves the existing command inputs, service construction, source-root handling, error boundary, and emitted result behavior.

Findings

No new findings.

What Looks Good

• _run_benchmark() and _analyze_benchmark() have explicit inputs and return types, so their behavior is visible and independently reusable without hidden closure state.
• functools.partial binds the command arguments without changing the zero-argument callable contract expected by the shared invocation/error policy.
• The existing CLI behavior test covers both benchmark commands and confirms their arguments and outputs remain unchanged.
• Focused validation is clean: lint, formatting, and exploratory complexity checks pass, and all 11 Slurm CLI tests pass.

Residual Risk

The earlier real-cluster acceptance caveat remains unchanged; this refactor itself does not alter scheduler or benchmark execution behavior.

Verdict

Ship it. The last optional callback-flattening nit is resolved cleanly, with no new correctness or maintainability concerns.

---

This review was generated by an AI assistant.