-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[bugfix] fix missing generate method for InternVL-2.5 #7019
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
Summary of ChangesHello @xwy-bit, 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 resolves a critical bug impacting InternLM2 models when integrated with 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 fix for a compatibility issue with transformers>=4.50 by dynamically injecting GenerationMixin when the generate method is missing. The approach of monkey-patching __bases__ is a direct way to address this kind of library incompatibility. My review includes a suggestion to improve code style by correcting indentation and using a logger instead of print, which aligns with the logging practices in other parts of the codebase.
| if model is not None: | ||
| # fix missing generate method for InternVL-2.5 models when using transformers >= 4.50 | ||
| llm_part = getattr(model, 'language_model', model) | ||
| if not hasattr(llm_part, 'generate'): | ||
| print("Detected missing 'generate' method (transformers >= 4.50). Injecting GenerationMixin...") | ||
|
|
||
| cls = llm_part.__class__ | ||
| if GenerationMixin not in cls.__bases__: | ||
| cls.__bases__ = cls.__bases__ + (GenerationMixin,) | ||
|
|
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 block has some indentation issues and uses print for logging. For consistency with the rest of the codebase and to allow users to control log levels, it's better to use a proper logger.
I've corrected the indentation and replaced print with logger.info.
To make this work, you'll also need to add the following at the top of the file:
from swift.utils import get_logger
logger = get_logger()| if model is not None: | |
| # fix missing generate method for InternVL-2.5 models when using transformers >= 4.50 | |
| llm_part = getattr(model, 'language_model', model) | |
| if not hasattr(llm_part, 'generate'): | |
| print("Detected missing 'generate' method (transformers >= 4.50). Injecting GenerationMixin...") | |
| cls = llm_part.__class__ | |
| if GenerationMixin not in cls.__bases__: | |
| cls.__bases__ = cls.__bases__ + (GenerationMixin,) | |
| if model is not None: | |
| # fix missing generate method for InternVL-2.5 models when using transformers >= 4.50 | |
| llm_part = getattr(model, 'language_model', model) | |
| if not hasattr(llm_part, 'generate'): | |
| logger.info("Detected missing 'generate' method (transformers >= 4.50). Injecting GenerationMixin...") | |
| cls = llm_part.__class__ | |
| if GenerationMixin not in cls.__bases__: | |
| cls.__bases__ = cls.__bases__ + (GenerationMixin,) | |
|
try use transformers==4.49 |
|
You're right. I hope this PR remains compatible with both versions prior to 4.49 and above, since 4.50 introduced the breaking change, and transformers is already at 4.57.3 (5.0.0rc). |
PR type
PR information
Summary This PR addresses a compatibility issue with
transformersversion 4.50+, wherePreTrainedModelno longer inherits fromGenerationMixinby default. As a result, the InternLM2 model loses its text generation capability, causing anAttributeError.Changes
get_model_tokenizer_internvlto perform a runtime check on the loaded model.GenerationMixininto the model's__bases__if thegeneratemethod is missing.transformersversions.Before vs. After
Before
After
