[None][feat] BREAKING Support saving last N turns in per-conversation policy - #16883
Conversation
5df809d to
c53368e
Compare
Signed-off-by: Jiagan Cheng <jiaganc@nvidia.com>
65e1b71 to
59b867a
Compare
|
/bot run |
|
PR_Github #62933 [ run ] triggered by Bot. Commit: |
|
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:
WalkthroughThe change adds nested ChangesBlock reuse configuration and conversation retention
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant KVCacheManagerV2
participant ConversationManager
participant DropHandle
Request->>KVCacheManagerV2: Start conversation request
KVCacheManagerV2->>ConversationManager: Pass max_num_turns
ConversationManager->>DropHandle: Append planned drop handle
ConversationManager->>DropHandle: Drop oldest handle after the limit
KVCacheManagerV2-->>Request: Complete cache reuse flow
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@tensorrt_llm/llmapi/llm_args.py`:
- Around line 3621-3649: Update stale block_reuse_policy references: in
test_mamba_cache_manager.py, pass the policy through KvCacheConfig’s
block_reuse_config using BlockReuseConfig(block_reuse_policy=...) instead of
passing the field directly; in telemetry.md, access it via
kv_cache_config.block_reuse_config.block_reuse_policy.
🪄 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: e7fd2494-2650-4401-a6c8-cc06c53c6e64
📒 Files selected for processing (11)
examples/disaggregated/slurm/cache_transceiver_test/run_cache_transceiver_test.pytensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.pytensorrt_llm/_torch/pyexecutor/mamba_cache_manager.pytensorrt_llm/llmapi/__init__.pytensorrt_llm/llmapi/llm_args.pytensorrt_llm/usage/llm_args_golden_manifest.jsontests/unittest/_torch/executor/test_kv_cache_manager_v2.pytests/unittest/disaggregated/test_cache_transceiver_single_process.pytests/unittest/disaggregated/test_kv_transfer.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_stats_behavior.pytests/unittest/llmapi/test_llm_args.py
Signed-off-by: Jiagan Cheng <jiaganc@nvidia.com>
|
/bot run |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/_torch/executor/test_mamba_cache_manager.py (1)
1651-1684: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose
max_num_turnsin the test helper.The generic
KVCacheManagerV2test coversmax_num_turns=2, but noMambaHybridCacheManagerV2test covers multi-turn retention or eviction. Add the parameter, pass it toBlockReuseConfig, and add hybrid coverage, including the five-turn case.Test coverage verdict: insufficient.
🤖 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/_torch/executor/test_mamba_cache_manager.py` around lines 1651 - 1684, Update _build_v2_hybrid_with_mamba_layer to accept a max_num_turns parameter and pass it into BlockReuseConfig. Add MambaHybridCacheManagerV2 tests covering multi-turn retention and eviction, including the five-turn case, matching the existing KVCacheManagerV2 max_num_turns=2 coverage.
🤖 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 `@tests/unittest/_torch/executor/test_mamba_cache_manager.py`:
- Around line 1651-1684: Update _build_v2_hybrid_with_mamba_layer to accept a
max_num_turns parameter and pass it into BlockReuseConfig. Add
MambaHybridCacheManagerV2 tests covering multi-turn retention and eviction,
including the five-turn case, matching the existing KVCacheManagerV2
max_num_turns=2 coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ba22a143-156c-4b0c-84da-c197c2383f09
📒 Files selected for processing (2)
docs/source/developer-guide/telemetry.mdtests/unittest/_torch/executor/test_mamba_cache_manager.py
|
PR_Github #62940 [ run ] triggered by Bot. Commit: |
|
PR_Github #62933 [ run ] completed with state |
|
/bot run |
|
PR_Github #63282 [ run ] triggered by Bot. Commit: |
|
PR_Github #63282 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #63319 [ run ] triggered by Bot. Commit: |
|
PR_Github #63319 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #63358 [ run ] triggered by Bot. Commit: |
|
PR_Github #63358 [ run ] completed with state |
yuanjingx87
left a comment
There was a problem hiding this comment.
Approved on oss compliance perspective.
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/unittest/_torch/executor/test_mamba_cache_manager.py`:
- Around line 2041-2089: Add a probe for request_b after its initial processing
and before request_c in
test_v2_hybrid_retains_configured_number_of_conversation_turns. Capture and
assert its prepopulated_prompt_len and Mamba state index match the expected
reused second-turn state, then free the probe resources before continuing the
existing eviction checks.
🪄 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: 7b34f034-b3e5-4aaa-8bb1-61da5f91761c
📒 Files selected for processing (9)
docs/source/developer-guide/telemetry.mdtensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.pytensorrt_llm/_torch/pyexecutor/mamba_cache_manager.pytensorrt_llm/llmapi/llm_args.pytensorrt_llm/usage/llm_args_golden_manifest.jsontests/unittest/_torch/executor/test_kv_cache_manager_v2.pytests/unittest/_torch/executor/test_mamba_cache_manager.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_stats_behavior.pytests/unittest/llmapi/test_llm_args.py
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/source/developer-guide/telemetry.md
- tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_stats_behavior.py
- tensorrt_llm/_torch/pyexecutor/mamba_cache_manager.py
- tests/unittest/_torch/executor/test_kv_cache_manager_v2.py
- tests/unittest/llmapi/test_llm_args.py
- tensorrt_llm/usage/llm_args_golden_manifest.json
- tensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.py
|
/bot run --disable-fail-fast |
|
PR_Github #63643 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #63684 [ run ] triggered by Bot. Commit: |
|
PR_Github #63643 [ run ] completed with state |
|
PR_Github #63684 [ run ] completed with state
|
1cc1948 to
8f8ad1a
Compare
Dev Engineer Review
BlockReuseConfigunderKvCacheConfig.max_num_turns, with a default value of1.per_conversationreuse now retains the configured number of completed turns.kv_cache_config.block_reuse_policywithkv_cache_config.block_reuse_config.policy.QA Engineer Review
tests/unittest/_torch/executor/test_kv_cache_manager_v2.py.test_per_conversation_policy_retains_configured_number_of_turns.managerfixture to acceptmax_num_turns.max_num_turnsfixture.tests/unittest/disaggregated/test_cache_transceiver_single_process.pytests/unittest/disaggregated/test_kv_transfer.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_stats_behavior.pytests/unittest/llmapi/test_llm_args.pytests/unittest/_torch/executor/test_mamba_cache_manager.pyDescription
Conversation-scoped KV-cache reuse currently retains only one completed turn, which prevents reuse when a conversation rewinds to an older turn.
This change:
BlockReuseConfigunderKvCacheConfig;block_reuse_policyinto that config;max_num_turns, a positive integer that defaults to1;per_conversationreuse.The default remains one retained turn, so runtime behavior is unchanged unless
max_num_turnsis configured. The configuration shape changes from:to:
Test Coverage
277527.all_reusable,per_request,per_conversationwith one turn, andper_conversationwith five turns. Each run processed more than 7,000 main-conversation requests before cancellation.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.