feat(inference): add fixed local serving profiles - #8399
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds feature-gated DGX Spark local-model profiles for vLLM and llama.cpp. The change includes catalog planning, non-interactive onboarding, authenticated runtime installation and recovery, ownership-aware cleanup, cache handling, tests, and operator documentation. ChangesManaged local model profiles
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant SetupFlow
participant ProfilePlan
participant ProfileOnboarder
participant ManagedRuntime
participant ProviderState
Installer->>SetupFlow: select local model runtime
SetupFlow->>ProfilePlan: resolve feature-gated catalog plan
ProfilePlan-->>SetupFlow: return vLLM or llama.cpp plan
SetupFlow->>ProfileOnboarder: execute dedicated onboarding
ProfileOnboarder->>ManagedRuntime: install and verify runtime
ManagedRuntime-->>ProfileOnboarder: return endpoint and credentials
ProfileOnboarder->>ProviderState: attach selected provider and model
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8399.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 7c25b61 in the TypeScript / code-coverage/cliThe overall coverage in commit 7c25b61 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
1 warning · 0 suggestionsWarningsWarnings do not block.
|
![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}
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (15)
src/lib/actions/uninstall/run-plan-local-model-profile.test.ts (1)
130-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a failing cleanup child.
This case proves the success path only.
removeHostLocalModelRuntimesinrun-plan.tslines 1444-1448 aborts the whole uninstall when the cleanup child exits nonzero, and that guard is the reason ownership failures cannot be reported as a successful uninstall. Add a case whererunLocalModelRuntimeCleanupreturnsnotFound(), then assertexitCodeis 1, assert the error contains "Host-local model cleanup did not complete", and assert thatrunDockernever received anrmargument.🤖 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/actions/uninstall/run-plan-local-model-profile.test.ts` around lines 130 - 153, Add a failing-cleanup test alongside the existing success case for runUninstallPlan, configuring runLocalModelRuntimeCleanup to return notFound(). Assert the result has exitCode 1, its error includes “Host-local model cleanup did not complete,” and the injected runDocker mock never receives an rm argument.src/lib/inference/llama-cpp/managed-installer.ts (1)
562-562: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the discarded validation call explicit.
Line 562 calls
readPrivateRegularFileand drops the result. The call only exists to fail closed when an existingruntime.jsonis not an owner-only regular file. A future reader can remove it as dead code. Assign the result or add a short comment that states the intent.♻️ Proposed clarification
- readPrivateRegularFile(path.join(privateStateDir, MANAGED_LLAMA_CPP_RUNTIME_RECEIPT_FILE)); + // Fail closed when a prior runtime receipt exists with unsafe ownership or mode. + readPrivateRegularFile(path.join(privateStateDir, MANAGED_LLAMA_CPP_RUNTIME_RECEIPT_FILE));🤖 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/llama-cpp/managed-installer.ts` at line 562, Make the intentional discarded result of readPrivateRegularFile in the managed installer explicit by assigning it to an appropriately named unused variable or adding a concise comment explaining that the call validates the existing runtime receipt and fails closed when it is not owner-only regular. Preserve the current validation behavior.src/lib/inference/llama-cpp/managed-installer.test.ts (2)
107-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant environment unstub.
The
cliVitest project already enablesunstubEnvs, sovi.unstubAllEnvs()repeats work the project isolation performs. Keep only the temporary-directory removal, which Vitest does not manage.♻️ Proposed simplification
afterEach(() => { - vi.unstubAllEnvs(); for (const directory of temporaryDirectories.splice(0)) { fs.rmSync(directory, { force: true, recursive: true }); } });Based on learnings: Vitest test files under
srcare executed by thecliVitest project, which enablesclearMocks,restoreMocks,unstubEnvs, andunstubGlobals; suite-level teardown should only clean up resources Vitest does not manage.🤖 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/llama-cpp/managed-installer.test.ts` around lines 107 - 112, Remove the redundant vi.unstubAllEnvs() call from the afterEach teardown in managed-installer.test.ts, leaving the temporaryDirectories cleanup loop intact because Vitest does not manage those filesystem resources.Source: Learnings
114-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the foreign-ownership abort paths.
installManagedLlamaCppreturns{ ok: false }when the container name or the network name carries foreign labels (managed-installer.tslines 570-576). No test exercises those branches. That guard prevents the installer from removing or reusing a resource it does not own, so it needs a test. Add cases wheredockerCaptureImplreturns a row with a mismatched generation label or a missing owner label, and assert thatdockerForceRmImplanddockerRunImplare not called.The readiness-timeout branch (lines 662-672) is also uncovered. Consider a case where
probeImplalways returns a failure andnowadvances past the deadline.🤖 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/llama-cpp/managed-installer.test.ts` around lines 114 - 241, The managed installer tests lack coverage for foreign-owned resource aborts and readiness timeout. Add tests around installManagedLlamaCpp where dockerCaptureImpl reports a container or network with a mismatched generation label or missing owner label, asserting the result is unsuccessful and neither dockerForceRmImpl nor dockerRunImpl is called; also cover the readiness-timeout path with probeImpl always failing and now advancing beyond the deadline.src/lib/actions/uninstall/run-plan.ts (1)
1471-1475: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the reserved managed-inference-name pattern into one constant.
This regex is repeated verbatim at lines 1492-1494. The two uses are coupled: this one aborts uninstall when a managed container remains, and the other excludes the same names from generic
docker rm -f. If one copy changes, a managed inference container becomes eligible for generic force removal and bypasses the ownership-aware cleanup path.The literal also duplicates
MANAGED_LLAMA_CPP_CONTAINER_NAMEfromsrc/lib/inference/llama-cpp/managed-installer.tsline 31 and the vLLM container names. Define one exported pattern or name list and use it in both places.🤖 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/actions/uninstall/run-plan.ts` around lines 1471 - 1475, Extract the repeated managed-inference container-name regex into one exported constant or name list, including the llama-cpp and vLLM names plus the rank pattern. Update both the residual-container check and the generic docker removal exclusion in the uninstall flow to reuse this shared symbol, and align it with MANAGED_LLAMA_CPP_CONTAINER_NAME and the existing vLLM container names.src/lib/inference/local-model-profile/cleanup.test.ts (1)
211-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the cache-deletion abort guards.
removeOwnedLlamaCppCacheperforms a recursive delete. Two guards protect it and neither has a test:
- An entry whose name does not match
^sha256-[a-f0-9]{64}$, or an entry that is not a directory, must abort the whole operation before any deletion (cleanup.tslines 316-318).- A symlinked path component must abort (
realOwnerDirectory,cleanup.tsline 122).Add a case that places a stray file and a symlinked entry inside the cache root, then assert
{ ok: false }and that the valid entry still exists.🤖 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/local-model-profile/cleanup.test.ts` around lines 211 - 244, Add a cleanupLocalModelRuntimes test covering both cache-deletion guards: create a valid receipt-bound entry plus a stray file or invalidly named/non-directory entry and a symlinked path component under the cache root, then assert the result is { ok: false }, no deletion occurs, and the valid entry remains. Use the existing home/cache setup and dependency mocks from the nearby test.src/lib/inference/local-model-profile/cleanup.ts (3)
187-271: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftShare the runtime-receipt schema between the writer and the reader.
src/lib/inference/llama-cpp/managed-installer.tslines 203-216 writesruntime.jsonfrom an inline object literal. This function validates the same document with an inline 60-line predicate and about 30as Record<string, unknown>casts. No shared type or schema links the two sides, so a field rename or an added field in the installer surfaces only as an uninstall failure on a user machine.The repository already has the pattern to follow:
src/lib/inference/llama-cpp/gguf-cache-receipt.tsexports a constructor and a verifier for the cache receipt. Add an equivalent module for the host-local runtime receipt that exports the typed shape, acreate...function used by the installer, and averify...function used here. That change also removes the cast noise and makes the required-key set a single declaration.🤖 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/local-model-profile/cleanup.ts` around lines 187 - 271, Extract the host-local runtime receipt shape and validation from llamaCppRuntimeReceipt into a shared module, following the constructor/verifier pattern of gguf-cache-receipt.ts. Export the typed receipt shape plus create and verify functions; update managed-installer.ts to build receipts through the constructor and update llamaCppRuntimeReceipt to call the verifier, preserving the existing ownership, authentication, container, network, runtime, and model validation behavior while centralizing the exact required keys.
63-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDocker ownership inspection is implemented three times. The install path and the cleanup path each reimplement the same contract: capture
docker inspectwithignoreError, parse the JSON, require exactly one row, verify the resource name, verify the id against^[a-f0-9]{12,64}$, verify the owner and identity labels, and classify the result as absent, foreign, or owned. These checks decide whether NemoClaw reuses a Docker resource and whether it deletes one, so the implementations must not drift.
src/lib/inference/local-model-profile/cleanup.ts#L63-L110: promoteinspectOwnedResourceinto a shared module, keep thecontainerandnetworkhandling, and import it here.src/lib/inference/llama-cpp/managed-installer.ts#L457-L534: deleteownedNetworkandownedContainerId, and call the shared helper with the owner label plus the generation and auth-fingerprint labels.🤖 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/local-model-profile/cleanup.ts` around lines 63 - 110, Centralize the duplicated Docker ownership inspection contract. In src/lib/inference/local-model-profile/cleanup.ts lines 63-110, move inspectOwnedResource into a shared module while preserving container/network handling, then import it here. In src/lib/inference/llama-cpp/managed-installer.ts lines 457-534, remove ownedNetwork and ownedContainerId and use the shared helper with the owner, generation, and auth-fingerprint labels.
356-363: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
DUAL_STATION_VLLM_ROLE_LABELin cleanup.
src/lib/inference/vllm-station-cluster-lifecycle.tsalready exports the label used for dual-station containers. Import it incleanup.tsinstead of duplicating the string. This keeps cleanup synchronized with the producer and prevents role detection from drifting.🤖 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/local-model-profile/cleanup.ts` around lines 356 - 363, Update cleanup.ts to import and use the existing DUAL_STATION_VLLM_ROLE_LABEL from vllm-station-cluster-lifecycle.ts in the labels lookup within the cleanup logic, replacing the duplicated literal string while preserving the current head/worker role handling.src/lib/onboard/local-model-profile/plan.ts (1)
59-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the registry predicates instead of re-declaring the refs.
isHostLocalVllmRecipehardcodes"vllm.host-local/v1"and"vllm.host-local.lifecycle/v1".src/lib/inference/serving/adapter-registry.tsalready owns those refs asHOST_LOCAL_VLLM_MATERIALIZER_REFandHOST_LOCAL_VLLM_LIFECYCLE_REFand exportsisHostLocalInferenceServingRecipeat line 126.isLlamaCppRecipelikewise re-declares the backend and provider literals thatisLlamaCppServingRecipealready covers for the catalog validator.That creates a second source of truth. If a ref is versioned later, this file keeps compiling and the profile fails at runtime with "selects an incompatible serving recipe" instead of failing at the registry.
Import the registry predicates and delete the local copies.
As per coding guidelines: "Use existing repository vocabulary and one name per concept".🤖 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/onboard/local-model-profile/plan.ts` around lines 59 - 89, Replace the local isHostLocalVllmRecipe and isLlamaCppRecipe implementations with imports of the existing isHostLocalInferenceServingRecipe and isLlamaCppServingRecipe predicates from the serving adapter registry. Remove the duplicate local predicates and any now-unused imports, preserving all existing call sites and type-guard behavior.Source: Coding guidelines
src/lib/onboard/setup-nim-flow.ts (1)
536-554: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the self-assigning destructuring fallback with a conditional block.
The fallback object at Lines 545-553 assigns each variable to itself, so it exists only to make the destructuring form valid when
localModelStateis null. Line 554 then assignsundefinedtovllmModelIdentityon that same path, which is already its value. A guarded block states the intent directly and removes the no-op branch.♻️ Proposed refactor
const localModelState = localModelProfile.state; - ({ - model, - provider, - endpointUrl, - credentialEnv, - preferredInferenceApi, - nimContainer, - allowToolsIncompatible, - } = localModelState ?? { - model, - provider, - endpointUrl, - credentialEnv, - preferredInferenceApi, - nimContainer, - allowToolsIncompatible, - }); - vllmModelIdentity = localModelState?.vllmModelIdentity; + if (localModelState) { + ({ + model, + provider, + endpointUrl, + credentialEnv, + preferredInferenceApi, + nimContainer, + allowToolsIncompatible, + } = localModelState); + vllmModelIdentity = localModelState.vllmModelIdentity; + }🤖 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/onboard/setup-nim-flow.ts` around lines 536 - 554, Replace the self-referential fallback destructuring in the local model state handling with a conditional block that destructures and assigns model state fields only when localModelState is present. Preserve the existing values when it is absent, and keep vllmModelIdentity assigned from localModelState?.vllmModelIdentity.src/lib/inference/serving/adapter-registry.ts (2)
396-412: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract one bounded-safe-text helper.
Both branches repeat the same allowlist regex and the same
includes("\0")check with only the byte cap differing. The allowlist already excludes the null byte, soincludes("\0")can never decide the result. Extract a single helper to remove the duplication and the unreachable check.♻️ Proposed refactor
+const SAFE_SERVING_TEXT = /^[A-Za-z0-9_@%+=:,./-]+$/u; + +function boundedSafeText(value: string, maximumBytes: number): boolean { + return Buffer.byteLength(value, "utf8") <= maximumBytes && SAFE_SERVING_TEXT.test(value); +}if ( recipe.spec.serve.arguments.some( - ({ value }) => - typeof value === "string" && - (Buffer.byteLength(value, "utf8") > 16_384 || - value.includes("\0") || - !/^[A-Za-z0-9_@%+=:,./-]+$/u.test(value)), + ({ value }) => typeof value === "string" && !boundedSafeText(value, 16_384), ) || - Object.values(recipe.spec.runtime.environment).some( - (value) => - Buffer.byteLength(value, "utf8") > 4_096 || - value.includes("\0") || - !/^[A-Za-z0-9_@%+=:,./-]+$/u.test(value), - ) + Object.values(recipe.spec.runtime.environment).some((value) => !boundedSafeText(value, 4_096)) ) { return "host-local vLLM serving values must be bounded safe text"; }🤖 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 396 - 412, In the validation around recipe.spec.serve.arguments and recipe.spec.runtime.environment, extract a shared bounded-safe-text helper that accepts a value and byte limit, applies the existing allowlist regex and cap, and removes the redundant null-byte checks. Replace both inline predicates with calls to this helper while preserving the 16,384-byte and 4,096-byte limits.
367-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate serving argument names, not only their values.
The validator checks argument names against
HOST_LOCAL_MATERIALIZER_OWNED_ARGUMENTSand checks argument values against the safe-text allowlist. It never checks that a name has a safe flag shape. A future recipe could declare a name with whitespace, a null byte, or a leading value, and it would pass this validator and reach the container argv unchanged.Add a flag-name pattern check next to the owned-argument check.
♻️ Proposed refactor
Add a constant next to the other patterns:
const SAFE_ENVIRONMENT_NAME = /^[A-Z][A-Z0-9_]{0,127}$/u; +const SAFE_SERVING_ARGUMENT_NAME = /^--[a-z0-9]+(?:-[a-z0-9]+)*$/u;Then extend the check:
if ( recipe.spec.serve.arguments.some(({ name }) => - HOST_LOCAL_MATERIALIZER_OWNED_ARGUMENTS.has(name), + HOST_LOCAL_MATERIALIZER_OWNED_ARGUMENTS.has(name) || !SAFE_SERVING_ARGUMENT_NAME.test(name), ) ) { - return "host-local vLLM recipe overrides a materializer-owned serving argument"; + return "host-local vLLM recipe uses an unsupported or materializer-owned serving argument"; }🤖 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 367 - 381, Update the serving-argument validation near HOST_MATERIALIZER_OWNED_ARGUMENTS to validate each argument name against a safe flag-name pattern, rejecting names with whitespace, null bytes, leading values, or other invalid shapes before container argv construction. Preserve the existing materializer-owned argument rejection and value validation behavior, and define the pattern alongside the existing validation constants.src/lib/onboard/local-model-profile/onboarder.ts (2)
4-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
LocalModelVllmProfilealias and import the installer type only.Two points in this block:
- Line 12 introduces
LocalModelVllmProfileas a second name forVllmProfile.integration.tsLine 11 re-exports it, but no consumer uses the alias. It adds a second name for one concept.- Line 4 imports the value
installManagedLlamaCpponly to derivetypeof installManagedLlamaCppat Line 24. Useimport typeso this module does not pull the managed installer into its runtime import graph. The onboarder receives the installer throughdeps.♻️ Proposed changes
-import { installManagedLlamaCpp } from "../../inference/llama-cpp/managed-installer"; +import type { installManagedLlamaCpp } from "../../inference/llama-cpp/managed-installer"; import { materializeHostLocalVllmSelection } from "../../inference/serving/host-local-vllm-selection"; import type { ResolvedHostLocalInferenceSelection } from "../../inference/serving/types"; import type { VllmProfile } from "../../inference/vllm"; import { VLLM_EXTRA_ARGS_ENV } from "../../inference/vllm-models"; import type { SetupNimSelectionResult, SetupNimSelectionState } from "../setup-nim-flow"; import type { LocalModelProfilePlan } from "./plan"; -export type LocalModelVllmProfile = VllmProfile; -Then drop
LocalModelVllmProfilefrom theexport typeblock insrc/lib/onboard/local-model-profile/integration.tsLines 9-12.As per coding guidelines: "Use existing repository vocabulary and one name per concept" and "Do not add configuration, fallback, migration, compatibility, or extension layers without a current requirement; identify the current consumer and protecting test".
🤖 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/onboard/local-model-profile/onboarder.ts` around lines 4 - 12, Remove the unused LocalModelVllmProfile alias from onboarder.ts and its re-export from the integration.ts export type block, keeping VllmProfile as the sole type name. Change the installManagedLlamaCpp import to a type-only import because it is used only for typeof in the onboarder dependency typing, while preserving runtime injection through deps.Source: Coding guidelines
66-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInject the environment instead of reading
process.envdirectly.
resolveLocalModelProfilePlaninplan.tsLine 94 accepts an injectableenvparameter. This override check readsprocess.envdirectly, so a test can only exercise the rejection branch by mutating the real environment and then undoing the stub. Add an optionalenvfield toLocalModelProfileOnboarderDepsthat defaults toprocess.env, and read the three override variables from it. That matches the injection style already used in this feature area and keeps the tests deterministic.As per coding guidelines: "deterministic tests must clear mock calls, restore spies, undo environment/global stubs".
🤖 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/onboard/local-model-profile/onboarder.ts` around lines 66 - 70, Add an optional env dependency to LocalModelProfileOnboarderDeps, defaulting to process.env, and use that injected object in the override check within resolveLocalModelProfilePlan’s onboarding flow for NEMOCLAW_VLLM_MODEL, VLLM_EXTRA_ARGS_ENV, and NEMOCLAW_VLLM_PORT. Preserve the existing rejection behavior while allowing tests to supply a deterministic environment without mutating process.env.Source: Coding guidelines
🤖 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 `@managed-inference/presets/local-model-profile.vllm.spark.v1.yaml`:
- Around line 16-68: Add a host.gpu.driver_version observation requirement to
the requirements.all list in the vLLM preset, requiring version 580.65.06 or
later using the existing readiness comparison conventions. Keep the current
qualification and capability requirements unchanged.
In `@src/lib/inference/local-model-profile/cleanup-entry.ts`:
- Around line 11-14: Update the cleanup flow around cleanupLocalModelRuntimes so
the removed and preserved resource lists are printed before handling result.ok
failure. Preserve both reporting loops for successful and failed results, then
throw result.reason after reporting when cleanup fails.
In `@src/lib/inference/local-model-profile/cleanup.test.ts`:
- Line 103: Update the run stub in the cleanup test to remove the no-op argv
conditional and return 0 directly, since both branches currently produce the
same result. Keep the stub signature and surrounding test behavior unchanged.
- Around line 196-209: Update the Docker-unavailable test around
cleanupLocalModelRuntimes to provide explicit stubs for both capture and forceRm
alongside run, preventing fallback to real Docker adapters. Assert the injected
capture and forceRm dependencies are never called while preserving the existing
unavailable-Docker failure assertion.
In `@src/lib/inference/serving/catalog.ts`:
- Around line 523-527: Update the error message in the preset selection guard
within the serving catalog validation flow to state that selection must not be
automatic, reflecting that both explicit-only and disabled values are accepted.
Preserve the existing automatic-selection condition and error behavior.
In `@src/lib/onboard/local-model-profile/onboarder.ts`:
- Around line 76-87: Update the materialization flow in
runDedicatedLocalModelProfile so errors thrown by
materializeHostLocalVllmSelection are caught and reported through the existing
operator-facing error path, then return "retry-selection". Ensure the catch
covers handleLocalModelProfile rather than only resolveLocalModelProfilePlan,
preventing the materialization exception from escaping setupNim as an unhandled
rejection.
---
Nitpick comments:
In `@src/lib/actions/uninstall/run-plan-local-model-profile.test.ts`:
- Around line 130-153: Add a failing-cleanup test alongside the existing success
case for runUninstallPlan, configuring runLocalModelRuntimeCleanup to return
notFound(). Assert the result has exitCode 1, its error includes “Host-local
model cleanup did not complete,” and the injected runDocker mock never receives
an rm argument.
In `@src/lib/actions/uninstall/run-plan.ts`:
- Around line 1471-1475: Extract the repeated managed-inference container-name
regex into one exported constant or name list, including the llama-cpp and vLLM
names plus the rank pattern. Update both the residual-container check and the
generic docker removal exclusion in the uninstall flow to reuse this shared
symbol, and align it with MANAGED_LLAMA_CPP_CONTAINER_NAME and the existing vLLM
container names.
In `@src/lib/inference/llama-cpp/managed-installer.test.ts`:
- Around line 107-112: Remove the redundant vi.unstubAllEnvs() call from the
afterEach teardown in managed-installer.test.ts, leaving the
temporaryDirectories cleanup loop intact because Vitest does not manage those
filesystem resources.
- Around line 114-241: The managed installer tests lack coverage for
foreign-owned resource aborts and readiness timeout. Add tests around
installManagedLlamaCpp where dockerCaptureImpl reports a container or network
with a mismatched generation label or missing owner label, asserting the result
is unsuccessful and neither dockerForceRmImpl nor dockerRunImpl is called; also
cover the readiness-timeout path with probeImpl always failing and now advancing
beyond the deadline.
In `@src/lib/inference/llama-cpp/managed-installer.ts`:
- Line 562: Make the intentional discarded result of readPrivateRegularFile in
the managed installer explicit by assigning it to an appropriately named unused
variable or adding a concise comment explaining that the call validates the
existing runtime receipt and fails closed when it is not owner-only regular.
Preserve the current validation behavior.
In `@src/lib/inference/local-model-profile/cleanup.test.ts`:
- Around line 211-244: Add a cleanupLocalModelRuntimes test covering both
cache-deletion guards: create a valid receipt-bound entry plus a stray file or
invalidly named/non-directory entry and a symlinked path component under the
cache root, then assert the result is { ok: false }, no deletion occurs, and the
valid entry remains. Use the existing home/cache setup and dependency mocks from
the nearby test.
In `@src/lib/inference/local-model-profile/cleanup.ts`:
- Around line 187-271: Extract the host-local runtime receipt shape and
validation from llamaCppRuntimeReceipt into a shared module, following the
constructor/verifier pattern of gguf-cache-receipt.ts. Export the typed receipt
shape plus create and verify functions; update managed-installer.ts to build
receipts through the constructor and update llamaCppRuntimeReceipt to call the
verifier, preserving the existing ownership, authentication, container, network,
runtime, and model validation behavior while centralizing the exact required
keys.
- Around line 63-110: Centralize the duplicated Docker ownership inspection
contract. In src/lib/inference/local-model-profile/cleanup.ts lines 63-110, move
inspectOwnedResource into a shared module while preserving container/network
handling, then import it here. In
src/lib/inference/llama-cpp/managed-installer.ts lines 457-534, remove
ownedNetwork and ownedContainerId and use the shared helper with the owner,
generation, and auth-fingerprint labels.
- Around line 356-363: Update cleanup.ts to import and use the existing
DUAL_STATION_VLLM_ROLE_LABEL from vllm-station-cluster-lifecycle.ts in the
labels lookup within the cleanup logic, replacing the duplicated literal string
while preserving the current head/worker role handling.
In `@src/lib/inference/serving/adapter-registry.ts`:
- Around line 396-412: In the validation around recipe.spec.serve.arguments and
recipe.spec.runtime.environment, extract a shared bounded-safe-text helper that
accepts a value and byte limit, applies the existing allowlist regex and cap,
and removes the redundant null-byte checks. Replace both inline predicates with
calls to this helper while preserving the 16,384-byte and 4,096-byte limits.
- Around line 367-381: Update the serving-argument validation near
HOST_MATERIALIZER_OWNED_ARGUMENTS to validate each argument name against a safe
flag-name pattern, rejecting names with whitespace, null bytes, leading values,
or other invalid shapes before container argv construction. Preserve the
existing materializer-owned argument rejection and value validation behavior,
and define the pattern alongside the existing validation constants.
In `@src/lib/onboard/local-model-profile/onboarder.ts`:
- Around line 4-12: Remove the unused LocalModelVllmProfile alias from
onboarder.ts and its re-export from the integration.ts export type block,
keeping VllmProfile as the sole type name. Change the installManagedLlamaCpp
import to a type-only import because it is used only for typeof in the onboarder
dependency typing, while preserving runtime injection through deps.
- Around line 66-70: Add an optional env dependency to
LocalModelProfileOnboarderDeps, defaulting to process.env, and use that injected
object in the override check within resolveLocalModelProfilePlan’s onboarding
flow for NEMOCLAW_VLLM_MODEL, VLLM_EXTRA_ARGS_ENV, and NEMOCLAW_VLLM_PORT.
Preserve the existing rejection behavior while allowing tests to supply a
deterministic environment without mutating process.env.
In `@src/lib/onboard/local-model-profile/plan.ts`:
- Around line 59-89: Replace the local isHostLocalVllmRecipe and
isLlamaCppRecipe implementations with imports of the existing
isHostLocalInferenceServingRecipe and isLlamaCppServingRecipe predicates from
the serving adapter registry. Remove the duplicate local predicates and any
now-unused imports, preserving all existing call sites and type-guard behavior.
In `@src/lib/onboard/setup-nim-flow.ts`:
- Around line 536-554: Replace the self-referential fallback destructuring in
the local model state handling with a conditional block that destructures and
assigns model state fields only when localModelState is present. Preserve the
existing values when it is absent, and keep vllmModelIdentity assigned from
localModelState?.vllmModelIdentity.
🪄 Autofix
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: f6384930-6127-4ae3-87b7-2f0372bf5cf1
📒 Files selected for processing (37)
docs/inference/choose-local-inference-server.mdxdocs/reference/commands.mdxdocs/reference/host-files-and-state.mdxmanaged-inference/presets/local-model-profile.llama-cpp.spark.v1.yamlmanaged-inference/presets/local-model-profile.vllm.spark.v1.yamlmanaged-inference/recipes/vllm.qwen3-6-35b-a3b-nvfp4.spark-single.v1.yamlmanaged-inference/schemas/preset.schema.jsonscripts/install.shsrc/lib/actions/uninstall/run-plan-local-model-profile.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/adapters/docker/local-model-runtime.tssrc/lib/inference/llama-cpp/host-local-runtime.tssrc/lib/inference/llama-cpp/managed-installer.test.tssrc/lib/inference/llama-cpp/managed-installer.tssrc/lib/inference/local-model-profile/cleanup-entry.tssrc/lib/inference/local-model-profile/cleanup.test.tssrc/lib/inference/local-model-profile/cleanup.tssrc/lib/inference/local.tssrc/lib/inference/serving/adapter-registry.tssrc/lib/inference/serving/catalog.tssrc/lib/inference/serving/host-local-vllm-selection.tssrc/lib/inference/serving/types.tssrc/lib/inference/serving/vllm-host-local-lifecycle.test.tssrc/lib/inference/serving/vllm-host-local-lifecycle.tssrc/lib/inference/serving/vllm-managed-support.tssrc/lib/inference/vllm-models.tssrc/lib/inference/vllm.tssrc/lib/onboard.tssrc/lib/onboard/local-model-profile/integration.tssrc/lib/onboard/local-model-profile/onboarder.test.tssrc/lib/onboard/local-model-profile/onboarder.tssrc/lib/onboard/local-model-profile/plan.test.tssrc/lib/onboard/local-model-profile/plan.tssrc/lib/onboard/setup-nim-flow.test.tssrc/lib/onboard/setup-nim-flow.tstest/install-local-model-profile.test.tstest/onboard-selection.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/actions/uninstall/run-plan-local-model-profile.test.ts`:
- Around line 50-66: Update the runDocker mock in the local model profile
uninstall test to match complete expected Docker argument lists, including both
recognized ps inventory queries, and throw or otherwise fail for any
unrecognized ps request or non-ps command instead of returning ok(). Preserve
the existing explicit inventory responses used by the cleanup behavior under
test.
🪄 Autofix
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: 5d8ae271-bfa6-4462-8cf8-005391539699
📒 Files selected for processing (3)
src/lib/actions/uninstall/run-plan-local-model-profile.test.tssrc/lib/inference/llama-cpp/managed-installer.test.tssrc/lib/inference/local-model-profile/cleanup.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/inference/llama-cpp/managed-installer.test.ts
- src/lib/inference/local-model-profile/cleanup.test.ts
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: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Adds discoverable serving profiles and carries the exact selected catalog preset and recipe identity through onboarding review, resume, status, diagnostics, recovery, and uninstall. Existing automatic defaults remain unchanged when no profile is selected, and legacy sessions and unlabeled runtimes remain compatible. ## Related Issues Fixes #8246 Fixes #8384 Parent epic: #8379 ## Changes - Add human and JSON `profiles list` discovery with stable IDs, display names, backend, model, topology, selection mode, support state, download estimates, and compatibility reasons. - Add model-independent `onboard --profile <id-or-display-name>` selection, with terminal-safe errors for unknown, ambiguous, disabled, incompatible, or conflicting selections before effects. - Show the selected profile, recipe, support state, runtime image, and download estimates on the review screen. - Persist secret-free catalog, preset, recipe, model, runtime-image, and digest provenance in onboarding sessions and sandbox registry records. - Automatically reuse the recorded profile on resume, reject selection or catalog drift before effects, and preserve legacy resume behavior when provenance is absent. - Expose profile and recipe provenance in human status, JSON status, and debug diagnostics. - Bind host-local vLLM recovery and uninstall to an owner-only receipt containing exact container, authentication, and serving-profile identity; retain compatibility with legacy authenticated unlabeled runtimes. - Build on the fixed local vLLM and llama.cpp catalog profiles from #8399; this PR is stacked on that dependency and carries only the generic lifecycle/provenance delta. - Document discovery, review, resume, status, recovery, uninstall, and legacy behavior. Station catalog migration and physical hardware qualification remain separately tracked under the parent epic. ## Type of Change - [x] Code change with doc updates - [ ] Code change (feature, bug fix, or refactor) - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Local implementation and independent final review covered secret-free provenance, environment restoration, authenticated loopback runtime ownership, recovery drift rejection, and fail-closed uninstall boundaries. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: The isolated implementation and documentation, reviewed against exact #8399 base `7c25b6175` for profile discovery, review, immutable provenance, resume drift rejection, status/diagnostics, authenticated recovery, legacy compatibility, and ownership-aware uninstall were reviewed. `npm run docs`, focused profile tests, `git diff --check`, and privacy checks passed; no private or unannounced model details or credentials were added. - Agent: Codex Desktop <!-- docs-review-head-sha: 33eb0c0 --> <!-- docs-review-agents-blob-sha: c69aad4 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable. - Station profile/scenario: Not applicable; tracked separately under #8379. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line and every authored commit is signed and includes DCO sign-off - [x] Normal pre-commit and pre-push hooks passed - [x] Targeted behavior tests pass — 13 focused suites passed with 317 tests, including discovery, selection, review, session persistence, resume drift, registry/status, recovery receipts, and uninstall ownership - [x] Applicable broad gates passed — repository checks, CLI typecheck, catalog compile/check, documentation build, and diff checks - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [x] `npm run docs` builds without errors (Fern reported 2 existing unprinted warnings) - [x] Doc pages follow the documentation style guide - [ ] New doc pages include SPDX header and frontmatter (no new pages) --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added `profiles:list` to discover installed serving profiles, view compatibility, and output results as text or JSON. * Added managed local-model onboarding for vLLM and llama.cpp on supported DGX Spark systems. * Added profile selection by ID or name, validation, resume tracking, and configuration details. * Added secure runtime installation, recovery, and ownership-aware cleanup for local models. * Added host-local vLLM recovery and authenticated runtime management. * **Documentation** * Expanded setup, installer, local inference, host-state, onboarding, and uninstall guidance. * **Bug Fixes** * Improved validation for conflicting options, incompatible profiles, catalog changes, and unsafe cleanup scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
3 participants
Summary
Adds opt-in fixed local model profiles for DGX Spark backed by public vLLM and llama.cpp catalog recipes. The dedicated noninteractive path pins model and runtime identity, authenticates loopback-only endpoints, and records ownership for recovery and uninstall.
Changes
Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/inference/choose-local-inference-server.mdx,docs/reference/commands.mdx, anddocs/reference/host-files-and-state.mdxdocument the fixed local profile installer, host state, recovery, and ownership-aware uninstall behavior;npm run docspasses.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:npm run docsbuilds without warnings (doc changes only)Additional validation:
npm run docscompleted with 0 errors and 2 pre-existing warnings. The affected package contracts passed; the full package-contract lane remains subject to unrelated live-registryETARGETfailures and existing CLI subprocess timeouts.Signed-off-by: Aaron Erickson aerickson@nvidia.com