-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Implement LoRA for MoE with support for LoRA injection for nn.parameters #9337
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
base: main
Are you sure you want to change the base?
Conversation
add lora parameter webUI
…-config-logic 3 add formating parameters to lora config logic
Summary of ChangesHello @Ziheng-Zhang-AUS, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the LoRA fine-tuning capabilities by enabling users to target specific "nn.Parameters" for LoRA injection, rather than being limited to entire modules. This granular control is especially valuable for advanced model architectures like Mixture-of-Experts (MoE) models, allowing for more precise and efficient adaptation. The changes span across configuration, model adaptation logic, testing, and the graphical user interface, ensuring a complete and robust implementation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a valuable feature to support LoRA injection for nn.Parameters, which is particularly useful for Mixture-of-Experts (MoE) models. The implementation looks mostly correct, with new arguments added to the configuration, CLI, and web UI.
However, I've found several critical issues in the tests. The utility function check_lora_model has been refactored, but the existing tests that rely on it have not been updated correctly, causing them to fail or test the wrong thing. The new test for lora_parameters also has incorrect logic. Additionally, the refactored check_lora_model seems to have lost some functionality compared to the old version, specifically for checking modules_to_save.
I've also noticed a minor issue with a print statement that should be a logger call, and an unrelated .drawio file that might have been added by mistake.
Please address the critical issues in the tests to ensure the new functionality is properly verified and doesn't introduce regressions.
|
Please resolve the gemini's review |
…-config-logic changed the implementation of func check_lora_model
|
I have modified the implementation according to Gemini's review :) |
…-config-logic conflict lora parameters test
| target_parameters = [] | ||
| if len(finetuning_args.lora_target) == 1 and finetuning_args.lora_target[0] == "all": | ||
| target_modules = find_all_linear_modules(model, finetuning_args.freeze_vision_tower) | ||
| if finetuning_args.lora_parameters: # if specified the parameters to be adapted, use them |
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.
When we specify the target parameters, the target modules should not be affected
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 if-else is strange
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.
Sorry, I didn't understand the idea very clearly, because I noticed that Lora target has a default value of "all". I was thinking that if this default value is not changed and Lora parameters are used for injection without the user specifying a target, then this if-else should be judged here.
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 current implementation will prevent target_parameters (lora_parameters) from being passed to peftconfig when target_modules (lora_target) is specified. There is no need to modify this peice of code to pass lora_parameters. Instead I would pass it immediatly to peft_kwargs:
peft_kwargs = {
"r": finetuning_args.lora_rank,
"target_modules": target_modules,
"target_parameters": finetuning_args.lora_parameters,
"lora_alpha": finetuning_args.lora_alpha,
"lora_dropout": finetuning_args.lora_dropout,
"use_rslora": finetuning_args.use_rslora,
"use_dora": finetuning_args.use_dora,
"modules_to_save": finetuning_args.additional_target,
}
target_parameters is an optional argument with None as default.
| "use_rslora": finetuning_args.use_rslora, | ||
| "use_dora": finetuning_args.use_dora, | ||
| "modules_to_save": finetuning_args.additional_target, | ||
| "target_parameters": target_parameters, |
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.
Is target parameters always be defined?
What does this PR do?
Fixes #9251
Before submitting