Skip to content

Allow configuring shift= for SD3 dynamically #8501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
isidentical opened this issue Jun 12, 2024 · 11 comments · Fixed by #10269
Closed

Allow configuring shift= for SD3 dynamically #8501

isidentical opened this issue Jun 12, 2024 · 11 comments · Fixed by #10269
Assignees

Comments

@isidentical
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Allow passing shift= per inference call (like timesteps) on the pipeline, for flow matching scheduler, or allow set_shift() etc. on the scheduler. This seems to be the key to getting good results with SD3 https://x.com/bfitzgerald242/status/1801018438120341911

@asomoza
Copy link
Member

asomoza commented Jun 12, 2024

Hi, you can do it like this:

from diffusers import  FlowMatchEulerDiscreteScheduler

pipe.scheduler = FlowMatchEulerDiscreteScheduler.from_config(pipe.scheduler.config, shift=3.0)

@isidentical
Copy link
Contributor Author

yep! but the same format is applicable for timesteps and was wondering if we can get around without re-instating the scheduler again and again?

@asomoza
Copy link
Member

asomoza commented Jun 13, 2024

Not for the moment, but I can see the potential in adding it as an argument if people change it a lot for each inference.

In my experience it didn't fix the anatomy problems and sometimes it made the quality worse but I tested it with the T5, still need to test it without it and do some more generations.

Can you share some examples where changing the shift helped with the generation? That would help a lot.

Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Sep 14, 2024
@a-r-r-o-w a-r-r-o-w removed the stale Issues that haven't received updates label Nov 18, 2024
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Dec 13, 2024
@hlky hlky removed the stale Issues that haven't received updates label Dec 17, 2024
@hlky hlky self-assigned this Dec 17, 2024
@hlky
Copy link
Contributor

hlky commented Dec 17, 2024

Useful to have, changing shift is popular with HunyuanVideo and SD3/Flux.

@bghira
Copy link
Contributor

bghira commented Dec 17, 2024

does it really help though? examples requested never were shared.

@hlky
Copy link
Contributor

hlky commented Dec 17, 2024

Documented benefit for HunyuanVideo.

- For smaller resolution images, try lower values of `shift` (between `2.0` to `5.0`) in the [Scheduler](https://huggingface.co/docs/diffusers/main/en/api/schedulers/flow_match_euler_discrete#diffusers.FlowMatchEulerDiscreteScheduler.shift). For larger resolution images, try higher values (between `7.0` and `12.0`). The default value is `7.0` for HunyuanVideo.

I'll run some tests for SD3/Flux to confirm.

@bghira
Copy link
Contributor

bghira commented Dec 17, 2024

but SD3 already has resolution-dependent shift using the mu calculations, right?

@hlky
Copy link
Contributor

hlky commented Dec 17, 2024

Actually this won't do anything for Flux because of dynamic shifting. We recently added support for dynamic shifting in SD3, it's not used by default though. Either way, it's a simple change that has at least some benefit to HunyuanVideo and it won't do any harm to add a function to change shift without creating the scheduler each time for those that want it.

@bghira
Copy link
Contributor

bghira commented Dec 17, 2024

well not to be argumentative but the schedulers are stateful, yes? doesn't this mean recreating it has to be done anyway?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants