Skip to content

PEFT Integration for Text Encoder to handle multiple alphas/ranks, disable/enable adapters and support for multiple adapters #5147

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 63 commits into from
Sep 27, 2023
Merged

PEFT Integration for Text Encoder to handle multiple alphas/ranks, disable/enable adapters and support for multiple adapters #5147

merged 63 commits into from
Sep 27, 2023

Conversation

@pacman100
Copy link
Contributor

@pacman100 pacman100 commented Sep 22, 2023

What does this PR do?

  1. support multiple rank/alpha values
  2. support multiple active adapters
  3. support disabling and enabling adapters

younesbelkada and others added 30 commits September 15, 2023 12:58
@younesbelkada
more fixes
ba24f2a
@younesbelkada
up
c17634c
@younesbelkada
up
2a6e535
@younesbelkada
style
01f6d1d
@younesbelkada
add in setup
5a150b2
@younesbelkada
oops
961e776
@younesbelkada
more changes
cdbe739
@younesbelkada
v1 rzfactor CI
691368b
@younesbelkada @patrickvonplaten
Apply suggestions from code review
7918851 
Co-authored-by: Patrick von Platen <[email protected]>
@younesbelkada
few todos
14db139
@younesbelkada
Merge branch 'main' into peftpart-1
c06c40b
@younesbelkada
protect torch import
d56a14d
@younesbelkada
style
ec87c19
@younesbelkada
fix fuse text encoder
40a6028
@younesbelkada
Merge remote-tracking branch 'upstream/main' into peftpart-1
0c62ef3
@younesbelkada @sayakpaul
Update src/diffusers/loaders.py
c4295c9 
Co-authored-by: Sayak Paul <[email protected]>
@younesbelkada
replace with recurse_replace_peft_layers
4162ddf
@younesbelkada
keep old modules for BC
1d13f40
@younesbelkada
adjustments on adjust_lora_scale_text_encoder
78a860d
@younesbelkada
Merge branch 'main' into peftpart-1
78a01d5
@younesbelkada
Merge remote-tracking branch 'upstream/main' into peftpart-1
ecbc714
@younesbelkada
Merge branch 'peftpart-1' of https://github.com/younesbelkada/diffusers…
9d650c9 
… into peftpart-1
@younesbelkada
nit
6f1adcd
@younesbelkada
move tests
f890906
@younesbelkada
add conversion utils
f8e87f6
@younesbelkada
Merge remote-tracking branch 'upstream/main' into peftpart-1
3ba2d4e
@younesbelkada
remove unneeded methods
dc83fa0
@younesbelkada
use class method instead
b83fcba
@younesbelkada
oops
74e33a9
@younesbelkada
use base_version
9cb8563
sayakpaul
sayakpaul reviewed Sep 25, 2023
View reviewed changes
sayakpaul
sayakpaul reviewed Sep 25, 2023
View reviewed changes
sayakpaul
sayakpaul reviewed Sep 25, 2023
View reviewed changes
sayakpaul
sayakpaul reviewed Sep 25, 2023
View reviewed changes
Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Patrick's and Benjamin's comments. Mine are mostly clarification questions and nits.

sayakpaul
sayakpaul reviewed Sep 27, 2023
View reviewed changes
src/diffusers/loaders.py
lora_scale=self.lora_scale,
low_cpu_mem_usage=low_cpu_mem_usage,
_pipeline=self,
adapter_name=adapter_name,
Copy link
Member

@sayakpaul sayakpaul Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the private variable at the end.

sayakpaul
sayakpaul reviewed Sep 27, 2023
View reviewed changes
sayakpaul
sayakpaul approved these changes Sep 27, 2023
View reviewed changes
Copy link
Member

@sayakpaul sayakpaul left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very clean to me! Thanks so much for iterating.

I see only "text_encoder" attribute being used in the PR. Are we not tackling "text_encoder_2" here?

I didn't see any tests, though. Will they be added in a follow-up?

patrickvonplaten
patrickvonplaten reviewed Sep 27, 2023
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Sep 27, 2023
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Sep 27, 2023
View reviewed changes
@younesbelkada
Copy link
Contributor

younesbelkada commented Sep 27, 2023

For the tests, I am happy to add them here: #5151 to complete the full testing suite

See my comment: #5151 (comment)

patrickvonplaten
patrickvonplaten reviewed Sep 27, 2023
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Sep 27, 2023
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Sep 27, 2023
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Sep 27, 2023
View reviewed changes
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 27, 2023 •
edited
Loading

The documentation is not available anymore as the PR was closed or merged.

@patrickvonplaten
Copy link
Contributor

patrickvonplaten commented Sep 27, 2023

