-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Add Scheduler.from_pretrained and better scheduler changing #1286
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
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
|
Also ping @keturn for some feedback since the PR originated from our discussions |
|
All slow tests are passing - ran them locally (except ONNX because they take forever 😅 - but we'll seen when merged on main :-) ) |
pcuenca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small typos.
pcuenca
left a comment
There was a problem hiding this 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.
patil-suraj
left a comment
There was a problem hiding this 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_compatiblesinConfigMixinorSchedulerMixin. AsConfigMixinis also used by models, and this compatibility notion does not exist for models. - Not sure if
_COMPATIBLE_STABLE_DIFFUSION_SCHEDULERSis 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!
| # first we need to login with our access token | ||
| login() |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
| 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 |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
| [`SchedulerMixin`] provides general loading and saving functionality via the [`SchedulerMixin.save_pretrained`] and | ||
| [`~SchedulerMixin.from_pretrained`] functions. |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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!
There was a problem hiding this 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!
| # first we need to login with our access token | ||
| login() |
There was a problem hiding this comment.
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
Co-authored-by: Pedro Cuenca <[email protected]> Co-authored-by: Anton Lozhkov <[email protected]> Co-authored-by: Suraj Patil <[email protected]>
|
Applied all changes - thanks a bunch for the review! Merging! |
…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 <[email protected]> Co-authored-by: Anton Lozhkov <[email protected]> Co-authored-by: Suraj Patil <[email protected]> * up * up Co-authored-by: Pedro Cuenca <[email protected]> Co-authored-by: Anton Lozhkov <[email protected]> Co-authored-by: Suraj Patil <[email protected]>
🚨🚨 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 offrom_configto change schedulers.from_configAPI. It should become as easy as:from_pretrained(...)API to the schedulers.Schedulers should now be loaded with:
Doing:
is deprecated.