[None][feat] Default Kimi K2.5 to KV cache manager V2 - #17125
Conversation
liji-nv
changed the title
liji-nv
changed the title
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughKimi K2.5 now returns defaults that enable the C++ KV cache manager V2. A structural test verifies the returned configuration. ChangesKimi K2.5 KV cache defaults
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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_kimi_k25.py (1)
1520-1523: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the override with the base type contract.
Line 1521 uses
Any, and the return annotation is an unparameterizeddict. Use the sameTorchLlmArgsinput annotation astensorrt_llm/_torch/models/modeling_utils.py. Parameterize the return type for this nested configuration. If Line 616 intentionally passesNone, confirm thatNoneis part of the runtime contract or update the test to use a valid argument.Suggested type alignment
- def get_model_defaults(cls, llm_args: Any) -> dict: + def get_model_defaults( + cls, llm_args: "TorchLlmArgs" + ) -> dict[str, dict[str, bool]]:🤖 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_kimi_k25.py` around lines 1520 - 1523, Update KimiForCausalLM.get_model_defaults to accept TorchLlmArgs instead of Any and annotate its nested configuration return with the appropriate parameterized mapping type, matching the base contract in modeling_utils.py. Check the call at line 616 and preserve None only if it is supported by the runtime contract; otherwise update that test or caller to provide a valid TorchLlmArgs instance.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/_torch/modeling/test_modeling_kimi_k25.py`:
- Around line 614-617: Register test_modeling_kimi_k25.py in the appropriate CI
or QA test list so
TestKimiK25AutoModelRegistration.test_defaults_to_kv_cache_manager_v2 is
included in automated coverage.
---
Nitpick comments:
In `@tensorrt_llm/_torch/models/modeling_kimi_k25.py`:
- Around line 1520-1523: Update KimiForCausalLM.get_model_defaults to accept
TorchLlmArgs instead of Any and annotate its nested configuration return with
the appropriate parameterized mapping type, matching the base contract in
modeling_utils.py. Check the call at line 616 and preserve None only if it is
supported by the runtime contract; otherwise update that test or caller to
provide a valid TorchLlmArgs instance.
🪄 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: 685036d2-737c-48dc-8acc-b83be74235f3
📒 Files selected for processing (2)
tensorrt_llm/_torch/models/modeling_kimi_k25.pytests/unittest/_torch/modeling/test_modeling_kimi_k25.py
|
PR_Github #63037 [ run ] triggered by Bot. Commit: |
|
PR_Github #63037 [ run ] completed with state
|
|
Depends on #16072 to resolve the perf issue. |
Use the C++ KV cache manager V2 as the model-specific default for Kimi K2.5. This keeps the default scoped to Kimi K2.5 without changing other Kimi implementations or the global LLM arguments. The existing auto resolution remains responsible for transport compatibility: NIXL with the Python transceiver keeps ManagerV2, while non-NIXL or C++ transceiver routes fall back to the legacy manager unless the user explicitly requests V2. Add a focused model registration test for the new default. Validation: - pre-commit run --files tensorrt_llm/_torch/models/modeling_kimi_k25.py tests/unittest/_torch/modeling/test_modeling_kimi_k25.py - git diff --check - B200 clean sqsh build (Slurm 1569850) Signed-off-by: Jin Li <59594262+liji-nv@users.noreply.github.com>
c0b198e to
81aa6b6
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 --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #64025 [ run ] triggered by Bot. Commit: |
|
PR_Github #64025 [ run ] completed with state
|
2 participants
Use the C++ KV cache manager V2 as the model-specific default for Kimi K2.5. This keeps the default scoped to Kimi K2.5 without changing other Kimi implementations or the global LLM arguments.
The existing auto resolution remains responsible for transport compatibility: NIXL with the Python transceiver keeps ManagerV2, while non-NIXL or C++ transceiver routes fall back to the legacy manager unless the user explicitly requests V2.
Add a focused model registration test for the new default.
Validation:
pre-commit run --files tensorrt_llm/_torch/models/modeling_kimi_k25.py tests/unittest/_torch/modeling/test_modeling_kimi_k25.py
git diff --check
B200 clean sqsh build (Slurm 1569850)
Dev Engineer Review
KimiK25ForConditionalGeneration.get_model_defaultsto select the C++ KV cache manager V2 for Kimi K2.5.git diff --check, and a clean B200 build.QA Engineer Review
get_model_defaults(None)and verifies the Kimi K2.5 default.tests/integration/test_lists/,test-db/, orqa/entries changed.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.