previous_dtype is now inferred from F.linear's result output type. by MFajcik · Pull Request #1010 · huggingface/peft

MFajcik · 2023-10-10T11:43:32Z

Fixes the issue from #971

HuggingFaceDocBuilderDev · 2023-10-10T14:53:50Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

BenjaminBossan · 2023-10-11T10:15:38Z

Thanks for providing the fix. Could you please run make style on your code? This should make the quality test pass, after which the full CI should run.

MFajcik · 2023-10-11T11:01:26Z

Done. Also, I tested it, successfully running my experiment with GPT-2 LORA fine-tuning, with original model computation running in bf16 as intended.

BenjaminBossan

Thanks for providing this fix and for testing it. I ran the GPU tests locally and they also passed (but I can't test with bf16). Do you happen to have a quick test at the ready to show that it works with bf16 and that doesn't involve inspecting intermediate results (as in the original issue)?

Regarding the PR, could you please remove the empty line 243, it's not required anymore. Also, should the forward methods of Embedding and Conv2d also be changed accordingly?

BenjaminBossan · 2023-10-25T14:04:43Z

@MFajcik Are you still working on this?

BenjaminBossan · 2023-11-16T13:17:33Z

@MFajcik Friendly reminder about this PR :)

There are some merge conflicts now due to a recent change. Let me know if you need help resolving it.

MFajcik · 2023-11-16T13:40:48Z

@BenjaminBossan Yeah I didn't forget about this. I just don't have time right now.
I will try to resolve these next week.

BenjaminBossan · 2023-11-16T14:12:30Z

Fantastic, thanks. I don't want to put any pressure on you, take the time you need, I just wanted to ping you because sometimes people just forget :)

MFajcik · 2023-12-06T11:46:55Z

@BenjaminBossan I merged changes, and reapplied fix for Conv2D, Linear and Embeddings. I also ran make format, and added 1 test covering the fix for LoRA. Is that ok?

BenjaminBossan · 2023-12-06T12:54:53Z

@MFajcik Thanks for making the updates. The quality check still fails, could you please re-run make style? If that doesn't report back anything, maybe some package versions are outdated and need upgrading.

added 1 test covering the fix for LoRA

Thanks for adding the test. Probably we can move it to some of the existing files, but I'll take a closer look later and make a suggestion.

MFajcik · 2023-12-06T13:40:29Z

@MFajcik Thanks for making the updates. The quality check still fails, could you please re-run make style? If that doesn't report back anything, maybe some package versions are outdated and need upgrading.

added 1 test covering the fix for LoRA

Thanks for adding the test. Probably we can move it to some of the existing files, but I'll take a closer look later and make a suggestion.

Yeah, I forgot to run make style after writing the test again... Should be okay now.

BenjaminBossan · 2023-12-07T15:48:52Z

@MFajcik Code quality checks are still failing:

tests/test_autocast_torchcompatibility_lora.py:141:20: F841 Local variable labels is assigned to but never used

BenjaminBossan · 2023-12-15T11:34:36Z

Friendly ping @MFajcik

MFajcik · 2023-12-15T13:28:39Z

now? @BenjaminBossan

younesbelkada

Thanks, I left a single comment with respect to failing tests

younesbelkada · 2023-12-18T09:40:18Z

tests/test_autocast_torchcompatibility_lora.py

+        return conv_output
+
+
+class TestAutoCast(unittest.TestCase):


This test seems to require a GPU, can you add the require_torch_gpu decorator here?

peft/tests/testing_utils.py

Line 25 in 3708793

def require_torch_gpu(test_case):

BenjaminBossan

Thanks a lot @MFajcik for making LoRA work correctly with AMP. This looks pretty good already. I have a couple of change requests and suggestions for improvement, please check them out.

In terms of the general approach, now that we use

result = self.base_layer(x, *args, **kwargs)

I think it's much safer to make this change than at the time the issue was originally discussed. This way, we should have a guarantee that LoRA does not change the dtype of the output.

BenjaminBossan · 2023-12-19T12:46:04Z

src/peft/tuners/lora/layer.py

        lora_dropout: float = 0.0,
