Skip to content
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

Fixed LoRA conversion for kohya_ss #916

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

kovalexal
Copy link
Contributor

diffusers have recently replaced Conv2d and Linear layers with LoraCompatibleConv and LoraCompatibleLinear.

This pull request fixes wrong conversion of kohya_ss trained LoRAs. It should be better to determine whether module is Linear or Conv2d block based on class inheritance, not based on its name.

@BenjaminBossan @pacman100 your review is kindly appreciated.

Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this looks good to me. Let's wait for @pacman100's review too, since he knows more about this.

@kovalexal do you have a (small) file at hand that can be loaded with your PR but fails without? Maybe we could use that for some sort of regression test.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@kovalexal
Copy link
Contributor Author

@BenjaminBossan hi!

I am not quite sure whether this case can be covered by some sort of test (also, from the perspective that it's just an example, not a full part of the library). I have run this script in an environment with one of the recent versions of diffusers and after conversion succeeded, I've discovered that the results seemed to differ from what I was getting in webui.

So, what I've found out during investigation is that now Linear and Conv layers are not processed properly and that 20-30% of LoRAs files were lost.

@BenjaminBossan
Copy link
Member

I am not quite sure whether this case can be covered by some sort of test (also, from the perspective that it's just an example, not a full part of the library). I have run this script in an environment with one of the recent versions of diffusers and after conversion succeeded, I've discovered that the results seemed to differ from what I was getting in webui.

It would probably not be part of PEFT proper, but we could, e.g., run a weekly GH action to load a checkpoint and compare the results to some expected outcome, similar to what you describe. That way, we can catch these types of regressions automatically in the future.

@kovalexal
Copy link
Contributor Author

kovalexal commented Sep 8, 2023

@BenjaminBossan sounds like a great idea, I'll provide all the details soon!

Also, do you need my help in terms of modifying GH actions?

@BenjaminBossan
Copy link
Member

I'll provide all the details soon!

Fantastic.

Also, do you need my help in terms of modifying GH actions?

As you wish. We can also add it in a separate PR later.

@kovalexal
Copy link
Contributor Author

@BenjaminBossan hi!

Here is a sample notebook which shows the loss of weights size when converting with the old script and the results after fix.

It can be used as some sort of reference for writing test.

Would you mind if I'll work on this test in a separate PR later? I have some plans on heavily refactoring this script for proper support of other models and would like to do it in a separate PR.

@BenjaminBossan
Copy link
Member

It can be used as some sort of reference for writing test.

Sounds good.

Would you mind if I'll work on this test in a separate PR later? I have some plans on heavily refactoring this script for proper support of other models and would like to do it in a separate PR.

Yeah, we can add it later.

Let me ping @pacman100 or @younesbelkada for a potential second review, but I think this should be good to go.

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Sounds great thanks! Can you maybe indicate through a comment at the beginning of the file the version of diffusers used? I would assume diffusers>=0.20.0?
If the feature is on main and not published yet we should maybe wait until the next release

@kovalexal
Copy link
Contributor Author

@younesbelkada hi!

As far as I know, diffusers added these layers in version 0.19.0

This script remains backward compatible with older versions of diffusers. So, are you sure that this comment will be meaningful?

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying!

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.

4 participants