[None][feat] KVCacheManagerV2 C++ translation#14047
Conversation
|
/bot run --disable-fail-fast --stage-list "PerfSanity" |
5bd984d to
456cfa8
Compare
4f4dd8d to
0802c6c
Compare
4c24d8a to
44c797c
Compare
b4592c1 to
5a1fa77
Compare
fd7d167 to
99bfc50
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.cpp`:
- Around line 159-163: Make CopyEngine::getStagingManager thread-safe by
ensuring mStagingManager is initialized eagerly or guarded with std::call_once;
preserve lazy initialization only if the one-time synchronization covers both
creation and publication before returning the manager.
- Around line 205-217: Update CopyEngine::transfer to return immediately when
numBytes is zero, before dispatching any direct or two-hop transfer, while
preserving existing behavior for nonzero transfers and task lists.
In `@cpp/tensorrt_llm/runtime/utils/mpiUtils.cpp`:
- Around line 600-606: Reflow the explanatory comment above the static MpiComm
teardown logic to keep every line within the 120-character limit, without
changing its meaning or the surrounding destructor/communicator-free behavior.
In `@docs/source/features/kvcache.md`:
- Line 83: Update the block-key hash requirement in the cache documentation to
distinguish digest size from generic collision resistance: state that SHA-256
has a 256-bit digest but approximately 128-bit collision strength, and require a
512-bit hash only if the intended requirement is 256-bit collision resistance.
Preserve the existing salt-isolation and digest-equality behavior.
In `@tensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.py`:
- Around line 1791-1801: Limit the KeyError/IndexError handling in the
role-buffer lookup to only the initial retrieval that determines whether
INDEX_KEY is registered. Move stride, upper-bound, and converter access outside
that try block so their IndexError failures propagate instead of returning None,
while preserving the missing-role-to-None behavior for both backends.
In `@tensorrt_llm/runtime/kv_cache_manager_v2/_storage_manager.py`:
- Line 921: Replace the assert validating max_util_for_resume with an explicit
runtime check in the storage manager initialization flow. Raise ValueError when
max_util_for_resume is less than or equal to 0 or greater than 1, preserving
acceptance of values in the range (0, 1].
🪄 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: cf471610-335a-44e6-9c6a-00a5f774f9a5
📒 Files selected for processing (77)
.github/CODEOWNERS.pre-commit-config.yaml3rdparty/sha256/LICENSE3rdparty/sha256/README.md3rdparty/sha256/attributes.h3rdparty/sha256/sha256.cpp3rdparty/sha256/sha256.h3rdparty/sha256/sha256_arm_shani.cpp3rdparty/sha256/sha256_endian.h3rdparty/sha256/sha256_x86_shani.cppcpp/tensorrt_llm/batch_manager/CMakeLists.txtcpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.cucpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/CMakeLists.txtcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventSink.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/exceptions.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/movingAverage.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/pendingStats.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/stats.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/math.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/sharedPtr.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/typedIndex.hcpp/tensorrt_llm/nanobind/CMakeLists.txtcpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.cppcpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.hcpp/tensorrt_llm/nanobind/bindings.cppcpp/tensorrt_llm/runtime/utils/mpiUtils.cppcpp/tests/unit_tests/batch_manager/CMakeLists.txtcpp/tests/unit_tests/batch_manager/kvCacheManagerV2HostMemTest.cppcpp/tests/unit_tests/batch_manager/kvCacheManagerV2StatsTest.cppcpp/tests/unit_tests/batch_manager/kvCacheManagerV2TypedIndexTest.cppdocs/source/features/kvcache.mdscripts/build_wheel.pyscripts/nanobind_stubgen.patternstensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.pytensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/runtime/kv_cache_manager_v2/__init__.pytensorrt_llm/runtime/kv_cache_manager_v2/__init__.pyitensorrt_llm/runtime/kv_cache_manager_v2/_block_radix_tree.pytensorrt_llm/runtime/kv_cache_manager_v2/_cache_key.pytensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.pytensorrt_llm/runtime/kv_cache_manager_v2/_introspection.pytensorrt_llm/runtime/kv_cache_manager_v2/_storage_manager.py
🚧 Files skipped from review as they are similar to previous changes (66)
- 3rdparty/sha256/LICENSE
- 3rdparty/sha256/attributes.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.h
- 3rdparty/sha256/sha256_endian.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/CMakeLists.txt
- cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.h
- cpp/tensorrt_llm/batch_manager/CMakeLists.txt
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/movingAverage.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventSink.h
- cpp/tensorrt_llm/nanobind/CMakeLists.txt
- cpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.cpp
- 3rdparty/sha256/sha256.h
- cpp/tests/unit_tests/batch_manager/CMakeLists.txt
- cpp/tensorrt_llm/nanobind/bindings.cpp
- .pre-commit-config.yaml
- tensorrt_llm/_torch/pyexecutor/py_executor.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.cpp
- tensorrt_llm/runtime/kv_cache_manager_v2/_introspection.py
- scripts/nanobind_stubgen.patterns
- tensorrt_llm/runtime/kv_cache_manager_v2/init.pyi
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.h
- tensorrt_llm/runtime/kv_cache_manager_v2/_cache_key.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/exceptions.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.cpp
- 3rdparty/sha256/sha256_arm_shani.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.h
- tensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/pendingStats.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.cpp
- tensorrt_llm/runtime/kv_cache_manager_v2/_block_radix_tree.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/stats.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.h
- scripts/build_wheel.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/math.h
- 3rdparty/sha256/README.md
- 3rdparty/sha256/sha256.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/sharedPtr.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.cpp
- tensorrt_llm/runtime/kv_cache_manager_v2/init.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.h
- 3rdparty/sha256/sha256_x86_shani.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/typedIndex.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.cpp
fda1959 to
efe3e0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.h (1)
307-341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing braces on single-statement control-flow bodies; duplicated dedup logic.
Lines 314-315, 319-320, 332-333, and 337-340 use unbraced single-statement bodies for
if/for. As per coding guidelines,**/*.{cpp,cc,cxx,h,hpp,cu,cuh}: "use Allman braces, braced control-flow bodies". Separately,streamWaitEventsandsynchronizeAllduplicate the same sort/unique dedup sequence; consider factoring a small shared helper.♻️ Proposed brace fix (illustrative for one instance)
for (auto const* ev : events) { - if (ev && !ev->isClosed()) - handles.push_back(ev->handle()); + if (ev && !ev->isClosed()) + { + handles.push_back(ev->handle()); + } }🤖 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 `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.h` around lines 307 - 341, Update streamWaitEvents and synchronizeAll to use Allman-style braces for every if and for body, including the null/closed-event checks, handle waits/synchronization, and event closing loop. Also factor their duplicated handle sort/unique deduplication into a small shared helper and reuse it from both functions.Source: Coding guidelines
cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.h (1)
310-330: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake
mRootsmutable or drop theconst_castindrainPendingRootErases()
drainPendingRootErases() constmutatesmRootsthroughconst_cast, which breaks the no-cv-removal guideline. If this cleanup is meant to stay on a const path, markmRootsmutable; otherwise make the helper non-const.🤖 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 `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.h` around lines 310 - 330, Resolve the const-correctness issue in drainPendingRootErases: either mark mRoots mutable so the const cleanup can erase pending roots without const_cast, or remove const qualification from drainPendingRootErases and its callers. Preserve the existing deferred root-erasure behavior and eliminate the const_cast.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 `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.h`:
- Around line 140-152: Update PoolItem get() so mOutstandingCount is incremented
only after mCreateFn() or popFront() successfully returns an item, ensuring
creation exceptions leave the counter unchanged. Preserve the existing deleter
selection and returned PoolItem behavior for successful allocations.
- Around line 39-71: Update the destructor of FuncGuard so exceptions from
mFunc(), including failures propagated by TemporaryCudaStream::enter() through
mStream.recordEvent(), are not forced into std::terminate; declare ~FuncGuard()
noexcept(false) while preserving the existing active-guard behavior.
- Around line 108-201: Add mutex protection to SimplePool’s shared state:
synchronize all accesses and mutations of mItems and mOutstandingCount in get(),
put(), clear(), outstandingCount(), cachedCount(), and the
constructor/destructor cleanup path as needed. Use scoped locking that does not
hold the mutex while invoking mCreateFn or mDestroyFn, and preserve the existing
pool size and ownership behavior.
In `@tensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.py`:
- Line 1364: Update both f-strings in the KV-cache logging code to format role
with the explicit conversion syntax `{role!s}` instead of calling `str(role)`,
including the occurrence near lifecycle_id and the matching occurrence in the
adjacent message.
---
Nitpick comments:
In `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.h`:
- Around line 310-330: Resolve the const-correctness issue in
drainPendingRootErases: either mark mRoots mutable so the const cleanup can
erase pending roots without const_cast, or remove const qualification from
drainPendingRootErases and its callers. Preserve the existing deferred
root-erasure behavior and eliminate the const_cast.
In `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.h`:
- Around line 307-341: Update streamWaitEvents and synchronizeAll to use
Allman-style braces for every if and for body, including the null/closed-event
checks, handle waits/synchronization, and event closing loop. Also factor their
duplicated handle sort/unique deduplication into a small shared helper and reuse
it from both functions.
🪄 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: 658f6547-404e-443a-b974-bb319f39217b
📒 Files selected for processing (83)
.github/CODEOWNERS.pre-commit-config.yaml3rdparty/sha256/LICENSE3rdparty/sha256/README.md3rdparty/sha256/attributes.h3rdparty/sha256/sha256.cpp3rdparty/sha256/sha256.h3rdparty/sha256/sha256_arm_shani.cpp3rdparty/sha256/sha256_endian.h3rdparty/sha256/sha256_x86_shani.cppcpp/tensorrt_llm/batch_manager/CMakeLists.txtcpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.cucpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/CMakeLists.txtcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventSink.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/exceptions.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/movingAverage.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/pendingStats.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/stats.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/math.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/sharedPtr.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/typedIndex.hcpp/tensorrt_llm/nanobind/CMakeLists.txtcpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.cppcpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.hcpp/tensorrt_llm/nanobind/bindings.cppcpp/tensorrt_llm/runtime/utils/mpiUtils.cppcpp/tests/unit_tests/batch_manager/CMakeLists.txtcpp/tests/unit_tests/batch_manager/kvCacheManagerV2HostMemTest.cppcpp/tests/unit_tests/batch_manager/kvCacheManagerV2StatsTest.cppcpp/tests/unit_tests/batch_manager/kvCacheManagerV2TypedIndexTest.cppdocs/source/features/kvcache.mdscripts/build_wheel.pyscripts/nanobind_stubgen.patternstensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.pytensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/runtime/kv_cache_manager_v2/__init__.pytensorrt_llm/runtime/kv_cache_manager_v2/__init__.pyitensorrt_llm/runtime/kv_cache_manager_v2/_block_radix_tree.pytensorrt_llm/runtime/kv_cache_manager_v2/_cache_key.pytensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.pytensorrt_llm/runtime/kv_cache_manager_v2/_introspection.pytensorrt_llm/runtime/kv_cache_manager_v2/_storage_manager.pytests/unittest/_torch/executor/test_kv_pool_rebalance.pytests/unittest/disaggregated/test_router.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_event_manager.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_salting.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_stats_api.py
🚧 Files skipped from review as they are similar to previous changes (73)
- 3rdparty/sha256/LICENSE
- scripts/nanobind_stubgen.patterns
- tests/unittest/_torch/executor/test_kv_pool_rebalance.py
- cpp/tensorrt_llm/batch_manager/CMakeLists.txt
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.h
- docs/source/features/kvcache.md
- cpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.cu
- tensorrt_llm/runtime/kv_cache_manager_v2/init.pyi
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/CMakeLists.txt
- cpp/tensorrt_llm/nanobind/CMakeLists.txt
- cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.h
- cpp/tensorrt_llm/nanobind/bindings.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/movingAverage.h
- cpp/tests/unit_tests/batch_manager/CMakeLists.txt
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.cpp
- cpp/tensorrt_llm/runtime/utils/mpiUtils.cpp
- 3rdparty/sha256/sha256.h
- 3rdparty/sha256/sha256_endian.h
- 3rdparty/sha256/attributes.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventSink.h
- cpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.h
- tensorrt_llm/runtime/kv_cache_manager_v2/_storage_manager.py
- tensorrt_llm/_torch/pyexecutor/py_executor.py
- tensorrt_llm/runtime/kv_cache_manager_v2/_cache_key.py
- tests/unittest/disaggregated/test_router.py
- .github/CODEOWNERS
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.cpp
- 3rdparty/sha256/sha256.cpp
- tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_salting.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/stats.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.h
- 3rdparty/sha256/sha256_x86_shani.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.cpp
- 3rdparty/sha256/sha256_arm_shani.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/exceptions.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/sharedPtr.h
- 3rdparty/sha256/README.md
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/pendingStats.h
- tensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.py
- tensorrt_llm/runtime/kv_cache_manager_v2/_block_radix_tree.py
- tensorrt_llm/runtime/kv_cache_manager_v2/init.py
- tensorrt_llm/runtime/kv_cache_manager_v2/_introspection.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.h
- scripts/build_wheel.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/math.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.cpp
- tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_stats_api.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/typedIndex.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.cpp
- tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.cpp
efe3e0a to
e1a92e6
Compare
e1a92e6 to
2e1f0f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.cpp (1)
398-407: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroad
catch (std::exception)swallows all failures (andeis unused).This converts every exception — including
std::bad_allocand logic errors that likely indicate a real bug — into a silentresize() == false, making failures hard to diagnose. Consider narrowing to the expected capacity-adjustment exception(s) and/or logging before returningfalse.🤖 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 `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.cpp` around lines 398 - 407, Update the exception handling in the resize flow around _adjustLevel so it catches only the expected capacity-adjustment exception types, allowing allocation and logic errors to propagate. If a fallback catch remains necessary, log the caught exception details before returning false, and remove the unused exception variable otherwise.cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.cpp (2)
28-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace pool-size magic literals with named constants.
Line 37 and Line 56 hard-code pool capacities. Define
kEventPoolInitialSizeandkStreamPoolInitialSizeinstead.Proposed fix
+constexpr std::size_t kEventPoolInitialSize = 1024; +constexpr std::size_t kStreamPoolInitialSize = 128; + CudaEventPool::CudaEventPool() ... - /*initSize=*/1024) + /*initSize=*/kEventPoolInitialSize) ... - /*initSize=*/128) + /*initSize=*/kStreamPoolInitialSize)Also applies to: 47-56
🤖 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 `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.cpp` around lines 28 - 37, Replace the hard-coded initial pool capacities in CudaEventPool and the corresponding stream pool constructor with named constants kEventPoolInitialSize and kStreamPoolInitialSize, defining them in the appropriate scope and preserving the existing capacity values.Source: Coding guidelines
162-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse braced control-flow bodies in
mergeEvents().Line 168, Line 171, Line 173, and Line 178 omit braces, contrary to the required Allman/braced-body style.
Proposed fix
for (auto& ev : events) { - if (!ev.isClosed()) + if (!ev.isClosed()) + { live.push_back(&ev); + } } - if (live.empty()) + if (live.empty()) + { return CachedCudaEvent::makeNull(); - if (live.size() == 1) + } + if (live.size() == 1) + { return std::move(*live[0]); + } ... - for (auto* ev : live) + for (auto* ev : live) + { priors.push_back(ev); + }🤖 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 `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.cpp` around lines 162 - 180, Update mergeEvents() to wrap every single-statement if and loop body in braces, including the live-event filtering, empty and single-event returns, and priors population, preserving the existing control flow and Allman formatting style.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 `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.cpp`:
- Around line 551-555: Update the CUDA memory query block to wrap both
cuCtxGetDevice and cuDeviceTotalMem with cuCheck, preserving the existing dev
and totalGpuMem flow while propagating either driver API failure instead of
continuing with an invalid zero value.
- Around line 685-687: Update the physMemSize calculation near kPageSize to
compute the quota ratio before calling std::log2, and handle a zero ratio
explicitly by using the lower exponent bound. Only call std::log2 when the ratio
is nonzero, while preserving the existing min/max clamping for positive ratios.
---
Nitpick comments:
In `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.cpp`:
- Around line 398-407: Update the exception handling in the resize flow around
_adjustLevel so it catches only the expected capacity-adjustment exception
types, allowing allocation and logic errors to propagate. If a fallback catch
remains necessary, log the caught exception details before returning false, and
remove the unused exception variable otherwise.
In `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.cpp`:
- Around line 28-37: Replace the hard-coded initial pool capacities in
CudaEventPool and the corresponding stream pool constructor with named constants
kEventPoolInitialSize and kStreamPoolInitialSize, defining them in the
appropriate scope and preserving the existing capacity values.
- Around line 162-180: Update mergeEvents() to wrap every single-statement if
and loop body in braces, including the live-event filtering, empty and
single-event returns, and priors population, preserving the existing control
flow and Allman formatting style.
🪄 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: f4a32390-4114-4c91-bb84-11e4cb2b9b2f
📒 Files selected for processing (84)
.github/CODEOWNERS.pre-commit-config.yaml3rdparty/sha256/LICENSE3rdparty/sha256/README.md3rdparty/sha256/attributes.h3rdparty/sha256/sha256.cpp3rdparty/sha256/sha256.h3rdparty/sha256/sha256_arm_shani.cpp3rdparty/sha256/sha256_endian.h3rdparty/sha256/sha256_x86_shani.cppcpp/tensorrt_llm/batch_manager/CMakeLists.txtcpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.cucpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/CMakeLists.txtcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventSink.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/exceptions.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/movingAverage.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/pendingStats.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/stats.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/math.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/sharedPtr.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/typedIndex.hcpp/tensorrt_llm/nanobind/CMakeLists.txtcpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.cppcpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.hcpp/tensorrt_llm/nanobind/bindings.cppcpp/tensorrt_llm/runtime/utils/mpiUtils.cppcpp/tests/unit_tests/batch_manager/CMakeLists.txtcpp/tests/unit_tests/batch_manager/kvCacheManagerV2HostMemTest.cppcpp/tests/unit_tests/batch_manager/kvCacheManagerV2StatsTest.cppcpp/tests/unit_tests/batch_manager/kvCacheManagerV2TypedIndexTest.cppdocs/source/features/kvcache.mdscripts/build_wheel.pyscripts/nanobind_stubgen.patternstensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.pytensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/runtime/kv_cache_manager_v2/__init__.pytensorrt_llm/runtime/kv_cache_manager_v2/__init__.pyitensorrt_llm/runtime/kv_cache_manager_v2/_block_radix_tree.pytensorrt_llm/runtime/kv_cache_manager_v2/_cache_key.pytensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.pytensorrt_llm/runtime/kv_cache_manager_v2/_introspection.pytensorrt_llm/runtime/kv_cache_manager_v2/_storage_manager.pytests/integration/test_lists/waives.txttests/unittest/_torch/executor/test_kv_pool_rebalance.pytests/unittest/disaggregated/test_router.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_event_manager.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_salting.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_stats_api.py
🚧 Files skipped from review as they are similar to previous changes (69)
- 3rdparty/sha256/LICENSE
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/CMakeLists.txt
- .pre-commit-config.yaml
- scripts/nanobind_stubgen.patterns
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.cpp
- tests/unittest/_torch/executor/test_kv_pool_rebalance.py
- cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.h
- 3rdparty/sha256/sha256.h
- 3rdparty/sha256/sha256_endian.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/movingAverage.h
- tensorrt_llm/_torch/pyexecutor/py_executor.py
- 3rdparty/sha256/attributes.h
- docs/source/features/kvcache.md
- cpp/tensorrt_llm/nanobind/bindings.cpp
- cpp/tests/unit_tests/batch_manager/CMakeLists.txt
- cpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.h
- .github/CODEOWNERS
- tensorrt_llm/runtime/kv_cache_manager_v2/init.pyi
- tests/unittest/disaggregated/test_router.py
- cpp/tensorrt_llm/batch_manager/CMakeLists.txt
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.cpp
- cpp/tensorrt_llm/nanobind/CMakeLists.txt
- cpp/tensorrt_llm/runtime/utils/mpiUtils.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.cpp
- 3rdparty/sha256/sha256.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/sharedPtr.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h
- tensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/stats.h
- tensorrt_llm/runtime/kv_cache_manager_v2/_cache_key.py
- tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_stats_api.py
- 3rdparty/sha256/sha256_x86_shani.cpp
- tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_salting.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.h
- 3rdparty/sha256/README.md
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/pendingStats.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/exceptions.h
- tensorrt_llm/runtime/kv_cache_manager_v2/_block_radix_tree.py
- tensorrt_llm/runtime/kv_cache_manager_v2/_introspection.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/math.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.h
- 3rdparty/sha256/sha256_arm_shani.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.h
- tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.cpp
- tensorrt_llm/runtime/kv_cache_manager_v2/init.py
- scripts/build_wheel.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventSink.h
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h (1)
33-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Doxygen documentation for the new public interfaces.
cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h#L33-L40: DocumentCopyTaskfields and address-direction semantics with Doxygen.cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h#L69-L104: Document staging-buffer ownership, locking, constructor arguments, and stream lifetime.cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h#L110-L158: Document allocation bounds and concurrency guarantees.cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h#L166-L197: Document transfer preconditions, async behavior, singleton lifetime, and shutdown requirements.As per coding guidelines, “use Doxygen comments for new interfaces.”
🤖 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 `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h` around lines 33 - 40, Add Doxygen documentation to the public interfaces in cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h: document CopyTask fields and source/destination address semantics at lines 33-40; document staging-buffer ownership, locking, constructor arguments, and stream lifetime at lines 69-104; document allocation bounds and concurrency guarantees at lines 110-158; and document transfer preconditions, asynchronous behavior, singleton lifetime, and shutdown requirements at lines 166-197.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 `@cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h`:
- Around line 33-40: Add Doxygen documentation to the public interfaces in
cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.h: document
CopyTask fields and source/destination address semantics at lines 33-40;
document staging-buffer ownership, locking, constructor arguments, and stream
lifetime at lines 69-104; document allocation bounds and concurrency guarantees
at lines 110-158; and document transfer preconditions, asynchronous behavior,
singleton lifetime, and shutdown requirements at lines 166-197.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3498fc47-b3d6-4d5a-b50e-f49d7393f756
📒 Files selected for processing (83)
.github/CODEOWNERS.pre-commit-config.yaml3rdparty/sha256/LICENSE3rdparty/sha256/README.md3rdparty/sha256/attributes.h3rdparty/sha256/sha256.cpp3rdparty/sha256/sha256.h3rdparty/sha256/sha256_arm_shani.cpp3rdparty/sha256/sha256_endian.h3rdparty/sha256/sha256_x86_shani.cppcpp/tensorrt_llm/batch_manager/CMakeLists.txtcpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.cucpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/CMakeLists.txtcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/copyEngine.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventSink.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/exceptions.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/movingAverage.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/pendingStats.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/stats.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.cppcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/math.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/sharedPtr.hcpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/typedIndex.hcpp/tensorrt_llm/nanobind/CMakeLists.txtcpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.cppcpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.hcpp/tensorrt_llm/nanobind/bindings.cppcpp/tensorrt_llm/runtime/utils/mpiUtils.cppcpp/tests/unit_tests/batch_manager/CMakeLists.txtcpp/tests/unit_tests/batch_manager/kvCacheManagerV2HostMemTest.cppcpp/tests/unit_tests/batch_manager/kvCacheManagerV2StatsTest.cppcpp/tests/unit_tests/batch_manager/kvCacheManagerV2TypedIndexTest.cppdocs/source/features/kvcache.mdscripts/build_wheel.pyscripts/nanobind_stubgen.patternstensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.pytensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/runtime/kv_cache_manager_v2/__init__.pytensorrt_llm/runtime/kv_cache_manager_v2/__init__.pyitensorrt_llm/runtime/kv_cache_manager_v2/_block_radix_tree.pytensorrt_llm/runtime/kv_cache_manager_v2/_cache_key.pytensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.pytensorrt_llm/runtime/kv_cache_manager_v2/_introspection.pytensorrt_llm/runtime/kv_cache_manager_v2/_storage_manager.pytests/unittest/_torch/executor/test_kv_pool_rebalance.pytests/unittest/disaggregated/test_router.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_event_manager.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_salting.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_stats_api.py
🚧 Files skipped from review as they are similar to previous changes (70)
- scripts/nanobind_stubgen.patterns
- 3rdparty/sha256/LICENSE
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.cpp
- 3rdparty/sha256/sha256.h
- cpp/tests/unit_tests/batch_manager/CMakeLists.txt
- tensorrt_llm/_torch/pyexecutor/py_executor.py
- 3rdparty/sha256/attributes.h
- 3rdparty/sha256/sha256_endian.h
- cpp/tensorrt_llm/nanobind/CMakeLists.txt
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventSink.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.cpp
- cpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.h
- .github/CODEOWNERS
- cpp/tensorrt_llm/batch_manager/CMakeLists.txt
- cpp/tensorrt_llm/batch_manager/kvCacheManagerV2Utils.cu
- tests/unittest/disaggregated/test_router.py
- 3rdparty/sha256/README.md
- cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.h
- cpp/tensorrt_llm/runtime/utils/mpiUtils.cpp
- tests/unittest/_torch/executor/test_kv_pool_rebalance.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/CMakeLists.txt
- tensorrt_llm/runtime/kv_cache_manager_v2/_cache_key.py
- cpp/tensorrt_llm/nanobind/bindings.cpp
- tensorrt_llm/runtime/kv_cache_manager_v2/_storage_manager.py
- tensorrt_llm/runtime/kv_cache_manager_v2/init.pyi
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/stats.h
- 3rdparty/sha256/sha256_x86_shani.cpp
- 3rdparty/sha256/sha256.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.h
- docs/source/features/kvcache.md
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/movingAverage.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/exceptions.h
- scripts/build_wheel.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/pendingStats.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/introspection.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/common.h
- tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_salting.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/page.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/sharedPtr.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/hostMem.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/config.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/config.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/math.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/cudaVirtMem.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/evictionController.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/eventManager.h
- tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py
- tensorrt_llm/runtime/kv_cache_manager_v2/init.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/lifeCycleRegistry.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/cudaEvent.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/utils/typedIndex.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/blockRadixTree.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.h
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.h
- tensorrt_llm/runtime/kv_cache_manager_v2/_block_radix_tree.py
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCache.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storage/core.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/kvCacheManager.cpp
- cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/storageManager.cpp
2e1f0f3 to
1d2226d
Compare
| @@ -0,0 +1,27 @@ | |||
| // Copyright (c) 2009-2010 Satoshi Nakamoto | |||
There was a problem hiding this comment.
@yuanjingx87 @tburt-nv Is this a proper way to import the MIT licensed codes?
There was a problem hiding this comment.
Almost, I'll put together a patch to tweak it.
e5a1453 to
a4efc5c
Compare
…dings Migrates tensorrt_llm/runtime/kv_cache_manager_v2 from pure Python to a C++ implementation under cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/ with nanobind bindings compiled into bindings.so, preserving the same public API. The dispatcher __init__.py selects the backend via TLLM_KV_CACHE_MANAGER_V2_BACKEND (default "cpp"); both backends pass the shared test suite. CODEOWNERS assigns the new C++ tree to trt-llm-kv-cache-manager-devs. Includes ports of subsequent main features: commit-min-snapshot + SWA-slot reservation, SHA-256 block-key hashing, CUDA-graph request IDs, event manager and stats API to C++, uint64 ReuseScope salt/lora_id, resume-utilization KV constraints, per-conversation KV cache block reuse (PlannedDropHandle), and an MPI teardown fix for the unittest/bindings CI shard. Migration planning docs (TODO.md, MIGRATION_PLAN_CPP.md, CPP_MIGRATION_PLAN_MAIN_15633.md) are kept on a separate branch. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
Base-branch (main) regressions surfaced by rebasing onto latest main; none are in the KVCacheManagerV2 change set. Two distinct patterns, both fixed by matching the existing sibling conventions. 1) tensorrt_llm import break (nvbugs/6442074): SpecWorkerBase.__init_subclass__ forbids subclasses from overriding forward() -- they must implement _forward_impl so the base forward() wrapper can guarantee spec-dec attn-metadata cleanup. MTPEagleDynamicTreeWorker still overrode forward(), so 'import tensorrt_llm' raised TypeError at class-definition time, breaking all test collection. Rename its forward() to _forward_impl(), matching every sibling worker; callers use the base forward() wrapper so no caller changes. 2) executor tests build objects via __new__ (bypassing __init__) then exercise teardown/lifecycle paths. main NVIDIA#16523 (multi-process HTTP frontends) added attributes to BaseWorker/GenerationExecutorProxy __init__ that teardown reads unguarded, so the __new__-built test objects raised AttributeError: - test_proxy_fast_death.py _bare_proxy: seed _multi_frontend_ipc_dir/_hmac. - test_event_loop_error_broadcast.py _WorkerStub: seed frontend_result_queues (responses_handler read it unguarded -> 4/6 tests failed). - test_proxy_postproc_terminate.py _make_proxy: seed workers_started + _multi_frontend_ipc_dir/_hmac (GC __del__ -> shutdown teardown). - test_base_worker.py __new__ shell: seed doing_shutdown (GC __del__). Each seeds only what that object's teardown reads, matching the _bare_proxy convention. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #61522 [ run ] triggered by Bot. Commit: |
|
PR_Github #61522 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61570 [ run ] triggered by Bot. Commit: |
Dev Engineer Review
QA Engineer Review
kvCacheManagerV2HostMemTest.cppkvCacheManagerV2StatsTest.cppkvCacheManagerV2TypedIndexTest.cpptests/integration/test_lists/,test-db/, orqa/coverage changes are listed. Test functions should be mapped to CI or manual-QA entries.