Skip to content

Conversation

@Cyrilvallez
Copy link
Member

@Cyrilvallez Cyrilvallez commented Aug 26, 2025

What does this PR do?

A lot of modular files had wrong calls to parent's method (in order to skip unravelling the definition), making then non-pythonic files. This is now fixed, and the converter is much more robust on this.

Here are the new rules to make this process much more pythonic, such that modular files are correct python files:

  • When we want to skip unravelling the parent's code (i.e. we do NOT want to call super), let's call the method from the actual class we would like the method to be used
  • If it's a grand-parent of the inherited class, no need to add the base again in the MRO : e.g., if we inherit from LlamaMLP, and we want to call nn.Module.__init__(...), no need to re-add nn.Module as a new base, as it's already part of the MRO (LlamaMLP is a nn.Module, so it's the grand-parent)
  • the converter will replace such class calls with super() to keep Python's best practices when the class called is one of the direct parents of the generated code

Overall, those rules are much more natural and pythonic, and clear the ambiguity that exists currently.

Also, fix a bug in how we were creating the dependency graphs (some models could be skipped due to non-exhaustive match)

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@Cyrilvallez Cyrilvallez changed the title [modular] Remove ambiguity in all calls to parent class methods [modular] Remove ambiguity in all calls to parent class methods + fix dependency graph Aug 27, 2025
@github-actions
Copy link
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: aimv2, aria, colqwen2, d_fine, deepseek_v2, deepseek_v3, deepseek_vl_hybrid, diffllama, doge, dpt, eomt, ernie4_5_moe, evolla, gemma3, gemma3n, glm4v

for node in ast.walk(tree):
if isinstance(node, (ast.Import, ast.ImportFrom)):
module = node.module if isinstance(node, ast.ImportFrom) else None
if module and (".modeling_" in module or "transformers.models" in module):
Copy link
Member Author

Choose a reason for hiding this comment

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

This was not exhaustive enough so some models could be skipped from dependencies if we import simply their image_processor or such

@Cyrilvallez Cyrilvallez merged commit 8b80431 into main Aug 27, 2025
20 checks passed
@Cyrilvallez Cyrilvallez deleted the fix-modular-super branch August 27, 2025 12:51
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.

4 participants