update rope_scaling #5421
update rope_scaling #5421
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @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 refines the rope_scaling initialization logic within the ModelArguments class. The primary goal is to improve how the original_max_position_embeddings (or origin_max_model_len) is determined, providing a more robust and flexible calculation. Additionally, a strict validation assertion related to model length has been converted into a warning, allowing for greater configuration tolerance.
Highlights
- Enhanced
origin_max_model_lendetermination: The logic for determiningorigin_max_model_lenwithin the_init_rope_scalingmethod has been improved. It now includes a more robust search order and a new fallback calculation that derives the original length fromself.model_info.max_model_lenandself.model_info['factor']iforiginal_max_position_embeddingsis not directly found. - Explicit storage of
origin_max_model_len: The calculatedorigin_max_model_lenis now explicitly added to therope_scalingdictionary, ensuring this derived value is consistently available for further use. - Relaxed model length validation from assert to warning: A critical assertion that
self.max_model_lenmust be less than or equal to the calculatedrope_model_lenhas been replaced with alogger.warning. This change allows the system to proceed with configurations wheremax_model_lenmight exceedrope_model_len, while still alerting the user to this condition.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request updates the logic for rope_scaling initialization. The changes introduce more complex logic for determining origin_max_model_len and replace a strict assertion with a warning for better user experience.
However, I've found a critical issue in the new logic where a ModelInfo object is incorrectly accessed as a dictionary, which will cause a runtime error. I've also identified a potential issue with using .get() for boolean checks which could lead to incorrect behavior with falsy values. My review comment includes a suggested fix for these issues.
| if rope_scaling and rope_scaling.get('original_max_position_embeddings'): | ||
| origin_max_model_len = rope_scaling['original_max_position_embeddings'] | ||
| elif self.model_info.rope_scaling and 'original_max_position_embeddings' in self.model_info.rope_scaling: | ||
| origin_max_model_len = self.model_info.rope_scaling['original_max_position_embeddings'] | ||
| else: | ||
| elif self.model_info.rope_scaling: | ||
| if self.model_info.rope_scaling.get('original_max_position_embeddings'): | ||
| origin_max_model_len = self.model_info.rope_scaling['original_max_position_embeddings'] | ||
| elif self.model_info.get('factor'): | ||
| origin_max_model_len = self.model_info.max_model_len // self.model_info['factor'] |
There was a problem hiding this comment.
This block has a couple of issues:
- Using
.get()in a boolean context can be problematic if the values for'original_max_position_embeddings'or'factor'can be falsy (e.g.,0) but still valid. It's safer to check for key existence explicitly. self.model_infois aModelInfoobject, not a dictionary. Accessing it with.get()or[]will raise an error. You likely intended to accessself.model_info.rope_scaling.
The logic can also be simplified to be less nested and more readable.
| if rope_scaling and rope_scaling.get('original_max_position_embeddings'): | |
| origin_max_model_len = rope_scaling['original_max_position_embeddings'] | |
| elif self.model_info.rope_scaling and 'original_max_position_embeddings' in self.model_info.rope_scaling: | |
| origin_max_model_len = self.model_info.rope_scaling['original_max_position_embeddings'] | |
| else: | |
| elif self.model_info.rope_scaling: | |
| if self.model_info.rope_scaling.get('original_max_position_embeddings'): | |
| origin_max_model_len = self.model_info.rope_scaling['original_max_position_embeddings'] | |
| elif self.model_info.get('factor'): | |
| origin_max_model_len = self.model_info.max_model_len // self.model_info['factor'] | |
| if rope_scaling and 'original_max_position_embeddings' in rope_scaling: | |
| origin_max_model_len = rope_scaling['original_max_position_embeddings'] | |
| elif self.model_info.rope_scaling: | |
| if 'original_max_position_embeddings' in self.model_info.rope_scaling: | |
| origin_max_model_len = self.model_info.rope_scaling['original_max_position_embeddings'] | |
| elif 'factor' in self.model_info.rope_scaling: | |
| origin_max_model_len = self.model_info.max_model_len // self.model_info.rope_scaling['factor'] |
…_2' into update_rope_scaling_2
No description provided.