Uh oh!
There was an error while loading. Please reload this page.
Implements Blockwise lora - #7352
Conversation
- implemented block lora - updated docs - added tests
…sers into 7231-blockwise-lora
asomoza
commented
Mar 16, 2024
this PR is really nice, I like to be able to have this kind of control over the LoRAs. Just by playing with it I found a way to break it though. If you do this: scales= {
"text_encoder": 0.5,
}
pipeline.set_adapters(["test"], adapter_weights=[scales])it throws: Also I would like to request if you can add to have control over the second text encoder in SDXL but if it's rare edge case, it doesn't matter, I can do it on my side after. |
❤️
fixed + expanded tests
added Let me know if there are any other issues! |
sayakpaul
left a comment
There was a problem hiding this comment.
I think the PR has a very nice start. I left some initial comments. We'll need to make sure to add rigorous checks on the inputs to prevent anything unexpected that we can foresee from our experience.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
UmerHA
commented
Mar 18, 2024
Re In principal, Re convs: The original implementation doesn't implements scaling the conv layers.
Let me know, then I'll adapt the code. |
sayakpaul
commented
Mar 18, 2024
I still don't see why we shouldn't make it configurable as well actually. Even if it's just a single block there might be effects if the users can control its contributions. So, I would vote for the ability to configure it too.
All of them should configurable IMO if we were to support this feature otherwise the design might feel very convoluted. But I would like to also take opinions from @BenjaminBossan and @yiyixuxu here. |
I think we're miscommunicating. With the current PR, You CAN already configure the pipe= ... # create pipelinepipe.load_lora_weights(..., adapter_name="my_adapter") scales= { "mid" : 0.5 }
pipe.set_adapters("my_adapter", scales)However, you can only pass a single number to it, because it has in total only 1 transformer layer in it. Unlike the up / down part, which have multiple blocks and multiple layers per block. That's why you can pass a |
sayakpaul
commented
Mar 18, 2024
Ah makes sense then. |
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for adding this feature. I don't have sufficient knowledge to judge if this covers all edge cases, so I'll leave that to the experts. Still added a few comments.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
yiyixuxu
left a comment
There was a problem hiding this comment.
hey! thanks for your PR
I did first round of review - let me know if it makes sense
Thanks
YiYi
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
sayakpaul
commented
Mar 19, 2024
@UmerHA would be helpful to resolve the comments that you have already addressed. But do feel free to keep them open if you're unsure. Will give this a thorough look tomorrow. Making a reminder. |
HuggingFaceDocBuilderDev
commented
Mar 19, 2024
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
BenjaminBossan
left a comment
There was a problem hiding this comment.
LGTM overall, nice tests. I just have some smaller comments, please check.
All in all, I don't know enough about diffusers to give a final approval, so I'll leave that to the experts.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
UmerHA
commented
Mar 24, 2024
With a small adjustment, yes. This works: list_adapters=self.get_list_adapters() # eg {"unet": ["adapter1", "adapter2"], "text_encoder": ["adapter2"]}all_adapters= {
adapterforadaptersinlist_adapters.values() foradapterinadapters
} # eg ["adapter1", "adapter2"]invert_list_adapters= {
adapter: [partforpart, adaptersinlist_adapters.items() ifadapterinadapters]
foradapterinall_adapters
} # eg {"adapter1": ["unet"], "adapter2": ["unet", "text_encoder"]}I have adjusted to code accordingly, and renamed |
UmerHA
commented
Mar 28, 2024
@sayakpaul gentle ping, as this has been stale for a week |
sayakpaul
commented
Mar 28, 2024
Will do a thorough review tomorrow first thing. Thanks for your patience. |
sayakpaul
left a comment
There was a problem hiding this comment.
Left a last couple of comments. Looking really nice. Going to run some low tests to ensure we're not breaking anything here.
Really amazing work!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
sayakpaul
commented
Mar 29, 2024
LoRA SD slow tests are passing which should be more than enough to confirm the PR isn't backward breaking. Let's address the last comments and ship this beast |
UmerHA
commented
Mar 29, 2024
@sayakpaul Everything is addressed! Iiuc, you now have to accept the PR for the uploaded images (https://huggingface.co/datasets/huggingface/documentation-images/discussions/310) and then we're done |
sayakpaul
commented
Mar 29, 2024
Already merged. |
asomoza
commented
Mar 29, 2024
really nice, when I have the time I'll play a lot with this and maybe post my results so people can understand better what can they do with this level of control over the LoRAs |
sayakpaul
commented
Mar 29, 2024
@UmerHA could you push an empty commit to start the CI? And please ping me once you do. |
UmerHA
commented
Mar 29, 2024
@sayakpaul empty commit pushed |
sayakpaul
commented
Mar 29, 2024
Thanks @UmerHA! Will wait for the CI to run and will then ship. To help promote this amazing feature, I welcome you to open a detailed thread here: https://github.com/huggingface/diffusers/discussions. Additionally, if you're on HF Discord, please let me know your username so that we can give you a shoutout. If you're not, you can join via this link: https://hf.co/join.discord. |
UmerHA
commented
Mar 29, 2024
❤️
Will do this evening/tomorrow ✅
Thanks :) My discord is |
@UmerHA I'll leave this here because I can't do a PR right now for a quick fix. Just updated my main and ran the test I did before, there a small typo Lora weight dict for adapter 'lora' contains uent, but this will be ignored because lora does not contain weights for uent. Valid parts for lora are: ['text_encoder', 'text_encoder_2', 'unet']. |
UmerHA
commented
Mar 29, 2024
UmerHA
commented
Mar 30, 2024
Done ✅ |
* Initial commit * Implemented block lora - implemented block lora - updated docs - added tests * Finishing up * Reverted unrelated changes made by make style * Fixed typo * Fixed bug + Made text_encoder_2 scalable * Integrated some review feedback * Incorporated review feedback * Fix tests * Made every module configurable * Adapter to new lora test structure * Final cleanup * Some more final fixes - Included examples in `using_peft_for_inference.md` - Added hint that only attns are scaled - Removed NoneTypes - Added test to check mismatching lens of adapter names / weights raise error * Update using_peft_for_inference.md * Update using_peft_for_inference.md * Make style, quality, fix-copies * Updated tutorial;Warning if scale/adapter mismatch * floats are forwarded as-is; changed tutorial scale * make style, quality, fix-copies * Fixed typo in tutorial * Moved some warnings into `lora_loader_utils.py` * Moved scale/lora mismatch warnings back * Integrated final review suggestions * Empty commit to trigger CI * Reverted emoty commit to trigger CI --------- Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
Fixed important typo
* Initial commit * Implemented block lora - implemented block lora - updated docs - added tests * Finishing up * Reverted unrelated changes made by make style * Fixed typo * Fixed bug + Made text_encoder_2 scalable * Integrated some review feedback * Incorporated review feedback * Fix tests * Made every module configurable * Adapter to new lora test structure * Final cleanup * Some more final fixes - Included examples in `using_peft_for_inference.md` - Added hint that only attns are scaled - Removed NoneTypes - Added test to check mismatching lens of adapter names / weights raise error * Update using_peft_for_inference.md * Update using_peft_for_inference.md * Make style, quality, fix-copies * Updated tutorial;Warning if scale/adapter mismatch * floats are forwarded as-is; changed tutorial scale * make style, quality, fix-copies * Fixed typo in tutorial * Moved some warnings into `lora_loader_utils.py` * Moved scale/lora mismatch warnings back * Integrated final review suggestions * Empty commit to trigger CI * Reverted emoty commit to trigger CI --------- Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
What does this PR do?
Allows setting LoRA weights more granularly, up to per-transformer block.
Fixes#7231
Example usage:
Before submitting
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Core library:
@dain5832: As you're the author of the solved issue, I invite you to also test this.