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

Revert removal and deprecation of int values for interpolation? #7205

Closed
pmeier opened this issue Feb 9, 2023 · 1 comment · Fixed by #7241
Closed

Revert removal and deprecation of int values for interpolation? #7205

pmeier opened this issue Feb 9, 2023 · 1 comment · Fixed by #7241

Comments

@pmeier
Copy link
Collaborator

pmeier commented Feb 9, 2023

Initially, torchvision supported int's for the interpolation (or their former equivalent) arguments on our transforms. They were usually not passed directly, but rather as part of the Pillow constants, like PIL.Image.BILINEAR.

#2952 (review) proposed to decouple our transforms from Pillow and this was achieved through the torchvision.transforms.InterpolationMode enum introduced later in the same PR. Although, the new enum was used as default value from then on, int's were still supported. Still, the language around them somewhat "soft-deprecated" them:

For backward compatibility integer values (e.g. ``PIL.Image.NEAREST``) are still acceptable.

With the release of Pillow==9.1.0, they deprecated the int constants and switched to their own enums as well. In response we introduced support for these enums as well in #5898 and deprecated ints in #5974.

Our deprecation was scheduled for removal in the upcoming 0.15 release and was acted upon in #7176.

However, there are two factors we didn't consider there:

  1. @NicolasHug pointed out that using ints is still widespread for Meta internal users

  2. Pillow seemed to have walked back on their deprecation in 9.4.0 as well:

    In Pillow 9.1.0, the following constants were deprecated. That has been reversed and these constants will now remain available.

    This follows API changes for Resampling modes python-pillow/Pillow#6537 which acknowledges that the change is too disruptive. However, since removing the enums now would also be BC breaking (Revert replacing and deprecating constants with enums python-pillow/Pillow#6684), they just reinstated the former int constants in python-pillow/Pillow@bcdb208

Given that our deprecation just followed Pillow's, I think we can also revert the removal and deprecation. Between >= 9.1.0 and < 9.4.0, Pillow never dropped support for int's. Thus, the only issues our users could run into when using int's on that versions is that Pillow will emit a deprecation warning. Our default values use our enum anyway

def __init__(self, size, interpolation=InterpolationMode.BILINEAR, max_size=None, antialias=None):

If we go that route note that we also need add support for int's to our prototype transforms. They don't have that, since we designed with the scheduled removal in mind.

Thoughts?

cc @vfdev-5

@NicolasHug
Copy link
Member

Thanks for digging the history here Philip.
I'm happy to go for the least disruptive change and put back support for PIL.Image int constants. Since they're here to stay (according to the issue/PR on the PIL repo), we can also remove our warning encouraging users to use our own InterpolationMode.

If we go that route note that we also need add support for int's to our prototype transforms

Good point - although considering how tight our deadline is, I would be fine with leaving that for after the release if we have to (if we can do it before, it's even better, but not a blocker)

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

Successfully merging a pull request may close this issue.

2 participants