Fix Qlora/lora for 3.2 vision #2028

felipemello1 · 2024-11-19T19:26:09Z

Context

What is the purpose of this PR? Is it to

add a new feature
fix a bug
update tests and/or documentation
other (please add here)

In this PR we removed apply_lora_to_output from vision encoder, but forgot to remove it from some places. This raises the error:

  File "/data/users/felipemello/torchtune/torchtune/models/llama3_2_vision/_model_builders.py", line 170, in lora_llama3_2_vision_11b
    encoder = lora_llama3_2_vision_encoder(
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/data/users/felipemello/torchtune/torchtune/models/llama3_2_vision/_component_builders.py", line 439, in lora_llama3_2_vision_encoder
    clip = lora_clip_vision_encoder(**clip_options, **lora_options)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/data/users/felipemello/torchtune/torchtune/models/clip/_component_builders.py", line 289, in lora_clip_vision_encoder
    self_attn = lora_clip_attention(
                ^^^^^^^^^^^^^^^^^^^^
  File "/data/users/felipemello/torchtune/torchtune/models/clip/_component_builders.py", line 437, in lora_clip_attention
    adapter_cls(
  File "/data/users/felipemello/torchtune/torchtune/modules/peft/lora.py", line 78, in __init__
    else to_nf4(linear.weight, **quantization_kwargs)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: to_nf4() got an unexpected keyword argument 'apply_lora_to_output'

Additionally, in 4.0 we added a check for quantize_kwargs and quantize_base. This raises the error:

Test plan

pytorch-bot · 2024-11-19T19:26:13Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/2028

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

[DomainsOnly] Jobs fail with GLIBC version not found

✅ No Failures

As of commit c4d155f with merge base d5c54f3 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

pbontrager

Thanks for fixing this. I think "apply_lora_to_output" still makes sense for the encoder, just not the CLIP model itself but still in the projection head.

pbontrager · 2024-11-19T20:17:26Z

torchtune/models/llama3_2_vision/_component_builders.py

    num_hidden_inputs: int,
    # LoRA args
    apply_lora_to_mlp: bool,
-    apply_lora_to_output: bool,


I think this should stay in the projection head, it just doesn't make sense in the CLIP model

pbontrager · 2024-11-19T20:18:06Z

torchtune/models/llama3_2_vision/_component_builders.py

    lora_options = {
        "lora_modules": lora_attn_modules,
        "apply_lora_to_mlp": apply_lora_to_mlp,
-        "apply_lora_to_output": apply_lora_to_output,


This should be removed from here and manually added to the projection head call

RdoubleA · 2024-11-20T15:29:27Z

torchtune/modules/peft/lora.py

        self._quantize_base = quantize_base

-        if not self._quantize_base and quantization_kwargs:
+        if not self._quantize_base and any([v for v in quantization_kwargs.values()]):


did and quantization_kwargs not work for a dictionary? you could also just check for length

good question! it outputs something like: {use_lora_on_output: None}. Even though the value is None, it has a key, which fails the check

In [1]: any([None]) Out[1]: False

sorry {use_lora_on_output: None} should fail the check?

If quantize_base is false, then we should NOT have any quantization args, right? If no quantization is happenning, then why have these args?

So this assertion was failing, because quantize_base WAS false (not False --> True), but we have {use_lora_on_output: None}, which returns True.

So if True and True: raise error.

I agree with Rafi here, there shouldn't be any kwargs passed in if quantize_base is False. {use_lora_on_output: None} should not be passed in when quantize_base = False.

pbontrager

Thank you for fixing this

codecov-commenter · 2024-11-20T20:14:51Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 24.88%. Comparing base (d0aa871) to head (c4d155f).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
torchtune/modules/peft/dora.py	0.00%	1 Missing ⚠️
torchtune/modules/peft/lora.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2028       +/-   ##
===========================================
- Coverage   67.27%   24.88%   -42.40%     
===========================================
  Files         318      318               
  Lines       17648    17631       -17     
===========================================
- Hits        11873     4387     -7486     
- Misses       5775    13244     +7469

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

fix

34b9ceb

felipemello1 requested review from RdoubleA, joecummings and pbontrager November 19, 2024 19:26

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 19, 2024

pbontrager reviewed Nov 19, 2024

View reviewed changes

RdoubleA reviewed Nov 20, 2024

View reviewed changes

fix

80abb7c

felipemello1 requested review from RdoubleA and pbontrager November 20, 2024 19:35

fix

c4d155f

pbontrager approved these changes Nov 20, 2024

View reviewed changes

felipemello1 merged commit d9b6c79 into meta-pytorch:main Nov 20, 2024
17 checks passed

felipemello1 deleted the fix_vision_lora branch November 20, 2024 20:38

ebsmothers mentioned this pull request Nov 26, 2024

v0.5.0 tracker #2008

Closed

44 tasks

Fix Qlora/lora for 3.2 vision #2028

Fix Qlora/lora for 3.2 vision #2028

Uh oh!

Conversation

felipemello1 commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Test plan

Uh oh!

pytorch-bot bot commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/2028

❗ 1 Active SEVs

✅ No Failures

Uh oh!

pbontrager left a comment

Choose a reason for hiding this comment

Uh oh!

pbontrager Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

pbontrager Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

RdoubleA Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

felipemello1 Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RdoubleA Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

RdoubleA Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

felipemello1 Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbontrager Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

felipemello1 Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

pbontrager left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 20, 2024

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

felipemello1 commented Nov 19, 2024 •

edited

Loading

pytorch-bot bot commented Nov 19, 2024 •

edited

Loading

felipemello1 Nov 20, 2024 •

edited

Loading

felipemello1 Nov 20, 2024 •

edited

Loading