[None][feat] Default on FlashInferTrtllmGenAttention - #14618
Conversation
|
/bot run --disable-fail-fast |
|
PR_Github #50471 [ run ] triggered by Bot. Commit: |
📝 WalkthroughWalkthroughThis PR hardens attention kernel and backend implementations against null KV cache scale pointers, reorganizes MLA parameter passing, adds batch-aware workspace sizing, and refactors the TRT-LLM-Gen backend with a unified parameter container and consolidated support checking. ChangesAttention system null-safety and backend reorganization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
tests/unittest/_torch/attention_backend/test_attention_op_sync.py (1)
496-519: QA list updates look unnecessary here.This is a narrow unittest-only static sync check under
tests/unittest/, so I don't think it needs atests/integration/test_lists/qa/update.🤖 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/attention_backend/test_attention_op_sync.py` around lines 496 - 519, The PR included unrelated updates to QA lists under tests/integration/test_lists/qa/ that are unnecessary for this unittest-only change; revert or remove those QA-list changes so the only changes are the unit test edits in tests/unittest/_torch/attention_backend/test_attention_op_sync.py (refer to the functions test_every_forward_args_field_is_consumed and test_no_unexpected_other_kwargs), then run the test suite or git diff to confirm no files under tests/integration/test_lists/qa/ are modified before committing.
🤖 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/thop/mlaPreprocessOp.cpp`:
- Around line 216-220: The chunked MLA path needs the FP8-only quantization
guard restored: in loadChunkedKVCacheForMLA() ensure you only take the
FP8-specific branch (the code that uses kv_scale_quant_orig_ptr and the FP8 read
path) when hasFp8KvCache() is true (or quant_mode indicates FP8); otherwise
route to the non-FP8/unquantized TCache instantiations so element types match.
Update the dispatch that currently only special-cases hasFp8KvCache() to
explicitly check quant_mode/hasFp8KvCache() before using kv_scale_quant_orig and
the FP8 reader, preventing non-FP8 quantized caches from being read with the
wrong element type.
In `@tensorrt_llm/_torch/attention_backend/interface.py`:
- Around line 756-765: The mask_type property currently raises for any
non-CAUSAL/FULL mask which breaks the custom-mask path; update mask_type to
detect CustomAttentionMask (e.g., isinstance(self.attention_mask,
CustomAttentionMask)) and return the corresponding integer
(int(AttentionMaskType.custom) or the enum value used for custom masks) while
keeping the existing returns for PredefinedAttentionMask.CAUSAL and .FULL, so
TrtllmAttention._run() and AttentionForwardArgs.attention_mask_data can use the
custom-mask path instead of failing early.
In `@tensorrt_llm/_torch/attention_backend/trtllm_gen.py`:
- Around line 506-510: The positive support result is being returned from the
cache before phase-specific validation, allowing a generation-only probe to
wrongly bypass later context checks; update the probe caching in the method that
uses self._support_result so that you do not short-circuit on a cached True
before running the [Context]/[Generation] validations — either move the
write/read of self._support_result to occur after all phase-specific checks, or
make the cache keyed by phase (e.g., store separate tokens like
self._support_result_context and self._support_result_generation or a dict keyed
by phase) so that a True for one phase does not satisfy a different phase;
ensure the code paths that call the context-specific checks reference the new
phase-aware cache and only return cached True when the stored entry matches the
current probe phase.
- Around line 743-750: Restore layer-level KV-scale fallback: the FmhaParams
construction currently copies kv_scale_orig_quant and kv_scale_quant_orig
directly from fwd (which can be None), so change the assignment in the
FmhaParams(...) call in trtllm_gen.py to use the layer/model defaults when the
forward-level values are None (e.g., set kv_scale_orig_quant =
fwd.kv_scale_orig_quant if not None else self._kv_scale_orig_quant and
kv_scale_quant_orig = fwd.kv_scale_quant_orig if not None else
self._kv_scale_quant_orig) so the C++ preprocess/postprocess path receives the
proper fallback.
- Around line 887-930: The call to thop.trtllm_gen_context_preprocess (and the
similar calls to trtllm_gen_context_postprocess and
trtllm_gen_generation_preprocess) incorrectly passes params.sink_token_length as
a positional argument; remove params.sink_token_length from those positional
argument lists so subsequent arguments (e.g., params.num_tokens,
params.batch_size, params.input_seq_length, params.max_past_kv_length, etc.)
align with the updated C++ signatures. Locate the calls by the function names
trtllm_gen_context_preprocess, trtllm_gen_context_postprocess, and
trtllm_gen_generation_preprocess and delete the params.sink_token_length token,
keeping the original order of the remaining parameters so they bind to the
correct runtime parameters.
In `@tensorrt_llm/_torch/attention_backend/trtllm.py`:
- Around line 177-184: The method spec_decoding_position_offsets_for_cpp
currently reshapes spec_decoding_position_offsets directly and ignores the
precomputed view stored in spec_decoding_position_offsets_cpp; change it to
return self.spec_decoding_position_offsets_cpp (which update_spec_dec_param
already populates with the active cpp_query_len and correct 2D layout) when that
attribute is set, falling back to the existing logic only if it's None—this
prevents using _internal_buf_dim/max_num_requests reshaping that exposes
padded/stale offsets to the C++ kernel.
- Around line 1453-1462: The fallback for forward_args.kv_scale_quant_orig
should be derived as the reciprocal of whichever kv_scale_orig_quant value is
actually being used (which may have just been assigned to
forward_args.kv_scale_orig_quant), rather than reusing a possibly-stale
self.kv_scale_quant_orig; change the second branch so that if
forward_args.kv_scale_quant_orig is None you compute it from
forward_args.kv_scale_orig_quant (e.g. torch.reciprocal or 1.0 / tensor) after
the first fallback assignment, ensuring forward_args.kv_scale_quant_orig stays
consistent with the live kv_scale value (referencing
forward_args.kv_scale_orig_quant, forward_args.kv_scale_quant_orig,
self.kv_scale_orig_quant, and self.kv_scale_quant_orig).
In `@tensorrt_llm/_torch/modules/attention.py`:
- Around line 744-754: The compiled LoRA path still ends up allocating FP8/NVFP4
outputs because attn_custom_op_inplace() calls _attn_impl(..., has_lora=False)
and create_attn_outputs() isn't aware of LoRA; fix by threading the has_lora
flag through attn_custom_op_inplace() and create_attn_outputs() so both
allocation and the custom op respect LoRA: update forward_impl to pass the real
has_lora into attn_custom_op_inplace(), modify attn_custom_op_inplace() to
accept and forward has_lora into _attn_impl(...) and create_attn_outputs(), and
change create_attn_outputs() to skip creating quantized buffers (force BF16 /
set out_scale=None) when has_lora is True.
In `@tests/unittest/_torch/attention_backend/test_attention_op_sync.py`:
- Around line 61-63: The test currently only verifies keyword names against the
C++ declaration (_HEADER / attentionOp.h); update it to also parse and assert
the nanobind binding names used for thop.attention in
cpp/tensorrt_llm/nanobind/thop/bindings.cpp so the runtime kwarg contract
matches the header; specifically, read the binding parameter names (the "_a"
identifiers) from bindings.cpp and compare them to the header-derived list and
to the Python-callable names used by thop.attention (also add the same check for
the other block mentioned around lines 132-179) so any drift in bindings.cpp
will fail the test.
- Around line 446-460: Adjust _collect_chains so it only counts attribute reads
that actually flow into the thop.attention(...) call: locate Call AST nodes
where func is an Attribute with attr "attention" and base Name "thop" (i.e., the
thop.attention call sites inside TrtllmAttention._run), then when walking
Attribute nodes only add a chain if that Attribute node is a descendant of one
of those Call nodes (appears in the call's args or keywords) and its root Name
matches the provided root; update _collect_chains to first collect those
thop.attention Call nodes and filter Attribute occurrences by ancestry to those
calls so preprocessing/branch-only reads are not treated as consumed.
---
Nitpick comments:
In `@tests/unittest/_torch/attention_backend/test_attention_op_sync.py`:
- Around line 496-519: The PR included unrelated updates to QA lists under
tests/integration/test_lists/qa/ that are unnecessary for this unittest-only
change; revert or remove those QA-list changes so the only changes are the unit
test edits in tests/unittest/_torch/attention_backend/test_attention_op_sync.py
(refer to the functions test_every_forward_args_field_is_consumed and
test_no_unexpected_other_kwargs), then run the test suite or git diff to confirm
no files under tests/integration/test_lists/qa/ are modified before committing.
🪄 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: 3371b37f-9f40-45f8-9c6d-bb963ed18227
📒 Files selected for processing (16)
cpp/kernels/xqa/mla_sm120.cucpp/tensorrt_llm/kernels/decoderMaskedMultiheadAttention/decoderMaskedMultiheadAttentionTemplate.hcpp/tensorrt_llm/kernels/flashMLA/flash_fwd_mla_kernel.hcpp/tensorrt_llm/kernels/unfusedAttentionKernels/unfusedAttentionKernels_2_template.hcpp/tensorrt_llm/nanobind/thop/bindings.cppcpp/tensorrt_llm/thop/attentionOp.cppcpp/tensorrt_llm/thop/attentionOp.hcpp/tensorrt_llm/thop/mlaPreprocessOp.cppcpp/tensorrt_llm/thop/trtllmGenQKVProcessOp.cpptensorrt_llm/_torch/attention_backend/interface.pytensorrt_llm/_torch/attention_backend/sparse/dsa.pytensorrt_llm/_torch/attention_backend/trtllm.pytensorrt_llm/_torch/attention_backend/trtllm_gen.pytensorrt_llm/_torch/auto_deploy/custom_ops/mla/trtllm_mla.pytensorrt_llm/_torch/modules/attention.pytests/unittest/_torch/attention_backend/test_attention_op_sync.py
💤 Files with no reviewable changes (1)
- tensorrt_llm/_torch/auto_deploy/custom_ops/mla/trtllm_mla.py
|
PR_Github #50471 [ run ] completed with state
|
48a828e to
eedec7f
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #50826 [ run ] triggered by Bot. Commit: |
|
PR_Github #50826 [ run ] completed with state
|
04aba35 to
915b240
Compare
Signed-off-by: Yihan Wang <yihwang@nvidia.com>
Signed-off-by: Yihan Wang <yihwang@nvidia.com>
Signed-off-by: Yihan Wang <yihwang@nvidia.com>
Signed-off-by: Yihan Wang <yihwang@nvidia.com>
…supported method Signed-off-by: Yihan Wang <yihwang@nvidia.com>
….is_supported Signed-off-by: Yihan Wang <yihwang@nvidia.com>
Signed-off-by: Yihan Wang <yihwang@nvidia.com>
c99928c to
48b9461
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #52720 [ run ] triggered by Bot. Commit: |
|
PR_Github #52636 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #52740 [ run ] triggered by Bot. Commit: |
|
PR_Github #52720 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #52780 [ run ] triggered by Bot. Commit: |
|
PR_Github #52740 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #52810 [ run ] triggered by Bot. Commit: |
|
PR_Github #52780 [ run ] completed with state |
|
PR_Github #52810 [ run ] completed with state |
… use PR NVIDIA#14618 made the trtllm-gen FMHA the default Blackwell attention backend; while it ships mRoPE plumbing (mrope_rotary_cos_sin is passed to trtllm_gen_context_{preprocess,postprocess}), its mRoPE numerics regress Qwen2.5-VL MMMU by ~6 points on every Blackwell SKU (B200/GB200/GB300), dropping accuracy from ~51 to ~44.9 and tripping the 47.343 threshold in test_llm_api_pytorch_multimodal:: TestQwen2_5_VL_7B::test_auto_dtype. Gate trtllm-gen off in _is_supported_with_reason whenever fwd.mrope_rotary_cos_sin is not None; the dispatcher then falls through to FallbackFmha (legacy attention path), which already plumbs mrope_rotary_cos_sin through fmha/fallback.py and handles it correctly. This is a per-request gate, so pure-text requests on the same model still take the fast trtllm-gen path. Also drop the three existing waivers for this bug (B200/GB200/GB300) now that the test passes (verified MMMU=47.78 on GB300, threshold 47.343). Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
… use PR NVIDIA#14618 made the trtllm-gen FMHA the default Blackwell attention backend; while it ships mRoPE plumbing (mrope_rotary_cos_sin is passed to trtllm_gen_context_{preprocess,postprocess}), its mRoPE numerics regress Qwen2.5-VL MMMU by ~6 points on every Blackwell SKU (B200/GB200/GB300), dropping accuracy from ~51 to ~44.9 and tripping the 47.343 threshold in test_llm_api_pytorch_multimodal:: TestQwen2_5_VL_7B::test_auto_dtype. Gate trtllm-gen off in _is_supported_with_reason whenever fwd.mrope_rotary_cos_sin is not None; the dispatcher then falls through to FallbackFmha (legacy attention path), which already plumbs mrope_rotary_cos_sin through fmha/fallback.py and handles it correctly. This is a per-request gate, so pure-text requests on the same model still take the fast trtllm-gen path. Also drop the three existing waivers for this bug (B200/GB200/GB300) now that the test passes (verified MMMU=47.78 on GB300, threshold 47.343). Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
… use PR NVIDIA#14618 made the trtllm-gen FMHA the default Blackwell attention backend; while it ships mRoPE plumbing (mrope_rotary_cos_sin is passed to trtllm_gen_context_{preprocess,postprocess}), its mRoPE numerics regress Qwen2.5-VL MMMU by ~6 points on every Blackwell SKU (B200/GB200/GB300), dropping accuracy from ~51 to ~44.9 and tripping the 47.343 threshold in test_llm_api_pytorch_multimodal:: TestQwen2_5_VL_7B::test_auto_dtype. Gate trtllm-gen off in _is_supported_with_reason whenever fwd.mrope_rotary_cos_sin is not None; the dispatcher then falls through to FallbackFmha (legacy attention path), which already plumbs mrope_rotary_cos_sin through fmha/fallback.py and handles it correctly. This is a per-request gate, so pure-text requests on the same model still take the fast trtllm-gen path. Also drop the three existing waivers for this bug (B200/GB200/GB300) now that the test passes (verified MMMU=47.78 on GB300, threshold 47.343). Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
Description
Built on top of #14279 (
[None][refactor] Add derived properties for the thop.attention call site). That PR refactors thethop.attention(...)call site to use explicit kwargs sourced from rich objects; this PR stacks the following on top of it:Default on
FlashInferTrtllmGenAttentionSwitches
TrtllmGenAttentionfrom an opt-in (env-gated) to the default decode backend on Blackwell+, replacing the previous default withFlashInferTrtllmGenAttention.