-        fan_in_fan_out: bool = False,  # Set this to True if the layer to replace stores weight like (fan_in, fan_out)
+        fan_in_fan_out: bool = False,
+        # Set this to True if the layer to replace stores weight like (fan_in, fan_out)


Can we undo this change, or at least move the comment above 254, where it belongs?

BenjaminBossan · 2023-12-19T12:49:34Z

tests/test_autocast_torchcompatibility_lora.py

@@ -0,0 +1,159 @@
+import unittest


Could you please add the comment header that we have in all our files? Also, please move this test to test_gpu_examples.py, otherwise it won't be run on CI (since our normal CI has no GPUs and only selected test files are run with GPU in our nightly tests).

BenjaminBossan · 2023-12-19T12:50:25Z

tests/test_autocast_torchcompatibility_lora.py

+
+class SimpleModel(nn.Module):
+    def __init__(self):
+        super(SimpleModel, self).__init__()


Nit:

Suggested change

super(SimpleModel, self).__init__()

super().__init__()

Same for other classes.

BenjaminBossan · 2023-12-19T12:51:58Z

tests/test_autocast_torchcompatibility_lora.py

+        self.embedding_layer = nn.Embedding(1000, 768)
+        self.layer_norm = nn.LayerNorm(768)
+        self.linear_transform_base = nn.Linear(768, 256)
+        self.linear_transform = LoraLinear(


Instead of wrapping the layer explicitly, let's create a LoraConfig and call get_peft_model with SimpleModel as input. test_simple_lora_linear_model could be responsible for initializing the class and passes the instance to _test_model. This way, we can be 100% that this generates a model the same way that users typically do.

BenjaminBossan · 2023-12-19T12:52:52Z

tests/test_autocast_torchcompatibility_lora.py

+        super(SimpleLorAEmbeddingModel, self).__init__()
+
+        self.embedding_layer_base = nn.Embedding(1000, 768)
+        self.embedding_layer = LoraEmbedding(


Same argument as above.

BenjaminBossan · 2023-12-19T12:52:59Z

tests/test_autocast_torchcompatibility_lora.py

+        self.embedding_layer = nn.Embedding(1000, 768)
+        self.layer_norm = nn.LayerNorm(768)
+        self.conv2d_transform_base = nn.Conv2d(1, 256, kernel_size=(3, 3), stride=(1, 1), padding=(1, 1))
+        self.conv2d_transform = LoraConv2d(


Same argument as above.

BenjaminBossan · 2023-12-19T12:56:08Z

tests/test_autocast_torchcompatibility_lora.py

+
+@require_torch_gpu
+class TestAutoCast(unittest.TestCase):
+    def test_simple_model(self):


WDYT about parametrizing the test (using parameterize, see other tests) over the dtype? That way, we can run a single test per test case, which is usually preferable.

BenjaminBossan · 2023-12-19T12:58:07Z

tests/test_autocast_torchcompatibility_lora.py

+        # Prepare dummy inputs
+        input_ids = torch.randint(0, 1000, (2, 10)).cuda()
+
+        # Forward pass with torch.bfloat16


For the bf16 case, can we please run self.skipTest if not torch.cuda.is_bf16_supported()?

BenjaminBossan · 2024-01-04T13:29:39Z

@MFajcik Do you still plan to work on this? :)

MFajcik · 2024-01-04T15:37:40Z

@MFajcik Do you still plan to work on this? :)

Hopefully this month sometimes :)

review-notebook-app · 2024-01-24T14:46:23Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

MFajcik · 2024-01-24T15:16:30Z

@BenjaminBossan I think I adressed all your comments now.

BenjaminBossan

Thanks a lot @MFajcik for providing the fix and adding tests for it. From my point of view, this looks good, but I'd like to have another review by @younesbelkada or @pacman100 to be 100% sure.

I saw that other methods probably need to be changed in the same way as done in this PR (LoHa, LoKr, poly, oft, IA³, lora.tp_layer.LoraParallelLinear), but that can be done in a subsequent PR.

tests/test_gpu_examples.py

pacman100

Thank you @MFajcik for the fixes and @BenjaminBossan for all the discussions and suggestions!

BenjaminBossan · 2024-02-19T11:14:29Z

