-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
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 :-) ) |
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.
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.
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_compatibles
inConfigMixin
orSchedulerMixin
. AsConfigMixin
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!
# 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
@@ -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 |
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
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 <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>
🚨🚨 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_config
to change schedulers.from_config
API. It should become as easy as:from_pretrained(...)
API to the schedulers.Schedulers should now be loaded with:
Doing:
is deprecated.