Conversation
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 significantly improves the compatibility of Unsloth-Zoo workflows, particularly for AMD ROCm environments, by making the Highlights
🧠 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. Changelog
Activity
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
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b84f897dcc
ℹ️ 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".
| try: | ||
| exec(f"import {item}", locals(), globals()) | ||
| except Exception: | ||
| continue |
There was a problem hiding this comment.
Catching all exceptions here means any failure inside a LoRA module (e.g., a real bug or a missing dependency unrelated to bitsandbytes) is silently ignored, so its Linear* classes are never added to Linear_LoRA_Layers. In that scenario, models using that module type won’t be patched and LoRA layers may be missed without any warning, leading to incomplete training behavior that is hard to diagnose. If the intent is only to ignore optional backends, consider narrowing the exception to the specific missing dependency or at least logging unexpected import failures.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes bitsandbytes an optional dependency, which is a great improvement for environments like ROCm that may not have it installed. The changes primarily involve adding try-except blocks to handle import errors gracefully. My review focuses on improving the robustness of this error handling by suggesting more specific exceptions (ImportError instead of Exception) to avoid masking other potential issues. Additionally, I've proposed a refactoring in peft_utils.py to replace the use of exec() with the safer importlib module for dynamic imports, which enhances code maintainability and security.
| try: | ||
| exec(f"import {item}", locals(), globals()) | ||
| except Exception: | ||
| continue | ||
| modules = dir(eval(item)) | ||
| modules = [x for x in modules if x.startswith("Linear") or x.endswith("Linear")] | ||
| if len(modules) == 0: continue | ||
| exec(f"from {item} import ({', '.join(modules)})", locals(), globals()) | ||
| try: | ||
| exec(f"from {item} import ({', '.join(modules)})", locals(), globals()) | ||
| except Exception: | ||
| continue | ||
| Linear_LoRA_Layers += [(eval(x), item, x,) for x in modules] |
There was a problem hiding this comment.
Using exec() and eval() for dynamic imports can introduce security vulnerabilities and make the code harder to debug and maintain. A safer and cleaner approach is to use the importlib module. This refactoring will make the code more robust and readable.
Please also add import importlib at the top of the file.
| try: | |
| exec(f"import {item}", locals(), globals()) | |
| except Exception: | |
| continue | |
| modules = dir(eval(item)) | |
| modules = [x for x in modules if x.startswith("Linear") or x.endswith("Linear")] | |
| if len(modules) == 0: continue | |
| exec(f"from {item} import ({', '.join(modules)})", locals(), globals()) | |
| try: | |
| exec(f"from {item} import ({', '.join(modules)})", locals(), globals()) | |
| except Exception: | |
| continue | |
| Linear_LoRA_Layers += [(eval(x), item, x,) for x in modules] | |
| try: | |
| module_obj = importlib.import_module(item) | |
| except ImportError: | |
| # This can happen if bitsandbytes is not installed for modules like bnb.py | |
| continue | |
| module_names = [ | |
| x for x in dir(module_obj) | |
| if (x.startswith("Linear") or x.endswith("Linear")) and isinstance(getattr(module_obj, x, None), type) | |
| ] | |
| for module_name in module_names: | |
| layer_class = getattr(module_obj, module_name) | |
| Linear_LoRA_Layers.append((layer_class, item, module_name)) |
| import bitsandbytes | ||
| try: | ||
| import bitsandbytes | ||
| except Exception: |
There was a problem hiding this comment.
It's a best practice to catch specific exceptions rather than the general Exception. In this case, an ImportError is expected if bitsandbytes is not installed. Using except ImportError: is more precise and prevents masking other potential errors during the import process.
| except Exception: | |
| except ImportError: |
| from bitsandbytes.nn import Linear4bit as Bnb_Linear4bit | ||
| except: | ||
| raise ImportError("Unsloth: Please install bitsandbytes via `pip install bitsandbytes`") | ||
| except Exception: |
There was a problem hiding this comment.
To make the code more robust, it's better to catch a more specific exception. Since you're handling a potentially missing optional dependency, ImportError is the more appropriate exception to catch here instead of the broad Exception.
| except Exception: | |
| except ImportError: |
| from peft.tuners.lora import Linear4bit as Peft_Linear4bit | ||
| except: | ||
| raise ImportError("Unsloth: Please install peft via `pip install peft`") | ||
| except Exception: |
There was a problem hiding this comment.
Similar to the previous comment, catching a specific ImportError is preferred over a general Exception. This ensures that you are only handling the case where the module is not found and not suppressing other unexpected errors.
| except Exception: | |
| except ImportError: |
| import bitsandbytes as bnb | ||
| try: | ||
| import bitsandbytes as bnb | ||
| except Exception: |
There was a problem hiding this comment.
It's better to catch specific exceptions. Here, ImportError is the expected exception if bitsandbytes is not installed. Using except ImportError: is safer as it won't hide other unexpected errors that might occur during the import.
| except Exception: | |
| except ImportError: |
1 participant
This makes bitsandbytes an optional dependency so ROCm environments without bitsandbytes can still run Unsloth-Zoo workflows (16bit / bf16) end to end.
Changes:
This is NVIDIA-safe: when bitsandbytes is installed, behavior is unchanged.