feat(vllm): adopt split gRPC services and Control discovery - #12734
Conversation
9e44c18 to
663fa53
Compare
|
🎯 Code Coverage (details) 🔗 Commit SHA: 1b1b290 | Docs | Datadog PR Page | Give us feedback! |
WalkthroughChangesvLLM API migration and discovery
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/sidecar/vllm/src/model.rs (1)
63-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the field that changed in the identity error.
ModelIdentityalso comparesaliases,reasoning_parser, andtool_call_parser. If only one of those changes, the message prints identical source and served names, which makes the failure hard to diagnose. Include the two identity values in the message, for example with{:?}onself.identityandobserved.identity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/sidecar/vllm/src/model.rs` around lines 63 - 71, Update ModelIdentity::ensure_same_identity to include the complete expected and observed identity values in the protocol error message, using debug formatting for self.identity and observed.identity. Preserve the existing source and served-name context while making changes to aliases, reasoning_parser, or tool_call_parser visible.lib/sidecar/vllm/src/tests.rs (1)
175-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an identity-mismatch startup test.
After bootstrap, set
server.service.model_info_overrideto a different model and assert thatengine.start(0)returnsmodel identity changed between bootstrap and startup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/sidecar/vllm/src/tests.rs` around lines 175 - 186, Add a test covering identity mismatch after bootstrap: update server.service.model_info_override with a different model than the bootstrapped one, then assert engine.start(0) fails with the error message “model identity changed between bootstrap and startup.” Reuse the existing bootstrap, server, and model-construction helpers in the surrounding tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/mocker/servers/vllm/src/server.rs`:
- Around line 89-98: Update the ModelInfo construction to set
supports_text_input to false, matching PreparedRequest::new’s rejection of
Prompt::Text; leave token-ID input capability unchanged.
In `@lib/sidecar/vllm/src/engine.rs`:
- Around line 254-279: Document the synchronous execution constraint on
VllmSidecarEngine::from_args and its bootstrap_discover Runtime::block_on path.
State that it must be called before dynamo_backend_common::run in the production
entry point, while asynchronous callers must invoke it through spawn_blocking or
an equivalent dedicated thread.
In `@lib/sidecar/vllm/src/model.rs`:
- Around line 90-101: Update the required and nonempty helpers to return the
trimmed value after validation, while preserving their existing error and None
behavior for blank inputs. Ensure GenerateRequest.model receives the normalized
model name without surrounding whitespace.
---
Nitpick comments:
In `@lib/sidecar/vllm/src/model.rs`:
- Around line 63-71: Update ModelIdentity::ensure_same_identity to include the
complete expected and observed identity values in the protocol error message,
using debug formatting for self.identity and observed.identity. Preserve the
existing source and served-name context while making changes to aliases,
reasoning_parser, or tool_call_parser visible.
In `@lib/sidecar/vllm/src/tests.rs`:
- Around line 175-186: Add a test covering identity mismatch after bootstrap:
update server.service.model_info_override with a different model than the
bootstrapped one, then assert engine.start(0) fails with the error message
“model identity changed between bootstrap and startup.” Reuse the existing
bootstrap, server, and model-construction helpers in the surrounding tests.
🪄 Autofix
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: Enterprise
Run ID: bd81be70-6b4b-4b97-8e51-4888cd03fadc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.github/workflows/copyright-check.ps1.pre-commit-config.yamllib/mocker/servers/vllm/Cargo.tomllib/mocker/servers/vllm/README.mdlib/mocker/servers/vllm/src/main.rslib/mocker/servers/vllm/src/server.rslib/mocker/servers/vllm/src/server_request.rslib/mocker/servers/vllm/src/server_tests.rslib/mocker/servers/vllm/tests/sidecar.rslib/sidecar/vllm/Cargo.tomllib/sidecar/vllm/README.mdlib/sidecar/vllm/build.rslib/sidecar/vllm/deploy/agg.yamllib/sidecar/vllm/deploy/disagg.yamllib/sidecar/vllm/launch/agg.shlib/sidecar/vllm/launch/disagg.shlib/sidecar/vllm/proto/README.mdlib/sidecar/vllm/proto/control.protolib/sidecar/vllm/proto/inference.protolib/sidecar/vllm/src/args.rslib/sidecar/vllm/src/client.rslib/sidecar/vllm/src/convert.rslib/sidecar/vllm/src/engine.rslib/sidecar/vllm/src/model.rslib/sidecar/vllm/src/tests.rslib/sidecar/vllm/tests/executable.rs
💤 Files with no reviewable changes (3)
- lib/sidecar/vllm/tests/executable.rs
- lib/sidecar/vllm/src/args.rs
- lib/sidecar/vllm/launch/disagg.sh
PeaBrane
left a comment
There was a problem hiding this comment.
✅ General approval, conditional on resolving the inline comments before merge. Please also replace the stale vLLM gRPC v0.25.1 validation wording in lib/sidecar/vllm/src/convert.rs with feature-based wording.
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
7a35d79 to
9a056be
Compare
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
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.
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Overview:
Part 1 of a 4-PR stack. This foundation moves the Dynamo vLLM sidecar and mocker to vLLM's split native gRPC services and discovers runtime metadata through Control.
Details:
InferenceandControlprotocols pinned to2d2c3af18c52e8e4efa4b0b4903843b15c0dba0e.Control.Deterministic DP/KV routing, cancellation handoff behavior, and multimodal conversion remain in later PRs.
Stack
Base:
mainValidation
cargo fmt --all -- --checkcargo test -p dynamo-vllm-sidecar -p dynamo-vllm-mocker— 30 tests passedcargo clippy -p dynamo-vllm-sidecar -p dynamo-vllm-mocker --all-targets -- -D warningsWhere should the reviewer start?
lib/sidecar/vllm/src/client.rsfor health and startup retry behavior.lib/sidecar/vllm/src/model.rsfor Control metadata validation and identity checks.lib/sidecar/vllm/src/engine.rsfor bootstrap discovery and shared transport startup.lib/mocker/servers/vllm/src/server.rsfor the split-service mock contract.Related Issues
This PR is NOT linked to an issue: