Repository navigation
test(sidecar): cover DEP rank-aware KV routing - #15086
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their 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 (6)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughChangesSidecar KV-routing test coverage
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR adds the intended DEP sidecar coverage and keeps it in the appropriate PR-sidecar jobs; no merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (3 skipped: 3 unsupported.)
Comment |
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approving 90bfc255ec. The DEP cases test rank-aware routing, and the new two-GPU CI legs run them with the sidecar binary they need.
Findings:
- [P3] The SGLang kernel override needs an in-code removal note (inline).
Open and deferred:
- The existing thread on post-merge and nightly coverage describes a gap that predates this PR. I replied there with measurements.
- I did not run the DEP cases myself, because my GPU host has one GPU.
mainnow has #14673, which changes the block size that the vLLM sidecar registers. No CI run tested the two together yet.
What I verified, with evidence.
- This head's PR run 35763241932 passed
test_sidecar_kv_routing[vllm-dep-tcp](job 106895995772) andtest_sidecar_kv_routing[sglang-dep-tcp](job 106895995901). Each job selected one test on a two-GPU runner, with no reruns. The replica,test_serve_deployment, and TRT-LLM sidecar cases also passed. - If routing ignores the cache, the rank checks in the helper fail. I ran this head's
_test_frontend_kv_routing(dp_ranks=(0, 1))on CPU against one mocker worker with two DP ranks that publishes ZMQ KV events. The runtime was the CI image of5e21f9c618with this head'stests/copied in.
| Case | Result |
|---|---|
| DP=2 worker, stock KV router | passed, 2 of 2 runs |
| DP=1 worker, helper pins rank 1 | failed at the rank-1 pin with HTTP 500 |
DP=2 worker, DYN_ROUTER_KV_OVERLAP_SCORE_CREDIT=0 |
failed with KV events did not converge |
DP=2 worker, DYN_ROUTER_TEMPERATURE=1000 |
failed 3 of 3 runs, twice at common.py:686 and once at common.py:690 |
- In the passing runs, the new unpinned queries chose rank 0 for the first prompt and rank 1 for the second. The six generations chose ranks 0, 0, 1, 0, 1, 1.
- actionlint 1.7.12 reports the same 57 errors on the base and the head of the three edited workflows. It catches a typo that I planted in the new matrix expressions.
- Every vLLM and SGLang
gpu_2lane in PR, post-merge, and nightly CI now excludessidecar._test_frontend_kv_routinghas one caller,test_sidecar_kv_routing. - This head merges cleanly with the parent tip
6e9eae39dband withmainat9ae086bb94. The parent's new commit only rejects a non-arrayextra_fields, and this helper sends an array. - SGLang
v0.5.19definesSGLANG_OPT_USE_JIT_KERNEL_GROUPED_TOPK, and its ports derived from--dist-init-addrfit inside the test's block of 12 ports.
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
I approve f0c84b4c9a. The new post-merge and nightly legs select the two DEP routing cases, and the last commit adds only a comment.
Findings: none new. My earlier P3 at tests/serve/test_sidecar.py:308, the removal note for the SGLang override, is fixed. I replied on its thread. The post-merge and nightly gap from the thread on .github/workflows/pr.yaml:380 is closed, as the table below shows.
Open and deferred:
- No workflow ran the new legs yet, because PR CI does not include them. The jobs still to run are
sidecar-runtime (2) / vLLM Sidecar 2-GPU E2E Testandsidecar-runtime (2) / SGLang Sidecar 2-GPU E2E Test, in post-merge and in nightly. - This branch still starts from the
mainof 2026-09-18.mainnow has #14673 (vLLM sidecar KV block sizes) and #14785 (router worker selection). No CI run of this PR tested the DEP cases with them. If the branch does not take inmainfirst, the new post-merge leg is the first run that tests them together.
What I measured, with controls.
The merge 1d84b61343 resolves no conflict. git merge-tree of its two parents gives a clean tree, so all 28 added and 12 removed lines in post-merge-ci.yml and nightly-ci.yml are new edits. I reviewed them as new code.
I ran pytest --collect-only -m <expression> on tests/serve/test_sidecar.py for every vLLM and SGLang GPU lane in the three workflows. I used the vLLM and SGLang runtime-test images of 5e21f9c618, with no network and each tree copied in. "Before" is 90bfc255ec merged with the base tip.
| Lane | Before | This head |
|---|---|---|
PR, gpu_2 leg |
DEP case | DEP case |
Post-merge, gpu_2 leg |
no leg | DEP case |
Nightly, gpu_2 leg |
no leg | DEP case |
Post-merge and nightly, gpu_1 leg |
test_serve_deployment and the replicas case |
the same 2 cases |
Every lane with not sidecar |
0 cases | 0 cases |
The controls gave their known answers. With no -m, this head collects 7 cases. The expression sidecar and trtllm and gpu_2 selects 0. On the base tip, which has no DEP case, every gpu_2 expression selects 0.
- actionlint 1.7.12 reports the same 99 errors in
.github/on the base tip, on90bfc255ec, and on this head. It flags both defects that I planted in the new matrix lines. - The new legs pass the sidecar binary artifact and use
prod-tester-amd-gpu-2-v2, like the other two-GPU lanes. Like those lanes, they do not readRUN_MULTIGPU_TESTS. Onlynotify-slackneeds them.backend-status-check,dynamo-status-check, anddeploy-status-checkhave the sameneedson the base tip and on this head. - This head's PR run 36466006757 passed
test_sidecar_kv_routing[vllm-dep-tcp](job 109090200404) andtest_sidecar_kv_routing[sglang-dep-tcp](job 109090200785) on two-GPU L40S runners, with one selected test in each job. Both one-GPU sidecar legs passed too. f0c84b4c9changes no code. The Python AST oftests/serve/test_sidecar.pyis the same before and after it.- The red
deploy-status-checkdoes not come from this diff. Invllm Deploy Test / agg(job 109082441525), the node had theDiskPressurecondition and rejected the frontend pod. That test deploysexamples/backends/vllm/deploy/agg.yaml, which this PR does not touch.
f0c84b4 to
b88e823
Compare
b88e823 to
dc5f5eb
Compare
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
I approve dc5f5ebfd8. The rebase onto #15082 keeps every line of this PR. Each DEP case still runs in one two-GPU job in PR, post-merge, and nightly CI.
Findings:
- [P3] The description still says "Stacked directly on #15081". #15081 is merged, and the base of this PR is now
jdarve/sidecar-disagg-e2e, the branch of #15082. Please update that line.
Open:
- No CI run tested the DEP cases on this head yet. The rebase brings in #15182, which moves the vLLM image to
v0.30.0, and the sidecar and KV router changes ofmain. PR run 36500843926 runs both cases in its twosidecar-runtime (2) / Sidecar 2-GPU E2E Test cuda13.0, amd64jobs. At 00:55 UTC, that run was still building the images for those jobs. - The post-merge and nightly workflows run on
main. Their new legs run by themselves only after this stack merges there.
What I measured, with controls.
I compared the added and removed lines of each of the 6 files. They are the same at f0c84b4c9a against its old base and at this head against the new base. Between the two bases, only tests/serve/test_sidecar.py changed in these 6 files. That change is the disaggregated cases of #15082.
I ran pytest --collect-only -m <expression> on tests/serve/test_sidecar.py for all 63 lane expressions of pr.yaml, post-merge-ci.yml, and nightly-ci.yml. I used the vLLM and SGLang runtime-test images of 5e21f9c618, with no network. In each image, I replaced the 6 paths that the sidecar jobs replace with the files of the tree under test. For this head, both images give the same result. Each DEP case of test_sidecar_kv_routing runs in these legs:
| Workflow | Job, matrix leg | Runner | Expression | Case |
|---|---|---|---|---|
| PR | sidecar-vllm-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
pre_merge and sidecar and vllm and gpu_2 |
[vllm-dep-tcp] |
| PR | sidecar-sglang-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
pre_merge and sidecar and sglang and gpu_2 |
[sglang-dep-tcp] |
| Post-merge | sidecar-vllm-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
(pre_merge or post_merge) and sidecar and vllm and gpu_2 |
[vllm-dep-tcp] |
| Post-merge | sidecar-sglang-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
(pre_merge or post_merge) and sidecar and sglang and gpu_2 |
[sglang-dep-tcp] |
| Nightly | sidecar-vllm-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
sidecar and vllm and gpu_2 |
[vllm-dep-tcp] |
| Nightly | sidecar-sglang-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
sidecar and sglang and gpu_2 |
[sglang-dep-tcp] |
- Each leg in the table selects only its DEP case. No other expression selects a DEP case.
- If
RUN_MULTIGPU_TESTSis nottrue, the PR has no leg withgpu_count2. The changed-files job 109192399329 of this head's PR run logsRUN_MULTIGPU_TESTS: true. - On
prod-tester-amd-gpu-2-v2, the job gets two GPUs. The vLLM DEP job 109090200404 off0c84b4c9alogsGPU count: 2andGPU Name: NVIDIA L40S. - Controls: with no
-m, this head collects 9 cases, 7 on one GPU and 2 on two GPUs.sidecar and trtllm and gpu_2selects no case. In the SGLang image, onb88e823f99, the first push after #15081 merged, no post-merge or nightly expression selects a DEP case. The probe finds that gap. - actionlint 1.7.12 reports the same 99 errors on
main, on the base tip, onf0c84b4c9a, and on this head. None of them is a duplicate key. It flags the 3 defects that I planted: two unknownmatrixproperties in the new legs and a duplicate job key. - This head contains the
maintipf6732746a8.git merge-treeof this head withmainreturns the tree of this head, with no conflict.
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
dc5f5eb to
f4c2035
Compare
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
I approve this PR at f4c2035f17. The rebase onto main keeps every line of the change that I approved at dc5f5ebfd8. Both DEP cases passed in the PR run of this head. Each DEP case runs in one two-GPU job in PR, post-merge, and nightly CI.
Findings:
- [P3] The description still says "Stacked directly on #15081". #15081 and #15082 are merged, and the base of this PR is now
main. The Validation section also says "Collected seven sidecar cases", but this head collects 9: 7 on one GPU and 2 on two GPUs. Please update these two lines.
Open:
- The post-merge and nightly workflows run on
main. Their newgpu_count2 legs ofsidecar-vllm-testandsidecar-sglang-testrun only after this PR merges. - When I posted, the SGLang
sidecar-runtime (1)leg, job 109646569226, was still starting its containers. That leg runs[sglang-replicas-tcp]through the changed routing scenario. - Note: the first commit,
73089756f0, is empty. Its tree is the tree of7f7b533d9c, the #15082 commit onmain. The repository squashes with the PR title and a blank body, so a squash merge does not add #15082 a second time.
What I measured, with controls.
- I compared the added and removed lines of the 6 files at this head and at
dc5f5ebfd8. Each list is the same, byte for byte and in order. Two planted changes gave 2 differences. The old base isc5954ef49e, and the new base is7f7b533d9c. Between them, onlypr.yamlandtests/router/common.pychanged in these 6 files, outside the hunks of this PR. #15063 made both changes. - At
2d5b116a23,mainis two commits ahead of7f7b533d9c. These commits change 4 operator files and 2 image-publishing files under.github.git merge-treeof this head with2d5b116a23gives no conflict. The merge keeps the workflows, the shared test workflow, and the test paths that I collected from. - In PR run 36634189962, jobs 109646569445 and 109646569320 are the two
sidecar-runtime (2) / Sidecar 2-GPU E2E Test cuda13.0, amd64jobs. Each checked outf4c2035f17, loggedGPU count: 2, and ran pytest once. Each selected 1 test, and[vllm-dep-tcp]and[sglang-dep-tcp]passed. - In the same run, job 109646569622 is the vLLM
sidecar-runtime (1)leg. It selected 3 cases:[vllm_aggregated-2], the #15082 case[vllm_disaggregated-2], and[vllm-replicas-tcp]. All 3 passed. - In the same run, jobs 109636877068 and 109636184853 are the vLLM and SGLang framework jobs
2-GPU Test cuda13.0, amd64. They ranpre_merge and vllm and gpu_2 and not sidecarandpre_merge and sglang and gpu_2 and not sidecaronce each. Neither job selected atest_sidecar.pycase.
I ran pytest --collect-only -m <expression> on tests/serve/test_sidecar.py for every lane expression of pr.yaml, post-merge-ci.yml, nightly-ci.yml, and pr-xpu.yaml. I used the vLLM and SGLang runtime-test images of 5e21f9c618, with no network. In each image, I replaced the 6 paths that the sidecar jobs replace with the files of the tree under test. The trees are this head, main at b4909a6413, and b88e823f99 as a control. For a job with a parallel GPU stage, I also collected each stage. Both images give the same result. Each DEP case of test_sidecar_kv_routing runs in these legs:
| Workflow | Job, matrix leg | Runner | Expression | Case |
|---|---|---|---|---|
| PR | sidecar-vllm-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
pre_merge and sidecar and vllm and gpu_2 |
[vllm-dep-tcp] |
| PR | sidecar-sglang-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
pre_merge and sidecar and sglang and gpu_2 |
[sglang-dep-tcp] |
| Post-merge | sidecar-vllm-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
(pre_merge or post_merge) and sidecar and vllm and gpu_2 |
[vllm-dep-tcp] |
| Post-merge | sidecar-sglang-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
(pre_merge or post_merge) and sidecar and sglang and gpu_2 |
[sglang-dep-tcp] |
| Nightly | sidecar-vllm-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
sidecar and vllm and gpu_2 |
[vllm-dep-tcp] |
| Nightly | sidecar-sglang-test, gpu_count 2 |
prod-tester-amd-gpu-2-v2 |
sidecar and sglang and gpu_2 |
[sglang-dep-tcp] |
- Each leg in the table selects only its DEP case. These jobs have no parallel GPU stage, so each leg runs pytest once. No other expression, and no stage of a framework job, selects a DEP case.
- The #15082 cases
test_serve_deployment[vllm_disaggregated-2]and[sglang_disaggregated-2]run once per workflow, in thegpu_count1 leg of the sidecar jobs. Onmain, they run once per workflow in the same sidecar jobs. - Controls: with no
-m, this head collects 9 cases, andmaincollects 7 with no DEP case.sidecar and gpu_2selects the 2 DEP cases, andsidecar and trtllm and gpu_2selects none. Onb88e823f99, no post-merge or nightly expression selects a DEP case, so the probe finds that gap. - These images export
AicPerfConfig, but this head andmainimportAisPerfConfigafter #15063. Without help, collection stops with anImportError. A pytest plugin gave the new name to the old class, and no tree file changed. Marker selection does not use this class. Onb88e823f99, which importsAicPerfConfig, the plugin changes no collected case.
Summary
The sidecar routing tests cover separate workers but do not exercise cached-prefix selection between data-parallel ranks in one engine group. Add a data/expert-parallel (DEP) case for vLLM and SGLang using
silence09/DeepSeek-R1-Small-2layers, two GPUs, and one sidecar.Warm different prefixes on ranks 0 and 1, then require unpinned routing queries and generation requests to select the matching
(worker_id, dp_rank)pair. Check accepted KV events and router-estimated cache overlap using the shared routing scenario. Keep replica coverage on one GPU and run DEP coverage in dedicated two-GPU CI jobs.Expose
VLLM_DATA_PARALLEL_SIZEin the aggregated launcher so the native Rust frontend and Python engine agree on DP size; the default stays 1. TensorRT-LLM DEP coverage is excluded because its native-sidecar API does not yet expose the required DP-rank routing contract.The SGLang DEP test temporarily sets
SGLANG_OPT_USE_JIT_KERNEL_GROUPED_TOPK=1to use the Triton fallback on SM86 GPUs, where the selected FlashInfer kernel is unsupported. Remove this test-only override once Dynamo's pinned SGLang includes the upstream capability-check fix and two-GPU CI passes without it.Stacked directly on #15081; disaggregated-serving coverage remains separately in #15082.
Validation
90bfc255ec) passed both vLLM and SGLang one-GPU sidecar jobs, the TRT-LLM sidecar job, and the SGLang two-GPU DEP test.Related Issues
Summary by CodeRabbit
New Features
VLLM_DATA_PARALLEL_SIZE.Tests