feat: add Slurm service foundation

[andreatnvidia]

📋 Summary

Adds the first independently mergeable #874 slice: typed public Slurm service facades and stable result/error contracts backed by private dependency-injected boundaries. This gives M0 a local plan/render service seam without wiring the planner, image, launcher, runtime, state, or benchmark implementations.

🔗 Related Issue

Part of #874

🔄 Changes

• Add SlurmRunService.plan() for immutable plan resolution and render_attempt() for rendering an existing plan without re-resolution.
• Add an immutable in-process RenderedSlurmAttempt result while keeping planner, renderer, image, and benchmark implementation protocols private.
• Add F3-based image resolution and benchmark run/analyze facades without implementing their later integration lanes.
• Normalize public failures through DataDesignerError-based stable codes and operation identifiers while redacting unexpected backend details.
• Add deterministic scripted service fakes and focused public-method, validation, error, export, and cancellation coverage.

🔍 Attention Areas

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

• services/run.py - separate M0 plan/render operations and the retry-safe immutable-plan boundary.
• services/errors.py - public error hierarchy, attribution, and unexpected-error redaction.

🧪 Testing

• Direct four-package pytest suites pass (4,514 passed, 1 skipped)
• make test-slurm passes (434 passed)
• make test-slurm-wheel-install passes
• make check-slurm passes
• Repository-prescribed Ruff fix and format commands pass
• Unit tests added/updated
• E2E tests not applicable to this local/fake foundation slice

✅ Checklist

• Follows commit message conventions
• Commits are signed off (DCO)
• Architecture docs not applicable; this implements the reviewed Stage 2 service boundary without changing it

[greptile-apps]

Greptile Summary

The PR introduces typed, dependency-injected service facades for Slurm run planning and rendering, image resolution, and benchmark execution and analysis.

• Adds stable public operation and error contracts with backend-detail redaction.
• Validates backend result types and correlation with the originating requests.
• Adds deterministic scripted fakes and focused tests for validation, error normalization, cancellation, and serialization.

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/services/errors.py	Defines stable service error codes and operation attribution while redacting unexpected backend failures.
packages/data-designer-slurm/src/data_designer/slurm/services/run.py	Adds injected planning and rendering facades with typed, request-correlated results.
packages/data-designer-slurm/src/data_designer/slurm/services/images.py	Adds a typed image-resolution facade that verifies reference and image-kind correlation.
packages/data-designer-slurm/src/data_designer/slurm/services/benchmark.py	Adds benchmark run and analysis facades with request/result correlation checks.
packages/data-designer-slurm/tests/services/test_services.py	Covers public service behavior, invalid requests, backend failures, cancellation, correlation, and error serialization.
packages/data-designer-slurm/tests/slurm_test_fakes/services.py	Provides deterministic scripted test doubles for each injected service boundary.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Caller[Public caller] --> Run[SlurmRunService]
    Caller --> Image[SlurmImageService]
    Caller --> Benchmark[SlurmBenchmarkService]
    Run --> Planner[Injected run planner]
    Run --> Renderer[Injected batch renderer]
    Image --> Resolver[Injected image resolver]
    Benchmark --> Backend[Injected benchmark backend]
    Planner --> Validate[Validate type and request correlation]
    Renderer --> Validate
    Resolver --> Validate
    Backend --> Validate
    Validate --> Result[Typed public result]
    Planner -. failure .-> Normalize[Normalize SlurmServiceError]
    Renderer -. failure .-> Normalize
    Resolver -. failure .-> Normalize
    Backend -. failure .-> Normalize

Loading

_{Reviews (4): Last reviewed commit: "fix: clarify Slurm service boundaries" | Re-trigger Greptile}

[nabinchha]

Thanks for putting this together, @andreatnvidia!

Summary

This adds the intended typed Slurm service foundation for run planning/rendering, image resolution, and benchmark operations, with stable normalized errors and deterministic fakes. The overall shape matches the PR's stated intent, but two correlation guarantees need tightening before these results are safe to consume downstream.

Findings

Critical — Let's fix these before merge

packages/data-designer-slurm/src/data_designer/slurm/services/run.py:91 — Returned plans are not bound to the requested config

