Skip to content

[https://nvbugs/6316983][fix] Fix RoPE support in flashinfer trtllm-gen backend - #15661

Merged
yuxianq merged 3 commits into
NVIDIA:mainfrom
yihwang-nv:yihwang/fix_rope_in_fi_trtllm_gen
Jul 9, 2026
Merged

[https://nvbugs/6316983][fix] Fix RoPE support in flashinfer trtllm-gen backend#15661
yuxianq merged 3 commits into
NVIDIA:mainfrom
yihwang-nv:yihwang/fix_rope_in_fi_trtllm_gen

Conversation

@yihwang-nv

@yihwang-nv yihwang-nv commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • New Features

    • Added an optional mrope_position_deltas input to generation preprocessing and exposed it in the public generation API, allowing more flexible mRoPE generation flows.
  • Bug Fixes

    • Updated generation preprocessing to apply provided per-batch position offsets when available.
  • Tests

    • Removed SKIP waivers for a multimodal auto-dtype integration test on selected platforms (B200/GB200/GB300).

Description

The Qwen2.5-VL MMMU accuracy drop on Blackwell (trtllm-gen FMHA, default since PR #14618) was mRoPE being dropped in the generation/decode phase, not context:

  • Context/prefill uses precomputed per-token mrope_rotary_cos_sin — trtllm-gen forwarded this correctly.
  • Generation/decode must use the per-batch int32 mrope_position_deltas; the V1 RoPE kernel computes rotary_position = token_idx_in_seq + mrope_position_deltas[batch_idx] (unfusedAttentionKernels_2_template.h:444).
  • But trtllmGenGenerationPreprocess hard-coded mrope_position_deltas = nullptr (trtllmGenQKVProcessOp.cpp:704), so every generated token used wrong absolute RoPE positions → corrupted decode attention → wrong answers.

Depends on #15660

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-compatible or api-breaking. For api-breaking, include BREAKING in 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.

@yihwang-nv
yihwang-nv requested a review from a team as a code owner June 26, 2026 09:33
@yihwang-nv
yihwang-nv requested a review from yuxianq June 26, 2026 09:33
@yihwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

1 similar comment
@yihwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds mrope_position_deltas to the generation preprocess path, updates the non-Hopper Marlin NVFP4 stub signature, and removes three integration test waiver entries.

Changes

Generation preprocess mRoPE plumbing

Layer / File(s) Summary
Public signature update
cpp/tensorrt_llm/thop/trtllmGenFusedOps.h, cpp/tensorrt_llm/nanobind/thop/bindings.cpp
trtllmGenGenerationPreprocess and the exported nanobind binding accept mrope_position_deltas after rotary_cos_sin.
Binding forwarding
cpp/tensorrt_llm/nanobind/thop/bindings.cpp, cpp/tensorrt_llm/thop/trtllmGenQKVProcessOp.cpp
trtllmGenGenerationPreprocessBinding forwards mrope_position_deltas, and generation preprocessing stores it in qkvParams.mrope_position_deltas with comments separating context and generation mRoPE handling.
Python call site
tensorrt_llm/_torch/attention_backend/fmha/flashinfer_trtllm_gen.py
run_generation passes fwd.mrope_position_deltas into thop.trtllm_gen_generation_preprocess.

Marlin NVFP4 stub update

Layer / File(s) Summary
Stub declaration update
cpp/tensorrt_llm/kernels/marlin/marlin_nvfp4_template.h
The non-Hopper Marlin stub removes has_act_order, uses MARLIN_KERNEL_PARAMS, and updates the copyright year and namespace closing comment.

Integration waiver cleanup

Layer / File(s) Summary
Waiver removals
tests/integration/test_lists/waives.txt
Three platform-specific waiver entries for TestQwen2_5_VL_7B::test_auto_dtype are deleted.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • NVIDIA/TensorRT-LLM#13476: Updates the Marlin NVFP4 kernel template declarations in the same header and follows the same stub-signature pattern used here.
  • NVIDIA/TensorRT-LLM#15505: Also changes tests/integration/test_lists/waives.txt around the TestQwen2_5_VL_7B::test_auto_dtype waiver set.
  • NVIDIA/TensorRT-LLM#15506: Also edits tests/integration/test_lists/waives.txt for the same multimodal test waiver family and platform-specific entries.

Suggested reviewers

  • xxi-nv
  • QiJune
  • xinhe-nv
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: fixing RoPE support in the flashinfer trtllm-gen backend.
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.
Description check ✅ Passed The PR includes the required sections and clearly explains the issue and fix, though the Test Coverage section is left blank.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #56058 [ run ] triggered by Bot. Commit: a377180 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #56059 [ run ] triggered by Bot. Commit: a377180 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #56058 [ run ] completed with state ABORTED. Commit: a377180

Link to invocation

Comment thread tensorrt_llm/_torch/attention_backend/fmha/flashinfer_trtllm_gen.py
@yihwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #56079 [ run ] triggered by Bot. Commit: 07ee7b7 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #56059 [ run ] completed with state ABORTED. Commit: a377180

Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #56079 [ run ] completed with state SUCCESS. Commit: 07ee7b7
/LLM/main/L0_MergeRequest_PR pipeline #44945 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@yihwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #56275 [ run ] triggered by Bot. Commit: 07ee7b7 Link to invocation

Comment thread cpp/tensorrt_llm/thop/trtllmGenQKVProcessOp.cpp Outdated
Comment thread cpp/tensorrt_llm/kernels/marlin/marlin_nvfp4_template.h
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #56275 [ run ] completed with state SUCCESS. Commit: 07ee7b7
/LLM/main/L0_MergeRequest_PR pipeline #45130 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@yihwang-nv
yihwang-nv force-pushed the yihwang/fix_rope_in_fi_trtllm_gen branch from 07ee7b7 to 544963f Compare July 7, 2026 06:13
…en backend

Signed-off-by: Yihan Wang <yihwang@nvidia.com>
…Blackwell

The trtllm-gen FMHA mRoPE numerics are now fixed (decode-phase
mrope_position_deltas is plumbed through), so remove the B200/GB200/GB300
SKIP waivers for TestQwen2_5_VL_7B::test_auto_dtype so CI covers it again.

Signed-off-by: Yihan Wang <yihwang@nvidia.com>
@yihwang-nv
yihwang-nv force-pushed the yihwang/fix_rope_in_fi_trtllm_gen branch from 544963f to e7a546a Compare July 7, 2026 06:26
@yihwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #57958 [ run ] triggered by Bot. Commit: e7a546a Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #57958 [ run ] completed with state SUCCESS. Commit: e7a546a
/LLM/main/L0_MergeRequest_PR pipeline #46636 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@yihwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58130 [ run ] triggered by Bot. Commit: e7a546a Link to invocation

@yihwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

1 similar comment
@yihwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58198 [ run ] triggered by Bot. Commit: 39ab907 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58130 [ run ] completed with state ABORTED. Commit: e7a546a

Link to invocation

@yihwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58206 [ run ] triggered by Bot. Commit: 39ab907 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58198 [ run ] completed with state ABORTED. Commit: 39ab907

Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58206 [ run ] completed with state SUCCESS. Commit: 39ab907
/LLM/main/L0_MergeRequest_PR pipeline #46850 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@yihwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58354 [ run ] triggered by Bot. Commit: 39ab907 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58354 [ run ] completed with state SUCCESS. Commit: 39ab907
/LLM/main/L0_MergeRequest_PR pipeline #46982 completed with status: 'SUCCESS'

CI Report

Link to invocation

@yuxianq
yuxianq merged commit 201b077 into NVIDIA:main Jul 9, 2026
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants