Support dynamic-shifting schedulers in SD3 ControlNet pipelines - #14675
Open
Devam0311 wants to merge 1 commit into
Open
Support dynamic-shifting schedulers in SD3 ControlNet pipelines#14675Devam0311 wants to merge 1 commit into
Devam0311 wants to merge 1 commit into
Conversation
Both SD3 ControlNet pipelines called `retrieve_timesteps()` without any `mu` handling and exposed no `mu` argument, so any scheduler configured with `use_dynamic_shifting=True` — the SD3.5-style configs — raised "`mu` must be passed when `use_dynamic_shifting` is set to be `True`" before inference. This made ControlNet inconsistent with the rest of the SD3 family. Port the `calculate_shift()` helper, the `mu` argument and the `scheduler_kwargs["mu"]` handling from the base SD3 pipelines. `mu` is derived from the latents, so latent preparation now runs before timestep preparation, matching the base SD3 ordering. Neither step depends on the other, and the existing expected slices are unchanged, confirming the reorder preserves behaviour. Add regression tests for the derived and explicitly passed `mu` paths in both pipelines. Ref huggingface#13611 (Issue 2) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XBYeq5vB4DNZDEaqUsroKR
76 tasks
Contributor
|
Hi @Devam0311, thanks for the PR! It does not appear to link an issue it fixes. If this PR addresses an existing issue, please add a closing keyword (e.g. Please note that PRs without a linked issue are likely to be automatically closed 10 days after this notice. Once the PR links an issue (or gets the |
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 does this PR do?
Fixes Issue 2 of #13611.
Both SD3 ControlNet pipelines call
retrieve_timesteps()with nomuhandling and expose nomuargument, so any scheduler withuse_dynamic_shifting=True— the SD3.5-style configs — fails before inference:The base
StableDiffusion3PipelineandStableDiffusion3InpaintPipelinealready compute and passmu, so ControlNet was the odd one out in the SD3 family.Solution
Ports the
calculate_shift()helper, themuargument, and thescheduler_kwargs["mu"]handling from the base SD3 pipelines into both ControlNet pipelines.One ordering change worth calling out.
muis derived from the latents' shape, but these pipelines prepared timesteps (step 4) before latents (step 5) — the reverse of the base SD3 pipelines. So latent preparation now runs first, and the steps are renumbered to match.That reorder is safe here:
prepare_latentsonly draws from the generator and never touches the scheduler, andretrieve_timestepsnever touches the latents. The strongest evidence is that every existing expected slice intests/pipelines/controlnet_sd3/still passes unchanged — I ran the suite after the reorder and before adding any new tests.The alternative was deriving
image_seq_lenfromheight/widthto avoid moving anything, but that re-derives whatprepare_latentsalready computed and would get the user-supplied-latentscase wrong.Testing
Two tests per pipeline (four total):
test_dynamic_shifting_scheduler—use_dynamic_shifting=Truenow runs, exercising the derived-mupathtest_dynamic_shifting_scheduler_accepts_explicit_mu— covers theelif mu is not NonebranchVerified all four catch the bug: with the fix reverted, all four fail with the
ValueErrorabove.Before submitting
self-reviewskill on the diff?Self-review notes
Reviewed against
.ai/references/review-rules.mdandpipelines.md. No blocking issues.For the reviewer:
image_seq_lenfromheight/widthinstead if you'd rather nothing moved.latent_height/latent_widthrather than shadowingheight/widthaspipeline_stable_diffusion_3.pydoes, since those names are still live later in these pipelines.max_shiftdefault of1.16for consistency with them, even thoughcalculate_shift's own signature defaults to1.15. That discrepancy already exists inpipeline_stable_diffusion_3.py; I did not want to silently change it here.Who can review?
@yiyixuxu @asomoza @hlky