• What: After checking that the planner returned a ResolvedSlurmRunPlan, the service never compares plan.authored_config.sha256 with config.compute_sha256(). I confirmed that plan(authored_run_single) accepts the unrelated multi_node_plan and returns its fully valid script even though the two config digests differ. The same request/result gap exists in services/benchmark.py:47, where run(config) accepts a BenchmarkManifest whose benchmark_config.sha256 identifies different bytes.
• Why: A cache, routing, or backend defect can make the public service return—and later submit—a workload for a different authored request, or make a benchmark caller track the wrong child runs. The existing plan/script, image/reference, and report/ID checks do not cover this first request-to-result link.
• Suggestion: Validate both digest links inside their respective backend closures (for example, reject when plan.authored_config.sha256 != config.compute_sha256() or manifest.benchmark_config.sha256 != config.compute_sha256()), then add mismatched config/plan and config/manifest tests.

Warnings — Worth addressing

packages/data-designer-slurm/src/data_designer/slurm/services/run.py:58 — Script binding validation accepts contradictory declarations

• What: validate_plan_binding() only checks whether the expected declaration occurs somewhere in the script. A script containing a stale readonly DD_PLAN_SHA256="..." followed by the expected declaration is accepted; I reproduced a result with two digest declarations passing validation. The attempt binding has the same behavior.
• Why: Because Bash variables are already readonly after the first declaration, the rendered job can retain the wrong value or abort on the later reassignment while the service advertises the result as correlated to the exact plan and attempt.
• Suggestion: Collect all declarations for each reserved variable and require the list to equal exactly [expected_digest] and [expected_attempt] (and, if the renderer contract fixes their location/order, validate that too). Add duplicate/stale-binding cases to test_run_service_rejects_unbound_render_results.

What Looks Good

• The protocols keep implementation types behind small, typed public seams and preserve the package's layering.
• Unexpected backend exceptions are redacted while normalized non-internal failures retain useful stable codes and operation attribution.
• The tests cover invalid runtime inputs, mismatched image/report outputs, cancellation signals, pickling, and render validation; the focused 33-test service suite plus changed-file Ruff and format checks pass locally.

Verdict

Needs changes — bind run plans and benchmark manifests to the exact requested configs, and reject contradictory script bindings before exposing this as a stable public contract.

---

This review was generated by an AI assistant.

[andreatnvidia]

Good catches. I addressed both in 1a6e4e9c:

• SlurmRunService.plan() now verifies the returned plan’s authored-config digest before rendering. SlurmBenchmarkService.run() applies the equivalent check to the returned manifest.
• SlurmRunPlanResult now requires exactly one expected declaration for both DD_PLAN_SHA256 and DD_ATTEMPT_ORDINAL, rejecting duplicate and stale-first bindings.

The regression coverage includes mismatched plans and manifests plus duplicate and stale-first variants for both script bindings.

[nabinchha]

Thanks for putting this foundation together, @andreatnvidia — the typed boundaries and deterministic fakes are a solid start.

Summary

This PR adds the intended run-planning/rendering, image-resolution, and benchmark service facades with immutable results and normalized public errors. The narrow M0 seam is present, but several cross-record, error-boundary, and public-API commitments are not yet strong or precise enough for a foundation described as stable.

Findings

Warnings — Worth addressing

packages/data-designer-slurm/src/data_designer/slurm/services/benchmark.py:50,70 — Benchmark results are only partially correlated

• What: run() verifies only the config digest, and analyze() verifies only benchmark_id. The checked-in benchmark config has 3 concurrency values × 2 deployment cases, yet the service test patches that config digest onto a manifest with only 2 children and accepts it. I also confirmed that analyze() accepts the golden report even though its benchmark_manifest.sha256 is dddd… while the supplied manifest serializes to 8b2e284c…. Pydantic validates each record's local shape, but no layer validates the config → ordered children → report chain.
• Why: A buggy cache or backend can omit cases, reorder child runs, or return analysis for a stale manifest while the public facade presents the result as correlated. That breaks the deterministic child mapping and fresh-process analysis contract in Run, observe, and analyze Slurm benchmarks #877, and the partial checks duplicate only fragments of an invariant that needs one owner.
• Suggestion: Add benchmark-domain validate_benchmark_manifest(config, manifest) and validate_benchmark_report(manifest, report) functions that check the exact ordered child identities/artifact digests. Change the analysis boundary so the facade can load or receive the authoritative manifest, then use genuinely correlated six-child fixtures instead of repairing only the digest with model_copy(update=...).

