[None][feat] Support DFlash RoPE, sliding-window configuration, and TRTLLM-gen attention backend - #16914
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDFlash now validates uniform effective RoPE settings, derives per-layer FlashAttention masking windows, and passes them to attention. Draft KV-cache window derivation and V1 budget splitting are updated, with unit and Qwen3.6 NVFP4 GSM8K coverage added. ChangesDFlash attention behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DFlashForCausalLM
participant get_attention_mask_args
participant flash_attn_with_kvcache
DFlashForCausalLM->>get_attention_mask_args: request arguments for layer_idx
get_attention_mask_args-->>DFlashForCausalLM: return causal and window_size
DFlashForCausalLM->>flash_attn_with_kvcache: pass causal and window_size
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unittest/_torch/modeling/test_modeling_speculative.py (1)
16-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage summary.
Added tests:
test_dflash_allows_mixed_layer_types_with_uniform_rope,test_dflash_rejects_different_effective_rope(both parametrized overrotary_emb/pos_embd_paramssources), andtest_dflash_attention_mask_args, exercisingDFlashForCausalLM._validate_uniform_ropeand_get_attention_mask_args. These are unit tests (not integration tests), so they aren't expected to appear intests/integration/test_lists/**.Coverage verdict: sufficient for the primary contract, but the following new error-raising branches in
_get_attention_mask_argsare untested:layer_typeslength mismatch vsnum_hidden_layers, an unsupportedlayer_typevalue, anduse_sliding_window=Truewith a missing/non-positivesliding_window. Consider adding a couple ofpytest.raisescases for these branches for completeness.As per path instructions for
tests/**: "Always produce a test coverage summary, even if no issues are found."Also applies to: 150-234
🤖 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/modeling/test_modeling_speculative.py` around lines 16 - 25, Add pytest.raises coverage for the validation branches in DFlashForCausalLM._get_attention_mask_args: layer_types length differing from num_hidden_layers, an unsupported layer_type, and use_sliding_window=True with a missing or non-positive sliding_window. Extend the existing test_dflash_attention_mask_args cases while preserving the current valid-path assertions.Source: Path instructions
tensorrt_llm/_torch/models/modeling_speculative.py (1)
1345-1379: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider caching per-layer mask args instead of recomputing every forward call.
_get_attention_mask_argsre-derives(causal, window_size)fromconfig.layer_types/sliding_windowon every call, but these values are static per layer once the config is loaded. Sincedflash_forwardcalls this once per layer on every decode step (a hot path), precomputing a list of(causal, window_size)tuples once (e.g. alongside_build_fused_kv_buffers) would avoid repeated attribute lookups and re-validation.🤖 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 `@tensorrt_llm/_torch/models/modeling_speculative.py` around lines 1345 - 1379, Cache the per-layer (causal, window_size) results during initialization, alongside _build_fused_kv_buffers, after validating layer_types and sliding-window configuration. Update _get_attention_mask_args to return the cached tuple for layer_idx without re-reading or re-validating config values, and ensure the cache preserves the existing defaults and ValueError behavior.
🤖 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 `@tensorrt_llm/_torch/models/modeling_speculative.py`:
- Around line 1345-1379: Cache the per-layer (causal, window_size) results
during initialization, alongside _build_fused_kv_buffers, after validating
layer_types and sliding-window configuration. Update _get_attention_mask_args to
return the cached tuple for layer_idx without re-reading or re-validating config
values, and ensure the cache preserves the existing defaults and ValueError
behavior.
In `@tests/unittest/_torch/modeling/test_modeling_speculative.py`:
- Around line 16-25: Add pytest.raises coverage for the validation branches in
DFlashForCausalLM._get_attention_mask_args: layer_types length differing from
num_hidden_layers, an unsupported layer_type, and use_sliding_window=True with a
missing or non-positive sliding_window. Extend the existing
test_dflash_attention_mask_args cases while preserving the current valid-path
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6bcbaa68-7fba-463e-9846-63e910c6d84b
📒 Files selected for processing (6)
tensorrt_llm/_torch/models/modeling_speculative.pytests/integration/defs/accuracy/references/gsm8k.yamltests/integration/defs/accuracy/test_llm_api_pytorch.pytests/integration/test_lists/qa/llm_function_core.txttests/integration/test_lists/test-db/l0_b200.ymltests/unittest/_torch/modeling/test_modeling_speculative.py
86c84bd to
282fc22
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unittest/_torch/executor/test_kv_cache_budget_split.py (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the new test method.
Add
-> Nonetotest_v1_separate_vswa_draft_requires_gpu_budget_split.As per coding guidelines, “Annotate every function.”
🤖 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_kv_cache_budget_split.py` at line 68, Add the explicit return annotation -> None to the test_v1_separate_vswa_draft_requires_gpu_budget_split test method, preserving its existing behavior and body.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.
Inline comments:
In `@tensorrt_llm/_torch/pyexecutor/_util.py`:
- Around line 1085-1098: Preserve the pre-target-manager sequence length in
build_managers before target SWA construction mutates self._max_seq_len, and
reuse it for both draft-window derivation sites, including
_get_draft_max_attention_window and the corresponding budget-split calculation.
Add a regression test covering target SWA with mixed draft layers such as
sliding followed by full attention, verifying the decision and final windows use
the original length.
---
Nitpick comments:
In `@tests/unittest/_torch/executor/test_kv_cache_budget_split.py`:
- Line 68: Add the explicit return annotation -> None to the
test_v1_separate_vswa_draft_requires_gpu_budget_split test method, preserving
its existing behavior and body.
🪄 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: 8b5baa98-3a9c-4578-974c-6020ea300f12
📒 Files selected for processing (9)
tensorrt_llm/_torch/models/modeling_speculative.pytensorrt_llm/_torch/pyexecutor/_util.pytests/integration/defs/accuracy/references/gsm8k.yamltests/integration/defs/accuracy/test_llm_api_pytorch.pytests/integration/test_lists/qa/llm_function_core.txttests/integration/test_lists/test-db/l0_b200.ymltests/unittest/_torch/executor/test_kv_cache_budget_split.pytests/unittest/_torch/modeling/test_modeling_speculative.pytests/unittest/_torch/speculative/test_eagle3.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/integration/test_lists/test-db/l0_b200.yml
- tests/integration/defs/accuracy/references/gsm8k.yaml
- tests/integration/test_lists/qa/llm_function_core.txt
- tests/integration/defs/accuracy/test_llm_api_pytorch.py
- tests/unittest/_torch/modeling/test_modeling_speculative.py
- tensorrt_llm/_torch/models/modeling_speculative.py
282fc22 to
54f1ced
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/unittest/_torch/executor/test_kv_cache_budget_split.py`:
- Around line 69-89: Update
test_v1_mixed_draft_uses_original_max_seq_len_for_budget_split to exercise
build_managers() rather than only calling _needs_gpu_kv_cache_budget_split() and
_get_draft_max_attention_window() directly. Mock the required manager-building
dependencies and verify the build path passes original_max_seq_len (16384) to
the relevant helper, while preserving coverage of the mixed V1 draft
configuration.
🪄 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: c02e6e9c-6f90-4239-a447-759e5c84c452
📒 Files selected for processing (8)
tensorrt_llm/_torch/models/modeling_speculative.pytensorrt_llm/_torch/pyexecutor/_util.pytests/integration/defs/accuracy/references/gsm8k.yamltests/integration/defs/accuracy/test_llm_api_pytorch.pytests/integration/test_lists/qa/llm_function_core.txttests/integration/test_lists/test-db/l0_b200.ymltests/unittest/_torch/executor/test_kv_cache_budget_split.pytests/unittest/_torch/modeling/test_modeling_speculative.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/integration/test_lists/qa/llm_function_core.txt
- tests/integration/defs/accuracy/references/gsm8k.yaml
- tensorrt_llm/_torch/models/modeling_speculative.py
- tests/integration/defs/accuracy/test_llm_api_pytorch.py
- tests/unittest/_torch/modeling/test_modeling_speculative.py
- tensorrt_llm/_torch/pyexecutor/_util.py
54f1ced to
cc69d53
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/_torch/models/modeling_speculative.py (1)
680-701: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUnused
residualfrom single-layer call.Ruff flags the
residualunpacked at Line 692 as unused. Sinceself.layerscontains exactly one layer here, discard it explicitly.🧹 Proposed fix
- hidden_states, residual = self.layers[0]( + hidden_states, _ = self.layers[0](🤖 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 `@tensorrt_llm/_torch/models/modeling_speculative.py` around lines 680 - 701, Update the single-layer call in the model forward path to explicitly discard the unused residual value instead of assigning it to residual. Preserve the existing hidden_states result and cleanup behavior in the surrounding try/finally block.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@tensorrt_llm/_torch/models/modeling_speculative.py`:
- Around line 680-701: Update the single-layer call in the model forward path to
explicitly discard the unused residual value instead of assigning it to
residual. Preserve the existing hidden_states result and cleanup behavior in the
surrounding try/finally block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c503fd15-4869-4073-81db-3ad7d90540ae
📒 Files selected for processing (8)
tensorrt_llm/_torch/models/modeling_speculative.pytensorrt_llm/_torch/pyexecutor/_util.pytests/integration/defs/accuracy/references/gsm8k.yamltests/integration/defs/accuracy/test_llm_api_pytorch.pytests/integration/test_lists/qa/llm_function_core.txttests/integration/test_lists/test-db/l0_b200.ymltests/unittest/_torch/executor/test_kv_cache_budget_split.pytests/unittest/_torch/modeling/test_modeling_speculative.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/integration/test_lists/qa/llm_function_core.txt
- tests/integration/test_lists/test-db/l0_b200.yml
- tests/unittest/_torch/executor/test_kv_cache_budget_split.py
- tests/integration/defs/accuracy/references/gsm8k.yaml
- tests/integration/defs/accuracy/test_llm_api_pytorch.py
- tests/unittest/_torch/modeling/test_modeling_speculative.py
- tensorrt_llm/_torch/pyexecutor/_util.py
|
PR_Github #66338 [ run ] triggered by Bot. Commit: |
|
PR_Github #66338 [ run ] completed with state
|
Validate the effective DFlash RoPE configuration, support mixed sliding-window drafter layers, split target and draft KV budgets correctly, and add unit and accuracy coverage. Signed-off-by: Jhao-Ting Chen <jtchen0528@gmail.com>
Signed-off-by: Jhao-Ting Chen <jtchen0528@gmail.com>
Centralize attention-window and VSWA classification, harden TRTLLM-Gen buffer management, and document the DFlash attention backend. Signed-off-by: Jhao-Ting Chen <jtchen0528@gmail.com>
Propagate the derived draft KV cache configuration through both one-model cost paths and align sliding-window validation and tests. Signed-off-by: GECOS <jtchen0528@gmail.com>
Signed-off-by: Jhao-Ting Chen <jtchen0528@gmail.com>
Signed-off-by: Jhao-Ting Chen <jtchen0528@gmail.com>
Signed-off-by: Jhao-Ting Chen <jtchen0528@gmail.com>
Signed-off-by: Jhao-Ting Chen <jtchen0528@gmail.com>
|
/bot run |
|
PR_Github #66415 [ run ] triggered by Bot. Commit: |
|
PR_Github #66415 [ run ] completed with state
|
|
/bot run |
|
PR_Github #66445 [ run ] triggered by Bot. Commit: |
|
PR_Github #66445 [ run ] completed with state
|
|
/bot run |
|
PR_Github #66507 [ ] completed with state |
|
/bot run |
|
PR_Github #66510 [ run ] triggered by Bot. Commit: |
|
PR_Github #66510 [ run ] completed with state
|
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #66517 [ run ] triggered by Bot. Commit: |
|
/bot run |
|
PR_Github #66590 [ run ] triggered by Bot. Commit: |
|
PR_Github #66517 [ run ] completed with state |
|
PR_Github #66590 [ run ] completed with state |
Description
Qwen3.6-35B-A3B-DFlash mixes sliding-window and full-attention draft layers. The DFlash path previously assumed simpler layer semantics: it shared one RoPE cache without validating the effective per-layer RoPE configuration, did not pass each layer's local-window arguments to FlashAttention, and could size a separate V1 draft KV cache from an unsplit budget or a target-clamped sequence length.
This PR adds end-to-end support for the combination of z-lab/Qwen3.6-35B-A3B-DFlash and nvidia/Qwen3.6-35B-A3B-NVFP4:
causalandwindow_sizearguments per draft layer. Qwen-style sliding-attention layers use their configured causal local window, full-attention layers remain global, and Laguna uses its checkpoint-defined uniform causal window.max_seq_lenwhen building the separate draft manager.max_gpu_total_bytesbetween target and draft before constructing the final V1 variable-window managers. The temporary estimation manager deliberately avoids installing the VSWA layout before that split is available.uses_vswa_kv_cache_layout()and exclude negative recurrent-state sentinels used by hybrid linear-attention models.TRTLLM-Gen speculative attention backend
The second commit adds an optional TRTLLM-Gen backend for DFlash drafter attention:
speculative_config.attention_backendwith the main-model-style valuesVANILLA(default) andTRTLLM.speculative_config.advanced_sampling_modeinto DFlash metadata so DFlash honors the configured sampling specialization instead of silently using the default.TRTLLMstores the pooled DFlash context K/V in a private HND paged cache shared by FlashInfer's paged append and the TRTLLM-Gen launchers. It dispatches the Blackwell generated context/decode FMHA kernels with CUDA-graph-compatible workspace and counter buffers.VANILLApreserves the existing FlashAttention implementation and contiguous context K/V cache.DFlash 7
trtllm-serveexampleThe TRTLLM-backend benchmark used the following equivalent standalone serving configuration.
attention_backenduses the same public value as the main-model attention backend:Benchmark results
Speed-Bench 2K throughput
Speed-Bench 2K throughput low-entropy dataset, 512 requests, average ISL 2210, requested OSL unset, and greedy decoding (
top_k=1). MTP uses draft length 3 and DFlash uses draft length 7. Generation stops at EOS;max_tokens=6144is only the safety cap.attention_backend=TRTLLM)Values are measured OTPS/user (output tokens/s/user).
¹ DFlash at concurrency 256 exceeds the available KV-cache capacity, so no measurement is reported.
Another run with TopK=0, TopP=1, Temp=0.7 (disable TopK, TopP).

Benchmark configuration notes:
TRTLLM_WINDOW_SIZE_SHARES=0.7,0.3to distribute the KV-cache budget between the sliding-window and full-attention pools.MAX_UTILIZATIONto admit more concurrent requests.MT-Bench acceptance length by category
DFlash draft length 7 (maximum acceptance length 8), greedy decoding (
temperature=0), TP1/EP1, B200. Each concurrency run contains 80 MT-Bench requests. The measured average actual OSL across all seven runs was 2,377.1 tokens per request; the per-run means ranged from 2,315.8 to 2,442.6 tokens, while 4,096 was the configured maximum output length.Overall average acceptance length remains stable across concurrency: c1
4.267, c24.262, c44.250, c84.265, c164.233, c324.290, and c644.234.These are greedy-decoding results. MT-Bench shows no systematic acceptance-length drop as concurrency increases: c64 remains at
4.234versus4.267at c1, and the intermediate measurements are similarly stable.Review follow-up
max_seq_lenbefore target SWA construction can clamp it, then reuse that value for the budget-split decision, draft-window derivation, and final draft-manager construction.int32cumulative sequence offsets.Refactoring
get_layer_attention_window()as the canonical interpreter foruse_sliding_window,sliding_window, andlayer_types:use_sliding_window=Falseexplicitly disables the window even when a scalar window remains in the HF config.use_sliding_window=Truerequires a positive integersliding_windowand fails early when it is absent or invalid.sliding_windowandlayer_types.layer_typespatterns are supported through modulo indexing; full-attention layers returnNone.layer_typeshonormax_window_layersas the first sliding-layer index._is_sliding_attention_layer()predicate toconfig_utils.pywithout changing its private naming or matching semantics, allowing model and executor code to share it.draft_config_defines_attention_layout()with explicit documentation of ownership semantics:use_sliding_window,sliding_window, orlayer_types, the draft layout is authoritative, including explicit full attention._normalize_attention_windows()to materialize full-attention entries asmax_seq_len, collapse all-full layouts toNone, collapse uniform layouts to one entry, and retain heterogeneous layouts for separate pools.uses_vswa_kv_cache_layout()toconfig_utils.py, removed the obsoleteis_vswa_enabled()copy, and reused it in V1, V2, and KV-connector guards. V2 estimation now counts actual pools separately, including hybrid linear/attention layouts._get_draft_max_attention_window()require an explicitKvCacheConfiginstead of implicitly reading the target configuration._get_one_model_draft_kv_cache_config()to clone the input config with the derived draft layout without mutating the target config._dflash_attention_backendand the model-side comparison value todraft_model_dflash_attention_backendto distinguish them from unrelated attention or MoE backends.int32once before the per-layer paged-append loop and hoisted the loop-invariant flattened page table, avoiding repeated conversion/allocation work.int32.28sliding +7full layers) and its actual512-token sliding window.Test Coverage
tests/unittest/_torch/modeling/test_modeling_speculative.pytests/unittest/_torch/executor/test_kv_cache_budget_split.pybuild_managers()preserves the originalmax_seq_lenand constructs the final mixed draft layout from it.tests/unittest/_torch/executor/test_kv_cache_estimation.pysliding_window=512.max_window_layersis covered by a focused helper test.tests/unittest/_torch/speculative/hw_agnostic/test_dflash_worker.pytests/integration/defs/accuracy/test_llm_api_pytorch.pytests/integration/defs/accuracy/references/gsm8k.yaml95.0.llm_function_core.txtandl0_b200.yml.5 passed; the full repository pre-commit suite, including compiled-binding mypy, passed.PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why.
PR follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions).
If the PR introduces API changes, an appropriate
api-compatibleorapi-breakinglabel is added.New dependencies, if any, have been scanned for license and vulnerabilities.
CODEOWNERS is updated if ownership changes.
Documentation and the TAVA architecture diagram are updated if needed.
The assigned reviewers are appropriate for the PR.
I reviewed the checklist above as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, comment
/bot help.