Repository navigation
ci(sidecar): run GPU E2E tests only post-merge and nightly - #15456
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Repository guideline files applied to this review (1)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 (4)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughSidecar test configurations now use post-merge and nightly markers. Nightly and post-merge workflows select sidecar tests with updated marker expressions. Pull request CI removes dedicated vLLM, SGLang, and TensorRT-LLM sidecar test jobs. ChangesSidecar CI scheduling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change moves sidecar GPU E2E tests to post-merge and nightly CI, and no concrete merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the change and validation, but it does not follow the required template. It omits the Overview, Details, and reviewer-start sections. The Related Issues section is also contradictory because it references issues while confirming that no related issue exists.
Comment |
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approve at 8391725a550833d889ce5aef627b8d4a2a0b1f2b. No P0, P1 or P2 is open. The two P3 items below do not block the merge.
I ran the head and main at 1a26bf9c side by side:
- Each of the 10 sidecar cases runs in exactly one job of
post-merge-ci.ymland in one job ofnightly-ci.yml. No pre-merge job selects a case, and no case lost its last job. backend-status-checkdrops only the three removed jobs, and noneeds:entry names a missing job. If a kept job fails or is cancelled, the check still fails.- actionlint 1.7.12 gives the same 57 messages at
mainand at the head.
[P3] The 10 tests/ lines in the sidecar filter still start the sidecar image build, and no pre-merge test consumes its output now. .github/filters.yaml:357-366. Please remove these lines. The core filter already covers tests/**.
[P3] The post-merge builds still push the three main-*-runtime-test tags, and no file in the repository reads them now. .github/workflows/post-merge-ci.yml:220, :287 and :354. Please remove the tags, or add a comment that names who still reads them.
Measured, with controls.
Collection: I ran pytest 9.0.3 --collect-only -m <expression> in the vLLM -runtime-test image. Each run used one stage expression of pr.yaml, pr-xpu.yaml, post-merge-ci.yml or nightly-ci.yml. The tree under test replaced the tests/, conftest.py and pyproject.toml of the image.
| case | pre-merge jobs at main |
pre-merge jobs at head | post-merge and nightly job at head |
|---|---|---|---|
test_serve_deployment[vllm_aggregated-2] |
1 | 0 | sidecar-vllm-test, 1 GPU |
test_serve_deployment[vllm_disaggregated-2] |
1 | 0 | sidecar-vllm-test, 1 GPU |
test_sidecar_kv_routing[vllm-replicas-tcp] |
1 | 0 | sidecar-vllm-test, 1 GPU |
test_sidecar_kv_routing[vllm-dep-tcp] |
1 | 0 | sidecar-vllm-test, 2 GPUs |
test_serve_deployment[sglang_aggregated-2] |
1 | 0 | sidecar-sglang-test, 1 GPU |
test_serve_deployment[sglang_disaggregated-2] |
1 | 0 | sidecar-sglang-test, 1 GPU |
test_sidecar_kv_routing[sglang-replicas-tcp] |
1 | 0 | sidecar-sglang-test, 1 GPU |
test_sidecar_kv_routing[sglang-dep-tcp] |
1 | 0 | sidecar-sglang-test, 2 GPUs |
test_serve_deployment[trtllm_aggregated-2] |
1 | 0 | sidecar-trtllm-test, 1 GPU |
test_serve_deployment[trtllm_disaggregated-2] |
1 | 0 | sidecar-trtllm-test, 1 GPU |
The post-merge and nightly mappings are the same at main and at the head.
Controls for the collection:
sidecarselects 10 cases at both trees, all intests/serve/test_sidecar.py.sidecar and trtllm and gpu_2selects 0.- For each workflow,
sidecarjoined with the union of all other stages selects 0 at both trees. - I removed
and not sidecarfromvllm-multi-gpu-testin that union. Atmain, the union then selectstest_sidecar_kv_routing[vllm-dep-tcp]in all three workflows. At the head, it selects 0 inpr.yamlbecause no case carriespre_merge, and it still selects the case in post-merge and nightly. - Both trees give the same 2 collection errors, in
tests/frontend/test_prepost.pyandtests/frontend/test_prepost_mistral.py.
Merge gates:
backend-status-checkneeds 41 jobs atmainand 38 at the head. The 3 dropped entries are the removed jobs. Theneeds:lists ofdeploy-status-checkanddynamo-status-checkdid not change.- I ran the
jqprogram ofbackend-status-checkfrom the head. Allsuccessorskippedgives exit 0. One failedvllm-test,sglang-test,trtllm-test,sidecar-buildorvllm-multi-gpu-testgives exit 1. A cancelledvllm-testalso gives exit 1. - Control: a planted
- sidecar-vllm-testin thatneeds:list gives the actionlint error[job-needs]. - No job named
sidecar-runtimeis one of the 7 required checks ofmain.
Filter: on this head, changed-files matched the sidecar filter only through tests/serve/test_sidecar.py. Then dynamo-sidecar / Build multi-arch cpu ran from 23:31:52Z to 00:24:56Z. lib/sidecar/Dockerfile copies no tests/ path. With the 10 lines removed, .github/scripts/test-filters.js --coverage still passes 74 of 74 tests and lists the same 18 uncovered files.
Tags: #14508 added the three tags for the pr.yaml fallback source_ref: ... || 'main'. At the head, a search of the whole tree finds the tag names only on these three lines.
Scope: after this PR, no pre-merge job starts a sidecar with a real engine, and no pre-merge job runs agg.sh, disagg.sh or agg_kv_router.sh. If Rust files change, Pre Merge rust-tests still runs the sidecar engines against the mocker gRPC servers.
Later main: after these runs, main moved to 522096a8 (#15051). That commit changes no workflow, no tests/ file and no file of this PR.
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
f4f8544 to
62c7556
Compare
✅ Dynamo PR CI passed — run 36944025026 (attempt 2) on
|
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approve at 62c7556ddf5e87b1b73954dd8190791300b1fa28. No P0, P1 or P2 is open. The two P3 items from my last review are still open, and they do not block the merge.
[P3] The 10 tests/ lines of the sidecar filter still start the sidecar image build, but no pre-merge job uses that build now. .github/filters.yaml:357-366. Please remove these lines. The core filter already covers tests/**.
On this head, a test file alone started the build, and the build failure failed backend-status-check.
changed-filesmatched thesidecarfilter only throughtests/serve/test_sidecar.py.- In attempt 1 of the PR run,
dynamo-sidecar / Build multi-arch cpuran from 00:04:00Z to 00:26:55Z and failed indynamo-sglang-sidecar. This PR does not change that crate.backend-status-checkfailed at 00:59:13Z because of it. Attempt 2 passed. - In
pr.yaml, onlybackend-status-checkandclean-k8s-builderlistsidecar-buildinneeds:. - With the 10 lines removed,
.github/scripts/test-filters.js --coveragestill passes 74 of 74 tests and lists the same 18 uncovered files.
[P3] The post-merge builds still push the three main-*-runtime-test tags, and no file in the repository reads them now. .github/workflows/post-merge-ci.yml:220, :287 and :354. Please remove the tags, or add a comment that names who still reads them.
Only the removed pre-merge sidecar jobs read these tags.
At e54dbaba29, three pr.yaml sidecar jobs read these tags through a source_ref fallback to 'main'. They are at pr.yaml:390, :420 and :446, and this PR removes them. At this head, a search of the whole tree finds the three tag names only on post-merge-ci.yml:221, :288 and :355.
Each sidecar case runs in one post-merge job and one nightly job, and in no pre-merge job.
I collected every pytest stage of pr.yaml, post-merge-ci.yml and nightly-ci.yml with its exact -m expression, 99 stages in total. The stages include those of shared-test.yml, dynamo-pipeline.yml, xpu-ci.yaml and the deploy-test workflows. The runs used the repository pyproject.toml and conftests, with --continue-on-collection-errors as in CI. They ran on macOS, so the stub list of tests/report_pytest_markers.py replaced the engines and dynamo._core.
| sidecar cases | post-merge job and nightly job |
|---|---|
vllm_aggregated, vllm_disaggregated, [vllm-replicas-tcp] |
sidecar-vllm-test, 1 GPU |
[vllm-dep-tcp] |
sidecar-vllm-test, 2 GPUs |
sglang_aggregated, sglang_disaggregated, [sglang-replicas-tcp] |
sidecar-sglang-test, 1 GPU |
[sglang-dep-tcp] |
sidecar-sglang-test, 2 GPUs |
trtllm_aggregated, trtllm_disaggregated |
sidecar-trtllm-test, 1 GPU |
- At
8391725a55, the same nightly jobs and GPU counts selected the same cases, withnightly and sidecar and .... No nightly job lost a case with the new selectors. - No other job selects a sidecar case. This includes the nightly GPU jobs whose expressions have no lifecycle marker.
- The merge with
mainat97bace4e46has no conflict and gives the same jobs for each case. tests/report_pytest_markers.py, thepytest-marker-reporthook, exits 0 at this head and reports 0 missing marker sets. Thepre-commitjob on this head also passed it.- actionlint 1.7.12 gives the same 14
[runner-label]errors fornightly-ci.ymlat8391725a55and at this head. For the three workflows, it gives the same 51 errors ate54dbaba29and at this head.
Controls:
- At
e54dbaba29, the collection finds onepr.yamlsidecar job for each case, in addition to the post-merge and nightly jobs. - In a copy, I removed
post_mergefromvllm_aggregated. The rootconftest.pythen addspre_merge, the collection finds 0 post-merge jobs for that case, andtests/report_pytest_markers.pyfails withMissing: Lifecycle. - With only 5 stubs (
dynamo._core,vllm,sglang,torchandtensorrt_llm), the same 10 cases come back with the same markers. The one difference is askipmark on the two TensorRT-LLM cases.tests/conftest.pyadds that mark in the full-stub run only, because TensorRT-LLM does not import there.@pytest.mark.sidecaroccurs only attests/serve/test_sidecar.py:253and:327. - A planted
matrix.gpu_cntinnightly-ci.ymladds one actionlint[expression]error. - On this head, the vLLM, SGLang and TensorRT-LLM pre-merge GPU test jobs, 1 GPU and 2 GPUs, ran no
test_sidecarcase.
I did not run the sidecar tests. The new selectors first run in post-merge-ci.yml after the merge.
Summary
Move native-sidecar GPU E2E tests to post-merge and nightly CI so their container startup delays and retries no longer block PR merges. Remove the pre-merge sidecar GPU jobs and their merge-gate dependencies, and update the pytest markers and scheduled selectors together.
Preserve separate framework jobs and separate 1-/2-GPU allocations. Give TensorRT-LLM the explicit
TensorRT-LLM Sidecar 1-GPU E2E Testlabel, matching the existing vLLM and SGLang labels. Keep pre-merge sidecar build/compliance checks and CPU test coverage unchanged.The CI investigation example spent about 35 minutes initializing the container and under four minutes running the sidecar suite.
Validation
libavutil.so.60; marker collection used the repository's dependency stubs.Related Issues
Follow-up to #15081, #15082, #15086, and #15328.
Summary by CodeRabbit