[TRTLLM-13948][feat] Set DeepSeekV3 to use Python KV-cache transceiver V2 by default - #16908
[TRTLLM-13948][feat] Set DeepSeekV3 to use Python KV-cache transceiver V2 by default#16908asfiyab-nvidia wants to merge 4 commits into
Conversation
|
/bot run --extra-stage "A10-PyTorch-1,A10-PyTorch-2,A10-PyTorch-3" |
WalkthroughDeepSeekV3 transceiver preference resolution now unconditionally selects the Python runtime. Unit tests cover the updated architectures and NIXL auto-resolution, while four disaggregated benchmark configurations explicitly select the CPP runtime. ChangesDeepSeek transceiver preference
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (1)
tensorrt_llm/_torch/models/modeling_deepseekv3.py (1)
1897-1907: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the override return type and document
pretrained_config.
get_preferred_transceiver_runtime()always returns"PYTHON", soOptional[Literal["PYTHON"]]can beLiteral["PYTHON"]. Add anArgs:entry forpretrained_configto match the hook doc style.🤖 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 `@tensorrt_llm/_torch/models/modeling_deepseekv3.py` around lines 1897 - 1907, Update get_preferred_transceiver_runtime to return Literal["PYTHON"] instead of Optional[Literal["PYTHON"]], and add an Args entry documenting the pretrained_config parameter in its docstring while preserving the existing behavior.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.
Inline comments:
In `@tests/unittest/llmapi/test_llm_args.py`:
- Around line 3407-3429: Update the three test
methods—test_preference_per_architecture, test_prefers_python_without_config,
and test_deepseek_resolves_auto_to_python_on_nixl—to include explicit -> None
return annotations; add the appropriate types for the architectures and
model_type parameters in test_preference_per_architecture.
---
Nitpick comments:
In `@tensorrt_llm/_torch/models/modeling_deepseekv3.py`:
- Around line 1897-1907: Update get_preferred_transceiver_runtime to return
Literal["PYTHON"] instead of Optional[Literal["PYTHON"]], and add an Args entry
documenting the pretrained_config parameter in its docstring while preserving
the existing 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: 1d535ade-6c5d-4531-a58f-aa4d096b1032
📒 Files selected for processing (2)
tensorrt_llm/_torch/models/modeling_deepseekv3.pytests/unittest/llmapi/test_llm_args.py
|
PR_Github #61983 [ run ] triggered by Bot. Commit: |
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/unittest/llmapi/test_llm_args.py (1)
3402-3432: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the new tests to the integration lists. None of
test_preference_per_architecture,test_prefers_python_without_config, ortest_deepseek_resolves_auto_to_python_on_nixlappears undertests/integration/test_lists/test-db/ortests/integration/test_lists/qa/, so CI/manual QA won’t pick them up.🤖 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/unittest/llmapi/test_llm_args.py` around lines 3402 - 3432, Add test-list entries for test_preference_per_architecture, test_prefers_python_without_config, and test_deepseek_resolves_auto_to_python_on_nixl under both tests/integration/test_lists/test-db/ and tests/integration/test_lists/qa/, following the existing list syntax and grouping conventions.Source: Path instructions
🤖 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/unittest/llmapi/test_llm_args.py`:
- Around line 3402-3432: Add test-list entries for
test_preference_per_architecture, test_prefers_python_without_config, and
test_deepseek_resolves_auto_to_python_on_nixl under both
tests/integration/test_lists/test-db/ and tests/integration/test_lists/qa/,
following the existing list syntax and grouping conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4130b30e-6fd2-4789-9624-723e47fa2f75
📒 Files selected for processing (1)
tests/unittest/llmapi/test_llm_args.py
|
PR_Github #61983 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #62029 [ run ] triggered by Bot. Commit: |
| pretrained_config, 'model_type', None) == 'glm_moe_dsa'): | ||
| return "PYTHON" | ||
| return None | ||
| return "PYTHON" |
There was a problem hiding this comment.
This change silently flips two GB300 perf configs that were deliberately reverted to the V1 transceiver. When #16669 migrated the DeepSeek R1/V3.2 disagg perf cases to transceiver_runtime: PYTHON, gb300_deepseek-r1-fp4_128k8k_con256_ctx1_pp4... and gb300_deepseek-r1-fp4_8k1k_con4096_ctx1_dep4... (4 files across tests/scripts/perf-sanity/disaggregated/ and tests/scripts/perf/disaggregated/) were intentionally left unpinned, relying on the then-current "auto → C++" resolution to stay on V1. This PR changes what auto resolves to for DeepSeek, so those 4 configs will silently switch to the V2 path they were reverted away from. If the GB300 issue is not yet resolved, please pin transceiver_runtime: CPP in those 4 files as part of this PR; if it is resolved, please state that in the description.
There was a problem hiding this comment.
Thanks for the catch. Pushed the change and updated the description accordingly
|
PR_Github #62029 [ run ] completed with state
|
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/scripts/perf-sanity/disaggregated/Untitled (1)
1-2: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required NVIDIA copyright header.
This new file must include the repository-standard NVIDIA copyright header before the manifest entry.
🤖 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/scripts/perf-sanity/disaggregated/Untitled` around lines 1 - 2, Add the repository-standard NVIDIA copyright header at the beginning of the new manifest file, before the existing YAML filename entry.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/scripts/perf-sanity/disaggregated/Untitled`:
- Around line 1-2: Add the repository-standard NVIDIA copyright header at the
beginning of the new manifest file, before the existing YAML filename entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5b959064-6798-4834-8abb-ae86193c0b51
📒 Files selected for processing (5)
tests/scripts/perf-sanity/disaggregated/Untitledtests/scripts/perf-sanity/disaggregated/gb300_deepseek-r1-fp4_128k8k_con256_ctx1_pp4_gen1_dep8_eplb0_mtp1_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb300_deepseek-r1-fp4_8k1k_con4096_ctx1_dep4_gen1_dep16_eplb0_mtp1_ccb-NIXL.yamltests/scripts/perf/disaggregated/gb300_deepseek-r1-fp4_128k8k_con256_ctx1_pp4_gen1_dep8_eplb0_mtp1_ccb-NIXL.yamltests/scripts/perf/disaggregated/gb300_deepseek-r1-fp4_8k1k_con4096_ctx1_dep4_gen1_dep16_eplb0_mtp1_ccb-NIXL.yaml
|
/bot run --disable-fail-fast |
|
PR_Github #62255 [ run ] triggered by Bot. Commit: |
|
Could you please confirm whether the DeepSeek unit tests have been fully replaced? Thanks! |
|
PR_Github #62255 [ run ] completed with state
|
|
Tests affected in this PR have passed - https://prod.blsm.nvidia.com/sw-tensorrt-top-1/job/LLM/job/main/job/L0_MergeRequest_PR/50424/testReport/A10-PyTorch-2.unittest.llmapi.test_llm_args/TestDeepseekTransceiverPreference/ Remaining tests are unrelated to the changes made |
Here is a link to the replaced tests: https://prod.blsm.nvidia.com/sw-tensorrt-top-1/job/LLM/job/main/job/L0_MergeRequest_PR/50424/testReport/A10-PyTorch-2.unittest.llmapi.test_llm_args/TestDeepseekTransceiverPreference/ |
|
/bot run --disable-fail-fast |
|
PR_Github #62645 [ run ] triggered by Bot. Commit: |
|
PR_Github #62645 [ run ] completed with state
|
…er by default Signed-off-by: Asfiya Baig <asfiyab@nvidia.com>
Signed-off-by: Asfiya Baig <asfiyab@nvidia.com>
…ython transceiver Signed-off-by: Asfiya Baig <asfiyab@nvidia.com>
Signed-off-by: Asfiya Baig <asfiyab@nvidia.com>
f95fc15 to
ea04c72
Compare
nv-xtf
changed the title
|
/bot run --disable-fail-fast |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tensorrt_llm/_torch/models/modeling_deepseekv3.py (1)
1900-1907: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
pretrained_configin the public docstring.The rewritten public-interface docstring omits its argument and does not use the required Google-style
Args:/Returns:sections. Add those sections while documenting that the preference does not inspectpretrained_config.Suggested documentation update
and the backend is NIXL. + + Args: + pretrained_config: Loaded Hugging Face configuration, if available. + This implementation does not inspect it. + + Returns: + The preferred KV-cache transceiver runtime, ``"PYTHON"``. """🤖 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 `@tensorrt_llm/_torch/models/modeling_deepseekv3.py` around lines 1900 - 1907, The public docstring for the transceiver-preference method must use Google-style Args and Returns sections. Document the pretrained_config argument and explicitly state that the preference does not inspect it, while preserving the existing explanation and "PYTHON" return behavior.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.
Nitpick comments:
In `@tensorrt_llm/_torch/models/modeling_deepseekv3.py`:
- Around line 1900-1907: The public docstring for the transceiver-preference
method must use Google-style Args and Returns sections. Document the
pretrained_config argument and explicitly state that the preference does not
inspect it, while preserving the existing explanation and "PYTHON" return
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f09550f9-391c-4ba8-aa20-7ccf3e2da2a7
📒 Files selected for processing (5)
tensorrt_llm/_torch/models/modeling_deepseekv3.pytests/scripts/perf-sanity/disaggregated/gb300_deepseek-r1-fp4_128k8k_con256_ctx1_pp4_gen1_dep8_eplb0_mtp1_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb300_deepseek-r1-fp4_8k1k_con4096_ctx1_dep4_gen1_dep16_eplb0_mtp1_ccb-NIXL.yamltests/scripts/perf/disaggregated/gb300_deepseek-r1-fp4_128k8k_con256_ctx1_pp4_gen1_dep8_eplb0_mtp1_ccb-NIXL.yamltests/scripts/perf/disaggregated/gb300_deepseek-r1-fp4_8k1k_con4096_ctx1_dep4_gen1_dep16_eplb0_mtp1_ccb-NIXL.yaml
|
PR_Github #62713 [ run ] triggered by Bot. Commit: |
5 participants
Description
Built on top of #16344 and #16669 (per-model transceiver runtime auto selection): opt DeepSeekV3.2/R1/V3-Lite into the Python KV-cache transceiver by default.
DeepseekV32ForCausalLM and DeepseekV3ForCausalLM now overrides get_preferred_transceiver_runtime() to return "PYTHON". Disaggregated deployments that leave transceiver_runtime at its default auto (backend NIXL/DEFAULT) use the Python transceiver (KvCacheTransceiverV2) out of the box. Explicit user settings always take precedence, and incompatible backends fall back to the C++ transceiver per the #16164 resolution semantics. No effect on non-disaggregated deployments.
Additionally set the below configs to use the CPP Transceiver while PR #16834 addresses the perf regression observed during migration of the transceiver from CPP to PYTHON.
Test Coverage
TestGlm5TransceiverPreferencetoTestDeepseekTransceiverPreferencetest_preference_per_architecturetest_no_config_defers_to_cpp->test_prefers_python_without_configandtest_glm5_resolves_auto_to_python_on_nixl->test_deepseek_resolves_auto_to_python_on_nixlPR 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
transceiver_runtime: CPPfor both context and generation workers, preserving the intended performance baseline.QA Engineer Review
tests/unittest/llmapi/test_llm_args.py; the suite was renamed/reworked from GLM-5-focused coverage to DeepSeek/GLM coverage.tests/integration/test_lists/,test-db/, orqa/files were modified, so CI/manual test-list coverage is not represented.Verdict: needs follow-up — test-list coverage data is unavailable.