[None][Test] Consolidate dis-agg E2E Tests - #16614
Conversation
WalkthroughThe change narrows disaggregated-serving coverage. Auto-dtype tests now use 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
|
| @pytest.mark.skip_less_device(4) | ||
| @pytest.mark.parametrize("llama_model_root", ['llama-3.1-8b-instruct'], | ||
| indirect=True) | ||
| def test_disaggregated_logprobs_serving(disaggregated_test_root, |
There was a problem hiding this comment.
Do we test log probs in any other place?
There was a problem hiding this comment.
The API-level coverage here is unique: streaming vs. non-streaming logprobs consistency and top_logprobs across the ctx/gen boundary (RCCA for https://nvbugs/5926823). Only the core propagation is covered elsewhere (test_disaggregated_single_gpu.py::test_disaggregated_logprobs).
The test isn't UCX-specific — it just reused the UCX yaml. It also has an open bug (https://nvbugs/6275959, fix in flight: #15221). Suggest migrating it to a DEFAULT/NIXL config and keeping the waive, instead of deleting.
|
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
There was a problem hiding this comment.
Please resolve my comments before merging.
nv-xtf
left a comment
There was a problem hiding this comment.
LGTM overall — just one question about the backend default behavior (DEFAULT still resolves to UCX in this harness), see inline comment.
f430de6 to
973bfa0
Compare
…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>
973bfa0 to
7a1e5eb
Compare
…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>
|
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. |
…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>
06b81fb to
c52c272
Compare
|
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" |
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>
c52c272 to
9732c55
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" |
|
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. |
|
PR_Github #64455 [ run ] triggered by Bot. Commit: |
|
PR_Github #64455 [ run ] completed with state
|
Description
This pull request removes the remaining UCX-specific disaggregated test configurations and test cases, consolidates and simplifies test parametrizations, and updates test lists and configs to reflect these changes. The main goal is to streamline the test suite by removing redundant or unnecessary UCX backend coverage, as this is now handled elsewhere or is no longer required.
Test configuration and coverage cleanup:
"backend": "DEFAULT"instead of"UCX"forcache_transceiver_config, ensuring consistent backend selection and test coverage. [1] [2] [3] [4] [5] [6]Test parametrization simplification:
test_auto_dtypeinTestLlama3_1_8BInstructto only test the two relevant block-reuse settings, removing overlap scheduler parameters which are now considered redundant. [1] [2] [3]xgrammar, since backend correctness is now covered by aggregated tests. [1] [2] [3] [4]Test list and config file maintenance:
llm_function_core.txtand updated test config lookups accordingly. [1] [2] [3] [4] [5] [6]These changes reduce maintenance overhead and make the test suite easier to understand and extend.
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.Dev Engineer Review
DEFAULTtransceiver backend.test_disaggregated_logprobs_serving, which guarded NVBug 5926823. Confirm equivalent coverage or restore and repair the test.DEFAULTprovides the intended transport coverage.QA Engineer Review
TestLlama3_1_8BInstruct.test_auto_dtype.xgrammar.test_disaggregated_logprobs_serving.llm_function_core.txt,llm_function_rtx6k.txt,l0_dgx_h100.yml,l0_rtx_pro_6000.yml,l0_dgx_b200.yml,l0_dgx_b300.yml,l0_h100.yml, andwaives.txt.