fix: properly setup and register vLLM worker for external / hybrid load balancing. Update launch script - #6695
Conversation
WalkthroughReplaces per-worker data-parallel size semantics with explicit (start_rank, size) ranges across vLLM, kv-router, and runtime config. Iteration and publishing of DP ranks now use worker-specific ranges; non-leader node gating logic was removed and new DP-range helpers were added. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
Both |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/kv-router/src/multi_worker_sequence.rs (1)
252-252: Renamenew_dp_sizestonew_dp_rangesfor semantic clarity.The tuple payload is now a range (
start, size), so the current name is slightly misleading for callers/readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/kv-router/src/multi_worker_sequence.rs` at line 252, Rename the parameter and all internal uses of new_dp_sizes to new_dp_ranges in the update_workers method to reflect that the tuple is a range (start, size); update the function signature pub fn update_workers(&self, new_dp_ranges: HashMap<u64, (u32, u32)>) and replace every occurrence of new_dp_sizes within update_workers, plus update all call sites that pass or reference that parameter name to use new_dp_ranges so names remain consistent across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/src/dynamo/vllm/handlers.py`:
- Around line 491-500: The function _to_local_dp_rank currently converts an
out-of-range explicit dp_rank into None which lets vLLM pick a DP rank; instead,
detect when routing.dp_rank is explicitly provided but falls outside this
worker’s dp_range and raise a hard rejection (e.g., throw a ValueError or a
RequestRoutingError) so the request fails rather than silently falling back.
Update _to_local_dp_rank to raise on out-of-range dp_rank and propagate that
change to any callers that pass data_parallel_rank (ensure callers
handle/propagate the exception); apply the same fix to the other duplicate
checks referenced (around the functions/blocks at the other locations you noted)
so all code paths fail fast on explicit, out-of-range routing.dp_rank.
In `@lib/llm/src/discovery/worker_monitor.rs`:
- Around line 459-465: The cleanup fallback assumes rank 0 which breaks when
data_parallel_start_rank != 0; update both cleanup branches that use vec![0] to
derive the fallback dp ranks from the actual data-parallel range for the worker
(use runtime_config.data_parallel_start_rank and
runtime_config.data_parallel_size to compute dp_start..dp_end) or, if
unavailable, derive them from existing keys in
worker_load_states/known_worker_dp_ranks for that lease_id; locate uses around
known_worker_dp_ranks, dp_start/dp_end, and worker_load_states cleanup branches
and replace the hardcoded vec![0] with a computed set based on those symbols so
stale metric series for non-zero start ranks are correctly cleaned up.
In `@lib/llm/src/kv_router/scheduler.rs`:
- Around line 444-446: The loop end uses unchecked addition of
data_parallel_start_rank and data_parallel_size which can overflow; update the
code around data_parallel_start_rank and the for loop to compute the end with
checked_add (e.g., data_parallel_start_rank.checked_add(data_parallel_size)) and
panic with a clear message if it returns None, then iterate using that computed
end (use the checked end as the upper bound in the for dp_rank in ... loop) so
invalid configs fail fast; refer to the variables data_parallel_start_rank and
data_parallel_size in scheduler.rs when making this change.
---
Nitpick comments:
In `@lib/kv-router/src/multi_worker_sequence.rs`:
- Line 252: Rename the parameter and all internal uses of new_dp_sizes to
new_dp_ranges in the update_workers method to reflect that the tuple is a range
(start, size); update the function signature pub fn update_workers(&self,
new_dp_ranges: HashMap<u64, (u32, u32)>) and replace every occurrence of
new_dp_sizes within update_workers, plus update all call sites that pass or
reference that parameter name to use new_dp_ranges so names remain consistent
across the codebase.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
components/src/dynamo/vllm/handlers.pycomponents/src/dynamo/vllm/main.pycomponents/src/dynamo/vllm/publisher.pyexamples/backends/vllm/launch/dep.shexamples/backends/vllm/launch/dsr1_dep.shlib/bindings/python/rust/llm/local_model.rslib/kv-router/src/multi_worker_sequence.rslib/llm/src/discovery/worker_monitor.rslib/llm/src/kv_router/queue.rslib/llm/src/kv_router/scheduler.rslib/llm/src/kv_router/sequence.rslib/llm/src/local_model/runtime_config.rs
💤 Files with no reviewable changes (1)
- components/src/dynamo/vllm/publisher.py
…ancing Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
… DP rank) Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
…ting stats logger, KV event publisher for its DP ranks Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
ptarasiewiczNV
left a comment
There was a problem hiding this comment.
Pls add the explicit logging on the "internal load balancing" code path. Otherwise approve on vllm/python side.
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
…ad balancing. Update launch script (#6695) Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
…ad balancing. Update launch script (ai-dynamo#6695) Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo 41310872+GuanLuo@users.noreply.github.com
Overview:
Code change:
Script change: We are moving from external load balancing to hybrid load balancing of DPs.
Details:
Example log from KV router with 2 worker each owns 4 DP ranks (hybrid load balancing)
First request
Repeat request
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Chores
Bug Fixes