feat: compile deterministic Slurm run plans

[andreatnvidia]

📋 Summary

Compile strict authored Slurm configuration and explicitly resolved inputs into an immutable, deterministic run plan before submission. This lands the M0 configuration-resolution and planning slice without taking ownership of serving, image discovery, scheduler execution, runtime rendering, or state transitions.

🔗 Related Issue

Closes #875

🔄 Changes

• Add a fluent authored-config builder plus strict JSON/YAML loading and deterministic profile selection.
• Resolve Data Designer invocation defaults while preserving every explicitly authored early-shutdown control.
• Bind sourced builder bytes, aliases, artifact paths, invocation values, images, dependencies, output, and runtime artifacts to the authored configuration.
• Hide input values in validation errors, including wrapped chained errors, so invalid secret-shaped payloads are not echoed.
• Reject unsafe multi-shard processors, profilers, shuffled or pre-partitioned seed inputs, non-parquet output, media output, and unknown, plugin, custom, or local-callable column semantics.
• Require deployments to cover declared Data Designer model aliases exactly.
• Compile deterministic deployment placement, topology, ports, client placement, mounts, shards, partition digests, and artifact references against the existing plan contracts.
• Add byte-exact single-node and multi-node goldens plus focused inline, sourced, direct-construction, secret, authorship, sharding, alias, and payload-drift tests.

🔍 Attention Areas

• The compiler consumes resolved image and dependency facts but does not discover images or build serving commands.
• Effective configuration validation applies the same identities and invariants to resolver-produced and directly constructed inputs.
• The ownership boundary was checked against the current heads of feat: resolve Slurm serving deployments #890, feat: add Slurm image registry foundation #891, and feat: add Slurm command client and renderer #892; their serving, image, command/runtime, scheduler, and state responsibilities remain separate.
• Public config and planning APIs are exported, and PyYAML is now a direct Slurm package dependency.

🧪 Testing

• make test passes (not invoked because it uses uv run; the equivalent direct four-package suite passed with 4,552 tests and one skip)
• Unit tests added/updated
• E2E tests added/updated (N/A for this pure compiler slice)
• .venv/bin/ruff check --fix . and .venv/bin/ruff format . pass
• make check-slurm passes
• Slurm dependency-audit and isolated-wheel checks pass

✅ Checklist

• Follows commit message conventions
• Commits are signed off (DCO)
• Architecture docs updated (N/A; this implements the agreed M0 contract without changing the architecture)

[greptile-apps]

Greptile Summary

The PR adds strict Slurm configuration loading and building, resolves authored inputs into immutable effective configuration, and compiles deterministic run plans.

• Adds JSON/YAML configuration loading, profile selection, normalized errors, and public authored-config APIs.
• Adds deterministic resolution and compilation for deployments, ports, shards, artifacts, dependencies, and outputs.
• Expands planning contracts, golden fixtures, and focused compiler and configuration tests.

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/config/loading.py	Adds strict JSON/YAML loading, interpolation rejection, profile-file resolution, and deterministic profile selection.
packages/data-designer-slurm/src/data_designer/slurm/config/builder.py	Adds a fluent builder for constructing and serializing strict authored Slurm configurations.
packages/data-designer-slurm/src/data_designer/slurm/planning/resolution.py	Resolves authored configuration and supplied environmental facts into validated effective planning inputs.
packages/data-designer-slurm/src/data_designer/slurm/planning/compiler.py	Compiles effective configuration into deterministic deployment, client, port, shard, and artifact records.
packages/data-designer-slurm/src/data_designer/slurm/planning/validation.py	Enforces consistency between authored inputs, dependency locks, builder payloads, and compiled plans.
packages/data-designer-slurm/src/data_designer/slurm/contracts.py	Extends immutable contracts with hidden validation inputs and persisted-JSON digest support.
uv.lock	Updates the workspace lockfile for the Slurm package’s direct PyYAML dependency.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Authored Slurm config] --> B[Strict loading and validation]
    B --> C[Resolved effective configuration]
    C --> D[Deterministic plan compiler]
    D --> E[Immutable Slurm run plan]
    E --> F[Runtime and submission boundaries]

Loading

_{Reviews (4): Last reviewed commit: "fix: unify Slurm builder artifact identi..." | Re-trigger Greptile}

