Repository navigation
build(vllm): consume upstream vllm-proto crate - #14738
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (24)
💤 Files with no reviewable changes (5)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe vLLM sidecar now uses the pinned ChangesvLLM protocol integration
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The protocol migration, Tonic v14 adapters, and unsupported-LoRA behavior are internally aligned and ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 15.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 10 files. (9 skipped: 9 unsupported.)
Comment |
333558e to
8b84cc6
Compare
8b84cc6 to
54e8ba8
Compare
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
Signed-off-by: Alec Flowers <aflowers@nvidia.com>
aac5914 to
591a939
Compare
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Reviewed at 591a939 against merge-base 702f532. Read DYN-4422 and the two linked vLLM PRs before the diff, then checked the PR out into a worktree and verified by execution on macOS ARM64 / Rust 1.96.1 (nothing here needs a GPU).
What I verified rather than read
- Wire compatibility is preserved. I pulled
vllm-proto 0.1.0from crates.io and diffed its.protofiles against the deleted vendored copies.inference.protodiffers by exactly one field — Dynamo'snative_sampling_params_json = 16is gone, upstream'slora_name = 15is new.control.protodiffers only by additions (the three LoRA RPCs,ServerInfo.max_loras = 10,ModelInfo.supports_lora = 22) plus the removal of Dynamo'ssupports_native_sampling_params_json = 12. Every delta is field-number-disjoint and additive, and the sidecar sendslora_name: "", a proto3 default that is never serialized — so a v0.28.0 engine sees a byte-identical request. Theagg.yamlcomment claim holds. - The crate needs no
protoc.vllm-proto'sbuild.rscompiles its bundled protos withprotox(pure Rust), soprotobuf-compilerin the Dockerfile is now only there for the SGLang and TRT-LLM sidecars — which is what the rewritten comment says. License isApache-2.0, insidedeny.toml's allow list. cargo test -p dynamo-sidecar-common -p dynamo-vllm-sidecar -p dynamo-vllm-mocker --all-targets --locked→ 67 passed, 0 failed, andnode .github/scripts/test-filters.js→ 71/71. Both match the PR body exactly.- The v14 status adapter is correct.
status.code() as i32→tonic::Code::from_i32round-trips all 17 gRPC codes to an identicalErrorTypeand an identical message string versus the 0.13 path. Details in the P3 onerror.rs. - The v14 connection pool is genuinely exercised.
include!("transport.rs")carries the test module in with it, sov14::tests::startup_deadline_caps_connection_retriesruns alongsidetransport::tests::…— the two pools are the same source and cannot drift. - Dropping the native-Generate capability costs nothing against a released vLLM.
supports_native_sampling_params_jsonwas Dynamo's own field 12, absent from upstreamcontrol.proto, so no released vLLM ever set it: the capability was already never advertised and/inference/v1/generatealready 404'd onmainfor sidecar workers. With one exception, below, this PR removes nothing that worked. - LoRA really is rejected, not silently downgraded.
validate_request(convert.rs:797) already errors onrouting.lora_name, so the hardcodedlora_name: String::new()is unreachable-by-construction rather than a silent switch to base weights.
CI
DGDR Deploy Test / CPU / profiling and the deploy-status-check aggregator it feeds are red. Triaged as unrelated and not reported as a finding: it fails in wait_for_phase_at_least on a Kubernetes profiling Job for backend='vllm', nothing in this diff reaches the profiler, the same job is green on main at four surrounding commits and on another PR from the same window, and rust-tests (.), rust-clippy (.), dynamo-sidecar / Build multi-arch cpu, vllm-runtime / Test, and all four vllm Deploy Test legs are green at this SHA. Worth a re-run.
Findings — one P2, two P3, none blocking
- P2 —
lib/sidecar/vllm/src/convert.rs:33. The new guard also breaks the v1.4-frontend compatibility path thatnormalize_response_optionsstill exists to serve. A/B measured with a control. This one is a decision, not a typo: narrow the guard, or delete the shim and document the break. - P3 —
.github/scripts/test-filters.js:130. The replaced case was the only test of thelib/sidecar/**/*.proto→rustrule, which still has two live consumers. Mutation-tested in both directions; verified drop-in suggestion attached. - P3 —
lib/sidecar/common/src/error.rs:92.status_to_dynamo_v14is untested. I proved it correct today, so this is only about pinning it across a future tonic bump. Verified snippet attached.
Deliberately not raised
The prost-types skew between the root and bindings lockfiles (0.14.3 vs 0.14.4) — I checked the base tree and it already skews the same way, along with tonic 0.14.6 already being in both trees, so neither is introduced here. Likewise the stale .gitattributes:22 entry pointing at the long-gone lib/backend/vllm-sidecar/proto/vllm_grpc.proto, and the dynamo-frontend:1.3.0 pin in deploy/agg.yaml: both pre-existing.
Where verification stopped
No live serving run. Everything above is a local cargo test, a crate-source diff, or a CI-log read on ARM64 macOS — so "wire compatible" here means the schemas and the encoder agree, not measured against a running engine. The PR body is equally explicit that no post-rebase GPU smoke run was done, and the pre-rebase validation lives in the earlier description. Given the protos differ by two additive fields neither side populates, I did not judge an engine run necessary to approve, but it is the gap.
Approving: zero P0/P1, three combined P2+P3.
|
I reviewed the rebased changes and CI. Rust and sidecar/vLLM checks passed at the PR head; deployment profiling failed from node DiskPressure before the container started, so that needs an infrastructure rerun once disk capacity is healthy. The legacy sampling guard fix, restored proto filter fixture, and README conflict resolution are implemented locally. The README retains main's vLLM-Omni guidance, and full native Generate capability remains disabled. The regression fails before the guard fix and passes afterward; all 68 common/sidecar/mock tests, all 71 filter tests, Clippy, formatting, and repository hooks pass. The fixes and merge resolution are staged together, uncommitted and not pushed. I replied to and resolved the three review conversations; the current PR head does not yet contain these local fixes. |
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
Summary
Use the official
vllm-proto = "=0.1.0"crate for the vLLM sidecar and mock server, removing copied schemas and the local generation script. Centralize the required Tonic/Prost 0.14 dependencies through workspace aliases and retain the shared connection policy for the existing 0.13 consumers.Rebased onto current main, preserving supported typed sampling controls,
skip_special_tokens, priority conversion, andtop_k = -1normalization. LoRA operations remain explicitly unsupported by the mock server.Native Generate compatibility
Main's #14398 added native sampling JSON fields from the still-open companion vLLM PR #56421. The published protocol crate does not contain those fields. This change deliberately uses the released upstream protocol: the sidecar does not advertise
vllm_inference_v1_generate, and aggregated/decode requests carryingextra_args.vllm_tito.sampling_paramsreturn an explicit unsupported-request error instead of silently dropping settings. Chat/completions retain supported typed controls. Prefill/encode retain their canonical one-token behavior.The frontend request types and projection code remain available. Native JSON passthrough can be enabled after an upstream protocol release supplies the payload and capability flag; no local schema fork or panic stub is retained.
Validation
cargo test -p dynamo-sidecar-common -p dynamo-vllm-sidecar -p dynamo-vllm-mocker --all-targets --locked: 67 passed.--all-targets --locked -- -D warningsfor common, vLLM sidecar/mocker, SGLang sidecar, and TensorRT-LLM sidecar: passed.cargo check --manifest-path lib/bindings/python/Cargo.toml --locked: passed.cargo fmt --all -- --check, pre-commit on changed files, and CI filter tests: passed (71 filter tests).Local Rust checks use the host's system
protocwith--experimental_allow_proto3_optional. No new GPU smoke run was performed after this rebase; the original PR's pre-rebase live validation is recorded in its earlier description.Related issues