-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add new features: Safe LoRA #2098
Conversation
Thanks for proposing this PR to add the Safe LoRA method to PEFT. I have only skimmed the code and paper, so this is not an in-depth review yet. Based on what I saw, this is my high level understanding. As a user, I start out by training a LoRA adapter on my dataset using an aligned model as the base model. Next, I want to restore safety which may be reduced after training. For this, I take my trained LoRA model, the base model, and the aligned model, Then Safe LoRA will create a new adapter that "injects" the alignment back into my LoRA adapter. Please correct me if my understanding is incorrect. Implementation-wise, I wonder if we really need a peft_model = ... # my trained LoRA model
# apply_safe_lora is a new function that does what SafeLoRA currently does
# additional arguments like select_layers_type are option args for apply_safe_lora
aligned_model = apply_safe_lora(peft_model, base_model_id, aligned_model_id)
# persist the new safe LoRA adapter
aligned_model.save_pretrained(...) IMO, this would be a simpler API and it would achieve the same thing at the end. In this example, the Another concern I saw when I read the code is that it looks like it will require a lot of memory. IIUC, we need to have the PEFT model, a copy of the PEFT model, the base model, and the aligned model in memory all at once. Even on CPU RAM, this could be difficult to achieve for many users. I wonder if it would be possible to load the weights from the base model and the aligned model one at a time, at least as an option. I have already implemented something like this for another use case. Here is the code: peft/src/peft/utils/loftq_utils.py Lines 263 to 411 in ccc3501
It only works for safetensors, because pickle does not allow to load the weights lazily, but I still think it could be a nice addition. Also, do we really need a full copy of the PEFT model? Would it be sufficient to only copy the LoRA weights? Ideally, I would like to see a solution that works even if the user has less than twice the memory required for the base model. If this doesn't work, it's also okay, but it would greatly reduce the number of users who can use the method. |
Thank you for taking the time to read the code and the paper. Yes, your understanding is correct. Since SafeLoRA is not a new training method for LoRA, but rather a process that enhances the safety of a well-trained LoRA model through certain operations, it may not be necessary to have a separate SafeLoRA class. The most CPU memory-intensive operation here is likely when executing Regarding your last question, my current code in |
Thanks for confirming and further explaining. Would you be willing to make the suggested changes? I think it would help a lot with user adoption of the new method. |
Yes, I will make the changes you suggested. I would like to ask if SafeLoRA is modified into the form of a function like |
Great, thanks.
Good question, I think it should be placed elsewhere. I don't have a strong opinion, how about |
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. |
@chiayi-hsu do you still plan on working on this? |
Yes, I’ll update the new version this week.
Thanks!
Benjamin Bossan ***@***.***>於 2024年10月28日 週一,18:39寫道:
… @chiayi-hsu <https://github.com/chiayi-hsu> do you still plan on working
on this?
—
Reply to this email directly, view it on GitHub
<#2098 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AS67YGV2JRHZNJTO6DFGBFLZ5YA6FAVCNFSM6AAAAABO3KQJSKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBRGIYTOMZVGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hello, I have changed the Additionally, regarding the projected weights, I originally loaded the weights of both the complete base model and the PEFT model, but I have now modified it to only load the weights of the PEFT model. I think this should be more memory-efficient for the user's GPU/CPU. If you have any suggestions, please let me know. Thank you! |
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 the updates. I reviewed the code and left a couple of comments. Before pushing your changes, always make sure to run make style
.
When this is addressed, there will be a few more steps before we can merge the PR:
- We should add an example to the
examples/
folder. Ideally, this can be something similar to what's in the paper, so that we can replicate the results there. - This feature should be documented.
Without these steps, users will have a hard time finding this feature.
src/peft/utils/safelora.py
Outdated
class SafeLoRAConfig: | ||
""" | ||
This is the configuration class to store the configuration of a safeLoRA. | ||
""" |
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 also document the parameters in the docstring? You can just use the same description as below in the help.
src/peft/utils/safelora.py
Outdated
""" | ||
|
||
base_model_path: str = field( | ||
default=None, |
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.
Let's not provide any defaults if None
is not a valid argument.
src/peft/utils/safelora.py
Outdated
) | ||
|
||
aligned_model_path: str = field( | ||
default=None, |
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.
Let's not provide any defaults if None
is not a valid argument.
src/peft/utils/safelora.py
Outdated
) | ||
|
||
peft_model_path: str = field( | ||
default=None, |
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.
Let's not provide any defaults if None
is not a valid argument.
src/peft/utils/safelora.py
Outdated
If config.saveWeights is True, the original LoRA weight file will be replaced by the SafeLoRA weights. | ||
""" | ||
|
||
with open(f"{os.path.join(configs.peft_model_path, 'adapter_config.json')}") as f: |
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.
You should use PeftConfig.from_pretrained
instead.
src/peft/utils/safelora.py
Outdated
|
||
with open(f"{os.path.join(configs.peft_model_path, 'adapter_config.json')}") as f: | ||
peft_config = json.load(f) | ||
v = get_aligned_matrix(configs.base_model_path, configs.aligned_model_path, configs.devices, peft_config) |
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.
Again, let's use more descriptive variable names throughout.
src/peft/utils/safelora.py
Outdated
if configs.devices == "cpu": | ||
peft_weights = {name: f.get_tensor(name).to(torch.float32) for name in f.keys()} | ||
else: | ||
peft_weights = {name: f.get_tensor(name).to(torch.bfloat16) for name in f.keys()} |
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.
Why hard-code bfloat16? Maybe it's better to let users provide the dtype
as an argument.
Sorry for the inconvenience. The pull request was closed due to syncing with the latest version of PEFT. I have resubmitted the PR. |
Hello,
We have published a paper called Safe LoRA (https://arxiv.org/abs/2405.16833).
This work focuses on improving the safety of well-trained LoRA models.
Additionally, I have provided an example implementation in the model.py file to illustrate how to apply the Safe LoRA approach effectively.