Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstract image processor arg checks. #28843

Merged
merged 23 commits into from
Feb 20, 2024

Conversation

molbap
Copy link
Contributor

@molbap molbap commented Feb 2, 2024

What does this PR do?

This refactors existing image processor argument checks that sprawl out on all existing models that have an ImageProcessor.
Lines such as

        if do_resize is not None and size is None:
            raise ValueError("Size and max_size must be specified if do_resize is True.")

        if do_rescale is not None and rescale_factor is None:
            raise ValueError("Rescale factor must be specified if do_rescale is True.")

        if do_normalize is not None and (image_mean is None or image_std is None):
            raise ValueError("Image mean and std must be specified if do_normalize is True.")

can be abstracted away in a validate... function residing in image_utils.
Also, fixing (when it doesn't break BC) some cases where existence of arguments is checked, but the actual preprocess method doesn't seem to call them. bridgetower doesn't pass center_crop from init, chinese_clip does not convert_to_rgb, and so on.

Who can review?

@amyeroberts

@molbap
Copy link
Contributor Author

molbap commented Feb 2, 2024

Also linked to #28711 as I discovered logic flow issues here, seems fitting to abstract them separately and deal with the actual processing in the main PR. Here I'll try to stick to verifications and fix what's necessary to satisfy existing tests.

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

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Looks great - so much tidier 🤩 Thanks for abstracting this out!

Let me know when it's ready for final review

@molbap molbap marked this pull request as ready for review February 9, 2024 17:02
Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for working on this and tidying all this code up 🧹 🧹 🧹

Just a few small comments

src/transformers/image_utils.py Outdated Show resolved Hide resolved
src/transformers/models/bit/image_processing_bit.py Outdated Show resolved Hide resolved
src/transformers/models/blip/image_processing_blip.py Outdated Show resolved Hide resolved
@molbap molbap changed the title WIP: abstract image processor arg checks. Abstract image processor arg checks. Feb 12, 2024
@molbap
Copy link
Contributor Author

molbap commented Feb 15, 2024

@amyeroberts I think it's ok to take another look at this one now! Improved a few things, didn't add much, will rebase the other refactor off of that one

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Looks great - thanks for working on this!

@molbap molbap merged commit 1c9134f into huggingface:main Feb 20, 2024
21 checks passed
This was referenced Feb 27, 2024
itazap pushed a commit that referenced this pull request May 14, 2024
* abstract image processor arg checks.

* fix signatures and quality

* add validate_ method to rescale-prone processors

* add more validations

* quality

* quality

* fix formatting

Co-authored-by: amyeroberts <[email protected]>

* fix formatting

Co-authored-by: amyeroberts <[email protected]>

* fix formatting

Co-authored-by: amyeroberts <[email protected]>

* Fix formatting mishap

Co-authored-by: amyeroberts <[email protected]>

* fix crop_size compatibility

* fix default mutable arg

* fix segmentation map + image arg validity

* remove segmentation check from arg validation

* fix quality

* fix missing segmap

* protect PILImageResampling type

* Apply suggestions from code review

Co-authored-by: amyeroberts <[email protected]>

* add back segmentation maps check

---------

Co-authored-by: amyeroberts <[email protected]>
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.

3 participants