-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
Abstract image processor arg checks. #28843
Conversation
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. |
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. |
There was a problem hiding this 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
src/transformers/models/chinese_clip/image_processing_chinese_clip.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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/models/bridgetower/image_processing_bridgetower.py
Outdated
Show resolved
Hide resolved
src/transformers/models/bridgetower/image_processing_bridgetower.py
Outdated
Show resolved
Hide resolved
src/transformers/models/bridgetower/image_processing_bridgetower.py
Outdated
Show resolved
Hide resolved
src/transformers/models/bridgetower/image_processing_bridgetower.py
Outdated
Show resolved
Hide resolved
src/transformers/models/bridgetower/image_processing_bridgetower.py
Outdated
Show resolved
Hide resolved
src/transformers/models/chinese_clip/image_processing_chinese_clip.py
Outdated
Show resolved
Hide resolved
Co-authored-by: amyeroberts <[email protected]>
Co-authored-by: amyeroberts <[email protected]>
Co-authored-by: amyeroberts <[email protected]>
Co-authored-by: amyeroberts <[email protected]>
@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 |
There was a problem hiding this 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!
src/transformers/models/efficientformer/image_processing_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/mobilevit/image_processing_mobilevit.py
Outdated
Show resolved
Hide resolved
src/transformers/models/bridgetower/image_processing_bridgetower.py
Outdated
Show resolved
Hide resolved
Co-authored-by: amyeroberts <[email protected]>
* 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]>
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
can be abstracted away in a
validate...
function residing inimage_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 passcenter_crop
from init,chinese_clip
does notconvert_to_rgb
, and so on.Who can review?
@amyeroberts