fix: use current Brev create flags (Fixes #998)#1038
fix: use current Brev create flags (Fixes #998)#1038deepujain wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Signed-off-by: Deepak Jain <[email protected]>
📝 WalkthroughWalkthroughUpdated the Brev CLI integration to replace the deprecated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/deployment/deploy-to-remote-gpu.md (1)
85-88: Example sets redundant environment variables.The example exports both
NEMOCLAW_GPUand the direct override variables (NEMOCLAW_BREV_TYPE,NEMOCLAW_BREV_GPU_NAME).
Per the code inbin/nemoclaw.js, the direct overrides take precedence, makingNEMOCLAW_GPUignored when both are set.
This may confuse readers about which variables to use.Consider showing two separate examples: one for legacy
NEMOCLAW_GPUand one for direct overrides.📝 Suggested documentation fix
For direct overrides, you can also set `NEMOCLAW_BREV_TYPE` and `NEMOCLAW_BREV_GPU_NAME` before running `nemoclaw deploy`: ```console -$ export NEMOCLAW_GPU="a2-highgpu-1g:nvidia-tesla-a100:1" -$ export NEMOCLAW_BREV_TYPE="a2-highgpu-1g" -$ export NEMOCLAW_BREV_GPU_NAME="A100" +$ export NEMOCLAW_BREV_TYPE="a2-highgpu-1g" +$ export NEMOCLAW_BREV_GPU_NAME="A100" $ nemoclaw deploy <instance-name></details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/deployment/deploy-to-remote-gpu.mdaround lines 85 - 88, The example
exports both NEMOCLAW_GPU and the override vars
NEMOCLAW_BREV_TYPE/NEMOCLAW_BREV_GPU_NAME which is confusing because
bin/nemoclaw.js treats the direct overrides as higher priority; split the
example into two clear, separate snippets: one showing the legacy usage with
only NEMOCLAW_GPU set, and a second showing the preferred direct override usage
with only NEMOCLAW_BREV_TYPE and NEMOCLAW_BREV_GPU_NAME set, and update the
surrounding text to state that direct overrides take precedence.</details> </blockquote></details> <details> <summary>test/cli.test.js (1)</summary><blockquote> `150-191`: **Test correctly validates the core fix, but coverage is incomplete.** The test verifies that `NEMOCLAW_GPU` is correctly parsed into `--type` and `--gpu-name` flags. However, `resolveBrevCreateConfig()` has three code paths, and only the legacy `NEMOCLAW_GPU` path is tested: 1. Direct overrides via `NEMOCLAW_BREV_TYPE` / `NEMOCLAW_BREV_GPU_NAME` — **not tested** 2. Legacy `NEMOCLAW_GPU` split — **tested here** ✓ 3. Defaults when no env vars are set — **not tested** Consider adding tests for the other paths to ensure full coverage. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@test/cli.test.jsaround lines 150 - 191, The test only covers the legacy
NEMOCLAW_GPU parsing path; add two more tests that call the CLI (using
runWithEnv as in the existing test) to exercise resolveBrevCreateConfig()'s
other branches: (1) set NEMOCLAW_BREV_TYPE and NEMOCLAW_BREV_GPU_NAME
environment variables and assert the generated brev create call uses those
values (e.g., --type and --gpu-name match the provided vars), and (2) run with
neither NEMOCLAW_GPU nor the BREV-specific env vars and assert the default brev
create arguments are produced; ensure each test stubs the local brev binary the
same way the current test does and checks the marker file for the expected
command.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@docs/deployment/deploy-to-remote-gpu.md:
- Around line 85-88: The example exports both NEMOCLAW_GPU and the override vars
NEMOCLAW_BREV_TYPE/NEMOCLAW_BREV_GPU_NAME which is confusing because
bin/nemoclaw.js treats the direct overrides as higher priority; split the
example into two clear, separate snippets: one showing the legacy usage with
only NEMOCLAW_GPU set, and a second showing the preferred direct override usage
with only NEMOCLAW_BREV_TYPE and NEMOCLAW_BREV_GPU_NAME set, and update the
surrounding text to state that direct overrides take precedence.In
@test/cli.test.js:
- Around line 150-191: The test only covers the legacy NEMOCLAW_GPU parsing
path; add two more tests that call the CLI (using runWithEnv as in the existing
test) to exercise resolveBrevCreateConfig()'s other branches: (1) set
NEMOCLAW_BREV_TYPE and NEMOCLAW_BREV_GPU_NAME environment variables and assert
the generated brev create call uses those values (e.g., --type and --gpu-name
match the provided vars), and (2) run with neither NEMOCLAW_GPU nor the
BREV-specific env vars and assert the default brev create arguments are
produced; ensure each test stubs the local brev binary the same way the current
test does and checks the marker file for the expected command.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `3f6c5d68-1a70-46ad-9580-89454642d9b7` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between eb4ba8c486ad97ea3ca67bea388690b641dc134e and 6c1dcb22894e6d91456c9a320f1a13f48f529f73. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `bin/nemoclaw.js` * `docs/deployment/deploy-to-remote-gpu.md` * `test/cli.test.js` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary
nemoclaw deploynow uses the current Brev CLI create flags instead of the removed--gpuoption. The deploy flow derives--typeand--gpu-namefrom the legacyNEMOCLAW_GPUvalue so existing env-based setups still work, and it also accepts direct overrides for each flag when needed.Changes
bin/nemoclaw.js- replaced the legacybrev create <name> --gpu ...call with--typeand--gpu-name, added helpers to normalize legacy GPU strings, and added explicitNEMOCLAW_BREV_TYPE/NEMOCLAW_BREV_GPU_NAMEoverrides.test/cli.test.js- added coverage thatnemoclaw deploynow invokes Brev with--typeand--gpu-nameand no longer emits the removed--gpuflag.docs/deployment/deploy-to-remote-gpu.md- documented the new Brev override variables alongside the legacyNEMOCLAW_GPUformat.Testing
npx vitest run test/cli.test.jspasses (24 passed).npm teststill has 2 pre-existing timeout failures intest/install-preflight.test.jsandtest/uninstall.test.js, unrelated to this change.Fixes #998
Summary by CodeRabbit
New Features
NEMOCLAW_BREV_TYPEandNEMOCLAW_BREV_GPU_NAMEenvironment variables for granular GPU control.Documentation
Tests