Repository navigation
test(sidecar): cover native KV-aware routing in GPU CI - #15081
Conversation
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe change adds end-to-end KV-event routing tests for vLLM and SGLang sidecars. It updates request metadata filtering, launcher script paths, GPU test matrices, CI filters, and marker expressions. ChangesSidecar KV routing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The sidecar routing coverage and its CI selection have no remaining actionable issue from the reviewed change. The PR is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 7 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
|
🤖 Completed: Fix pre-merge checks in PR #15081 — View commit |
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Reviewed at head 49bd4fe. The CI selection does what the PR says it does, and the Rust regression test pins the fix. Two non-blocking findings are inline, one P2 and one P3.
Scope: 11 files from pulls/15081/files and 11 from a local three-dot diff off the merge base b078517. The two counts agree. Three commits, linear, no force-push, and the last one is the CodeRabbit docstring commit, which changes no behavior.
What I ran, and what came back clean
Marker selection, measured with pytest --collect-only on this head:
| expression | collected |
|---|---|
pre_merge and sidecar and vllm and gpu_2 |
test_sidecar_kv_routing[vllm-tcp-2] |
pre_merge and sidecar and sglang and gpu_2 |
test_sidecar_kv_routing[sglang-tcp-2] |
pre_merge and sidecar and vllm and gpu_1 |
test_serve_deployment[vllm_aggregated-2] only |
pre_merge and sidecar and vllm and gpu_9 |
nothing, 5 deselected |
pre_merge and vllm and gpu_2 |
selects the new test |
pre_merge and vllm and gpu_2 and not sidecar |
nothing |
The last two rows show the not sidecar additions carry weight. I then ran every literal gpu_test_markers expression in pr.yaml, post-merge-ci.yml and nightly-ci.yml against the new test, 31 expressions in all. Only the two templated pr.yaml lanes at gpu_count=2 select it, and those are the only lanes that pass sidecar_binary_artifact. A first version of that probe was wrong, because the quoted YAML values made pytest fail and print every collected ID. I discarded it and added a self-check: a bogus expression must select nothing and sidecar and gpu_2 must select two.
The new Rust test pins the convert.rs change. On the GPU box, crate-scoped:
| arm | convert.rs |
subject test | control test |
|---|---|---|---|
| head | 2a59a7b7 |
ok |
ok |
| fix hunk removed | e21c1b53 |
FAILED at tests.rs:1060 |
ok |
| restored | 2a59a7b7 |
Also measured and clean: tests/serve/test_sidecar.py imports and collects inside the vLLM runtime-test image on both trees, so the heavier import chain is satisfiable there. The 1 GPU lane still collects the same pre-existing case, and the 2 GPU lane collects nothing on the base tree. Qwen/Qwen3-0.6B resolves at the Hub with HTTP 200, and a bogus tag returns 401. The two prompts tokenize to 674 and 578 tokens, above the block_size * 4 floor of 256 and below MAX_MODEL_LEN=2048. prod-tester-amd-gpu-2-v2 is already used by 11 other jobs. RUN_MULTIGPU_TESTS is on, because vllm-runtime / 2-GPU Test cuda13.0, amd64 ran green on #15080. Artifact names carry test_type, which now differs per matrix leg, so the two legs do not collide.
I merged the current base tip 90a9ab3 myself with git merge-tree --write-tree. It merges clean, and all 11 files are byte-identical between the head and the merged tree, so neither finding depends on base drift.
Three hypotheses I retracted. DYN_SYSTEM_PORT2 does not fall back to the hardcoded 8082, because tests/serve/common.py:216-219 injects DYN_SYSTEM_PORT1..N from the port fixture. The launcher change from $DYNAMO_HOME to $SCRIPT_DIR is not a regression. agg.sh already uses that form and explains why, and no sourced file reads DYNAMO_HOME. The heavier import chain does not break collection in the runtime image.
Where verification stopped. I did not run the end-to-end test itself. That needs two GPUs and the sidecar binaries, and this box has one RTX 6000 Ada. So the routing assertions, the hit_rate >= 0.5 floor, the Stored-event convergence and the real phase durations are unverified.
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approving at head 49bd4fe. The CI selection does what the PR claims. The Rust regression test fails on the pre-fix code, so it pins the convert.rs change rather than passing both ways.
Two non-blocking items stay open. The P2 on tests/serve/test_sidecar.py:165 is the 780 second mark, which is now smaller than the sum of the waits the test delegates to. The P3 on .github/filters.yaml:357 is the two modules that tests/router/common.py imports and the sidecar filter does not list. Neither blocks the merge. Both are cheap to fix here, and the P3 gets harder to notice later, because the sidecar lanes run only in pr.yaml.
Deferred on purpose: I did not run the end-to-end test. It needs two GPUs and both sidecar binaries, and my box has one RTX 6000 Ada. The routing assertions, the hit_rate >= 0.5 floor, the Stored-event convergence and the real phase durations are therefore unverified by me. The two GPU lane has not reported on this PR yet either, so that run is still the first real evidence.
I wrote no commits on this PR.
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
jthomson04
left a comment
jthomson04
left a comment
There was a problem hiding this comment.
Source review of d3b5991. One P2 test issue and two P3 style notes. No tests run or CI results inspected.
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Round 2, on 5ef322c11f
Re-approving. Our earlier approval named 49bd4feaf4. The tree then moved forward by two commits, both written by the author, and I read it again from the start. One P3 is left, on the error text for a wrong-typed extra_fields.
I re-ran the measurement behind each of the three points we raised, and the three that jthomson04 raised. All six hold.
Push shape: a linear append, proven by ancestry in both directions
The timeline has no head_ref_force_pushed event. Ancestry agrees:
git merge-base --is-ancestor 49bd4feaf4 5ef322c11f -> 0
git merge-base --is-ancestor 5ef322c11f 49bd4feaf4 -> 1
Timestamps agree as well. 49bd4feaf4 was committed at 2026-09-21T20:07:50Z and our approval followed at 2026-09-21T20:36:27Z. 5ef322c11f was committed at 2026-09-22T17:31:36Z, which is after that approval. No approval predates the commit it names, so this is genuine forward motion and not a re-pointed review.
The delta is two commits, d3b599128f and 5ef322c11f. Both are authored and committed by the author, and git rev-list --merges over that range returns 0. No commit arrived from main.
Both file-count instruments agree: a local three-dot diff against the merge base b0785175ee and the paginated files API both report 8 files, 346 insertions, 10 deletions.
The seven new filter lines cover every dependency this pull request adds
I walked the module-level import graph from tests/serve/test_sidecar.py on both trees with an AST reader, and took the difference.
The base tree reaches 15 modules under tests/. The head tree reaches 20. The five new ones are:
| new dependency | in the sidecar filter |
|---|---|
tests/router/common.py |
yes |
tests/router/helper.py |
yes |
tests/router/router_process.py |
yes |
tests/utils/gpu_args.py |
yes |
tests/utils/router_logs.py |
yes |
All five are covered. The other two added lines, tests/utils/prometheus.py and tests/utils/router_nvext.py, were already dependencies on the base tree, so they add coverage this pull request did not need.
Other modules in the closure stay uncovered, for example tests/serve/common.py and tests/utils/managed_process.py. Every one of them was already a dependency on the base tree, so that gap is older than this change. I am not raising it here.
Marker routing: both new test items reach a lane that installs the sidecar binary
The jobs in .github/workflows/pr.yaml select with these expressions:
| job | marker expression |
|---|---|
sidecar-vllm-test |
pre_merge and sidecar and vllm and gpu_1 |
sidecar-sglang-test |
pre_merge and sidecar and sglang and gpu_1 |
sidecar-trtllm-test |
pre_merge and sidecar and trtllm and gpu_1 |
plain vllm 1-GPU lane |
pre_merge and vllm and gpu_1 and not sidecar |
plain sglang 1-GPU lane |
pre_merge and sglang and gpu_1 and not sidecar |
I collected with each of them inside the vllm-runtime-test image on an x86_64 box, against both trees:
| tree | expression | collected |
|---|---|---|
base b0785175ee |
plain vllm 1-GPU lane |
no tests collected (3 deselected) |
base b0785175ee |
sidecar-vllm-test |
1/3 |
head 5ef322c11f |
sidecar-vllm-test |
2/5, including test_sidecar_kv_routing[vllm-tcp-2] |
head 5ef322c11f |
sidecar-sglang-test |
2/5, including test_sidecar_kv_routing[sglang-tcp-2] |
head 5ef322c11f |
sidecar-trtllm-test |
1/5 |
head 5ef322c11f |
plain vllm 1-GPU lane |
no tests collected (5 deselected) |
head 5ef322c11f |
plain sglang 1-GPU lane |
no tests collected (5 deselected) |
head 5ef322c11f |
gpu_2 |
no tests collected (5 deselected) |
Declared: 2 new test items. Selected: 1 in the vLLM sidecar lane and 1 in the SGLang sidecar lane, so 2 of 2. Nothing leaks into the plain 1-GPU lanes, because and not sidecar excludes it. Nothing carries gpu_2, so no item is stranded in a lane that does not exist.
The base run is the control. It collects cleanly in the same image, which rules out an import fault in the image.
The 27 new Rust test lines pin the production line in three directions
I ran cargo test -p dynamo-vllm-sidecar --lib on the head, then mutated lib/sidecar/vllm/src/convert.rs three ways. tests::skip_special_tokens_is_forwarded_without_compatibility_envelope is the control in every arm.
| arm | convert.rs change |
subject | control |
|---|---|---|---|
| 1 | none | pass | pass |
| 2 | the new block deleted | FAIL | pass |
| 3 | allowlist narrowed to Some("worker_id") |
FAIL | pass |
| 4 | retain(...) replaced by fields.clear() |
FAIL | pass |
Arm 1 gives a clean baseline. Arm 2 shows the test pins the block. Arm 3 shows it pins timing by name, not only worker_id. Arm 4 shows it pins the rejection of engine_data, so a widened allowlist cannot pass unnoticed.
File checksum at the start and after restore: 2a59a7b7f369948818479d0822522f01d2e11328eb6a3fc796e03beea37f660e, and git status --porcelain printed 0 lines.
Base drift: the merge is clean and the tests still pass and still pin
Main has moved 53 commits since the merge base. It also touched files this pull request changes: lib/sidecar/vllm/src/convert.rs by 424 insertions and 33 deletions, and lib/sidecar/vllm/src/tests.rs by 606 insertions and 12 deletions.
I merged the main tip ff3ac59e83c73e03b98a5d0ec192ec28847130f7 into the head in a scratch worktree. The merge is clean, with 0 conflicted files. Both the new convert.rs block and tests::frontend_router_metadata_does_not_require_engine_support survive it.
On the merged tree the whole crate suite reports 80 passed; 0 failed. Deleting the new block there still turns the subject test red, so the guard survives the drift.
The two launch scripts stayed in step, and no consumer of DYNAMO_HOME is left behind
lib/sidecar/vllm/launch/agg_kv_router.sh and lib/sidecar/sglang/launch/agg_kv_router.sh take the same three changes each: the two-GPU sentence leaves the header comment, export DYNAMO_HOME gives way to a path relative to SCRIPT_DIR, and the banner says (2 workers) instead of (2 GPUs). Neither file gained a change the other missed.
The removal is safe and it matches the pattern already used by agg.sh in all three backends, which carries this comment:
# Resolved relative to this script, not via $DYNAMO_HOME: some runtime images
# (e.g. vllm_runtime.Dockerfile) bake DYNAMO_HOME to a minimal install path
DYNAMO_HOME appears 0 times in examples/common/gpu_utils.sh, 0 times in examples/common/launch_utils.sh, and 0 times in the rest of either changed script, so nothing reads the variable these two files stopped exporting.
The new shared helpers do not change the contract for callers outside this diff
12 files outside this diff import tests/router/helper.py and 5 import tests/router/common.py. Both files change by additions only, 46 and 139 lines, with 0 deletions, so no existing signature moved. generate_random_suffix is unchanged.
The one way a purely additive change can still break a caller is a new module-level import. Both new ones resolve on this tree: sum_metric_samples at tests/utils/prometheus.py:63, and kv_publisher.ZMQ_EVENTS_TOTAL and name_prefix.COMPONENT in lib/bindings/python/src/dynamo/prometheus_names.py. tests/utils/router_nvext.py was already present on the base tree.
What ran on this head, and where my checking stopped
Checks split across two branches, so I read both. pull-request/15081 still points at 24358776c6, which is two pushes behind. The runs attached to 5ef322c11f are on jdarve/sidecar-kv-routing-e2e and on refs/pull/15081/head, and they are the light ones: Pre Merge, codeowners, Copyright Checks, Lint PR, DCO Commenter, Docs link check, Label PR.
The sidecar GPU lanes have therefore not run on the head I reviewed. The pull request description says the same thing. I proved the two new items are selected by the right lane, and I could not prove they pass on real hardware, because the lane has not started. That boundary is the main limit on this round.
I also did not measure peak VRAM for two engines sharing one device. Keeping the tests in the sequential stage until that number exists is the right call.
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
|
/ok to test 6e9eae3 |
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approving at 6e9eae39db. The new commit closes our P3: every non-array extra_fields now gets its own error, and no accept or reject decision changed. No finding from us is open. I did not run test_sidecar_kv_routing, and its sidecar CI lanes had not started when I posted.
What I verified on this head and on a merge with main
- The probe that first showed the P3 gives
extra_args.nvext.extra_fields must be an arrayon this head for a string, an object,null, a number and a boolean. With theconvert.rsof5ef322c11f, the same inputs giveextra_args.nvext is not supported by vLLM gRPC. The full table is on theconvert.rsthread. frontend_router_metadata_rejects_non_array_fieldsfails on the oldconvert.rsand passes on the new one.cargo test --locked -p dynamo-vllm-sidecarpasses on this head, with 65 unit tests and 1 executable test.- I merged main
a83ba19b4einto this head, and the merge is clean. The crate passes there with 81 unit tests and 1 executable test. When I reverse the fix hunk on that merge, the new test fails.
I wrote no commits on this PR.
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approving at 50d24a0fed. The new commit runs the vLLM and SGLang sidecar tests in post-merge and nightly CI, and I found no defect in it.
These new jobs have not run yet: sidecar-vllm-test and sidecar-sglang-test in post-merge-ci.yml, and sidecar-build, sidecar-vllm-test and sidecar-sglang-test in nightly-ci.yml. Neither workflow runs on a pull request, so my test of these jobs stops at lint and test selection.
The routing test stays in pre-merge, and the new lanes select it too.
The commit changes only .github/workflows/post-merge-ci.yml and .github/workflows/nightly-ci.yml. It does not change pr.yaml, .github/filters.yaml or tests/serve/test_sidecar.py, so the pre-merge sidecar lanes select the same tests as on 6e9eae39db. There, run 35907114978 passed test_sidecar_kv_routing[vllm-tcp-2] and test_sidecar_kv_routing[sglang-tcp-2].
I collected tests/serve/test_sidecar.py with each marker expression. I used the vLLM and SGLang runtime-test images of 5e21f9c618 for this head, and the vLLM image for a merge with main 13cbb76b50. All runs gave this result:
| lane | marker expression | selected |
|---|---|---|
| post-merge vLLM | (pre_merge or post_merge) and sidecar and vllm and gpu_1 |
test_serve_deployment[vllm_aggregated-2], test_sidecar_kv_routing[vllm-tcp-2] |
| nightly vLLM | sidecar and vllm and gpu_1 |
the same 2 tests |
| post-merge SGLang | (pre_merge or post_merge) and sidecar and sglang and gpu_1 |
test_serve_deployment[sglang_aggregated-2], test_sidecar_kv_routing[sglang-tcp-2] |
| nightly SGLang | sidecar and sglang and gpu_1 |
the same 2 tests |
| plain 1-GPU lanes | 4 expressions that end in and not sidecar |
0 of 5 |
On the merge base, each sidecar expression selects only the aggregated test, 1 of 3. The control expression sidecar and not sidecar selects 0 of 5.
actionlint finds no new error, and it catches planted defects.
I ran actionlint 1.7.12 on copies with $/ changed to ./. This head gives the same 99 findings as the merge base, and a merge with main gives the same 99 as main. None of them is on a line that this commit adds or changes.
As a control, I planted five defects in the new jobs. actionlint reported all five:
- An unknown input.
- A missing required input.
- An unknown output of
sidecar-build. - An unknown output of
config. - A
needsentry for a job that does not exist.
No required check depends on the new jobs.
The commit does not change the triggers. Post-merge runs on a push to main or release/*.*.* and on a manual run. Nightly runs on its schedule and on a manual run. The required checks come from pr.yaml and pre-merge.yml, and this commit does not change them. A needs entry cannot name a job in another workflow. The post-merge job deploy-status-check has the name of a required check, but its needs did not change, and it never runs on a pull request.
| event | before | after |
|---|---|---|
| post-merge push | sidecar-build only |
sidecar-build, then each test after its backend build |
| post-merge manual run | no sidecar job | sidecar-build for all, vllm or sglang, and the test of each selected backend |
nightly with run_tests true |
no sidecar job | sidecar-build and both tests |
notify-slack.yml lists every failed job of the run. The new needs entries make it wait for the sidecar jobs, so a sidecar failure reaches the alert. In nightly, clean-k8s-builder now waits for sidecar-build, so the builder stays up until that build ends.
The earlier Rust fix still passes on a merge with the current main.
Main changed lib/sidecar/vllm/src/tests.rs after a83ba19b4e, the main commit that I merged in my last round. Pre Merge run 36463793853 tested this head merged with main 13cbb76b50. There, dynamo_vllm_sidecar passed 84 unit tests with 0 failures, and both frontend_router_metadata_* tests passed.
I wrote no commits on this PR.
|
/ok to test 50d24a0 |
4 participants
Summary
Add native-sidecar KV-routing E2E tests for vLLM and SGLang using Qwen3-0.6B. Each test runs two independent workers on one allocated GPU, warms a different prefix on each, requires Stored KV-cache events to reach both sidecars, and verifies that subsequent unpinned requests select the cached worker with a KV hit rate of at least 50%.
Use the existing one-GPU pre-merge sidecar jobs and binary installation from #14508. Keep the tests in the sequential stage until their peak VRAM is formally profiled, with a 1,200-second test timeout. Extend the sidecar CI filter to cover the shared routing helpers and their dependencies.
Fix launcher helper paths and allow frontend-owned worker/timing response metadata through the vLLM sidecar, with a focused regression test. TensorRT-LLM KV routing remains outside scope until its event support exists.
Validation
Related Issues
Relates to #14508.
Linear: https://linear.app/nvidia/issue/DIS-2949
Summary by CodeRabbit
New Features
Bug Fixes
Tests