[nabinchha]

Thanks for putting this together, @andreatnvidia — the separation between authored configuration, effective resolution, and deterministic compilation is a strong foundation.

Summary

This PR adds strict JSON/YAML loading, a fluent authored-config builder, profile resolution, and a pure compiler that reproduces the existing single- and multi-node plan contracts byte-for-byte. The broad implementation matches the PR’s stated intent, but a few configuration and identity invariants still need to be enforced before these plans are safe to submit.

Findings

Critical — Let's fix these before merge

packages/data-designer-slurm/src/data_designer/slurm/planning/compiler.py:278 — Partial early-shutdown settings are silently discarded

• What: _materialize_run_config() always injects disable_early_shutdown=True. If an author supplies only shutdown_error_rate or shutdown_error_window, RunConfig.normalize_shutdown_settings() then rewrites the rate to 1.0; for example, {"shutdown_error_rate": 0.25} resolves to disable_early_shutdown=True and shutdown_error_rate=1.0.
• Why: This silently ignores an explicit runtime safety threshold and contradicts the agreed contract in plans/850/data-designer-contract.md, which says partial shutdown controls must retain Data Designer's enabled-shutdown defaults.
• Suggestion: Treat the shutdown fields as one group: when a threshold/window is authored without disable_early_shutdown, omit the package's disabling defaults before validating the merged mapping. Please add rate-only and window-only regression tests in addition to the current test that explicitly sets disable_early_shutdown=False.

packages/data-designer-slurm/src/data_designer/slurm/config/loading.py:143 — Rejected credentials can be echoed in validation errors

• What: The loader interpolates the full Pydantic ValidationError into ConfigLoadError, and the same pattern appears in config/builder.py:196 and planning/compiler.py:181. Pydantic includes input_value by default; a rejected inline api_key: SEKRET appears verbatim in str(ConfigLoadError) / str(ConfigurationResolutionError), while raise ... from error can expose it again in a traceback.
• Why: These errors are likely to be printed by the CLI or collected in job logs, turning the secret-rejection path into a credential disclosure path.
• Suggestion: Hide validation inputs at the contract-model boundary or format error.errors(include_input=False, include_url=False) into a sanitized message, and ensure the chained exception cannot reintroduce raw input. Add regression tests asserting a sentinel credential is absent from both loader and resolver errors.

Warnings — Worth addressing

packages/data-designer-slurm/src/data_designer/slurm/planning/compiler.py:314 — Multi-shard validation accepts unknown/plugin column semantics

• What: The column check rejects only column_type == "image". A two-shard builder containing an unknown/plugin column such as {"column_type": "custom_plugin", ...} resolves and compiles successfully; built-in custom columns and local-callable validation semantics are likewise not conservatively classified.
• Why: The public Slurm contract explicitly requires multi-partition runs to reject plugin or otherwise unknown semantics because Data Designer exposes no merge-safety capability metadata. Allowing these plans can produce independently generated partitions whose results are not safe to merge.
• Suggestion: Maintain an explicit allowlist of known shard-safe built-in column types (and reject callable-backed forms), treating every plugin/unknown type as unshardable. Cover both inline and sourced builder payloads in the invalid-input tests.

packages/data-designer-slurm/src/data_designer/slurm/planning/compiler.py:243 — Declared model aliases do not require deployment coverage

• What: _resolve_builder() records every alias from model_configs, but compilation only requires deployment aliases to be a subset of those models. Adding an undeployed model config while keeping the existing deployments produces a valid plan with model aliases ("generator", "judge", "undeployed") and only the first two deployments.
• Why: Client preflight binds every declared ModelConfig to a logical endpoint, so the missing deployment cannot be materialized and the run fails after planning (and potentially after submission). The public contract also assigns complete declared-alias coverage to submission-time validation.
• Suggestion: Require the declared model-alias set to equal the authored deployment-alias set during resolution, with focused inline and sourced-payload tests for an extra undeployed model.

packages/data-designer-slurm/src/data_designer/slurm/planning/compiler.py:103 — Direct effective-config construction can detach a sourced payload from its digest

• What: EffectiveDataDesignerSlurmConfig.validate_resolution() checks only whether a sourced builder has a payload. Reusing a valid ResolvedBuilderInput while changing builder_payload (for example, its library_version) still constructs and compiles successfully because neither validation pass recomputes the payload digest.
• Why: The plan's builder.source.sha256 can then identify different bytes from the payload that will be staged, breaking the checksum and deterministic-artifact guarantee. This matters because the effective type is public and the PR explicitly supports invariant checking for direct construction.
• Suggestion: For sourced builders, normalize the payload exactly as _resolve_builder() does, recompute its pretty-JSON SHA-256 and aliases, and compare all three against self.builder in the effective-config validator. Add a direct-construction drift test beside the existing output-invariant cases.

What Looks Good

• Resolution and compilation stay pure and scheduler-free, with resolved image/dependency facts passed in explicitly.
• The strict loader handles duplicate keys, YAML anchors/aliases, source precedence, and literal builder interpolation thoughtfully.
• The golden tests give strong deterministic coverage across single-node and multi-node placement, ports, shards, artifacts, and output paths.

Verdict

Needs changes — please address the shutdown-default bug and validation-error secret disclosure before merge, then close the multi-shard plugin gap, require complete model/deployment coverage, and bind sourced payload bytes to their recorded digest.

---

This review was generated by an AI assistant.

[andreatnvidia]

Addressed all five cases in d3264a92 and 8026ffe4:

• Partial early-shutdown authorship now retains Data Designer defaults and every explicit control, including disable_early_shutdown: false by itself.
• Contract validation hides input values in both wrapped errors and chained causes.
• Multi-shard resolution conservatively rejects unknown, plugin, custom, media, and local-callable column semantics for inline and sourced builders.
• Deployment aliases must exactly cover declared Data Designer model aliases.
• Sourced builder payload bytes, aliases, digest, source, and package-managed artifact path are re-derived at the effective-config boundary and rechecked in plan validation.

[andreatnvidia]

Thanks, @nabinchha. I addressed all five findings in the latest head, including regression coverage for inline and sourced builders. Partial early-shutdown settings are preserved, validation errors no longer expose secret inputs, unsafe multi-shard semantics are rejected conservatively, deployment aliases must exactly cover declared models, and sourced payload bytes, digests, and aliases are now bound at the effective-config boundary. I also cross-checked the changes against the current #890-#892 heads to keep the ownership boundaries intact.

[nabinchha]

Thanks for the follow-up, @andreatnvidia. I restarted the review from the current 8026ffe4 head and read the full 13-file change, its tests, the Slurm planning contracts, and the public Data Designer invocation contract, with a separate pass for simplicity, naming, API coherence, and long-term maintenance.

Summary

The five issues from the first review have targeted fixes, and the submitted tests are green. The deeper pass found two correctness/security gaps that those fixes do not cover, plus several public-interface and ownership decisions that need to be settled while this API is new. Because these choices define serialized contracts and public compatibility commitments, the design findings below are merge-blocking rather than optional follow-up cleanup.

Verification remains clean for the submitted suite: 469 Slurm tests pass in the isolated source worktree with only the two installed-package entry-point checks excluded, and standard Ruff lint, Ruff format, and git diff --check all pass. The PR head is still 8026ffe4; GitHub reports no formal or inline reviews, all reported checks are successful or intentionally skipped, and the merge state is clean.

Findings

Critical — Let's fix these before merge

packages/data-designer-slurm/src/data_designer/slurm/config/loading.py:143 — Secret-bearing parser and validator errors are still exposed

• What: hide_input_in_errors=True removes Pydantic's input_value, but the new boundaries still interpolate and chain raw exception messages. PyYAML includes the offending source line in str(yaml.YAMLError); loading api_key: super-secret-token: x includes that complete line in both ConfigLoadError and its cause. Custom validators can also put input into their message: passing an invalid requirement containing super-secret-token through with_client() includes the sentinel in both ConfigBuilderError and its chained ValidationError because builder.py:196 forwards str(error).
• Why: This leaves the credential-disclosure path from the first review open for malformed YAML and for validators whose messages contain user values. Both errors are likely to be printed by the CLI or captured in submission logs, and the current regression tests cover only Pydantic's automatically rendered input_value case.
• Suggestion: Centralize safe boundary error formatting instead of forwarding arbitrary str(error). Do not include raw config values in validator messages, render parse failures from sanitized type/location metadata rather than PyYAML's source snippet, and do not retain an unsafe chained cause. Add regression tests for a malformed YAML line and an invalid custom-validator value containing a sentinel, checking both the wrapper and cause.

