feat(onboard): show managed vLLM by default on DGX Spark and Station#3921
Merged
Conversation
Signed-off-by: zyang-dev <267119621+zyang-dev@users.noreply.github.com>
Contributor
|
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 (10)
✅ Files skipped from review due to trivial changes (6)
📝 WalkthroughWalkthroughPlatform-aware vLLM menu construction now gates the install-vllm entry based on detected GPU platform (DGX Spark and DGX Station by default, generic Linux NVIDIA behind experimental or env-var opt-in), in addition to the existing NEMOCLAW_PROVIDER and experimental flag controls. Documentation and examples are updated consistently across skill modules, guides, and release notes. ChangesvLLM Platform-Aware Menu Gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Review used the provided trusted deterministic PR context and supplied diff; no scripts, tests, package-manager commands, workflow dispatches, or network actions were executed.; CI, mergeability, review decision, and E2E status may change after this advisory result; this result reflects the supplied rollup for head SHA 9bca801.; No linked issue acceptance clauses were available because github.linkedIssues is empty.; CodeRabbit/review thread resolution state was not fully available; the rollup still shows CodeRabbit PENDING.; The diff was truncated to the supplied content; files were reviewed based on provided changed-file list, deterministic context, and visible diff excerpts. Full advisor summaryPR Review AdvisorBase: The code change is small and unit-tested, but the PR is blocked by hard gates: mergeStateStatus=BLOCKED, reviewDecision=CHANGES_REQUESTED, many pending status contexts, and missing required onboarding E2E evidence for head SHA 9bca801. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/vllm-menu.ts (1)
72-76: ⚡ Quick winConsider removing the type assertion for better type safety.
The type assertion
as NvidiaPlatformon line 75 bypasses the type checker whenopts.platformisundefined. While the code works correctly at runtime (becauseSet.has(undefined)returnsfalse), an explicit check would be more type-safe and clearer.♻️ Proposed fix for type safety
if ( userChoseManagedVllm || (opts.vllmProfile && - (opts.experimental || MANAGED_VLLM_DEFAULT_PLATFORMS.has(opts.platform as NvidiaPlatform))) + (opts.experimental || (opts.platform && MANAGED_VLLM_DEFAULT_PLATFORMS.has(opts.platform)))) ) {🤖 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/vllm-menu.ts` around lines 72 - 76, The condition uses a type assertion (opts.platform as NvidiaPlatform) which bypasses type checking when opts.platform can be undefined; update the condition in the if that checks userChoseManagedVllm / opts.vllmProfile / opts.experimental so it only calls MANAGED_VLLM_DEFAULT_PLATFORMS.has when opts.platform is present (e.g., change the sub-expression to (opts.experimental || (opts.platform && MANAGED_VLLM_DEFAULT_PLATFORMS.has(opts.platform)))) so type safety is preserved and undefined platforms aren’t asserted.
🤖 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.
Nitpick comments:
In `@src/lib/onboard/vllm-menu.ts`:
- Around line 72-76: The condition uses a type assertion (opts.platform as
NvidiaPlatform) which bypasses type checking when opts.platform can be
undefined; update the condition in the if that checks userChoseManagedVllm /
opts.vllmProfile / opts.experimental so it only calls
MANAGED_VLLM_DEFAULT_PLATFORMS.has when opts.platform is present (e.g., change
the sub-expression to (opts.experimental || (opts.platform &&
MANAGED_VLLM_DEFAULT_PLATFORMS.has(opts.platform)))) so type safety is preserved
and undefined platforms aren’t asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8006d60e-a33d-4c00-9e12-a9a74d0329b1
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/onboard/vllm-menu.test.tssrc/lib/onboard/vllm-menu.ts
zyang-dev
added
v0.0.47
…orm before testing the managed-vLLM default platform set. Signed-off-by: zyang-dev <267119621+zyang-dev@users.noreply.github.com>
…nstead of relying on generated dist declarations. Signed-off-by: zyang-dev <267119621+zyang-dev@users.noreply.github.com>
ericksoa
requested changes
May 20, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Thanks, the implementation path itself looks small and coherent: setupNim() now passes gpu?.platform, the menu helper allowlists only spark/station, generic Linux stays behind the experimental gate, and the focused helper tests cover those cases.\n\nI think this needs one doc follow-up before merge, though. The PR changes user-facing onboarding behavior for DGX Spark/Station, but current docs still say managed vLLM install/start requires NEMOCLAW_EXPERIMENTAL=1 or NEMOCLAW_PROVIDER=install-vllm. In particular, docs/inference/use-local-inference.mdx still says to set NEMOCLAW_EXPERIMENTAL=1 before selecting Install/Start vLLM, and docs/security/best-practices.mdx still says managed vLLM install/start is hidden by default and surfaced only via NEMOCLAW_EXPERIMENTAL=1. After this PR, that is no longer true on DGX Spark and DGX Station, so users following the docs/security guidance would get stale information.\n\nCould you update the affected docs (and any generated skill/reference text if that is the source of truth) to say DGX Spark and DGX Station surface managed vLLM by default while generic Linux remains gated?\n\nValidation I checked locally: git diff --check and vitest run src/lib/onboard/vllm-menu.test.ts both pass. Live CI is also green at head 3d5965fc1ddddd8819ecb3b820debbc75cff8275.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci/platform-matrix.json (1)
1-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd SPDX copyright and Apache-2.0 license metadata to this JSON source file.
Line 1 starts a JSON source file without SPDX metadata, which violates the repository-wide source-file requirement.
As per coding guidelines, "
**/*.{js,ts,tsx,jsx,sh,yaml,yml,json,md,mdx}: Every source file must include an SPDX license header for copyright and Apache-2.0 license".🤖 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 `@ci/platform-matrix.json` around lines 1 - 5, Add an SPDX header object at the top of this JSON file containing copyright and license metadata: insert keys like "spdxCopyright" (or "spdx": {"copyright":"...","license":"Apache-2.0"}) or equivalent JSON fields before the existing "$comment" so the file includes the required SPDX copyright and Apache-2.0 license information; ensure the new fields are valid JSON properties (e.g., "spdx": {"copyright":"2026 YourOrg or Contributors","license":"Apache-2.0"}) and placed above "version" to satisfy the repository-wide source-file requirement while preserving existing keys such as "$comment", "version", and "updated".
🧹 Nitpick comments (1)
docs/about/release-notes.md (1)
91-91: ⚡ Quick winSplit Line 91 into one sentence per source line.
Line 91 contains multiple sentences on a single line; please split them for diff readability and docs consistency.
As per coding guidelines, "
docs/**formatting rule: One sentence per line in source (makes diffs readable)."🤖 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 `@docs/about/release-notes.md` at line 91, The sentence "The onboarding provider menu offers an already-running local vLLM server directly when `localhost:8000` responds. Managed vLLM install and start options now appear by default on DGX Spark and DGX Station, while generic Linux NVIDIA GPU hosts remain behind the experimental opt-in." should be split so each sentence is on its own source line for docs/about/release-notes.md; replace the single-line paragraph with three separate lines: one for the onboarding provider/local vLLM server statement, one for the managed vLLM install/start options defaulting on DGX Spark and DGX Station, and one for the note about generic Linux NVIDIA GPU hosts being experimental opt-in.
🤖 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.
Outside diff comments:
In `@ci/platform-matrix.json`:
- Around line 1-5: Add an SPDX header object at the top of this JSON file
containing copyright and license metadata: insert keys like "spdxCopyright" (or
"spdx": {"copyright":"...","license":"Apache-2.0"}) or equivalent JSON fields
before the existing "$comment" so the file includes the required SPDX copyright
and Apache-2.0 license information; ensure the new fields are valid JSON
properties (e.g., "spdx": {"copyright":"2026 YourOrg or
Contributors","license":"Apache-2.0"}) and placed above "version" to satisfy the
repository-wide source-file requirement while preserving existing keys such as
"$comment", "version", and "updated".
---
Nitpick comments:
In `@docs/about/release-notes.md`:
- Line 91: The sentence "The onboarding provider menu offers an already-running
local vLLM server directly when `localhost:8000` responds. Managed vLLM install
and start options now appear by default on DGX Spark and DGX Station, while
generic Linux NVIDIA GPU hosts remain behind the experimental opt-in." should be
split so each sentence is on its own source line for
docs/about/release-notes.md; replace the single-line paragraph with three
separate lines: one for the onboarding provider/local vLLM server statement, one
for the managed vLLM install/start options defaulting on DGX Spark and DGX
Station, and one for the note about generic Linux NVIDIA GPU hosts being
experimental opt-in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2cf3c9ab-e142-49ef-bf75-7ba8f0006e08
📒 Files selected for processing (16)
.agents/skills/nemoclaw-user-configure-inference/SKILL.md.agents/skills/nemoclaw-user-configure-inference/references/inference-options.md.agents/skills/nemoclaw-user-configure-security/references/best-practices.md.agents/skills/nemoclaw-user-get-started/SKILL.md.agents/skills/nemoclaw-user-overview/references/release-notes.mdci/platform-matrix.jsondocs/about/release-notes.mddocs/about/release-notes.mdxdocs/get-started/quickstart.mddocs/get-started/quickstart.mdxdocs/inference/inference-options.mddocs/inference/inference-options.mdxdocs/inference/use-local-inference.mddocs/inference/use-local-inference.mdxdocs/security/best-practices.mddocs/security/best-practices.mdx
✅ Files skipped from review due to trivial changes (4)
- docs/about/release-notes.mdx
- .agents/skills/nemoclaw-user-overview/references/release-notes.md
- .agents/skills/nemoclaw-user-configure-inference/references/inference-options.md
- .agents/skills/nemoclaw-user-configure-inference/SKILL.md
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-3921.docs.buildwithfern.com/nemoclaw |
ericksoa
approved these changes
May 20, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Follow-up looks good now. The DGX Spark/Station managed-vLLM default behavior is reflected in the MDX docs, generated skill/reference text, and the platform matrix, while generic Linux remains gated behind NEMOCLAW_EXPERIMENTAL=1 or NEMOCLAW_PROVIDER=install-vllm.\n\nI also merged current main into the branch and resolved the generated-doc conflicts. Local validation on head 9bca801: git diff --cached --check before commit, python3 scripts/generate-platform-docs.py --check, generated skills compared cleanly against scripts/docs-to-skills.py output, vitest run src/lib/onboard/vllm-menu.test.ts, and npm run docs:strict.
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Show the managed vLLM install/start option by default on DGX Spark and DGX Station during onboarding. This keeps generic Linux NVIDIA GPU hosts behind the existing experimental gate while making supported DGX local-inference paths easier to discover.
Changes
vllm-menu.ts.src/lib/onboard.tsline-budget neutral.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: zyang-dev 267119621+zyang-dev@users.noreply.github.com
Summary by CodeRabbit
New Features
Improvements
Documentation