feat(vllm): enable deterministic DP and KV routing - #12735
Conversation
8db0c99 to
3bfb361
Compare
This comment has been minimized.
This comment has been minimized.
3463378 to
522511d
Compare
4e5e10b to
4e06d75
Compare
4e06d75 to
dfbf023
Compare
dfbf023 to
30e68eb
Compare
tanmayv25
left a comment
There was a problem hiding this comment.
Automated multi-agent code-review pass over this PR's diff (framing + expert lenses, each finding adversarially verified). Findings below are all minor / non-blocking.
30e68eb to
7b775cc
Compare
7b775cc to
8304227
Compare
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
8304227 to
696dfeb
Compare
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
|
/ok to test 8a37d26 |
karen-sy
left a comment
There was a problem hiding this comment.
Findings
-
P1
lib/sidecar/vllm/src/engine.rs:133,371
KV routing is enabled unconditionally, but an emptyGetKvEventSourcesresponse is treated as a protocol error. At the vendored vLLM commit, Control intentionally returns no source whenkv_events_configis absent, disabled, or uses a non-ZMQ publisher. That makes an otherwise valid non-KV-event vLLM sidecar fail worker startup. Return an empty source list in the genuinely no-source case so the worker’s existing KV-routing opt-out path applies; retain the strict all-ranks check when sources are advertised. Add a no-KV-events startup test.
Upstream behavior: [vLLM Control source](https://github.com/vllm-project/vllm/blob/3d1f5cee1552b8208f3009c75f8bc856f27e0eff/rust/src/server/src/grpc/control.rs#L117-L121). -
P2
lib/sidecar/vllm/src/engine.rs:181,333
start()returns the freshly observed DP topology, butkv_event_sources()still validates against the bootstrap-timeself.model.ensure_same_identity()does not compare parallelism. If the endpoint lands on a same-model frontend with a different DP size between bootstrap and startup, Dynamo registers the new range but source validation uses the old size. Reject topology changes during startup, or retain the observed topology in the engine.
Noting some flags above from AI review, but generally looks good!
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
|
/ok to test d9d1ae4 |
Overview
Part 3 of a 4-PR stack. This layer enables deterministic vLLM data-parallel rank selection and KV-event routing without extending the vLLM protocol.
Details
x-data-parallel-rankgRPC metadata; omit the metadata when no rank is selected so vLLM retains normal load balancing.x-data-parallel-rankgRPC metadata for deterministic DP routing. No protocol capability field is used.nvext.token_in=truemarker while preserving strict rejection of unsupported nvext semantics.Multimodal request conversion remains in the top PR.
Stack
Base: #12736
Validation
cargo fmt --all -- --checkcargo test -p dynamo-vllm-sidecar— 18 unit tests and 1 executable integration test passedcargo test -p dynamo-vllm-mocker— 12 unit tests and 3 sidecar integration tests passedcargo clippy -p dynamo-vllm-sidecar --all-targets -- -D warningscargo clippy -p dynamo-vllm-mocker --all-targets -- -D warningscargo test -p dynamo-runtime utils::pool::tests::test_sync_pool_blocking_acquire -- --exact— passed; verifies the prior CI failure was transientWhere should the reviewer start?
lib/sidecar/vllm/src/client.rsfor request-scoped gRPC metadata.lib/sidecar/vllm/src/model.rsfor the supported full-group DP registration contract.lib/sidecar/vllm/src/engine.rsfor rank selection and KV-event source validation.lib/mocker/servers/vllm/src/server.rsfor mock transport parity.Related Issues
This PR is NOT linked to an issue: