Skip to content

[https://nvbugs/6939212][fix] Unwaive unittest/_torch/attention after the KVCM2 teardown fix - #20471

Merged
chenfeiz0326 merged 1 commit into
NVIDIA:mainfrom
chenfeiz0326:repair-bot-v2/nvbug-6939212-r6
Oct 19, 2026
Merged

chenfeiz0326 merged 1 commit into
NVIDIA:mainfrom
chenfeiz0326:repair-bot-v2/nvbug-6939212-r6

Conversation

@chenfeiz0326

@chenfeiz0326 chenfeiz0326 commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Removes this bug's waiver for unittest/_torch/attention. The failure it covered is fixed on main by #19650.

Fixes https://nvbugs/6939212

Root cause (fixed on main by #19650)

  • test_dsa_indexer's test-local MockMetadata.__init__ captured cache_manager in a closure. A class object is always part of a reference cycle, so the KVCacheManagerV2 outlived the test and was freed later by the cyclic GC, on whichever thread triggered the collection.
  • In CI that could be the pytest-timeout or periodic-junit watchdog thread, which has no CUDA context. KVCM2's teardown (~KvCache → close() → cuEventRecord) then failed with CUDA driver error: 201 (invalid device context) and poisoned KVCM2 for the rest of the process, so every later V2 test failed with CorruptedError.
  • [None][perf] Fill the KVCM2 host tier progressively #19650 (0cd9d0b) passes cache_manager to MockMetadata.__init__ as a parameter, so no closure keeps the manager alive and it is released on the test's own thread.

Change

tests/integration/test_lists/waives.txt: removes
unittest/_torch/attention --ignore=unittest/_torch/attention/sparse/test_cute_dsl_gvr_topk_decode.py --ignore=unittest/_torch/attention/sparse/msa/test_minimax_m3_sparse_attn_decode.py SKIP (https://nvbugs/6939212).

Verification (H100, main 0372799 + this change, post-merge wheel containing #19650, CI image)

Check Result
Run a test_dsa_indexer case, gc.collect() on a thread without a CUDA context, then the next case (poisoned KVCM2 with error 201 before #19650) passes, no poison
The un-waived unittest/_torch/attention (with both --ignores) via test_unittests_v2, as CI runs it 3932 passed, 2643 skipped, no KVCM2 poison

The run had one failure: test_cute_dsl_mla.py::test_mla_generation_routes_wide_head_fp8_verify_away_from_trtllm_gen[103-96-1-True-False-cute_dsl_mla]. It is unrelated and already waived on main for https://nvbugs/6948129; the local run did not apply the waiver list.

🤖 Generated with Claude Code

Dev Engineer Review

The change removes the unittest/_torch/attention waiver for NVBug 6939212. The scope is limited to the test waiver list. The reported fix is in #19650; this change does not modify test code.

QA Engineer Review

tests/integration/test_lists/waives.txt removes the waiver that excluded sparse-attention tests. The author reports 3932 tests passed and one failure that they attribute to a separate test already waived on main. The author has not separately run the test_cutedsl_mqa_logits_output_buffer_persistent case associated with NVBug 6948011. Coverage verdict: needs follow-up.

Per-File QA Perspective

  • tests/integration/test_lists/waives.txt: Removing the waiver allows the affected attention tests to run instead of being skipped. Verify the tests pass with the #19650 fix; the separate NVBug 6948011 case remains unverified.

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/TensorRT-LLM/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: d98319c6-7597-4d6d-a072-61206a006f80
📥 Commits

Reviewing files that changed from the base of the PR and between 615673e and b8b0b02.

📒 Files selected for processing: 1
  • tests/integration/test_lists/waives.txt
💤 Files with no reviewable changes: 1
  • tests/integration/test_lists/waives.txt

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

Walkthrough

The waiver list no longer contains the entry for the unittest/_torch/attention directory, its two file-specific ignores, or the NVBug 6939212 reference.

Changes

Attention Test Waiver Removal

Layer / File(s) Summary
Remove attention test waiver
tests/integration/test_lists/waives.txt
Removes the directory-level waiver entry, including its two file-specific ignores and NVBug 6939212 reference.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~2 minutes

Change: Other

Suggested reviewers: trtllm-agent

Merge Risk: ⚪ Minimal · up to b8b0b

This change re-enables the attention unit tests by removing a waiver whose underlying cause was fixed on main. It affects test runs only, with no production impact. Merge risk is minimal.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 46.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description check Passed The description clearly explains the root cause, the waiver removal, the fix from #19650, and the verification results. It provides relevant test coverage and identifies an unrelated existing failure.…
Title check Passed The title follows the required ticket-and-type format and clearly describes the waiver removal after the KVCM2 teardown fix.
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.
Full details: Description check

Explanation

The description clearly explains the root cause, the waiver removal, the fix from #19650, and the verification results. It provides relevant test coverage and identifies an unrelated existing failure. The template checklist is not reproduced, but the required technical information is substantially complete.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@lowsfer

lowsfer commented Oct 17, 2026

Copy link
Copy Markdown
Member

Tests should close KVCM2 deterministically and KVCM2 was design to work with existing cuda context. Capturing a cuda context is not a reliable fix. the user could have destroyed that context when GC happens. I think what needs to be fixed is the test:

  1. ref cycles should be avoided if possible.
  2. if that's hard, call shutdown() of KVCM2 explicitly, like we do in many other tests.

@lowsfer

lowsfer commented Oct 17, 2026

Copy link
Copy Markdown
Member

I included a fix here: 1dc346c

@chenfeiz0326

Copy link
Copy Markdown
Collaborator Author

@lowsfer Thanks, agreed on both points:

  • KVCM2's contract is that the caller provides the CUDA context, and the test leaked the manager to the cyclic GC instead of closing it deterministically.
  • Capturing a CUcontext at construction is not safe if the owner has destroyed that context before the GC runs. cuCtxPushCurrent on a destroyed context is invalid, so this PR swaps one failure for another.

What this PR established, for whoever lands the test fix (H100, base 6544bcd):

  • The root cause is the closure you describe. _create_mock_metadata's local MockMetadata.__init__ captures cache_manager in a cell, so after test_indexer_decode_with_paged_kv_cache returns, the KVCacheManagerV2 is still alive and only the cyclic GC frees it.
  • Running that gc.collect() on a fresh threading.Thread reproduces the NVBug exactly: CachedCudaEvent ... CUDA driver error: 201 (invalid device context), then the next test fails with CorruptedError. Running it on the main thread passes.

Your commit 1dc346c on #19650 fixes it at that source. #19650 is a perf PR, though, so NVBug 6939212 would stay open until it merges.

Which do you prefer?

  1. I replace this PR's change with your test_dsa_indexer.py hunk only (your authorship kept), re-verify it with the reproducer above plus unittest/_torch/attention on H100, and you drop it from [None][perf] Fill the KVCM2 host tier progressively #19650.
  2. [None][perf] Fill the KVCM2 host tier progressively #19650 carries the fix and this PR is closed in its favor.

@HuiGao-NV

Copy link
Copy Markdown
Collaborator

Please unwaive cases by searching "https://nvbugs/6948011" in waives.txt.

@lowsfer

lowsfer commented Oct 19, 2026

Copy link
Copy Markdown
Member

#19650 is merged. I included it there because it blocked CI and is very small. but I didn't unwaive, so you still need this PR to do it.

… the KVCM2 teardown fix

The failures came from test_dsa_indexer leaving its KVCacheManagerV2 to
the cyclic GC: freed on a thread without a CUDA context, its teardown
failed with CUDA error 201 and poisoned KVCM2 for the rest of the
process. NVIDIA#19650 now passes the cache manager to MockMetadata explicitly,
so no closure keeps it alive, and the test owns its release. Remove the
waiver for this bug.

Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
@chenfeiz0326

Copy link
Copy Markdown
Collaborator Author

@lowsfer thanks for landing the test fix in #19650 (0cd9d0b). I'm reworking this PR as you suggest: drop the CUDA-context capture entirely and carry only the waiver removal for https://nvbugs/6939212 (waives.txt line 198, added by #20563).

@HuiGao-NV main has no waives.txt line referencing https://nvbugs/6948011 (git grep 6948011 origin/main finds nothing). Its error matches this bug, though: test_cutedsl_mqa_logits_output_buffer_persistent with CUDA driver error: 201 (invalid device context), the same KVCM2 poison. So #19650 should cover it too, and I'll mention it when this PR's CI passes.

The reworked commit has not been pushed yet. It still goes through the bot's review, then H100 verification on a main build that includes #19650, before it replaces this PR's head.

@chenfeiz0326
chenfeiz0326 force-pushed the repair-bot-v2/nvbug-6939212-r6 branch from 615673e to b8b0b02 Compare October 19, 2026 07:14
@chenfeiz0326 chenfeiz0326 changed the title [https://nvbugs/6939212][fix] Bind the CUDA context for KVCM2 teardown on any thread [https://nvbugs/6939212][fix] Unwaive unittest/_torch/attention after the KVCM2 teardown fix Oct 19, 2026
@chenfeiz0326

Copy link
Copy Markdown
Collaborator Author

This PR now contains only the unwaive. The CUDA-context capture is dropped, as @lowsfer asked: the root cause is fixed on main by #19650, where test_dsa_indexer's MockMetadata no longer captures the cache manager in a closure, so the manager is not left to the cyclic GC.

Change: removes unittest/_torch/attention --ignore=... SKIP (https://nvbugs/6939212) from waives.txt (1 line). I updated the PR title and description to match.

Verified on H100 with main 0372799 plus this change, using a post-merge wheel that contains #19650:

  • The reproducer that runs a test_dsa_indexer case and then a gc.collect() on a thread with no CUDA context, which previously poisoned KVCM2 with CUDA driver error: 201, now passes.
  • The previously waived unittest/_torch/attention (with its two --ignores), run through test_unittests_v2 as CI runs it: 3932 passed, no KVCM2 poison. Its one failure, test_cute_dsl_mla.py::test_mla_generation_routes_wide_head_fp8_verify_away_from_trtllm_gen[103-96-1-True-False-cute_dsl_mla], is already waived on main for https://nvbugs/6948129 and is unrelated; my local run did not apply the waiver list.

@HuiGao-NV: no waiver line references https://nvbugs/6948011 on main, so there is nothing to remove for it. Its reported error is the same CUDA driver error: 201 KVCM2 poison in test_dsa_indexer, so I expect #19650 to resolve it too, but I have not run that test separately.

/bot run follows.

@chenfeiz0326

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #78837 [ run ] triggered by Bot. Commit: b8b0b02 Link to invocation

@github-actions

Copy link
Copy Markdown

Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements.

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #78837 [ run ] completed with state SUCCESS. Commit: b8b0b02
/LLM/main/L0_MergeRequest_PR pipeline #64955 completed with status: 'SUCCESS'
Pipeline passed with automatic retried tests. Check the rerun report for details.

CI Report

Link to invocation

@chenfeiz0326
chenfeiz0326 merged commit 321a38c into NVIDIA:main Oct 19, 2026
18 checks passed
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