Repository navigation
[Model][MiniMax-H3] Support FastH3 model-level CPU offload - #6949
EchoHayate wants to merge 10 commits into
Conversation
Signed-off-by: Allen Wu <allenwu2795@gmail.com> Co-authored-by: TRAE CLI <traecli@bytedance.com>
|
This PR touches tests/diffusion/, recipes/MiniMaxAI/, vllm_omni/diffusion/ (4 files). Based on CODEOWNERS coverage of the changed files, the most-related reviewers appear to be: Could one of you take a look when you get a chance? Thanks! |
…offload Co-authored-by: TRAE CLI <traecli@bytedance.com>
A100 evidence updateI completed the planned controlled Base H3 versus FastH3 run for this PR on one The comparison kept the model revision, hardware, prompt, seed, FPS, output
Peak HBM was 65961/65989 MiB for Base/FastH3 at 672x384 and 68456/68527 All 16 final requests returned HTTP 200. The outputs contain decodable H.264 The source-level scope remains intentionally narrow:
After merging the latest upstream main, the two complete MiniMax-H3 I also reran 672x384 end to end at the current PR head The complete raw text evidence, exact artifact revisions, hashes, commands, |
|
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 be related to model: MinimaxH3. Model owners: @david6666666 @EchoHayate, 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. |
Author self-reviewI reviewed the production diff, tests, recipe documentation, and the exercised runtime path at the current PR head.
Current head reviewed: |
…offload Co-authored-by: TRAE CLI <traecli@bytedance.com> # Conflicts: # tests/diffusion/model_loader/test_diffusers_loader.py
Current-head refresh / author self-review
Current head reviewed: |
|
update docs since #5929 merged |
|
Updated for #5929 in
Fresh focused verification: Current head reviewed: |
Signed-off-by: Allen Wu <allenwu2795@gmail.com> Co-authored-by: TRAE CLI <traecli@bytedance.com>
4977e7a to
bf5b0aa
Compare
|
please add an indepdent A100 recipe pelase |
Signed-off-by: Allen Wu <allenwu2795@gmail.com> Co-authored-by: TRAE CLI <traecli@bytedance.com>
Independent A100 recipe update / author self-reviewAdded the requested standalone single-A100 recipe in
Current head reviewed: |
|
@hsliuustc0106, the requested standalone A100 recipe is included in The PR is ready for review and current CI is green. Could you take another look, or help route the remaining review to the appropriate owner? |
|
@hsliuustc0106, a gentle follow-up on my September 14 message. The requested standalone A100 recipe and the #5929 documentation update are included in Could you confirm any remaining blockers and help route this to an available reviewer? An expected review timeframe would also help. Thanks! |
| @@ -607,17 +607,22 @@ def check_serving_contract( | |||
| raise ValueError("FastH3 preview v1 distills T2VA only, so it cannot serve a Ref2VA partition") | |||
| offloads = [ | |||
There was a problem hiding this comment.
Please check:
check_serving_contract() still inspects only the legacy enable_*_offload booleans. Compact diffusion_offload_config users keep those booleans false, so {"mode":"layer",...}, distributed layer options, and VSA plus {"mode":"module",...} bypass the intended fail-closed matrix (the VSA check at line 622 has the same problem). Please resolve the strategy with resolve_offload(od_config) and reject LAYER_WISE/DISTRIBUTED_LAYER_WISE; for VSA, reject MODEL_LEVEL as well.
There was a problem hiding this comment.
Fixed in d85f0d7. check_serving_contract() now uses resolve_offload(od_config).strategy rather than the legacy booleans. It rejects LAYER_WISE and DISTRIBUTED_LAYER_WISE, and also rejects MODEL_LEVEL for VSA; Dense/Data-Free module offload remains supported.
The regression cases cover compact module allow/reject behavior, compact layer mode, distributed AllGather, and rank-local transfer with positive resident_layers, with legacy booleans left false. Replaying the published-head guard in memory against the nine offload cases reproduced four missing-rejection failures; the repaired focused suite passes 64 tests. All applicable changed-file pre-commit hooks passed, including mypy, CI marks, and SPDX.
These are local macOS CPU checks, not new GPU/E2E or performance evidence. Could you recheck the guard change?
There was a problem hiding this comment.
@Bounty-hunter Could you recheck this thread when convenient? The fix in the reply above is preserved on refreshed head 7b3aacbf6cfdf6e726185cd3c2ca71cc2cfda887.
check_serving_contract() now calls resolve_offload_strategy(od_config) (the current helper returns resolve_offload(config).strategy). Both layerwise strategies remain rejected, as does model-level offload for VSA; Dense/Data-Free module offload remains allowed. The current-head author self-review records 76 focused tests passing, including compact-policy guard coverage.
GitHub marks this thread outdated but still unresolved. If the current change addresses your concern, could you mark it resolved, or point out anything still missing? I have left the resolution to you. The local test result is not a new GPU/E2E qualification.
Signed-off-by: Allen Wu <allenwu2795@gmail.com> Co-authored-by: TRAE CLI <traecli@bytedance.com>
Author self-review — compact-offload guard follow-upCurrent head reviewed:
Focused reproduction, with this checkout and the compatible vLLM source on HF_HUB_OFFLINE=1 python -m pytest \
-p no:cacheprovider \
-o addopts='--strict-markers --strict-config' \
tests/diffusion/models/minimax_h3/test_minimax_h3_fasth3.py \
tests/diffusion/models/minimax_h3/test_minimax_h3_offload.py \
tests/diffusion/model_loader/test_diffusers_loader.py::test_compact_model_cpu_offload_uses_ordinary_loader_without_host_weight_plan \
-q --disable-warnings |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Both asks from 09-06 are addressed and the core fix is the right shape: check_serving_contract now resolves the offload strategy through the component-selective API instead of blanket-rejecting every offload flag — model-level CPU offload is correctly allowed (it installs after ordinary loading, so the fusion completeness check still runs) while layerwise paths stay rejected, and the VSA variant is explicitly fenced to Dense/Data-Free with a clear error. The recipe's new single-A100 section with pinned revisions and the component-selective docs update close out the documentation asks, and the fasth3 contract tests cover both the allowed and rejected paths. Checks green.
Merge upstream 7ab582a, preserve the reviewed offload capability matrix through the canonical strategy accessor, and clarify recipe evidence and non-offloaded VSA instructions. Signed-off-by: Allen Wu <allenwu2795@gmail.com> Co-authored-by: TRAE CLI <traecli@bytedance.com>
Author self-review — upstream conflict refreshCurrent head reviewed: Refreshed against upstream
Focused reproduction (with compatible vLLM source available on HF_HUB_OFFLINE=1 python -m pytest \
-p no:cacheprovider \
-o addopts='--strict-markers --strict-config' \
tests/diffusion/models/minimax_h3/test_minimax_h3_fasth3.py \
tests/diffusion/models/minimax_h3/test_minimax_h3_offload.py \
tests/diffusion/model_loader/test_diffusers_loader.py::test_compact_model_cpu_offload_uses_ordinary_loader_without_host_weight_plan \
-q --disable-warnings |
Omni ReviewBot: supersededThe CI failure noted on |
CI failure triage — September 28@hsliuustc0106, I inspected the Buildkite failures on Confirmed inherited failure Main build #16231 AMD failures still needing attribution AMD build #12868
Those test files are also unchanged by this PR, but I have not reproduced Could you point me to an existing upstream fix for the Qwen gate and an AMD No new code, tolerance change, or CI retry has been made in this triage. |
September 30 CI follow-upThis FastH3 compatibility fix is still planned. I located concrete upstream
These are upstream fixes or reported evidence, not a fresh base reproduction @hsliuustc0106, the concrete next step is an upstream refresh with the existing |
|
@EchoHayate this PR is labeled Could you please take a look and push an update to get CI green? Once the checks pass we can proceed with review/merge. Thanks! |
Record the verified October 1 upstream merge at 423f343 without changing the reviewed FastH3 support matrix. Signed-off-by: Allen Wu <allenwu2795@gmail.com> Co-authored-by: TRAE CLI <traecli@bytedance.com>
Merge upstream 3ab726c, including consolidated AMD CI repairs, while preserving the existing four-file FastH3 offload delta and historical evidence boundaries. Signed-off-by: Allen Wu <allenwu2795@gmail.com> Co-authored-by: TRAE CLI <traecli@bytedance.com>
Author self-review — October 4 CI refresh@hsliuustc0106 I refreshed this branch onto upstream
Current head reviewed: Please evaluate the new exact-head CI rather than retrying the old |
Omni ReviewBot routing recordAssigned Strict on zcode (GLM-5.3-Flash) under experiment |
Signed-off-by: Allen Wu <allenwu2795@gmail.com> Co-authored-by: TRAE CLI <traecli@bytedance.com>
Author self-review — upstream conflict refreshRefreshed against upstream Dense/Data-Free module offload still uses ordinary checkpoint loading and completes fusion/validation before offloader installation. Layerwise and distributed-layerwise offload, VSA plus module offload, and preview-v1 Ref2VA remain fail-closed. The recipe now clarifies the separate AdaLN offload boundary and removes module/layer offload flags from the VSA example. Validation:
This refresh does not renew the historical A100 benchmarks or establish GPU/E2E or production-performance acceptance. GitHub reports the refreshed head as mergeable; new-head basic CI was still pending when checked. Current head reviewed: |
|
@hsliuustc0106 Following up on your October 4 CI request: the refreshed head is All five basic checks on this head are now green: Python 3.11/3.12 wheel builds, pre-commit, DCO and Read the Docs. The scope, local verification and evidence limitations are documented in the author self-review above. I cannot see a The existing approval remains recorded; this is a request to verify the remaining gates, not to bypass them. |
Summary
Model-level offload follows ordinary checkpoint loading: FastH3 deltas are fused and validated before the offloader is installed. The loader contract tests verify that this path does not create a
HostWeightPlan.Validation
bf5b0aaa: 59 focused tests passed; changed-file pre-commit hooks andgit diff --checkpassed.1086cf79change adds only the standalone A100 recipe; its applicable pre-commit hooks and whitespace checks passed.1086cf79018a959631167a17ad754baaf7ab95f7. GitHub build, pre-commit, DCO, and documentation checks pass.Single-A100 E2E evidence
The previously published controlled run used one NVIDIA A100 80GB PCIe, model-level CPU offload, concurrency one, and one warmup followed by three measured requests per arm. Model revision, prompt, seed, output dimensions/length, and attention backend were fixed.
Outputs contained decodable video and non-silent audio; repeated warmed outputs were byte-identical within each arm. At
7dce3afb, a separate 672x384 rerun measured 120.352 s versus 26.391 s, with output hashes matching the earlier run.These are historical GPU results, not a GPU rerun of
1086cf79. The speedup comes from FastH3's four-step inference, not a new offload optimization. This does not establish Base/FastH3 perceptual-quality parity or production throughput.The measured recipe uses legacy
--enable-cpu-offload, including VAE staging. The newerdit/text_encodercomponent selector keeps VAEs resident and does not inherit those memory measurements.Detailed evidence and revision boundaries: #6949 (comment)