additional import try except handling for mlx#654
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dea4e16d75
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return getattr(self._resolve(), name) | ||
| try: | ||
| real = self._resolve() | ||
| except ModuleNotFoundError: |
There was a problem hiding this comment.
Restrict ModuleNotFoundError catch to missing mlx only
Catching every ModuleNotFoundError here and converting it to AttributeError masks unrelated import failures inside the real target module (for example, if another dependency is missing or a sub-import path regresses). In those cases, legacy imports like from unsloth_zoo.mlx_loader import ... now fail as a generic missing-attribute error instead of surfacing the true missing module, which makes real breakages hard to diagnose and can hide regressions on MLX-capable environments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request modifies the _LazyMLXAlias.__getattr__ method in unsloth_zoo/__init__.py to handle cases where the mlx submodule cannot be resolved on non-Apple environments. By catching the import failure and raising an AttributeError, the code allows external tools like pickle.whichmodule to skip the stub gracefully. The reviewer suggested catching ImportError instead of ModuleNotFoundError to improve robustness against various loading failures, such as incompatible binaries or missing system dependencies.
| return getattr(self._resolve(), name) | ||
| try: | ||
| real = self._resolve() | ||
| except ModuleNotFoundError: |
There was a problem hiding this comment.
Consider catching ImportError instead of ModuleNotFoundError. While ModuleNotFoundError specifically handles the case where the package is missing, ImportError is more general and will also catch cases where the package is found but fails to load (e.g., due to missing system dependencies or incompatible binaries). This provides better robustness for the goal of allowing pickle.whichmodule to skip the shim on non-supported environments. This also aligns with the error handling patterns used elsewhere in this repository.
| except ModuleNotFoundError: | |
| except ImportError: |
References
- The project's general rules and existing code patterns favor using ImportError for checking the availability of optional or platform-specific dependencies.
There was a problem hiding this comment.
Code Review
This pull request modifies the lazy module resolution in unsloth_zoo/__init__.py to catch ModuleNotFoundError during attribute access, preventing crashes in torch.compile on non-Apple platforms. The reviewer suggests catching the broader ImportError and improving the AttributeError message to include the original exception context, ensuring better clarity for users when resolution fails.
| try: | ||
| real = self._resolve() | ||
| except ModuleNotFoundError: | ||
| # mlx is Apple-only. On non-mlx hosts the real submodule import | ||
| # fails. Surface as AttributeError so callers that walk sys.modules | ||
| # (notably pickle.whichmodule, used by torch._inductor's FX graph | ||
| # hash pickler) skip this stub cleanly instead of crashing the | ||
| # whole compile. Real user attribute access on a non-mlx host | ||
| # still surfaces a useful error -- they will see the AttributeError | ||
| # rather than a torch._dynamo.exc.BackendCompilerFailed wrapper. | ||
| raise AttributeError(name) | ||
| return getattr(real, name) |
There was a problem hiding this comment.
While the logic correctly addresses the torch.compile crash by surfacing an AttributeError for pickle.whichmodule, the current implementation loses significant context for human users. Raising a generic AttributeError(name) results in a message like AttributeError: FastMLXModel, which is unhelpful for a user on a non-Apple host who might be confused as to why the attribute is missing.
I suggest:
- Catching
ImportErrorinstead ofModuleNotFoundErrorto be more robust against various import failures (e.g., corrupted installs or circular dependencies) that should also be skipped bypickle. - Preserving the original error message and using
from eto provide a clear explanation to the user while still satisfying theAttributeErrorrequirement forpickle.
| try: | |
| real = self._resolve() | |
| except ModuleNotFoundError: | |
| # mlx is Apple-only. On non-mlx hosts the real submodule import | |
| # fails. Surface as AttributeError so callers that walk sys.modules | |
| # (notably pickle.whichmodule, used by torch._inductor's FX graph | |
| # hash pickler) skip this stub cleanly instead of crashing the | |
| # whole compile. Real user attribute access on a non-mlx host | |
| # still surfaces a useful error -- they will see the AttributeError | |
| # rather than a torch._dynamo.exc.BackendCompilerFailed wrapper. | |
| raise AttributeError(name) | |
| return getattr(real, name) | |
| try: | |
| real = self._resolve() | |
| except ImportError as e: | |
| # mlx is Apple-only. On non-mlx hosts the real submodule import | |
| # fails. Surface as AttributeError so callers that walk sys.modules | |
| # (notably pickle.whichmodule, used by torch._inductor's FX graph | |
| # hash pickler) skip this stub cleanly instead of crashing the | |
| # whole compile. Real user attribute access on a non-mlx host | |
| # still surfaces a useful error -- they will see the AttributeError | |
| # rather than a torch._dynamo.exc.BackendCompilerFailed wrapper. | |
| raise AttributeError(f"module '{self.__name__}' has no attribute '{name}' (resolution failed: {e})") from e | |
| return getattr(real, name) |
Summary
Fix
ModuleNotFoundError: No module named 'mlx'raised inside anytorch.compile/ inductor compile path on non-Apple hosts. The bug surfaces as
torch._dynamo.exc.BackendCompilerFailed: backend='inductor' raised: ModuleNotFoundError: No module named 'mlx'and breaks training for any modelwhose attention implementation triggers an inductor compile (Qwen3 FFT through
the compiled-cache shim, Gemma3 default through
flex_attention, etc.) onmachines where
mlxis not installed (every Linux/CUDA host).Root cause
unsloth_zoo/__init__.pyregisters six legacy-name shim modules insys.modulesat import time, one perunsloth_zoo.mlx_*legacy name: