[None][feat] Enable block reuse for flashinfer - #17106
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe executor now disables block reuse only for FlashInfer Star Attention. Tests cover FlashInfer and TRTLLM prefix-cache reuse, FlashInfer Star Attention metadata, and chunked-prefill cache statistics. ChangesKV-cache reuse behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/_torch/executor/test_py_executor_creator_mla_cache_reuse_sync.py (1)
213-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd annotations to the new and modified Python function interfaces.
The coding guidelines require annotations for every function.
tests/unittest/_torch/executor/test_py_executor_creator_mla_cache_reuse_sync.py#L213-L244: Annotate_run_create_py_executorparameters and return type.tests/unittest/_torch/executor/test_py_executor_creator_mla_cache_reuse_sync.py#L392-L417: Annotate both added test function interfaces.tests/integration/defs/accuracy/test_llm_api_pytorch.py#L85-L96: Annotate_latest_kv_cache_statsparameter and return type.tests/unittest/llmapi/test_llm_pytorch.py#L255-L280: Annotatetest_llm_prefix_cache_reuseparameter and return type.As per coding guidelines, “Annotate every function, use
Nonefor non-returning functions.”🤖 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_py_executor_creator_mla_cache_reuse_sync.py` around lines 213 - 244, Annotate every affected function interface: add parameter and return annotations to _run_create_py_executor in tests/unittest/_torch/executor/test_py_executor_creator_mla_cache_reuse_sync.py:213-244, both added test functions in tests/unittest/_torch/executor/test_py_executor_creator_mla_cache_reuse_sync.py:392-417, _latest_kv_cache_stats in tests/integration/defs/accuracy/test_llm_api_pytorch.py:85-96, and test_llm_prefix_cache_reuse in tests/unittest/llmapi/test_llm_pytorch.py:255-280; use None for functions that do not return a value.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
`@tests/unittest/_torch/executor/test_py_executor_creator_mla_cache_reuse_sync.py`:
- Around line 213-244: Annotate every affected function interface: add parameter
and return annotations to _run_create_py_executor in
tests/unittest/_torch/executor/test_py_executor_creator_mla_cache_reuse_sync.py:213-244,
both added test functions in
tests/unittest/_torch/executor/test_py_executor_creator_mla_cache_reuse_sync.py:392-417,
_latest_kv_cache_stats in
tests/integration/defs/accuracy/test_llm_api_pytorch.py:85-96, and
test_llm_prefix_cache_reuse in
tests/unittest/llmapi/test_llm_pytorch.py:255-280; use None for functions that
do not return a value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a0b6452a-e518-421d-9e57-e5d4172ac7d9
📒 Files selected for processing (5)
tensorrt_llm/_torch/pyexecutor/py_executor_creator.pytests/integration/defs/accuracy/test_llm_api_pytorch.pytests/integration/defs/accuracy/test_llm_api_pytorch_multimodal.pytests/unittest/_torch/executor/test_py_executor_creator_mla_cache_reuse_sync.pytests/unittest/llmapi/test_llm_pytorch.py
|
Reviewed Why I agree the guard is stale. I checked the metadata builder rather than taking it on faith. 1. No L0 pipeline has run. All 11 checks on this head are GitHub Actions gates (title format, DCO, pre-commit, base freshness). For a PR whose entire content is a behavior flip on the KV-cache path, the pipeline result is the evidence — please 2. Your strongest regression guard doesn't run pre-merge. What does run pre-merge is 3. The Gemma4 change is unrelated to this PR and it lands on a pre-merge gate. 4. Minor. |
|
Correction to my point 2 above — I got the evidence wrong, and the conclusion needs restating more precisely.
So, concretely, for the assertion you added:
The ask is therefore not "wire it into test-db" — ignore that part. It's that the FlashInfer half of your MMLU guard, which is the half that covers the behavior this PR changes, won't report in this PR's own pipeline. Pre-merge coverage of FlashInfer+reuse comes from One consequence worth flagging that I understated: because the TRTLLM lanes are pre-merge, Everything else in my earlier comment stands — in particular the Gemma4 config flip (point 3), which is the one I'd most like addressed before this merges. |
453c4ab to
e415c8e
Compare
There was a problem hiding this comment.
Approving — rechecked at e415c8e1.
Gemma4 is gone; that was my blocker. Both sides of the flip are now pinned pre-merge: test_flashinfer_preserves_cache_reuse / ..._star_attention_disables_cache_reuse via unittest/_torch/executor (l0_h100.yml:22), and test_llm_prefix_cache_reuse[TRTLLM|FLASHINFER] via part3 (l0_a100.yml:19, l0_h100.yml:269).
Also re-checked that py_executor_creator.py is still the only place coupling FLASHINFER to enable_block_reuse — _util.py:2839 only asserts the star-attention backend name — so narrowing the guard leaves no stale assumption behind it.
Post-merge is fine for the FLASHINFER test_chunked_prefill case, and no need to rework the test changes.
One to note, not act on: assert reusedBlocks > 0 and enable_iter_perf_stats=True also hit the TRTLLM parametrization, which runs pre-merge — an eviction or prompt-structure change can redden an accuracy gate for a non-accuracy reason.
e415c8e to
7485ba9
Compare
|
/bot run |
|
PR_Github #63502 [ run ] triggered by Bot. Commit: |
|
PR_Github #63502 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63554 [ run ] triggered by Bot. Commit: |
|
PR_Github #63554 [ run ] completed with state
|
Signed-off-by: William Zhang <133824995+2ez4bz@users.noreply.github.com>
Signed-off-by: William Zhang <133824995+2ez4bz@users.noreply.github.com>
7485ba9 to
9640690
Compare
|
/bot run |
|
PR_Github #63575 [ run ] triggered by Bot. Commit: |
|
PR_Github #63575 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63658 [ run ] triggered by Bot. Commit: |
|
PR_Github #63658 [ run ] completed with state |
Dev Engineer Review
FLASHINFERnow allows block reuse.FLASHINFER_STAR_ATTENTIONstill disables block reuse.test_chunked_prefill.reusedBlocks > 0.5assertion can affect TRTLLM pre-merge tests.QA Engineer Review
Test functions added or modified:
_latest_kv_cache_statstest_chunked_prefilltest_flashinfer_preserves_cache_reusetest_flashinfer_star_attention_disables_cache_reusetest_llm_prefix_cache_reuseCoverage includes:
The summary does not show corresponding
test-db/orqa/entries. FlashInfer chunked-prefill MMLU coverage remains post-merge. Existing pre-merge prefix-cache coverage does not include partial non-page-aligned reuse, chunked prefill, or CUDA graphs.Verdict: needs follow-up.
Description
Removes outdated guard for block reuse with the FLASHINFER
attention backend.
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-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.