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

API changes for Resampling modes #6537

Closed
99991 opened this issue Aug 25, 2022 · 10 comments · Fixed by #6830
Closed

API changes for Resampling modes #6537

99991 opened this issue Aug 25, 2022 · 10 comments · Fixed by #6830
Milestone

Comments

@99991
Copy link

99991 commented Aug 25, 2022

With Pillow 9.2.0, I get the following deprecation notice:

DeprecationWarning: BILINEAR is deprecated and will be removed in Pillow 10 (2023-07-01). Use Resampling.BILINEAR instead.

Is it really necessary to introduce this breaking API change? Can the old names not be kept for backwards compatibility? Thousands of repositories will need to update their code, wasting countless man-hours.

You can get a rough idea on how many repositories are affected from the following two websites, which search a subset of GitHub repositories:

from PIL import Image
img = Image.new("RGB", (5, 5))
img.resize((7, 7), Image.BILINEAR)
@radarhere
Copy link
Member

radarhere commented Aug 25, 2022

To provide context, the deprecations being discussed were added in #5954. The intention was not to make an arbitrary change, but to make use of a newish Python language feature, enums.

#5954 (comment)

Do we keep the old constants "forever"?

Or do we want to deprecate the old constants and remove them in Pillow 10 (15 months' deprecation period)?

They were first deprecated in Pillow 9.1.0 - https://pillow.readthedocs.io/en/stable/releasenotes/9.1.0.html#deprecations

@radarhere
Copy link
Member

I think in the scheme of things, this is a very minor change for users to make when upgrading to a new version. In Javascript programming, I'm used to finding old npm packages abandoned and replaced with new ones, and in iOS programming, I'd be surprised if Apple went a year without changing any constants.

If your argument is that Pillow is extremely widely used, I'll try and use NumPy as a point of reference. That is another Python library, downloaded twice as much as Pillow. In NumPy 1.20.0, they deprecated a series of aliases.

https://grep.app/search?q=np.int seems to show that at least 20 times the amount of code is affected by that compared to https://grep.app/search?q=Image.BILINEAR.

I make this comment not in the interest of taking a stance, but in the interest of giving you a quick response.

@99991
Copy link
Author

99991 commented Aug 26, 2022

In NumPy 1.20.0, they deprecated a series of aliases.

I believe that the argument for deprecating inbuilt types in NumPy is a bit stronger since it was know for a long time to be an actual problem numpy/numpy#6103

The intention was not to make an arbitrary change, but to make use of a newish Python language feature, enums.

The usage of a new Python language feature does not sound like a very convincing argument to me. Also, it is not an argument for breaking backwards compatibility.

https://grep.app/search?q=np.int seems to show that at least 20 times the amount of code is affected by that compared to https://grep.app/search?q=Image.BILINEAR.

Your query also includes the non-deprecated types like np.int_ or np.int32. If you exclude those with a regular expression like https://grep.app/search?q=np%5C.int%5B%5E0-9%5Ec%5Ep%5D&regexp=true there are 10 times fewer hits than https://grep.app/search?q=Image.BILINEAR

@dynobo
Copy link

dynobo commented Sep 3, 2022

While I agree with the change in general, may I ask: What were the reasons to only bump pillow's minor version? Wouldn't, at least according to semver, a breaking API change of such a widely used feature would justify a new major release?
Oh, scrape that, it was just introduced a deprecation warning so far. Everything good ;-)

@radarhere
Copy link
Member

The deprecation warning may have appeared in a minor version, but the removal of the original values won't happen until the next major version.

@aclark4life aclark4life changed the title Unnecessary API changes for Resampling modes API changes for Resampling modes Sep 24, 2022
@hugovk
Copy link
Member

hugovk commented Oct 18, 2022

I'm having second thoughts about this API change.

While enums are useful for this, and would be great for new things, I think this might be a bit too disruptive given the gains.

It would be good to avoid a situation like the PILLOW_VERSION change as mentioned in #6614 (comment).

What are other's thoughts?

(As an aside, I wouldn't be averse to re-adding PILLOW_VERSION, it's just a constant; for example, it could be commented as deprecated with no warnings.)

@radarhere
Copy link
Member

radarhere commented Oct 19, 2022

PILLOW_VERSION was deprecated in 5.2.0, removed in 7.0.0, restored as deprecated in 7.1.0 and removed again in 9.0.0. Fully restoring it may well help people on a practical level, but as an end user, I would feel very unsure about whether PILLOW_VERSION will be present or not in any future version of Pillow.

I don't have strong feeling on what the implementation is, only a preference that we aim to be consistent - if a decision is made, that we follow through. For a different example, I'm reluctant about #5309 because we previously told users to use close() to close files, but that issue would like to prevent that behaviour. I think clear communication and expectations are more fundamental than backwards compatibility.

@hugovk
Copy link
Member

hugovk commented Oct 24, 2022

PILLOW_VERSION was deprecated in 5.2.0, removed in 7.0.0, restored as deprecated in 7.1.0 and removed again in 9.0.0. Fully restoring it may well help people on a practical level, but as an end user, I would feel very unsure about whether PILLOW_VERSION will be present or not in any future version of Pillow.

I think that would be fine: the message is end users should not use PILLOW_VERSION. We'd comment it as deprecated, no removal date listed. Anyone accessing PILLOW_VERSION via legacy code (often as a dependency as a dependency) won't know either way. Anyone writing new code who looks it up and sees the comment should use __version__ instead. But if they still use (e.g. by using autocomplete), it doesn't really matter. It's just a single constant, very little cost to leave it there than worry about removal.

I don't have strong feeling on what the implementation is, only a preference that we aim to be consistent - if a decision is made, that we follow through. For a different example, I'm reluctant about #5309 because we previously told users to use close() to close files, but that issue would like to prevent that behaviour. I think clear communication and expectations are more fundamental than backwards compatibility.

I think we can change our minds, but should do so with clear communication. We need to balance cost/benefits of BC breaks, and it would be better to do so before release (both as PILLOW_VERSION taught us).

Here's a PR to revert in case we want to go ahead: #6684

@hugovk
Copy link
Member

hugovk commented Oct 25, 2022

I closed PR #6684, I somehow had glossed over that the enums were already released in 9.1.0, so this itself would be a breaking change.

Some other ideas we're considering:

  • Keep the enums and deprecated ints
  • Keep the enums and deprecated ints, but extend deprecation period (e.g. from v10/July 2023 to v11/Oct 2024, or even without advertising an end date)
  • Provide API via both ints and enums, remove int deprecations

@radarhere
Copy link
Member

I've created PR #6830 as a suggestion for this.

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