feat: resolve Slurm serving deployments - #890
Conversation
Greptile SummaryAdds a package-owned resolver that converts inspected images, authored vLLM configuration, and planner placement into immutable Slurm serving contracts.
|
| Filename | Overview |
|---|---|
| packages/data-designer-slurm/src/data_designer/slurm/serving/resolver.py | Resolves planner placement and authored vLLM policy into deterministic deployment-wide process and endpoint records. |
| packages/data-designer-slurm/src/data_designer/slurm/serving/deployment.py | Defines immutable resolved deployment contracts and revalidates topology, process placement, readiness, rendezvous, and network identities. |
| packages/data-designer-slurm/src/data_designer/slurm/config/vllm.py | Defines the authored vLLM lifecycle, launch timing, readiness, backpressure, argument, and environment configuration. |
| packages/data-designer-slurm/src/data_designer/slurm/config/vllm_validation.py | Centralizes validation for vLLM-owned arguments, environment bindings, readiness paths, and plaintext-secret shapes. |
| packages/data-designer-slurm/src/data_designer/slurm/planning/models.py | Extends planner-owned topology and placement contracts used to bind deployments to serving endpoint claims. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Authored vLLM configuration] --> D[Serving resolver]
B[Inspected serving image] --> D
C[Planner placement and endpoint claims] --> D
D --> E[Resolved process records]
D --> F[Rendezvous and readiness records]
D --> G[Backend endpoints]
D --> H[Logical endpoint]
E --> I[Persisted resolved deployment]
F --> I
G --> I
H --> I
Reviews (23): Last reviewed commit: "fix option argument validation gaps" | Re-trigger Greptile
| return values | ||
|
|
||
|
|
||
| def validate_vllm_environment_bindings( |
There was a problem hiding this comment.
VLLM_API_KEY seems worth treating as an owned environment name unless we add an explicit auth contract. vLLM turns it into bearer authentication for /v1, while ResolvedLogicalEndpoint has no credential or header-forwarding input, so readiness can succeed but generation requests through the proxy can return 401. Could we reject it for V1, or carry the secret reference into the logical-endpoint contract?
There was a problem hiding this comment.
Agreed and addressed in 35d44f5. VLLM_API_KEY is now rejected as runtime-owned at both the authored configuration and persisted VllmLaunchPolicy boundaries. V1 will not accept bearer authentication until credential or header forwarding is explicit in the logical-endpoint contract.
| class PortClaim(ContractValue): | ||
| name: Identifier | ||
| role: Literal["http", "rendezvous", "logical_endpoint"] | ||
| # TODO: Rename node_index to allocation_node_index after downstream Stage 2 branches converge on this contract. |
There was a problem hiding this comment.
@andreatnvidia while reviewing this, I noticed a few things to change in the agreed upon contract. To not disrupt the parallel work stream, I added TODO's to reconcile later after perhaps M0 and M1 gates close.
| return True | ||
| return flag.startswith("--") and any( | ||
| owned_flag.startswith(flag) for owned_flag in _OWNED_VLLM_FLAGS if owned_flag.startswith("--") | ||
| ) |
There was a problem hiding this comment.
extra_args still has a short-option escape hatch: -t 4 gets past this check, but argparse treats -t as the owned -tp option. That can change tensor parallelism at launch while the resolved topology still says something else. Could we run the abbreviation check against the single-dash aliases as well and cover their unambiguous prefixes in the rejection tests?
There was a problem hiding this comment.
Good catch. Addressed in 56c7b677. The owned-flag abbreviation check now applies within both the single-dash and double-dash namespaces, so -t is rejected as an abbreviation of the runtime-owned -tp option. Regression coverage exercises the tokenized ["-t", "4"] form and all currently unambiguous owned short prefixes (-a, -as, -dc, -e, -pc, and -t) at both the authored-configuration and persisted launch-policy boundaries. The full Slurm suite passes with 638 tests.
| raise ValueError("vLLM argument option terminators are not supported") | ||
| if _is_reserved_vllm_flag(candidate_flag): | ||
| raise ValueError(f"vLLM argument {candidate_flag!r} is owned by the compiler or runtime") | ||
| if candidate_flag.startswith("--") and is_secret_bearing_name(candidate_flag.removeprefix("--")): |
There was a problem hiding this comment.
There's another credential path through extra_args: only option names are checked, so an opaque value token such as --hf-overrides, {"api_key":"..."} is accepted and then copied into ResolvedVllmLaunchPolicy. Since the resolved plan is meant to contain no credentials, can we reject secret-bearing values before persistence or model them with SecretRef instead?
There was a problem hiding this comment.
Addressed in 505a7f5. The authored VllmServerConfig and persisted ResolvedVllmLaunchPolicy now share recursive secret-key validation for structured and assignment-shaped argument payloads, including standalone JSON values and --flag=value JSON. Non-null values under secret-bearing keys are rejected without echoing payload contents, while non-secret plugin-owned JSON remains accepted. Regression coverage exercises both boundaries.
| @@ -157,8 +93,6 @@ def validate_input(self) -> BuilderInput: | |||
| valid = isinstance(self.inline.get("columns"), list) | |||
| if not valid: | |||
There was a problem hiding this comment.
I think this boundary still needs a recursive secret check. BuilderInput.inline accepts an arbitrary serialized Data Designer config once the outer shape matches, and that object is copied into the resolved builder and plan. An inline payload containing something like api_key: "..." will therefore persist the credential in both authored and resolved manifests. Could we reject secret-bearing keys here, or explicitly externalize them before persistence?
There was a problem hiding this comment.
Addressed in 505a7f5. BuilderInput and ResolvedBuilderInput now both apply the same recursive persisted-JSON secret check. Non-null values under secret-bearing keys are rejected, canonical null secret fields remain accepted, known non-secret relational keys remain allowed, and validation errors do not include the rejected value. Added authored and resolved-record regression coverage.
| """Raised when planner inputs cannot produce a supported vLLM server specification.""" | ||
|
|
||
|
|
||
| @dataclass(frozen=True) |
There was a problem hiding this comment.
This context keeps the two values together, but it doesn't prove they came from the same plan. A valid deployment from plan A can be paired with plan B's same-named logical endpoint claim, and the resolver will publish plan B's port. Could the boundary take a plan plus deployment ID again, or carry a shared plan identity on both inputs?
There was a problem hiding this comment.
Addressed in 505a7f5. I removed VllmServerResolutionContext. resolve_vllm_server now takes the complete ResolvedSlurmRunPlan plus a deployment ID and derives both the deployment placement and client-owned logical endpoint from that one plan. It also checks the client host and network collision at the boundary, so independently composed plan fragments are no longer representable.
| raise ValueError("resolved server nodes must divide evenly into replica groups") | ||
| if self.gpus_per_node % self.topology.tensor_parallel: | ||
| raise ValueError("resolved server GPUs must divide evenly into tensor-parallel lanes") | ||
| expected_topology = ResolvedTopology( |
There was a problem hiding this comment.
Would it be worth centralizing this arithmetic on ResolvedTopology? The same seven fields are derived in planning/models.py, while the resolver also assumes pipeline_parallel == nodes_per_replica. A small derive(...) helper would let each boundary compare against its own inputs without requiring future topology changes in two places.
There was a problem hiding this comment.
Addressed in 505a7f5. ResolvedTopology.derive now owns the seven derived topology fields and validates the node-group and GPU-lane divisibility inputs. Both ResolvedDeployment and ResolvedVllmServerDeployment compare against that shared derivation, with focused valid and invalid derivation tests.
| ) | ||
| ) | ||
|
|
||
| try: |
There was a problem hiding this comment.
Could the try cover the whole derivation rather than only the final aggregate construction? Intermediate records can still raise raw pydantic.ValidationError; for example, a valid long deployment ID produces an over-limit derived process ID before reaching this block. Wrapping all record construction would keep callers on VllmServerResolutionError as constraints evolve.
There was a problem hiding this comment.
Addressed in 505a7f5. The public resolver now wraps the entire private derivation call, so Pydantic ValidationError from any intermediate or aggregate record is normalized to VllmServerResolutionError. The regression test uses a maximum-length valid deployment ID that makes an intermediate derived process ID invalid, confirming that the raw validation error no longer escapes.
| for value in values: | ||
| validate_plain_text(value, field_name="vLLM argument") | ||
| flag = value.partition("=")[0].replace("_", "-") | ||
| candidate_flag = flag.split(maxsplit=1)[0] |
There was a problem hiding this comment.
Small parsing edge here: candidate_flag is computed from the unstripped token. A whitespace-only value raises IndexError, while leading whitespace makes an owned option look like a non-option and bypasses the reserved and secret checks. Requiring canonical tokens with value == value.strip() before splitting would keep both cases inside the config validation boundary.
|
One coordination point with #893: the branches currently conflict in |
Add pure, package-owned vLLM compatibility and deterministic process, readiness, backend, and logical endpoint resolution. Validate topology, launch inputs, and network identities with single-node and multi-node golden coverage. Closes #866 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Revalidate persisted launch policy inputs, require canonical process ordering, and normalize resolver errors. Reject vLLM CLI aliases and transports that could override compiler-owned topology or endpoints. Refs #866 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Preserve independent single-node expert-parallel replicas while rejecting cross-node EP. Block attached compiler-owned vLLM topology aliases, require canonical runtime versions, and cover the boundaries with regression tests. Refs #866 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Reject unmodeled topology, plugin, LoRA, speculative-model, and environment overrides at authored and persisted boundaries. Validate JSON arguments, secret references, readiness paths, and compatibility version lengths. Refs #866 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Derive serving placement and client endpoint records from the same resolved plan so callers cannot mix contracts across plans. Reject vLLM API authentication until the logical endpoint models credential forwarding. Refs #866 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
- enforce plan-owned environment and endpoint behavior - limit compatibility to tested vLLM release series - split shared and vLLM configuration from run records - cover multi-group ordering and launch delays Refs #866 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Use action-named shared helpers for option extraction and duration conversion while keeping vLLM flag canonicalization backend-specific. Refs #866 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Track the tested runtime range and contract revision without repeating baseline vLLM or package-owned behaviors as capability booleans. Refs #866 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Preserve inspected runtime versions as digest-bound provenance without hard-coded support gates or redundant compatibility records. Continue to validate persisted text and compiler-owned vLLM arguments. Refs #866 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
- make vLLM resolution consume placement inputs before plan assembly - keep serving internals backend-specific and remove speculative exports - clarify validation ownership, secret redaction, and digest semantics
- Join placement and endpoint claims through one planner-owned context.\n- Normalize inconsistent resolved records to the serving error boundary.\n- Split resolver, record, and launch-policy tests by ownership. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Derive deployment and endpoint inputs from one resolved plan and centralize topology arithmetic. Reject plaintext secrets in persisted builder and vLLM argument payloads, normalize resolver errors, and require canonical argv tokens.
505a7f5 to
5ed2f07
Compare
2 participants
📋 Summary
Adds the pure, package-owned Slurm serving resolver for vLLM deployments. It combines inspected images, authored serving configuration, and planner-owned placement into deterministic process, readiness, backend, and logical-endpoint records without submitting work or managing processes.
🔗 Related Issue
Closes #866
🔄 Changes
Added
Changed
data_designer.slurm.typeswhile retaining temporary compatibility re-exports for in-flight Stage 2 branches.node_indexwith a Stage 2 migration TODO.Fixed
VllmServerResolutionError.Retry-Afterbehavior.🧪 Testing
make check-slurmpassesmake test-slurm-wheel-installpasses✅ Checklist
Review Focus
VllmServerConfiglaunch-policy boundary and its persisted-record revalidation.Description updated with AI