-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[v4] refactor ms-swift v4 (pipelines/arguments/swiftmixin/callback/tuner_plugin) #7385
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
[v4] refactor ms-swift v4 (pipelines/arguments/swiftmixin/callback/tuner_plugin) #7385
Conversation
Summary of ChangesHello @Jintao-Huang, 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 initiates a refactoring effort for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 refactors how extra callbacks are managed in swift/plugins/callback.py. The extra_callbacks variable has been changed from a list to a dictionary, which serves as a registry for available callbacks like EarlyStopCallback and PerfMetricsLogCallback. This is a positive change that improves the code's structure and maintainability by allowing callbacks to be accessed by name. The implementation is clean and follows good practices. I have no specific suggestions for improvement on this change.
…callbacks' into refactor_pipelines_callbacks
|
/gemini review |
|
/gemini review |
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 is a significant refactoring effort for ms-swift v4. The core changes include renaming the --train_type argument to --tuner_type for better clarity, and a major restructuring of the plugins module into more organized packages like callbacks, loss, optimizers, and tuner_plugin. The move away from the custom swift.tuners.Swift class in favor of the standard peft library is a great improvement for maintainability and interoperability. The changes are widespread across documentation, examples, and core library code, and for the most part, they are consistent and well-executed.
I've found a few minor inconsistencies and potential issues:
- A couple of documentation files and an example script have inconsistent parameter renames (
sft_typetotrain_typeinstead oftuner_type, and--tuner_backendto--train_backend). - An example notebook seems to contain user-specific metadata changes.
- Some comments in an example script point to outdated file paths after the refactoring.
Overall, this is a solid refactoring that improves the framework's structure and clarity. Addressing the minor issues I've pointed out will make it even better.
|
/gemini review |
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 significant and valuable refactoring for ms-swift v4. The key changes include renaming the train_type argument to tuner_type for better clarity, restructuring the plugin system for callbacks and tuners into dedicated modules, and aligning with the peft library by replacing custom Swift class methods with standard PeftModel usage. These changes improve code organization, maintainability, and standardization. The updates across documentation, examples, and internal arguments are largely consistent. I've identified a minor inconsistency in one of the examples and a potential bug in the new LISACallback implementation. Overall, this is a solid step forward for the framework.
|
/gemini review |
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 significant refactoring for the upcoming v4 release. The changes primarily focus on improving modularity and aligning with standard libraries. Key changes include renaming train_type to tuner_type across the codebase, restructuring the plugins module into more specific modules like callbacks, rewards, and tuner_plugin, and replacing the custom Swift class with peft.PeftModel. These are excellent changes that enhance maintainability and extensibility. The refactoring has been applied consistently across a large number of documentation and example files. I have one suggestion to improve consistency in an example file.
| model = Swift.from_pretrained(model, adapter_dir) | ||
| # You can also write it as: | ||
| # model = PeftModel.from_pretrained(model, adapter_dir) |
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.
While Swift.from_pretrained might still work for backward compatibility, it's better to consistently use PeftModel.from_pretrained across all examples to align with the refactoring's goal of adopting the peft library more directly. This will improve consistency and reduce confusion for users.
| model = Swift.from_pretrained(model, adapter_dir) | |
| # You can also write it as: | |
| # model = PeftModel.from_pretrained(model, adapter_dir) | |
| model = PeftModel.from_pretrained(model, adapter_dir) |
No description provided.