packages/data-designer-slurm/src/data_designer/slurm/services/errors.py:34,66 — Error normalization is outside the Data Designer hierarchy and cannot map package errors

• What: SlurmServiceError derives from RuntimeError, unlike the project's public domain errors, which derive from DataDesignerError. invoke_backend() can emit INVALID_REQUEST and INTERNAL itself, but every non-SlurmServiceError becomes INTERNAL; the other advertised codes have no production construction path in this PR. The neighboring image API raises package-owned ImageNotFoundError, ImageConflictError, and ImageVerificationError, while the renderer raises BatchRenderError, so injecting those natural implementations loses their semantics. The workaround is for lower layers to manufacture the public facade error, which moves normalization below the boundary and lets an injected backend mark arbitrary free-form text as safe to expose.
• Why: Existing callers using except DataDesignerError will miss Slurm failures, expected not-found/conflict cases will be mislabeled as internal faults, and backend implementations become coupled to service operation enums and redaction policy. A backend that raises a matching SlurmServiceError can also pass sensitive detail through verbatim despite the facade being the advertised redaction boundary.
• Suggestion: Derive SlurmServiceError from DataDesignerError. Keep lower-layer exceptions package/domain-specific and map them in each facade to a code plus a service-owned, fixed safe message; reserve the generic helper for truly unexpected exceptions and preserve their cause only in private diagnostics/logging.

packages/data-designer-slurm/src/data_designer/slurm/services/__init__.py:8 — Implementation seams are being frozen as public API before wiring exists

• What: The public services package exports BenchmarkBackend, ImageResolver, RunPlanningBackend, and BatchScriptRenderer, and every facade constructor requires one of these caller-supplied implementation objects. These are implementation-lane seams rather than the user operations promised by Epic: Slurm batch execution v1 #850; names such as Backend and Renderer are also too broad if they are intended as long-lived extension APIs. There is no default wiring or factory, and the protocol docstrings do not define exception, lifecycle, thread-safety, or compatibility obligations.
• Why: __all__ turns provisional constructor and protocol shapes into compatibility commitments before the planner, benchmark, and image implementations are integrated. It also conflicts with Define shared Slurm configuration and execution-plan contracts #873's guidance to keep lane-local helper objects private and makes the new "public services" unusable without understanding internal composition.
• Suggestion: Keep these protocols private (for example _RunPlanner and _BatchScriptRenderer) and remove them from services.__all__; expose ready-to-use services through package-owned defaults or a factory in the wiring slice. If third-party implementations are intentionally supported, give each protocol a Slurm-specific public name and document its error and stability contract explicitly before export.

packages/data-designer-slurm/src/data_designer/slurm/services/run.py:36,77 — Planning and attempt rendering are conflated in an ambiguously named result

• What: SlurmRunService.plan(config, attempt_ordinal=...) always invokes the planner and then renders an attempt-specific script. The resolved plan is attempt-independent, so the only public way to render attempt 2 currently re-resolves the authored config. SlurmRunPlanResult.plan and batch_script do not say that these are a resolved plan and rendered attempt artifact, and inheriting the unversioned ContractValue leaves it unclear whether this "stable result contract" is an in-process value or a serialized cross-process record.
• Why: Retry code can accidentally produce a new run ID or re-resolve changed profile/image facts instead of rendering the already persisted immutable plan. The generic names also become increasingly ambiguous once authored configs, resolved plans, plan artifact references, submitted attempts, and observed results coexist.
• Suggestion: Split this into plan(config) -> ResolvedSlurmRunPlan and render_attempt(resolved_plan, attempt_ordinal) -> RenderedSlurmAttempt, with fields such as resolved_plan and rendered_batch_script. If one combined M0 operation is required, name it prepare_run_attempt; use a frozen dataclass for an in-process DTO or a versioned ContractRecord if it is intended for serialization.

