Skip to content

[Lora] Seperate logic - #5809

Merged
patrickvonplaten merged 5 commits into
mainfrom
seperate_lora_better
Nov 21, 2023
Merged

[Lora] Seperate logic#5809
patrickvonplaten merged 5 commits into
mainfrom
seperate_lora_better

Conversation

@patrickvonplaten

@patrickvonplaten patrickvonplaten commented Nov 15, 2023

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR separates old and new (PEFT) lora logic better to make it easier to remove the old LoRA logic.

return mlp_modules


def text_encoder_lora_state_dict(text_encoder):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is only used in training scripts, let's remove it from here. It'll make it easier to later completely remove once the training scripts are refactored.

LORA_DEPRECATION_MESSAGE = "You are using an old version of LoRA backend. This will be deprecated in the next releases in favor of PEFT make sure to install the latest PEFT and transformers packages in the future."


def text_encoder_attn_modules(text_encoder):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need those for PEFT for now, so to be able to easily delete src/diffusers/models/lora.py let's move it here.

@@ -41,7 +41,7 @@
import diffusers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Missing example refactor from this PR: #5331

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Nov 15, 2023

Copy link
Copy Markdown

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

@sayakpaul sayakpaul left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All in for sweeping off 馃Ч

Thanks!

state_dict = {}

def text_encoder_attn_modules(text_encoder):
from transformers import CLIPTextModel, CLIPTextModelWithProjection

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this needed? We can directly import it at the top.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So that we can remove text_encoder_attn_modules from src/diffusers

Comment thread examples/dreambooth/train_dreambooth_lora.py
set_weights_and_activate_adapters(text_encoder, adapter_names, text_encoder_weights)

def disable_lora_for_text_encoder(self, text_encoder: Optional["PreTrainedModel"] = None):
def disable_lora_for_text_encoder(self, text_encoder: Optional["PreTrainedModel"] = None): # noqa: F821

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why the noqa?

@patrickvonplaten patrickvonplaten Nov 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Some linters complain about PreTrainedModel not being present

@yiyixuxu yiyixuxu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks!

@BenjaminBossan BenjaminBossan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, looks like a good step in the direction of separating the logic. Some minor comments, but no blockers from my side.

Comment thread src/diffusers/loaders/__init__.py Outdated
"text_encoder_load_state_dict in `models`",
"0.27.0",
"`text_encoder_lora_state_dict` has been moved to `diffusers.models.lora`. Please make sure to import it via `from diffusers.models.lora import text_encoder_lora_state_dict`.",
"`text_encoder_lora_state_dict` is deprecated and will be removed in 0.27.0. Make sure to retrieve the weights with PEFT.",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe add the info how the weights can be retrieved with PEFT (or a reference).

Comment thread examples/dreambooth/train_dreambooth_lora.py
# Parse the attention module.
attn_module = unet
for n in attn_processor_name.split(".")[:-1]:
attn_module = getattr(attn_module, n)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

operator.attrgetter can be used to replace this pattern, but looping explicitly is also fine.

@patrickvonplaten
patrickvonplaten merged commit 13d73d9 into main Nov 21, 2023
affromero pushed a commit to affromero/diffusers that referenced this pull request Nov 24, 2023
* [Lora] Seperate logic

* [Lora] Seperate logic

* [Lora] Seperate logic

* add comments to explain the code better

* add comments to explain the code better
@kashif
kashif deleted the seperate_lora_better branch December 5, 2023 08:59
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* [Lora] Seperate logic

* [Lora] Seperate logic

* [Lora] Seperate logic

* add comments to explain the code better

* add comments to explain the code better
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
* [Lora] Seperate logic

* [Lora] Seperate logic

* [Lora] Seperate logic

* add comments to explain the code better

* add comments to explain the code better
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants