Fix NaN attention scores on MPS from uninitialized baddbmm buffer - #14459
Fix NaN attention scores on MPS from uninitialized baddbmm buffer#14459RudraMantri123 wants to merge 1 commit into
Conversation
On MPS, torch.baddbmm does not honor the documented beta=0 semantics: NaN/Inf present in the input buffer propagate to the output. Both copies of get_attention_scores (Attention and AttentionModuleMixin) pass torch.empty() as that buffer, so recycled allocator pages containing NaN poison the attention scores, producing all-black images with SlicedAttnProcessor (e.g. SDXL + enable_model_cpu_offload + enable_attention_slicing). Use a buffer-free scaled bmm on MPS when there is no attention mask. This avoids relying on the beta=0 contract, skips the scores-sized buffer allocation entirely (lower peak memory on the memory-constrained devices the sliced path targets), and benchmarks ~35% faster than the baddbmm+empty path on Apple Silicon. Other backends are unchanged. Fixes huggingface#14438
061b461 to
92bd5b5
Compare
|
Upstream status update — correcting my earlier note, and it changes the framing of this PR. I said above that I'd file the
But the fix is not in any released version yet. The import torch # 2.13.0
inp = torch.full((1, 1, 1), float("nan"), device="mps")
torch.baddbmm(inp, torch.ones(1,1,1,device="mps"), torch.ones(1,1,1,device="mps"), beta=0)
# MPS -> nan CPU -> 1.0 (docs: input ignored when beta=0, nan/inf not propagated)So every Apple Silicon user on torch ≤ 2.13.0 — i.e. everyone on the current stable release — still hits #14438 with sliced attention, and will until 2.14.0 ships and they upgrade. What that means for this PR, three honest options:
Tell me which you prefer and I'll adjust — (2) is a small change on this branch. One process note: CI hasn't run here yet, since first-time-contributor workflows need a maintainer's approval click. Whenever someone has a spare second for that, the test suites (including the new MPS regression test) can go green and make this reviewable at a glance. |
|
Gentle ping @yiyixuxu @dg845 @asomoza — this one is blocked only on workflow approval for a first-time contributor, so CI has never actually run here. A single approval click would get the suites green (including the new MPS regression test, which fails deterministically on On scope, in case it helps prioritise: it closes #14438 and also #11229, which has been open since April 2025 with the same root cause. And I'm still happy to switch to the |
dineshyadav03
left a comment
There was a problem hiding this comment.
Verified the diff: the MPS branch replaces baddbmm(torch.empty(...), beta=0, alpha=self.scale) with a buffer-free torch.bmm(query * self.scale, key.mT), applied identically in both call sites. Root cause and repro are convincing — MPS not honoring beta=0's "input ignored" contract is a real PyTorch-side gap.
One question: the PR frames the 35% speedup as bmm vs. the buggy empty+baddbmm path. Do you have numbers comparing bmm against a zeros-init baddbmm rather than against the broken baseline? That's the actually relevant comparison for justifying the larger code-path split.
Also: both regression tests are skipif(torch_device != "mps"), and HF CI doesn't appear to run MPS runners — so this fix has no automated regression coverage. Is there an MPS runner in CI?
Approve, with the zeros-vs-bmm perf question as non-blocking follow-up.
|
Thanks @dineshyadav03 — both questions are fair, and the first one is a good catch. Answering with fresh numbers rather than the ones in the description. 1.
So That is the real argument for the code-path split, and it is stronger than the one I put in the description: the alternative fix is not merely slower than this one, it is slower than the bug. 2. No MPS in CI — correct, and worth stating plainly. These tests provide no automated regression coverage in HF CI. There is no MPS runner, and adding a hosted macOS one would not fix it: I hit this directly while building a separate MPS test harness, where GitHub's Given that, what the tests are for: they fail deterministically on If maintainers would prefer these gated differently (e.g. marked as hardware-dependent, or moved to a slow/nightly suite), happy to adjust. |
|
Correcting two things in my comment above — I checked my own numbers more carefully and one figure was wrong, and the explanation was incomplete in a way that undersells the result. The tensor size was wrong. I wrote 256 MB. The scores tensor at these dimensions is And the cost is not only the zero-fill. I attributed the whole gap to writing a buffer that gets overwritten. Decomposing it properly, with the buffer hoisted out of the timed region so allocation and kernel are separated (same setup as before, median of 3 trials × 40 reps):
Two things fall out of this:
The end-to-end numbers in my previous comment stand (2.07 / 3.16 / 4.56 ms); it is the attribution that was too simple. Net effect on the argument: unchanged in direction, stronger in substance — the buffer-free path wins on two independent counts rather than one. Separately, since both PRs have been open a while: I re-verified both against current |
What does this PR do?
Fixes #14438 — SDXL produces all-black images on MPS when
enable_attention_slicing()is used (most visibly together withenable_model_cpu_offload()).Root cause
get_attention_scores(in bothAttentionandAttentionModuleMixin) passestorch.empty(...)totorch.baddbmmwithbeta=0, relying on the documented guarantee that the input is ignored and NaN/Inf in it are not propagated. The MPS backend violates that guarantee, so NaN garbage in recycled allocator pages leaks into the attention scores. Only the sliced attention processors reach this code path (the default processor uses SDPA), which makesenable_attention_slicing()the trigger; offload merely churns the allocator sotorch.emptyrecycles dirty pages more often — plainpipe.to("mps")+enable_attention_slicing("max")reproduces without any offload.Minimal reproduction of the underlying defect (torch 2.13.0, Apple Silicon, no diffusers):
I will file this against PyTorch separately; this PR makes diffusers robust to it.
The fix
On MPS, when there is no attention mask, compute scores with a buffer-free scaled
bmminstead ofbaddbmm:beta=0semantics and the uninitialized buffer,10×4096×64, M5 Pro, torch 2.13.0; median of 5 trials): 2.07 ms, vs 3.16 ms forbaddbmm+empty(the buggy path, -34.7%) and 4.56 ms forbaddbmm+zeros(the correct alternative, -54.7%). Zero-initialising costs 44% on top of the buggy path: it writes a 320 MiB scores-sized tensor that is then entirely overwritten (~1.6 ms of pure waste), andbmmis additionally the faster kernel for this shape even when the buffer is already allocated (2.03 ms vs 3.19 ms),The issue's reproduction script now renders correctly across seeds (verified on M5 Pro 24GB; end-to-end images inspected).
Tests
test_get_attention_scores_no_nan_from_recycled_buffer— poisons the MPS allocator pool and exercises both implementations; fails deterministically on currentmain, passes with the fix.test_get_attention_scores_matches_cpu_reference— guards numerical equivalence with the CPU path, not just NaN-absence.Both are gated to MPS.
Notes for reviewers
AI disclosure per the contribution policy: AI-assisted debugging and drafting; all experiments were run and verified by me on real hardware.
query * self.scale) instead of post-scaling viaalpha— mathematically identical, bounded by the CPU-parity test (fp32 exact). A zero-initialized buffer also fixes the bug but benchmarked +53% slower and keeps the allocation.torch.empty+baddbmm(beta=0)pattern exists inpipelines/kolors/text_encoder.py; left for a follow-up since I cannot end-to-end test Kolors on this hardware.Who can review?
@yiyixuxu @dg845 @asomoza — cc @pupa3066 for co-verification on M1 8GB (the memory-constrained case I can't cover).