packages/data-designer-slurm/src/data_designer/slurm/planning/compiler.py:287 — Do not infer plugin semantics from arbitrary payload key names

• What: _resolve_builder() recursively scans the entire opaque builder payload through _extract_builder_aliases(), treating every model_alias, *_model_alias, and model_aliases key as a Data Designer reference. An envelope-valid, single-shard plugin column containing an optional fallback_model_alias: null is rejected with ConfigurationResolutionError: builder fallback_model_alias must be a string before plugin installation or client preflight.
• Why: The public contract explicitly limits submission-time inspection to declared aliases in the known model_configs list and defers plugin-specific referenced-alias semantics to fresh-process client validation. The heuristic can reject valid future/plugin fields, misclassify unrelated metadata, and still miss references that use a different schema. It therefore makes the planning boundary both stricter and less reliable than the declared contract.
• Suggestion: Extract only declared aliases from the known model_configs envelope during planning. Defer referenced-alias discovery to client preflight, where installed config classes can use the public get_model_aliases()/profiler APIs. Remove the recursive key-name heuristic and add a single-shard opaque-plugin regression test; multi-shard plugin rejection can remain in the conservative sharding check.

packages/data-designer-slurm/src/data_designer/slurm/planning/compiler.py:98 — Give each invariant one owner

• What: Closely related invariants are recomputed across EffectiveDataDesignerSlurmConfig.validate_resolution(), the resolved record validators in models.py, compiler prechecks, and validate_resolved_plan(). Builder identity is checked in three places; invocation/output correspondence in two or three; GPU, port, and path relationships are likewise checked before and during final model construction.
• Why: The overlap has already produced a 19-branch effective-config validator and subtle differences between layers. Every new authored or resolved field now requires maintainers to discover and update several manually synchronized checks, making omissions and contradictory error behavior increasingly likely. The plan also fragments authored state across ResolvedInvocation.authored, ResolvedClient.authored, deployments, output, and the authored-config artifact, which drives much of the cross-record comparison code.
• Suggestion: Define an ownership rule: record validators enforce only local self-consistency, resolution binds authored values to externally resolved facts once, and one cross-artifact validator checks persisted digests/identities. Let final model construction enforce record invariants instead of prechecking the same values in the compiler. Also consider retaining one typed authored-config snapshot as the source of truth instead of copying authored subtrees into several resolved records.

packages/data-designer-slurm/src/data_designer/slurm/planning/compiler.py:80 — Separate configuration resolution from plan compilation and trim the public entry points

• What: The 573-line compiler.py defines the effective-config model, authored-to-effective resolution/defaulting, sharding and dependency validation, and topology/port/shard compilation. It also publicly exposes both a stateless SlurmRunCompiler.compile() and the one-line compile_slurm_run_plan() wrapper; only the function is used by the repository.
• Why: These are two distinct phases with different inputs and error families, so the module name no longer describes half of its contents. The duplicate class/function API and publicly constructible intermediate model create compatibility surface without current consumers, while direct-construction support is a major reason for the oversized validator.
• Suggestion: Move the effective-input type and resolve_slurm_config() concerns to resolution.py, leaving compiler.py responsible for deterministic plan construction. Keep one public functional entry point per phase, remove SlurmRunCompiler unless it gains real state/extension behavior, and keep the intermediate type internal unless external construction is an intentional supported use case. If it is public, a role-oriented name such as ResolvedSlurmRunInputs is clearer than EffectiveDataDesignerSlurmConfig.

packages/data-designer-slurm/src/data_designer/slurm/planning/compiler.py:85 — Make lifecycle state and artifact references explicit in field names

• What: EffectiveDataDesignerSlurmConfig stores authored and resolved values side by side, but only some names communicate their state: authored, builder, invocation, client_image, deployment_images, dependency_lock, submission, output, and runtime_bundle. Similar ambiguity appears in resolved records: ResolvedInvocation.authored, ResolvedClient.authored, and ResolvedDeployment.authored are different config types, while ResolvedSlurmRunPlan.authored_config and ResolvedClient.dependency_lock are artifact references rather than the objects their names imply.
• Why: Readers have to repeatedly trace annotations to know whether a field is authored intent, a resolved value, or a persisted artifact. These names also become serialized contract keys, so fixing them later is more expensive than fixing them before consumers exist.
• Suggestion: Apply a consistent state/kind vocabulary. For example: authored_config, resolved_builder, resolved_invocation, resolved_client_image, resolved_deployment_images, resolved_dependency_lock, resolved_submission, and resolved_output; use authored_invocation, authored_client, and authored_deployment in nested records; and suffix references such as authored_config_artifact, dependency_lock_artifact, builder_config_artifact, and runtime_bundle_artifact. Apply the same readability sweep to locals such as selected, resolved, count, requested, and partition where more specific names remove context lookup.

packages/data-designer-slurm/src/data_designer/slurm/planning/__init__.py:8 — Scope and centralize the public Slurm names

• What: The public surface exports generic names from several sibling modules: ConfigBuilderError, ConfigLoadError, ConfigurationResolutionError, PlanCompilationError, and PlanContractError. ConfigLoadError already exists elsewhere in Data Designer, and the exported ContractValue base is similarly generic even though it is Slurm implementation scaffolding.
• Why: Module qualification is often lost at import sites, exception handlers, logs, and documentation. These names are easy to confuse with core Data Designer errors and commit unnecessary base-class APIs to compatibility.
• Suggestion: Define a small hierarchy in a clearly owned data_designer.slurm.errors module with names such as SlurmConfigBuilderError, SlurmConfigLoadError, SlurmConfigResolutionError, SlurmPlanCompilationError, and SlurmPlanContractError. Keep base implementation classes private when consumers should not subclass them, or use an explicit name such as SlurmContractValue if they are genuinely public.

packages/data-designer-slurm/src/data_designer/slurm/planning/compiler.py:39 — Shared helpers should not be private imports from models.py

• What: compiler.py imports _extract_builder_aliases and _extract_builder_identity, and validation.py imports _extract_builder_identity, even though the leading underscore declares them module-private. They also live in models.py despite performing builder-payload analysis, not defining model behavior; _extract_builder_aliases() hides additional logic in a nested recursive closure.
• Why: Three modules are coupled to implementation details whose names and location say they may change freely. This obscures ownership, discourages direct testing, and makes the model module a catch-all.
• Suggestion: Once plugin-reference inference is removed, move the remaining declared-alias/digest operation to a clearly named resolution or builder-identity module and give any cross-module helper a non-private, testable name. Keep models.py focused on record definitions and local validators.

packages/data-designer-slurm/src/data_designer/slurm/config/builder.py:72 — Prefer concrete config objects in the Python builder

• What: Builder methods accept Mapping[str, object] alternatives or untyped **values, then immediately convert them to the package's concrete Pydantic models through _validate_model().
• Why: Dict-shaped input is already owned by JSON/YAML loading. Repeating that boundary in the Python API weakens autocomplete and static checking and introduces _ConfigValue, _validate_model(), repeated normalization, and mixed method shapes. Data Designer's builder generally accepts concrete objects for config-bearing operations such as add_model_config() and add_processor().
• Suggestion: Accept InvocationConfig, ClientConfig, ServerDeploymentConfig, ArrayTasksConfig, SubmissionConfig, and OutputConfig directly, leaving mapping support to the loader. If mapping convenience is retained, make it a deliberately separate constructor and keep the validation helper on the builder as a private static helper rather than a generic module-level function.

packages/data-designer-slurm/src/data_designer/slurm/config/builder.py:118 — Use add_deployment() for the repeatable operation

• What: with_deployment() appends a complete deployment, while every other with_* method configures a singleton section.
• Why: Data Designer's builder uses add_* for repeatable config entries and with_* for singleton semantic inputs. Matching that vocabulary makes append versus replacement behavior predictable without reading the implementation.
• Suggestion: Rename this method to add_deployment() while the API is new. with_client(), with_submission(), and the other singleton methods can remain unchanged.

packages/data-designer-slurm/src/data_designer/slurm/contracts.py:168 — Name digest operations by the exact bytes they hash

• What: Module-level compute_sha256() hashes compact canonical JSON, while AuthoredConfig.compute_sha256() and the new compute_pretty_sha256() hash pretty persisted JSON. ResolvedBuilderInput then uses canonical bytes for inline input and pretty bytes for sourced input, requiring a comment to explain the difference.
• Why: pretty describes presentation, not artifact identity, and the same compute_sha256 spelling now has different byte semantics. A future artifact writer can easily choose the wrong helper and produce a reference whose digest does not match the staged bytes.
• Suggestion: Make serialization own the bytes and hash those bytes directly, or use explicit semantic names such as compute_canonical_json_sha256() and compute_serialized_json_sha256(). Avoid exporting compute_pretty_sha256() as a general public helper; a typed staged-artifact serializer for builder configs and input partitions would keep byte production and digest production together.

Warnings — Worth addressing

packages/data-designer-slurm/src/data_designer/slurm/config/loading.py:31 — Make generic type variables recognizable

• What: _Config, _ConfigValue, _Key, and _Value are TypeVars whose names read like concrete private types.
• Why: The generic relationship is not visible at use sites, especially in _load_config(config_type: type[_Config]) -> _Config; this is inconsistent with the repository's prevalent *T convention.
• Suggestion: Rename retained generics to _ConfigT, _ModelT/_ConfigValueT, _KeyT, and _ValueT. Accepting concrete builder config objects would remove _ConfigValue entirely.

packages/data-designer-slurm/src/data_designer/slurm/config/loading.py:79 — Document the non-obvious public APIs at their actual complexity

• What: resolve_profile() has eight parameters and precedence/ambient-state behavior, and resolve_slurm_config() has ten parameters spanning already-resolved facts and package-owned defaults, but both have one-line docstrings. The new builder methods likewise omit Args, Returns, and Raises details.
• Why: These exported functions encode important ownership boundaries that are not discoverable from their names or signatures alone. The style guide requires fuller Google-style documentation for non-obvious public APIs, and the Slurm README currently contains only installation instructions.
• Suggestion: After narrowing the public surface, document each remaining entry point's source precedence, caller-owned resolved inputs, defaults, return value, and normalized errors, and add one end-to-end authored → resolved → compiled example to the Slurm package documentation.

What Looks Good

• Resolution and compilation remain scheduler-free and deterministic, with external image, dependency, and runtime facts supplied explicitly.
• The early-shutdown remediation now preserves every explicitly authored control, and the focused regressions cover the previously missed partial cases.
• The conservative multi-shard allowlist and exact deployment coverage close the earlier execution-safety gaps for known built-in semantics.
• Golden-plan coverage is strong across single-node and multi-node placement, topology, ports, shards, paths, and digests.

Verdict

Needs changes — please close the remaining secret-disclosure paths and remove submission-time inference over opaque plugin payloads before merge. The public field names, exception names, module boundary, compiler entry points, validation ownership, builder types, and digest vocabulary are also merge-blocking design decisions because deferring them would lock avoidable complexity into the serialized and public APIs.

---

This review was generated by an AI assistant.

[nabinchha]

Thanks for the quick follow-up, @andreatnvidia — the boundary cleanup addresses several important parts of the previous review.

Summary

I re-reviewed the two commits after 8026ffe4, through 079da690. They fix the secret-bearing validation output, remove plugin-key inference, split resolution from compilation, scope the error names, move builder identity into an owned module, and clarify persisted-JSON digest semantics. The refactor also removes effective-config validation, however, which creates a new correctness bypass at the compiler boundary.

Findings

Critical — Let's fix these before merge

packages/data-designer-slurm/src/data_designer/slurm/planning/resolution.py:64 — Direct effective inputs now bypass resolution invariants

• What: EffectiveDataDesignerSlurmConfig.validate_resolution() and its direct-construction regression tests were deleted, while SlurmRunCompiler.compile() still accepts that public-looking model directly. I reproduced successful compilation of configurations that resolve_slurm_config() rejects: a two-shard jsonl run, output rooted under another run, 101 output partitions for 100 requested records, and a runtime bundle at /tmp/runtime.tar.gz.
• Why: Callers can now persist and execute plans that violate the package's declared resolution contract. This also contradicts the PR description's statement that effective validation applies the same invariants to resolver-produced and directly constructed inputs.
• Suggestion: Give effective-input validation one authoritative implementation and invoke it at the compiler boundary, then restore the deleted direct-construction tests. If direct construction is intentionally unsupported, make the intermediate model and compiler input private and expose a public path that cannot skip resolution.