For the tests, I am happy to add them here: #5151 to complete the full testing suite

See my comment: #5151 (comment)

Ok for me to add tests in a later PR

@patrickvonplaten
Copy link
Contributor

patrickvonplaten commented Sep 27, 2023

Very nice job @pacman100 - think the design is very nice & also agree that higher level enable/disable functions are very useful for the users (to quickly switch on/off all LoRAs)

patrickvonplaten
patrickvonplaten approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for me to merge once CI is green (ok if LoRA peft tests are still failing)

@patrickvonplaten patrickvonplaten merged commit 02247d9 into main Sep 27, 2023
@kashif kashif deleted the smangrul/peft-integration branch September 29, 2023 11:38
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
@pacman100 @younesbelkada
@patrickvonplaten @sayakpaul @BenjaminBossan
PEFT Integration for Text Encoder to handle multiple alphas/ranks, di…
7301c0f 
…sable/enable adapters and support for multiple adapters (huggingface#5147)

* 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 <[email protected]>

* few todos

* protect torch import

* style

* fix fuse text encoder

* Update src/diffusers/loaders.py

Co-authored-by: Sayak Paul <[email protected]>

* 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 <[email protected]>
Co-authored-by: Patrick von Platen <[email protected]>

* Apply suggestions from code review

Co-authored-by: Patrick von Platen <[email protected]>

* add comment

* Apply suggestions from code review

Co-authored-by: Sayak Paul <[email protected]>

* conv2d support for recurse remove

* added docstrings

* more docstring

* add deprecate

* revert

* try to fix merge conflicts

* peft integration features for text encoder

1. support multiple rank/alpha values
2. support multiple active adapters
3. support disabling and enabling adapters

* fix bug

* fix code quality

* Apply suggestions from code review

Co-authored-by: Younes Belkada <[email protected]>

* fix bugs

* Apply suggestions from code review

Co-authored-by: Younes Belkada <[email protected]>

* address comments

Co-Authored-By: Benjamin Bossan <[email protected]>
Co-Authored-By: Patrick von Platen <[email protected]>

* fix code quality

* address comments

* address comments

* Apply suggestions from code review

* find and replace

---------

Co-authored-by: younesbelkada <[email protected]>
Co-authored-by: Younes Belkada <[email protected]>
Co-authored-by: Patrick von Platen <[email protected]>
Co-authored-by: Sayak Paul <[email protected]>
Co-authored-by: Benjamin Bossan <[email protected]>
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
@pacman100 @younesbelkada
@patrickvonplaten @sayakpaul @BenjaminBossan
PEFT Integration for Text Encoder to handle multiple alphas/ranks, di…
ce216c4 
…sable/enable adapters and support for multiple adapters (huggingface#5147)

* 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 <[email protected]>

* few todos

* protect torch import

* style

* fix fuse text encoder

* Update src/diffusers/loaders.py

Co-authored-by: Sayak Paul <[email protected]>

* 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 <[email protected]>
Co-authored-by: Patrick von Platen <[email protected]>

* Apply suggestions from code review

Co-authored-by: Patrick von Platen <[email protected]>

* add comment

* Apply suggestions from code review

Co-authored-by: Sayak Paul <[email protected]>

* conv2d support for recurse remove

* added docstrings

* more docstring

* add deprecate

* revert

* try to fix merge conflicts

* peft integration features for text encoder

1. support multiple rank/alpha values
2. support multiple active adapters
3. support disabling and enabling adapters

* fix bug

* fix code quality

* Apply suggestions from code review

Co-authored-by: Younes Belkada <[email protected]>

* fix bugs

* Apply suggestions from code review

Co-authored-by: Younes Belkada <[email protected]>

* address comments

Co-Authored-By: Benjamin Bossan <[email protected]>
Co-Authored-By: Patrick von Platen <[email protected]>

* fix code quality

* address comments

* address comments

* Apply suggestions from code review

* find and replace

---------

Co-authored-by: younesbelkada <[email protected]>
Co-authored-by: Younes Belkada <[email protected]>
Co-authored-by: Patrick von Platen <[email protected]>
Co-authored-by: Sayak Paul <[email protected]>
Co-authored-by: Benjamin Bossan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BenjaminBossan BenjaminBossan BenjaminBossan left review comments

@sayakpaul sayakpaul sayakpaul approved these changes

+2 more reviewers

@patrickvonplaten patrickvonplaten patrickvonplaten approved these changes

@younesbelkada younesbelkada younesbelkada approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pacman100 @patrickvonplaten @younesbelkada @HuggingFaceDocBuilderDev @BenjaminBossan @sayakpaul