[PEFT / LoRA] PEFT integration - text encoder#5058
[PEFT / LoRA] PEFT integration - text encoder#5058younesbelkada merged 76 commits intohuggingface:mainfrom
PEFT / LoRA] PEFT integration - text encoder#5058Conversation
|
@younesbelkada let me know early feedback / PR jamming is needed. |
src/diffusers/loaders.py
Outdated
|
|
||
| is_peft_lora_compatible = version.parse(importlib.metadata.version("transformers")) > version.parse("4.33.1") | ||
|
|
||
| if not is_peft_lora_compatible: |
There was a problem hiding this comment.
@sayakpaul @patrickvonplaten what is the approach we want to follow here? For now I am forcing users to use latest transformers.
There was a problem hiding this comment.
I think Patrick has already proposed a nice plan i.e., we follow a deprecation cycle and then completely remove the older methods.
src/diffusers/loaders.py
Outdated
|
|
||
| # Remove any existing hooks. | ||
| if is_accelerate_available() and is_accelerate_version(">=", "0.17.0.dev0"): | ||
| if is_accelerate_available() and version.parse(importlib.metadata.version("accelerate")) >= version.parse("0.17.0"): |
There was a problem hiding this comment.
For some reason this test was always failing on my end with the previous logic, I replaced it with that one which is a one-liner (maybe there was a silent bug in the is_accelerate_version method
tests/models/test_lora_layers.py
Outdated
| # Outputs shouldn't match. | ||
| self.assertFalse(torch.allclose(torch.from_numpy(orig_image_slice), torch.from_numpy(lora_image_slice))) | ||
|
|
||
| @unittest.skip("this is an old test") |
There was a problem hiding this comment.
I need to re-write the tests here as they depend on private methods I have removed
src/diffusers/loaders.py
Outdated
| rank.update({rank_key_fc2: text_encoder_lora_state_dict[rank_key_fc2].shape[1]}) | ||
|
|
||
| # for diffusers format you always get the same rank everywhere | ||
| # is it possible to load with PEFT |
There was a problem hiding this comment.
Is this supposed to be a question? :D
Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
tests/models/test_lora_layers.py
Outdated
| # TODO: @younesbelkada add save / load tests with text encoder | ||
| # TODO: @younesbelkada add public method to attach adapters in text encoder |
There was a problem hiding this comment.
Don't forget this
src/diffusers/loaders.py
Outdated
| attention_modules = text_encoder_attn_modules(text_encoder) | ||
| text_encoder_lora_state_dict = convert_old_state_dict_to_peft( | ||
| attention_modules, text_encoder_lora_state_dict | ||
| ) |
src/diffusers/loaders.py
Outdated
| # New diffusers format to PEFT | ||
| elif any("lora_linear_layer" in k for k in text_encoder_lora_state_dict.keys()): | ||
| attention_modules = text_encoder_attn_modules(text_encoder) | ||
| text_encoder_lora_state_dict = convert_diffusers_state_dict_to_peft( |
There was a problem hiding this comment.
Yeah this part could be bit confusing for the maintainers to understand as to why we're using two different peft methods. Maybe if we name the previous one a bit more explicitly, it will help clear the confusion.
There was a problem hiding this comment.
Actually @younesbelkada @sayakpaul , I'd change the logic here slightly for now:
-
- We convert all the different formats first to
diffusersformat (this way this method can be leveraged by the deprecated method). And then we convert all the differnt formats to peft.
So we should IMO have to methods:
- We convert all the different formats first to
# 1. convert to diffusers format
text_encoder_lora_state_dict = convert_state_dict_to_diffusers(attention_modules, text_encoder_lora_state_dict)
# 2. convert to peft format
text_encoder_lora_state_dict = convert_diffusers_to_peft(attention_modules, text_encoder_lora_state_dict)Then when saving we can add the reverse method:
# 1. convert PEFT to diffusers:
text_encoder_lora_state_dict = convert_peft_to_diffusers(attention_modules, text_encoder_lora_state_dict)This has some advantages:
- 1.) Easier to read
- 2.) Easier to maintain / test
- 3.) Easier to add new conversions
There was a problem hiding this comment.
Makes totally sense !
There was a problem hiding this comment.
Proposed something in f8e87f6 , we could easily extend that to other formats if needed. Let me know what do you think
src/diffusers/loaders.py
Outdated
| lora_rank = list(rank.values())[0] | ||
| alpha = lora_scale * lora_rank |
There was a problem hiding this comment.
Maybe a comment to add a justification on why this makes sense?
BenjaminBossan
left a comment
There was a problem hiding this comment.
Just a couple of comments from my side. I don't know enough about diffusers to comment on the bigger picture, so it's mostly local stuff.
Also, I would like to ensure that not too many deprecation messages are shown if a user just uses their normal code with the latest diffusers release. Personally, I would be quite annoyed to get 5 deprecation messages that are basically for the same thing. Maybe that's not happening, in that case please disregard this comment :)
setup.py
Outdated
| "torchvision", | ||
| "transformers>=4.25.1", | ||
| "urllib3<=2.0.0", | ||
| "peft>=0.5.0" |
There was a problem hiding this comment.
Not sure what the exact plan for releasing the updates is, but does it make sense to require peft>=0.5.0 when further below, USE_PEFT_BACKEND requires >0.5?
There was a problem hiding this comment.
I added it here so that the CI will automatically fall back to PEFT tests once we make a new release on PEFT. I can also remove that and make a PR on diffusers once we make a release.
In both cases after the PEFT release we'll have to update that line to use PEFT >= 0.6.0
| if patch_mlp: | ||
| target_modules += ["fc1", "fc2"] | ||
|
|
||
| lora_config = LoraConfig(r=lora_rank, target_modules=target_modules, lora_alpha=alpha) |
There was a problem hiding this comment.
I guess this will be replaced once huggingface/peft#873 is merged? If so, I'd leave a # TODO comment about it.
| import torch | ||
|
|
||
|
|
||
| def recurse_remove_peft_layers(model): |
There was a problem hiding this comment.
I wonder if we can somehow use unload from PEFT? The logic seems pretty similar. Maybe it would work with some small tweaks?
There was a problem hiding this comment.
unload is only available on LoRAModel: https://github.com/huggingface/peft/blob/main/src/peft/tuners/lora/model.py#L673 which currently makes it not usable at layer-level I think
There was a problem hiding this comment.
Yeah, it would require some tweaks on PEFT side. Maybe we can add those later to avoid code duplication, it should be fine as is for this PR.
There was a problem hiding this comment.
yes, we could think of exposing a public method that will be leveraged by unload method in LoraModel and use that later in diffusers
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
patrickvonplaten
left a comment
There was a problem hiding this comment.
Played around with different inference scenarios and everything works as expected. PR is good to merge for me. As a final thing to make sure we test the new PEFT implementation it would be nice if we could add:
python -m pip install git+https://github.com/huggingface/peft.git
python -m pip install git+https://github.com/huggingface/transformers.git
here:
diffusers/.github/workflows/pr_tests.yml
Line 75 in d558811
and here:
https://github.com/huggingface/diffusers/blob/d558811b2654ad89c93b3eea10120f3878fec2be/.github/workflows/push_tests.yml#L66C9-L66C80
|
Hello @younesbelkada , could you update this PR from main? |
Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
* more fixes * up * up * style * add in setup * oops * more changes * v1 rzfactor CI * Apply suggestions from code review Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * few todos * protect torch import * style * fix fuse text encoder * Update src/diffusers/loaders.py Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> * replace with `recurse_replace_peft_layers` * keep old modules for BC * adjustments on `adjust_lora_scale_text_encoder` * nit * move tests * add conversion utils * remove unneeded methods * use class method instead * oops * use `base_version` * fix examples * fix CI * fix weird error with python 3.8 * fix * better fix * style * Apply suggestions from code review Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * Apply suggestions from code review Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * add comment * Apply suggestions from code review Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> * conv2d support for recurse remove * added docstrings * more docstring * add deprecate * revert * try to fix merge conflicts * v1 tests * add new decorator * add saving utilities test * adapt tests a bit * add save / from_pretrained tests * add saving tests * add scale tests * fix deps tests * fix lora CI * fix tests * add comment * fix * style * add slow tests * slow tests pass * style * Update src/diffusers/utils/import_utils.py Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com> * circumvents pattern finding issue * left a todo * Apply suggestions from code review Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * update hub path * add lora workflow * fix --------- Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
* more fixes * up * up * style * add in setup * oops * more changes * v1 rzfactor CI * Apply suggestions from code review Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * few todos * protect torch import * style * fix fuse text encoder * Update src/diffusers/loaders.py Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> * replace with `recurse_replace_peft_layers` * keep old modules for BC * adjustments on `adjust_lora_scale_text_encoder` * nit * move tests * add conversion utils * remove unneeded methods * use class method instead * oops * use `base_version` * fix examples * fix CI * fix weird error with python 3.8 * fix * better fix * style * Apply suggestions from code review Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * Apply suggestions from code review Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * add comment * Apply suggestions from code review Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> * conv2d support for recurse remove * added docstrings * more docstring * add deprecate * revert * try to fix merge conflicts * v1 tests * add new decorator * add saving utilities test * adapt tests a bit * add save / from_pretrained tests * add saving tests * add scale tests * fix deps tests * fix lora CI * fix tests * add comment * fix * style * add slow tests * slow tests pass * style * Update src/diffusers/utils/import_utils.py Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com> * circumvents pattern finding issue * left a todo * Apply suggestions from code review Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * update hub path * add lora workflow * fix --------- Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
What does this PR do?
Tackles the PEFT integration step by step #4780 , the first step being the ability to leverage PEFT for the text encoder
cc @patrickvonplaten @sayakpaul @pacman100
Still draft for now
to check correctness, I run
and this should return on main: