fix(onboard): warn on arm64 NIM image compatibility (Fixes #5772)#5868
fix(onboard): warn on arm64 NIM image compatibility (Fixes #5772)#5868deepujain wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 18 minutes and 30 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds an ARM64/Linux NVIDIA NIM compatibility warning helper, calls it during onboarding NIM setup, and adds unit and integration tests covering the warning condition, formatted message, and logged onboarding output. ChangesArm64 NIM compatibility warning
Sequence Diagram(s)sequenceDiagram
participant setupNim
participant warnAboutArm64NimImageCompatibility
participant "console.log" as consoleLog
setupNim->>warnAboutArm64NimImageCompatibility: pass gpu and nim-local availability
warnAboutArm64NimImageCompatibility->>consoleLog: emit warning lines when the condition matches
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Refs NVIDIA#5772 Signed-off-by: Deepak Jain <deepujain@gmail.com>
deepujain
force-pushed
the
fix/5772-arm64-nim-warning
branch
from
25ddc8b to
f9e79e7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/onboard.ts (1)
4009-4012: 🩺 Stability & Availability | 🔵 TrivialRun the onboarding E2E cohort before merge.
This hook lands in the core sandbox creation flow, so it’s worth running the selective nightly onboarding jobs called out for
src/lib/onboard.tsto catch cross-flow regressions that the unit/integration tests here won’t cover. As per path instructions,src/lib/onboard.ts“contains core onboarding logic” and the listed E2E jobs are the recommended coverage for changes in this 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/onboard.ts` around lines 4009 - 4012, This change touches the core onboarding flow via warnAboutArm64NimImageCompatibility in onboard.ts, so validate it by running the selective onboarding E2E cohort before merging. Use the recommended nightly onboarding jobs for src/lib/onboard.ts to cover cross-flow regressions that unit/integration tests may miss, and confirm the new nim-local option handling behaves correctly in the sandbox creation path.Source: Path instructions
src/lib/onboard/nim-image-compat-warning.test.ts (1)
13-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression case for the
gpu.sparkbranch.The predicate supports both
gpu.spark === trueandgpu.platform === "spark" | "station", but this suite only locks down theplatformpath. A future refactor could break the existing GB10 boolean path without failing tests. Based on the PR objectives, onboarding should keep the current GB10 detection behavior unchanged.🤖 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/nim-image-compat-warning.test.ts` around lines 13 - 54, Add a regression test in shouldWarnAboutArm64NimImageCompatibility coverage for the gpu.spark boolean path, since the current suite only exercises gpu.platform for spark/station. Extend the existing nim-image-compat-warning.test case to assert the same warning behavior when gpu.spark is true on Linux arm64 with nimLocalAvailable enabled, keeping the existing function behavior unchanged.
🤖 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 `@test/onboard-nim-image-compat-warning.test.ts`:
- Around line 88-100: The spawned onboarding flow in the compat warning test has
no bound and can hang the Vitest worker if it becomes interactive or deadlocks.
Update the spawnSync call in this test to include a timeout so the process fails
deterministically instead of wedging CI, keeping the change localized to the
onboarding test helper invocation.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 4009-4012: This change touches the core onboarding flow via
warnAboutArm64NimImageCompatibility in onboard.ts, so validate it by running the
selective onboarding E2E cohort before merging. Use the recommended nightly
onboarding jobs for src/lib/onboard.ts to cover cross-flow regressions that
unit/integration tests may miss, and confirm the new nim-local option handling
behaves correctly in the sandbox creation path.
In `@src/lib/onboard/nim-image-compat-warning.test.ts`:
- Around line 13-54: Add a regression test in
shouldWarnAboutArm64NimImageCompatibility coverage for the gpu.spark boolean
path, since the current suite only exercises gpu.platform for spark/station.
Extend the existing nim-image-compat-warning.test case to assert the same
warning behavior when gpu.spark is true on Linux arm64 with nimLocalAvailable
enabled, keeping the existing function behavior unchanged.
🪄 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: a241359c-89d4-475f-95cd-01b72a31b42f
📒 Files selected for processing (4)
src/lib/onboard.tssrc/lib/onboard/nim-image-compat-warning.test.tssrc/lib/onboard/nim-image-compat-warning.tstest/onboard-nim-image-compat-warning.test.ts
Signed-off-by: Deepak Jain <deepujain@gmail.com>
wscurran
added
area: onboarding
|
✨ Thanks for adding the arm64 NIM image-compatibility warning for DGX Spark and Station users during onboarding. This proposes a way to surface an advisory notice when Local NIM is offered on Linux arm64 so users know some images may lack linux/arm64 manifests while the pull path remains unchanged. Related open issues: |
2 participants
Summary
Adds the missing arm64 Local NVIDIA NIM image-compatibility warning during onboarding.
GB10 detection and the NIM-local provider menu are already working on current
main, so this PR leaves that behavior alone. The new warning is advisory only: it tells Linux arm64 DGX Spark/Station users that some NIM images may not publishlinux/arm64manifests, while still allowing the existing NIM pull/start path to try the selected image.Fixes #5772
Changes
src/lib/onboard/nim-image-compat-warning.ts: adds a small helper for the Linux arm64 DGX Spark/Station warning.src/lib/onboard/provider-host-state.ts: prints the warning once whennim-localis available in the provider options.src/lib/onboard/nim-image-compat-warning.test.ts: covers the warning eligibility and wording.test/onboard-nim-image-compat-warning.test.ts: exercises the compiled onboarding flow with a fake Linux arm64 DGX Spark GPU and confirms the warning appears when Local NIM is offered.Testing
npm install --ignore-scripts- completed.npm run build:cli- passed.npx vitest run src/lib/onboard/nim-image-compat-warning.test.ts test/onboard-nim-image-compat-warning.test.ts- passed.npm run typecheck:cli- passed.npm run source-shape:check- passed outside the sandbox after the sandboxedtsxIPC pipe failed withEPERM.npm run test-size:check- passed outside the sandbox after the sandboxedtsxIPC pipe failed withEPERM.git diff --check- passed.npx @biomejs/biome format src/lib/onboard/nim-image-compat-warning.ts src/lib/onboard/nim-image-compat-warning.test.ts src/lib/onboard.ts test/onboard-nim-image-compat-warning.test.ts- passed.npx @biomejs/biome lint src/lib/onboard/nim-image-compat-warning.ts src/lib/onboard/nim-image-compat-warning.test.ts src/lib/onboard.ts test/onboard-nim-image-compat-warning.test.ts- passed.Evidence it works
The focused onboarding regression overrides
process.archandprocess.platformto simulate a Linux arm64 DGX Spark host, passes a GB10-style GPU object withnimCapable: true, enablesNEMOCLAW_EXPERIMENTAL=1, and selects the default cloud provider so no real NIM image is pulled. The test confirms onboarding still completes withnvidia-prodwhile the output contains both the Local NIM arm64 warning and thelinux/arm64manifest note.Signed-off-by: Deepak Jain deepujain@gmail.com