[https://nvbugs/6418912][fix] Fix KV-cache lifecycle test failures - #16019
Conversation
|
/bot run --disable-fail-fast --stage-list "A30-CPP-1" |
|
PR_Github #57861 [ run ] triggered by Bot. Commit: |
|
PR_Github #57861 [ run ] completed with state
|
|
/bot run --disable-fail-fast --stage-list "A30-CPP-1" |
|
PR_Github #57889 [ run ] triggered by Bot. Commit: |
|
PR_Github #57889 [ run ] completed with state
|
|
/bot run --disable-fail-fast --stage-list "A30-CPP-1" |
|
PR_Github #57931 [ run ] triggered by Bot. Commit: |
|
PR_Github #57931 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #57951 [ run ] triggered by Bot. Commit: |
|
PR_Github #57951 [ run ] completed with state
|
chienchunhung
changed the title
|
/bot run --disable-fail-fast |
|
Duplicate trigger avoided; using the earlier full CI request at #16019 (comment). |
dpitman-nvda
left a comment
dpitman-nvda
left a comment
There was a problem hiding this comment.
Approve for CI touches
|
PR_Github #58050 [ run ] triggered by Bot. Commit: |
📝 WalkthroughWalkthroughThis PR fixes a stale prev-in-sequence pointer for detached KV cache blocks in ChangesKV Cache Manager Fix and Test Updates
CI Test Stage Addition
Estimated code review effort: 3 (Moderate) | ~25 minutes 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)
cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp (1)
920-1041: 📐 Maintainability & Code Quality | 🔵 TrivialMechanical block-id expectation updates look consistent with the recycling fix.
These updates shift expected recycled/new block ids (e.g., reused block 3 instead of a freshly-allocated block 4) to reflect that detached blocks are now correctly recycled to the front of the free queue rather than being kept alive by a stale
prevBlockInSeqchain. As per path instructions fortests/**, one gap worth calling out: none of these updated tests explicitly assert the complementary branch — that a block still registered in the lookup tree (non-detached, stored for reuse) keeps a non-nullgetPrevBlockInSeq()after release. Consider adding one such assertion (e.g., inBlockManagerReuseTest, right after a block is stored for reuse) to fully pin down both branches of the new conditional inWindowBlockManager::releaseBlocks.Based on path instructions for
tests/**: "Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM. Keep feedback actionable: suggest concrete list file names and whether coverage is sufficient, insufficient, or needs follow-up outside the PR."Also applies to: 1246-1264, 1317-1327, 1347-1357, 1510-1520, 1548-1559, 1681-1861, 2320-2351, 3067-3068, 4090-4122
🤖 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/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp` around lines 920 - 1041, The updated block-id expectations are fine, but the test coverage is missing the complementary case for the new release path. In the relevant KV cache manager tests (for example around KvCacheManagerTest and any BlockManagerReuseTest coverage), add an assertion after releasing a block that is still registered for reuse to verify its prevBlockInSeq remains non-null, so both branches of WindowBlockManager::releaseBlocks are covered. Use the existing reuse/release setup in addSequenceBatch, releaseBlocks, and getPrevBlockInSeq to place the check where the stored-for-reuse block is returned.
🤖 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/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp`:
- Around line 479-491: The partial-copy verification is hardcoded to the DRAM
path, so the GDS templated instantiation never exercises this test. Update the
partial-copy check around blockManager.offloadBlock and the later copy
verification to use the existing transferMode/directory plumbing, or split the
assertion into a DRAM-only branch if that behavior is intentional, so the
kvCacheManagerTest coverage matches the templated transfer mode.
---
Nitpick comments:
In `@cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp`:
- Around line 920-1041: The updated block-id expectations are fine, but the test
coverage is missing the complementary case for the new release path. In the
relevant KV cache manager tests (for example around KvCacheManagerTest and any
BlockManagerReuseTest coverage), add an assertion after releasing a block that
is still registered for reuse to verify its prevBlockInSeq remains non-null, so
both branches of WindowBlockManager::releaseBlocks are covered. Use the existing
reuse/release setup in addSequenceBatch, releaseBlocks, and getPrevBlockInSeq to
place the check where the stored-for-reuse block is returned.
🪄 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: 72cca991-0eec-4a5d-a8a8-392419829510
📒 Files selected for processing (5)
cpp/tensorrt_llm/batch_manager/kvCacheManager.cppcpp/tests/unit_tests/batch_manager/bufferIndexHolderTest.cppcpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cppjenkins/L0_Test.groovytests/integration/test_lists/test-db/l0_a30.yml
|
PR_Github #58051 [ run ] triggered by Bot. Commit: |
|
PR_Github #58050 [ run ] completed with state |
|
/bot run --disable-fail-fast --stage-list "A30-CPP-1" |
|
PR_Github #58059 [ run ] triggered by Bot. Commit: |
|
PR_Github #58051 [ run ] completed with state |
|
PR_Github #58059 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #58072 [ run ] triggered by Bot. Commit: |
|
PR_Github #58072 [ run ] completed with state
|
chienchunhung
changed the title
Provision two fixture slots for BufferIndexHolder move assignment so the second acquisition cannot deadlock. Clear stale predecessor links before detached KV-cache blocks return to the front of the eviction queue. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Update detached-block eviction, partial-copy inspection, recycled block IDs, event-stream accounting, and cached-prefix reuse expectations to match the corrected lifecycle behavior. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Register A30-CPP-1, move batch_manager-80 from post-merge to pre-merge, and remove the broad A30 batch-manager waiver so lifecycle regressions are exercised before landing. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
6874806 to
e27882e
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #58271 [ run ] triggered by Bot. Commit: |
|
PR_Github #58271 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #58483 [ run ] triggered by Bot. Commit: |
|
PR_Github #58483 [ run ] completed with state |
5 participants
Summary
Resolves NVBUG 6418912 by fixing the
BufferIndexHoldermove-assignment deadlock and the related KV-cache lifecycle and test failures discovered while validatingbatch_manager-80.This is the intended merge candidate for the bug. It consolidates the fixes originally developed in:
batch_manager-80to A30 pre-mergeChanges
BufferIndexHoldermove assignment, preventing therecvandsendvariants from waiting indefinitely.prevBlockInSeqlinks when zero-reference detached KV-cache blocks are recycled.A30-CPP-1pre-merge stage and movecpp/test_unit_tests.py::test_unit_tests[batch_manager-80]from post-merge to pre-merge.Validation
Targeted unwaived validation on the tree-equivalent pre-rewrite head
68748060passed in pipeline #46726:A30-CPP-1: 524 passed, 0 failed, 2 skippedSideVariants/BufferIndexHolderLifecycleTest.MoveAssignReleasesPriorThenTransfers/send: passedSideVariants/BufferIndexHolderLifecycleTest.MoveAssignReleasesPriorThenTransfers/recv: passedkvCacheManagerTest.cppcases passed, including detached-block recycling, partial copy, reuse accounting, andFillKvCacheWithRequestsAndCompleteOneByOne/5The branch was subsequently consolidated into three signed commits and rebased onto upstream
50423007d; the current head ise27882e0. A fresh full CI run was requested here and launched as pipeline #46909.The earlier full pipeline #46737 reused the same-head A30 result and marked
A30-CPP-1as skipped. Its failures were unrelated current-main PyTorch attention-contract failures, known NVBUG 6427411 failures, and infrastructure cancellations; no NVBUG 6418912 or changed-file fingerprint appeared in those failures.Bug
NVBUG 6418912