Skip to content
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

Add Scheduler.from_pretrained and better scheduler changing #1286

Merged
merged 21 commits into from
Nov 15, 2022

Conversation

patrickvonplaten
Copy link
Contributor

@patrickvonplaten patrickvonplaten commented Nov 14, 2022

🚨🚨 Scheduler.from_pretrained & Better scheduler switching 🚨🚨

This PR was inspired by @keturn's feedback here: #1268

It deprecates SchedulerClass.from_config(<path>) and instead improves the usability of from_config to change schedulers.

    1. Allow to change schedulers with the from_config API. It should become as easy as:
from diffusers import DiffusionPipeline, DDIMScheduler

pipeline = DiffusionPipeline.from_pretrained("runwayml/stable-diffusion-v1-5")

pipeline.scheduler = DDIMScheduler.from_config(pipeline.scheduler.config)
    1. Add a from_pretrained(...) API to the schedulers.

Schedulers should now be loaded with:

from diffusers import DDIMScheduler

ddim = DDIMScheduler.from_pretrained("google/ddpm-cifar10-32")

Doing:

from diffusers import DDIMScheduler

ddim = DDIMScheduler.from_config("google/ddpm-cifar10-32")

is deprecated.

@patrickvonplaten patrickvonplaten changed the title Add scheduler save pretrained Add scheduler from pretrained Nov 14, 2022
@HuggingFaceDocBuilder
Copy link

HuggingFaceDocBuilder commented Nov 14, 2022

The documentation is not available anymore as the PR was closed or merged.

@patrickvonplaten patrickvonplaten changed the title Add scheduler from pretrained Add Scheduler.from_pretrained and better scheduler changing Nov 15, 2022
@patrickvonplaten
Copy link
Contributor Author

Also ping @keturn for some feedback since the PR originated from our discussions

@patrickvonplaten
Copy link
Contributor Author

All slow tests are passing - ran them locally (except ONNX because they take forever 😅 - but we'll seen when merged on main :-) )

@patrickvonplaten patrickvonplaten requested review from pcuenca, patil-suraj and anton-l and removed request for pcuenca November 15, 2022 13:02
Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small typos.

docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very awesome! Changes look good, but there are many files touched so I'll try to test a checkout for real later.

docs/source/api/configuration.mdx Outdated Show resolved Hide resolved
docs/source/api/pipelines/stable_diffusion.mdx Outdated Show resolved Hide resolved
docs/source/quicktour.mdx Outdated Show resolved Hide resolved
docs/source/using-diffusers/loading.mdx Outdated Show resolved Hide resolved
docs/source/using-diffusers/loading.mdx Outdated Show resolved Hide resolved
src/diffusers/schedulers/scheduling_utils.py Outdated Show resolved Hide resolved
src/diffusers/schedulers/scheduling_utils.py Show resolved Hide resolved
src/diffusers/schedulers/scheduling_utils_flax.py Outdated Show resolved Hide resolved
tests/test_pipelines.py Outdated Show resolved Hide resolved
Copy link
Contributor

@patil-suraj patil-suraj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing PR, thanks a lot for working on this :)

The PR looks good to me, also tried it out and works super well. My main comment are

  • Whether we should add _get_compatibles in ConfigMixin or SchedulerMixin. As ConfigMixin is also used by models, and this compatibility notion does not exist for models.
  • Not sure if _COMPATIBLE_STABLE_DIFFUSION_SCHEDULERS is a good name, since as said in the comment below schedulers can be compatible with each other irrespective of the model.

Apart from this everything looks good!

docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
Comment on lines +39 to +40
# first we need to login with our access token
login()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function available in previous hub versions ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

login() is till in the hf_hub RC, let's wait until a full release and then add it with huggingface_hub>=0.11.0 in the dependencies.
Until then we still need to use the CLI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
src/diffusers/configuration_utils.py Show resolved Hide resolved
src/diffusers/configuration_utils.py Outdated Show resolved Hide resolved
@@ -24,7 +24,7 @@

from ..configuration_utils import ConfigMixin, register_to_config
from ..utils import BaseOutput
from .scheduling_utils import SchedulerMixin
from .scheduling_utils import _COMPATIBLE_STABLE_DIFFUSION_SCHEDULERS, SchedulerMixin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if _COMPATIBLE_STABLE_DIFFUSION_SCHEDULERS is the right name. Think schedulers can be compatible with each other irrespective of the model. For example, DDIM and DDPM will always be compatible and could be interchanged for sampling. Maybe we could call this _COMPATIBLE_SCHEDULERS ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think there will be other "groups" of schedulers soon so just _COMPATIBLE_SCHEDULERS is a bit too generic. Will leave for now, we can always adapt down the road

src/diffusers/configuration_utils.py Outdated Show resolved Hide resolved
Comment on lines +99 to +100
[`SchedulerMixin`] provides general loading and saving functionality via the [`SchedulerMixin.save_pretrained`] and
[`~SchedulerMixin.from_pretrained`] functions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need _COMPATIBLE_STABLE_DIFFUSION_SCHEDULERS here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually yeah let's add it right away for Flax as well - why not!

src/diffusers/schedulers/scheduling_utils.py Outdated Show resolved Hide resolved
src/diffusers/schedulers/scheduling_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@anton-l anton-l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall a very clear upgrade, and the tests are solid, thank you @patrickvonplaten!

Comment on lines +39 to +40
# first we need to login with our access token
login()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

login() is till in the hf_hub RC, let's wait until a full release and then add it with huggingface_hub>=0.11.0 in the dependencies.
Until then we still need to use the CLI

docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
docs/source/using-diffusers/schedulers.mdx Outdated Show resolved Hide resolved
tests/test_pipelines.py Show resolved Hide resolved
patrickvonplaten and others added 3 commits November 15, 2022 16:45
Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
Co-authored-by: Anton Lozhkov <anton@huggingface.co>
Co-authored-by: Suraj Patil <surajp815@gmail.com>
@patrickvonplaten
Copy link
Contributor Author

Applied all changes - thanks a bunch for the review! Merging!

@patrickvonplaten patrickvonplaten merged commit a052019 into main Nov 15, 2022
@patrickvonplaten patrickvonplaten deleted the add_scheduler_save_pretrained branch November 15, 2022 17:17
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
…ace#1286)

* add conversion script for vae

* uP

* uP

* more changes

* push

* up

* finish again

* up

* up

* up

* up

* finish

* up

* uP

* up

* Apply suggestions from code review

Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
Co-authored-by: Anton Lozhkov <anton@huggingface.co>
Co-authored-by: Suraj Patil <surajp815@gmail.com>

* up

* up

Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
Co-authored-by: Anton Lozhkov <anton@huggingface.co>
Co-authored-by: Suraj Patil <surajp815@gmail.com>
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 this pull request may close these issues.

[RFC] Proposal to change API of ConfigMixin and SchedulerMixin.
5 participants