Repository navigation
[Bugfix][Qwen-Image] Restore RotaryEmbedding CUDA RoPE for Diffusers e2e - #7513
Conversation
|
This PR appears to belong to: docs/design/module/diffusion/offloader.md, docs/design/module/diffusion/diffusion_model_integration.md, docs/design/module/diffusion/index.md. Module owners: @Bounty-hunter @fhfuih @wtomin Routing: @Bounty-hunter via module of the changed files, CODEOWNERS; @fhfuih via module of the changed files, CODEOWNERS; @wtomin via module of the changed files, CODEOWNERS @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. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
|
@yenuo26 PTAL |
fhfuih
left a comment
There was a problem hiding this comment.
I do remember having tested this code when reviewing #6110. The CUDAs-specific branch is introduced when Rebasing onto 0.29.0, and this PR actually reverts it. When testing it back then, I remember CUDA does pass the accuracy test. Together with the latest test results in the PR description, this PR looks good to me.
QwenImageTransformerBlock. That matches the Diffusers helper in unit tests but drops Omni vs Diffusers pipeline PSNR below 27. Use the pre-vllm-project#7230 RotaryEmbedding path in forward again; keep the helper for CPU tests. Fixes vllm-project#7494 Signed-off-by: NumberWan <wantszkin2003@gmail.com>
Forward no longer calls _apply_qwen_image_rotary_emb. Remove the function and the CPU test that only covered it. Signed-off-by: NumberWan <wantszkin2003@gmail.com>
Keep vllm-project#5931 fused QK-RoPE. Eager CUDA fallback uses RotaryEmbedding like other devices. Tests keep a local FP32 reference for the fused kernel. Signed-off-by: NumberWan <wantszkin2003@gmail.com>
b5e8bce to
a7f9638
Compare
|
please fix pre-commit |
Signed-off-by: NumberWan <wantszkin2003@gmail.com>
@yenuo26 Thanks — pre-commit is fixed on this branch ( |
Nightly test_qwen_image_matches_diffusers on Buildkite #15339 scored SSIM 0.978375. Raise SSIM_THRESHOLD so a drop is visible. Signed-off-by: NumberWan <wantszkin2003@gmail.com>
Omni ReviewBot routing recordAssigned Direct under experiment |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
PR description
This bugfix restores Qwen-Image’s non-fused CUDA RoPE path to shared RotaryEmbedding (BF16/activation-dtype cos/sin) after #7230’s FP32 complex multiply helper regressed Omni↔Diffusers pipeline PSNR below the nightly gate. The dead _apply_qwen_image_rotary_emb helper and its unit test are removed; fused Q/K path and e2e thresholds are left intact. User-visible effect is recovering Diffusers-matching image quality on the CUDA eager RoPE path without loosening PSNR_THRESHOLD.
Change flow
flowchart TD
A["[EXISTING] QwenImageCrossAttention.forward<br/>vid/txt freqs + qk_norm"]:::existing
B["[CHANGED] _qwen_image_qk_norm_rope<br/>eager: RotaryEmbedding on all devices"]:::changed
C["[REMOVED] _apply_qwen_image_rotary_emb<br/>CUDA FP32 complex multiply"]:::removed
D["[CHANGED] benchmark + fused fallback tests<br/>native/eager pin RotaryEmbedding"]:::changed
E["[EXISTING] test_qwen_image_matches_diffusers<br/>SSIM/PSNR vs Diffusers pipeline"]:::existing
A --> B
B -.-> C
B --> D
B --> E
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
See inline comments below.
…into one main (vllm-project#5931/vllm-project#7513) already fuses Q/K RMSNorm + RoPE per stream and then concatenates text and image into the joint sequence. Replace the two launches and the three cats with a single fused_joint_qkv_norm_rope call that writes the joint [B, S_txt+S_img, H, D] Q/K/V attention consumes, with no intermediate copies. The table carries the same fp32 coefficients the per-stream path builds, so the joint launch is bitwise equal to it. Any sequence parallelism keeps the per-stream chain: vid_freqs is sharded while txt_freqs is not. Signed-off-by: HuangYuwei <yuweih205@gmail.com>
…into one main (vllm-project#5931/vllm-project#7513) already fuses Q/K RMSNorm + RoPE per stream and then concatenates text and image into the joint sequence. Replace the two launches and the three cats with a single fused_joint_qkv_norm_rope call that writes the joint [B, S_txt+S_img, H, D] Q/K/V attention consumes, with no intermediate copies. The table carries the same fp32 coefficients the per-stream path builds, so the joint launch is bitwise equal to it. Any sequence parallelism keeps the per-stream chain: vid_freqs is sharded while txt_freqs is not. Signed-off-by: HuangYuwei <yuweih205@gmail.com>
…e2e (vllm-project#7513) Signed-off-by: NumberWan <wantszkin2003@gmail.com> Co-authored-by: wangyu <53896905+yenuo26@users.noreply.github.com> Signed-off-by: Matthieu Laneuville <matthieu.laneuville@surf.nl>
…e2e (vllm-project#7513) Signed-off-by: NumberWan <wantszkin2003@gmail.com> Co-authored-by: wangyu <53896905+yenuo26@users.noreply.github.com>
Summary
test_qwen_image_matches_diffusersPSNR 26.13 vs gate 27._apply_qwen_image_rotary_emb(FP32 complex multiply) inside regional compile ofQwenImageTransformerBlock. The helper matches Diffusersapply_rotary_emb_qwen(..., use_real=False)(unit test atol=0), but this e2e test scores Omni vs Diffusers pipeline images, not that helper. Inductor also cannot codegen complex ops.RotaryEmbeddingpath again on all devices._apply_qwen_image_rotary_emband its CPU helper unit test are removed as dead code.cos/sintoimg_query.dtype(BF16) is intentional, not a leftover bug: Qwen-Image activations are BF16, andRotaryEmbedding.forward_cudafeedsvllm_flash_attnrotary, which expects frequency tables in the same dtype asx. [Rebase] Rebase to vLLM 0.29.0 #7230 treated that round as a mismatch vs the Diffusers helper; this e2e gate measures pipeline images, where the BF16 kernel path is the one that matches.Local L20X, same nightly recipe (FA3 hub,
--fa-deterministic, 512² / 20-step / seed 42). Absolute numbers are not H100; this box is stable:038c9b948fPSNR < 27)RotaryEmbedding25.98 is the same band as H100 26.13. Not lowering
PSNR_THRESHOLD.Test plan
tests/e2e/accuracy/test_qwen_image.py::test_qwen_image_matches_diffusersafter revert → SSIM 0.987344 / PSNR 34.317nightly-test):Diffusion X2I(&A&T) · Accuracy Testtest_qwen_image_matches_diffusers→ SSIM 0.978375 / PSNR 31.83 (then raisedSSIM_THRESHOLDto 0.97;PSNR_THRESHOLDstays 27)test_diffusers_backend_t2i_matches_diffuserspassedready):Diffusion · Qwen Image Testpytest -s -v tests/e2e/online_serving/test_qwen_image.py -m 'core_model' --run-level 'core_model'merge-test):Diffusion · Qwen Image Testpytest -s -v tests/e2e/online_serving/test_qwen_image.py -m 'advanced_model and cuda' --run-level 'advanced_model'Ready/merge serving jobs did not run on #15339 (nightly pipeline only). They should run when those labels are applied; this change only restores eager CUDA RoPE.
Test Result
ready/merge-testCI.