Skip to content

[Fix] Wrong type hint in get_number_of_image_patches - #47788

Merged
remi-or merged 1 commit into
mainfrom
fix-type-hinting-img-processing
Aug 5, 2026
Merged

[Fix] Wrong type hint in get_number_of_image_patches#47788
remi-or merged 1 commit into
mainfrom
fix-type-hinting-img-processing

Conversation

@remi-or

@remi-or remi-or commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

CI

This PR fixes a wrong type hint + default value in get_number_of_image_patches: currently, the image_kwargs is annotated as an optionnal dictionary with default value None but passing None would crash the function.
To correect this, we use image_kwargs = image_kwargs or {} to go from the default dict to an empty one and then use get with appropriate defaults.

Also, glm4v used to hardcode a default value (call it V) for size that was different from a potential user-supplied self.size: now, we default to self.size, which if not user supplied, defaults itself to V. So in absence of user intervention, behavior is identical.

One thing to consider: depending on the model, get_number_of_image_patches returns either an int (number of patches, n_patches = n_width_patches * n_height_patches) a tuple of 2 ints (n_width_patches, n_height_patches) or a tuple of 3 ints (n_patches, n_width_patches, n_height_patches) -> maybe it would be worth uniformising this. WDYT? cc. @molbap @zucchini-nlp

PS: we can go the other route and force image_kwargs to be passed, but it seems even more breaking, plus I think VLLM uses this function.

@remi-or
remi-or requested review from molbap and zucchini-nlp August 5, 2026 06:10
@remi-or remi-or self-assigned this Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

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

run-slow: aria, cohere2_vision, cosmos3_edge, deepseek_ocr2, ernie4_5_vl_moe, glm46v, glm4v, glm_image, glmga, got_ocr2, hunyuan_vl, idefics3, kimi_k25, minimax_m3_vl, paddleocr_vl, qwen2_vl

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

CI recap

Dashboard: View test results in Grafana
Latest run: 30980457597
Result: success | Grafana metrics are not available yet.

@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.

@zucchini-nlp zucchini-nlp left a comment

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.

hm, i think we don't have cases when kwargs are explicitly None if we assume the helper is used only by vllm 馃
In most cases None would have thrown an error when we try to get a value from it

Re different return types: agreed on that, Anton raises similar question recently on making get_num_patches a "required/uniform" method for image processors. Will be a bit breaking but imo worth it in long term

@remi-or

remi-or commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

hm, i think we don't have cases when kwargs are explicitly None if we assume the helper is used only by vllm 馃

I will also use the helper in CB I think!

In most cases None would have thrown an error when we try to get a value from it

yes definitely. Hence my PS about the fact we can go the other way, but I think the default is a good solution as well

@zucchini-nlp

Copy link
Copy Markdown
Member

I will also use the helper in CB I think!

ah, nice, didn't know about that. We can make it optional, no big deal in any case. If you wish to work on uniform return format, imo the height/width tuple makes most sense to support non-square grids in some models

I'll review this one in a sec

@zucchini-nlp zucchini-nlp left a comment

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.

lgtm

Comment on lines -271 to +272
min_patches = images_kwargs.get("min_patches", self.min_patches) if images_kwargs else self.min_patches
max_patches = images_kwargs.get("max_patches", self.max_patches) if images_kwargs else self.max_patches
patch_size = images_kwargs.get("patch_size", self.size) if images_kwargs else self.size
crop_to_patches = (
images_kwargs.get("crop_to_patches", self.crop_to_patches) if images_kwargs else self.crop_to_patches
)
images_kwargs = images_kwargs or {}
min_patches = images_kwargs.get("min_patches", self.min_patches)

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.

oke, i see where the confusion comes from 馃珷

@remi-or

remi-or commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

If you wish to work on uniform return format, imo the height/width tuple makes most sense to support non-square grids in some models

Actually I liked the 3 ints tuple, with product, width, height (or the other order, idk). But we can leave that for a downstream PR

@molbap molbap left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM! type balance is restored, thanks. For the return uniformization, the three-element tuple is nice but I'm always a fan of dataclasses for typing outputs as well, slightly more future-proof.
To note, the name of the util is also misleading as we don't just return the number of image patches, but rich image patches metadata in many cases, though we can't really change that (just to keep in mind when we uniformize everything)

@zucchini-nlp

Copy link
Copy Markdown
Member

dataclasses for typing outputs as well

ah right, this will be easier for future extremely weird processors indeed

@remi-or
remi-or added this pull request to the merge queue Aug 5, 2026
Merged via the queue into main with commit d35d1f8 Aug 5, 2026
42 checks passed
@remi-or
remi-or deleted the fix-type-hinting-img-processing branch August 5, 2026 10:14
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