Skip to content

[LoRA] improve LoRA fusion tests #11274

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

sayakpaul
Copy link
Member

@sayakpaul sayakpaul commented Apr 10, 2025

What does this PR do?

This PR improves our LoRA fusion tests by:

  • Additionally checking the pipe.num_fused_loras argument.
  • Correctly determines num_fused_loras within lora_base.py.
  • Adding a test test_lora_scale_kwargs_match_fusion that checks LoRA + scale outputs match with LoRA fusion with the same scale.

@BenjaminBossan if I could pick your brain, the following tests are failing on a GPU but passing on a CPU:

FAILED tests/lora/test_lora_layers_sdxl.py::StableDiffusionXLLoRATests::test_simple_inference_with_text_denoiser_lora_unfused - AssertionError: False is not true : Fused lora should not change the output
FAILED tests/lora/test_lora_layers_sdxl.py::StableDiffusionXLLoRATests::test_simple_inference_with_text_lora_denoiser_fused_multi - AssertionError: False is not true : Fused lora should not change the output
FAILED tests/lora/test_lora_layers_sdxl.py::StableDiffusionXLLoRATests::test_lora_scale_kwargs_match_fusion_0 - AssertionError: False is not true : Fused lora should not change the output
FAILED tests/lora/test_lora_layers_sdxl.py::StableDiffusionXLLoRATests::test_lora_scale_kwargs_match_fusion_1 - AssertionError: False is not true : Fused lora should not change the output

Would you have any pointers? I have tested this with both peft main and latest.

@HuggingFaceDocBuilderDev

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
Copy link
Member

if I could pick your brain, the following tests are failing on a GPU but passing on a CPU:

I ran those 4 tests locally on a 4090 and they all passed. Same with the CPU. How did you run the tests?

My first instinct would be that it's a precision problem. Assuming the issue is that some tensors are not torch.allclose, could you jump in and check the values? Are they hugely different or just barely?

@sayakpaul
Copy link
Member Author

@BenjaminBossan could you share your diffusers-cli env?

Also, feel free to review the rest of PR for the changes in lora_base.py and tests

@BenjaminBossan
Copy link
Member

could you share your diffusers-cli env?

- 🤗 Diffusers version: 0.33.0.dev0 [221726b82df3a71be5870e842aa3677a9a1f80f6]
- Platform: Linux-6.11.0-21-generic-x86_64-with-glibc2.39
- Running on Google Colab?: No
- Python version: 3.12.9
- PyTorch version (GPU?): 2.6.0+cu124 (True)
- Flax version (CPU?/GPU?/TPU?): not installed (NA)
- Jax version: not installed
- JaxLib version: not installed
- Huggingface_hub version: 0.30.1
- Transformers version: 4.52.0.dev0 [0ddad2d6553b42fe21d64c18ad6617360cbe8af8]
- Accelerate version: 1.6.0
- PEFT version: 0.15.2.dev0 [5e9ee26e79fe1d6f5fe74c6aa1d0054278f6ed15]
- Bitsandbytes version: 0.45.3
- Safetensors version: 0.5.3
- xFormers version: not installed
- Accelerator: NVIDIA GeForce RTX 4090, 24564 MiB
NVIDIA GeForce RTX 4090, 24564 MiB
- Using GPU in script?: <fill in>
- Using distributed or parallel set-up in script?: <fill in>

Also, feel free to review the rest of PR for the changes in lora_base.py and tests

I'll take a look.

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.

The PR generally LGTM. I have one comment, but it's probably an edge case.

Regarding the failing tests, I don't see why they would start failing now (the ones that were already there), since the changes shouldn't really affect them. Are you sure they pass without the changes, all else being equal? As mentioned, I think tweaking the tolerances might be a solution (albeit not a nice one).

@sayakpaul
Copy link
Member Author

Are you sure they pass without the changes, all else being equal? As mentioned, I think tweaking the tolerances might be a solution (albeit not a nice one).

@BenjaminBossan I should have been clear. The first two tests fail without the PR changes (the latter two being added in this PR).

@BenjaminBossan
Copy link
Member

I should have been clear. The first two tests fail without the PR changes (the latter two being added in this PR).

I see. Did you check by how much the values differ?

@sayakpaul
Copy link
Member Author

@BenjaminBossan I decided to just relax the tolerance for GPU. I requested for a clarification here: #11274 (comment)

@BenjaminBossan
Copy link
Member

I decided to just relax the tolerance for GPU

Makes sense.

I requested for a clarification here: #11274 (comment)

I already replied 🤔

Co-authored-by: BenjaminBossan <[email protected]>
@sayakpaul sayakpaul marked this pull request as draft April 11, 2025 03:51
@sayakpaul sayakpaul marked this pull request as ready for review April 11, 2025 04:45
Co-authored-by: BenjaminBossan <[email protected]>
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.

Overall LGTM, thanks. I made one small suggestion for a possible edge case, up to you if you want to fix it.

@sayakpaul
Copy link
Member Author

@DN6 could you give the changes under lora_base.py a look too?

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.

3 participants