Repository navigation
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: @david6666666 @wtomin @Isotr0py Routing: @david6666666 via module of the changed files, module named in the PR description, semantic router, model owner; @wtomin via module of the changed files, module named in the PR description, CODEOWNERS; @Isotr0py via module of the changed files, module named in the PR description @yosh20004, 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 routing recordAssigned Strict on zcode (GLM-5.3-Flash) under experiment |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
|
Self-review: The CUDA Graph output-lifetime failure with MINIMAX_H3_VAE_DECODER_VIT_ROPE_TORCH_COMPILE=1 is fixed. RoPE Q/K outputs were verified against eager within the test tolerances, including retention across subsequent calls, while compilation remained active. Full-model generation also completed successfully. We expect #6607’s fused eager path to be preferable for performance, but have not performed a controlled comparison. The fix introduces intentional tensor copies. I expect their overhead to be small, but this has not been measured. |
hsliuustc0106
added
bug
|
The frozen merge-base ( On the head, I also captured eight real VAE Q/K RoPE pairs at shape |
|
@Dong1017 Thanks for verifying this on SM90! Really appreciate the additional validation |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 39fef635-ab8d-4953-90c4-49e58d140dc2) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 1472c83f-c73b-423b-a06c-8894ff605d2e) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 05ac967e-d640-4f35-a4fa-8f05fd13f90a) — check |
vllm-omni-review-bot
left a comment
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
0 actionable finding(s).
CI at
bc1d560c555c(2026-10-10T09:42:11.846811+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Note: The assigned review arm
strict/zcode/GLM-5.3-Flashcould not complete this review, so it was produced by the fallback armdirect/cursor/auto. It is excluded from the routing experiment.
Full review analysis
PR description
MiniMax-H3 video VAE decoding can crash when the checkpoint's optional native RoPE helper is compiled with torch.compile(mode="reduce-overhead"). Attention runs that helper for Q and then K, and the second CUDA Graph replay overwrites the first call's output before attention reads it. MiniMaxH3VideoVAE now installs a wrapper around the checkpoint's apply_rotary_pos_emb. The wrapper clones the result only when the live helper is the compiled cache, so Q and K keep independent storage; eager, disabled, and fallback calls still return the original tensor. RoPE math and compile settings are unchanged, and the fused eager VAE path is left in place.
Change flow
flowchart LR
attn["[EXISTING] Decoder attention calls RoPE for Q then K"]:::existing
init["[CHANGED] MiniMaxH3VideoVAE.__init__"]:::changed
guard["[NEW] install_compiled_rope_output_guard"]:::new
clone["[NEW] Clone only when compiled helper is selected"]:::new
eager["[EXISTING] Eager, disabled, and fallback keep original tensor"]:::existing
attn --> init
init --> guard
guard --> clone
guard --> eager
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
No actionable findings.
🤖 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 10 days@yosh20004 this pull request has had no human commit, comment or review since 2026-10-07. 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. |
|
This PR is still active and ready for maintainer review. Self-review, B300 validation, and independent SM90 validation are documented above. The automated review reported no actionable findings, and all reported CI checks are passing. Could a maintainer please review when available and confirm whether the missing |
Omni ReviewBot: no human activity for 7 days@yosh20004 this pull request has had no human commit, comment or review since 2026-10-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. |
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
@yosh20004 Thanks for the compiled-RoPE lifetime reproducer and the SM90 follow-up. This is a readiness check, not a completed code review.
- Resolve the current merge conflict. GitHub currently reports this open PR as not mergeable at
bc1d560c555c7f584f3b4892520d83cdac97e155. Please update against the target branch and resolve the conflict without dropping the retained Q/K-output regression. - Confirm the updated head. Supply the resulting SHA and the regression command, environment and result. The acceptance condition is a mergeable PR with the compiled path still exercised and returned Q/K values retained correctly across later calls; keep the existing baseline-fails/head-passes evidence, and identify any part invalidated by conflict resolution.
Your existing full-model and component results are acknowledged, not treated as absent. Current-head project CUDA CI remains a reviewer/maintainer validation step after the update; no request to trigger unrelated platform jobs.
Signed-off-by: yosh20005 <2172622103@qq.com>
bc1d560 to
fcda8e4
Compare
|
@hsliuustc0106 Updated to fcda8e4, rebased onto 7cbc776. GitHub now reports no merge conflicts. The resolution preserves main’s NVFP4 decoder exclusion. The RoPE guard and retained-Q/K assertions are unchanged; test hardware markers were updated to the current convention. Fresh B300 validation (Python 3.12.3, PyTorch 2.13.0+cu130, Triton 3.7.1): 6 tests passed, none skipped, using an isolated component harness. The real checkpoint RoPE helper reproduced the overwrite error before the fix at both tested shapes. After installing the guard, all 8 retained Q/K pairs matched eager within the existing tolerances, with compilation still active and no eager fallback. Applicable pre-commit checks also passed. This rerun covers UT and GPU components, not end-to-end generation. Previous full-model and SM90 evidence remains tied to its original revisions; the conflict resolution does not change the tested native RoPE logic |
Purpose
MiniMax-H3 video VAE decoding can fail with native RoPE
torch.compile(mode="reduce-overhead"):Attention calls the same cached compiled RoPE helper for Q and then K. A later call can invalidate Q's graph-owned output before attention consumes it. The VAE adapter now clones outputs outside the compiled region when the checkpoint actually selects the compiled helper. Eager and fallback outputs retain their original identity; RoPE math and compile options are unchanged.
Relationship to #6607
#6607 is the preferred performance path on supported configurations. Its eager VAE optimizations cover Q/K RMSNorm + RoPE, SwiGLU, scaled residuals, and FP16 Linear weight materialization. It reports substantial decode speedups, including approximately 2× on B300 versus reference eager (8719.9 ms → 4414.5 ms). Those published measurements use a different workload from our validation and are not a direct comparison with native RoPE compile.
The fused Q/K path bypasses the native compiled RoPE helper repaired here. Enabling native FF compile prevents installation of the eager operator set; RoPE compile alone does not, but the native helper remains reachable through the original attention path, including fallback. Our repair validation enabled both native FF and RoPE compile.
This PR is only a correctness fix for that optional native-compile path, not an alternative to #6607's performance optimizations. It preserves compiled outputs at the cost of an extra copy and should not be taken as a recommendation to enable native FF/RoPE compile for performance. No controlled performance A/B between these paths was performed.
Test Plan
The CUDA regression requires a CUDA GPU and a working Inductor toolchain, but no model weights.
To reproduce using the checkpoint's actual helper, set
H3_ROOTto the downloaded Ref2VA directory (tested checkpoint revision42ed227ee7df40d41602854ae760620d6eb651fe):This directly calls the unpatched checkpoint helper and reproduces its failure even with this PR installed. The fix is installed by the Omni VAE adapter; the regression tests exercise that installer.
vLLM Version: 0.31.0; PyTorch 2.13.0+cu130; Transformers 5.14.1; Diffusers 0.40.0.
vLLM-Omni Commit: validated on
25fb257667abb397eab10b71ec7de7b35b42e3a3plus this fix; rebased onto65699fe94c355e235f1acabddd866bce0bba1c02, with no intervening changes to the tested H3 code.Test Result
The unpatched official checkout at
25fb257667abb397eab10b71ec7de7b35b42e3a3reproduced the full-model failure with native FF/RoPE compile enabled (reduce-overhead, both FATAL switches enabled). The traceback below is copied from that run; only ANSI colors and per-line logger prefixes have been removed.Full traceback before the fix
dynamic=False), TRTLLM_ATTN; native FF/RoPE compile inreduce-overheadmode, both FATAL switches enabled.65699fe94c355e235f1acabddd866bce0bba1c02completed three Ref2VA requests with native FF/RoPE compile disabled and without this fix. All outputs passed full decoding, and the trace confirmed [Perf] MiniMax-H3 VAE Decoder Ops #6607's fused kernels executed. This validates the separate eager path, not the native-compile repair.Validation covers helper numerics and successful media decoding on one GPU/software profile, not full-model eager/compile equivalence or perceptual quality.
AI assistance: Codex assisted with the patch, regression tests, GPU validation, and drafting this PR description.