Skip to content

chore(linter): fixes for rule 16#46023

Open
tarekziade wants to merge 4 commits into
mainfrom
tarek-new-rules
Open

chore(linter): fixes for rule 16#46023
tarekziade wants to merge 4 commits into
mainfrom
tarek-new-rules

Conversation

@tarekziade
Copy link
Copy Markdown
Collaborator

@tarekziade tarekziade commented May 18, 2026

What does this PR do?

Adds support for rules 16

@tarekziade tarekziade requested review from vasqu and zucchini-nlp May 18, 2026 05:16
@tarekziade tarekziade self-assigned this May 18, 2026
@tarekziade
Copy link
Copy Markdown
Collaborator Author

run-slow: albert, align, altclip, autoformer, bit, blt, bridgetower, chinese_clip, clap, clip, clipseg, clvp, colmodernvbert, colpali, colqwen2, conditional_detr

@github-actions
Copy link
Copy Markdown
Contributor

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs:

models: ["models/albert", "models/align", "models/altclip", "models/autoformer", "models/bit", "models/blt", "models/bridgetower", "models/chinese_clip", "models/clap", "models/clip", "models/clipseg", "models/clvp", "models/colmodernvbert", "models/colpali", "models/colqwen2", "models/conditional_detr"]
quantizations: []

Comment thread utils/modular_model_converter.py Outdated
@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

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.

@github-actions
Copy link
Copy Markdown
Contributor

CI Results

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 64943dac workflow commit (merge commit)
PR 15c0ac4f branch commit (from PR)
main ad327c9b base commit (on main)

✅ No failing test specific to this PR 🎉 👏 !

Comment thread src/transformers/models/gemma4/modeling_gemma4.py Outdated
Copy link
Copy Markdown
Contributor

@vasqu vasqu left a comment

Choose a reason for hiding this comment

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

I'm good with the current changes.

It kind of remains how we want to handle TRF 018: Imo, it would make sense to call this in any case - the performance loss is negligible for much cleaner modular paths in the future; any init already covered in the super can be overwritten anyway. But I'm not sure what @zucchini-nlp meant with the weird init patterns because they probably have been removed (by limiting the model scope) --> is there anything that would interfere? Maybe we could even remove some small overrides if they already do what the super call does

Comment thread utils/rules.toml Outdated
Comment thread utils/rules.toml
image_std = [1.0, 1.0, 1.0]
size = None
default_to_square = True
do_convert_rgb = True
Copy link
Copy Markdown
Member

@zucchini-nlp zucchini-nlp May 27, 2026

Choose a reason for hiding this comment

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

why is it being removed btw? i see a few more deletes below for do pad, resize

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that's TRF016 kicking in here, it's not referenced. Maybe we need more edge cases?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hmm indeed, prob they assumed it happens automatically outside prerpcessor, because that is how image processing works. I will pass by processing changes one-by-one to verify it can actually be deleted then

Copy link
Copy Markdown
Member

@zucchini-nlp zucchini-nlp left a comment

Choose a reason for hiding this comment

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

Re init weights thing, i followed the slack thread with Cyril and prob misunderstood that init weights once via super will set the flag on the weight already. Since it can be init correctly even after calling super, I am fine with it

My only q is about deleted do_xxx in some processors. Maybe we can split into 3 PRs for each rule, imo we don't need to PR small subsets of models if we are happy with calling super().init_weights always

@tarekziade tarekziade changed the title chore(linter): fix for rules 16-17-18 chore(linter): fix for rule 16 May 28, 2026
@github-actions
Copy link
Copy Markdown
Contributor

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

run-slow: gemma4, glm46v, glm4v, glmga, kosmos2_5, llama4, mask2former, maskformer, minicpmv4_6, mllama, oneformer, perception_lm, phi4_multimodal, qwen2_vl, qwen3_vl, video_llama_3

@tarekziade
Copy link
Copy Markdown
Collaborator Author

splited out rules 17 and 18 on their own PR

#46260
#46259

@tarekziade
Copy link
Copy Markdown
Collaborator Author

run-slow: gemma4, glm46v, glm4v, glmga, kosmos2_5, llama4, mask2former, maskformer, minicpmv4_6, mllama, oneformer, perception_lm, phi4_multimodal, qwen2_vl, qwen3_vl, video_llama_3

@github-actions
Copy link
Copy Markdown
Contributor

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs:

models: ["models/gemma4", "models/glm46v", "models/glm4v", "models/glmga", "models/kosmos2_5", "models/llama4", "models/mask2former", "models/maskformer", "models/minicpmv4_6", "models/mllama", "models/oneformer", "models/perception_lm", "models/phi4_multimodal", "models/qwen2_vl", "models/qwen3_vl", "models/video_llama_3"]
quantizations: []

@github-actions
Copy link
Copy Markdown
Contributor

CI Results

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN e615adb2 workflow commit (merge commit)
PR 6ed253df branch commit (from PR)
main bc8f70a9 base commit (on main)

⚠️ Model CI failed to report results

The test failure analysis could not be completed. Please check the workflow run for details.

Copy link
Copy Markdown
Member

@zucchini-nlp zucchini-nlp left a comment

Choose a reason for hiding this comment

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

A few comments and lets' revert unrelated commits

+ tie_word_embeddings: bool = True
```

### TRF016
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i suppose the docs contains unrelated changes

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

changes in this file and rules.toml will be removed from the patch. we just want to make the code base rule-16-compatible so we can update mlinter to the next release in another patch

Comment on lines 590 to 594
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unrelated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

also not related

do_resize = True
do_rescale = True
do_normalize = True
do_convert_rgb = True
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seeing all video processors don't use donvert_rgb. I suppose the contribs assumed it gets caught up internally

imo we have to keep the flag and make sure it is actually used. We need to squeeze it in here:

if do_convert_rgb:
    video = self.convert_to_rgb(video)

if device is not None:
video = video.to(device)
processed_videos.append(video)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unrelated as well

image_mean = [0.5, 0.5, 0.5]
image_std = [0.5, 0.5, 0.5]
size = {"height": 336, "width": 336}
do_resize = True
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we TRF ignore it? imo it is meant to always resize and we can actually check if do_resize=False and raise an error that it cannot be False, otherwise the output pixel values cannot be tensorized

do_rescale = True
do_normalize = True
default_to_square = False
do_center_crop = False
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can also delete crop_size = None, same for PIL backend

@tarekziade tarekziade changed the title chore(linter): fix for rule 16 chore(linter): fixes for rule 16 May 29, 2026
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.

5 participants