Repository navigation
fix(serve): respect user-set CUDA_VISIBLE_DEVICES in gpu_env - #750
paxiaatucsdedu wants to merge 2 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR enhances GPU environment setup in the Python serve module by adding support for respecting externally provided CUDA_VISIBLE_DEVICES as a GPU pool. When set, it partitions this list according to tensor parallelism size and distributed parallelism rank to assign appropriate GPU subsets to workers, with bounds validation and fallback behavior for legacy setups. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the issue of respecting an existing CUDA_VISIBLE_DEVICES environment variable when launching workers. The implementation is solid: it properly parses the existing variable, slices the available GPU pool based on data and tensor parallelism ranks, and includes a helpful bounds check with a clear error message. The new unit tests are thorough and effectively validate the changes. I have one minor suggestion to simplify the code slightly.
When CUDA_VISIBLE_DEVICES is already set (e.g. via Docker -e), treat it as the available GPU pool instead of overriding it. Adds bounds check and unit tests." Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
3984d1a to
b598982
Compare
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 `@bindings/python/src/smg/serve.py`:
- Around line 67-83: Validate tp_size returned by _get_tp_size in serve.py
before using it in GPU index math: ensure tp_size is a positive integer (tp_size
> 0) and raise a clear ValueError if not; update the logic around tp_size,
base_idx = dp_rank * tp_size, and the available_gpus slicing to assume a valid
tp_size only after this check (refer to _get_tp_size, tp_size, base_idx,
available_gpus, and gpu_ids in the shown block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9d0d2c5-03de-4f7e-984d-92a46842c871
📒 Files selected for processing (2)
bindings/python/src/smg/serve.pybindings/python/tests/test_serve.py
Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
7cdd2bb to
ed6f625
Compare
Description
Problem
When running
smg serveinside a Docker container withCUDA_VISIBLE_DEVICESset(e.g.
docker run -e CUDA_VISIBLE_DEVICES=4 ...), the gpu_env method inWorkerLauncher unconditionally overrides the variable with sequential IDs starting
from 0. This causes workers to land on the wrong GPU, leading to OOM errors when
another process is already using GPU 0.
Solution
Modify gpu_env to check whether
CUDA_VISIBLE_DEVICESis already set in theenvironment. If it is, treat it as the available GPU pool and slice into it by
dp_rankandtp_size. If it is not set (or empty), fall back to the originalsequential assignment (
0,1,...). A bounds check raises a clearValueErrorwhenthe pool has fewer GPUs than the requested
dp_rank * tp_sizerange.Changes
into an existing
CUDA_VISIBLE_DEVICESpool instead of overriding it. Add boundscheck with descriptive error message.
(parametrized across multiple GPU layouts), cross-backend verification (vllm),
bounds check (
ValueError), and empty-string fallback.Test Plan
Manual verification (Docker):
Added unit test at bindings/python/tests/test_serve.py
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Tests