packages/data-designer-slurm/src/data_designer/slurm/services/run.py:56 — A Pydantic validator cannot establish the semantics of an opaque Bash program

• What: validate_plan_binding() reparses a plugin/injected renderer's string with line-prefix heuristics. The already-posted tab/multiple-space case is one symptom; more fundamentally, even an anchored whitespace regex cannot account for shell control flow or alternate declarations. I reproduced a Bash-syntax-valid script where the exact expected declaration appears inside if false; then … fi and a stale tab-separated readonly executes outside it; SlurmRunPlanResult accepts the script.
• Why: This creates a false security boundary and duplicates invariants already owned by the deterministic renderer. Future fixes can keep expanding an incomplete shell parser while still accepting scripts that fail checksum validation or execute with different bindings.
• Suggestion: Do not expose an arbitrary renderer and then infer its semantics from text. Make the reviewed package renderer own reserved bindings and return a RenderedSlurmAttempt created from structured plan-digest/ordinal inputs, with direct renderer tests for the exact output. If independent verification of arbitrary Bash is genuinely required, use a real shell parser rather than Pydantic string heuristics.

packages/data-designer-slurm/src/data_designer/slurm/services/run.py:94 — Digest and artifact-reference names hide exact-byte semantics

• What: The new correlation checks call config.compute_sha256() and compare it with generic .sha256 fields named authored_config or benchmark_config, although those fields are ArtifactReference objects rather than configs. AuthoredConfig.compute_sha256() hashes the exact pretty-printed serialize_json() bytes, while the same module's top-level compute_sha256() hashes canonical JSON bytes; I confirmed the two functions produce different digests for the same config.
• Why: A planner/benchmark implementation can reasonably choose the same-named canonical helper or persist serialize_canonical_json() and be rejected by the service. Once this is a stable public guarantee, callers also cannot tell from authored_config.sha256 whether it is a semantic digest, a canonical serialization digest, or the digest of exact persisted bytes.
• Suggestion: Use representation-specific names such as compute_persisted_json_sha256() and compute_canonical_json_sha256(), and rename reference-valued fields to authored_config_ref / benchmark_config_ref (or *_artifact). A factory such as ArtifactReference.from_serialized_config(path, config) would centralize the exact-byte rule and remove duplicated comparisons.

Suggestions — Take it or leave it

packages/data-designer-slurm/src/data_designer/slurm/services/errors.py:61 — Shared private helpers look like generic public functions

• What: invalid_request() and invoke_backend() are imported across service modules but have no leading underscore and live in a public services.errors module, while being omitted from the package export list. Their names are also broad enough to collide with future non-service helpers.
• Why: Python's name-based convention makes direct imports appear supported even though these are implementation details, and ownership becomes less obvious as the services package grows.
• Suggestion: Move them to services/_boundary.py and name them _make_invalid_request_error() and _invoke_service_backend(), or make a documented public abstraction if external callers are expected to use them.

packages/data-designer-slurm/src/data_designer/slurm/services/run.py:70 — Stable public methods are not documented as contracts

• What: The exported services, result, protocols, constructors, and methods have only summary docstrings. They do not document dependency ownership, arguments/returns, which SlurmServiceError codes may be raised, exact-byte digest behavior, whether results are serialized, or what refresh_state is allowed to observe or mutate.
• Why: These omissions leave backend implementers and callers to infer the compatibility contract from tests and source. Deferring Fern workflow docs does not provide Python API documentation or prevent incompatible implementations from being written against this foundation.
• Suggestion: Add Google-style Args, Returns, and Raises sections to every exported class/method now, including the digest, redaction, freshness, and side-effect guarantees. The later Fern slice can then document workflows against an already explicit Python contract.

packages/data-designer-slurm/tests/services/test_services.py:52 — The omnibus test module obscures service-specific contracts

• What: One 455-line module covers run planning, rendering, image resolution, benchmarks, error serialization, cancellation, and fake-harness behavior. Names such as test_image_service_delegates_to_the_injected_f3_resolver also expose an internal milestone label rather than the behavior under test.
• Why: The benchmark fixture's partial digest repair was easy to read as full correlation, and future edge cases will make the module harder to navigate and review against the four production modules.
• Suggestion: Split it into test_run_service.py, test_image_service.py, test_benchmark_service.py, and test_service_errors.py; keep fixtures domain-local and rename the F3 test around verified image resolution behavior.

What Looks Good

• The service signatures use concrete DataDesignerSlurmConfig, DataDesignerSlurmBenchmarkConfig, ImageRef, and result types; this PR does not add broad Mapping[str, object] or **kwargs entry points.
• Input type checks, image reference/kind correlation, unexpected-exception redaction, and BaseException cancellation propagation are covered directly.
• The production modules are otherwise small and cohesive, imports preserve the Slurm → interface → engine → config direction, and the focused 39-test suite, full 438-test Slurm suite against a temporary package install, plus changed-file Ruff/format checks pass.

Verdict

Needs changes — centralize the full benchmark correlation contract, put error translation at the public boundary, and settle the plan/render and backend-seam API before treating these classes as stable. The broader opaque-Bash validation and exact-byte digest naming should also be resolved rather than extending heuristic checks.

---

This review was generated by an AI assistant.

[andreatnvidia]

@nabinchha Thanks for the detailed pass. I reviewed each point against the #874 slice plan and Stage 2 ownership, then pushed af7ae44c.

For this slice:

• The planner, renderer, image resolver, and benchmark backend protocols are now private and removed from the public exports. The shared boundary helpers are private as well.
• SlurmServiceError now derives from DataDesignerError.
• Run planning and attempt rendering are separate operations. plan() returns the immutable resolved plan, while render_attempt() accepts that plan directly, so retries do not re-resolve mutable inputs.
• The rendered attempt is an explicitly in-process result, and the service no longer infers Bash semantics from the renderer's text. The package-owned renderer in feat: add Slurm command client and renderer #892 owns and tests those bindings.

I kept the remaining items with their planned owners:

• Full config → ordered children → report benchmark validation belongs to Run, observe, and analyze Slurm benchmarks #877, with the public workflow integrated in Expose Slurm services, CLI, packaging, and documentation #874 slice 4. This foundation retains only the F3 links it can authoritatively validate.
• Concrete planner, image, and renderer exception mappings plus default service wiring belong to Expose Slurm services, CLI, packaging, and documentation #874 slice 2, when those implementations are integrated.
• Digest-method and artifact-field renaming would change shared F3 contracts across CFG/PLN and BEN. Current feat: compile deterministic Slurm run plans #893 already uses the compatible exact persisted-byte digest method, so any redesign should be a separate prerequisite reviewed by those owners.

I also kept the documentation and test organization changes minimal rather than expanding every docstring or splitting the test module without a contract benefit.

[nabinchha]

Thanks for the thoughtful remediation, @andreatnvidia — the latest commit materially improves the retry boundary and public naming.

Summary

This PR adds typed facades for Slurm planning/rendering, image resolution, and benchmark execution/analysis. af7ae44c correctly separates planning from attempt rendering, removes the Bash-text heuristic, privatizes the protocols/helpers, and moves SlurmServiceError under DataDesignerError; the remaining concerns are about what the package is declaring stable before the concrete lanes are wired.

Findings

Warnings — Worth addressing

packages/data-designer-slurm/src/data_designer/slurm/services/errors.py:69-77 — A backend can bypass the advertised redaction boundary

