[Vision] `.to` function for ImageProcessors #20536

younesbelkada · 2022-12-01T17:23:35Z

What does this PR do?

PoC for adding .to support on ImageProcessors

related #20453

src/transformers/feature_extraction_utils.py

HuggingFaceDocBuilderDev · 2022-12-01T17:39:35Z

The documentation is not available anymore as the PR was closed or merged.

younesbelkada · 2022-12-01T17:46:28Z

Here is a quick v1, but I am afraid it's a bit too much in the sense that I am literally testing every possible combination
Also regarding tests, we can remove them or put them as slow. I checked with deit, vit & vilt (for multimodal setup) and the tests are green (the failing test for LayoutLM can be easily fixed)
May I ask you to have a quick look @sgugger @ydshieh ? Thanks!

src/transformers/feature_extraction_utils.py

sgugger · 2022-12-01T17:57:55Z

You're looking at something too complicated: to() does all that work for you already. You can pass it a string, a device or a dtype.

younesbelkada · 2022-12-01T18:22:36Z

Yes I was thinking of something very complicated where someone could set .to(device, dtype) let's maybe keep it even simpler and force the user to put only a single argument in .to ?

EDIT: it seems that there is a workaround for that

src/transformers/utils/generic.py

ydshieh · 2022-12-02T16:46:33Z

src/transformers/utils/generic.py

+
+    if isinstance(x, str):
+        if hasattr(torch, x):
+            return True


I would be more careful -> i.e. to get that attribute, than test if it is indeed a torch.dtype

you mean

if hasattr(torch, x): x = getattr(torch, x) else: return False

?

yeah, x = getattr(torch, x) and then test if it is a torch dtype, just as what you did a few line below.

Be careful, I always complicate things. You can hold on until sylvain's review.

Adapted your suggestion in da35051 ;)

amyeroberts

Thanks for opening this PR! Looking good :)

Just a few small comments and +1 on @ydshieh's suggestions on _is_torch_dtype.

I'm still unsure about the behaviour with to when some of the inputs are ints, as it doesn't match PyTorch (I can cast ints to floats), however with logging it's probably OK. I wonder if logger.warning might be too aggressive a log level as it's not something the user can really change.

amyeroberts · 2022-12-02T18:16:54Z

tests/models/deit/test_feature_extraction_deit.py

 class DeiTFeatureExtractionTest(FeatureExtractionSavingTestMixin, unittest.TestCase):

    feature_extraction_class = DeiTFeatureExtractor if is_vision_available() else None
+    test_cast_dtype = True


Why is it needed here for DeiT and not other models?

I just waited for everyone's approval on the concept and planned to add it on more models! Probably on the main models only as it may slow down the CI test suite

src/transformers/feature_extraction_utils.py

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

younesbelkada · 2022-12-02T23:07:41Z

Thanks everyone for the feedback! Let me know if you think it's relevant to add the test_cast_dtype for all ImageProcessors as it may slow down our CI testing suite

sgugger

Good catch on the int tensors, I have comments on the actual implementation however.

src/transformers/feature_extraction_utils.py

sgugger · 2022-12-05T16:58:22Z

src/transformers/feature_extraction_utils.py

+        import torch  # noqa
+
+        new_data = {}
+        device = self._parse_args_to_device(*args, **kwargs)


Still not fan of the name :-p

Also, until it's used in another place, I would avoid doing a separate method for this. Let's just put the code there!

sgugger

Thanks for bearing with me :-)

younesbelkada · 2022-12-05T17:31:22Z

Ahaha no worries! thanks for all the iterations 💪

* add v1 with tests * add checker * simplified version * update docstring * better version * fix docstring + change order * make style * tests + change conditions * final tests * modify docstring * Update src/transformers/feature_extraction_utils.py Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com> * replace by `ValueError` * fix logic * apply suggestions * `dtype` is not needed * adapt suggestions * remove `_parse_args_to_device` Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

