Repository navigation
[Bugfix] Optionally bind fa3-fwd dense FlashAttention on Hopper - #9201
Conversation
vllm-project#7735 stopped binding flash_attn_func, so dense diffusion attention fell onto vLLM varlen and stayed slow after vllm-project#9100. Bind fa3-fwd only when the package is already installed. Signed-off-by: NumberWan <wantszkin2003@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@amy-why-3459 PTAL |
|
@natureofnature PTAL |
|
This PR appears to belong to: docs/design/module/diffusion/index.md. Module owners: @Isotr0py @david6666666 @princepride Routing: @Isotr0py via module of the changed files, module named in the PR description, CODEOWNERS; @david6666666 via module of the changed files, module named in the PR description; @princepride via module of the changed files, module named in the PR description @NumberWan, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the careful bisect, the before/after table makes the regression and the fix easy to follow. Keeping fa3-fwd optional and leaving the #9100 fallback intact makes sense.
Two things I'd like to see tightened before this lands, both about the scope of the new bind:
- The bind runs on every non-Blackwell CUDA device, but
fa3_fwd_interfaceonly ships Hopper (SM90) kernels. On SM80/SM89 with the wheel present (e.g. a shared image), we'd now route dense attention to a kernel that can't launch, instead of vLLM varlen FA2 as today. - It also swaps
flash_attn_varlen_func, which moves the masked, packed and piecewise paths (and the FP8 path, viaflash_attn_func) off vLLM's dispatcher. The measurements only cover the dense unmasked Qwen-Image case.
Details inline. Minor: an info log when fa3_fwd_interface gets bound (as the FA4 branch does) would help, since the PR body itself shows that whether the wheel is actually used isn't obvious from the outside.
natureofnature
left a comment
There was a problem hiding this comment.
Reviewed both changed files, provider discovery, dense/masked/packed/piecewise and FP8 call sites, and the reported current-head tests. The latency recovery is useful evidence for the measured Qwen-Image BF16 workload. I left one FP16 compatibility finding inline and followed up in the existing provider-validation discussion; wheel/build checks passed, but numerical coverage of the newly selected routes is still needed.
【Conclusion: Found blocking code issues and validation gaps in commit 0997e4b.】
【Action recommended: Preserve working FP16 attention with the wheel installed, and verify BF16 dense/masked/packed outputs plus Hopper FP8 accuracy before merging.】
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
Omni ReviewBot: three questions on the performance claim@NumberWan this PR reads as a performance or value claim:
Before the full evidence checklist, three short questions:
When you answer, the evidence that settles it is: base and head SHA, hardware, model, workload, warm-up and repeat count, mean or percentiles with their spread, and a correctness/quality-equivalence signal; an end-to-end claim also needs stage attribution. |
Keep vLLM varlen for masked/packed/piecewise/FP8, skip FP16 where fa3-fwd disables kernels, and gate the optional bind with _is_hopper. Signed-off-by: NumberWan <wantszkin2003@gmail.com>
Author self-reviewChecked on
|
|
Follow-up: L20X
Binding check on that run: |
natureofnature
left a comment
There was a problem hiding this comment.
Rechecked all three changed files and the dense/masked/packed/piecewise/FP8 call paths. The FP16 finding is fixed: the optional callable is separate, and FP16 falls back to the existing implementation. The broader provider changes are also removed, so my previous masked/packed/FP8 validation request no longer applies to this revision. The reported 63 CPU passes and current-head L20X latency run support the routing fix and performance recovery.
I suggest verifying BF16 unmasked dense outputs against SDPA on this head with fa3-fwd==0.0.3 installed on Hopper before merging, including the Qwen-Image attention geometry and confirming that optional_fa3_fwd_dense_func is actually selected. Outputs should be finite and agree within the existing attention tests' BF16 tolerances. This is the remaining provider change; the latency run does not check numerical equivalence, and the direct vLLM adapter test bypasses this selector.
【Conclusion: Static review found no blocking code issues in commit ffd87f4.】
【Action recommended: Verify the selected Hopper BF16 dense path against SDPA within the existing BF16 tolerances before merging.】
|
@natureofnature Verified the selected Hopper BF16 unmasked dense path against SDPA on Host: NVIDIA L20X, SM90 Qwen-Image geometry from Tolerance is the existing BF16 bound in
Both also sit inside the tighter CUDA bounds used by |
…-project#9201) Signed-off-by: NumberWan <wantszkin2003@gmail.com> (cherry picked from commit bea9b9e)
#9201 (#9206) Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Signed-off-by: kunkunblueberry <1833921874@qq.com> Signed-off-by: y-null <y-null@users.noreply.github.com> Signed-off-by: yasong.wang <yasong.wang@inferact.ai> Signed-off-by: wangyu <410167048@qq.com> Signed-off-by: NumberWan <wantszkin2003@gmail.com> Co-authored-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: amy-why-3459 <amy-why-3459@users.noreply.github.com> Co-authored-by: kunkun <72174834+kunkunblueberry@users.noreply.github.com> Co-authored-by: y-null <y_null@qq.com> Co-authored-by: y-null <y-null@users.noreply.github.com> Co-authored-by: Roy Wang <jasonailu87@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: wangyu <53896905+yenuo26@users.noreply.github.com> Co-authored-by: NumberWan <wantszkin2003@gmail.com>
…-project#9201) Signed-off-by: NumberWan <wantszkin2003@gmail.com>
Fixes #9092.
1. What outcome do you want to achieve?
Put Qwen-Image dense attention back on the pre-#7735 FA3 path when
fa3-fwdis installed, so single-device 512 latency returns to that band. Keepfa3-fwdoptional. Do not revert #7735.2. What is the highest-leverage variable for that outcome?
Whether CUDA dense attention has
flash_attn_func. After #7735 that binding isNone, soFlashAttentionImpl.forward_cudaemulates the dense call with vLLMflash_attn_varlen_func. #9100 only passesfa_versionon that varlen call. On this L20X that is still much slower thanfa3_fwd_interface.flash_attn_func.3. How will this work move that variable?
After the vLLM varlen candidate is bound, and only on non-Blackwell, import
fa3_fwd_interfaceif it is present and bind bothflash_attn_funcandflash_attn_varlen_funcfrom it. If the import fails, leave the vLLM varlen + #9100fa_versionpath unchanged.requirements/cuda.txtstays withoutfa3-fwd.4. Whose customer or user needs does this address, and what are those needs?
People running Qwen-Image (and any other dense diffusion attention on the same backend) on Hopper. #9092 reports the perf case about 60% slower than baseline after the image moved off dense FA3.
Purpose
Before #7735 (
ebe2c1827), CUDA candidate 2 importedfa3_fwd_interfaceand set bothflash_attn_funcandflash_attn_varlen_func. Dense, unmasked attention inFlashAttentionImpl.forward_cudacalledflash_attn_funcdirectly.#7735 removed
fa3-fwdfromrequirements/cuda.txtand replaced that candidate with vLLM'sflash_attn_varlen_funconly.flash_attn_funcstayedNone, so dense attention always went through_forward_varlen_dense. vLLM's dispatcher defaults to FA2 unlessfa_versionis passed. #9100 (vllm_varlen_version_kwargs) fixes that version on the varlen call site (FA3 on SM90). It does not restore a dense FA3 kernel. On Qwen-Image the varlen FA3 path is still far from the old dense FA3 number.This PR inserts one optional bind,
_bind_optional_fa3_fwd, after the vLLM varlen candidate and before standaloneflash_attn_interface/ FA2, and only when the device is not Blackwell:flash_attn_funcalready set (FA4, AITER, MUSA, or an earlier candidate) is left alone.fa3_fwd_interfaceimportable: both dense and varlen callables come from that package, matching pre-[Kernel] Remove fa3-fwd dependency #7735 candidate 2.fa3-fwdis not added back as a hard dependency.No user guide or example changes. Backend selection is unchanged. The only new behavior is "use the optional dense FA3 package when it is already installed."
CI images that do not install
fa3-fwdwill keep the post-#9100 varlen FA3 path. This PR does not by itself move an H100 nightly that lacks the package back to the old JSON baseline.What we measured before the fix
Same host (NVIDIA L20X, SM90), same probe
test_qwen_image_single_device/512x512_steps20(512x512, 20 steps, 10 prompts, concurrency 1), modelQwen/Qwen-Imagefrom the local hub snapshot.flash_attn_func4ec9e5a31fa3_fwd_interfaceebe2c1827None6839c4d16Nonefa_version=315bce3458withfa3-fwd==0.0.3installed but not selectedNonefa_version=3Installing the wheel is not enough. Main never imports it, so the package sitting in the environment does not change the path.
Test Plan
vLLM Version: 0.32.0 (
torch2.13.0+cu130, CUDA 13.0)vLLM-Omni Commit:
0997e4b08on base15bce3458Hardware: 1x NVIDIA L20X (SM90). Optional package:
fa3-fwd==0.0.3._bind_optional_fa3_fwd(package present, package absent, existing dense callable left alone)tests/diffusion/attention/test_flash_attn.py -m "core_model and cpu"on this committest_qwen_image_single_device/512x512_steps20on this commit withfa3-fwdinstalled15bce3458withfa3-fwdinstalled, to show the wheel alone does not recover latencyfa3-fwd, to show the varlen FA3 fallback still runsTest Result
Passed: optional bind
With
fa3-fwd==0.0.3importable, this commit binds:flash_attn_func=fa3_fwd_interface.flash_attn_funcflash_attn_varlen_func=fa3_fwd_interface.flash_attn_varlen_funcWith the package uninstalled,
flash_attn_funcstaysNone, varlen staysvllm.vllm_flash_attn, andvllm_varlen_version_kwargsreturns{"fa_version": 3}on this SM90 host.requirements/cuda.txtdoes not mentionfa3-fwd.Passed: CPU tests
pytest tests/diffusion/attention/test_flash_attn.py -m "core_model and cpu" -o addopts= -p no:xdist58 passed, 7 deselected in 0.97s. The three new tests are in that run:
test_bind_optional_fa3_fwd_sets_dense_when_package_presenttest_bind_optional_fa3_fwd_keeps_vllm_varlen_when_package_absenttest_bind_optional_fa3_fwd_does_not_override_existing_densePassed: L20X Qwen-Image 512, same machine
Command shape (working tree, no checkout of the dirty fix during the "with fix" run):
Config is the upstream
test_qwen_image_single_deviceentry restricted to512x512_steps20, model path pointed at the localQwen-Imagesnapshot.num-prompts=10,max-concurrency=1, 20 steps, negative prompt enabled.QwenImagePipeline.diffuse4ec9e5a3115bce34580997e4b080997e4b08With the package, latency is 35% lower than the same-tree control (3.4534 s -> 2.2444 s) and sits next to the #7735 parent (2.2832 s). Without the package the server still comes up and the probe finishes on varlen FA3, in the same slow band as current main.
Not run
test_qwen_image_vllm_omni.json. No H100 or B200 on this host. An image that does not installfa3-fwdwill not pick up the dense path.1536x1536_steps35, 4-GPU, andcache_dit. [Bug][Perf]: qwen-image, performance metrics regressed by more than 10% compared to the baseline in some scenarios #9092 CI moved those the same direction as single-device 512; this validation stays on the single-device probe that isolated [Kernel] Remove fa3-fwd dependency #7735.