chore(mlinter): implement part of rule TRF003 - #45720
Conversation
|
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. |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: bloom, data2vec, deberta, deberta_v2, falcon, hubert, longformer, longt5, mbart, mt5, sew, sew_d, t5, umt5, unispeech, unispeech_sat |
|
View the CircleCI Test Summary for this PR: https://huggingface.co/spaces/transformers-community/circle-ci-viz?pr=45720&sha=cdd535 |
| @can_return_tuple | ||
| @auto_docstring | ||
| def forward( |
There was a problem hiding this comment.
not really sure this is what we want to enforce with linter. For base models we prefer capture_outputs which covers "collecting intermediate_outputs + return in tuple/dict format"
There was a problem hiding this comment.
Tho updating all models for @capture_outputs will be a long PR, and prob needs to be delegated to a smarter variant of code agents
There was a problem hiding this comment.
the change was generated by an agent using the mlinter rule as:
https://github.com/huggingface/transformers-mlinter/blob/main/mlinter/rules.toml#L31-L48
and I restricted it to 10 models only to avoid a large PR.
do you see something in that rule we should tweak to make sure capture_outputs takes precedence?
There was a problem hiding this comment.
yeah, this model for ex (BloomModel), should have capture_outputs. But as said, it prob needs human intervention at some point, because we have to add correct classes in cls._can_record_ouputs and change returns from some modules
There was a problem hiding this comment.
so what is your recommandation? drop rule 13 altogether?
There was a problem hiding this comment.
no, no, we defi need a rule for these decorators. Can we check with new models that they don't use explicitly output_xx and don't check old models yet?
I think for old models we need a separate PR updating all of them to use correct decorators
There was a problem hiding this comment.
Also, I don't know if it is possible, but enforcing a better choice of decorator would be great. For ex, base models need to use capture_outputs while the task-head models needs a can_return_tuple
The bad choice will return incorrect last_hidden_states and was already reported once with qwen3-vl
There was a problem hiding this comment.
Yea, it's pretty hard due to old legacy models. The pattern usually is
can_return_tuple+auto_docstringon a wrapper model, mostly the ForXXX onesmerge_with_config_defaults+capture_outputs+auto_docstringfor the base models underlying
It's not trivial, just take a look at #43590 😅
vasqu
left a comment
There was a problem hiding this comment.
I only went through the list that would need another round. I just wanted to comment on those I think that are important / recent
Not sure whether it makes sense in this PR directly tbh. Wdyt @zucchini-nlp to get this in and progressively refactor those we still need? I think we should also deprecate a bunch of these models either way tbh 👀
| # longt5, mbart, mt5, t5, umt5, wav2vec2, and xlnet; the remaining ones are tracked here. | ||
| allowlist_models = [ | ||
| "bark", "beit", "bit", "blip_2", "canine", "codegen", "cpmant", "ctrl", "cvt", | ||
| "dab_detr", "dac", "data2vec", "decision_transformer", "depth_anything", "depth_pro", |
There was a problem hiding this comment.
dac is bound to dia so would be nice to fix here
| "dinat", "donut", "dpr", "efficientnet", "encodec", "falcon_mamba", | ||
| "fastspeech2_conformer", "flaubert", "flava", "fnet", "focalnet", "fsmt", "funnel", | ||
| "glpn", "gpt_neo", "gpt_neox_japanese", "gptj", "granite_speech", | ||
| "granite_speech_plus", "grounding_dino", "hgnet_v2", "hiera", |
There was a problem hiding this comment.
granite speech plus is very recent so should be fixed
| "fastspeech2_conformer", "flaubert", "flava", "fnet", "focalnet", "fsmt", "funnel", | ||
| "glpn", "gpt_neo", "gpt_neox_japanese", "gptj", "granite_speech", | ||
| "granite_speech_plus", "grounding_dino", "hgnet_v2", "hiera", | ||
| "higgs_audio_v2_tokenizer", "hubert", "ibert", "imagegpt", "kyutai_speech_to_text", |
| "glpn", "gpt_neo", "gpt_neox_japanese", "gptj", "granite_speech", | ||
| "granite_speech_plus", "grounding_dino", "hgnet_v2", "hiera", | ||
| "higgs_audio_v2_tokenizer", "hubert", "ibert", "imagegpt", "kyutai_speech_to_text", | ||
| "led", "levit", "lilt", "llama4", "luke", "lxmert", "mamba", "mamba2", |
There was a problem hiding this comment.
llama4 ig, maybe the mamba models since they are base model references for other models
| "granite_speech_plus", "grounding_dino", "hgnet_v2", "hiera", | ||
| "higgs_audio_v2_tokenizer", "hubert", "ibert", "imagegpt", "kyutai_speech_to_text", | ||
| "led", "levit", "lilt", "llama4", "luke", "lxmert", "mamba", "mamba2", | ||
| "mask2former", "maskformer", "megatron_bert", "mgp_str", "mimi", "mm_grounding_dino", |
| "higgs_audio_v2_tokenizer", "hubert", "ibert", "imagegpt", "kyutai_speech_to_text", | ||
| "led", "levit", "lilt", "llama4", "luke", "lxmert", "mamba", "mamba2", | ||
| "mask2former", "maskformer", "megatron_bert", "mgp_str", "mimi", "mm_grounding_dino", | ||
| "mobilenet_v1", "mobilenet_v2", "mobilevit", "mobilevitv2", "moshi", "mpnet", "mpt", |
|
Yes, we do need this rule for sure. My point is to not just update with I am really bad at prompting but i think a capable agent like claude-code can do well if prompted correctly. It is a waste of engineering resources for us to update everything by hand 😭 |
|
I think this one is simple enough for now where can return tuple works well for most edge cases on the rule. I think the models you think about will need a deeper restructure either way, just to have the proper wrapper patterns - not sure how well claude would manage nowadays :D |
|
So the goal with this PR is to collect the low hanging fruits and then adapt progressively |
What does this PR do?
Enables rule TRF003 and implement it on 10 models