Skip to content

Abstract image processor arg checks.#28843

Merged
molbap merged 23 commits into
huggingface:mainfrom
molbap:refactor_image_processor_arg_check
Feb 20, 2024
Merged

Abstract image processor arg checks.#28843
molbap merged 23 commits into
huggingface:mainfrom
molbap:refactor_image_processor_arg_check

Conversation

@molbap

@molbap molbap commented Feb 2, 2024

Copy link
Copy Markdown
Collaborator

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

molbap commented Feb 2, 2024

Copy link
Copy Markdown
Collaborator Author

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

HuggingFaceDocBuilderDev commented Feb 2, 2024

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.

amyeroberts
amyeroberts reviewed Feb 6, 2024
View reviewed changes

@amyeroberts amyeroberts left a comment

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.

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

Let me know when it's ready for final review

Comment thread src/transformers/models/chinese_clip/image_processing_chinese_clip.py Outdated
@amyeroberts amyeroberts mentioned this pull request Feb 6, 2024
Draft
5 tasks
@molbap molbap marked this pull request as ready for review February 9, 2024 17:02
amyeroberts
amyeroberts reviewed Feb 9, 2024
View reviewed changes

@amyeroberts amyeroberts left a comment

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.

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

Just a few small comments

Comment thread src/transformers/image_utils.py Outdated
Comment thread src/transformers/models/bit/image_processing_bit.py Outdated
Comment thread src/transformers/models/blip/image_processing_blip.py Outdated
Comment thread src/transformers/models/bridgetower/image_processing_bridgetower.py Outdated
Comment thread src/transformers/models/bridgetower/image_processing_bridgetower.py Outdated
Comment thread src/transformers/models/bridgetower/image_processing_bridgetower.py Outdated
Comment thread src/transformers/models/bridgetower/image_processing_bridgetower.py Outdated
Comment thread src/transformers/models/bridgetower/image_processing_bridgetower.py Outdated
Comment thread src/transformers/models/bridgetower/image_processing_bridgetower.py
Comment thread src/transformers/models/chinese_clip/image_processing_chinese_clip.py Outdated
molbap and others added 12 commits February 11, 2024 14:34
@molbap @amyeroberts
fix formatting
483c4f6 
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@molbap @amyeroberts
fix formatting
db9a7b1 
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@molbap @amyeroberts
fix formatting
30a79e1 
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@molbap @amyeroberts
Fix formatting mishap
f483244 
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@molbap
Merge branch 'main' into refactor_image_processor_arg_check
14aab03
@molbap
fix crop_size compatibility
f8ed379
@molbap
fix default mutable arg
112c763
@molbap
fix segmentation map + image arg validity
ac28e19
@molbap
remove segmentation check from arg validation
226c158
@molbap
fix quality
cf4fce3
@molbap
fix missing segmap
ac6f33d
@molbap
protect PILImageResampling type
0ab363f
@molbap molbap changed the title WIP: abstract image processor arg checks. Abstract image processor arg checks. Feb 12, 2024
@molbap

molbap commented Feb 15, 2024

Copy link
Copy Markdown
Collaborator Author

@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

amyeroberts
amyeroberts approved these changes Feb 15, 2024
View reviewed changes

@amyeroberts amyeroberts left a comment

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.

Looks great - thanks for working on this!

Comment thread src/transformers/models/efficientformer/image_processing_efficientformer.py Outdated
Comment thread src/transformers/models/mobilevit/image_processing_mobilevit.py
Comment thread src/transformers/models/bridgetower/image_processing_bridgetower.py Outdated
Comment thread src/transformers/models/sam/image_processing_sam.py
molbap and others added 2 commits February 16, 2024 10:40
@molbap @amyeroberts
Apply suggestions from code review
2a52beb 
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@molbap
add back segmentation maps check
fe430ba
@molbap molbap merged commit 1c9134f into huggingface:main Feb 20, 2024
This was referenced Feb 27, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@amyeroberts amyeroberts amyeroberts approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@molbap @HuggingFaceDocBuilderDev @amyeroberts