Uh oh!
There was an error while loading. Please reload this page.
[LoRA Attn Processors] Refactor LoRA Attn Processors - #4765
Conversation
The documentation is not available anymore as the PR was closed or merged. |
patrickvonplaten
commented
Aug 24, 2023
Necessary for #4473 |
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
Aug 25, 2023
Did some first pass and left comments. At this point, the changes seem a little too complicated to me but will probably have a better idea once the PR is in a more comprehensive state. |
…iffusers into refactor_lora_attn
patrickvonplaten
commented
Aug 25, 2023
A bit more explanation for what's going on here:
Overall this PR should make the whole LoRA loading and functioning easier to understand and build upon (such as needed for #4473 |
| expected = np.array([0.3636, 0.3708, 0.3694, 0.3679, 0.3829, 0.3677, 0.3692, 0.3688, 0.3292]) | ||
| self.assertTrue(np.allclose(images, expected, atol=1e-4)) | ||
| self.assertTrue(np.allclose(images, expected, atol=1e-3)) |
There was a problem hiding this comment.
1e-3 is a bit too tight for integration tests. We should be able to run most tests on many different GPUs without having them fail.
| images = images[0, -3:, -3:, -1].flatten() | ||
| expected = np.array([0.4115, 0.4047, 0.4124, 0.3931, 0.3746, 0.3802, 0.3735, 0.3748, 0.3609]) | ||
| expected = np.array([0.4015, 0.3761, 0.3616, 0.3745, 0.3462, 0.3337, 0.3564, 0.3649, 0.3468]) |
There was a problem hiding this comment.
Updating the expected values here because I'm quite sure that this PR corrects a bug.
The lora:
lora_model_id = "hf-internal-testing/sdxl-0.9-kamepan-lora"
lora_filename = "kame_sdxl_v2-000020-16rank.safetensors"
is quite unique since it has different network_alphas for q,k,v and out LoRA of the attention layer. Previously we always set the same network_alpha for all q,k,v and out (see here)
However, it might very well be that q,k,v and out have different network_alpha values. This is now possible as of this PR and therefore should mean it's a bug fix since this checkpoint has different network_alphas for each q,k,v and out.
There was a problem hiding this comment.
diffusers/src/diffusers/loaders.py
Line 1407 in 3bba44d
There was a problem hiding this comment.
Ah got it. We do that for the text encoder only it seems but not the UNet, is my understanding correct?
There was a problem hiding this comment.
Yeah exactly! I'm pretty sure it's corrected now :-)
patrickvonplaten
commented
Aug 25, 2023
As soon as you're good with the PR @sayakpaul - I think we can merge this one and unblock the fuse_lora PR :-) |
| # 2. else it is not posssible that only some layers have LoRA activated | ||
| if not all(is_lora_activated.values()): | ||
| raise ValueError( | ||
| f"Make sure that either all layers or no layers have LoRA activated, but have {is_lora_activated}" |
There was a problem hiding this comment.
Maybe we should just display list(is_lora_activated.keys()) to the end user.
| hasattr(self, "processor") | ||
| and not isinstance(processor, LORA_ATTENTION_PROCESSORS) | ||
| and self.to_q.lora_layer is not None | ||
| ): |
There was a problem hiding this comment.
Neat.
I am a little concerned about:
and self.to_q.lora_layer is not None
i.e., only check if to_q.lora_layer is not None. Is there a better alternative?
| f"Make sure that either all layers or no layers have LoRA activated, but have {is_lora_activated}" | ||
| ) | ||
| # 3. And we need to merge the current LoRA layers into the corresponding LoRA attention processor |
There was a problem hiding this comment.
This bit is nasty but needed for ensuring backward compatibility.
| return new_f | ||
| ## Deprecated |
There was a problem hiding this comment.
Do you want to directly throw a deprecation message when the class is initialized or would that be too brutal?
There was a problem hiding this comment.
I thought about this too - think this will be a bit too much though at the moment because it would mean every time we call unet.attn_processors we would get a deprecation warning which would be every time we save LoRAs. I'd suggest to only start throwing aggressive deprecation warnings when we do the peft integration
| return attn.processor(attn, hidden_states, *args, **kwargs) | ||
| class LoRAAttnProcessor2_0(nn.Module): |
| attn.processor = AttnProcessor2_0() | ||
| return attn.processor(attn, hidden_states, *args, **kwargs) | ||
* [LoRA Attn] Refactor LoRA attn * correct for network alphas * fix more * fix more tests * fix more tests * Move below * Finish * better version * correct serialization format * fix * fix more * fix more * fix more * Apply suggestions from code review * Update src/diffusers/pipelines/stable_diffusion/pipeline_onnx_stable_diffusion_img2img.py * deprecation * relax atol for slow test slighly * Finish tests * make style * make style
* [LoRA Attn] Refactor LoRA attn * correct for network alphas * fix more * fix more tests * fix more tests * Move below * Finish * better version * correct serialization format * fix * fix more * fix more * fix more * Apply suggestions from code review * Update src/diffusers/pipelines/stable_diffusion/pipeline_onnx_stable_diffusion_img2img.py * deprecation * relax atol for slow test slighly * Finish tests * make style * make style
What does this PR do?
This PR deprecates all the "LoRA..." attention processors as explained here: #4473 (comment)
This is due to the following reasons:
LoRACompatibleLinearthere is no reason to not also leverage it in the attention classes. This way we have a single point of logic, removing overhead.fuse_loraas seen here:Fuse loras #4473
TODO:
attn_processormethod to (for now) return derprecated attn processor if LoRA layers are activatedThe PR works now for inference:
@sayakpaul@williamberman I'd be very happy if you could give this a first review and if ok for you I can finish the final TODOs tomorrow.