Skip to content

Conversation

@amyeroberts
Copy link
Contributor

@amyeroberts amyeroberts commented Dec 9, 2022

What does this PR do?

Addresses issue when importing functions in the image_transforms module if Pillow isn't installed. Namely:

  • functions which don't require Pillow have errors when importing. This was because other objects were only imported if Pillow was available e.g. ChannelDimension. Resolved by rearranging imports.
  • Unuseful error message hiding that the issue is with Pillow in the environment. Resolved by adding a vision_required decorator to allow for errors only on select functions.

Fixes #20627

This needs to be merged in before the transforms can be removed from the init in #20704

Snippet below shows new behaviour when running in an environment without Pillow installed:

>>> import numpy as np
>>> 
>>> # Show we can import both methods without issue
>>> from transformers.image_transforms import rescale
>>> from transformers.image_transforms import center_crop
>>> 
>>> img = np.random.randint(0, 256, (3, 244, 360))
>>> 
>>> # We can call rescale successfully without having Pillow
>>> rescale_img = rescale(img, 10)
>>> print(rescale_img.shape)
(3, 244, 360)

>>> # center_crop call raises error
>>> cropped_img = center_crop(img, (100, 100))
Traceback (most recent call last):
  File "/Users/amyroberts/code/transformers/../scripts/test_image_transforms_imports.py", line 10, in <module>
    cropped_img = center_crop(img, (100, 100))
  File "/Users/amyroberts/code/transformers/src/transformers/utils/import_utils.py", line 1073, in wrapper
    raise ImportError(f"Method `{func.__name__}` requires Pillow.")
ImportError: Method `center_crop` requires Pillow.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Dec 9, 2022

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

@amyeroberts amyeroberts marked this pull request as ready for review December 9, 2022 17:52
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thansk for working on this. Not too sur we actually need a decorator for that, with the existing tools.

return rescaled_image


@vision_required
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this different than having requires_backend(["vision"]) as the first line of the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I just saw the tf_required and copied from there. I'll remove the decorator and add those instead 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yes, tf_required and torch_required could probably be cleaned up too!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened a draft PR here: #20715

@amyeroberts amyeroberts changed the title Add require_vision decorator Add vision requirement to image transforms Dec 9, 2022
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@ydshieh
Copy link
Collaborator

ydshieh commented Dec 9, 2022

A quick question: If we add requires_backends(center_crop, ["vision"]) in some methods in src/transformers/image_transforms.py, shouldn't we do the same in src/transformers/image_utils.py, say, load_image?

@amyeroberts
Copy link
Contributor Author

A quick question: If we add requires_backends(center_crop, ["vision"]) in some methods in src/transformers/image_transforms.py, shouldn't we do the same in src/transformers/image_utils.py, say, load_image?

Yep. I'll add those in too.

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Thank you @amyeroberts

(I was think if it is good to call require_backends in each call - but it seems this call is very fast so there should be no problem)

@amyeroberts amyeroberts merged commit b58beeb into huggingface:main Dec 12, 2022
@amyeroberts amyeroberts deleted the can-import-non-pillow-transforms branch December 12, 2022 17:43
mpierrau pushed a commit to mpierrau/transformers that referenced this pull request Dec 15, 2022
* Add require_vision decorator

* Fixup

* Use requires_backends

* Add requires_backend to utils functions
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.

When Pillow is not installed, importing from transformers.image_transforms raises an unclear NameError

4 participants