feat: Make vllm sleep level customizable#2523
Conversation
WalkthroughThis PR introduces configurable sleep levels for vLLM workers. A new optional ChangesSleep Level Configuration and Implementation
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/models/generation/test_vllm_worker.py`:
- Line 46: The regex in the pytest.raises match is using unescaped dots and
should be made a raw string with the dots escaped to avoid over-matching; update
the assertion in the test_vllm_worker test (the pytest.raises line that
currently matches "vllm_cfg.sleep_level must be 0, 1, or 2") to use a raw string
and escape the literal dots (e.g., change the match to a raw pattern where '.'
is written as '\.').
🪄 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: Enterprise
Run ID: 3d403493-38f5-4cce-8ded-c73aae6bdb1c
📒 Files selected for processing (4)
nemo_rl/models/generation/vllm/config.pynemo_rl/models/generation/vllm/vllm_worker.pynemo_rl/models/generation/vllm/vllm_worker_async.pytests/unit/models/generation/test_vllm_worker.py
|
@yuki-97 @parthmannan to review @Kipok out of curiosity, what scenario was this helpful in? if host mem was around 900GB for GB but VRAM was ~200GB with only fraction of that being model weights, i would imagine the savings here to not be very impactful unless there was something else very large eating up host memory. |
|
I haven't looked too much into what was eating up cpu memory, but somehow my experiments using nemotron-super worked on one internal cluster, but didn't work on another one because of cpu out-of-memory. I think there was something like 50gb difference or so in total available memory per node but that was enough to push it over the limit. Setting sleep level to 2 helped to resolve it with no impact on speed. But again I don't fully understand what was taking the memory and maybe there is a better fix, but this seems to work and I thought could be useful to others as well |
|
FWIW, the change is okay with me, just trying to understand the symptom @joyang-nv @guyueh1 could this have potentially been explained with the numa issue on some clusters? |
Signed-off-by: Igor Gitman <igitman@nvidia.com>
Signed-off-by: Igor Gitman <igitman@nvidia.com>
Signed-off-by: Igor Gitman <igitman@nvidia.com>
Signed-off-by: Igor Gitman <igitman@nvidia.com>
feaa99b to
209fb05
Compare
|
thanks @yuki-97, addressed the comments |
|
@Kipok I think this will cause issues to validation; in grpo.py, when doing validation, we first do refit then do validation then sleep the engine; then in the next step we wakeup the engine and directly start generation because we think we have done refit already. If we use level=2 here, then the sleep will discard the weights and the next generation after validation will use dummy weights. I implemented a way to only set sleep_level=2 in proper places in #2495 in parallel to your PR |
|
sure, sounds good, let me close this one then! |
What does this PR do ?
Makes vllm sleep level customizable. E.g. sleep level 2 is useful for cases when CPU memory is limited
Summary by CodeRabbit
New Features
Tests