feat: Sglang Rollout Refactor - #2267
Conversation
|
/ok to test 4d0c640 |
terrykong
left a comment
There was a problem hiding this comment.
SGLang Rollout Refactor — Review
Thanks for this substantial refactoring effort, @xiuhu17! The new architecture (router, multi-node support, fault tolerance, weight/KV offload) is a meaningful step toward production readiness. Below are findings grouped by priority.
Merge Conflicts
This PR currently has merge conflicts with main (mergeable: CONFLICTING). Please rebase on main and resolve conflicts before further review iterations.
Missing Copyright Headers
8 new/modified source files under nemo_rl/ are missing the NVIDIA Apache 2.0 copyright header (required per project guidelines for non-test files):
sglang_generation.py,sglang_worker.py,fault_tolerance.py,sglang_router.pyutils/async_utils.py,utils/http_utils.py,utils/misc.py,utils/ray_utils.py
Missing # pragma: no cover
3 @ray.remote classes are missing the coverage pragma (required for Ray actors per testing conventions):
RouterActorinsglang_router.py:10Lockinutils/ray_utils.py:22_HttpPosterActorinutils/http_utils.py:106
Performance / Convergence Evidence
This PR rewrites the weight-streaming protocol, adds CUDA-graph/KV offload, multi-node SGLang, and a new router — but provides no quantitative evidence (throughput, refit latency, memory, convergence curves). The test link is to an external Google Doc. Could you share tokens/sec or step-time numbers (with and without the new features on a representative model) directly in the PR description?
Documentation
No docs/ updates for the user-facing additions (router config, fault tolerance, pause/continue, weight check, async generation). Per project conventions, new features should include in-repo documentation.
Test Deletions
332 lines deleted from test_grpo.py and 224 lines from test_policy_utils.py — could you clarify what coverage replaces these? They don't appear to be SGLang-specific tests.
Generated by Claude Code
yuki-97
left a comment
There was a problem hiding this comment.
mainly some style things.
|
hi @xiuhu17 , I finished review, sorry so many comments. could you help to solve them and the remaining previous comments? also please lmk if you need help. besides there're some other fails will need you to fix.
|
e5d8496 to
d6017ff
Compare
7d5f0b4 to
2deff62
Compare
|
/ok to test 2deff62 |
|
/ok to test 3979db4 |
|
/ok to test a051b72 |
|
/ok to test c620535 |
|
/ok to test 9f3948c |
|
/ok to test 749fb1a |
Signed-off-by: Yuki Huang <yukih@nvidia.com>
|
/ok to test 9f09507 |
Refactor SGLang Rollout with the New Design
Overview
This PR refactors the SGLang rollout stack to support the new rollout architecture while preserving backward compatibility with the previous interface.
The new design focuses on improving scalability, reliability, and maintainability of the rollout system. In particular, it introduces support for:
async def generate_async(...)for asynchronous multi-turn rolloutThe goal is to make the rollout layer production-ready for larger-scale serving and training integration, while minimizing disruption to existing callers.
Tests:
Thorough tests(around 75 tests):
Summary by CodeRabbit
Release Notes
New Features
Improvements