fix: support approx routing in mm router - #8135
Conversation
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
WalkthroughThis pull request introduces support for approximate KV routing mode in vLLM multimodal router setups. Changes include a new CLI flag to disable router KV events, a bash launch script for Qwen3.5 model orchestration with three-process setup, and parametrized test fixtures to validate both exact and approximate routing configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/backends/vllm/qwen35/launch.sh (1)
51-51: Avoid fixed fallback ports in a new launch script.Defaulting to 8000/8081/8082 makes this example fail unnecessarily when another local stack is already up. Prefer reserving ports dynamically when the env vars are unset, then thread the resolved values through the banner and child processes.
Based on learnings, "Flag hard-coded portability-reducing constants in shell/scripts across the repository (e.g., static ports). Prefer portable alternatives available in the repo: use alloc_port for dynamic ports."
Also applies to: 97-97, 113-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/backends/vllm/qwen35/launch.sh` at line 51, Replace the hard-coded fallbacks for HTTP/WS ports with dynamic allocation: when DYN_HTTP_PORT (and the other env vars used at the other assignments) are unset, call the repository's alloc_port helper to reserve an available port and assign that to HTTP_PORT (and the corresponding WS/other port variables), then export these resolved variables so the banner and the child processes receive the same values; update the references to DYN_HTTP_PORT/HTTP_PORT (and the other port variables present later in the script) to use the newly allocated values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/backends/vllm/qwen35/launch.sh`:
- Line 51: The script computes HTTP_PORT but never passes it into the frontend
invocation, so the banner/curl examples can point at the wrong socket; update
each call to dynamo.frontend (the frontend command invocation seen near the
HTTP_PORT assignment around line 51 and the other invocation around lines
121-122) to accept the resolved port by supplying "$HTTP_PORT" (or the
appropriate --port "$HTTP_PORT" flag) as an argument or environment variable so
the actual frontend listens on the same port printed in banners and curl
examples.
In `@tests/mm_router/test_vllm_mm_router_e2e.py`:
- Around line 216-230: The start_vllm_mm_services fixture currently calls
allocate_ports(...) manually and branches on request.param; replace that manual
port allocation by adding num_system_ports and the runtime fixtures
(runtime_services_dynamic_ports and dynamo_dynamic_ports) to the fixture
signature and consume the dynamic port lists they provide; when approx_routing
is True pull three ports from runtime_services_dynamic_ports, otherwise pull
four ports (including an extra kv_event port) by combining
runtime_services_dynamic_ports and dynamo_dynamic_ports as needed, removing
allocate_ports and start_port usage and keeping the rest of the fixture logic
unchanged so xdist-safe dynamic port allocation is used.
---
Nitpick comments:
In `@examples/backends/vllm/qwen35/launch.sh`:
- Line 51: Replace the hard-coded fallbacks for HTTP/WS ports with dynamic
allocation: when DYN_HTTP_PORT (and the other env vars used at the other
assignments) are unset, call the repository's alloc_port helper to reserve an
available port and assign that to HTTP_PORT (and the corresponding WS/other port
variables), then export these resolved variables so the banner and the child
processes receive the same values; update the references to
DYN_HTTP_PORT/HTTP_PORT (and the other port variables present later in the
script) to use the newly allocated values.
🪄 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: Pro
Run ID: 3908be6a-1169-4466-8737-03926bb9ecc0
📒 Files selected for processing (3)
examples/backends/vllm/mm_router_worker/mm_router_worker.pyexamples/backends/vllm/qwen35/launch.shtests/mm_router/test_vllm_mm_router_e2e.py
|
|
||
| MAX_MODEL_LEN="${MAX_MODEL_LEN:-4096}" | ||
| MAX_CONCURRENT_SEQS="${MAX_CONCURRENT_SEQS:-2}" | ||
| HTTP_PORT="${DYN_HTTP_PORT:-8000}" |
There was a problem hiding this comment.
Pass the resolved frontend port into dynamo.frontend.
HTTP_PORT drives the banner and curl examples, but the frontend command never consumes it. That makes the documented port override path rely on implicit frontend behavior and can leave the printed curl command pointing at the wrong socket.
🔧 Proposed fix
python -m dynamo.frontend \
+ --http-port "$HTTP_PORT" \
--router-mode round-robin &Also applies to: 121-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/backends/vllm/qwen35/launch.sh` at line 51, The script computes
HTTP_PORT but never passes it into the frontend invocation, so the banner/curl
examples can point at the wrong socket; update each call to dynamo.frontend (the
frontend command invocation seen near the HTTP_PORT assignment around line 51
and the other invocation around lines 121-122) to accept the resolved port by
supplying "$HTTP_PORT" (or the appropriate --port "$HTTP_PORT" flag) as an
argument or environment variable so the actual frontend listens on the same port
printed in banners and curl examples.
| @pytest.fixture(scope="module", params=[False, True], ids=["exact_kv", "approx_kv"]) | ||
| def start_vllm_mm_services( | ||
| request, mm_runtime_services | ||
| ) -> Generator[tuple[int, ManagedProcess], None, None]: | ||
| frontend_port, vllm_port, router_port, kv_event_port = allocate_ports( | ||
| count=4, start_port=10000 | ||
| ) | ||
| approx_routing = request.param | ||
|
|
||
| if approx_routing: | ||
| frontend_port, vllm_port, router_port = allocate_ports( | ||
| count=3, start_port=10000 | ||
| ) | ||
| kv_event_port = None | ||
| else: | ||
| frontend_port, vllm_port, router_port, kv_event_port = allocate_ports( | ||
| count=4, start_port=10000 | ||
| ) |
There was a problem hiding this comment.
Use the repo’s dynamic service/port fixtures here instead of extending the hand-rolled setup.
Adding the exact/approx split on top of manual allocate_ports(..., start_port=10000) keeps this module outside the xdist-safe path the rest of the suite uses. Please request the needed system ports via num_system_ports and consume runtime_services_dynamic_ports + dynamo_dynamic_ports instead of branching port allocation manually here.
As per coding guidelines, "Use runtime_services_dynamic_ports and dynamo_dynamic_ports fixtures together for xdist/parallel safety" and "Use num_system_ports parametrize ... to request multiple system ports."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mm_router/test_vllm_mm_router_e2e.py` around lines 216 - 230, The
start_vllm_mm_services fixture currently calls allocate_ports(...) manually and
branches on request.param; replace that manual port allocation by adding
num_system_ports and the runtime fixtures (runtime_services_dynamic_ports and
dynamo_dynamic_ports) to the fixture signature and consume the dynamic port
lists they provide; when approx_routing is True pull three ports from
runtime_services_dynamic_ports, otherwise pull four ports (including an extra
kv_event port) by combining runtime_services_dynamic_ports and
dynamo_dynamic_ports as needed, removing allocate_ports and start_port usage and
keeping the rest of the fixture logic unchanged so xdist-safe dynamic port
allocation is used.
| --enable-multimodal \ | ||
| --mamba-cache-mode align \ | ||
| --block-size "$BLOCK_SIZE" \ | ||
| --enforce-eager \ |
There was a problem hiding this comment.
should we leave this out?
for perf reasons, we should not enforce eager by default.
people can pass this as extra_args
|
Thanks for working on these! just want to make sure you two are aware of these parallel work and coordinate
|
2 participants
Overview:
Details:
Where should the reviewer start?
Test plan
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Release Notes
New Features
Tests