Skip to content

Reject SDE algorithm types on DPMSolverMultistepInverseScheduler - #14356

Open
MohammadSadeghSalehi wants to merge 2 commits into
huggingface:mainfrom
MohammadSadeghSalehi:scheduler-correctness
Open

Reject SDE algorithm types on DPMSolverMultistepInverseScheduler#14356
MohammadSadeghSalehi wants to merge 2 commits into
huggingface:mainfrom
MohammadSadeghSalehi:scheduler-correctness

Conversation

@MohammadSadeghSalehi

Copy link
Copy Markdown
Contributor

Summary

Under inversion the noise schedule runs from clean to noisy, so sigmas increase with step index. Since lambda = log(alpha) - log(sigma), an increasing sigma makes lambda decrease, and h = lambda_t - lambda_s is therefore negative by construction. On the reported configuration (sde-dpmsolver++, Karras, 25 steps) the first step measures lambda_s=3.534699, lambda_t=3.075704, h=-0.458995, and 1 - exp(-2h) = -1.504252.

Both stochastic update branches then take the square root of a negative number:

algorithm_type variance term for h < 0
sde-dpmsolver++ sqrt(1 - exp(-2h)) negative, because exp(-2h) > 1
sde-dpmsolver sqrt(exp(2h) - 1) negative, because exp(2h) < 1

The deterministic branches (dpmsolver, dpmsolver++) contain no square root and step without NaN. The same first-step NaN appears with use_karras_sigmas=False, so the defect is not specific to Karras.

No published method defines stochastic DPM-Solver inversion. Inventing a sign-corrected variance term (for example sqrt(exp(-2h) - 1)) would define an unvalidated sampler and turn a loud all-NaN failure into a quiet wrong latent. Inversion is deterministic by purpose: recovering the noise that produces a given sample. This PR therefore rejects the unsupported types at construction with a concise error, and corrects the class docstring so it only advertises the deterministic algorithms.

Why the SDE code paths stay

Four of the SDE arms sit inside methods marked # Copied from DPMSolverMultistepScheduler, where those algorithm types remain valid. Deleting them would mean removing the copy headers and unlinking those methods from the forward scheduler for no behavioural change; make fix-copies exits clean with the arms left in place. The two uncopied SDE arms in step still consume variance_noise; removing them would leave a parameter that is accepted and ignored, which is the defect already reported in #14353.

Backwards compatibility

A saved config with algorithm_type="sde-dpmsolver++" (or sde-dpmsolver) will now raise ValueError at load or construction instead of producing all-NaN samples at the first step. That config already could not invert correctly, so the change replaces a silent wrong answer with an immediate error. Happy to convert the raise into a deprecation warning with a transition window if maintainers prefer that.

Test plan

  • test_rejects_stochastic_algorithm_types fails at pristine base 4b8e466bf (ValueError not raised) and passes on this branch
  • Construction with sde-dpmsolver and sde-dpmsolver++ raises; message names the scheduler, the type, and that inversion is deterministic
  • Construction and first step with dpmsolver and dpmsolver++ remain finite (no NaN)
  • Full tests/schedulers/test_scheduler_dpm_multi_inverse.py: 39 passed
  • make style and make fix-copies exit 0

Platform: macOS arm64, CPU, CPython 3.12.

Fixes #10748


Self-review

Scope

Commit range on scheduler-correctness against base 4b8e466bf.

Files:

  • src/diffusers/schedulers/scheduling_dpmsolver_multistep_inverse.py
  • tests/schedulers/test_scheduler_dpm_multi_inverse.py

Blocking issues

None.

Non-blocking issues

  1. Deprecation key still names sde-dpmsolver
    • deprecate("algorithm_types dpmsolver and sde-dpmsolver", ...) keeps the historical key so existing warning filters stay stable. The user-facing message only deprecates dpmsolver.
    • Intentional: renaming the key would re-fire warnings for no benefit.

Dead code (advisory): left on purpose

Method # Copied from SDE branches
convert_model_output yes (forward multistep) kept for copy sync
dpm_solver_first_order_update yes kept for copy sync
multistep_dpm_solver_second_order_update yes kept for copy sync
multistep_dpm_solver_third_order_update yes kept for copy sync
step no still uses variance_noise

Documentation impact

  • Class Args for algorithm_type and euler_at_final updated so the docstring only describes supported behaviour.
  • No separate docs/source page for this scheduler was changed.

Copied-from blast radius

Block Action Why
__init__ Edited Not a # Copied from block
update methods with SDE arms Untouched Synchronised with the forward scheduler
No inverse consumers of # Copied from this class N/A Grep shows no copies of the inverse scheduler

Verdict

READY. Fix before submitting: nothing. Leave for review: whether the construction-time raise should be a deprecation window instead.

Under inversion sigmas increase with step, so lambda decreases and
h = lambda_t - lambda_s is negative by construction. Measured on the
reported sde-dpmsolver++ config: h=-0.458995 and 1-exp(-2h)=-1.504252.
Both SDE variance terms then take the square root of a negative number
and the first step is all-NaN. The deterministic branches contain no
square root and are unaffected. The same NaN appears without Karras
sigmas.

Raise ValueError at construction for sde-dpmsolver and sde-dpmsolver++
instead of failing after a model is loaded. Leave the SDE arms in the
copied update methods: they are synchronised with the forward scheduler
where those types remain valid. Leave the uncopied step SDE arms so
variance_noise stays used.

Fixes huggingface#10748
The SDE algorithm types cannot be constructed on the inverse scheduler,
so the class docstring must not describe stabilising an SDE variant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NaN in DPMSolverMultistepInverseScheduler

1 participant