Repository navigation
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/diffusion/index.md. Module owners: @Isotr0py @princepride @SamitHuang @wtomin @ZJY0516 @RuixiangMa @david6666666 @xuechendi @tthakkal, 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. |
Signed-off-by: Thanaji Rao Thakkalapelli <thanaji.rao.thakkalapelli@intel.com>
Signed-off-by: Thanaji Rao Thakkalapelli <thanaji.rao.thakkalapelli@intel.com>
Signed-off-by: Thanaji Rao Thakkalapelli <thanaji.rao.thakkalapelli@intel.com>
Self-review @vllm-omni-review-bot
Checked and clearedRing path has no reroute gap ( Evidence (4× Intel Arc Pro B70, torch 2.13.0+xpu, vLLM 0.29.0)Verified natively with |
Signed-off-by: Thanaji Rao Thakkalapelli <thanaji.rao.thakkalapelli@intel.com>
Signed-off-by: Thanaji Rao Thakkalapelli <thanaji.rao.thakkalapelli@intel.com>
|
Could someone add the ready label so L1/L2 run? Fix is XPU-only (@yma11 @xuechendi FYI) — CUDA/ROCm/MUSA flag values are unchanged, verified by test_mask_capability_flags.py. |
|
@Joshna-Medisetty , please help to initial scan this PR |
|
scanned it. the xpu part looks correct to me - supports_piecewise_spans really is hardcoded True at flash_attn.py:24 and forward_xpu never reads full_attn_spans, so the flag gating plus the sdpa reroute is the right fix. main ask is whether this can be split. the actual xpu fix is abstract.py + the flash_attn.py flags + layer.py + the hidream one-liner. mask_excludes_tokens (cuda routing), the npu kv-quant raise, and the sequence_parallel gate are three separate fixes riding along. also the table says npu is Fixed but the follow-up section says mindiesd may invert that mask. can we mark npu unverified until CI passes? |
Signed-off-by: Thanaji Rao Thakkalapelli <thanaji.rao.thakkalapelli@intel.com>
@Joshna-Medisetty Thank you for the review, both asks done. The PR is now only the XPU correctness fix. @xuechendi Split out One addition to your list: |
Omni ReviewBot: no human activity for 7 days@tthakkal this pull request has had no human commit, comment or review since 2026-09-17. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
2 actionable finding(s).
CI at
2ca811e00f16(2026-10-10T01:42:52.024242+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 2 finding(s) below |
| Security | no finding reported |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
Validated:
- [resolved] NPU 4D mask → mindiesd: PR table marks unverified after Joshna; HiDream uses causal=False (hidream_o1_image_transformer.py:154) so forward_fa_npu takes the mindiesd path (flash_attn.py:962), not the torch_npu keep-mask invert (916).
- [resolved] split request: net diff matches the stated XPU-only set.
- [resolved] sdpa_fallback None: custom_attention returns at layer.py:584-585 before the reroute; sdpa_fallback is constructed at layer.py:226 when custom is None.
- [resolved] attn_spec dropped in _mask_needs_sdpa — author self-review already named FLASHINFER True/False-with-spec; residual: a future backend that answers False spec-less / True spec-ful would still reroute. Not republished.
- [resolved] Riding-along mask_excludes_tokens / NPU kv-quant raise / sequence_parallel truthiness gate — at this head sequence_parallel.py has no supports_piecewise_spans; grep confirms.
- [claim-verified] 'flag's one consumer' besides layer.py: repo-wide grep of supports_piecewise_spans in vllm_omni/ hits only hidream transformer + flash_attn*.py + abstract.py + layer.py.
2 actionable finding(s).
Verdict: COMMENT
Findings
- [P2] HiDream's piecewise probe is now a call (
supports_piecewise_spans()at hidrea… —vllm_omni/diffusion/models/hidream_o1_image/hidream_o1_image_transformer.py
Evidence for HiDream's piecewise probe is now a call (`supports_piecewise_spans()` at hidrea…
HiDream's piecewise probe is now a call (supports_piecewise_spans() at hidream_o1_image_transformer.py:518), which is what pipeline_hidream_o1_image.py:646 (if not self.model.supports_piecewise_attention) uses to build the 4D mask. No test exercises that property: test_hidream_o1_attention_mask.py only imports build_hidream_o1_attention_mask, and test_piecewise_spans_is_a_call_not_a_truthy_attribute only checks AttentionBackend.supports_piecewise_spans. Dropping the () makes all(...) True on a bound method, so the pipeline skips the mask and every added test still passes. Add a CPU test that stubs a transformer whose backends return False and asserts supports_piecewise_attention is False.
Evidence: hidream_o1_image_transformer.py:518 layer.self_attn.attn.attn_backend.supports_piecewise_spans() for layer in self.model.language_model.layers — only production probe for HiDream piecewise. Unchanged by this diff, present in the PR-time tree: pipeline_hidream_o1_image.py:646 if not self.model.supports_piecewise_attention: then :647-650 attention_mask = build_hidream_o1_attention_mask(...). Trigger: FLASH_ATTN on XPU/NPU where supports_piecewise_spans() is False. Unmet requirement: no test reads supports_piecewise_attention (repo-wide only those two production hits). Unchanged-by-diff import in the new test file: test_hidream_o1_attention_mask.py:25-27 from vllm_omni.diffusion.models.hidream_o1_image.pipeline_hidream_o1_image import ( build_hidream_o1_attention_mask, ). test_mask_capability_flags.py:148-149 assert callable(AttentionBackend.supports_piecewise_spans) / assert AttentionBackend.supports_piecewise_spans() is False never instantiates the HiDream property. Reverting the () at :518 leaves a truthy bound method, all(...) is True, the pipeline skips the mask, and those tests still pass.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
Omni ReviewBot: no human activity for 29 days@tthakkal this pull request has had no human commit, comment or review since 2026-09-17. Please consider marking this PR as draft until work can resume. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Signed-off-by: Thanaji Rao Thakkalapelli <thanaji.rao.thakkalapelli@intel.com>
@vllm-omni-review-bot nothing read While auditing the other callers I found the same problem from the latest merge with main:
All three are fixed in this PR. A new test also checks every backend in |
|
Thanks @tthakkal for the thorough analysis here. I re-checked this against current main (2077854). #9123 (merged 2026-10-22) made Measured on an Arc Pro B70 (vLLM 0.32.0, torch 2.14.0+xpu): real
The XPU fix is superseded by #9123, and merging this PR would make HiDream-O1 attention ~6x slower per call on XPU. So I'd suggest closing this PR. Two things in it are still valid on main and would be better as small follow-ups:
|
FIX #6882
Purpose
HiDream-ai/HiDream-O1-Imagegenerates pure noise on XPU — exit 0, no warning.supports_piecewise_spanswas a plain class attribute:Falseon theAttentionBackendbase, but hardcodedTrueon all three flash backends (flash_attn.py:175,flash_attn_hub.py:85/:253) regardless of platform — andFLASH_ATTNis what HiDream-O1 selects on XPU. Onlyforward_cudadispatchespiecewise_attnoverAttentionMetadata.full_attn_spans.hidream_o1_image_transformer.pytrusted the flag, skipped building the equivalentattn_mask, and XPU/NPU then ran fully dense unmasked attention — text rows that must stay causal attended to future image tokens. Full analysis in #6882.A flag describes the
forward_*that actually runs, not the backend class, so the mask-capability flags become platform-gatedclassmethods. CUDA / ROCm / MUSA flag values are identical before and after.abstract.py—supports_piecewise_spans()becomes aclassmethoddefaulting toFalse; newsupports_dense_attention_mask()defaults tosupports_attention_mask(), which only promises some mask is read (several kernels take just a 2D[B, Skv]padding mask).flash_attn.py,flash_attn_hub.py— in all three backends spans areTrueonly for the CUDA family, exactly the set that reachesforward_cuda(ROCm/MUSA via base-classforward_hip/forward_musadelegation). The hub backends are converted along with the plain one because the base declaration is now aclassmethod: leaving theirsupports_piecewise_spans: bool = Truein place would shadow it with a non-callable and break every caller.cudnn_attn.py,vdnh3_attn.py— the same conversion for two flag declarations that landed on main after this PR was opened (CUDNN_ATTN's via [New model] Qwen-Image-2.1 support (rebase + review follow-ups) #8099, the newVDNH3_ATTNbackend via [Model] Add VDN-H3 hybrid attention (VDNH3_ATTN) for MiniMax-H3 #8439). Both are CUDA-only and keep their old values (True/False); left asboolattributes, everysupports_piecewise_spans()call on them raisedTypeError: 'bool' object is not callable.flash_attn.py— only this backend overridessupports_dense_attention_mask(), returningFalseon XPU alone, where its one mask route is_forward_varlen_masked, which asserts 2D. The two hub backends inherit theabstract.pydefault and keep reportingTrue.layer.py—_run_local_attentionreroutes a >2D mask to the existingsdpa_fallbackwith awarning_once, scoped to backends that claim mask support so it cannot preemptTRTLLM_ATTN's deliberateValueError._assert_piecewise_compatiblenow calls the flag.hidream_o1_image_transformer.py— HiDream-O1's probe now calls the flag. This()is what turns the noise into an image.qwen_image_21_transformer.py— the other model-side consumer, from the same [New model] Qwen-Image-2.1 support (rebase + review follow-ups) #8099, gets the same(). Without it the bound method is always truthy, soattn_path_hint="auto"picked"piecewise"for every backend — includingTORCH_SDPA, which then hit the_assert_piecewise_compatibleValueError.supports_piecewise_spans()True(wasTrue)False(wasTrue)False(wasTrue)aclnnFlashAttentionScoreinstead of being skipped, which should be a strict improvement over the dense unmasked run, but no NPU hardware was available. Needs confirmation from someone who has one; see the follow-up onmindiesdmask conventions below before assuming it is fixed.The reroute fires for exactly one backend on one platform:
FLASH_ATTNon XPU. It needssupports_attention_mask()andnot supports_dense_attention_mask(), andflash_attn.pyis the only backend that overrides the latter — every other backend inherits theabstract.pydefault, where the two are the same value, so the pair can never both hold.CUDNN_ATTN,FLASHINFER_ATTN,TORCH_SDPAand the two hub backends cannot reroute.Scope
Per review, this PR is only the XPU correctness fix:
abstract.py+ the backend flags +layer.py+ the two model-side()fixes. Unrelated fixes that were riding along have been reverted out; the one still open is listed under Follow-ups.Reviewer attention
FLASH_ATTNa >2D mask now runs that layer on SDPA after onewarning_once— HunyuanImage3, glm_image, flux, flux2 and ernie_image all unsqueeze to 4D beforeAttention. Previously those masks hitforward_xpu's 2D assert, so this turns a hard failure into a working run. CUDA keeps HunyuanImage3's 4D mask + spans onpiecewise_attn.supports_dense_attention_mask()is deliberatelyTrueon CUDA, though only conditionally honored there:forward_cudahonors a >2D mask only viapiecewise_attn, so a >2D mask without spans still hits the 2D assert — reachable bylingbot_videoandglm_image. ReturningFalsefixes both but drags HunyuanImage3 (4D mask with spans) offpiecewise_attnonto SDPA, so it is a follow-up. Same for the hub backends.build_hidream_o1_attention_maskis newly load-bearing on XPU/NPU. It was dead code there before this PR, so its test file is new coverage for existing unexercised code rather than for anything this PR rewrites; the function itself is untouched.forward_xpu.layer.pydiverts it to SDPA first, so the revertedtorch.any(~attention_mask)on that path is not on the HiDream-O1 route. Were it ever reached with an additive mask it would raise on~over bf16 rather than fail silently.bool-shadows-classmethodregression, which is howCUDNN_ATTN/VDNH3_ATTNslipped through the merge. The registry-wide test below is the guard.Test Plan
136 new cases: 134 CPU (
core_model), 2 hardware-gated.test_mask_capability_flags.py(new, 116)OmniPlatformsubclass.forward_hip/forward_musa→forward_cudadelegation the ROCm/MUSATruerests on, and that the flags are callables rather than truthy attributes._mask_needs_sdpatruth table over 2D/3D/4D masks × mask-aware / dense-capable / mask-blind backends._run_local_attentiondispatch withFLASH_ATTNon a patched platform, recording which impl actually runs: XPU sends 4D (with or without spans) and 3D masks tosdpa_fallbackand keeps 2D / no mask on flash; CUDA keeps HunyuanImage3's 4D mask + spans on flash; NPU keeps 4D on flash.DiffusionAttentionBackendEnum× five platforms answers both flags as a callable returningbool, so a futuresupports_piecewise_spans: bool = ...fails CI. Backends that cannot be imported on the host are skipped (SAGE_ATTN_3on XPU).test_hidream_o1_attention_mask.py(new, 18) — rows causal or full atseq_len129 / 1000 / 4231 across bf16 / fp16 / fp32 against an independenttorch.wherereference, asserting the fully-unmasked row count and not just the values, since a partially-applied mask writes correct values into the rows it reaches and silently leaves the rest causal. AlsoHiDreamO1ImageTransformer.supports_piecewise_attentionover stub backends (all on / all off / mixed), which is the property the pipeline reads to decide whether to build the mask. The 2hardware_testcases build the mask on the accelerator itself and skip rather than false-pass when none is visible; both ran on XPU here.test_attn_path_probe.py(new, 2) — Qwen-Image-2.1's_resolve_attn_path("auto")returns"piecewise"/"mask"according to the backend'ssupports_piecewise_spans().test_attention_config.py— no new cases, only the stubs the flag change invalidates: threeSimpleNamespacebackends whosesupports_piecewise_spansbecomes a callable, a_mask_needs_sdpastub for the one test that drives_run_local_attentiondirectly, and aFlashAttentionBackendsubclass with both flags pinned to the CUDA-family answers so those CPU dispatch assertions do not depend on the host platform.Each new guard was checked against its regression: dropping the
()in either model probe, revertingCUDNN_ATTN/VDNH3_ATTNtoboolattributes, or disabling the reroute inlayer.pyeach fails the corresponding tests. Acrosstests/diffusion/attention/,tests/diffusion/models/hidream_o1_image/andtests/diffusion/models/qwen_image_21/on XPU, the failing set is identical to a cleanmainworktree on the same container (54 pre-existing environment failures, none new).vLLM Version: 0.32.0 (torch 2.14.0+xpu)
Test Result
HiDream-O1-Image, 2048x2048,
--seed 42, on Intel Graphics [0xe223], 30.3 GiB each.TORCHDYNAMO_DISABLE=1is required on this box for an unrelated Triton/Intel defect. Timings exclude a discarded warm-up generation (first-run kernel JIT).* Peak memory from the earlier run on vLLM 0.29.0 / torch 2.13.0; not re-measured on 0.32.0.
"Before" is
mainat the merge base, same script and seed, re-run on 0.32.0 for all three rows: SSIM vs the baseline is 0.053 (TP1/8), 0.029 (TP2/8) and 0.032 (TP2/50), against 0.49 / 0.47 / 0.82 after.The reroute is observable in the log —
layer.py:776emitsAttention backend 'FLASH_ATTN' cannot honor a 4D attn_mask on this platform; running this layer on SDPAonce per run.At 50 steps against the CUDA-generated
tests/assets/hidream_o1/hidream_o1_baseline.png, using the same LANCZOS downscale the accuracy test applies: SSIM 0.8174 (passes the 0.8 gate), PSNR 18.577 (just under the CUDA-tuned 19.0), visually indistinguishable.FLASH_ATTNand an explicitDIFFUSION_ATTENTION_BACKEND=TORCH_SDPAare bit-identical (max abs diff 0 over the full 2048x2048 RGB image), which is what the reroute guarantees, so the PSNR gap is XPU-vs-CUDA numerics rather than a masking difference. These numbers reproduce exactly on both the 0.29.0 and 0.32.0 stacks.CUDNN_ATTN,VDNH3_ATTNand Qwen-Image-2.1: their behavior is restored to whatmainhad, verified on CPU only.Follow-ups (not in this PR)
Split out of this PR at review request — an independent fix to a pre-existing bug, not needed for the XPU noise fix:
torch.any(~attention_mask)mis-routes int keep-masks at four flash call sites (forward_cuda/forward_xpuinflash_attn.py, and both hub backends):~1 == -2, so every int keep-mask is unconditionally treated as padded and takes the slower masked-varlen path. Qwen-Image is the affected in-tree model. The same helper should reject an additive float mask instead of inverting its meaning inside_unpad_input.Pre-existing and untouched by this PR:
lingbot_videoandglm_imagecannot useFLASH_ATTNon CUDA with a >2D mask and no spans. Make the reroute predicate metadata-aware: reroute unless spans are present andsupports_piecewise_spans(). Needs CUDA hardware, since it changes which kernel HunyuanImage3 runs on.[1,1,S,S]mask thatforward_fa_npupasses unchanged intomindiesd.attention_forward, where every other producer supplies a bool keep-mask — so ifmindiesdreads it as one the semantics invert. This is why the NPU row above is marked unverified rather than fixed. That path ran dense and silently wrong before this PR too.SAGE_ATTN,SAGE_ATTN_3andRAINFUSION_ATTNsilently ignore anattn_maskinstead of raising; they should either raise likeTRTLLM_ATTNor opt into the reroute. And the CUDA-tuned PSNR threshold intests/e2e/accuracy/test_hidream_o1_image.pyneeds a per-platform value before that test can gate XPU.BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.