-
Notifications
You must be signed in to change notification settings - Fork 66
update ci and support llava-next #380
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
Conversation
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.
Hello @SmudgedWings, 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!
Summary of Changes
Hello team, Gemini here to provide a summary of this pull request by @SmudgedWings. The primary goal of this PR, as indicated by the title and description, is to add support for the LLaVA-Next model, specifically focusing on the Vicuna variant. The changes involve modifications to the LLaVA model implementation file (llmc/models/llava_lht.py) to accommodate LLaVA-Next loading and configuration. Additionally, the requirements.txt file containing the project's Python dependencies has been removed in its entirety. While the title mentions updating CI, the provided patches focus solely on the code and dependency changes.
Highlights
- LLaVA-Next Support: Adds initial support for the LLaVA-Next model, specifically the Vicuna variant, by updating the model loading and configuration logic.
- Dependency Management Change: Removes the
requirements.txtfile, indicating a potential shift in how project dependencies are managed. - Model Loading Adjustments: Modifies the
llmc/models/llava_lht.pyfile to adjust variable names (llava_llama_configtollava_config), reorder arguments in theload_pretrained_modelfunction call, and update comments related to context truncation for LLaVA-1.6.
Changelog
- llmc/models/llava_lht.py
- Renamed
self.llava_llama_configtoself.llava_configin thebuild_modelmethod. - Adjusted the order of arguments (
deviceandtorch_dtype) when callingload_pretrained_model. - Updated the comment for the
truncate_contextparameter in the__init__method to clarify its setting for LLaVA-1.6. - Added minor blank lines for formatting.
- Renamed
- requirements.txt
- Removed the entire file containing pinned Python dependencies.
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 is currently in preview and 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 to provide feedback.
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.
New model arrives,
Dependencies now depart,
Code adapts and runs.
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.
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 aims to update the CI and add support for llava-next. The changes within llmc/models/llava_lht.py appear to be adjustments for llava-next compatibility, such as renaming configuration variables and minor refactoring of model loading, which seem reasonable.
A key area for discussion is the change in dependency management. The deletion of a top-level requirements file, which previously pinned dependencies to specific versions, in favor of requirements/runtime.txt (which uses minimum versions like torch>=2.2.0), is a significant shift. This change could affect build reproducibility and stability. The PR description is brief and doesn't elaborate on this aspect of the "update ci" objective. Further clarification on this strategy would be beneficial.
Summary of Findings
- Dependency Management Strategy Change: A top-level requirements file with specifically pinned dependencies was deleted. The project now appears to rely on
requirements/runtime.txt, which uses minimum version specifications (e.g.,torch>=2.2.0). This is a significant shift that could affect reproducibility and stability, and has been raised as a high-severity concern in the review comments. - Refactoring in
LlavaLHT.build_model: Inllmc/models/llava_lht.py, theimage_processorreturned byload_pretrained_modelis no longer assigned toself.image_processor. This seems to be an intentional cleanup, asLlavaLHTEval(a likely consumer) sources its image processor directly from the model's vision tower. This was assessed as low severity and, per review settings, not commented on directly. - Variable Renaming and Stylistic Updates: The
llmc/models/llava_lht.pyfile includes a variable rename fromself.llava_llama_configtoself.llava_configand the addition of some blank lines for improved readability. These are minor, low-severity changes and were not commented on directly due to review settings. - Comment Update: A comment regarding the
truncate_contextparameter inllmc/models/llava_lht.pywas updated for clarity. This is a low-severity improvement and was not commented on directly due to review settings.
Merge Readiness
The changes related to llava-next support in llmc/models/llava_lht.py appear to be minor and functional. However, the significant alteration in dependency management strategy (moving from pinned to minimum versions) raises concerns about potential impacts on reproducibility and stability. It's recommended to address the questions regarding this dependency change before merging the pull request. As a reviewer, I am not authorized to approve pull requests; this feedback is intended to help improve the changes. Other reviewers should also assess this PR.
support llava-next(only Vicuna)