Fix NaN in KDPM2DiscreteScheduler.sigmas_interpol (all-NaN output on MPS) - #14374
Open
4ktLuffy wants to merge 1 commit into
Open
Fix NaN in KDPM2DiscreteScheduler.sigmas_interpol (all-NaN output on MPS)#143744ktLuffy wants to merge 1 commit into
4ktLuffy wants to merge 1 commit into
Conversation
sigmas_interpol was computed as exp(lerp(log(sigmas), log(sigmas.roll(1)), 0.5)), the geometric mean of each sigma and its predecessor taken through log space. The sigma schedule always ends in a zero sigma, so log(0) = -inf entered the lerp. torch resolves that -inf differently on CPU and MPS (pytorch#111374), so: - on CPU the NaN landed on sigmas_interpol[0], a wrap-around entry that step() never reads — the value was wrong but harmless, and present in every config tested (24/24); - on MPS it landed on the trailing live entries instead, propagating through dt = sigma_interpol - sigma_hat into prev_sample and turning the whole latent NaN. A tiny StableDiffusionPipeline run with this scheduler returns 49152/49152 NaN pixels — a fully black image. Computing the geometric mean directly as sqrt(a * b) avoids log(0) entirely and is exactly equivalent for positive sigmas. Measured against a float64 ground truth it is also slightly more accurate than the log/exp round trip: max relative error 5.0e-08 vs 1.4e-07, better on 8 of 25 entries and worse on none. The largest sigma across configs is 2.0e+04, so the squared intermediate has ample float32 headroom. After the change MPS output matches CPU exactly (max delta 0.0 across 15 configurations) and sigmas_interpol is finite on both devices in all 48 configurations tested. The ancestral variant has the same log/lerp pattern but is left alone here: huggingface#14221 is already rewriting that line as part of the fix for huggingface#14213.
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.
What this fixes
Fixes #14368 (the
KDPM2DiscreteSchedulerhalf).sigmas_interpolis the geometric mean of each sigma and its predecessor, computed through log space:The sigma schedule always ends in a zero sigma, so
log(0) = -infenters thatlerp. torch resolves the-infdifferently on CPU and MPS (pytorch/pytorch#111374, open since 2023), which is why the same line behaves differently per device:sigmas_interpol[0], a wrap-around entry fromroll(1)thatstep()never reads. Harmless in effect, butsigmas_interpolis a public attribute and it was non-finite in 24 of 24 configurations I tested.dt = sigma_interpol - sigma_hatis NaN on the final step and the whole latent is destroyed.End to end, on a tiny
StableDiffusionPipelinewith this scheduler:The change
Compute the geometric mean directly, which never touches
log(0):exp(½·log a + ½·log b) == sqrt(a·b)exactly for positive sigmas, so this is the same quantity by a different route.On accuracy — the new form is not just equivalent but slightly better. Against a float64 ground truth on a real 25-step schedule:
exp(lerp(log, log))sqrt(a*b)More accurate on 8 of 25 entries, equal on 17, worse on none. Existing CPU/CUDA results therefore shift by a small amount (largest observed output delta 2.4e-04 after 25 steps) — toward the exact value, not away from it. Happy to keep the old expression under a device check instead if you would rather have bit-identical CUDA output; this seemed cleaner.
On overflow — the largest sigma across all configs I tried is
2.03e+04, whose square is4.1e+08, against a float32 max of3.4e+38.Verification
sigmas_interpolfinite on both devices across 48 configurations (2 schedulers × 8 configs × 3 step counts): karras / exponential / beta sigmas, all threetimestep_spacingmodes,squaredcos_cap_v2andscaled_linear.0.0across the 15 configurations that run on both.tests/schedulers/suite: 977 passed, 6 failed — identical list before and after this change. All 6 are pre-existing local MPS failures unrelated to this line (test_full_loop_with_v_predictionhere is the separate CPU-scalarstorage_offsetbug; it passes on a torch 2.14 nightly, with and without this patch).Test
test_set_timesteps_sigmas_interpol_no_nanassertssigmas_interpolis finite at 4/10/25 steps. It fails onmainand passes with this change — and because CPU also carried the NaN at index 0, it fails on CPU too, so CI will catch a regression without needing Apple hardware.Scope
The ancestral variant has the same log/lerp pattern, but I have deliberately left it alone: #14221 is already rewriting that line (as
(sigmas.clamp(min=0) * sigmas_down) ** 0.5) while fixing #14213. This PR touches only the non-ancestral scheduler to avoid conflicting with it.