downcast MiniMax-H3 position_ids at the device transfer on fp64-less backends - #14767
Open
SammyTourani wants to merge 1 commit into
Open
SammyTourani wants to merge 1 commit into
SammyTourani wants to merge 1 commit into
Conversation
…backends Both layout steps build position_ids in float64 on CPU and then move it to the execution device with a plain .to(device), which raises on MPS. Pass the dtype through maybe_adjust_dtype_for_device so the transfer lands as float32 on mps/npu/neuron and is unchanged everywhere else. The fp64 construction is untouched, and MiniMaxH3RotaryPosEmbed.forward already casts position_ids to float32, so no device sees different rotary angles.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14639
One file, 9 added / 3 removed lines. Both
.to(device)sites now pass the dtype throughmaybe_adjust_dtype_for_devicefromdiffusers.utils.torch_utils:Why this shape rather than the
torch.device(device).type == "mps"check the issue proposes:transformer_flux.py:512,transformer_anyflow_far.py:48,unet_2d_condition.py:857,pipeline_pixart_alpha.py:922,controlnet_sparsectrl.py:608,auraflow_transformer_2d.py:438,pipeline_flux2_klein.py:408..ai/references/models.md, gotcha Add UNet for Latent Diffusion #5("Only if float32 visibly degrades output, use the
maybe_adjust_dtype_for_devicehelper ... Never leave an unconditional
torch.float64in the model")._FP64_UNSUPPORTED_DEVICES = {"mps", "npu", "neuron"}),which a hand-rolled MPS check does not.
The float64 construction is untouched, and no device's numerics change — including MPS.
The issue's precision table (worst case 1.6e-4 rad) is pessimistic: the grid's fp64
precision never reaches the model at all, because
MiniMaxH3RotaryPosEmbed.forward(
transformer_minimax_h3.py:95) castsposition_idsto float32 as its first statement, onevery device.
position_idshas no other consumer — I grepped the wholeminimax_h3package; outside
before_denoise.pyit only appears in docstrings. So the downcast justmoves a cast that already happens one step earlier. Measured on the largest realistic grid
(64×64 latent frame at patch 2, 102 latent frames, 4000 audio latents, max |coord| 5023):
And on cpu/cuda
maybe_adjust_dtype_for_device(torch.float64, device)returnstorch.float64,so those paths are byte-for-byte the code that was there before.
I added no test. Reasoning, which a reviewer may disagree with (see below): the existing
suite already covers this on MPS, because the shared mixins build the pipeline with
.to(torch_device)andtests/testing_utils.py:116resolvestorch_deviceto"mps"automatically on Apple silicon — and
.github/workflows/push_tests_mps.ymlrunspytest tests/on a macOS runner. The before/after numbers below are that suite. A test that only ass
Verification
Environment:
.venvwith torch 2.14.0 (MPS available), transformers 5.17.0, torchvision0.29.0, torchaudio 2.14.0, accelerate, peft.
torchvisionandtorchaudiowere missing atfirst and caused unrelated failures; both are needed for this suite to run at all.
1. Full suite on MPS, without the patch (baseline,
git stashed):Command:
Result:
286 occurrences of
Cannot convert a MPS Tensor to float64in that log. Broken tests includeTestMiniMaxH3ModularPipelineFast::test_inference_is_not_nan,TestMiniMaxH3Ref2VAModularPipelineFast::test_inference_is_not_nan,::test_float16_inferenceon both, bothLoading::test_save_from_pretrained, and most ofthe LoRA class.
2. Same command, same machine, with the patch:
Command:
Result:
The 4 remaining failures are all
Memory::test_*_auto_cpu_offload*and are an unrelated,pre-existing MPS gap —
components_manager.py:749raisesNotImplementedError: enable_auto_cpu_offload() relies on the mem_get_info() method. It's not implemented for mps.becausetorch.mpshas nomem_get_info. They fail the same waywith and without this patch and have nothing to do with
position_ids.3. Full suite on CPU with the patch (no regression on the default path):
Command:
Result:
4. All three workflows end to end on MPS (scratch script, tiny checkpoint, 124 frames,
32×32, 2 steps) — fails at
before_denoise.py:444/:768before, passes after:5. Style / consistency: