-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Allow to set config params directly in init #1419
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. |
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.
Makes total sense! Thank you!
| """ | ||
|
|
||
| _compatibles = _COMPATIBLE_STABLE_DIFFUSION_SCHEDULERS.copy() | ||
| _deprecated_kwargs = ["predict_epsilon"] |
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.
@pcuenca what do you think about this logic? It has another advantage in that I can now easily write a test for all models and schedulers that verifies that kwarg should only be in init if this list is > 0
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.
I like it! We don't have to introduce workarounds, it gives us a clean path to deprecate or replace arguments and is testable. Very cool.
| init_dict["prediction_type"] = "epsilon" if predict_epsilon else "sample" | ||
| for deprecated_kwarg in cls._deprecated_kwargs: | ||
| if deprecated_kwarg in unused_kwargs: | ||
| init_dict[deprecated_kwarg] = unused_kwargs.pop(deprecated_kwarg) |
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.
Note we don't need to deprecate here this will be handled in the init
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.
Thank you!
| - **_deprecated_kwargs** (`List[str]`) -- Keyword arguments that are deprecated. Note that the init function | ||
| should only have a `kwargs` argument if at least one argument is deprecated (should be overridden by | ||
| subclass). |
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.
💯
| """ | ||
|
|
||
| _compatibles = _COMPATIBLE_STABLE_DIFFUSION_SCHEDULERS.copy() | ||
| _deprecated_kwargs = ["predict_epsilon"] |
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.
I like it! We don't have to introduce workarounds, it gives us a clean path to deprecate or replace arguments and is testable. Very cool.
* fix * fix deprecated kwargs logic * add tests * finish
* fix * fix deprecated kwargs logic * add tests * finish
@pcuenca - I think this is the cleaner way:
Since the two are very independent, I don't see a reason why this could break anything. Also note that we're already doing this in Flax:
diffusers/src/diffusers/configuration_utils.py
Line 598 in d52388f