feat(inference): add dual DGX Spark express profile - #8129
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a managed two-DGX Spark vLLM profile for DeepSeek V4 Flash. Added catalog schemas, discovery, topology qualification, plan materialization, distributed execution, runtime receipts, recovery, cleanup, onboarding integration, validation hooks, and documentation. ChangesManaged inference catalog and distributed serving
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8129.docs.buildwithfern.com/nemoclaw |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…xpress Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…xpress Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added
feature
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
senthilr-nv
left a comment
senthilr-nv
left a comment
There was a problem hiding this comment.
Review result: no blocking code or regression defect found in the reviewed PR-owned diff. Build, catalog validation, CLI typecheck, repository checks, focused managed-cluster/runtime/uninstall tests, installer tests, the focused package contract, and strict docs validation passed.
Two non-blocking clarity follow-ups:
ci/platform-matrix.jsonstill classifies the full “Local vLLM (managed install/start)” provider surface as “Tested with limitations,” while the dedicated two-DGX-Spark guide correctly labels the new automatic profile Experimental with physical two-node E2E pending. Please align the matrix source and regenerated documentation so the provider table communicates that distinction.- RFC #7636 accepts the generic managed-serving catalog architecture but intentionally does not qualify a specific backend, model, platform, or agent. Since this is a maintainer-owned product-scope decision, please make that decision explicit in the PR record, including ownership, lifecycle/compatibility expectations, security acceptance, and the physical-validation gate.
The dedicated guide otherwise documents the elevated container privileges, partial bearer coverage, operator-enforced firewall boundary, credential visibility, host-global ownership, cleanup behavior, and pending validation accurately.
GitHub checks are authoritative and are not part of this review rationale.
senthilr-nv
left a comment
senthilr-nv
left a comment
There was a problem hiding this comment.
Review result: no blocking code or regression defect found in the reviewed PR-owned diff. Build, catalog validation, CLI typecheck, repository checks, focused managed-cluster/runtime/uninstall tests, installer tests, the focused package contract, and strict docs validation passed.
Two non-blocking clarity follow-ups:
- ci/platform-matrix.json still classifies the full Local vLLM (managed install/start) provider surface as Tested with limitations, while the dedicated two-DGX-Spark guide correctly labels the new automatic profile Experimental with physical two-node E2E pending. Please align the matrix source and regenerated documentation so the provider table communicates that distinction.
- RFC #7636 accepts the generic managed-serving catalog architecture but intentionally does not qualify a specific backend, model, platform, or agent. Since this is a maintainer-owned product-scope decision, please make that decision explicit in the PR record, including ownership, lifecycle/compatibility expectations, security acceptance, and the physical-validation gate.
The dedicated guide otherwise documents the elevated container privileges, partial bearer coverage, operator-enforced firewall boundary, credential visibility, host-global ownership, cleanup behavior, and pending validation accurately.
GitHub checks are authoritative and are not part of this review rationale.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Resolved the advisor security blocker on the current head. The managed recipe no longer enables |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai resume |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
src/lib/inference/vllm-managed-cluster-runtime-receipt.test.ts (1)
191-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the listening ports from the plan instead of hard-coding
8000and25000.
snapshothard-codes the API port and the rendezvous port. Both values come from the compiled catalog recipe throughplan.apiPortandplan.masterPort. If the recipe changes either port,classifyManagedClusterExistingStatestops observing the expected listener and these tests fail with an unclear reason instead of a port mismatch. Pass the plan intosnapshotand read the ports from it.The
rolePlan.endpoint ? 8000 : 0guard is also unreachable, becausematerializeManagedClusterVllmPlanalways setsendpointfor the head role.♻️ Proposed plan-derived ports
function snapshot( + runtimePlan: ManagedClusterVllmPlan, rolePlan: ManagedClusterVllmRolePlan, containerId: string, ): ManagedClusterNodeSnapshot { @@ listeningPorts: - rolePlan.role === "head" ? [rolePlan.endpoint ? 8000 : 0, 25000].filter(Boolean) : [25000], + rolePlan.role === "head" + ? [runtimePlan.apiPort, runtimePlan.masterPort] + : [runtimePlan.masterPort], }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/vllm-managed-cluster-runtime-receipt.test.ts` around lines 191 - 192, Update snapshot to accept the compiled plan and derive listeningPorts from plan.apiPort and plan.masterPort instead of hard-coded 8000 and 25000 values. Remove the unreachable rolePlan.endpoint guard, preserve the head/worker port structure, and update all snapshot callers to pass the plan through.src/lib/inference/serving/managed-cluster-ssh-binding.ts (1)
9-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink the retirement issue and state exit criteria for this compatibility alias layer.
This module forwards every managed vLLM SSH binding name to the legacy
vllm-station-ssh-bindingowner. The comment states the transitional intent but does not link the tracking issue or give observable exit criteria. Without those, the dual naming can persist indefinitely and new callers can keep choosing either name.Add a GitHub issue or PR link for the migration of the legacy Station path to the serving catalog, and state when this alias module gets deleted.
As per path instructions: "Retain an old path only for a demonstrated external/persisted-data contract or a bounded confidence/rollback window. Keep the replacement authoritative, freeze the old path against new callers and features, link the retirement issue or PR in GitHub, and state observable exit criteria."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/managed-cluster-ssh-binding.ts` around lines 9 - 13, Update the module comment above the managed vLLM SSH binding aliases to include a GitHub issue or PR link tracking migration of the legacy Station path to the serving catalog, explicitly identify the serving catalog as authoritative, and state observable criteria for deleting this compatibility module, including completion of migration and confirmation that no external or persisted-data consumers require the legacy name.Source: Path instructions
src/lib/inference/serving/managed-cluster-runtime-receipt.ts (1)
592-598: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate each binding rollback so one failure does not skip the rest.
The
trywraps the whole loop. IfclearManagedVllmSshBindingthrows for the newest state path, the loop stops and the older created binding trees stay on disk. A laterpersistManagedClusterVllmRuntimeReceiptcall then fails withManaged cluster SSH binding state already exists. Move thetryinside the loop so each path is cleared independently.♻️ Proposed rollback isolation
} catch (error) { - try { - for (const statePath of createdBindingStatePaths.reverse()) { + for (const statePath of createdBindingStatePaths.reverse()) { + try { clearManagedVllmSshBinding(statePath); + } catch { + // Preserve the receipt persistence error. } - } catch { - // Preserve the receipt persistence error. } throw error; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/managed-cluster-runtime-receipt.ts` around lines 592 - 598, Move the try/catch from around the entire rollback loop into the loop over createdBindingStatePaths in the receipt persistence flow. Wrap each clearManagedVllmSshBinding(statePath) call independently so one failure does not prevent older binding paths from being cleared, while preserving the existing behavior of swallowing rollback errors and retaining the persistence error.src/lib/inference/serving/managed-cluster-ssh-binding.test-support.ts (1)
5-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
retargetManagedVllmSshBindingFixturealias. No repository consumer uses it. Add the alias when a test requires it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/managed-cluster-ssh-binding.test-support.ts` around lines 5 - 8, Remove the unused retargetManagedVllmSshBindingFixture alias from the re-export block, while preserving createManagedVllmSshBindingFixture. Do not add a replacement alias unless a consuming test requires it.Source: Coding guidelines
managed-inference/schemas/recipe.schema.json (2)
119-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the
imageDigestdefinition forruntime.image.Line 121 repeats the exact regex that
#/$defs/imageDigestdeclares at line 894. Two copies of a 100-character digest pattern will drift. Reference the definition instead.♻️ Proposed refactor
"image": { - "type": "string", - "pattern": "^(?:[a-z0-9]+(?:[._-][a-z0-9]+)*(?::[0-9]+)?/)?(?:[a-z0-9]+(?:[._-][a-z0-9]+)*/)*[a-z0-9]+(?:[._-][a-z0-9]+)*`@sha256`:[0-9a-f]{64}$" + "$ref": "`#/`$defs/imageDigest" },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@managed-inference/schemas/recipe.schema.json` around lines 119 - 122, Update the runtime.image schema definition to reference the existing `#/`$defs/imageDigest definition instead of duplicating its pattern regex, preserving the same validation behavior and removing the inline pattern.
844-860: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReject contradictory tmpfs options in the schema. The materializer checks allowed values and duplicates but forwards both
rw/roorexec/noexecto Docker. Add mutual-exclusion constraints for these option pairs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@managed-inference/schemas/recipe.schema.json` around lines 844 - 860, Update the tmpfs options schema around the options array to reject configurations containing both mutually exclusive values: rw with ro, and exec with noexec. Preserve the existing enum, maxItems, and uniqueItems constraints while adding schema-level validation for each conflicting pair.src/lib/inference/serving/catalog.ts (2)
267-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the adapter validation message.
The
catchblock replaces every thrown error with a fixed string. A maintainer who breaks a recipe then sees "does not satisfy its registered adapter contract" with no cause. Include the original message.♻️ Proposed fix
- } catch { - registrationError = "does not satisfy its registered adapter contract"; + } catch (error) { + registrationError = `does not satisfy its registered adapter contract: ${ + error instanceof Error ? error.message : String(error) + }`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/catalog.ts` around lines 267 - 275, Update the catch block around registries.validateRecipe in the recipe registration validation flow to capture the thrown error and preserve its original message in registrationError, while retaining the fixed fallback only when the thrown value has no usable message. Keep the existing ServingCatalogValidationError format and successful validation behavior unchanged.
456-472: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUnregistered binding qualifications pass compilation.
Line 460 resolves
descriptorwith optional chaining. WhentopologyQualificationshas no entry forqualificationId@schemaVersion, theoutputandoutputSchemacomparisons are skipped and the binding is accepted.validatePresetRequirementsvalidates registry membership only fortopologyRequiremententries, not for bindings. The mismatch surfaces later inassertCatalogSelectioninsrc/lib/inference/serving/managed-cluster-materialize.ts, which fails at selection time instead of at compile time. Reject an unregistered binding qualification here so the catalog fails closed during compilation.♻️ Proposed fix
const key = `${expected.qualificationId}@${String(expected.schemaVersion)}`; const descriptor = registries.topologyQualifications?.get(key); + if (registries.topologyQualifications && !descriptor) { + throw new ServingCatalogValidationError( + `Preset ${preset.metadata.id} binding ${name} references unknown topology qualification ${key}.`, + ); + } if (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/catalog.ts` around lines 456 - 472, Update the binding validation loop over recipeBindings to reject any binding whose `${qualificationId}@${schemaVersion}` key is absent from registries.topologyQualifications. Make the descriptor lookup mandatory before comparing actual and expected outputs, while preserving the existing mismatch validation and ServingCatalogValidationError behavior.src/lib/inference/serving/adapter-registry.ts (1)
337-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the replacement-text bounds with the schema.
Lines 340 and 342 bound
expectedTextandreplacementTextat 65_536.managed-inference/schemas/recipe.schema.jsonbounds both at 16384 at lines 810 and 815. Schema validation runs first, so the checks here can never fire and the two files now hold different limits. Use one value, and reference the schema limit in a comment or a shared constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/adapter-registry.ts` around lines 337 - 345, Update the replacement validation around expectedText and replacementText to use the schema’s 16,384-character maximum instead of 65,536, keeping both checks aligned with the recipe schema. Reference the shared limit or document the schema-derived value with a comment so the validation rules cannot diverge.src/lib/inference/serving/generate-catalog.ts (1)
67-69: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake
catalog:checkcompare the generated serialization withdist/managed-inference/catalog.jsonwhen the artifact exists. The current check only validates source compilation, while runtime and packaging consume the ignoreddistartifact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/generate-catalog.ts` around lines 67 - 69, Update checkServingCatalog to compare the generated catalog serialization from compileServingCatalog with dist/managed-inference/catalog.json when that artifact exists, while retaining source compilation validation and handling a missing artifact without failing the check.managed-inference/schemas/preset.schema.json (1)
235-278: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConstrain
valueforbetween. The resolver acceptsbetweenonly with exactly two numeric bounds and returnsfalsefor other shapes, including reversed bounds. The schema and catalog checks still allow these invalid requirements to pass and remain silently unmatched. Add anif/thenrule for two numeric items and validate bound ordering semantically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@managed-inference/schemas/preset.schema.json` around lines 235 - 278, Constrain the value branch when operator is between so it must be an array containing exactly two numeric items. Update the catalog validation for these requirements to reject reversed bounds as well as any non-numeric or incorrectly sized value, matching resolver behavior and preventing silently unmatched entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/inference/serving/adapter-registry.ts`:
- Around line 511-518: Update getManagedInferenceServingCatalogRegistries so its
receipts and readinessContracts sets include the authoritative llama.cpp
contract references required by llama.cpp recipes, ensuring compilation reaches
validateLlamaCppPreset. If those contracts are not available, remove the
llama.cpp-only schema requirements instead of registering nonexistent
references; preserve the existing vLLM behavior.
In `@src/lib/inference/serving/catalog-loader.ts`:
- Around line 39-42: Update the catalog-loading flow around
parseCompiledServingCatalogJson to validate the parsed result against the
managed catalog schema, including required spec.requirements,
spec.plan.bindings, spec.bindings, and execution.nodeCount fields, before
assigning it to loadedCatalog. Reject invalid catalogs rather than relying on
the CompiledManagedInferenceCatalog type assertion, while preserving
immutableManagedInferenceCopy for valid results.
In `@src/lib/inference/serving/catalog.ts`:
- Around line 325-333: Populate SERVING_READINESS_REGISTRY with observation
entries for the readiness signals used by serving presets, including each
signal’s valueType and required role metadata. Ensure the declared roles match
what validateLlamaCppPreset looks up so comparison requirements resolve
successfully instead of reporting “no value type” or missing roles.
In `@src/lib/inference/vllm-managed-cluster-runtime-receipt.test.ts`:
- Around line 300-309: Add explicit definedness assertions for currentPreset and
currentRecipe immediately after their find calls in the test, before invoking
managedInferenceDigest; retain the existing digest comparisons after these
assertions so the test verifies both lookups succeeded and the expected digests
match.
---
Nitpick comments:
In `@managed-inference/schemas/preset.schema.json`:
- Around line 235-278: Constrain the value branch when operator is between so it
must be an array containing exactly two numeric items. Update the catalog
validation for these requirements to reject reversed bounds as well as any
non-numeric or incorrectly sized value, matching resolver behavior and
preventing silently unmatched entries.
In `@managed-inference/schemas/recipe.schema.json`:
- Around line 119-122: Update the runtime.image schema definition to reference
the existing `#/`$defs/imageDigest definition instead of duplicating its pattern
regex, preserving the same validation behavior and removing the inline pattern.
- Around line 844-860: Update the tmpfs options schema around the options array
to reject configurations containing both mutually exclusive values: rw with ro,
and exec with noexec. Preserve the existing enum, maxItems, and uniqueItems
constraints while adding schema-level validation for each conflicting pair.
In `@src/lib/inference/serving/adapter-registry.ts`:
- Around line 337-345: Update the replacement validation around expectedText and
replacementText to use the schema’s 16,384-character maximum instead of 65,536,
keeping both checks aligned with the recipe schema. Reference the shared limit
or document the schema-derived value with a comment so the validation rules
cannot diverge.
In `@src/lib/inference/serving/catalog.ts`:
- Around line 267-275: Update the catch block around registries.validateRecipe
in the recipe registration validation flow to capture the thrown error and
preserve its original message in registrationError, while retaining the fixed
fallback only when the thrown value has no usable message. Keep the existing
ServingCatalogValidationError format and successful validation behavior
unchanged.
- Around line 456-472: Update the binding validation loop over recipeBindings to
reject any binding whose `${qualificationId}@${schemaVersion}` key is absent
from registries.topologyQualifications. Make the descriptor lookup mandatory
before comparing actual and expected outputs, while preserving the existing
mismatch validation and ServingCatalogValidationError behavior.
In `@src/lib/inference/serving/generate-catalog.ts`:
- Around line 67-69: Update checkServingCatalog to compare the generated catalog
serialization from compileServingCatalog with
dist/managed-inference/catalog.json when that artifact exists, while retaining
source compilation validation and handling a missing artifact without failing
the check.
In `@src/lib/inference/serving/managed-cluster-runtime-receipt.ts`:
- Around line 592-598: Move the try/catch from around the entire rollback loop
into the loop over createdBindingStatePaths in the receipt persistence flow.
Wrap each clearManagedVllmSshBinding(statePath) call independently so one
failure does not prevent older binding paths from being cleared, while
preserving the existing behavior of swallowing rollback errors and retaining the
persistence error.
In `@src/lib/inference/serving/managed-cluster-ssh-binding.test-support.ts`:
- Around line 5-8: Remove the unused retargetManagedVllmSshBindingFixture alias
from the re-export block, while preserving createManagedVllmSshBindingFixture.
Do not add a replacement alias unless a consuming test requires it.
In `@src/lib/inference/serving/managed-cluster-ssh-binding.ts`:
- Around line 9-13: Update the module comment above the managed vLLM SSH binding
aliases to include a GitHub issue or PR link tracking migration of the legacy
Station path to the serving catalog, explicitly identify the serving catalog as
authoritative, and state observable criteria for deleting this compatibility
module, including completion of migration and confirmation that no external or
persisted-data consumers require the legacy name.
In `@src/lib/inference/vllm-managed-cluster-runtime-receipt.test.ts`:
- Around line 191-192: Update snapshot to accept the compiled plan and derive
listeningPorts from plan.apiPort and plan.masterPort instead of hard-coded 8000
and 25000 values. Remove the unreachable rolePlan.endpoint guard, preserve the
head/worker port structure, and update all snapshot callers to pass the plan
through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f5963ebf-19f5-4924-921d-db451b32475f
📒 Files selected for processing (50)
.pre-commit-config.yamlci/source-architecture-budget.jsondocs/index.ymldocs/inference/set-up-vllm-on-two-dgx-sparks.mdxdocs/manage-sandboxes/uninstall-nemoclaw.mdxdocs/reference/host-files-and-state.mdxmanaged-inference/recipes/vllm.deepseek-v4-flash-0731.spark-dual.v1.yamlmanaged-inference/schemas/preset.schema.jsonmanaged-inference/schemas/recipe.schema.jsonpackage.jsonscripts/install.shsrc/lib/actions/uninstall/run-plan-dual-station.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/inference/local.tssrc/lib/inference/serving/adapter-registry.test.tssrc/lib/inference/serving/adapter-registry.tssrc/lib/inference/serving/catalog-loader.tssrc/lib/inference/serving/catalog.tssrc/lib/inference/serving/generate-catalog.tssrc/lib/inference/serving/managed-cluster-discovery-production.tssrc/lib/inference/serving/managed-cluster-discovery.test.tssrc/lib/inference/serving/managed-cluster-discovery.tssrc/lib/inference/serving/managed-cluster-executor.test.tssrc/lib/inference/serving/managed-cluster-executor.tssrc/lib/inference/serving/managed-cluster-fixture.test-support.tssrc/lib/inference/serving/managed-cluster-identifiers.tssrc/lib/inference/serving/managed-cluster-installer.test.tssrc/lib/inference/serving/managed-cluster-installer.tssrc/lib/inference/serving/managed-cluster-lifecycle.test.tssrc/lib/inference/serving/managed-cluster-lifecycle.tssrc/lib/inference/serving/managed-cluster-materialize.test.tssrc/lib/inference/serving/managed-cluster-materialize.tssrc/lib/inference/serving/managed-cluster-preparation.test.tssrc/lib/inference/serving/managed-cluster-preparation.tssrc/lib/inference/serving/managed-cluster-runtime-receipt-path.tssrc/lib/inference/serving/managed-cluster-runtime-receipt.tssrc/lib/inference/serving/managed-cluster-ssh-binding.test-support.tssrc/lib/inference/serving/managed-cluster-ssh-binding.tssrc/lib/inference/serving/managed-cluster-topology.test.tssrc/lib/inference/serving/managed-cluster-topology.tssrc/lib/inference/serving/managed-runtime-receipts.tssrc/lib/inference/serving/resolver.test.tssrc/lib/inference/serving/resolver.tssrc/lib/inference/serving/types.tssrc/lib/inference/vllm-api-key.tssrc/lib/inference/vllm-docker-env.tssrc/lib/inference/vllm-dual-station.test.tssrc/lib/inference/vllm-managed-cluster-runtime-receipt.test.tssrc/lib/inference/vllm.tssrc/lib/onboard.ts
💤 Files with no reviewable changes (1)
- src/lib/inference/serving/managed-runtime-receipts.ts
🚧 Files skipped from review as they are similar to previous changes (31)
- src/lib/onboard.ts
- scripts/install.sh
- ci/source-architecture-budget.json
- docs/index.yml
- src/lib/inference/serving/managed-cluster-preparation.test.ts
- src/lib/inference/serving/managed-cluster-runtime-receipt-path.ts
- src/lib/inference/serving/managed-cluster-installer.test.ts
- src/lib/inference/serving/managed-cluster-lifecycle.test.ts
- .pre-commit-config.yaml
- docs/inference/set-up-vllm-on-two-dgx-sparks.mdx
- src/lib/inference/vllm-dual-station.test.ts
- src/lib/inference/local.ts
- src/lib/inference/serving/managed-cluster-installer.ts
- src/lib/inference/serving/managed-cluster-preparation.ts
- src/lib/inference/serving/resolver.ts
- src/lib/inference/vllm.ts
- src/lib/inference/serving/managed-cluster-topology.ts
- src/lib/actions/uninstall/run-plan.ts
- src/lib/inference/serving/managed-cluster-discovery.ts
- src/lib/inference/vllm-docker-env.ts
- src/lib/inference/serving/adapter-registry.test.ts
- src/lib/inference/serving/managed-cluster-fixture.test-support.ts
- managed-inference/recipes/vllm.deepseek-v4-flash-0731.spark-dual.v1.yaml
- src/lib/inference/serving/managed-cluster-lifecycle.ts
- src/lib/inference/serving/managed-cluster-topology.test.ts
- src/lib/inference/serving/managed-cluster-discovery-production.ts
- src/lib/inference/serving/managed-cluster-discovery.test.ts
- src/lib/inference/serving/managed-cluster-materialize.ts
- src/lib/inference/serving/resolver.test.ts
- src/lib/inference/serving/managed-cluster-executor.test.ts
- src/lib/inference/serving/managed-cluster-executor.ts
✅ Action performedReviews resumed. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Product-scope blocker for head 655a464: this PR creates a new Experimental Express integration, canonical DGX Spark documentation, managed serving profile, and reusable cluster lifecycle/materializer contracts, but GitHub reports no closing or connected issue and I could not find an accepted issue/design for the dual-DGX-Spark surface. The PR body also leaves physical two-system hardware validation unchecked. Green unit/CI evidence is not sufficient to approve a new supported product surface. Please link the accepted issue or design decision that defines scope, owner, lifecycle, compatibility/security boundaries, and validation expectations, then attach exact-head physical two-system E2E evidence—or record an explicit maintainer waiver that states what may ship as Experimental and what remains gated. The documentation-writer receipt is bound to 655a464, so it remains current only while that is the head. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed commit 3191c0030. This PR creates a new automatic dual-DGX Spark Express profile, managed-cluster runtime, YAML extension contract, uninstall lifecycle, and canonical user documentation. The PR body does not link an accepted issue or design decision that defines product ownership, lifecycle, compatibility, security, and physical qualification. Calling the path Experimental does not satisfy the product-scope gate. Add the accepted product decision or stop this canonical integration work.
The PR also states that physical two-system end-to-end validation is incomplete. That evidence is required before this hardware-dependent profile can be approved. At 79 files and roughly 19,000 added lines, the change combines catalog/schema foundations, generic adapters, discovery, execution, lifecycle, receipts, uninstall, one concrete profile, and documentation. Provide an accepted sequence with independently reviewable boundaries and qualification stop conditions, or an approved reason this must land atomically.
The maintainer update from main is complete and CI is running. Current checks and the documentation-writer receipt must be regenerated for this commit after the scope and qualification gates are resolved.
7 participants
Summary
Adds an Experimental Express path that detects a qualified two-system DGX Spark cluster and selects its managed-vLLM profile from the compiled catalog. Cluster cardinality and serving configuration now live in YAML, while generic TypeScript adapters validate and execute the ranked topology without encoding a fixed cluster size. Existing runtimes and explicit legacy inference choices remain authoritative.
Changes
vllm.managed-cluster/v1materializer andvllm.managed-cluster.lifecycle/v1lifecycle with thehost-cluster.direct-cx7topology contract. A direct profile-specific implementation would make every compatible profile require TypeScript changes; adapter-registry and catalog-compiler tests protect registration and compatibility checks.cluster.nodeCountfrom the discovered readiness set, validate it against YAMLexecution.nodeCount, and materialize every topology node as an ordered rank. Topology and materializer tests cover the shipped two-node profile and a synthetic three-node profile through the same adapters, with execution eligibility determined by direct master-address reachability instead of a fixed cardinality check.nodeCount: 2in YAML for DeepSeek V4 Flash 0731 on two DGX Spark systems. A profile that uses the registered contracts can be added through preset and recipe YAML; a different execution or qualification contract still requires a registered adapter.The profile remains Experimental until physical two-system end-to-end validation is complete.
Type of Change
Quality Gates
Documentation Writer Review
docs-updatedorigin/main, the generated DGX Spark platform row remains in sync withci/platform-matrix.json, and the PR's terminology, profile-selection, YAML extensibility,--trust-remote-codeomission, SHA-256 verification before snapshot-code copy, managed-runtime ownership, legacy receipt discovery, uninstall behavior, and fail-closed catalog narrowing remain accurate;npm run catalog:check,python3 scripts/generate-platform-docs.py --check, andgit diff --checkpassed.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: exact-headCI / Pull Requestpassed after the isolated flaky shard rerun, and the required seven-scenarioE2E / PR Gatepassednpm run docsbuilds without warnings (doc changes only)Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit