Skip to content

[None][chore] Cleanup kv cache manager - #18538

Merged
Funatiq merged 5 commits into
NVIDIA:mainfrom
Funatiq:dev/cleanup_kv_cache_manager
Sep 7, 2026
Merged

Funatiq merged 5 commits into
NVIDIA:mainfrom
Funatiq:dev/cleanup_kv_cache_manager

Conversation

@Funatiq

@Funatiq Funatiq commented Sep 1, 2026 •

Copy link
Copy Markdown
Collaborator

Dev Engineer Review

  • Removed obsolete KV cache manager APIs and implementations from KVCacheManager, BlockManager, and WindowBlockManager.
  • Removed KvCacheConfig::kDefaultGpuMemFraction.
  • Retained active linear-attention block-copy functionality.
  • Removed the related BlockManagerTestWindowSizeToShare unit test.
  • API and implementation declarations remain consistent based on the changed files.
  • No configuration or test-list files changed.
  • CI reported failed or unstable main pipelines. The failures require follow-up before merge.

QA Engineer Review

  • Removed test: BlockManagerTestWindowSizeToShare.
  • The test covered single-window, variable-window, cache-size-weighted, and equal-contribution window-share calculations.
  • No tests/integration/test_lists/ entries were changed.
  • Coverage status for the removed test is unavailable.
  • Verdict: needs follow-up.

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-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.

@Funatiq Funatiq changed the title Dev/cleanup kv cache manager [None][chore] Cleanup kv cache manager Sep 1, 2026
@Funatiq

Funatiq commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70713 [ run ] triggered by Bot. Commit: 95be14d Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70713 [ run ] completed with state SUCCESS. Commit: 95be14d
/LLM/main/L0_MergeRequest_PR pipeline #57906 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@Funatiq
Funatiq force-pushed the dev/cleanup_kv_cache_manager branch from 95be14d to 61aaf60 Compare September 2, 2026 10:20
Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
- Removed allocateBlock, setOffsets, and updateCacheBlockOffsetsAtIdx methods from kvCacheManager.h and kvCacheManager.cpp.

Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
@Funatiq
Funatiq force-pushed the dev/cleanup_kv_cache_manager branch from 61aaf60 to dae70b8 Compare September 2, 2026 10:21
@Funatiq

Funatiq commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70976 [ run ] triggered by Bot. Commit: dae70b8 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70976 [ run ] completed with state SUCCESS. Commit: dae70b8
/LLM/main/L0_MergeRequest_PR pipeline #58139 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@Funatiq

Funatiq commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71003 [ run ] triggered by Bot. Commit: dae70b8 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71003 [ run ] completed with state SUCCESS. Commit: dae70b8
/LLM/main/L0_MergeRequest_PR pipeline #58160 completed with status: 'UNSTABLE'

CI Report

⚠️ Multi-GPU Label Required:
Multi-GPU tests require the ci: full pre-merge approved label on this PR. Ask a member of NVIDIA/trt-llm-ci-approvers to add the label, then re-trigger CI with the same bot command (no rebase needed).

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

Link to invocation

@Funatiq
Funatiq marked this pull request as ready for review September 4, 2026 08:07
@Funatiq
Funatiq requested review from a team as code owners September 4, 2026 08:07
@coderabbitai

coderabbitai Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6d3f7f57-ece7-40ed-92ac-0fe058266fbc

📥 Commits

Reviewing files that changed from the base of the PR and between 24a5c64 and dae70b8.

📒 Files selected for processing (4)
  • cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h
  • cpp/include/tensorrt_llm/executor/executor.h
  • cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp
  • cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp
💤 Files with no reviewable changes (4)
  • cpp/include/tensorrt_llm/executor/executor.h
  • cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp
  • cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h
  • cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The change removes obsolete KV cache allocation, replacement, offset, window-share, and free-memory helpers from public interfaces and implementations. It also removes the default GPU memory fraction constant and its related window-share test.

Changes

KV cache API cleanup

Layer / File(s) Summary
Public API contract cleanup
cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h, cpp/include/tensorrt_llm/executor/executor.h
Removed obsolete KVCacheManager helper declarations and KvCacheConfig::kDefaultGpuMemFraction. Retained copyLinearAttentionBlock.
Implementation helper removal
cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp
Removed sequence traversal, window-share, allocation, replacement, indexed offset, forwarding, and free-memory helper implementations.
Obsolete share-calculation test removal
cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp
Removed BlockManagerTestWindowSizeToShare and its window-share assertions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to dae70

This change removes obsolete KV cache manager APIs, implementations, and a related test. No concrete unresolved merge-blocking behavior risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description contains the repository template, but the Description and Test Coverage sections are empty. It also does not document the removed public APIs or the required breaking-change handling. Add a concise explanation of the removed KV cache manager APIs and the reason for their removal. List the relevant tests and CI results. Because the changes remove public APIs, add the appropriate api-breaking label and include BREAKING in …
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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.
Title check ✅ Passed The title clearly identifies a KV cache manager cleanup and follows the required [None][chore] format.
Full details: Description check

Resolution

Add a concise explanation of the removed KV cache manager APIs and the reason for their removal. List the relevant tests and CI results. Because the changes remove public APIs, add the appropriate api-breaking label and include BREAKING in the pull request title if required by repository policy.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@Funatiq

Funatiq commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71912 [ run ] triggered by Bot. Commit: dae70b8 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71912 [ run ] completed with state SUCCESS. Commit: dae70b8
/LLM/main/L0_MergeRequest_PR pipeline #58981 completed with status: 'SUCCESS'

CI Report

Link to invocation

@Funatiq
Funatiq merged commit ebf619a into NVIDIA:main Sep 7, 2026
14 checks passed
@Funatiq
Funatiq deleted the dev/cleanup_kv_cache_manager branch September 7, 2026 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants