compat trl 0.15 #5905
compat trl 0.15 #5905
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 implements a minor but essential adjustment to the 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a compatibility fix for trl. It adds the ld_alpha field to DPOConfig to support older versions of trl where this field is not present, preventing potential AttributeError. The change is correct and minimal. I've added one suggestion to improve the clarity of a code comment for better future maintainability.
| @dataclass | ||
| class DPOConfig(SwiftArgumentsMixin, HfDPOConfig): | ||
| pass | ||
| ld_alpha: Optional[float] = None # compat trl==0.15 |
There was a problem hiding this comment.
The comment # compat trl==0.15 is a bit confusing as there doesn't seem to be a 0.15 version for the trl library. This could make future maintenance more difficult. The ld_alpha parameter was introduced in trl version 0.7.1. To improve clarity, consider updating the comment to specify the correct version range, for example: # compat trl < 0.7.1.
| ld_alpha: Optional[float] = None # compat trl==0.15 | |
| ld_alpha: Optional[float] = None # compat trl < 0.7.1 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to add compatibility for trl version 0.15 and includes a refactoring of the model conversion logic. The changes involve adding a new ld_alpha field to DPOConfig and modifying convert_mcore2hf to delay model loading, which is a good memory optimization. My review focuses on a potential performance consideration in the refactored code.
| from swift.megatron import prepare_mcore_model, adapter_state_dict_context | ||
| hf_model, template = prepare_model_template( | ||
| args, load_model=args.to_hf, patch_offload=not args.test_convert_precision) | ||
| _, template = prepare_model_template(args, load_model=False) |
There was a problem hiding this comment.
This change refactors the logic to delay loading the hf_model until it's needed inside the if args.to_hf: block, which is a good practice for memory efficiency. However, this introduces a second call to prepare_model_template on line 297. While this improves code structure, it's worth ensuring that calling prepare_model_template twice doesn't introduce a significant performance overhead. If the function is lightweight when load_model=False, this is fine. Otherwise, it might be better to refactor prepare_model_template to separate template creation from model loading more explicitly.
2 participants
No description provided.