Uh oh!
There was an error while loading. Please reload this page.
[Core] introduce PeftAdapterMixin module. - #6416
Conversation
DN6
left a comment
There was a problem hiding this comment.
Makes sense to me. Think we could also do this for xformers.
sayakpaul
commented
Jan 2, 2024
|
Uh oh!
There was an error while loading. Please reload this page.
patrickvonplaten
left a comment
There was a problem hiding this comment.
Nice idea to factor out PEFT into a Mixin!
Two questions:
- 1.) It was already the case before, but do we really want to allow PEFT adaptors for all models. E.g. does it make sense that a VAE can also add LoRA layers?
- 2.) Can we maybe move the Mixin to
src/diffusers/loaders?
sayakpaul
commented
Jan 2, 2024
This is what I think too. So, instead of adding the PEFT mixin to
I am okay. I just followed what |
HuggingFaceDocBuilderDev
commented
Jan 2, 2024
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
DN6
commented
Jan 2, 2024
I like this idea. It also keeps ModelMixin from getting bloated. |
| _import_structure["textual_inversion"] = ["TextualInversionLoaderMixin"] | ||
| _import_structure["ip_adapter"] = ["IPAdapterMixin"] | ||
| _import_structure["peft"] = ["PeftAdapterMixin"] |
There was a problem hiding this comment.
This is out of the condition because:
- It's gonna be directly used in the core models (wherever applicable).
- In our core test environment, we don't install
peft. So, if we add the above underis_peft_available(), we will run into import errors.
Can move it under is_torch_available(), though.
@DN6 let me know if you have a better way of handling this.
| from .single_file import FromSingleFileMixin | ||
| from .textual_inversion import TextualInversionLoaderMixin | ||
| from .peft import PeftAdapterMixin |
There was a problem hiding this comment.
Uh oh!
There was an error while loading. Please reload this page.
sayakpaul
commented
Jan 3, 2024
For the failing test: #6426 (comment). |
sayakpaul
commented
Jan 3, 2024
@patrickvonplaten addressed your comments:
This is my open question: #6416 (comment) @DN6 LMK your thoughts too. |
patrickvonplaten
left a comment
There was a problem hiding this comment.
Nice that works for me!
integrations module.PeftAdapterMixin module.sayakpaul
commented
Jan 5, 2024
@stevhliu feel free to edit the docs introduced in this PR in a follow-up. |
younesbelkada
left a comment
There was a problem hiding this comment.
This is very clean thanks @sayakpaul and team!
* introduce integrations module. * remove duplicate methods. * better imports. * move to loaders.py * remove peftadaptermixin from modelmixin. * add: peftadaptermixin selectively. * add: entry to _toctree * Empty-Commit
What does this PR do?
Delegate the
peftrelated methods residing underModelMixinto a separate class calledPeftAdapterMixinfollowing what's done intransformers: https://github.com/huggingface/transformers/blob/main/src/transformers/integrations/peft.py#L43.It also introduces a new module called
integrations, again, followingtransformers. I see it playing to our advantage:modeling_utils.pybecomes cleaner.bitsandbytes), we'd have a better structure.I don't suspect anything introduced in this PR to be backwards-breaking but happy to be proven wrong and fix it.