[None][feat] Add SkipSoftmax sparse attention support for visual generation - #12947
Conversation
e6e2898 to
fbe2732
Compare
200f766 to
c464578
Compare
c464578 to
48c17e8
Compare
📝 WalkthroughWalkthroughThis PR adds skip-softmax sparse attention configuration for diffusion visual-generation models. It introduces schema types for threshold formulas and per-layer overrides, auto-detection helpers for YAML and checkpoint metadata, backend parameter support, and pipeline integration to apply resolved thresholds to TRTLLM modules. ChangesSparse Attention Configuration and Integration
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tensorrt_llm/_torch/visual_gen/config.py (1)
408-409: 💤 Low valueLazy imports create circular dependency risk
The function imports
TrtllmAttentionandSkipSoftmaxAttentionConfigat call-time (lines 408-409) rather than at module level. This pattern typically indicates a circular import issue betweenconfig.pyand the attention backend.While functional, lazy imports add runtime overhead on each call and obscure the dependency graph. If the circular dependency can be resolved (e.g., by moving
apply_skip_softmax_overridesto a separate module or using TYPE_CHECKING), that would be cleaner.🤖 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/visual_gen/config.py` around lines 408 - 409, The lazy imports of TrtllmAttention and SkipSoftmaxAttentionConfig inside apply_skip_softmax_overrides introduce circular-import risk and runtime overhead; to fix, move those imports to the module top (module-level imports) or guard them with typing.TYPE_CHECKING, or alternatively refactor apply_skip_softmax_overrides into a new module (e.g., visual_gen/skip_softmax_utils) that can import TrtllmAttention and SkipSoftmaxAttentionConfig without creating a cycle; update references to apply_skip_softmax_overrides accordingly and ensure unit tests/imports still pass.tests/unittest/_torch/visual_gen/test_skip_softmax_config.py (1)
147-162: 💤 Low valueOptional: Silence Ruff RUF012 for test fixture constant
Ruff flags
MODELOPT_CHECKPOINTas a mutable class attribute, but since this is a read-only test fixture, the warning is a false positive.🔧 Optional fix to silence static analysis
+from typing import Final + class TestUseCaseScenarios: ... - MODELOPT_CHECKPOINT = { + MODELOPT_CHECKPOINT: Final = { "sparse_attention_config": { ... } }Alternatively, move the constant outside the class (module-level) if it's not logically tied to the test class.
🤖 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/visual_gen/test_skip_softmax_config.py` around lines 147 - 162, MODELOPT_CHECKPOINT is being flagged by Ruff as a mutable class attribute but it's a read-only test fixture; to silence RUF012 either move MODELOPT_CHECKPOINT out of the test class to the module level (so it’s a true constant) or keep it in the class and append a per-line ignore comment for Ruff (RUF012) to the declaration; update the test file so references to MODELOPT_CHECKPOINT still resolve and run tests to confirm no behavioral change.
🤖 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/visual_gen/attention_backend/trtllm.py`:
- Line 187: The parameter sparse_attention_config in the function/method defined
in trtllm.py lacks a type annotation; update its signature to include an
explicit type (for example sparse_attention_config: Optional[Dict[str, Any]] =
None) and add the required imports (from typing import Optional, Dict, Any) at
the top of the module; ensure you use the exact parameter name
sparse_attention_config and adjust any related callers or type checks if they
rely on None vs a typed mapping.
---
Nitpick comments:
In `@tensorrt_llm/_torch/visual_gen/config.py`:
- Around line 408-409: The lazy imports of TrtllmAttention and
SkipSoftmaxAttentionConfig inside apply_skip_softmax_overrides introduce
circular-import risk and runtime overhead; to fix, move those imports to the
module top (module-level imports) or guard them with typing.TYPE_CHECKING, or
alternatively refactor apply_skip_softmax_overrides into a new module (e.g.,
visual_gen/skip_softmax_utils) that can import TrtllmAttention and
SkipSoftmaxAttentionConfig without creating a cycle; update references to
apply_skip_softmax_overrides accordingly and ensure unit tests/imports still
pass.
In `@tests/unittest/_torch/visual_gen/test_skip_softmax_config.py`:
- Around line 147-162: MODELOPT_CHECKPOINT is being flagged by Ruff as a mutable
class attribute but it's a read-only test fixture; to silence RUF012 either move
MODELOPT_CHECKPOINT out of the test class to the module level (so it’s a true
constant) or keep it in the class and append a per-line ignore comment for Ruff
(RUF012) to the declaration; update the test file so references to
MODELOPT_CHECKPOINT still resolve and run tests to confirm no behavioral change.
🪄 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: 871ce345-e692-4912-8ab3-716766dddc17
📒 Files selected for processing (5)
tensorrt_llm/_torch/visual_gen/attention_backend/trtllm.pytensorrt_llm/_torch/visual_gen/config.pytensorrt_llm/_torch/visual_gen/modules/attention.pytensorrt_llm/_torch/visual_gen/pipeline_loader.pytests/unittest/_torch/visual_gen/test_skip_softmax_config.py
e4a895f to
4d82ce2
Compare
…on-config comment Three review fixes from bobboli on PR NVIDIA#12947: 1. (Bug, blocking) `_validate_sage_attn_config` was sitting INSIDE `auto_detect_sparse_yaml` after a `return`, making it dead code — silently regressing PR NVIDIA#13570 (sage-attention validation). The rebase put it in the wrong place; this restores it as a method of `AttentionConfig` right after `sparse_config_path`. Added two regression tests: - `test_sage_attn_requires_trtllm_backend` - `test_sage_attn_rejects_unsupported_block_combo` 2. (Cleanup) The dynamic-read-contract comment on `TrtllmAttention.sparse_attention_config` referred to an internal doc and was overly verbose. Trimmed to 4 lines stating the contract only. Signed-off-by: Kanghwan Jang <861393+karljang@users.noreply.github.com>
|
BTW, could you add a subsection |
…on-config comment Three review fixes from bobboli on PR NVIDIA#12947: 1. (Bug, blocking) `_validate_sage_attn_config` was sitting INSIDE `auto_detect_sparse_yaml` after a `return`, making it dead code — silently regressing PR NVIDIA#13570 (sage-attention validation). The rebase put it in the wrong place; this restores it as a method of `AttentionConfig` right after `sparse_config_path`. Added two regression tests: - `test_sage_attn_requires_trtllm_backend` - `test_sage_attn_rejects_unsupported_block_combo` 2. (Cleanup) The dynamic-read-contract comment on `TrtllmAttention.sparse_attention_config` referred to an internal doc and was overly verbose. Trimmed to 4 lines stating the contract only. Signed-off-by: Kanghwan Jang <861393+karljang@users.noreply.github.com>
7e2d403 to
c1f3d6e
Compare
Could you please check the updated sparse-attention.md? |
|
/bot run --disable-fail-fast |
|
PR_Github #49248 [ run ] triggered by Bot. Commit: |
|
PR_Github #49248 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #49339 [ run ] triggered by Bot. Commit: |
|
PR_Github #49339 [ run ] completed with state |
chang-l
left a comment
There was a problem hiding this comment.
It looks like skip-softmax calibration can come from both ModelOpt sparse.yaml and checkpoint config.json. Why do we need a dedicated sparse_config_path in addition to the normal config path? Is this sparse YAML a separate ModelOpt artifact, or is it supposed to be the same kind of YAML users pass to LLM/server config?
f13f501 to
b1e9adb
Compare
chang-l
left a comment
There was a problem hiding this comment.
Thanks for the effort. Can we have a follow-up doc update PR to cover:
- How to enable skipSoftmax from both sources: the user’s server config YAML and the ModelOpt checkpoint config YAML, including how we merge them or which source takes precedence if conflict.
- Some of your skipSoftmax accuracy findings and perf trade-offs for AIGV models (referring to @yibinl-nvidia 's PR that adds the LPIPS metric)
Sure, I'll make a follow-up PR to cover those items. Thank you! |
Signed-off-by: Kanghwan Jang <861393+karljang@users.noreply.github.com>
b1e9adb to
f7726a4
Compare
Signed-off-by: Kanghwan Jang <861393+karljang@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #50004 [ run ] triggered by Bot. Commit: |
|
PR_Github #50004 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #50062 [ run ] triggered by Bot. Commit: |
|
PR_Github #50062 [ run ] completed with state |
zhenhuaw-me
left a comment
There was a problem hiding this comment.
Hi @karljang , thanks you for rebasing on the VisualGenArgs PR and getting this PR merged! I apologize that didn't get chance to take a look before, but I have 2 general concerns which I think we should try to address:
- The API needs rework. I can create a doc to align, but the general idea is that:
tensorrt_llm/visual_gendir is for API, all non-API should be intensorrt_llm/_torch/visual_gen. Debug purpose knob should be added through env var rather than inVisualGenArgs. - No E2E testing. With the new commits we don't know if SkipSoftmax is works while we claim the support in the doc. We should try to add E2E testing (1 test is better than 0) if we are targeting to support as a product feature.
cc @chang-l in case he has other comments. Thanks!
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| """Skip-softmax sparse attention helpers for visual generation.""" |
There was a problem hiding this comment.
It limits the scope to skip softmax
There was a problem hiding this comment.
Sure, I'll address that in a follow up PR~
| return values | ||
|
|
||
|
|
||
| class SkipSoftmaxConfig(SkipSoftmaxAttentionConfig): |
There was a problem hiding this comment.
Why do we need SkipSoftmaxConfig for VisualGen instead of re-exporting SkipSoftmaxAttentionConfig while "there is no new user-facing fields"?
There was a problem hiding this comment.
SkipSoftmaxConfig extends the parent with private calibration state: per-layer threshold overrides and formula-based resolution from ModelOpt's diffusion-format coefficients. The parent SkipSoftmaxAttentionConfig only handles uniform thresholds, it has no concept of per-layer disabled layers or component routing.
Still, I agree this class is internal machinery rather than user-facing API. I can move it to tensorrt_llm/_torch/visual_gen/ in a follow-up and keep only the parent SkipSoftmaxAttentionConfig as the user-facing type in the public dir.
| status="prototype", | ||
| description="Sparse attention configuration. Currently supports: skip_softmax.", | ||
| ) | ||
| sparse_config_path: Optional[str] = Field( |
There was a problem hiding this comment.
What's the purpose of this config? It seems to me that we can auto-detect from the checkpoint and this knob sounds like a debug one?
There was a problem hiding this comment.
The sparse_config_path supports the case where users have a separately produced ModelOpt calibration YAML they want to apply, because users don't have to fully calibrate their checkpoints. That said, if we want to minimize the public API surface, we could move this to an env var. Happy to discuss the right placement.
There was a problem hiding this comment.
It seems to me that only the users that are as powerful as TRT-LLM/ModelOpt dev team knows how to use this knob? I'd suggest moving to an env var at this stage and bring to be API if we see explicit user requirements.
…ration (NVIDIA#12947) Signed-off-by: Kanghwan Jang <861393+karljang@users.noreply.github.com>
…ration (NVIDIA#12947) Signed-off-by: Kanghwan Jang <861393+karljang@users.noreply.github.com>
Summary
BaseSparseAttentionConfigwith discriminated union pattern (extensible for future algorithms) andSkipSoftmaxConfigconfig.json— no user config needed for calibrated checkpointslog_a) and LLM format (a) coefficients in the YAML / config — auto-normalized tolog_a = log(a)at the Pydantic layerBenchmark
WAN 2.2 T2V-A14B on B200, 720 × 1280, 81 frames, 50 inference steps (production recipe),
torch_compile=True, mean across 10 stress prompts. Calibration: ModelOptsparse-0506.yaml(formula + per-componentdisabled_layers). Quality measured as LPIPS-AlexNet against the same-precision baseline over all 81 frames; band thresholds are <0.30 same trajectory, 0.30–0.55 "scene preserved", >0.55 "diverged".Per-precision speedup at three representative targets
In-precision$S = T_\text{baseline (same precision, compile on)} / T_\text{skip}$ .
Dense fill at 1pp resolution from 0.60 → 0.75 (omitted from this table) localises the LPIPS-0.55 cliff between t=0.71 (FP8) and t=0.73 (NVFP4); t=0.70 is the last universally safe ceiling (all three precisions LPIPS mean ≤ 0.547).
Joint speedup vs BF16 no-compile reference (638 s baseline)
torch_compile=TrueonlyFactors compose roughly multiplicatively. The recommended
t=0.70configuration buys ~60 % over the previous-gen no-compile BF16 stack while keeping every prompt's LPIPS in the "scene preserved" band.Kernel breakdown (nsys, baseline inference, no compile)
Self-attention dominates at 720p (quadratic in seq_len ≈ 75 K), which is why skip-softmax has this much headroom on visual-gen workloads. Cross-attention is negligible — not worth targeting.
Use Cases
Case 1: Normal HF checkpoint (no skip_softmax in config.json)
Case 2: ModelOpt checkpoint (has calibrated a, b in config.json)
Priority Order
threshold_scale_factor(if set, used directly — no formula needed)target_sparsity+ userformula(user formula takes precedence)target_sparsity+ checkpoint formula (auto-detected from config.json)Changes
_torch/visual_gen/config.pyBaseSparseAttentionConfig,SkipSoftmaxConfig,SkipSoftmaxFormulawith dual-format (log_aora) validator,auto_detect_sparse_attention_config(),apply_skip_softmax_overrides(), cachedget_or_resolve_threshold()(PrivateAttr), strictsparse.yaml/sparse.*.yamlauto-detect glob_torch/visual_gen/attention_backend/trtllm.pysparse_attention_configin__init__; documents the post-construction dynamic-read contract_torch/visual_gen/modules/attention.py_torch/visual_gen/pipeline_loader.pyapply_skip_softmax_overrides()+ auto-detect from checkpointtests/unittest/_torch/visual_gen/test_skip_softmax_config.pyKnown Limitations
attentionOp.cpp:640). This is a TRT-LLM-wide limitation (also affects LLM models like MLLama). In WAN 2.2 the cost is negligible — cross-attn is 0.45 % of GPU time vs. self-attn at 53.88 % (nsys, 720p).Test plan
torch_compile=True): in-precisiontarget_sparsity=0.70for BF16/FP8/NVFP4, mean LPIPS ≤ 0.55 (universally safe)target_sparsity=0.70, STEPS=10 after PrivateAttr caching refactor: reproducesdisabled_layershonored)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests