-
-
Notifications
You must be signed in to change notification settings - Fork 11.2k
[Bugfix] Make get_mrope_input_positions instance methods
#27342
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
[Bugfix] Make get_mrope_input_positions instance methods
#27342
Conversation
Signed-off-by: DarkLight1337 <[email protected]>
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 correctly changes get_mrope_input_positions from a classmethod to an instance method across multiple models to align with the SupportsMRoPE interface. While reviewing, I noticed that the type hints for image_grid_thw in these methods do not match the interface, which expects them to be optional. The current implementations would raise an error if None is passed. I've added comments with high severity to correct the type hints and suggest adding None checks to prevent potential runtime errors.
| self, | ||
| input_tokens: list[int], | ||
| hf_config: PretrainedConfig, | ||
| image_grid_thw: list[list[int]] | torch.Tensor, |
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 get_mrope_input_positions method signature in the SupportsMRoPE interface specifies that image_grid_thw can be None. This implementation's type hint should be updated to reflect this by adding | None.
Additionally, the method body does not correctly handle the case where image_grid_thw or video_grid_thw are None. For example, image_grid_thw.tolist() at line 1430 will raise an AttributeError if image_grid_thw is None. Please add checks to handle None values at the beginning of the method, for instance by initializing them to an empty list.
| image_grid_thw: list[list[int]] | torch.Tensor, | |
| image_grid_thw: list[list[int]] | torch.Tensor | None, |
| self, | ||
| input_tokens: list[int], | ||
| hf_config: PretrainedConfig, | ||
| image_grid_thw: list[list[int]] | torch.Tensor, |
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 get_mrope_input_positions method signature in the SupportsMRoPE interface specifies that image_grid_thw can be None. This implementation's type hint should be updated to reflect this by adding | None.
Additionally, the method body does not correctly handle the case where image_grid_thw or video_grid_thw are None. For example, image_grid_thw.tolist() at line 644 will raise an AttributeError if image_grid_thw is None. Please add checks to handle None values at the beginning of the method, for instance by initializing them to an empty list.
| image_grid_thw: list[list[int]] | torch.Tensor, | |
| image_grid_thw: list[list[int]] | torch.Tensor | None, |
| self, | ||
| input_tokens: list[int], | ||
| hf_config: PretrainedConfig, | ||
| image_grid_thw: list[list[int]] | torch.Tensor, |
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 get_mrope_input_positions method signature in the SupportsMRoPE interface specifies that image_grid_thw can be None. This implementation's type hint should be updated to reflect this by adding | None.
Additionally, the method body does not correctly handle the case where image_grid_thw or video_grid_thw are None. For example, len(image_grid_thw) at line 641 will raise a TypeError if image_grid_thw is None. Please add checks to handle None values at the beginning of the method, for instance by initializing them to an empty list.
| image_grid_thw: list[list[int]] | torch.Tensor, | |
| image_grid_thw: list[list[int]] | torch.Tensor | None, |
| self, | ||
| input_tokens: list[int], | ||
| hf_config: PretrainedConfig, | ||
| image_grid_thw: list[list[int]] | torch.Tensor, |
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 get_mrope_input_positions method signature in the SupportsMRoPE interface specifies that image_grid_thw can be None. This implementation's type hint should be updated to reflect this by adding | None.
Additionally, the method body does not correctly handle the case where image_grid_thw or video_grid_thw are None. For example, torch.tensor(image_grid_thw) at line 1035 will raise an error if image_grid_thw is None. Please add checks to handle None values at the beginning of the method, for instance by initializing them to an empty list or tensor.
| image_grid_thw: list[list[int]] | torch.Tensor, | |
| image_grid_thw: list[list[int]] | torch.Tensor | None, |
| self, | ||
| input_tokens: list[int], | ||
| hf_config: PretrainedConfig, | ||
| image_grid_thw: list[list[int]] | torch.Tensor, |
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 get_mrope_input_positions method signature in the SupportsMRoPE interface specifies that image_grid_thw can be None. This implementation's type hint should be updated to reflect this by adding | None.
Additionally, the method body does not correctly handle the case where image_grid_thw or video_grid_thw are None. For example, indexing image_grid_thw at line 1128 will raise a TypeError if it is None. Please add checks to handle None values at the beginning of the method, for instance by initializing them to an empty list.
| image_grid_thw: list[list[int]] | torch.Tensor, | |
| image_grid_thw: list[list[int]] | torch.Tensor | None, |
| self, | ||
| input_tokens: list[int], | ||
| hf_config: PretrainedConfig, | ||
| image_grid_thw: list[list[int]] | torch.Tensor, |
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 get_mrope_input_positions method signature in the SupportsMRoPE interface specifies that image_grid_thw can be None. This implementation's type hint should be updated to reflect this by adding | None.
Additionally, the method body does not correctly handle the case where image_grid_thw or video_grid_thw are None. For example, indexing image_grid_thw at line 1529 will raise a TypeError if it is None. Please add checks to handle None values at the beginning of the method, for instance by initializing them to an empty list.
| image_grid_thw: list[list[int]] | torch.Tensor, | |
| image_grid_thw: list[list[int]] | torch.Tensor | None, |
…ect#27342) Signed-off-by: DarkLight1337 <[email protected]>
…ect#27342) Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: Alberto Perdomo <[email protected]>
…ect#27342) Signed-off-by: DarkLight1337 <[email protected]>
…ect#27342) Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: 0xrushi <[email protected]>
…ect#27342) Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: 0xrushi <[email protected]>
…ect#27342) Signed-off-by: DarkLight1337 <[email protected]>
…ect#27342) Signed-off-by: DarkLight1337 <[email protected]>
Purpose
Some methods were still defined as
classmethods, contrary to the interface.Follow up to #24172
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.