add v1 with tests

1f1ee46

younesbelkada commented Dec 1, 2022

View reviewed changes

src/transformers/feature_extraction_utils.py Outdated Show resolved Hide resolved

younesbelkada requested review from sgugger and ydshieh December 1, 2022 17:46

add checker

a8053dd

younesbelkada commented Dec 1, 2022

View reviewed changes

src/transformers/feature_extraction_utils.py Outdated Show resolved Hide resolved

younesbelkada added 2 commits December 1, 2022 18:19

simplified version

94c2a93

update docstring

f9045b1

younesbelkada added 5 commits December 2, 2022 08:53

better version

4af5971

fix docstring + change order

e595cdf

make style

4d05a90

tests + change conditions

4b9fddd

final tests

09e4c49

younesbelkada changed the title ~~[draft] .to function for ImageProcessors~~ [Vision] .to function for ImageProcessors Dec 2, 2022

younesbelkada marked this pull request as ready for review December 2, 2022 10:49

younesbelkada requested a review from amyeroberts December 2, 2022 10:49

younesbelkada mentioned this pull request Dec 2, 2022

[Vision] Support different floating precision inputs from the ImageProcessor #20453

Closed

ydshieh reviewed Dec 2, 2022

View reviewed changes

src/transformers/utils/generic.py Outdated Show resolved Hide resolved

ydshieh reviewed Dec 2, 2022

View reviewed changes

modify docstring

ba94418

amyeroberts reviewed Dec 2, 2022

View reviewed changes

younesbelkada and others added 3 commits December 2, 2022 23:46

Update src/transformers/feature_extraction_utils.py

f331dac

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

replace by ValueError

af0a4c3

fix logic

da35051

sgugger reviewed Dec 5, 2022

View reviewed changes

src/transformers/feature_extraction_utils.py Outdated Show resolved Hide resolved

younesbelkada added 2 commits December 5, 2022 16:38

apply suggestions

af04179

dtype is not needed

fee0585

younesbelkada requested a review from sgugger December 5, 2022 15:40

sgugger reviewed Dec 5, 2022

View reviewed changes

src/transformers/feature_extraction_utils.py Outdated Show resolved Hide resolved

src/transformers/feature_extraction_utils.py Outdated Show resolved Hide resolved

adapt suggestions

475f5ef

sgugger reviewed Dec 5, 2022

View reviewed changes

remove _parse_args_to_device

c5b4b37

sgugger approved these changes Dec 5, 2022

View reviewed changes

younesbelkada merged commit ef0f85c into huggingface:main Dec 5, 2022

sgugger mentioned this pull request Dec 22, 2022

Adding support for fp16 for asr pipeline. #20864

Merged

5 tasks

[Vision] .to function for ImageProcessors #20536

[Vision] .to function for ImageProcessors #20536

Uh oh!

Conversation

younesbelkada commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Uh oh!

Uh oh!

HuggingFaceDocBuilderDev commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

younesbelkada commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sgugger commented Dec 1, 2022

Uh oh!

younesbelkada commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ydshieh Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

younesbelkada Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

ydshieh Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

younesbelkada Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

amyeroberts left a comment

Choose a reason for hiding this comment

Uh oh!

amyeroberts Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

younesbelkada Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

younesbelkada commented Dec 2, 2022

Uh oh!

sgugger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sgugger Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

sgugger left a comment

Choose a reason for hiding this comment

Uh oh!

younesbelkada commented Dec 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Vision] `.to` function for ImageProcessors #20536

[Vision] `.to` function for ImageProcessors #20536

younesbelkada commented Dec 1, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Dec 1, 2022 •

edited

Loading

younesbelkada commented Dec 1, 2022 •

edited

Loading

younesbelkada commented Dec 1, 2022 •

edited

Loading

younesbelkada Dec 2, 2022 •

edited

Loading