Remove stale and redundant _no_split_modules entries - #47645
Conversation
Some models list `_no_split_modules` entries naming classes that do not exist
anywhere in transformers, so `device_map` never matches them and the modules
they were meant to keep together get split across devices:
- idefics2: Idefics2DecoderLayer
- idefics3: Idefics3DecoderLayer
- video_llava: VideoLlavaVisionAttention
Others hardcode classes owned by a submodel that is loaded through the Auto*
classes. `post_init` already collects `_no_split_modules` from child submodels,
so these entries are redundant, and in blip_2's case actively wrong: it declared
both T5Block and OPTDecoderLayer regardless of which language model the
checkpoint actually uses.
- blip_2: T5Block, OPTDecoderLayer
- depth_anything: DPTViTEmbeddings (the backbone is an AutoBackbone, so this
never matched; it now correctly reports the backbone's Dinov2Layer)
- llava_next: LlamaDecoderLayer
- moshi: MimiTransformerLayer
The resulting runtime sets are more accurate than the hardcoded lists were.
Generated modeling files updated with the modular converter, which propagates
the removals to smolvlm, llava_next_video, llava_onevision,
kyutai_speech_to_text and prompt_depth_anything. deepseek_ocr2 and
granite4_vision only shift the position of their own override.
|
run-slow: blip_2, deepseek_ocr2, depth_anything, granite4_vision, idefics2, idefics3, kyutai_speech_to_text, llava_next, llava_next_video, llava_onevision, moshi, prompt_depth_anything, smolvlm, video_llava |
|
This comment contains models: ["models/blip_2", "models/deepseek_ocr2", "models/depth_anything", "models/granite4_vision", "models/idefics2", "models/idefics3", "models/kyutai_speech_to_text", "models/llava_next", "models/llava_next_video", "models/llava_onevision", "models/moshi", "models/prompt_depth_anything", "models/smolvlm", "models/video_llava"] |
|
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. |
…n/transformers into fix-stale-no-split-modules
molbap
left a comment
There was a problem hiding this comment.
🧹 🧹
Nice and clean! Happy for the linter too 🎉
There was a problem hiding this comment.
I think above this one, cohere_asr also has a CohereEncoderLayer that does not exist, but maybe more are like this, your linter rule will help!
There was a problem hiding this comment.
I think this is the loophole that still exists, see Note in the PR description
There was a problem hiding this comment.
Hmm, I can't really follow tbh. Shouldn't the linter follow the modeling files? Or is there an conflict of order of applications re fix/check repo?
There was a problem hiding this comment.
ah indeed, same as you described, I think it's fixable in modular_model_converter (and should be): we could either include it as a regex str match in convert_modular_file , just would need re.findall(r"_no_split_modules = \ somewhere and would not cost much
There was a problem hiding this comment.
Hmm, I can't really follow tbh. Shouldn't the linter follow the modeling files? Or is there an conflict of order of applications re fix/check repo?
The issue is that the linter doesn't check modeling files that are generated. Agree with @molbap that this should be fixed in modular converter.
There was a problem hiding this comment.
The issue is that the linter doesn't check modeling files that are generated
That kind of seems like a self contained issue in itself? I would expect modeling to be checked as well. But yea anyways, agree that it could be done at modular level as well (I just find it a bit less intuitive)
There was a problem hiding this comment.
Yes I was surprised to learn that the linter doesn't check the auto-generated files. The motivation seems to be that errors should be reported in the modular files instead. But in this case here it would definitely also make sense to check the modeling file. Will ask Tarek once he is back.
vasqu
left a comment
There was a problem hiding this comment.
Some small questions but nothing major, thanks 🫡 maybe also run slow ci for some whom that should fix?
There was a problem hiding this comment.
Hmm, I can't really follow tbh. Shouldn't the linter follow the modeling files? Or is there an conflict of order of applications re fix/check repo?
|
run-slow: blip_2, data2vec, deepseek_ocr2, deepseek_vl, deepseek_vl_hybrid, depth_anything, diffusion_gemma, exaone4_5, gemma3, granite4_vision, higgs_audio_v2_tokenizer, idefics2, idefics3, janus, kyutai_speech_to_text, llava_next |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: blip_2, dac, data2vec, deepseek_ocr2, deepseek_vl, deepseek_vl_hybrid, depth_anything, diffusion_gemma, exaone4_5, gemma3, granite4_vision, higgs_audio_v2_tokenizer, idefics2, idefics3, janus, kyutai_speech_to_text |
|
This comment contains models: ["models/blip_2", "models/data2vec", "models/deepseek_ocr2", "models/deepseek_vl", "models/deepseek_vl_hybrid", "models/depth_anything", "models/diffusion_gemma", "models/exaone4_5", "models/gemma3", "models/granite4_vision", "models/higgs_audio_v2_tokenizer", "models/idefics2", "models/idefics3", "models/janus", "models/kyutai_speech_to_text", "models/llava_next"] |
CI recapDashboard: View test results in Grafana |
TRF005 only validates the shape of `_no_split_modules`. TRF022 additionally checks that each entry names a class that actually exists, resolving it against the classes defined in the modeling file, the names it imports, and the classes defined by sibling modules of the same model directory. `device_map` matches these strings against `module.__class__.__name__` at runtime, so a stale or misspelled entry is silently ignored. Entries naming another model's classes are flagged too: `post_init` already collects `_no_split_modules` from child submodels, so hardcoding them is redundant. Corresponding transformers PR to make checks pass: huggingface/transformers#47645 --------- Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
This PR removes or updates
_no_split_modulesentires with the addition of a new mlinter rule: huggingface/transformers-mlinter#12The new rule verifies that
_no_split_modulescontains only entries from modules defined inmodeling_*.py,modular_*.pyor a file in the same model directory.This helps avoiding wrong
_no_split_modulesentries. It also simplifies reviews.Updates can be grouped into these cases:
Idefics2DecoderLayer)Data2VecTextForTextEmbeddings->Data2VecTextEmbeddings)_no_split_modulesof submodule (e.g.T5Blockinblip_2)_no_split_modulesto avoid inheriting entry for parent class (e.g. inDeepseekVLPreTrainedModelto avoid inheritingJanusVisionEncoderLayer)There are some special cases that are automatically handled by the linter:
Parametrized*classes created throughtorch.nn.utils.parametrizeare ignored as they are only created at runtimeTimmWrapperForImageClassificationis ignoredNote
There are still some blind spots for the linter. As the linter only checks non-autogenerated files, it can miss cases where a modular file doesn't define
_no_split_modulesand the generatedmodeling.pyfile inherits a wrong name from the parent class. For exampleAriaPreTrainedModeldoesn't have_no_split_modules:transformers/src/transformers/models/aria/modular_aria.py
Lines 791 to 795 in 71c6f69
which gets automatically extended to this in
modeling_aria.py:transformers/src/transformers/models/aria/modeling_aria.py
Lines 597 to 601 in 71c6f69
where
AriaDecoderLayeris automatically renamed fromLlamaDecoderLayerbut there is noAriaDecoderLayer.Code Agent Policy
The Transformers repo is currently being overwhelmed by a large number of PRs and issue comments written by
code agents. These often are low-quality, or fix extremely minor issues that occur rarely or never in practice.
As a result, we're instituting a rule that first-time contributors should not use code agents to submit PRs or issues.
We'd also ask autonomous "OpenClaw"-like agents not to open any PRs or issues.
Issues/PRs from first-time contributors that violate this rule will probably just be closed without review, and we
might block you, especially if you open more than one or appear to be deliberately ignoring this. We especially do not
want new contributors to jump in on random issues to contribute an agent-written fix. This creates lots of noise
for reviewers and other users and will almost certainly get you blocked.
For more information, please read
CONTRIBUTING.md.Before submitting
Pull Request checks?
to it if that's the case.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.