-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
LoftQ: Add LoftQ method integrated into LoRA. Add example code for LoftQ usage. #1150
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
|
||
def extract_answer_number(sentence: str) -> float: | ||
sentence = sentence.replace(",", "") | ||
pred = [s for s in re.findall(r"-?\d+\.?\d*", sentence)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, ruff complains that [s for s in re.findall(r"-?\d+\.?\d*", sentence)]
should be list(re.findall(r"-?\d+\.?\d*", sentence))
. Fine I guess, but when I tried, findall
already returns a list, so neither should be necessary.
What you could do on top is pre-compile the regex. So something like
PATTERN_NUMBER = re.compile(r"-?\d+\.?\d*")
def extract_answer_number(sentence: str) -> float:
sentence = sentence.replace(",", "")
pred = PATTERN_NUMBER.findall(sentence)
...
pred_answer = PATTERN_NUMBER.findall(segment[1])
...
This should be a bit faster and avoid repeating the same regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestion. I have edited it. Let me know if there is still something I need to do.
change peft model path on HF auto float16/32 for bnb
I failed the test because |
Ah, too bad that this isn't implemented in PyTorch. I would suggest not to add scipy as a standard dependency, as we want to keep those lean. But we can add it as a dev dependency and use a local import of Alternatively, we could add our own ( WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @yxli2123 for super cool work wrt adding LoftQ initialization method for LoRA adapters! 🤩
wrt scipy
error, we can overcome by importing from peft.utils.loftq_utils import loftq_init
in the def loftq_init(self, adapter_name)
method wherein we first raise and error if scipy is not installed.
Apart from that, left a nit suggestion and a question regarding loftq_fake
.
src/peft/tuners/lora/config.py
Outdated
) | ||
loftq_bits: int = field(default=4, metadata={"help": "Quantization bits for LoftQ"}) | ||
loftq_iter: int = field(default=1, metadata={"help": "Alternating iterations for LoftQ"}) | ||
loftq_fake: bool = field( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used anywhere in loftq_utils.py
, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loftq_bits
and loftq_iter
are used. loftq_fake
and bits_pattern
are not used, but could be the future feature. Anyway, I have deleted the loftq_fake
and bits_pattern
.
Co-authored-by: Sourab Mangrulkar <13534540+pacman100@users.noreply.github.com>
Hi, thanks for providing constructive suggestions. I have edited the code. Could you review the code and run test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, I think we're almost finished with the PR.
Could you please run make style
so that CI passes? Also, I found a few minor issues, please check my comments. Finally, the license snippet is missing on some files, would you please be so kind to add it? Thanks!
src/peft/utils/loftq_utils.py
Outdated
try: | ||
from scipy.stats import norm | ||
except ImportError: | ||
raise ImportError("The required package 'scipy' is not installed. Please install it to continue.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please import norm
inside of create_normal_map
, just so that we can be super safe that we don't accidentally introduce a dependency on it? Also, inside of __post_init__
of LoraConfig
, let's check that we can successfully import scipy if self.init_lora_weights == "loftq"
. That way, we can fail as early as possible.
src/peft/utils/loftq_utils.py
Outdated
values /= values.max() | ||
# print(values) | ||
return values | ||
# assert values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
src/peft/utils/loftq_utils.py
Outdated
return weight | ||
|
||
def quantize_block(self, weight): | ||
assert len(weight.shape) == 2 and weight.shape[0] * weight.shape[1] % self.block_size == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we could replace this assert
and the ones below by a proper ValueError
, including a helpful error message for users.
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
…sg, edit example docs
Thanks again. I have added license to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for addressing the remaining comments. This is a great PR and a very useful feature to have in PEFT.
For me, the PR is in a state that it can be merged. I'll check if we want to have another final review by a colleague. For the future, we also should add documentation and unit tests. If no one else wants to work on those, I'll try to find some time next week to tackle that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @yxli2123 on adding LoftQ and addressing all the comments! 🔥🚀✨
I have edited my branch with the conflicts. It should be fine to just accept my branch when conflicts happen. |
Thanks a lot @yxli2123, great PR and super cool feature, I hope it will find a lot of adoption. |
Thank you! We are glad to contribute to this wonderful open-source project. |
--------- Co-authored-by: Sourab Mangrulkar <13534540+pacman100@users.noreply.github.com> Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
Hi @yxli2123 I think I discovered a small bug. When I try to use peft/src/peft/utils/loftq_utils.py Lines 201 to 216 in 1b1091c
Since The same error would occur for 2 bit, but I don't know how relevant that is right now. Another error (lower priority) that I encountered was when trying to pass a model that is already quantized. Of course, this is incorrect usage and we should document that. But maybe we can raise a nice error message when we encountered that, as right now, at first it appears that everything works and we only get an error during the forward pass. Edit: Ran one test and |
Add GPU tests for LoftQ with 4bit quantization. Notes Tests for 8bit quantization are already there but not run at the moment, see this comment: huggingface#1150 (comment) In my testing, 8bit passes when using NFQuantizer, so if the original author is fine with using that, I can make the adjustment.
Hi @BenjaminBossan, thanks for pointing it out. As for 8bit, LoftQ doesn't have advantages over QLoRA, so we leave 8bit for experimental programs. However, if it leads to unwanted errors, we could remove 8bit. As for 2bit, since bitsandbytes hasn't supported 2bit yet, we will use |
Thanks for commenting on this. What would you think about enabling 8bit with the Edit: Solves in #1276. |
Add GPU tests for LoftQ with 4bit quantization. Notes Tests for 8bit quantization are already there but not run at the moment, see this comment: #1150 (comment) In my testing, 8bit passes when using NFQuantizer, so if the original author is fine with using that, I can make the adjustment. --------- Co-authored-by: Younes Belkada <49240599+younesbelkada@users.noreply.github.com>
I have added the LoftQ method to LoRA. I have run the
make style
command.