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

WIP Add PIL 4.1.1 to CI checks #3638

Closed
wants to merge 8 commits into from

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Apr 6, 2021

This PR lets the linux/mac 3.6 unittest runs use pillow=4.1.1 instead of the latest pil version.

Addresses #3631 (comment)

@NicolasHug
Copy link
Member Author

NicolasHug commented Apr 6, 2021

@fmassa this is still WIP but the CI failures of
3df73d2 revealed that while we require pillow >= 4.1.1 in setup.py, there are some functionalities that are unavailable even for pillow >= 4.1.1. For example:

  • draw_bounding_boxes requires >= 5.3.0
  • rotate requires >= 5.2.0
  • perspective requires >= 5.0.0
  • etc...

I think we should avoid this and bump the minimal required version to 5.3.0 (released in October 2018, so it's been a while).

IMHO it's not ideal to have more than one minimal version requirements for the dependencies. It might seem like we're doing the users a favour by treating some features as "opt-in", but in reality this is likely to complicate their lives. If a user wants to use e.g. draw_bounding_boxes, they can't rely on the automatic dependency resolving of pip or conda anymore: they need to manually specify that they need 5.3.0, and they would only know that after the fact.

CC @oke-aditya

@oke-aditya
Copy link
Contributor

oke-aditya commented Apr 6, 2021

I think 5.3.0 seems to have made a lot of changes in plotting APIs. Looking at docs I see that many functionalities differed from v5.3.0

Also by default torchvision installs the latest pillow (tested by conda and pip installing torchvision locally). (it installed 8.2 last week and 8.3 this week)
So most users might be Pillow 5.3.0+ (since it came out while ago).

I agree with @NicolasHug that this bump would solve the issue.

It will help to clean up the code of _parse_fill at places. If the minimal version is 5.3.0 we no longer would need to check for these.

@fmassa
Copy link
Member

fmassa commented Apr 7, 2021

Ok with me to bump the minimum version to be 5.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants