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

Update kwargs validation for preprocess with decorator #32024

Merged
merged 42 commits into from
Aug 6, 2024

Conversation

qubvel
Copy link
Member

@qubvel qubvel commented Jul 17, 2024

What does this PR do?

Update most of the image processors with filter_out_non_signature_kwargs decorator for kwargs validation instead of _valid_processor_keys (continue of work started in #30799)

Before submitting

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

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

Comment on lines +444 to +455
# validation done by @filter_out_non_signature_kwargs decorator
if hasattr(image_processor.preprocess, "_filter_out_non_signature_kwargs"):
if hasattr(self.image_processor_tester, "prepare_image_inputs"):
inputs = self.image_processor_tester.prepare_image_inputs()
elif hasattr(self.image_processor_tester, "prepare_video_inputs"):
inputs = self.image_processor_tester.prepare_video_inputs()
else:
self.skipTest(reason="No valid input preparation method found")

with warnings.catch_warnings(record=True) as raised_warnings:
warnings.simplefilter("always")
image_processor(inputs, extra_argument=True)
Copy link
Member Author

@qubvel qubvel Jul 17, 2024

Choose a reason for hiding this comment

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

The decorated method will have _filter_out_non_signature_kwargs argument, here we test that the warning is raised if an unknown argument is passed to the method.

@qubvel
Copy link
Member Author

qubvel commented Jul 17, 2024

CLIP/Git image processors require updating their processors first, DETR* image processors require adding deprecation. In this PR only simple cases are handled, where we need to replace _valid_processor_keys with a decorator, other cases will be done in follow-up PRs for review simplicity.

@qubvel
Copy link
Member Author

qubvel commented Jul 17, 2024

@molbap whenever you have time, please have a look

@molbap molbap assigned molbap and unassigned molbap Jul 23, 2024
@molbap molbap self-requested a review July 23, 2024 16:33
Copy link
Contributor

@molbap molbap left a comment

Choose a reason for hiding this comment

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

LGTM! Much tidier. One comment is since now inspect.signature is used everywhere, is there any impact on tests speed?

@qubvel
Copy link
Member Author

qubvel commented Aug 2, 2024

@molbap inspect.signature is used only once the class method when the class is defined (on class import), so I would not expect any slowdown. Even if we have 1k imports of objects overall it should not take longer than a fraction of a second. Am I missing something here? Afair, you choose to use _valid_processor_kwargs instead inspect.signature, was there any specific reason?

@molbap
Copy link
Contributor

molbap commented Aug 5, 2024

Yes you're right, it's only at import time. I was just curious since inspect-related calls have ended up costing me some precious ms before, so I steer away from it

@qubvel
Copy link
Member Author

qubvel commented Aug 5, 2024

@amyeroberts please review whenever you have time

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.

Beautiful ❤️

self.assertIn("extra_argument", messages)
is_tested = True

if not is_tested:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice :)

@qubvel qubvel merged commit fb66ef8 into huggingface:main Aug 6, 2024
23 checks passed
nbroad1881 pushed a commit that referenced this pull request Aug 7, 2024
* BLIP preprocess

* BIT preprocess

* BRIDGETOWER preprocess

* CHAMELEON preprocess

* CHINESE_CLIP preprocess

* CONVNEXT preprocess

* DEIT preprocess

* DONUT preprocess

* DPT preprocess

* FLAVA preprocess

* EFFICIENTNET preprocess

* FUYU preprocess

* GLPN preprocess

* IMAGEGPT preprocess

* INTRUCTBLIPVIDEO preprocess

* VIVIT preprocess

* ZOEDEPTH preprocess

* VITMATTE preprocess

* VIT preprocess

* VILT preprocess

* VIDEOMAE preprocess

* VIDEOLLAVA

* TVP processing

* TVP fixup

* SWIN2SR preprocess

* SIGLIP preprocess

* SAM preprocess

* RT-DETR preprocess

* PVT preprocess

* POOLFORMER preprocess

* PERCEIVER preprocess

* OWLVIT preprocess

* OWLV2 preprocess

* NOUGAT preprocess

* MOBILEVIT preprocess

* MOBILENETV2 preprocess

* MOBILENETV1 preprocess

* LEVIT preprocess

* LAYOUTLMV2 preprocess

* LAYOUTLMV3 preprocess

* Add test

* Update tests
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