-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Nightly #3720
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
Nightly #3720
Conversation
for more information, see https://pre-commit.ci
* Fix get_input_embeds call for VLMs * patch input_require_grads instead * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * cleanup old patch * cleanup old patch * cleanup * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Apply suggestion from @danielhanchen * use logger instead of prints * Move unsloth present set * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com>
for more information, see https://pre-commit.ci
Summary of ChangesHello @danielhanchen, 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 primarily focuses on maintenance, compatibility improvements, and internal refactoring. It updates a core dependency, introduces new patches to enhance compatibility with vision models and TRL's OpenEnv module, and streamlines the logging infrastructure. Additionally, it includes a version bump and a minor adjustment to the trainer's padding-free collator logic under specific conditions. 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.
💡 Codex Review
Lines 73 to 77 in b5b57b3
| # Check for unsloth_zoo | |
| try: | |
| unsloth_zoo_version = importlib_version("unsloth_zoo") | |
| if Version(unsloth_zoo_version) < Version("2025.12.3"): | |
| print( |
The runtime version gate still allows unsloth_zoo 2025.12.3, but this commit both bumps the packaging requirement to unsloth_zoo>=2025.12.4 and starts importing new symbols such as unsloth_zoo.utils.Version (e.g., in trainer.py). In environments where an older 2025.12.3 install is already present or installed with --no-deps, the check here will pass even though the new imports will raise ImportError, preventing unsloth from loading. The minimum version enforced at import should be raised to the new floor.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 several updates and fixes, including a version bump for unsloth_zoo, refactoring of initialization code, and new patches for transformers and trl compatibility. The logging has been improved by replacing print statements with a proper logger.
My review focuses on improving code style and robustness. I've pointed out a few places where bare except blocks are used, which can be risky. I've also suggested a minor cleanup of redundant parentheses.
Overall, the changes are positive and improve the library's stability and maintainability.
| import inspect | ||
| from transformers import PreTrainedModel | ||
|
|
||
| # Check if the original function iterates over self.modules() instead of just returning the enable_input_require_grads |
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.
Using a bare except: is generally discouraged as it can catch unexpected exceptions like SystemExit or KeyboardInterrupt, making it harder to debug and interrupt the program. It's better to catch specific exceptions, or Exception at the very least.
| # Check if the original function iterates over self.modules() instead of just returning the enable_input_require_grads | |
| except Exception: |
| if importlib.util.find_spec("trl") is None: | ||
| return | ||
| trl_location = importlib.util.find_spec("trl").origin | ||
| trl_location = os.path.split(trl_location)[0] |
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.
|
|
||
| diffusers_logger.addFilter(HideLoggingMessage("are deprecated")) | ||
| del diffusers_logger | ||
| except: |
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.
| # We also disable vision language models for padding free collators | ||
| blocked = ( | ||
| data_collator is not None | ||
| (data_collator is not None) |
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.
No description provided.