[TRTLLM-13948][feat] Clean up DeepSeek tests using CPP Transceiver v1 - #17090
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:
WalkthroughDeepSeek disaggregated-serving tests now use NIXL with the Python transceiver runtime across accuracy scenarios and YAML configurations. UCX coverage is split by tensor parallel size. Test lists and duration records remove obsolete entries. ChangesDisaggregated-serving test coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/integration/defs/accuracy/test_disaggregated_serving.py`:
- Line 1056: Update the test_gen_only_sync method with the required return type
annotation, using the appropriate annotation for its test method behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69392dbb-874d-41f6-9c44-e37bd47fcebe
📒 Files selected for processing (8)
tests/integration/defs/accuracy/test_disaggregated_serving.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_dgx_h200.ymltests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (7)
- tests/integration/test_lists/test-db/l0_dgx_b300.yml
- tests/integration/test_lists/qa/llm_function_rtx6k.txt
- tests/integration/test_lists/test-db/l0_dgx_h100.yml
- tests/integration/test_lists/test-db/l0_dgx_h200.yml
- tests/integration/test_lists/test-db/l0_dgx_b200.yml
- tests/integration/test_lists/waives.txt
- tests/integration/test_lists/qa/llm_function_core.txt
chienchunhung
left a comment
chienchunhung
left a comment
There was a problem hiding this comment.
test_gen_only_sync is no longer parametrized, so pytest no longer generates the [python] node ID. Please update this test-list entry to ...::test_gen_only_sync; otherwise the QA selection references a nonexistent test and the retained synchronous-transfer coverage will not run.
|
Thanks for the effort! Please note, |
Thanks for the catch! Updated the test lists |
Thanks for the input! I have removed |
|
CI Report analyses:
|
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #64400 [ run ] triggered by Bot. Commit: |
|
PR_Github #64400 [ run ] completed with state |
|
/bot run |
|
PR_Github #64640 [ run ] triggered by Bot. Commit: |
|
PR_Github #64640 [ run ] completed with state |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Solid cleanup and the sweep is thorough — I grepped for the deleted node IDs and the deleted YAML and found no dangling references. Three things worth a look before merge, none blocking.
Description gaps. The description says the UCX configs were "left unchanged," but the PR also adds disagg_config_ctxtp1_gentp1_deepseek_v3_lite_ucx.yaml and renames the config keys to deepseek_v3_lite_fp8_tp1_ucx / ..._tp2_ucx. That's the most interesting change here and deserves a line: once the shared TP1 config is pinned to backend: NIXL, TRTLLM_USE_UCX_KVCACHE=1 no longer steers test_disaggregated_deepseek_v3_lite_fp8_ucx_tp1_single_gpu to UCX, so it would have silently become a second NIXL test. Good catch; please say so in the description so the next reader understands why the file exists.
Coverage question on the removed V3Lite test_kv_cache_v2_nixl_python. The V32Exp one was parametrized [False]/cache_mgr_v1, so it genuinely tested nothing unique — agreed. But the TestDeepSeekV3Lite variant hardcoded use_kv_cache_manager_v2: True with enable_block_reuse: False, and neither test_auto_dtype nor test_guided_decoding sets those. Is DeepSeek KV-cache-manager-v2 disagg still covered somewhere (the disagg_config_conditional_deepseek_v3_v2.yaml path), or does this drop it? Worth stating explicitly in the description either way.
Loss of the auto path. Every DeepSeek config now pins NIXL + PYTHON explicitly, which is fine for determinism but means nothing exercises _resolve_default_backend() → get_preferred_transceiver_runtime() for DeepSeek anymore — the hook that also has to return CPP for GLM checkpoints. Consider leaving one config on backend: DEFAULT with the runtime unset so a regression in that resolution still shows up.
|
/bot run --add-multi-gpu-test --disable-fail-fast --extra-stage "DGX_H100-4_GPUs-PyTorch-DeepSeek-1" |
e6e7f0b to
9b6085c
Compare
Signed-off-by: Asfiya Baig <asfiyab@nvidia.com>
… coverage of DSV3.2 Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> remove TestDeepSeekV3Lite::test_auto_dtype_with_helix as helix can't run with Python transceiver Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> Update DSV3-Lite to retain test_auto_dtype and remove test_kv_cache_v2_nixl_python Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> TestDeepSeekV3Lite::test_auto_dtype_with_helix to use CPP transceiver Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> minor rearrange Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> migrate deepseek configs to Python transceiver for tests/integration/defs/disaggregated/test_configs Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> migrate deepseek configs to Python transceiver for tests/integration/defs/disaggregated/test_configs Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> remove disagg_config_ctxtp2_gentp2_deepseek_v3_lite_transceiver_runtime_python config from test Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> remove test_disaggregated_deepseek_v3_lite_fp8_transceiver_runtime_python and associated test list entries Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> remove test_disaggregated_deepseek_v3_lite_fp8_ucx_tp1_single_gpu and associated test list entries Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> Revert "remove test_disaggregated_deepseek_v3_lite_fp8_ucx_tp1_single_gpu and associated test list entries" This reverts commit d1eb996. Add config for TP1 UCX test Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> fix rebase bugs Signed-off-by: Asfiya Baig <asfiyab@nvidia.com> fix rebase bugs Signed-off-by: Asfiya Baig <asfiyab@nvidia.com>
9b6085c to
0a4b65b
Compare
|
/bot run --add-multi-gpu-test --disable-fail-fast --extra-stage "DGX_H100-4_GPUs-PyTorch-DeepSeek-1" |
|
PR_Github #65140 [ run ] triggered by Bot. Commit: |
|
PR_Github #65140 [ run ] completed with state
|
BowenFu
left a comment
BowenFu
left a comment
There was a problem hiding this comment.
Test-only migration preserves product behavior; the explicit NIXL/Python pins and test inventory removals are internally consistent. The open TP1 UCX resource thread is non-blocking because its author approved that reviewed head; the other open threads are bot/style or optional coverage notes. Current CI failed on pod-launch infrastructure and an unrelated Harmony test.
Signed-off-by: Asfiya Baig <asfiyab@nvidia.com>
|
/bot run |
|
PR_Github #65350 [ run ] triggered by Bot. Commit: |
|
PR_Github #65350 [ run ] completed with state |
asfiyab-nvidia
changed the title
9 participants
Description
This PR removes the DeepSeek tests that use the CPP Transceiver V1. Summary of changes:
Test Changes in
test_disaggregated_serving.pyAll tests that are not removed have been updated to use the NIXL backend with the Python Transceiver runtime if they weren't already.
TestDeepSeekV32Exptest_kv_cache_v2_nixl_python- redundant withtest_auto_dtype(besides TP settings)TestDeepSeekV3LiteRetained tests:
test_gen_only_sync- Updated test and corresponding test-lists to be non-parametrized over the Transceiver runtimetest_gen_only_spec_dectest_auto_dtypetest_guided_decodingtest_gen_firsttest_auto_dtype_with_helix- Uses UCX backend with CPP runtime and does not support Python transceiver execution path. This test has been left unchanedRemoved tests:
test_nixl_backend- redundant withtest_auto_dtypetest_kv_cache_v2_nixl_python- shares redundancies withtest_auto_dtypeandtest_guided_decodingThe tests that were removed were also deleted from corresponding test lists
Test Changes in
tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_deepseek_v3_lite_transceiver_runtime_python.yamlas it doesn't test anything uniquely anymore post the transceiver migrationdisagg_config_ctxtp1_gentp1_deepseek_v3_lite_ucx.yamlto testtest_disaggregated_deepseek_v3_lite_fp8_ucx_tp1_single_gpuwhich was using a config that has been migrated to PYTHON backenddisagg_config_ctxtp2_gentp1cp2_deepseek_v3_lite_bf16_tllm_gen.yaml,disagg_config_ctxtp2_gentp2_deepseek_v3_lite_mpi.yaml,disagg_config_ctxtp2_gentp2_deepseek_v3_lite_ucx.yamlunchangedTest 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
test_gen_only_syncentry to use the non-parameterized node ID.QA Engineer Review
Modified or removed test functions:
TestDeepSeekV3Lite.test_gen_only_sync; covered by QA and CI test lists.TestDeepSeekV3Lite.test_nixl_backend; no test-list entries remain.TestDeepSeekV3Lite.test_auto_dtype; no test-list entries remain.TestDeepSeekV32Exp.test_guided_decoding; covered by existing test-list coverage.TestDeepSeekV32Exp.test_kv_cache_v2_nixl_python; no test-list entries remain.test_disaggregated_deepseek_v3_lite_fp8_transceiver_runtime_python; no test-list entries remain.Retained DeepSeek R1 KV-cache v2 NIXL Python and DeepSeek V3.2 auto-dtype coverage in
qa/llm_function_core.txt.Verdict: sufficient.