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

[Call for contributions] help us improve LoKr, LoHa, and other LyCORIS #1935

Open
6 tasks
sayakpaul opened this issue Jul 18, 2024 · 36 comments
Open
6 tasks

Comments

@sayakpaul
Copy link
Member

Originally reported by @bghira in #1931.

Our LoKr, LoHA, and other LyCORIS modules are outdated and could benefit from your help quite a bit. The following is a list of things that need modifications and fixing:

  • fixed rank dropout implementation
  • fixed maths (not multiplying against the vector, but only the scalar)
  • full matrix tuning
  • 1x1 convolutions
  • quantised LoHa/LoKr
  • weight-decomposed LoHa/LoKr

So, if you are interested, feel free to take one of these up at a time and open PRs. Of course, we will be with you for the PRs, learning from them and provide guidance as needed.

Please mention this issue when opening PRs and tag @BenjaminBossan and myself.

@NouamaneELGueddarii
Copy link

Hey @sayakpaul, i would like to work one of these issues, preferably on the quantised LoHa/LoKr.

@BenjaminBossan
Copy link
Member

Thanks @NouamaneELGueddarii. Check out the quantization support for LoRA, e.g. for bitsandbytes. Also, feel free to open an early draft PR in case you encounter any roadblocks.

@MnCSSJ4x
Copy link

Hey @sayakpaul and @BenjaminBossan I would like to take a shot at weight-decomposed LoHa/LoKr. However, I am new to the PEFT codebase but have a conceptual understanding of LoRA. Please let me know if I can take this up and guide me in the direction I should go ahead with.

@sayakpaul
Copy link
Member Author

Thank you for expressing your interest. I would recommend going over existing PRs that added some new parameter-efficient fine-tuning method to get a better idea of the places that might need updates.

@KohakuBlueleaf
Copy link

KohakuBlueleaf commented Aug 5, 2024

for everyone who is interesting in support new feature in LyCORIS into PEFT repo:
https://github.com/KohakuBlueleaf/LyCORIS/tree/dev/lycoris/functional

