Skip to content

fix: use current Brev create flags#1383

Open
13ernkastel wants to merge 9 commits intoNVIDIA:mainfrom
13ernkastel:codex/issue-1377-brev-deploy-flags
Open

fix: use current Brev create flags#1383
13ernkastel wants to merge 9 commits intoNVIDIA:mainfrom
13ernkastel:codex/issue-1377-brev-deploy-flags

Conversation

@13ernkastel
Copy link
Copy Markdown

@13ernkastel 13ernkastel commented Apr 2, 2026

Summary

  • replace the legacy brev create --gpu invocation with --type and --gpu-name
  • keep NEMOCLAW_GPU backwards compatible by parsing its legacy combined value
  • document the direct Brev overrides and add a TypeScript regression test for deploy

Testing

  • npx vitest run test/deploy-brev.test.ts
  • npx eslint bin/nemoclaw.js test/deploy-brev.test.ts

Fixes #1377.

Summary by CodeRabbit

  • New Features

    • New env vars to set Brev instance type and GPU name independently; they override the legacy combined setting. CLI now supplies type + GPU-name when provisioning, and GPU names are normalized (prefixes stripped, casing standardized).
  • Documentation

    • Updated GPU configuration guide with new variables, precedence rules, adjusted legacy examples, and a direct-override workflow.
  • Tests

    • Added tests for legacy compatibility, override precedence, whitespace handling, GPU-name normalization, instance-exists behavior, and fallback defaults.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 2, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Replaces deprecated Brev --gpu usage with resolved --type and --gpu-name values derived from NEMOCLAW_BREV_TYPE / NEMOCLAW_BREV_GPU_NAME or legacy NEMOCLAW_GPU; updates bin/nemoclaw.js logging and invocation, updates docs, and adds tests covering parsing and behavior.

Changes

Cohort / File(s) Summary
Core Implementation
bin/nemoclaw.js
Added helpers (normalizeBrevGpuName, firstNonEmptyValue, resolveBrevCreateConfig) to derive {type, gpuName} from NEMOCLAW_BREV_TYPE/NEMOCLAW_BREV_GPU_NAME or legacy NEMOCLAW_GPU. deploy() now uses {type, gpuName}, logs (${type}, ${gpuName}), and invokes brev create with --type and --gpu-name (removed --gpu).
Documentation
docs/deployment/deploy-to-remote-gpu.md
Clarified GPU configuration: documented legacy NEMOCLAW_GPU encoding as type+gpu-name, added direct overrides NEMOCLAW_BREV_TYPE/NEMOCLAW_BREV_GPU_NAME (they take precedence), adjusted examples and deploy invocation.
Tests
test/deploy-brev.test.ts
New Vitest suite stubbing brev/ssh/rsync/scp to assert Brev calls: legacy parsing, explicit overrides, defaults, whitespace handling, GPU-name normalization, existing-instance branch, and exit/IO capture for diagnostics.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇
I nibbled env vars by the moonlight,
split type from name with gentle bite,
swapped one flag for two — tidy and bright,
brev and I now hop and build all night. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: use current Brev create flags' directly and clearly summarizes the main change: replacing legacy --gpu flag with current Brev CLI flags.
Linked Issues check ✅ Passed The PR successfully addresses issue #1377 by replacing legacy --gpu with --type and --gpu-name flags, maintaining backward compatibility for NEMOCLAW_GPU, and adding comprehensive tests.
Out of Scope Changes check ✅ Passed All changes are within scope: helper functions for GPU configuration, updated deploy logic to use new flags, documentation updates explaining the changes, and test coverage for the new behavior.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5454419 and cf3742c.

📒 Files selected for processing (3)
  • bin/nemoclaw.js
  • docs/deployment/deploy-to-remote-gpu.md
  • test/deploy-brev.test.ts

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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

📥 Commits

Reviewing files that changed from the base of the PR and between cf3742c and 4034eab.

📒 Files selected for processing (2)
  • bin/nemoclaw.js
  • docs/deployment/deploy-to-remote-gpu.md

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 4034eab and 51b0f8f.

📒 Files selected for processing (2)
  • bin/nemoclaw.js
  • test/deploy-brev.test.ts

@wscurran wscurran added bug Something isn't working Platform: Brev Support for Brev deployment labels Apr 3, 2026
@wscurran
Copy link
Copy Markdown
Contributor

wscurran commented Apr 3, 2026

✨ 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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Platform: Brev Support for Brev deployment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[NemoClaw][Brev] nemoclaw deploy — brev create --gpu flag incompatible with current brev CLI

2 participants