Skip to content

[[TRTLLM-13948][feat] Clean up DeepSeek tests using CPP Transceiver v1 - #17090

Draft
asfiyab-nvidia wants to merge 1 commit into
NVIDIA:mainfrom
asfiyab-nvidia:asfiyab/ds-remove-cpp-transceiver-tests
Draft

[[TRTLLM-13948][feat] Clean up DeepSeek tests using CPP Transceiver v1#17090
asfiyab-nvidia wants to merge 1 commit into
NVIDIA:mainfrom
asfiyab-nvidia:asfiyab/ds-remove-cpp-transceiver-tests

Conversation

@asfiyab-nvidia

@asfiyab-nvidia asfiyab-nvidia commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Description

This PR removes the DeepSeek tests that use the CPP Transceiver V1. Summary of changes:

Test Changes in test_disaggregated_serving.py

TestDeepSeekV32Exp

  • Removed: test_auto_dtype - redundant with test_kv_cache_v2_nixl_python (besides TP settings)

TestDeepSeekV3Lite

Removed tests:

  • test_nixl_backend - redundant with test_kv_cache_v2_nixl_python
  • test_auto_dtype - shares redundancies with test_kv_cache_v2_nixl_python and test_guided_decoding

Retained tests:

  • test_gen_only_sync - retained to test sync KV cache transfer
  • test_auto_dtype_with_helix - retained for Python transceiver execution path
  • test_guided_decoding - retained for testing guided decoding backends and ctx+gen spec dec format

The tests that were removed were also deleted from corresponding test lists

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-compatible or api-breaking. For api-breaking, include BREAKING in 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

  • Removed redundant DeepSeek CPP Transceiver V1 coverage and updated retained tests to use the Python transceiver with NIXL.
  • Retained synchronous KV-cache transfer, Helix, and guided-decoding coverage.
  • Updated QA, CI, and waiver lists to remove deleted or obsolete test cases.
  • No apparent configuration or formatting inconsistencies were identified.

QA Engineer Review

  • Modified test_disaggregated_serving.py: removed test_auto_dtype from TestDeepSeekV32Exp, removed test_nixl_backend and test_auto_dtype from TestDeepSeekV3Lite, and updated test_gen_only_sync.
  • Updated qa/llm_function_core.txt, qa/llm_function_rtx6k.txt, test-db/l0_dgx_b200.yml, l0_dgx_b300.yml, l0_dgx_h100.yml, l0_dgx_h200.yml, and waives.txt to remove corresponding entries.
  • Retained tests remain represented across the relevant CI and QA lists.
  • Verdict: needs follow-up pending CBTS coverage data.

Signed-off-by: Asfiya Baig <asfiyab@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Updated DeepSeek disaggregated-serving accuracy tests to use Python/NIXL configurations, removed obsolete auto-dtype cases, expanded KV-cache evaluations, and synchronized QA, platform test lists, and waiver entries.

Changes

Disaggregated-serving test coverage

Layer / File(s) Summary
Python and NIXL runtime configuration
tests/integration/defs/accuracy/test_disaggregated_serving.py
Synchronous generation-only coverage is Python-only; Helix and guided-decoding transfers use NIXL with the Python runtime.
Accuracy coverage adjustments
tests/integration/defs/accuracy/test_disaggregated_serving.py
Removed obsolete DeepSeek auto-dtype tests and expanded selected KV-cache evaluations to MMLU plus GSM8K.
Configured test inventory updates
tests/integration/test_lists/..., tests/integration/test_lists/waives.txt
Updated QA and platform test lists and removed obsolete auto-dtype waiver entries.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: tburt-nv

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title concisely summarizes the primary change—cleaning up DeepSeek tests that use CPP Transceiver V1—and includes the relevant ticket and feature type.
Description check ✅ Passed The description clearly explains the removed and retained tests and rationale, though its required Test Coverage section is empty and the checklist remains unchecked.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Fix failing CI checks
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5c5ef98 and 36f4732.

📒 Files selected for processing (8)
  • tests/integration/defs/accuracy/test_disaggregated_serving.py
  • tests/integration/test_lists/qa/llm_function_core.txt
  • tests/integration/test_lists/qa/llm_function_rtx6k.txt
  • tests/integration/test_lists/test-db/l0_dgx_b200.yml
  • tests/integration/test_lists/test-db/l0_dgx_b300.yml
  • tests/integration/test_lists/test-db/l0_dgx_h100.yml
  • tests/integration/test_lists/test-db/l0_dgx_h200.yml
  • tests/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

ids=["python", "cpp"])
def test_gen_only_sync(self, transceiver_runtime):
"""Test gen-only synchronous KV transfer with each NIXL runtime.
def test_gen_only_sync(self):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add the required return annotation.

-def test_gen_only_sync(self):
+def test_gen_only_sync(self) -> None:

As per coding guidelines, every Python function must be annotated.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def test_gen_only_sync(self):
def test_gen_only_sync(self) -> None:
🤖 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/defs/accuracy/test_disaggregated_serving.py` at line 1056,
Update the test_gen_only_sync method with the required return type annotation,
using the appropriate annotation for its test method behavior.

Source: Coding guidelines

@asfiyab-nvidia
asfiyab-nvidia marked this pull request as draft July 30, 2026 23:51

@chienchunhung chienchunhung left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -1255,7 +1175,8 @@ def test_auto_dtype_with_helix(self, comms_medium, cuda_graph_config,
"enable_chunked_prefill": False,
"cuda_graph_config": None,
"cache_transceiver_config": {
"backend": "UCX",
"backend": "NIXL",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For test_auto_dtype_with_helix — could you double-check whether the Python transceiver actually supports cp_size > 1? From what I can see, KvCacheTransceiverV2._check_compatible rejects cp_size != 1 (tensorrt_llm/_torch/disaggregation/transceiver.py), and Helix relies on context parallelism on the gen side, so this test likely can't be migrated and has to stay on the C++ transceiver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants