[None][feat] Default GLM-5 to the Python KV-cache transceiver#16524
Open
chuangz0 wants to merge 2 commits into
Open
[None][feat] Default GLM-5 to the Python KV-cache transceiver#16524chuangz0 wants to merge 2 commits into
chuangz0 wants to merge 2 commits into
Conversation
…in disagg GLM-5 and GLM-5.2 (GlmMoeDsaForCausalLM / glm_moe_dsa) share the DeepseekV3ForCausalLM implementation class; override get_preferred_transceiver_runtime() to opt GLM checkpoints into the Python (v2) transceiver, differentiated per checkpoint so DeepSeek-V3/ V3.2 keep the C++ default. The preference is adopted only when the user leaves cache_transceiver_config.transceiver_runtime at 'auto' and the effective backend is NIXL. Set transceiver_runtime: PYTHON explicitly in the GLM-5 perf-sanity disaggregated NIXL configs, and switch the GLM-5 disagg stress-test config from backend DEFAULT to NIXL + PYTHON so the TRTLLM_USE_UCX_KVCACHE=1 fallback cannot silently revert it to the C++ transceiver. Add unit tests covering the per-architecture preference and the end-to-end 'auto' resolution for GLM-5 on NIXL. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
chuangz0
requested review from
dc3671,
kaiyux,
mikeiovine,
nv-ananjappa,
nv-xtf,
reasonsolo,
ruodil,
tongyuantongyu and
yingguo-trt
July 17, 2026 03:56
Collaborator
Author
|
/bot run --disable-fail-fast |
Contributor
📝 WalkthroughWalkthroughAdds GLM-5-specific transceiver runtime selection to ChangesGLM-5 transceiver runtime
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TorchLlmArgs
participant _resolve_transceiver_runtime_auto
participant DeepseekV3ForCausalLM
TorchLlmArgs->>_resolve_transceiver_runtime_auto: Resolve auto runtime with NIXL configuration
_resolve_transceiver_runtime_auto->>DeepseekV3ForCausalLM: Inspect pretrained GLM-5 configuration
DeepseekV3ForCausalLM-->>_resolve_transceiver_runtime_auto: Return PYTHON
_resolve_transceiver_runtime_auto-->>TorchLlmArgs: Set transceiver_runtime to PYTHON
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tensorrt_llm/_torch/models/modeling_deepseekv3.py (1)
1896-1899: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the hook’s configuration type.
Anydefeats the new interface contract. UsePretrainedConfig | NoneandLiteral["PYTHON"] | None; add Google-styleArgs/Returnsdocumentation for the hook.Based on coding guidelines: “Annotate every function, use
Nonefor non-returning functions, avoidAny… and use …|.”🤖 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 1896 - 1899, Update the classmethod get_preferred_transceiver_runtime signature to accept PretrainedConfig | None and return Literal["PYTHON"] | None, replacing Any and Optional annotations. Add Google-style Args and Returns documentation describing the configuration parameter and optional runtime result.Source: Coding guidelines
tests/unittest/llmapi/test_llm_args.py (1)
3170-3211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the newly added test functions.
Add precise parameter and return annotations to
_pretrained_configand the three test methods, including-> Nonefor tests.Based on coding guidelines: “Annotate every function … [and] avoid
Any.”🤖 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 3170 - 3211, Annotate `_pretrained_config` with concrete parameter and return types, avoiding `Any`, and add precise parameter annotations plus `-> None` to `test_preference_per_architecture`, `test_no_config_defers_to_cpp`, and `test_glm5_resolves_auto_to_python_on_nixl`. Use types matching the architecture list, model type, expected runtime value, and test fixtures already used.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 3178-3189: Update test_preference_per_architecture in
tests/unittest/llmapi/test_llm_args.py to isolate both production predicates:
add a GLM architecture paired with a non-GLM model_type expecting "PYTHON", and
add a non-GLM architecture paired with model_type="glm_moe_dsa" expecting the
fallback result. The existing coverage is insufficient until these cases are
included; no follow-up outside this test file is needed.
---
Nitpick comments:
In `@tensorrt_llm/_torch/models/modeling_deepseekv3.py`:
- Around line 1896-1899: Update the classmethod
get_preferred_transceiver_runtime signature to accept PretrainedConfig | None
and return Literal["PYTHON"] | None, replacing Any and Optional annotations. Add
Google-style Args and Returns documentation describing the configuration
parameter and optional runtime result.
In `@tests/unittest/llmapi/test_llm_args.py`:
- Around line 3170-3211: Annotate `_pretrained_config` with concrete parameter
and return types, avoiding `Any`, and add precise parameter annotations plus `->
None` to `test_preference_per_architecture`, `test_no_config_defers_to_cpp`, and
`test_glm5_resolves_auto_to_python_on_nixl`. Use types matching the architecture
list, model type, expected runtime value, and test fixtures already used.
🪄 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: c8e06700-376f-4f2c-b544-22493efbc27e
📒 Files selected for processing (15)
tensorrt_llm/_torch/models/modeling_deepseekv3.pytests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4ep4_gentp4ep4_glm5_nvfp4_dp_tllm.yamltests/scripts/perf-sanity/disaggregated/gb200_glm-5-fp4_1k1k_con1_ctx1_dep4_gen1_tep4_eplb0_mtp3_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb200_glm-5-fp4_1k1k_con4096_ctx1_dep4_gen1_dep8_eplb256_mtp1_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb200_glm-5-fp4_1k1k_con512_ctx1_dep4_gen1_dep32_eplb0_mtp3_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb200_glm-5-fp4_8k1k_con1024_ctx1_dep4_gen1_dep8_eplb256_mtp1_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb200_glm-5-fp4_8k1k_con1_ctx1_dep4_gen1_tep8_eplb0_mtp3_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb200_glm-5-fp4_8k1k_con512_ctx1_dep4_gen1_dep32_eplb0_mtp3_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb300_glm-5-fp4_1k1k_con1_ctx1_dep2_gen1_tep4_eplb0_mtp3_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb300_glm-5-fp4_1k1k_con4096_ctx1_dep2_gen1_dep8_eplb256_mtp1_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb300_glm-5-fp4_1k1k_con512_ctx1_dep2_gen1_dep32_eplb0_mtp3_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb300_glm-5-fp4_8k1k_con1024_ctx1_dep2_gen1_dep8_eplb256_mtp1_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb300_glm-5-fp4_8k1k_con1_ctx1_dep2_gen1_tep8_eplb0_mtp3_ccb-NIXL.yamltests/scripts/perf-sanity/disaggregated/gb300_glm-5-fp4_8k1k_con512_ctx1_dep2_gen1_dep32_eplb0_mtp3_ccb-NIXL.yamltests/unittest/llmapi/test_llm_args.py
Comment on lines
+3178
to
+3189
| @pytest.mark.parametrize("architectures,model_type,expected", [ | ||
| (["GlmMoeDsaForCausalLM"], "glm_moe_dsa", "PYTHON"), | ||
| (["DeepseekV3ForCausalLM"], "deepseek_v3", None), | ||
| (["DeepseekV32ForCausalLM"], "deepseek_v32", None), | ||
| ]) | ||
| def test_preference_per_architecture(self, architectures, model_type, | ||
| expected): | ||
| from tensorrt_llm._torch.models.modeling_deepseekv3 import \ | ||
| DeepseekV3ForCausalLM | ||
| cfg = self._pretrained_config(architectures, model_type) | ||
| assert DeepseekV3ForCausalLM.get_preferred_transceiver_runtime( | ||
| cfg) == expected |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate the architecture and fallback branches.
Coverage in tests/unittest/llmapi/test_llm_args.py is insufficient: the GLM-positive row matches both fields, so deleting either production predicate still passes. Add an architecture-only GLM case with a non-GLM model_type, and a non-GLM architecture with model_type="glm_moe_dsa".
As per path instructions: “suggest concrete list file names and whether coverage is sufficient, insufficient, or needs follow-up outside the PR.”
🤖 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 3178 - 3189, Update
test_preference_per_architecture in tests/unittest/llmapi/test_llm_args.py to
isolate both production predicates: add a GLM architecture paired with a non-GLM
model_type expecting "PYTHON", and add a non-GLM architecture paired with
model_type="glm_moe_dsa" expecting the fallback result. The existing coverage is
insufficient until these cases are included; no follow-up outside this test file
is needed.
Source: Path instructions
Collaborator
|
PR_Github #59866 [ run ] triggered by Bot. Commit: |
…ansceiver test Address review: the GLM-positive case satisfied both predicates at once, so deleting either production check still passed. Add one case per predicate in isolation (GLM architecture with a non-GLM model_type, and a non-GLM architecture with model_type=glm_moe_dsa). Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Shixiaowei02
approved these changes
Jul 17, 2026
Collaborator
|
PR_Github #59866 [ run ] completed with state
|
mikeiovine
approved these changes
Jul 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…in disagg
GLM-5 and GLM-5.2 (GlmMoeDsaForCausalLM / glm_moe_dsa) share the DeepseekV3ForCausalLM implementation class; override get_preferred_transceiver_runtime() to opt GLM checkpoints into the Python (v2) transceiver, differentiated per checkpoint so DeepSeek-V3/ V3.2 keep the C++ default. The preference is adopted only when the user leaves cache_transceiver_config.transceiver_runtime at 'auto' and the effective backend is NIXL.
Set transceiver_runtime: PYTHON explicitly in the GLM-5 perf-sanity disaggregated NIXL configs, and switch the GLM-5 disagg stress-test config from backend DEFAULT to NIXL + PYTHON so the TRTLLM_USE_UCX_KVCACHE=1 fallback cannot silently revert it to the C++ transceiver.
Add unit tests covering the per-architecture preference and the end-to-end 'auto' resolution for GLM-5 on NIXL.
Summary by CodeRabbit
New Features
Configuration
Tests
Description
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.