packages/data-designer-slurm/src/data_designer/slurm/planning/__init__.py:8 — Define one canonical public API for each planning phase

• What: The follow-up removes resolve_slurm_config, the compiler, validation, and all planning errors from the package exports. Tests now import implementation submodules directly, while the PR description still says the planning APIs are public. The remaining operation shapes are also asymmetric: resolution is a function, while compilation is a stateless class with one static method.
• Why: It is unclear which symbols are compatibility commitments and whether direct construction of EffectiveDataDesignerSlurmConfig is supported. That ambiguity is what allows the validation bypass above to exist without an obvious owner.
• Suggestion: Choose and export one supported entry point per phase plus the errors callers should catch. A simple functional surface such as resolve_slurm_config(...) followed by compile_slurm_run_plan(...), with a private intermediate input type, would keep phase ownership explicit. If the class is an intentional extension point, export and document it and restore validation for its public input.

Warnings — Worth addressing

packages/data-designer-slurm/src/data_designer/slurm/_errors.py:14 — Secret-safe validation errors are no longer actionable

• What: format_validation_error() keeps only the error count and distinct Pydantic error types. For example, an invalid num_records now produces InvocationConfig failed validation (1 error: greater_than) without identifying the field. Several tests were correspondingly weakened from domain-specific messages such as exactly cover to failed validation.
• Why: Removing inputs and unsafe causes fixes the disclosure bug, but callers cannot tell which field or invariant to correct when a model has many inputs.
• Suggestion: Preserve sanitized, schema-owned field locations and safe project-authored messages while continuing to exclude input values, contexts, source snippets, and unsafe chained causes. Add assertions that errors identify the failing field while still excluding a sentinel secret.

What Looks Good

• The secret-handling fix is materially safer: parser snippets, Pydantic inputs, and unsafe causes no longer escape the Slurm boundary.
• Removing recursive alias-key inference and adding an unfamiliar single-shard plugin case restores the intended opaque extension boundary.
• Splitting resolution.py, scoping the exception names, and moving builder identity out of models.py substantially improves module ownership.
• Persisted builder, authored-config, plan, and partition digests now use explicit deterministic serialized-JSON bytes.

Verdict

Needs changes — the new effective-input bypass must be closed before merge, and the supported planning API needs to be explicit enough that callers cannot accidentally skip resolution. The previously reported lifecycle/artifact field names and Python builder-surface decisions also remain open compatibility choices.

---

This review was generated by an AI assistant.

[andreatnvidia]

Thanks, @nabinchha. I addressed the remaining correctness/security gaps and the plan-aligned ownership items in 879995c7 and 079da690.

Config, builder, resolution, and compilation boundaries now sanitize parser and Pydantic failures without forwarding raw messages or unsafe causes. The regressions cover malformed YAML, custom-validator values, and profile-selection validation containing secret sentinels.

Planning now inspects only aliases declared in data_designer.model_configs. The recursive plugin-key heuristic and serialized referenced_model_aliases field are gone; plugin reference discovery remains with client preflight. Inline and sourced opaque-plugin cases are covered, while conservative multi-shard plugin rejection remains.

I also split configuration resolution from compilation, moved builder identity out of the record module, scoped the error types, reduced the package-root API, preserved contract-error classification, and made content_sha256 consistently identify deterministic persisted builder JSON bytes. Uneven shard remainder coverage was added as well.

I reconciled the broader recommendations against the agreed migration plan and current #890-#892 heads. I retained with_deployment(), mapping-friendly builder inputs, the named internal SlurmRunCompiler, and the frozen persisted field names because those are explicit plan contracts or already consumed by the sibling lanes. The duplicate compiler wrapper and unintended public intermediate/compiler exports were removed.

I also ran an independent follow-up review against those constraints after the changes; it found no remaining blockers or regressions. Could you take another look at the latest head?

[nabinchha]

Thanks, @andreatnvidia. GitHub is still showing 079da69 as the PR head—the same head I reviewed after those two commits—and the direct effective-input invariant bypass reproduces there. Do you have additional commits locally that haven’t been pushed yet?