[DRAFT] ✨ Add Brev VM lifecycle management for model serving#9
[DRAFT] ✨ Add Brev VM lifecycle management for model serving#9taagarwa-rh wants to merge 9 commits into
Conversation
Automatically manage Brev GPU instances from the job queue service so benchmark jobs can use models served on Brev without manual setup. Pass server_url="brev" to trigger the lifecycle; any other value skips Brev entirely for backwards compatibility. Lifecycle: create instance on first job → start model container → run benchmark → swap or stop model between jobs → delete instance when queue drains. Same-model jobs are grouped in the queue and reuse the running container to avoid redundant start/stop cycles. Fault tolerance: startup orphan cleanup, retry-3 on delete, SIGTERM/SIGINT/atexit handlers as a safety net for expensive VMs. Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change installs the Brev CLI, exposes a Brev model endpoint, adds Brev instance and model lifecycle management, and integrates model-aware routing, queue ordering, startup recovery, shutdown cleanup, and post-job cleanup into the job queue service. ChangesBrev model execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JobWorker
participant BrevInstance
participant BrevCLI
participant ModelEndpoint
JobWorker->>BrevInstance: ensure running and select model
BrevInstance->>BrevCLI: create or execute model
BrevInstance->>ModelEndpoint: poll /health
JobWorker->>ModelEndpoint: run command with managed server URL
JobWorker->>BrevInstance: post-job cleanup
BrevInstance->>BrevCLI: stop model or delete instance
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/coding_agent_bench/brev.py (1)
24-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
MODEL_CONFIGSis empty, so every Brev job fails.With no entries,
start_model(line 172-178) raisesValueError("No Brev model config...")for anymodel_name, andstop_modelbecomes a no-op. Any command routed with--server-url brevwill therefore fail. Confirm this placeholder is intended to be populated before this path is exercised.🤖 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/coding_agent_bench/brev.py` around lines 24 - 29, Populate MODEL_CONFIGS with at least one valid Brev model entry, including its Docker command and container name, before enabling the Brev server path; ensure the configured model names match those accepted by start_model and stop_model.
🤖 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 `@Containerfile`:
- Around line 24-25: The Brev installation command in the Containerfile runs
after curl has been removed and therefore cannot execute. Move the Brev install
RUN command before the cleanup that removes curl, or defer removing curl until
after the installation step.
In `@src/coding_agent_bench/api.py`:
- Around line 305-318: Brev initialization failures in _run_job can escape
before its existing try block, terminating _worker without updating job state.
Move the uses_brev setup, including ensure_running, stop_model, start_model, and
_substitute_server_url, inside the guarded error-handling path so failures mark
the job FAILED and invoke _brev_post_job_cleanup; alternatively add equivalent
exception handling around _run_job in _worker.
- Around line 186-189: Remove the signal.signal registrations for SIGTERM and
SIGINT from the lifespan setup, preserving uvicorn’s handlers so it can initiate
graceful shutdown and execute the async _brev_instance.destroy() path; retain
_brev_emergency_cleanup via lifespan/atexit as appropriate.
In `@src/coding_agent_bench/brev.py`:
- Around line 219-246: After all retries fail in destroy, do not mark the VM as
stopped or return normally; preserve _running (and the instance reference) and
re-raise last_err so _brev_post_job_cleanup surfaces the failure instead of
discarding a potentially running VM. Keep cleanup state resets only on
successful deletion.
---
Nitpick comments:
In `@src/coding_agent_bench/brev.py`:
- Around line 24-29: Populate MODEL_CONFIGS with at least one valid Brev model
entry, including its Docker command and container name, before enabling the Brev
server path; ensure the configured model names match those accepted by
start_model and stop_model.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: ed7b603e-af3a-4e5d-a3e7-afc4af3f7c98
📒 Files selected for processing (4)
Containerfiledeploy/job-queue-service.ymlsrc/coding_agent_bench/api.pysrc/coding_agent_bench/brev.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Containerfile (2)
24-24: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftPin the Brev installer. This image runs
install-latest.shfrommain, so upstream changes can alter the build without review. Pin a release or commit and validate its checksum/signature before executing it.🤖 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 `@Containerfile` at line 24, Pin the installer referenced by the Containerfile’s Brev installation RUN command to a specific release or commit instead of downloading install-latest.sh from main, then verify the downloaded script’s checksum or signature before executing it.
24-24: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInstall
brevonto the final image PATH
install-latest.shputs the binary in${HOME}/.local/bin, andContainerfilenever adds that directory toPATH. Sincesrc/coding_agent_bench/brev.pyrunsbrevdirectly viasubprocess, runtime calls can fail unless you install it into/usr/local/binas root or exportPATH=$HOME/.local/bin:$PATHin the image.🤖 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 `@Containerfile` at line 24, Ensure the brev binary installed by the RUN command is available to runtime subprocess calls in the final image: either install it into /usr/local/bin as root or configure the image PATH to include ${HOME}/.local/bin, so src/coding_agent_bench/brev.py can invoke brev directly.
🤖 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 `@README.md`:
- Around line 186-194: The documented optional BREV_TOKEN conflicts with the
required SecretKeyRef in the deployment. Update the BREV_TOKEN reference in the
job-queue deployment manifest to set optional: true, preserving the README
instructions that omit the key.
---
Outside diff comments:
In `@Containerfile`:
- Line 24: Pin the installer referenced by the Containerfile’s Brev installation
RUN command to a specific release or commit instead of downloading
install-latest.sh from main, then verify the downloaded script’s checksum or
signature before executing it.
- Line 24: Ensure the brev binary installed by the RUN command is available to
runtime subprocess calls in the final image: either install it into
/usr/local/bin as root or configure the image PATH to include
${HOME}/.local/bin, so src/coding_agent_bench/brev.py can invoke brev directly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: ad6ee39f-917b-495f-943c-88a01f810285
📒 Files selected for processing (6)
.env.exampleContainerfileREADME.mddeploy/job-queue-service.ymlsrc/coding_agent_bench/api.pysrc/coding_agent_bench/brev.py
✅ Files skipped from review due to trivial changes (1)
- .env.example
🚧 Files skipped from review as they are similar to previous changes (3)
- deploy/job-queue-service.yml
- src/coding_agent_bench/api.py
- src/coding_agent_bench/brev.py
| 4. Create a secret file named `job-queue-secret` with an `API_KEY` (and optional `BREV_TOKEN`) and apply it: | ||
| ```yaml | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: job-queue-secret | ||
| stringData: | ||
| API_KEY: <your-api-key> | ||
| # BREV_TOKEN: <your-brev-token> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Align the optional-token documentation with the deployment contract.
deploy/job-queue-service.yml references job-queue-secret/BREV_TOKEN without marking the SecretKeyRef optional. A secret created exactly as documented without that key can leave the pod in CreateContainerConfigError, even when Brev is not used. Mark the reference optional in the deployment, or make BREV_TOKEN required in these instructions.
🤖 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 `@README.md` around lines 186 - 194, The documented optional BREV_TOKEN
conflicts with the required SecretKeyRef in the deployment. Update the
BREV_TOKEN reference in the job-queue deployment manifest to set optional: true,
preserving the README instructions that omit the key.
taagarwa-rh
changed the title
1 participant
Automatically manage Brev GPU instances from the job queue service so benchmark jobs can use models served on Brev without manual setup. Pass server_url="brev" to trigger the lifecycle; any other value skips Brev entirely for backwards compatibility.
Lifecycle: create instance on first job → start model container → run benchmark → swap or stop model between jobs → delete instance when queue drains. Same-model jobs are grouped in the queue and reuse the running container to avoid redundant start/stop cycles.
Fault tolerance: startup orphan cleanup, retry-3 on delete, SIGTERM/SIGINT/atexit handlers as a safety net for expensive VMs.
NOTE: Brev tokens are short lived and cannot be reliably used for creating and deleting VMs. Until long-lived tokens are introduced, this must stay in draft