• What: _invoke_service_backend() re-raises a same-operation SlurmServiceError unchanged and reattributes a different non-INTERNAL operation while preserving its message. The test at tests/services/test_services.py:161-210 deliberately codifies that behavior. I reproduced a FakeBenchmarkBackend raising SlurmServiceError(CONFLICT, RUN_BENCHMARK, "DUMMY_SECRET_VALUE"); the public service returned that text verbatim. At the same time, the real image and renderer lanes raise their own package errors, so NOT_FOUND, CONFLICT, and UNAVAILABLE still have no facade-owned mapping in this PR.
• Why: The lower layer, rather than the public boundary, decides which arbitrary text is safe to expose. A buggy implementation can therefore leak paths, command output, credentials, or payload fragments simply by wrapping them in the public exception. It also couples private implementations to public operation enums and leaves expected image/launcher failures mislabeled as INTERNAL until later wiring.
• Suggestion: Do not trust a dependency-raised SlurmServiceError as already sanitized. Catch concrete planner/image/renderer/benchmark exceptions in their owning facade and construct a fixed service-owned message and code; log or retain the original only in private diagnostics. If those mappings intentionally belong to slice 2/4, keep the affected facades/error codes private until that wiring lands rather than publishing a partially enforceable redaction contract.

packages/data-designer-slurm/src/data_designer/slurm/services/__init__.py:17-24 — Exported services still require private, unwired implementation seams

• What: SlurmRunService, SlurmImageService, and SlurmBenchmarkService are in the public __all__, but their constructors require _RunPlanner, _BatchScriptRenderer, _ImageResolver, and _BenchmarkBackend (run.py:47, images.py:29, benchmark.py:37). There is no public factory or default wiring. SlurmRunService also requires both planner and renderer even when a caller uses only one of the newly separated operations; the tests have to pass an empty fake for the unused dependency. The neighboring concrete image API exposes VerifiedImageRegistry.resolve_for_planning(), not the protocol's .resolve(), so the planned implementation does not conform without another adapter.
• Why: Hiding the protocol names does not hide the constructor compatibility commitment—the public signatures now point at types users cannot import or implement against as supported API. The next wiring slice must either break these constructors or add redundant factories/adapters, and a retry-only caller still has to understand planning composition despite the retry-safe method split.
• Suggestion: Keep these facade classes internal until package-owned construction is available, then export one supported ready-to-use service/factory. If manual dependency injection is intentionally public, expose and document Slurm-specific protocols and align their method names with the concrete lanes. For the run path, avoid requiring an unused planner during attempt rendering (separate the focused objects or let package wiring own the combined object privately).

packages/data-designer-slurm/src/data_designer/slurm/services/benchmark.py:46-72 — Correlation validation remains partial and has no single owner

• What: run() checks only benchmark_config.sha256, while analyze() checks only benchmark_id. The checked-in config defines 3 concurrency values × 2 deployment cases, but the service fixture patches the config digest onto a two-child manifest and accepts it. The same service accepts the report's dddd… manifest digest even though that accepted manifest serializes to 8b2e284c…. The run facade has the same ownership split in run.py:57-62: validate_resolved_plan() already owns the complete authored-config-to-plan invariant, while the facade duplicates only its digest check. Image result checks similarly repeat facts constructed and verified by the package registry.
• Why: These selected checks look like an authoritative boundary but permit missing/reordered benchmark cases and stale analysis, while future validators can drift from the duplicated facade fragments. That conflicts with Run, observe, and analyze Slurm benchmarks #877's exact ordered-child and fresh-process analysis requirements and makes it unclear whether Pydantic records, domain validators, implementations, or facades own cross-record validity.
• Suggestion: Give each complete invariant one domain-owned validator and call it at one boundary. For benchmarks, add validate_benchmark_manifest(config, manifest) and validate_benchmark_report(manifest, report) with exact ordered identities and artifact digests, and use genuinely correlated six-child fixtures. For plans, rely on or invoke the complete validate_resolved_plan() path rather than repeating one field. If Run, observe, and analyze Slurm benchmarks #877 owns the benchmark validators, defer the public benchmark facade/test contract to that slice or mark it explicitly provisional instead of stable.

packages/data-designer-slurm/src/data_designer/slurm/services/run.py:36-41 — The “in-process” result is an unversioned public serialization contract

• What: RenderedSlurmAttempt is documented as in-process but inherits ContractValue, whose stated purpose is immutable values shared across Slurm boundaries, and it is publicly exported. It serializes the full resolved plan plus script (about 5 KB for the small golden plan), duplicates the plan and ordinal already passed to render_attempt(), and accepts an empty rendered_batch_script when directly constructed.
• Why: Callers can reasonably persist or reconstruct this Pydantic value even though it has no schema version or artifact semantics, freezing an accidental wire format. Conversely, if it is only a local DTO, Pydantic validation/serialization and a second public construction path add abstraction without establishing anything the package renderer does not already own.
• Suggestion: Choose one contract explicitly: return the rendered str or use a small frozen, slotted dataclass for an in-process DTO; if the value is intended to cross a process/artifact boundary, make it a versioned ContractRecord and validate/document the persisted script representation.

Suggestions — Take it or leave it

packages/data-designer-slurm/src/data_designer/slurm/services/run.py:61 — Generic digest and reference names obscure exact-byte behavior

• What: The new correlation checks call config.compute_sha256() and compare it with .sha256 on fields named authored_config / benchmark_config, although those fields are ArtifactReference objects. AuthoredConfig.compute_sha256() hashes the exact pretty-printed serialize_json() bytes, while the module-level function with the same name hashes canonical JSON bytes.
• Why: A planner can reasonably choose the same-named canonical helper and be rejected, and callers cannot tell whether these are semantic, canonical-JSON, or persisted-byte digests. The service boundary makes that ambiguity operational even though the underlying names came from the shared-contract slice.
• Suggestion: Use representation-specific names such as compute_persisted_json_sha256() and compute_canonical_json_sha256(), and rename reference-valued fields to authored_config_ref, benchmark_config_ref, and benchmark_manifest_ref. If the shared rename must be a prerequisite, at least centralize the new comparisons in an ArtifactReference.from_serialized_config(...) helper whose name documents the byte representation.

packages/data-designer-slurm/src/data_designer/slurm/services/run.py:44-73 — Public Python contracts need more than summary docstrings

• What: The exported services, constructors, result fields, error attributes, and methods do not document dependency ownership, possible SlurmServiceError codes, side effects, refresh behavior, exact-byte digest semantics, or whether returned values may be serialized. The public signatures expose private protocol annotations, making source inspection the only way to infer the intended contract.
• Why: This is a compatibility surface that later CLI and wiring slices will consume. Without Args, Returns, Raises, and Attributes sections, callers and implementers must reverse-engineer behavior from fakes and tests, and later documentation cannot distinguish an intentional guarantee from current scaffolding.
• Suggestion: Add Google-style contract documentation to every exported class/method before export, especially constructor ownership/lifecycle, error codes/redaction, refresh_state, and persisted-vs-in-process semantics.

packages/data-designer-slurm/tests/services/test_services.py:38-425 — One omnibus test module hides four independent contracts

• What: The 425-line module mixes run planning/rendering, image resolution, benchmark workflows, error serialization, cancellation, and fake-harness behavior. The shared correlated_benchmark_manifest fixture is especially easy to mistake for a fully correlated fixture even though it repairs only one digest.
• Why: Domain-specific gaps are harder to see and the test file will grow with every service operation from later Expose Slurm services, CLI, packaging, and documentation #874 slices.
• Suggestion: Split it into test_run_service.py, test_image_service.py, test_benchmark_service.py, and test_service_errors.py; keep fixtures local and name partial fixtures precisely if they remain.

What Looks Good

• The latest commit resolves the earlier conflation cleanly: retries can render an existing immutable ResolvedSlurmRunPlan, field names now distinguish resolved plans from rendered scripts, and the fragile Bash parser is gone.
• The API uses concrete Slurm config/result types rather than Mapping[str, object] or **kwargs; method names are verb-based and the production modules remain small and cohesive with correct package layering.
• The focused service suite passes (35 tests), all changed files pass Ruff and format checks, and the isolated built-wheel installation test passes. The broader source suite produced 433 passes; its sole failure was the distribution-entry-point assertion expected from an uninstalled detached worktree, which the passing wheel test covers.

Verdict

Needs changes — make redaction service-owned, avoid publishing facades whose only constructors expose private/unwired dependencies, and establish one authoritative validation path for complete request/result correlation. The persistence status of RenderedSlurmAttempt should also be settled before treating this foundation as stable.

---

This review was generated by an AI assistant.