You may want to check this new functional API (which should be easier to transfer into PEFT's codebase)

Currently I'm super busy so it's difficult for mr to directly submit PR about it sry, but I can help to review all the related PR or changes.

@KohakuBlueleaf
Copy link

for everyone who is interesting in support new feature in LyCORIS into PEFT repo: https://github.com/KohakuBlueleaf/LyCORIS/tree/dev/lycoris/functional

You may want to check this new functional API (which should be easier to transfer into PEFT's codebase)

Currently I'm super busy so hard to directly submit PR about it sry, but I can help to review all the related PR or changes.

(The doc string about these API are basically WIP, if you have any question, try to dm me on DC or email me directly)

@sayakpaul
Copy link
Member Author

Cc: @BenjaminBossan

@BenjaminBossan
Copy link
Member

Thanks for the suggestion.

So from my understanding, if we wanted to use that functionality, it would involve a complete rewrite of the corresponding adapters. I think this is a bit of a last resort solution, I'd prefer to update the existing code, with the lycoris code base serving as inspiration (or outright copying some functions were it makes sense). Most fixes are probably not that hard but what's missing, at least for me, is a clear description of what the error is -- AFAICT none of the mentioned ones were ever reported here.

Also pinging @kovalexal since he did most of the work around this.

@AmericanPresidentJimmyCarter

I think the issue is that @KohakuBlueleaf continues to maintain the upstream with new features as they come out -- so any rewrite not relying on upstream means future technical debt and having to continually update PEFT.

@bghira
Copy link

bghira commented Aug 6, 2024

yeah it sounds like a win to rewrite it to function as an API wrapper... any reported issues only have to be reported upstream unless it's a problem with the wrapper logic.

@sayakpaul
Copy link
Member Author

Another option could be to add a "use_upstream" argument in the classes/methods and advertise them? But I will defer to @BenjaminBossan for that.

@BenjaminBossan
Copy link
Member

What I could envision is to have a separate implementation with a different name. With a separate implementation, we can ensure:

  • existing code not breaking
  • not requiring full feature parity right from the start (I imagine the supported feature sets are not completely overlapping and we don't want to put burden on Kohaku to add features just for PEFT)
  • we would not require an extra dependency for those users who don't need it

Of course, there is a downside of having two different implementations that do very similar things, but if we find that the new one works better and the old one cannot be kept up-to-date, we can slowly phase out the old one.

@sayakpaul
Copy link
Member Author

Not a bad idea and seems like win-win to me honestly.

@bghira
Copy link

bghira commented Aug 6, 2024

thanks Ben! your work here is invaluable.

@sayakpaul
Copy link
Member Author

@BenjaminBossan WDYT about a non-exhaustive list of changes you envision would be needed to achieve #1935 (comment). We could restrict that to LoKr, for example. Then we could have a good first PR that could act as a template for contributors interested to work on bringing other techniques through the functional API. If that sounds good, could you create a thread with the list?

@KohakuBlueleaf would be really helpful if we could have the docs here:
https://github.com/KohakuBlueleaf/LyCORIS/tree/dev/lycoris/functional

Some minimal and self-contained examples of how to use the functional API so that we can get a headstart on this.

@BenjaminBossan
Copy link
Member

WDYT about a non-exhaustive list of changes you envision would be needed to achieve #1935 (comment). We could restrict that to LoKr, for example. Then we could have a good first PR that could act as a template for contributors interested to work on bringing other techniques through the functional API. If that sounds good, could you create a thread with the list?

When I have a calm minute, I'll take a look at what's already there in lycoris and draft something up or even do a POC myself. Feature parity with the existing implementations is not necessary, the aim would be more along the lines of a bare minimum of being able to train an adapter and load it again. Furthermore, it would be great if this first example could already address at least some of the original grievances to prove that there is an actual advantage.

@sayakpaul
Copy link
Member Author

102 percent on the same page. Eager to jam with you on this

@BenjaminBossan
Copy link
Member

would be really helpful if we could have the docs here:

Or if you could give us a pointer where the functional API is being used. I couldn't find an example in the repo.

@KohakuBlueleaf
Copy link

KohakuBlueleaf commented Aug 8, 2024

would be really helpful if we could have the docs here:

Or if you could give us a pointer where the functional API is being used. I couldn't find an example in the repo.

I implement the Functional API bcuz some user want to use different way to achieve PEFT (like custom layer or want to transfer to jax)

Basically the library itself doesn't use it (or, precisely, doesn't use the "functional API", the module have been used though)

I can give you some example and refine the doc string/finish the document within few days

Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

@BenjaminBossan
Copy link
Member

not stale

@yaswanth19
Copy link
Contributor

@BenjaminBossan Let me take at stab at this. From what I understand we need to update the PEFT adapters. Now regarding that you have proposed a solution of maintaining two different implementations. Are we still leaning in that direction and if so can you give a high level overview what all necessary steps which I need to take and in the mean time I would look at a recent PR where a new PEFT method is included.

@BenjaminBossan
Copy link
Member

Thanks @yaswanth19 for taking this up. As you correctly noted, this would be akin to adding a completely new PEFT method. However, the existing methods could be used to compare results. Also, one thing to keep in mind is that I would like to keep the dependency optional, so this would need to be taken into account when implementing the new method.

I can give you some example and refine the doc string/finish the document within few days

@KohakuBlueleaf do you have something ready to share?

@yaswanth19
Copy link
Contributor

yaswanth19 commented Sep 27, 2024

So, to be clear the new implementation would be a different name adapter with peft style implementation but updated with latest features like weight decomposition and quantization etc.. Also would be keeping a optional dependency to use upstream LyCORIS. I will try to create draft PR for either LoKr first.

@BenjaminBossan
Copy link
Member

Yes, exactly @yaswanth19, it should leverage LyCORIS to do the heavy lifting of the logic of the respective method.

@yaswanth19
Copy link
Contributor

I can give you some example and refine the doc string/finish the document within few.

@KohakuBlueleaf A gentle ping. If you could update the doc strings and give us some examples then it would be great!! to integrate lycoris into PEFT

@KohakuBlueleaf
Copy link

I can give you some example and refine the doc string/finish the document within few.

@KohakuBlueleaf A gentle ping. If you could update the doc strings and give us some examples then it would be great!! to integrate lycoris into PEFT

Sorry for the late reply
Recently busy on releasing some my other projects

I will try to refine the doc and put more example in this week, optimistically these 2 days.

Sorry for the delaying of this thing

@yaswanth19
Copy link
Contributor

@BenjaminBossan I can see lycoris utils being used for Lora, Loha and oft. Do we still want to use that in new implementation or shift to base tuner/ base layer which are native to PEFT. Ig I can try to remove that and take inspiration from LORA for the rewrite.

@BenjaminBossan
Copy link
Member

I will try to refine the doc and put more example in this week, optimistically these 2 days.

Thanks a lot.

I can see lycoris utils being used for Lora, Loha and oft.

You mean the classes inside of lycoris_utils.py, right? They are not used for LoRA, but for LoKr, LoHa and OFT. Regarding OFT, there is a PR with fixes to OFT that will also remove the that dependency. I would rather not use these classes.

@KohakuBlueleaf
Copy link

I can give you some example and refine the doc string/finish the document within few.

@KohakuBlueleaf A gentle ping. If you could update the doc strings and give us some examples then it would be great!! to integrate lycoris into PEFT

I have added some simple document and comment in functional API source code.
Also added a quick example on how to use functional API.

I think it can be a useful for getting around with the basic logic of LyCORIS

functional API src: https://github.com/KohakuBlueleaf/LyCORIS/tree/dev/lycoris/functional
Quick Example: https://github.com/KohakuBlueleaf/LyCORIS/blob/dev/example/functional_example.py

The functional API here is really "functional"
But since I guess diffusers want to maintain their own module scheme so I guess functional API is still the best start point...

If you want to know more about how LyCORIS patch the module, you can check these files:
lycoris/modules/base.py, lycoris/modules/locon.py

@JINO-ROHIT
Copy link
Contributor

hello everyone! just wondering if this is still active, if yes can we have an updated list of components that have already been updated?

Id like to work on this if possible

@KohakuBlueleaf
Copy link

@JINO-ROHIT I think @yaswanth19 is working on that: #2133

@JINO-ROHIT
Copy link
Contributor

ahh okay no worries

@BenjaminBossan
Copy link
Member

@yaswanth19 created a new PR #2180 which fixes some of the issues mentioned above for the current PEFT LoKr implementation. Feedback is welcome.

Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

@BenjaminBossan
Copy link
Member

not stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants