Skip to content

Delay warning for deprecated parameter 'vert' of box and violin #29155

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

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

ksunden
Copy link
Member

@ksunden ksunden commented Nov 18, 2024

The new parameter, 'orientation', was only added in the current release, so downstream wanting to avoid warnings
would require version gates. Therefore delaying by at least one release to ease the transition.

This was motivated by Pandas failing tests on this warning.

PR summary

PR checklist

@QuLogic
Copy link
Member

QuLogic commented Nov 19, 2024

I would suggest targeting v3.10.x with a removal directly, and main with a change from 3.10 to 3.11.

@timhoffm
Copy link
Member

@ksunden do you have a link to the issue?

This is a fundamental problem with deprecations. When you introduce a deprecation third-party libraries will see it. This will have two effects:

  • The tests of the library will pick up the warning. Libraries have to version-gate to silence the warning. This seems the current motivation.
  • Users will see the warning without being able to do something about it as soon as we release, because the code is in the third-party library and they did not have time to react/release.

I believe that you cannot reasonably circumvent version gating in third party libraries. Even if we give one release of transition where both versions are available equally, they have to pin matplotlib quite aggressively. It's rather the second point that motivates me for pending releases.

By default, we currently do not do pending deprecations. Is that something we should should regularly do and add to our API change guideline? OTOH I'm not aware of substantial complaints on our current deprecation strategy.

@rcomer
Copy link
Member

rcomer commented Nov 19, 2024

Assuming pandas is following SPEC0, does this mean we should wait until May 2026 (drop date for mpl3.9 support) to deprecate the old parameter?

@ksunden
Copy link
Member Author

ksunden commented Nov 19, 2024

This was discovered by the run of Fedora builds that @QuLogic did, rather than being specifically reported/having an issue to link to.

I discussed it with @tacaswell and we came to the decision to delay it by just one release, which still may incur version gates on libraries, but minimizes impact to especially downstream applications (which tend to have narrower support windows). If the new API were available previously, I would be less worried about it, even if a version without fell within Pandas' minimal support window, since the further downstream effects are minimized.

@ksunden
Copy link
Member Author

ksunden commented Nov 19, 2024

As for release notes, I will take that on in release, not including them in the compiled form and leaving them in next... But thanks for the reminder there.

@tacaswell
Copy link
Member

I would like to give one version with both the old and new way not warning before we warn on the old way to reduce the impact of the transition.

What got me with this one is that use are something like:

def.plot.some_plotting_method(..., vert=True)

where pandas is just passing kwargs through to us so no library changes are needed to pandas (and I do not think they should be smoothing out this change and doing the translation from vert to orientation!) so the impact is probably going to predominately fall on individual users (not on the libraries) who are typing this out interactively or in notebooks. Given that and the rough guess that day-to-day working environments tend to fluctuate +/- 1 in terms of versions of packages delaying the deprecation warning one release increases the number of users who can switch to the new way and never think about it again.

The new parameter, 'orientation', was only added in the current release, so downstream wanting to avoid warnings
would require version gates. Therefore delaying by at least one release to ease the transition.

This was motivated by Pandas failing tests on this warning.
ksunden added a commit to ksunden/matplotlib that referenced this pull request Nov 19, 2024
The new parameter, 'orientation', was only added in the current release, so downstream wanting to avoid warnings
would require version gates. Therefore delaying by at least one release to ease the transition.

This was motivated by Pandas failing tests on this warning.

This portion is made direct to v3.10.x with matplotlib#29155 being the accompanying version to edit the deprecation on main.
@QuLogic QuLogic added this to the v3.11.0 milestone Nov 20, 2024
@QuLogic
Copy link
Member

QuLogic commented Nov 20, 2024

The tests need updating to match the changes here.

@timhoffm
Copy link
Member

Coming back to the general question: Do we need to refine our deprecation policy? e.g.

The suggested alternative must be available one release prior to deprecation. If the alternative has just been added, do a pending deprecation for one release and switch to the actual depreaction in the subsequent release.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

These are not pending anymore in 3.11

@timhoffm
Copy link
Member

I'm overall not clear what we want. The comments and code here and in #29159 send mixed signals

a. 3.10: discourage, 3.11: deprecate
b. 3.10: discourage, 3.11: pending deprecate
c. 3.10: pending deprecate, 3.11: deprecate

I would have expected c.

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@ksunden
Copy link
Member Author

ksunden commented Nov 20, 2024

I was largely thinking a) discourage in 3.10, deprecate in 3.11.

A pending deprecation still does fail Pandas tests, so if we find "don't break pandas with our release" motivating, that doesn't solve that issue.

@timhoffm
Copy link
Member

As far as I understand #29155 (comment). The failure of pandas test should come from the way the pandas test code is written not from the pandas code itself. That's an important difference because then users of pandas don't rave to wait for a pandas release. In other words, pandas can right now change their tests to fix this - yes this requires version gating to get a smooth transition, but pandas has a Minimum Version CI run (currently using Matplotlib 3.6) and unless they aggressively pin to 3.10 in the future they'll have to do version gating anyway.

On plus side, other libraries would become aware through the pending deprecation.

@tacaswell
Copy link
Member

The failure of pandas test should come from the way the pandas test code is written not from the pandas code itself.

That is also my understanding and I agree that pandas putting version gating in their tests is not a big deal.

That's an important difference because then users of pandas don't rave to wait for a pandas release.

However, if we add the new way and deprecate the old way in one step we are going to inflict version gating on user's code which I think is a problem.

@timhoffm
Copy link
Member

That's why

However, if we add the new way and deprecate the old way in one step we are going to inflict version gating on user's code which I think is a problem.

That's why I suggested a pending deprecation. Users should not see these.

@tacaswell
Copy link
Member

That's why I suggested a pending deprecation. Users should not see these.

Ok, then I think we agree! I get lost in the details of what warnings users do/do not see under what situations.

@tacaswell tacaswell merged commit 84464dd into matplotlib:main Nov 21, 2024
40 of 43 checks passed
@tacaswell
Copy link
Member

@ksunden will do a independent PR to 3.10.x and handle the (likely) conflicts on the merge-up.

ksunden added a commit to ksunden/matplotlib that referenced this pull request Nov 22, 2024
The new parameter, 'orientation', was only added in the current release, so downstream wanting to avoid warnings
would require version gates. Therefore delaying by at least one release to ease the transition.

This was motivated by Pandas failing tests on this warning.

This portion is made direct to v3.10.x with matplotlib#29155 being the accompanying version to edit the deprecation on main.

Reinstate warning as a pending warning

STY: fits on one line
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.

5 participants