@MFajcik I think we should be good to merge this PR once the merge conflict is resolved. It should be pretty straightforward, let me know if there are any questions. Please note that we now use normal asserts everywhere instead of unittest's self.assertEqual etc.

# Conflicts: # tests/test_gpu_examples.py

BenjaminBossan · 2024-02-19T12:59:36Z

Could you please run make style too :)

previous_dtype is now inferred from F.linear's result output type.

ec29aa8

make style formatting

552935a

BenjaminBossan reviewed Oct 11, 2023

View reviewed changes

BenjaminBossan mentioned this pull request Oct 11, 2023

Add implementation of LyCORIS LoKr (KronA-like adapter) for SD&SDXL models #978

Merged

4 tasks

Martin Fajcik added 2 commits December 6, 2023 10:49

merged with recent changes

2c1c8b0

added fix, also for Conv2D and Embedding layer

f6ad119

MFajcik force-pushed the main branch from 63595c2 to f6ad119 Compare December 6, 2023 10:17

added autocast compatibility test

aac3781

MFajcik mentioned this pull request Dec 6, 2023

What's up with autocast in PEFT? #971

Closed

4 tasks

run make style on test

0f0daae

baoleai mentioned this pull request Dec 8, 2023

change dtype of output after passing through lora_A #1172

Closed

4 tasks

removed labels

96d9ec7

younesbelkada reviewed Dec 18, 2023

View reviewed changes

added gpu requirement for this test

3c7eb2e

BenjaminBossan requested changes Dec 19, 2023

View reviewed changes

MFajcik force-pushed the main branch from 8c809e8 to 3c7eb2e Compare January 24, 2024 14:51

Martin Fajcik added 4 commits January 24, 2024 15:53

review changes

c09fbe7

merge with main

0b4001c

added skip test

1748f21

reverted comment position

57f6d8d

BenjaminBossan approved these changes Feb 8, 2024

View reviewed changes

tests/test_gpu_examples.py Show resolved Hide resolved

added comment for lora dtype consistency test

6d1df05

pacman100 approved these changes Feb 19, 2024

View reviewed changes

Martin Fajcik added 2 commits February 19, 2024 13:08

Merge remote-tracking branch 'peft/main'

6b38805

# Conflicts: # tests/test_gpu_examples.py

use regular assert to comply with make style

c697dbe

make style

751c5ce

BenjaminBossan merged commit 7b7e4b2 into huggingface:main Feb 19, 2024

BenjaminBossan pushed a commit to BenjaminBossan/peft that referenced this pull request Mar 14, 2024

Better respect result dtype in LoRA layers (huggingface#1010)

cbafd66

Guy-Bilitski pushed a commit to Guy-Bilitski/peft that referenced this pull request May 13, 2025

Better respect result dtype in LoRA layers (huggingface#1010)

76886a8

Conversation

MFajcik commented Oct 10, 2023

Uh oh!

HuggingFaceDocBuilderDev commented Oct 10, 2023

Uh oh!

BenjaminBossan commented Oct 11, 2023

Uh oh!

MFajcik commented Oct 11, 2023

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan commented Oct 25, 2023

Uh oh!

BenjaminBossan commented Nov 16, 2023

Uh oh!

MFajcik commented Nov 16, 2023

Uh oh!

BenjaminBossan commented Nov 16, 2023

Uh oh!

MFajcik commented Dec 6, 2023

Uh oh!

BenjaminBossan commented Dec 6, 2023

Uh oh!

MFajcik commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenjaminBossan commented Dec 7, 2023

Uh oh!

BenjaminBossan commented Dec 15, 2023

Uh oh!

MFajcik commented Dec 15, 2023

Uh oh!

younesbelkada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan commented Jan 4, 2024

Uh oh!

MFajcik commented Jan 4, 2024

Uh oh!

review-notebook-app bot commented Jan 24, 2024

Uh oh!

MFajcik commented Jan 24, 2024

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pacman100 left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan commented Feb 19, 2024

Uh oh!

BenjaminBossan commented Feb 19, 2024

Uh oh!

Reviewers

Assignees

MFajcik commented Dec 6, 2023 •

edited

Loading