fix: use current Brev create flags#1383
Conversation
|
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:
📝 WalkthroughWalkthroughReplaces deprecated Brev 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/nemoclaw.js`:
- Around line 371-379: resolveBrevCreateConfig uses environment vars
NEMOCLAW_BREV_TYPE and NEMOCLAW_BREV_GPU_NAME but doesn’t treat whitespace-only
values as empty, causing invalid brev create args; update
resolveBrevCreateConfig to trim the env values first and treat "" as unset (fall
back to legacyParts or defaults) before calling normalizeBrevGpuName so
whitespace-only overrides are ignored; specifically, read and trim
process.env.NEMOCLAW_BREV_TYPE and process.env.NEMOCLAW_BREV_GPU_NAME into local
vars, use those trimmed values only if non-empty, otherwise fall back to
legacyParts[0]/[1] or "a2-highgpu-1g"/"A100", and pass the final gpuName into
normalizeBrevGpuName.
🪄 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: Pro
Run ID: 9d448ed7-3fd4-4bcd-ad49-fd62d45af3e2
📒 Files selected for processing (3)
bin/nemoclaw.jsdocs/deployment/deploy-to-remote-gpu.mdtest/deploy-brev.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/deployment/deploy-to-remote-gpu.md (1)
103-103: One sentence per line for readable diffs.This line contains two sentences. Per the docs style guide, each sentence should be on its own line to make diffs more readable.
-These take precedence over `NEMOCLAW_GPU` when both are set: +These take precedence over `NEMOCLAW_GPU` when both are set.Then remove the colon entirely or move it to introduce the code block on a separate line if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/deploy-to-remote-gpu.md` at line 103, The line "These take precedence over `NEMOCLAW_GPU` when both are set:" contains two sentences on one line and ends with a colon; split the content so each sentence is on its own line (one sentence per line for readable diffs) and remove the trailing colon or move the colon to a separate line that introduces the subsequent code block; update the sentence(s) replacing the single-line "These take precedence over `NEMOCLAW_GPU` when both are set:" with two lines (one sentence per line) and either drop the colon or place it immediately above the code block as the block introducer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/deployment/deploy-to-remote-gpu.md`:
- Line 103: The line "These take precedence over `NEMOCLAW_GPU` when both are
set:" contains two sentences on one line and ends with a colon; split the
content so each sentence is on its own line (one sentence per line for readable
diffs) and remove the trailing colon or move the colon to a separate line that
introduces the subsequent code block; update the sentence(s) replacing the
single-line "These take precedence over `NEMOCLAW_GPU` when both are set:" with
two lines (one sentence per line) and either drop the colon or place it
immediately above the code block as the block introducer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 470921bd-f5eb-4582-8c5f-08be96d0b5e7
📒 Files selected for processing (2)
bin/nemoclaw.jsdocs/deployment/deploy-to-remote-gpu.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/nemoclaw.js`:
- Around line 676-682: The current fallback chain applies normalizeBrevGpuName
only after firstNonEmptyValue, so a raw override like
NEMOCLAW_BREV_GPU_NAME="nvidia-" can become an empty string and prematurely
force the fallback "A100"; instead normalize each candidate before picking the
first non-empty one: call normalizeBrevGpuName on each source
(process.env.NEMOCLAW_BREV_GPU_NAME, legacyParts[1], and the literal) or map the
candidates through normalizeBrevGpuName, then pass the normalized candidates
into firstNonEmptyValue and finally default to "A100" if needed; update the
expression involving normalizeBrevGpuName, firstNonEmptyValue,
NEMOCLAW_BREV_GPU_NAME, and legacyParts accordingly and adjust the affected test
(test/deploy-brev.test.ts) as noted.
🪄 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: Pro
Run ID: dfc9f543-44cb-4ba8-b877-8c12adf23272
📒 Files selected for processing (2)
bin/nemoclaw.jstest/deploy-brev.test.ts
|
✨ Thanks for submitting this pull request, which proposes a way to keep NemoClaw in sync with Brev's evolving CLI interface. Possibly related open issues: |
…ty (#1470) ## Summary - unify installer and onboarding host detection around shared TypeScript preflight logic - move `deploy` behavior into TypeScript, thin the Brev compatibility wrapper, and harden Brev readiness handling - demote or remove legacy platform-specific setup paths (`setup-spark`, `brev-setup.sh`) in favor of the canonical installer + onboard flow - update docs, CLI help, and Brev E2E coverage to match the new behavior ## What Changed - added shared host assessment and remediation planning in `src/lib/preflight.ts` - wired installer and onboard flows to the same host preflight decisions - changed Podman handling from hard block to unsupported-runtime warning - migrated deploy logic into `src/lib/deploy.ts` - updated `nemoclaw deploy` to use the authenticated Brev CLI, current Brev create flags, explicit GCP provider default, stricter readiness checks, and standard installer/onboard flow - removed `scripts/setup-spark.sh` and reduced `scripts/brev-setup.sh` to a deprecated compatibility wrapper - updated README/docs/help text and hardened the Brev E2E cleanup path ## Validation - `npm run build:cli` - targeted Vitest coverage for `src/lib/preflight.test.ts`, `src/lib/deploy.test.ts`, `test/install-preflight.test.js`, `test/cli.test.js`, `test/runner.test.js` - live Brev validation with `TEST_SUITE=deploy-cli` on `cpu-e2.4vcpu-16gb` - confirmed successful end-to-end remote deploy after waiting for Brev `status=RUNNING`, `build_status=COMPLETED`, `shell_status=READY` ## Related Issues - Fixes #1377 - Addresses #1330 - Addresses #1390 - Related to #1404 ## Credit / Prior Work This branch builds on ideas and prior work from: - #1368 by @zyang-dev for simplifying Spark setup and removing the old cgroup workaround - #1395 and #1468 by @kjw3 for the thin installer/bootstrap direction and installer path reliability - #1450 by @cjagwani for switching Brev flows toward GCP for reliability - #1383 by @13ernkastel for the current Brev create flag compatibility work - #1364 by @WuKongAI-CMU for deploy sync-path fixes - #1362 and #1266 by @jyaunches for the Brev E2E/launchable infrastructure direction - issue ideas from #1377 and #1404 by @zNeill, #1330 by @Marcelo5444, and #1390 by @ericksoa <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved host diagnostics with actionable remediation guidance surfaced during installer/onboard preflight. * **Improvements** * macOS (Intel) now recommends Docker Desktop; DGX Spark guidance now uses the standard installer + `nemoclaw onboard`. * Preflight output shows detected runtime and WSL notes; installer prints remediation actions and will skip onboarding on blocking issues. * **Deprecations** * `nemoclaw deploy`, `nemoclaw setup-spark`, and the legacy bootstrap wrapper are now deprecated compatibility paths. * **Documentation** * Quickstart, troubleshooting, and command reference updated to reflect installer+onboard flow and deprecation guidance. * **Tests** * Added/updated tests covering preflight, deploy compatibility, CLI aliases, and deploy e2e scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
brev create --gpuinvocation with--typeand--gpu-nameNEMOCLAW_GPUbackwards compatible by parsing its legacy combined valueTesting
npx vitest run test/deploy-brev.test.tsnpx eslint bin/nemoclaw.js test/deploy-brev.test.tsFixes #1377.
Summary by CodeRabbit
New Features
Documentation
Tests