[bugfix]: apply FP8 to MiniMax H3's feed-forward - #1780
Conversation
mergify
Bot
added
type: bugfix
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI
🔴 PR merge requirementsWaiting for
This rule is failing.
|
KyleNeverGivesUp
changed the title
SolitaryThinker
left a comment
SolitaryThinker
left a comment
There was a problem hiding this comment.
The FP8 matching and coverage-reporting code looks sound in focused review: five of the six exact remote tests passed on this CPU host, a separate zero-coverage probe emitted the expected warning, and fastcheck/pre-commit are green. I am requesting changes for the sixth test, which is described as CPU-only but fails before its FP8 assertion on a CPU-only machine. Please also amend the commit to remove the Co-Authored-By: Claude Opus 5 trailer; FastVideo maintainer policy does not allow AI agents as co-authors.
| from fastvideo.models.dits.minimax_h3 import MiniMaxH3TokenRefinerBlock | ||
| from fastvideo.platforms import AttentionBackendEnum | ||
|
|
||
| block = MiniMaxH3TokenRefinerBlock( |
There was a problem hiding this comment.
Medium: This test is not CPU-only as written. At exact head 6fd11aa, the suite gives 5 passed, 1 failed on a CPU host: constructing MiniMaxH3TokenRefinerBlock reaches get_attn_backend(...) and raises ValueError: Invalid attention backend for CPU before either FP8 assertion runs. The exact minimax_h3.py and selector blobs match the versions used in the reproduction. Please mock the backend/platform selection or isolate the block-to-FF wiring so this test honors the file's CPU-only contract.
| ".ff.fc_in", | ||
| ".ff.fc_out", | ||
| ) | ||
|
|
There was a problem hiding this comment.
High: These new H3 suffixes expose the existing post-load FP8 converter to sharded FSDP inference. maybe_load_fsdp_model calls fully_shard before _maybe_quantize_model; conversion then quantizes only weight.to_local(), registers that local shard as an ordinary _fp8_weight buffer, and removes the FSDP-managed parameter. FP8QuantizeMethod.apply reads the local buffer directly, so no FSDP all-gather occurs. I reproduced this on two CPU ranks: logical (8,4) / (4,8) DTensor weights became local (4,4) / (2,8) buffers, and both ranks failed the FF forward with mat1 and mat2 shapes cannot be multiplied (2x4 and 8x2). use_fsdp_inference=True is a documented multi-GPU path, so please reject FP8 when the FSDP shard dimension is greater than one until supported, or keep the quantized representation under FSDP unshard/all-gather semantics. Add a multi-rank regression test before reporting these layers as successfully quantized.
…f weights FP8Config.get_quant_method picks layers by substring-matching their state-dict name against _FP8_SUFFIXES. The list carried "ffn.fc_in" and "ffn.fc_out", which fits WanVideo and LTX-2, but MiniMax H3 mounts its feed-forward on .ff, so "ffn.fc_in" in "blocks.0.ff.fc_in" is False and every H3 feed-forward kept its bf16 weights. Nothing surfaced it. H3's attention projections are named to_q/to_k/to_v/ to_out, which the list already matched, so quantization ran, saved memory and produced video while covering 8.01B of the checkpoint's 22.09B parameters. Partial coverage was indistinguishable from full coverage. Add the two H3 names. The leading dot keeps them from matching an unrelated "stuff.fc_in", and WanVideo and GLM-Image are unaffected even though their state dicts also carry ".ff.fc_in": both build that module without a quant_config or a prefix, so get_quant_method never runs for it. Also report what was left behind. The conversion now logs the quantized and skipped parameter totals along with the distinct trailing names that matched no entry, so the next model with different naming shows up as "left 14.08B parameters ... ff.fc_in, ff.fc_out" rather than as silence. Measured on one GB10 with the r16 MiniMax H3 checkpoint: quantized share 8.01B to 20.04B, resident weights 44.23 to 32.99 GiB. Generation time is unchanged, 1587 s against 1643 s at 512x896, consistent with the existing guidance that linear quantization is not a speed lever on long-sequence video models.
6fd11aa to
a0f7448
Compare
…U-only Both items from the review on hao-ai-lab#1780. The wiring test declared itself CPU-only but built a real MiniMaxH3TokenRefinerBlock, whose attention resolves a backend through the platform. A CPU-only host answers with an empty qualname and the selector raises before either FP8 assertion runs, so on such a host the file reported 5 passed 1 failed instead of exercising the contract. A fixture resolves that one lookup to SDPA. Nothing in the test runs attention. _maybe_quantize_model runs after shard_model, so convert_model_to_fp8 sees sharded DTensor weights. It reads weight.to_local, registers that shard as a plain _fp8_weight buffer and pops the FSDP-managed parameter, which leaves nothing for FSDP to all-gather and a forward that multiplies mismatched shapes. The loader now refuses when hsdp_shard_dim is greater than one and the model carries layers the converter would touch. The check keys on FP8QuantizeMethod rather than on FP8Config, so a model that requested FP8 and matched no suffix converts nothing and is not rejected, and AbsMaxFP8, FP8QAT and NVFP4QAT are left alone. It sits inside the use_fsdp branch and ahead of the device mesh, which is where the sharding decision is made and the only point a CPU host can reach, since the mesh is hard-coded to CUDA for every non-NPU platform. A two-rank gloo test records the conversion behaviour that makes the refusal necessary and will fail if FP8 ever becomes FSDP-aware, which is the signal to drop the guard. Two further cases assert the helper does not over-fire.
Resolves the one conflict, in fsdp_load.py. hao-ai-lab#1771 added _summarize_param_names at the same position where this branch adds _has_fp8_convertible_layers. Both are new and unrelated, so both are kept. The to_cpu=cpu_offload change from hao-ai-lab#1793 comes in from main unchanged.
…ER_PORT Fixes three problems in the tests added by the previous commit. The block-wiring fixture swapped in a hand-written platform stub that defined only is_mps and get_attn_backend_cls. Constructing the block reaches set_weight_attrs, which calls current_platform.is_tpu on every linear, so the test raised AttributeError inside the attention to_q before the feed-forward was built and before either FP8 assertion ran. It now pins the real CpuPlatform, which answers the whole current Platform interface rather than the two methods that happened to be needed. The two-rank test wrote MASTER_ADDR and MASTER_PORT into the environment. test_gpu_tests_preserve_a_launcher_assigned_rendezvous_port scans every file under fastvideo/tests and fails on a written MASTER_PORT, because that clobbers the port the CI runner leased to the lane. The rendezvous now goes through init_method, and the os import is gone with it. The loader guard test passed device=cpu, which does not change the global platform. maybe_load_fsdp_model turns FSDP off on MPS, so on a Mac the guard sat inside a branch that never ran and the test failed later for an unrelated reason. It pins CpuPlatform too, so every host takes the same path.
2 participants
Problem
FP8Config.get_quant_methodchooses what to quantize by substring-matching a layer's state-dict name against_FP8_SUFFIXES. That tuple carriedffn.fc_inandffn.fc_out, which fits WanVideo and LTX-2. MiniMax H3 mounts its feed-forward on.ff, so the layer istransformer_blocks.0.ff.fc_inand"ffn.fc_in" in "transformer_blocks.0.ff.fc_in"is False.All 104 H3 feed-forward linears were skipped, 50 main blocks and 2 refiner blocks at two linears each, and kept their bf16 weights.
Nothing surfaced it. H3's attention projections are named
to_q,to_k,to_vandto_out, which the tuple already matched, so quantization ran without error, freed real memory, and produced correct video while covering 8.01B of the checkpoint's 22.09B parameters. Partial coverage was indistinguishable from full coverage.Every model added so far has had its feed-forward registered, including Kandinsky5's in #1601. H3 is the only one where it is missing, so this is an omission against a consistent pattern rather than a deliberate exclusion.
What this changes
The two H3 names are matched by true suffix, not substring. They live in a separate
_FP8_EXACT_SUFFIXEStuple checked withendswith.DiTConfig.prefixis settable from the CLI, so under a substring rule a root prefix containing.ff.fc_inwould tag every descendant, includingadaln_proj.linear, whose forward readsself.linear.weight.dtypeafter the converter has removed that weight. The original tuple keeps substring matching because it depends on it:to_qis also how Kandinsky5'sto_queryis reached.WanVideo and GLM-Image are unaffected even though their state dicts also carry
.ff.fc_in. Both build that module without passingquant_configor aprefix, soget_quant_methodis never consulted for it.param_names_mappingonly renames checkpoint keys during weight loading and takes no part in quantization selection.Partial coverage is now reported.
convert_model_to_fp8counts what it quantized, what was handed anFP8Configbut matched no entry, and what was never a candidate at all, then logs all three with the distinct trailing names of the unmatched layers. On H3 that reads:The names are exactly what a new
_FP8_SUFFIXESentry would have to spell, so the next model with different naming is one log line away from a fix rather than a code read.Zero coverage warns.
_maybe_quantize_modelonly calls the converter once it finds a layer carrying anFP8QuantizeMethod. If FP8 is requested and not one layer matches, the converter never runs and the log above never appears, so the worst case was the quietest. It now warns and names the layers it saw.Measured
One GB10, 121 GiB unified memory, and KyleNeverGivesUp/FastH3-4-step-Preview-v1-r16, which is FastH3 Preview v1 with the rank-reduced AdaLN converter applied, at 22.09B parameters. Every number below can be reproduced against that checkpoint.
This is not a speed lever, and the patch does not claim to be one. Generation is 1587 s against 1643 s at 512x896 with 124 frames, which is noise. That agrees with the existing guidance in
docs/getting_started/installation/spark_performance.mdthat quantizing linear layers buys almost nothing on an attention-bound video model. What changes is what fits: on this machine the 11.24 GiB is the difference between a workload that is killed and one that completes.Output quality
Same seed, resolution and frame count, with
--fp8as the only variable.SSIM is 0.669 and PSNR is 19.3 dB against the bf16 run, but neither measures degradation here. FP8 perturbs the model's prediction at each step, so the trajectory diverges and the result is a different video rather than a degraded copy of the same one.
What does transfer: mean luma is 106.4 against 106.2 and standard deviation is 61.2 against 61.1 across three sampled frames, so there is no exposure or contrast drift. Visual inspection of those frames shows no blocking, no banding, no texture collapse and no structural error. The chroma channels track closely, PSNR u 32.1 and v 34.8, while luma carries the divergence, which is where the difference in sampled content lives.
Callers who want tighter accuracy already have
granularity="channel", documented indocs/inference/optimizations.md.Tests
fastvideo/tests/ops/quantization/test_fp8_ffn_suffix_wiring.py, six CPU-only cases in a directoryunit_test.shalready collects.FP8QuantizeMethod,transformer_blocks.N.ffandtoken_refiner.refiner_blocks.N.ffMiniMaxH3TokenRefinerBlockis constructed, so removing thequant_configor.ffpropagation from the block fails even with the tuple intactffn.fc_inandffn.fc_outstill matchUnquantizedLinearMethodtenant.ff.fc_indoes not dragadaln_proj.linearinto FP8LogRecordarguments rather than the rendered string, which formats both totals to0.00Bat test scaleReverting the two new entries turns three of the six red and leaves the three that should stay green.
On the GB10: 34 passed across
fastvideo/tests/ops/quantization/, 5 passed infastvideo/tests/loader/test_fsdp_load_releases_checkpoint.py, mypy clean, and an end-to-end H3 generation confirming the new log and unchanged component residency.Not in this patch
The underlying shape is a hardcoded allowlist where partial coverage is indistinguishable from full coverage. A deeper fix would have models declare their own quantizable layers, but that touches WanVideo, LTX-2 and Kandinsky5 and belongs in its own change. The reporting added here makes the current mechanism's misses visible in the meantime.
This module uses
logging.getLoggerrather thaninit_logger, matching all five quantization modules beside it. Switching it would make this file the odd one out, so that also belongs in its own change.