Fix wandb import crash in trl callbacks (accelerate path)#4148
Conversation
trl/trainer/callbacks.py imports is_wandb_available from accelerate.utils, not from transformers. The original fix in #4147 only patched the transformers version, so `from trl import GRPOTrainer` still crashed via the callbacks.py -> accelerate -> wandb path. Must patch both the source module (accelerate.utils.imports) AND the re-export namespace (accelerate.utils) since Python's `from accelerate.utils import X` reads from the latter, which holds its own cached reference.
Summary of ChangesHello, 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 an issue where Highlights
Changelog
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6680599ecb
ℹ️ 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: | ||
| import accelerate.utils.imports as acc_imports | ||
|
|
||
| acc_imports.is_wandb_available = _wandb_false | ||
| except (ImportError, AttributeError): |
There was a problem hiding this comment.
Catch non-import failures when patching accelerate
In disable_broken_wandb(), the new accelerate patch path only handles ImportError/AttributeError, so any other exception raised while importing accelerate.utils.imports (for example runtime/library mismatch errors during module initialization) will propagate and crash import unsloth in the broken-wandb recovery path. This is a regression from the goal of making imports resilient to broken optional dependencies; broaden this handler (and the similar block below) to tolerate non-import exceptions as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request correctly extends the fix for broken wandb installations by patching accelerate's is_wandb_available function, in addition to the one in transformers, which is necessary to prevent import crashes in trl. The implementation is sound. I've provided one suggestion to refactor the repeated patching logic into a loop, which would improve code clarity and maintainability.
| # Patch transformers' is_wandb_available (used by most trl trainers) | ||
| try: | ||
| import transformers.integrations.integration_utils as tf_integration | ||
|
|
||
| tf_integration.is_wandb_available = lambda: False | ||
| tf_integration.is_wandb_available = _wandb_false | ||
| except (ImportError, AttributeError): | ||
| pass | ||
| # Patch accelerate's is_wandb_available (used by trl/trainer/callbacks.py). | ||
| # Must patch both the source module AND the re-export namespace since | ||
| # `from accelerate.utils import is_wandb_available` reads from | ||
| # accelerate.utils, not accelerate.utils.imports. | ||
| try: | ||
| import accelerate.utils.imports as acc_imports | ||
|
|
||
| acc_imports.is_wandb_available = _wandb_false | ||
| except (ImportError, AttributeError): | ||
| pass | ||
| try: | ||
| import accelerate.utils as acc_utils | ||
|
|
||
| acc_utils.is_wandb_available = _wandb_false | ||
| except (ImportError, AttributeError): | ||
| pass |
There was a problem hiding this comment.
While the current implementation is correct, there's an opportunity to refactor the repeated try...except blocks to improve code clarity and maintainability. By using a loop to iterate over the modules that need patching, you can reduce duplication and make it easier to add more modules in the future.
# Patch all known is_wandb_available functions to prevent import errors.
# We patch both the source module and any re-export namespaces.
modules_to_patch = [
"transformers.integrations.integration_utils", # Used by most trl trainers
"accelerate.utils.imports", # Source for accelerate's check
"accelerate.utils", # Re-export used by trl/trainer/callbacks.py
]
for module_name in modules_to_patch:
try:
module = importlib.import_module(module_name)
module.is_wandb_available = _wandb_false
except (ImportError, AttributeError):
pass…slothai#4148) trl/trainer/callbacks.py imports is_wandb_available from accelerate.utils, not from transformers. The original fix in unslothai#4147 only patched the transformers version, so `from trl import GRPOTrainer` still crashed via the callbacks.py -> accelerate -> wandb path. Must patch both the source module (accelerate.utils.imports) AND the re-export namespace (accelerate.utils) since Python's `from accelerate.utils import X` reads from the latter, which holds its own cached reference.
Summary
disable_broken_wandb()from Fix broken wandb import crashing unsloth startup #4147 to also patchaccelerate.utils.is_wandb_availableaccelerate.utils.importsandaccelerate.utilsnamespace levelsProblem
After #4147,
from unsloth import FastLanguageModelworks with a broken wandb, butfrom trl import GRPOTrainerstill crashes:This is because
callbacks.pyimportsis_wandb_availablefromaccelerate.utils, which is a separate function from thetransformersversion that #4147 patched. The accelerate version does not checkWANDB_DISABLEDand was not covered by the original fix.Additionally, Python's
from accelerate.utils import Xresolves throughaccelerate/utils/__init__.pywhich holds its own cached reference. Patchingaccelerate.utils.imports.is_wandb_availablealone does not affect the re-exported name inaccelerate.utils, so both namespaces must be patched.Test plan
from unsloth import FastLanguageModelsucceedsfrom trl import GRPOConfig, GRPOTrainersucceedsis_wandb_available()returnsTrue,import wandbworks