Skip to content

Comments

fix: multiple bug fixes (#390, #311, #316)#405

Open
yurekami wants to merge 1 commit intounslothai:mainfrom
yurekami:fix/multiple-issues-batch-2
Open

fix: multiple bug fixes (#390, #311, #316)#405
yurekami wants to merge 1 commit intounslothai:mainfrom
yurekami:fix/multiple-issues-batch-2

Conversation

@yurekami
Copy link
Contributor

Summary

Changes

  1. llama_cpp.py: Pass system_type argument to do_we_need_sudo() call
  2. patching_utils.py: Refactor import handling with proper namespace and error handling
  3. vision_utils.py: Normalize tuple patch sizes to single integer

Test plan

  • Verify do_we_need_sudo() works correctly with different Linux distros
  • Verify bitsandbytes/peft patching works without errors when peft is not installed
  • Verify InternVL3-2B and other models with tuple patch sizes work correctly

Fixes #390, #311, #316

🤖 Generated with Claude Code

- Fix unslothai#390: Pass system_type to do_we_need_sudo() for proper package manager detection on non-Debian systems
- Fix unslothai#311: Use namespace dictionary for exec/eval in patch_compiling_bitsandbytes to properly access imported modules, add ImportError handling for optional peft dependency
- Fix unslothai#316: Handle tuple patch sizes (e.g., (14, 14) for InternVL3 models) in UnslothVisionDataCollator

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @yurekami, 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 addresses three distinct bugs to enhance the stability and compatibility of the system. It improves the detection of system utilities across different Linux distributions, makes module patching more robust by handling optional dependencies and using safer execution contexts, and extends vision model support by correctly interpreting varied patch size formats. These changes collectively contribute to a more reliable and versatile application.

Highlights

  • Improved System Utility Detection: The do_we_need_sudo() function now correctly receives the system_type argument, ensuring proper package manager detection on non-Debian systems like Fedora, which use yum or dnf.
  • Robust Module Patching: The patch_compiling_bitsandbytes function has been refactored to use a dedicated namespace dictionary for exec/eval operations, preventing unintended global scope modifications. It also gracefully handles ImportError for the optional peft dependency, making the patching more resilient.
  • Enhanced Vision Model Compatibility: The UnslothVisionDataCollator now correctly processes patch_size values that are provided as tuples or lists (e.g., (14, 14) for InternVL3 models) by extracting the first element, ensuring broader compatibility with various vision models.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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

  1. 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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 important bug fixes that enhance compatibility and robustness. The changes correctly pass system_type for broader Linux distribution support, refactor module patching to be safer and more robust by using a dedicated namespace and handling ImportError, and add support for tuple-based patch sizes in the vision data collator. The fixes are well-implemented. I've added a couple of suggestions to improve exception handling by replacing bare except clauses with more specific ones, which will improve code clarity and prevent unintended side effects.

for fx in layers:
try: layer = eval(f"{x}.{fx}")
try: layer = getattr(module, fx)
except: continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a bare except is generally discouraged as it can catch unexpected errors, including SystemExit and KeyboardInterrupt, making the program harder to debug and interrupt. Per PEP 8, it's better to catch specific exceptions. In this case, since accessing attributes from dir() can sometimes raise various exceptions if they are computed properties, catching Exception would be a safer and more explicit choice.

Suggested change
except: continue
except Exception: continue

if isinstance(patch_size, (tuple, list)):
patch_size = patch_size[0]
self.patch_size = patch_size
except:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of a bare except is not ideal as it can hide unexpected errors and goes against PEP 8 guidelines. The code in the try block is likely to raise AttributeError if model.config.vision_config.patch_size does not exist, or IndexError if patch_size is an empty list or tuple. Specifying these exceptions makes the code more robust and easier to understand.

Suggested change
except:
except (AttributeError, IndexError):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing argument to do_we_need_sudo UnslothVisionDataCollator cannot handle a tuple as patch size 'peft' is not defined in patching_utils.py

1 participant