[None][test] Consolidate dis-agg E2E Tests - #16614
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change narrows disaggregated-serving coverage. Auto-dtype tests now use three block-reuse cases. Guided-decoding tests retain ChangesDisaggregated serving test coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
/bot run --stage-list "DGX_H100-2_GPUs-PyTorch-Others-*, DGX_H100-4_GPUs-PyTorch-DeepSeek-1" |
|
PR_Github #60342 [ run ] triggered by Bot. Commit: |
|
PR_Github #60342 [ run ] completed with state
|
9ce9bbb to
f430de6
Compare
|
/bot run --stage-list "DGX_H100-2_GPUs-PyTorch-Others-*, DGX_H100-4_GPUs-PyTorch-DeepSeek-1" |
|
PR_Github #60359 [ run ] triggered by Bot. Commit: |
|
PR_Github #60359 [ run ] completed with state
|
|
One thing worth calling out that isn't mentioned in the PR description: besides the UCX consolidation and parameter trimming, this PR also removes Per our RCCA / test-escape policy, a customer-bug regression test shouldn't be silently dropped — it should be confirmed by the RCCA owner and the removal should be documented in the description. Could you please either (a) confirm that scenario is now covered elsewhere and note it in the PR description, or (b) keep (fix) the test instead of deleting it? Thanks! |
fredricz-20070104
left a comment
fredricz-20070104
left a comment
There was a problem hiding this comment.
Please resolve my comments before merging.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --add-multi-gpu-test --disable-fail-fast --extra-stage "DGX_B200-4_GPUs-PyTorch-Post-Merge-1, DGX_B200-4_GPUs-PyTorch-Post-Merge-2, DGX_B200-4_GPUs-PyTorch-Post-Merge-3, DGX_B200-4_GPUs-PyTorch-Post-Merge-4, RTXPro6000D-PyTorch-Post-Merge-1, RTXPro6000D-4_GPUs-PyTorch-Post-Merge-1, RTXPro6000D-4_GPUs-PyTorch-Post-Merge-2, DGX_B300-4_GPUs-PyTorch-Post-Merge-1, DGX_B300-4_GPUs-PyTorch-Post-Merge-2" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_lists/waives.txt (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the NVIDIA copyright header.
This modified file has no visible NVIDIA copyright header. Add the repository-standard header and use
2026as the latest meaningful modification year.🤖 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 `@tests/integration/test_lists/waives.txt` at line 1, Add the repository-standard NVIDIA copyright header at the beginning of tests/integration/test_lists/waives.txt, using 2026 as the latest meaningful modification year, while preserving the existing waiver entry unchanged.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/integration/test_lists/waives.txt`:
- Line 1: Add the repository-standard NVIDIA copyright header at the beginning
of tests/integration/test_lists/waives.txt, using 2026 as the latest meaningful
modification year, while preserving the existing waiver entry unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4b011645-1eb7-49df-ab77-0f41f2b3c5b9
📒 Files selected for processing (12)
tests/integration/defs/accuracy/test_disaggregated_serving.pytests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_deepseek_v3_lite_ucx.yamltests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_llama31_8b.yamltests/integration/defs/disaggregated/test_disaggregated.pytests/integration/test_lists/qa/llm_function_core.txttests/integration/test_lists/qa/llm_function_rtx6k.txttests/integration/test_lists/test-db/l0_dgx_b200.ymltests/integration/test_lists/test-db/l0_dgx_b300.ymltests/integration/test_lists/test-db/l0_dgx_h100.ymltests/integration/test_lists/test-db/l0_h100.ymltests/integration/test_lists/test-db/l0_rtx_pro_6000.ymltests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (6)
- tests/integration/test_lists/test-db/l0_rtx_pro_6000.yml
- tests/integration/test_lists/test-db/l0_dgx_b200.yml
- tests/integration/test_lists/test-db/l0_h100.yml
- tests/integration/test_lists/qa/llm_function_rtx6k.txt
- tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_deepseek_v3_lite_ucx.yaml
- tests/integration/test_lists/test-db/l0_dgx_b300.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_llama31_8b.yaml
- tests/integration/test_lists/qa/llm_function_core.txt
- tests/integration/defs/disaggregated/test_disaggregated.py
- tests/integration/test_lists/test-db/l0_dgx_h100.yml
- tests/integration/defs/accuracy/test_disaggregated_serving.py
|
PR_Github #64931 [ run ] triggered by Bot. Commit: |
|
PR_Github #64931 [ run ] completed with state
|
The 2**4 cross-product of ctx/gen overlap-scheduler x ctx/gen block-reuse ran a full MMLU+GSM8K eval per combo. Overlap scheduler is token-invariant (covered by unit tests test_benchmark_disagg.py and test_disagg_gen_transfer_gate.py), so only the block-reuse axis changes which KV blocks are transferred. Keep block-reuse on/off with overlap enabled (production default) and drop the other 14 combos from CI (l0_dgx_h100, llm_function_core). Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com>
…d_decoding Guided decoding runs entirely on the generation worker and the KV-cache handoff is grammar-backend-agnostic, so exercising both xgrammar and llguidance under disaggregated serving only re-validates grammar-engine correctness that the aggregated guided-decoding tests already cover (test_llm_api_pytorch.py has 7 backend-parametrized sites). Keep xgrammar (the default) for the disagg plumbing guard and the DeepSeek guided+MTP interaction; drop the llguidance combos from TestLlama3_1_8BInstruct/TestDeepSeekV3Lite::test_guided_decoding (7 CI entries across l0_rtx_pro_6000, llm_function_core, llm_function_rtx6k). Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com>
…kv_cache_v2 UCX cache-transceiver coverage is model-agnostic plumbing already exercised by the cheap unit/C++ tests (unittest/disaggregated UCX params, cpp/test_multi_gpu test_cache_transceiver ucx_kvcache), so the expensive e2e UCX-specialized integration tests are redundant. Remove test_disaggregated_deepseek_v3_lite_fp8_ucx and test_disaggregated_deepseek_v3_lite_fp8_ucx_tp1_single_gpu, plus their now orphaned config disagg_config_ctxtp2_gentp2_deepseek_v3_lite_ucx.yaml and its config-map entry. Also remove TestLlama3_1_8BInstruct::test_kv_cache_v2_nixl_python: the KVCacheManager-v2 + NIXL + PYTHON transceiver path is model-agnostic and byte-covered by test_cache_transceiver_single_process.py; TestDeepSeekV3Lite keeps the canonical MLA representative. Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com>
…ogprobs test Switch the hardcoded backend=UCX in TestDeepSeekV3Lite/TestQwen3_8B test_auto_dtype_with_helix and the TestQwen3_8B chunked-prefill helper to DEFAULT, matching the rest of the accuracy suite (these tests cover helix and chunked prefill, not UCX specifically). Remove test_disaggregated_logprobs_serving and the llama31_8b_ucx config-map entry plus the orphaned yaml it exclusively used. Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com>
…nd config Review follow-up on NVIDIA#16614 (yingguo-trt, fredricz-20070104): the consolidation dropped test_disaggregated_logprobs_serving, which is the resident regression guard for NVBug 5926823 (disagg + streaming + logprobs). Its API-level coverage is unique -- streaming vs. non-streaming logprobs consistency across both the completions and chat APIs, plus chat top_logprobs across the ctx/gen boundary. test_disaggregated_single_gpu.py::test_disaggregated_logprobs only covers the core prefill->decode propagation, so deleting this would be a test escape. The test was never UCX-specific; it only reused the UCX yaml. Restore it on a new DEFAULT-backend config and drop the explicit TRTLLM_USE_UCX_KVCACHE pin, which keeps the de-UCX-specialization goal of this PR intact. UCX_TLS is kept, matching every other non-pinned test in this file. The waive for the open bug (NVBugs 6275959, fix in flight in NVIDIA#15221) is restored with it. Restores the numpy import: np.isclose in this test is its only user. Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
test_disaggregated_logprobs_serving is the regression guard for NVBug 5926823 (disagg + streaming + logprobs). The migration to the DEFAULT-backend config kept the test function and its waive, but dropped its entry from qa/llm_function_core.txt, leaving it in no test list at all -- so lifting the 6275959 waive later would not have brought it back. Restore the entry; the waive stays. validate-test-lists was already reporting this as a waive with no active test list. Two more entries that hook rejected, both from this branch's parametrize consolidation rather than from review: - test_guided_decoding[llguidance-mtp_nextn=2]: the backend list is now ["xgrammar"] only, so this ID no longer exists. - test_auto_dtype[False-True-True-True]: the four boolean parametrize decorators became one (ctx,gen) pair with ids block_reuse/no_block_reuse, and the waived combination was the mixed ctx=True/gen=False one, which no longer exists. Both waives point at test IDs that cannot be collected, so they are removed. If 6525893 still reproduces on a surviving test_auto_dtype variant, its waive needs re-pointing rather than deleting. Also record why DEFAULT is used instead of NIXL: launch_disaggregated_llm still sets TRTLLM_USE_UCX_KVCACHE=1 for every backend but NIXL, so dropping the per-test UCX pinning leaves transport coverage unchanged. Signed-off-by: Xiaowei Shi <39303645+Shixiaowei02@users.noreply.github.com>
- test_auto_dtype: zipping the two block-reuse flags to [(True, True), (False, False)] kept only the symmetric pairs. ctx_enable_block_reuse=True with gen_enable_block_reuse=False is the one combination where the servers disagree about which blocks are resident, so it takes a different transfer path; add it back as `ctx_block_reuse_only`. Three runs instead of two, still a 5x cut from sixteen. The GB200 waive for nvbugs/6525893 -- which tracked exactly that combination under the old [False-True-True-True] id -- is re-pointed at the new id rather than dropped. - waives.txt: drop the two H100 TestQwen3_5_*_VL::test_auto_dtype entries. They are unrelated to this consolidation and came in as a rebase artifact. - test_disaggregated_logprobs_serving: with TRTLLM_USE_UCX_KVCACHE=1 gone and the config on backend DEFAULT, this harness no longer selects UCX, so the leftover UCX_TLS export does nothing. (The accuracy harness is different: launch_disaggregated_llm still sets TRTLLM_USE_UCX_KVCACHE=1 for every backend but NIXL, which is why its DEFAULT configs keep UCX coverage.) Signed-off-by: Xiaowei Shi <39303645+Shixiaowei02@users.noreply.github.com>
b4fa76f to
125fa22
Compare
|
/bot run --add-multi-gpu-test --disable-fail-fast --extra-stage "DGX_B200-4_GPUs-PyTorch-Post-Merge-1, DGX_B200-4_GPUs-PyTorch-Post-Merge-2, DGX_B200-4_GPUs-PyTorch-Post-Merge-3, DGX_B200-4_GPUs-PyTorch-Post-Merge-4, RTXPro6000D-PyTorch-Post-Merge-1, RTXPro6000D-4_GPUs-PyTorch-Post-Merge-1, RTXPro6000D-4_GPUs-PyTorch-Post-Merge-2, DGX_B300-4_GPUs-PyTorch-Post-Merge-1, DGX_B300-4_GPUs-PyTorch-Post-Merge-2" |
|
PR_Github #65542 [ run ] triggered by Bot. Commit: |
Shixiaowei02
changed the title
Thank you for the reminder. This was an intentional change. Similar tests are often waived in practice, so we'd like to see what issues, if any, might surface when removing them at a larger scale. We'd also like to evaluate whether those issues can be adequately covered and addressed through unit tests rather than relying on dis-agg E2E tests. @brnguyen2 |
Thanks for the comment. Kept the test rather than dropping it, |
|
PR_Github #65542 [ run ] completed with state
|
|
/bot run --add-multi-gpu-test --disable-fail-fast --extra-stage "DGX_B200-4_GPUs-PyTorch-Post-Merge-1, DGX_B200-4_GPUs-PyTorch-Post-Merge-2, DGX_B200-4_GPUs-PyTorch-Post-Merge-3, DGX_B200-4_GPUs-PyTorch-Post-Merge-4, RTXPro6000D-PyTorch-Post-Merge-1, RTXPro6000D-4_GPUs-PyTorch-Post-Merge-1, RTXPro6000D-4_GPUs-PyTorch-Post-Merge-2, DGX_B300-4_GPUs-PyTorch-Post-Merge-1, DGX_B300-4_GPUs-PyTorch-Post-Merge-2" |
|
PR_Github #65854 [ run ] triggered by Bot. Commit: |
|
PR_Github #65854 [ run ] completed with state
|
|
/bot run --add-multi-gpu-test --disable-fail-fast --extra-stage "DGX_B200-4_GPUs-PyTorch-Post-Merge-1, DGX_B200-4_GPUs-PyTorch-Post-Merge-2, DGX_B200-4_GPUs-PyTorch-Post-Merge-3, DGX_B200-4_GPUs-PyTorch-Post-Merge-4, RTXPro6000D-PyTorch-Post-Merge-1, RTXPro6000D-4_GPUs-PyTorch-Post-Merge-1, RTXPro6000D-4_GPUs-PyTorch-Post-Merge-2, DGX_B300-4_GPUs-PyTorch-Post-Merge-1, DGX_B300-4_GPUs-PyTorch-Post-Merge-2" |
|
PR_Github #66111 [ run ] triggered by Bot. Commit: |
|
PR_Github #66111 [ run ] completed with state |
9 participants
Description
Consolidates the disaggregated E2E test suite: drops UCX-specialized duplicates, trims
over-parametrized cases, and removes one redundant NIXL test. Net effect on the test lists is
50 entries removed, 7 added.
1. UCX-specialized disagg tests removed
test_disaggregated_deepseek_v3_lite_fp8_ucxandtest_disaggregated_deepseek_v3_lite_fp8_ucx_tp1_single_gpu,plus their YAML configs and list entries. The
_tp1_single_gpuone ran the samedeepseek_v3_lite_fp8_tp1config as the non-UCX test, so it was a straight duplicate. Theaccuracy configs flip
UCXtoDEFAULT, which preserves UCX coverage:launch_disaggregated_llmstill sets
TRTLLM_USE_UCX_KVCACHE=1for every backend but NIXL, so this only removes theper-test pinning, it does not change the transport under test.
2.
TestLlama3_1_8BInstruct::test_kv_cache_v2_nixl_pythonremovedCalled out explicitly because this is NIXL + PYTHON transceiver coverage, not UCX, so it sits
outside the stated scope above. It is removed as redundant:
test_kv_cache_v2_nixl_pythonisstill defined and scheduled for
TestDeepSeekR1,TestGPTOSSandTestGemma3_1BInstruct, sothe code path keeps three independent guards.
3.
TestLlama3_1_8BInstruct::test_auto_dtypetrimmed from 16 combinations to 3The two overlap-scheduler axes are dropped — overlap scheduling is token-invariant and
unit-tested — leaving the block-reuse axis, which is what actually changes which KV gets
transferred. The surviving ids are
block_reuse,ctx_block_reuse_onlyandno_block_reuse.The asymmetric
ctx_block_reuse_onlycase (ctx reuse on, gen reuse off) is kept deliberately:it is the only combination where the two servers disagree about which blocks are already
resident. The GB200 waive for nvbugs/6525893, which tracked exactly that combination under the
old
[False-True-True-True]id, is re-pointed at the new id rather than dropped.4.
TestDeepSeekV3Lite::test_guided_decodingloses itsllguidanceaxisKnown coverage reduction, stated rather than hidden: after this change the disaggregated suite
has no
llguidanceguided-decoding coverage for DeepSeek. The aggregatedtest_llm_api_pytorch.py::TestDeepSeekV3Lite::test_guided_decoding[llguidance-*]cases stillrun, but unlike Llama — which additionally keeps
test_guided_decoding_with_eagle3[llguidance-*]in the disagg suite — DeepSeek has no disagg-side fallback. Restoring one
llguidancemtpvariant is a reasonable follow-up if the runtime cost is acceptable.
5.
test_disaggregated_logprobs_servingis kept, not removedThis is the resident regression guard for customer bug NVBug 5926823 (disagg + streaming +
logprobs), and its API-level coverage is unique — streaming vs non-streaming consistency and
top_logprobsacross the ctx/gen boundary.test_disaggregated_single_gpu.py::test_disaggregated_logprobsonly covers core propagation. Rather than dropping it with the other UCX-reusing tests, it was
migrated to a DEFAULT-backend config (
disagg_config_ctxtp2_gentp2_llama31_8b.yaml), and bothits nvbugs/6275959 waive and its
qa/llm_function_core.txtentry are retained, so it staysscheduled and returns automatically once the waive lifts. The leftover
UCX_TLSexport in thattest is dropped, since this harness no longer selects UCX.
Test Coverage
No product code changes — every file in this PR is under
tests/. Validation is the repo's ownpre-commitsuite, in particularvalidate-test-lists, which AST-checks that every id in thetest-db / qa lists and every waive resolves to a real, collectible test. It passes clean,
including the re-pointed
test_auto_dtype[ctx_block_reuse_only]waive and its matching entriesin
l0_dgx_h100.ymlandqa/llm_function_core.txt.