Skip to content

Expire deprecations in cbook.deprecation #22514

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 25, 2022
Merged

Expire deprecations in cbook.deprecation #22514

merged 1 commit into from
Feb 25, 2022

Conversation

oscargus
Copy link
Member

@oscargus oscargus commented Feb 20, 2022 •
edited
Loading

PR Summary

Expire deprecations in cbook.deprecation. Will add a release note, but was a bit confused which things actually are deprecated.

cbook.MatplotlibDeprecationWarning and cbook.mplDeprecation are deprecated based on the earlier release note:

The module matplotlib.cbook.deprecation is considered internal and will be removed from the public API. This also holds for deprecation-related re-imports in matplotlib.cbook, i.e. matplotlib.cbook.deprecated(), matplotlib.cbook.warn_deprecated(), matplotlib.cbook.MatplotlibDeprecationWarning and matplotlib.cbook.mplDeprecation.
If needed, external users may import MatplotlibDeprecationWarning directly from the matplotlib namespace. mplDeprecation is only an alias of MatplotlibDeprecationWarning and should not be used anymore.

However, it is possible to import cbook.MatplotlibDeprecationWarning and cbook.mplDeprecation and use them without any deprecation warnings. Is it still OK to removed them?

Btw, the current code gave a dual deprecation warning: :-)


In [4]: from matplotlib.collections import LineCollection

In [5]: LineCollection(1, 2)
C:\Users\Oscar\AppData\Local\Temp\ipykernel_21208\127307086.py:1: MatplotlibDeprecationWarning: 
The warn_deprecated function was deprecated in Matplotlib 3.4 and will be removed two minor releases later.
  LineCollection(1, 2)
C:\Users\Oscar\AppData\Local\Temp\ipykernel_21208\127307086.py:1: MatplotlibDeprecationWarning: Since 3.4, passing LineCollection arguments other than the first, 'segments', as positional arguments is deprecated, and they will become keyword-only arguments two minor releases later.
  LineCollection(1, 2)

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@oscargus oscargus force-pushed the cbookdeprecationremoval branch from c2e06ae to fb3a50e Compare February 20, 2022 16:07
@oscargus oscargus marked this pull request as draft February 20, 2022 16:54
@oscargus
Copy link
Member Author

The failing tests are an indication that these were not properly deprecated earlier (and that I didn't look carefully enough across the entire code base...).

Should they still be removed or should they be properly deprecated now instead?

@timhoffm
Copy link
Member

I don't think we have or easily could have added a warning on importing the class via cbook.MatplotlibDeprecationWarning atbruntime. But we have properly documented the deprecation: https://matplotlib.org/devdocs/api/prev_api_changes/api_changes_3.4.0.html#deprecation-related-functionality-is-considered-internal

Also it's rather unlikely for end users to import MatplotlibDeprecationWarning. So let's continue with the removal as planned.

The internal import errors are an oversight and should be fixed.

@oscargus oscargus force-pushed the cbookdeprecationremoval branch from fb3a50e to 6f621c1 Compare February 24, 2022 17:55
@oscargus oscargus marked this pull request as ready for review February 24, 2022 17:57
@oscargus oscargus force-pushed the cbookdeprecationremoval branch from 6f621c1 to ebc9d65 Compare February 24, 2022 18:17
@oscargus
Copy link
Member Author

I've fixed the failing tests and added release note. (Azure is not that happy, but I doubt it is my fault.)

@QuLogic QuLogic merged commit 3a994d2 into matplotlib:main Feb 25, 2022
@oscargus oscargus deleted the cbookdeprecationremoval branch February 25, 2022 05:32
@oscargus oscargus added this to the v3.6.0 milestone Aug 1, 2022
@mwaskom
Copy link

mwaskom commented Aug 23, 2022

This removal hard breaks a seaborn function.

Also it's rather unlikely for end users to import MatplotlibDeprecationWarning.

Curious what the basis for this claim was? https://github.com/search?q=%22from+matplotlib.cbook+import+MatplotlibDeprecationWarning%22&type=Code shows 2k+ direct imports of this class, and searching for alternate usage patterns turns up others as well.

I don't think we have or easily could have added a warning on importing the class

I agree that this would have been hard! But IMO that puts the burden on matplotlib to deprecate with a bit more care (e.g., proactively searching for usage in downstream libraries, etc.). You are right that this was "properly documented", but it is mentioned in the middle of a very long list of API changes (there are >4000 words on https://matplotlib.org/stable/api/prev_api_changes/api_changes_3.4.0.html). Downstream libraries have the reasonable expectation that they can rely on unit test coverage to surface deprecation warnings, since matplotlib is usually good about that sort of thing.

In the future, please boost the signal of any silent deprecations.

@tacaswell
Copy link
Member

We should back out the change that broke seaborn and sort out a way to loudly complain. This is a blocking issue for mpl36.

@tacaswell tacaswell mentioned this pull request Aug 23, 2022
Closed
@oscargus
Copy link
Member Author

oscargus commented Aug 23, 2022 •
edited
Loading

Assuming that it is primarily the import of MatlabDeprecationWarning from cbook that is required (and that importing cbook.deprecation is not required), it should at least be pretty straightforward to get it up and running again. Making it scream loud is another problem (which I believe more classes suffer from).

@QuLogic
Copy link
Member

QuLogic commented Aug 23, 2022

Making it scream loud is another problem (which I believe more classes suffer from).

This was not possible in 3.4 as it supported Python 3.6; it is possible now with __getattr__, for which we already have a helper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
API: changes Maintenance
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@oscargus @timhoffm @mwaskom @tacaswell @QuLogic