-
Notifications
You must be signed in to change notification settings - Fork 674
refactor: align multimodal example port allocation with vLLM components #4163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
3992615 to
6aabff9
Compare
…in multimodal example Align multimodal example port configuration with components/src/dynamo/vllm/args.py approach. Changes: - Replace allocate_and_reserve_port() with get_kv_port() using DYN_VLLM_KV_EVENT_PORT env var - Update configure_ports() to be synchronous and environment-based - Replace set_side_channel_host_and_port() with ensure_side_channel_host() (host only) - Remove side_channel_port from Config class - Update worker.py to use new configure_ports(config) signature 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
6aabff9 to
0c3cae8
Compare
WalkthroughPort configuration logic is refactored across two files. The async port allocation mechanism is replaced with synchronous environment-driven retrieval. Function signatures are updated, and the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks✅ Passed checks (3 passed)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/multimodal/utils/args.py (1)
139-170: Fix missing kv_port initialization when prefix caching default is None
AsyncEngineArgs.enable_prefix_cachingcomes out of CLI parsing asNoneuntil vLLM applies its defaults.(docs.vllm.ai) With the new synchronous flow, that falsy value skips theconfigure_portsbranch and leavesconfig.kv_portunset. As soon asoverwrite_argsformats the ZMQ endpoint,config.kv_port - dp_rankraises aTypeError, so the worker never starts.Set
config.kv_portbefore we build the defaults by treatingNoneas “we will enable prefix caching” (which matches the V1 default) and assigning the port eagerly.def configure_ports(config: Config): """Configure port settings from dedicated environment overrides.""" - if config.engine_args.enable_prefix_caching: - config.kv_port = get_kv_port() + enable_prefix = config.engine_args.enable_prefix_caching + if enable_prefix is None: + enable_prefix = True + config.engine_args.enable_prefix_caching = True + + if enable_prefix: + config.kv_port = get_kv_port() ensure_side_channel_host()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/multimodal/components/worker.py(1 hunks)examples/multimodal/utils/args.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/multimodal/components/worker.py (1)
examples/multimodal/utils/args.py (1)
configure_ports(139-145)
examples/multimodal/utils/args.py (4)
lib/bindings/python/examples/hello_world/server_sglang_static.py (1)
Config(31-37)lib/bindings/python/examples/hello_world/server_sglang.py (1)
Config(40-46)lib/bindings/python/examples/hello_world/server_sglang_tok.py (1)
Config(43-49)lib/bindings/python/examples/hello_world/server_vllm.py (1)
Config(52-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: trtllm (amd64)
- GitHub Check: trtllm (arm64)
- GitHub Check: vllm (arm64)
- GitHub Check: operator (arm64)
- GitHub Check: sglang (amd64)
- GitHub Check: sglang (arm64)
- GitHub Check: vllm (amd64)
- GitHub Check: Build and Test - dynamo
Update launch scripts to set unique VLLM_NIXL_SIDE_CHANNEL_PORT values for multi-worker deployments to avoid port conflicts. Uses ports 20097, 20098, 20099 to match the pattern from components folder examples. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The overwrite_args function always uses config.kv_port regardless of prefix caching settings, so configure_ports must always set this value to prevent "NoneType and int" TypeError. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
316141e to
c0da5bd
Compare
Summary
Changes Made
Port Allocation Strategy
runtime.allocate_port_block()DYN_VLLM_KV_EVENT_PORTConfiguration Functions
configure_ports()to be synchronous and use environment variablescreate_kv_events_config()function to match VLLM components patternoverwrite_args()to follow the same structure as VLLM componentsHost Management
ensure_side_channel_host()andget_host_ip()functionsset_side_channel_host_and_port()functionCode Structure
side_channel_portfieldconfigure_ports(config)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit