Skip to content
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

FIX: A few issues with AdaLora, extending GPU tests #1146

Merged

Conversation

BenjaminBossan
Copy link
Member

This PR fixes a handful of issues with AdaLora, should resolve #1113.

Description

  1. lora_A.weight.device was called but for AdaLora, lora_A is an nn.Paramter, not an nn.Module, so the weight attribute does not exist. lora_A.device is sufficient.
  2. For 8bit, an inplace operation failed because it was on a view. Now the operation is no longer inplace.
  3. The loss term of the model output is not necessarily a torch tensor. In the test, it was a dict and did not contain an actual loss. Therefore, I added a check to make sure the loss is a torch tensor. Is there a better way?

Notes

Running pytest tests/test_gpu_examples.py -k adalora locally (with GPU) passes. Ideally, someone else can confirm, as normal unit tests won't catch this.

If this is merged before #1115, skipping AdaLora tests in that PR can be removed.

This PR fixes a handful of issues with AdaLora, should resolve huggingface#1113.

Description

1. lora_A.weight.device was called but for AdaLora, lora_A is a
   nn.Paramter, not an nn.Module, so the weight attribute does not
   exist. lora_A.device is sufficient.
2. For 8bit, an inplace operation failed because it was on a view. Now
   the operation is no longer inplace.
3. The loss term of the model output is not necessarily a torch tensor.
   In the test, it was a dict and did not contain an actual loss.
   Therefore, I added a check to make sure the loss is a torch tensor.
   Is there a better way?

Notes

Running pytest tests/test_gpu_examples.py -k adalora locally (with GPU)
passes. Ideally, someone else can confirm, as normal unit tests won't
catch this.

If this is merged before huggingface#1115, skipping AdaLora tests in that PR can be
removed.
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 17, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing clean up, thanks @BenjaminBossan !

@BenjaminBossan BenjaminBossan merged commit b5a8a29 into huggingface:main Nov 17, 2023
14 checks passed
@BenjaminBossan BenjaminBossan deleted the fix-adalora-some-small-issues branch November 17, 2023 14:18
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.

AdaLora + bnb not working
3 participants