fix(compiler): unblock all model_types across transformers 4.57.6 and 5.x#632
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves the compiler's reliability by adding guards for empty source strings, refining regex-based code transformations to preserve indentation, and handling complex return statements. It also introduces error handling for dynamic attribute access and ensures correct topological sorting of functions. The review feedback focuses on using getattr instead of eval for safer attribute lookups and removing a redundant import of textwrap inside a loop.
| try: | ||
| module = eval(f"modeling_file.{module}") | ||
| except AttributeError: | ||
| return None |
There was a problem hiding this comment.
Using getattr is safer and more idiomatic than eval for attribute access. Since modeling_file is already the module object, you can use getattr(modeling_file, module, None) to handle cases where the attribute might be missing.
| try: | |
| module = eval(f"modeling_file.{module}") | |
| except AttributeError: | |
| return None | |
| module = getattr(modeling_file, module, None) | |
| if module is None: | |
| return None |
| try: | ||
| inner_class = eval(f"modeling_file.{inner_class}") | ||
| except AttributeError: | ||
| continue |
There was a problem hiding this comment.
| try: | ||
| source = eval(f"{model_location}.{module}") | ||
| except AttributeError: | ||
| continue |
There was a problem hiding this comment.
Since modeling_file is the module object corresponding to model_location, you can use getattr(modeling_file, module, None) instead of eval to safely access the attribute.
| try: | |
| source = eval(f"{model_location}.{module}") | |
| except AttributeError: | |
| continue | |
| source = getattr(modeling_file, module, None) | |
| if source is None: | |
| continue |
| try: | ||
| source = eval(f"{model_location}.{module}") | ||
| except AttributeError: | ||
| continue |
| try: | ||
| source = eval(f"{model_location}.{module}") | ||
| except AttributeError: | ||
| continue |
| try: | ||
| source = eval(f"{model_location}.{module}") | ||
| except AttributeError: | ||
| continue |
There was a problem hiding this comment.
| try: | ||
| module_cls = eval(f"{model_location}.{module}") | ||
| except AttributeError: | ||
| continue |
| try: | ||
| module_cls = eval(f"{model_location}.{module}") | ||
| except AttributeError: | ||
| continue |
| if source and source[0] in (" ", "\t"): | ||
| import textwrap as _tw | ||
| source = _tw.dedent(source) |
There was a problem hiding this comment.
textwrap is already imported at the top of the file (line 40). Re-importing it inside a loop is unnecessary and inefficient.
| if source and source[0] in (" ", "\t"): | |
| import textwrap as _tw | |
| source = _tw.dedent(source) | |
| if source and source[0] in (" ", "\t"): | |
| source = textwrap.dedent(source) |
… 5.x
unsloth_compile_transformers() now compiles every transformers
model_type that has a modeling_<x>.py file:
transformers 4.57.6: 359 ok / 359 (was 28 broken)
transformers 5.8.0: 439 ok / 439 (was 5 broken on top of those 28)
total broken: 0
Production models (llama, qwen3, gemma3, mistral, mixtral,
idefics{,2,3}, gpt_oss, deepseek_v3, ...) keep working unchanged on
both transformers versions.
Eight narrowly-scoped fixes:
1. create_new_function: empty new_source[0] IndexError.
Guard `if new_source and new_source[0] == " "`.
Fixes (tf4.57.6): colpali, colqwen2, colmodernvbert, dpr,
gemma4_assistant, rag, shieldgemma2, timm_backbone (8).
2. convert_attention_masks_to_bool: nested-paren regex mis-parse.
Skip when return body contains `(`. Fixes (tf4.57.6): kosmos2,
kosmos2_5 (2).
3. eval(f"{model_location}.{module}") AttributeError on dir() names.
Wrap 6 eval() sites with try/except AttributeError: continue.
Fixes (tf4.57.6): auto, bit, regnet, resnet (4).
4. Idefics-style "loss = loss_fct(...)" rewrite: substring + dedent
double-corruption. Replace flat .replace() with re.sub anchored to
`^([ \t]+)loss = ...$` that captures and inherits the leading
whitespace. Fixes (tf4.57.6): gpt2 + sets up Cat-B chain.
5. called_functions sig-rebuild fallback dropped trailing `:`.
Restore `:` and `\n` so the body lands as a proper indented suite.
Fixes (tf4.57.6): electra, tapas (2).
6. Decorator prepended without dedenting indented source.
textwrap.dedent before adding @torch.compile. Fixes (tf4.57.6):
xlstm + (combined w/ #4) clvp, falcon_mamba, imagegpt, mamba (5).
7. Topological ordering: full_source.find(name) hit forward refs.
Switch to `find("class N(")` / `find("class N:")` / `find("def N(")`.
Fixes (tf4.57.6): perceiver, sam3_lite_text + unblocks
audioflamingo3, musicflamingo, voxtral, voxtral_realtime (4).
8. @auto_docstring (and 4 sibling decorators) used `[^)]*` to match
their argument, which stops at the first `)` inside a string
like `"... (a log mel spectrogram), meaning ..."`. Switch to a
`regex` recursive group that respects nested parens AND nested
string literals. Fixes (tf5.x): voxtral, voxtral_realtime,
audioflamingo3, musicflamingo (4 of the 5 tf5-only failures).
9. Base-class items lookup: `class X(Y, ...)` where Y has only an
inherited forward (e.g. Sam3LiteTextLayerScaledResidual which is
used purely as a base) was filtered out of `functions`, then
excluded from the auto-import list, so subclasses raised NameError
at cache import. Detect base-class names from `class N(...)` in
the emitted source and add them to `items` if they exist on the
modeling file but aren't defined in the cache. Fixes (tf5.x):
sam3_lite_text.
All fixes either widen try/except, anchor a substitution to
respect indentation, switch to a strictly-more-correct sort key, or
detect-and-import a previously-missed reference. Verified on a full
sweep of every transformers.models.* package: 0 broken on both
transformers 4.57.6 and 5.8.0.
b77cd34 to
aa491f6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a572d2b5a
ℹ️ 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".
| # Strip decorators with a paren-balanced match. A `[^\)]*` group | ||
| # stops at the first `)` inside a string argument and leaves an | ||
| # unterminated literal in the emitted source. | ||
| _PAREN_GROUP = r"(?P<grp>\((?:[^()'\"]|'[^'\\]*(?:\\.[^'\\]*)*'|\"[^\"\\]*(?:\\.[^\"\\]*)*\"|(?P>grp))*\))" |
There was a problem hiding this comment.
Handle triple-quoted decorator strings atomically
When compiling a Transformers model whose @auto_docstring(...) argument uses a triple-quoted string containing an unescaped quote before a parenthesized ) (for example the Lxmert custom_intro in Transformers 5.x), this pattern treats the inner " as the end of a normal string and closes the recursive paren match too early. The substitution then leaves the tail of the decorator in the emitted cache file, producing invalid Python instead of stripping the decorator, so these model types still fail to compile.
Useful? React with 👍 / 👎.
…s merged PR unslothai/unsloth-zoo#627 (GGUF NotImplementedError + LoRA local_path fixes) landed on unsloth-zoo main as e9d1be8. Drop the temporary branch pin and revert to bare `unsloth_zoo @ git+...` so subsequent runs pick up further main changes. PR unslothai/unsloth-zoo#632 (compiler unblock for transformers 4.57.6 and 5.x) also merged (232d950); consolidated-tests-ci.yml already follows main via UNSLOTH_ZOO_REF default, so no change there.
Summary
unsloth_compile_transformers()now compiles every transformers model_type that has amodeling_<x>.pyfile, on both transformers 4.57.6 and 5.x:Production models (llama, qwen3, gemma3, mistral, mixtral, idefics{,2,3}, gpt_oss, deepseek_v3, ...) keep working unchanged on both versions.
Models unblocked
IndexError: string index out of rangeunexpected indent/expected ':'unterminated string literalmodule has no attribute _BaseModelWithGenerate/Linearname 'AbstractPreprocessor' is not definedNine narrowly-scoped fixes
create_new_function: emptynew_source[0]IndexError. Guard withif new_source and new_source[0] == " ".convert_attention_masks_to_bool: nested-paren regex mis-parse. Thefindallregex doesn't handle nested parens likereturn inverted_mask.masked_fill(inverted_mask.to(torch.bool), torch.finfo(dtype).min). Skip when the return body contains(.eval(f"{loc}.{module}")AttributeError ondir()-derived names. Wrap 6 eval() sites withtry/except AttributeError: continue(private names in__all__like_BaseModelWithGenerate, ornn.Linearaliases that fail attribute lookup on some transformers versions).Idefics-style
loss = loss_fct(...)rewrite: substring + dedent double-corruption. The flatforward.replace(...)matched insidelm_loss = loss_fct(...)and inserted unindented lines that the downstream dedent then chopped 4 chars off. Replace withre.subanchored to^([ \t]+)loss = ...$that captures and inherits the leading whitespace.called_functionssig-rebuild fallback dropped trailing:. Wheninspect.signaturerepr disagrees with source quote style or fully-qualified annotation, the fallback consumed the colon. Restore:and\nso the body lands as a proper indented suite.Decorator prepended without dedenting indented source.
textwrap.dedentbefore adding the@torch.compile(...)decorator (xLSTMsoft_capis defined under anelse:soinspect.getsourcereturns it indented 4 chars).Topological ordering by
full_source.find(name)used the wrong match. Bare-name find hits forward refs in docstrings / type annotations /Union[..., NAME]before the actualclass NAMEdefinition. Switch tofind("class N(") / find("class N:") / find("def N(").@auto_docstring(and 4 sibling decorators) used[^)]*to match the argument list, which stops at the first)inside a string like"... (a log mel spectrogram), meaning ..."and leaves an unterminated string literal in the cache. Switch to aregexrecursive group that respects nested parens AND nested string literals (single and triple-quoted).Base-class import missing for classes used purely as bases.
Sam3LiteTextLayerScaledResidualhas only an inheritedforwardso it's filtered out of the per-module compile and out offunctions, butclass Sam3LiteTextRepMixer(Sam3LiteTextLayerScaledResidual)references it. Scanclass N(B1, B2, ...)patterns in the emitted source and add bases that exist on the modeling file but aren't defined-in-cache to the auto-import list.All fixes either widen
try/except, anchor a substitution to respect indentation, switch to a strictly-more-correct sort key, or detect-and-import a previously-missed reference. None of them change semantics for working models.Test plan
loss=loss_fctrewrite) -> 3/3 ok; synthetic equivalence test confirms the newre.subproduces a properly-indented expansion vs the old.replace()'s broken zero-indent emit.convert_attention_masks_to_bool-> simple-return rewrite still fires (Case A), tuple-of-bare-names still rewrites (Case C), nested-paren returns are correctly skipped (Case B).