Move cbook.deprecation to _api.deprecation #18657

timhoffm · 2020-10-04T22:34:55Z

PR Summary

This is the first step of removing the deprecation functions from the public interface.

There's still a lot to do in follow-up:

Re-import the submodule contents in _api (as we are currently doing in cbook). This then allows use to write code like _api.warn_deprecated or @_api.delete_parameter()
Change all interal usage to _api.* calls
Deprecate the functions made public via cbook.

The additional steps will generate a lot of trivial internal changes and are best reviewed as a separate PR, which I will prepare after this one is in.

ianhi · 2020-10-05T01:28:47Z

Is much of cbook going to disappear into private api? I think lots of stuff in there is helpful for 3rd party packages. For example, I'm sad to see the deprecation functions go as I've used warn_deprecated (https://github.com/ianhi/mpl-interactions/blob/0.6.1/mpl_interactions/pyplot.py#L6). I tried for a bit to figure out the best way to make deprecation warnings, but in the end it was far easier to piggyback off the work already done in matplotlib. This allowed me to avoid reinventing anything and to avoid picking up a non-matplotlib dependency.

A nice alternative for 3rd party packages would be to include simple advice on how to make warnings to the imagined page giving advice on making a 3rd party package #18496?

jklymak · 2020-10-05T20:39:20Z

@ianhi I think the idea is that its private so we can change at our convenience w/o a huge deprecation dance. If another library wanted to vendor the machinery, that would be great, but we don't want to be in the position of publicly maintaining it.

timhoffm · 2020-10-05T20:49:29Z

@ianhi I understand your motivation. However, this is internal functionality to make matplotlib API evolution easier to manage. It's not part of the functionality, we commit to provide to users or downstream libraries.

While convenient for downstream libraries, having this is a burden on the matplotlib developer side. First, these functions are only designed to work within matplotlib. We don't claim or aspire that they will still work in other contexts. Second, a public API is a fixed interface, that we cannot easily change. This reduces our own capability of further adapting it to our needs.

Even though not recommended you could still import from the private API. But we're not going to guarantee the API, which is basically the whole point of making this private. Possibly the best way for you is to vendor the relevant code part.

dopplershift · 2020-10-05T20:59:00Z

For MetPy we vendored the deprecation functionality. I'd suggest any other projects relying on matplotlib's deprecation code to do that or to look at a proper library for it like debtcollector.

tacaswell · 2020-10-09T02:13:49Z

👍 in principle.

lib/matplotlib/_api/__init__.py

This is needed right now to not re-import cbook from _api.deprecation. It's intended anyway, but changing all occurences in the code is left for another time to keep this PR reasonably sized.

jklymak · 2020-10-23T02:16:15Z

doc/api/_api_api.rst

@@ -0,0 +1,13 @@
+*******************


I'm confused why we are making a documentation page for something we are making private?

This targets contributors. It may be easier to read this in HTML than in plaintext.

Pandas does something similar. See the warning note at https://pandas.pydata.org/docs/reference/index.html?highlight=private.

Do we need similar warnings at the beginning of _api.deprecation?

Yes makes sense. I'll also extend the warning to explicitly mention that these Functions are only for developers/contributors.

We read the docs too!

Sigh, had a stale browser and only saw the first comment when I posted, sorry for the noise.

jklymak

This seems fine to me aside from my question about the docs...

tacaswell · 2020-10-23T15:25:55Z

lib/matplotlib/_api/__init__.py

+
+.. warning:
+
+    This module and its submodules are for internal use only.


Suggested change

This module and its submodules are for internal use only.

This module and its submodules are for internal use only. We may change

the API at any time with no warning. If you want to use these functions in your

own code please vendor them.

I've chosen a slightly different expression. In particular I've left out the vendoring part. First, I'm not sure that less experienced developers know that term. But more importantly, I don't think we should/need to actively promote the possibility of vendoring. A bit philosophical: Bringing up the possibility would already imply that it could be useful for other people, but I'd rather not make any statement about external usage of this code. Users can themselves decide if they want to copy and adapt the code (n.b. some messages explicitly contain "Matplotlib").

tacaswell

I added a suggestion to make the wording a bit more verbose, but happy with this going in with or with out that change.

Anyone can merge this.

timhoffm · 2020-10-26T20:33:01Z

I'll self merge based on @tacaswell's comment (wording still a bit more refined), otherwise we'll collect more and more conflicts.

timhoffm added the Maintenance label Oct 4, 2020

timhoffm added this to the v3.4.0 milestone Oct 4, 2020

timhoffm force-pushed the move-deprecation branch from b4220c3 to 20c92ae Compare October 5, 2020 20:24

timhoffm force-pushed the move-deprecation branch from 20c92ae to c6419cf Compare October 5, 2020 20:52

QuLogic added the status: needs rebase label Oct 9, 2020

timhoffm force-pushed the move-deprecation branch 5 times, most recently from 085d837 to 13ce015 Compare October 13, 2020 21:26

timhoffm removed the status: needs rebase label Oct 13, 2020

QuLogic reviewed Oct 14, 2020

View reviewed changes

lib/matplotlib/_api/__init__.py Outdated Show resolved Hide resolved

timhoffm force-pushed the move-deprecation branch from 13ce015 to 99de4a6 Compare October 14, 2020 21:50

QuLogic approved these changes Oct 15, 2020

View reviewed changes

timhoffm added 5 commits October 19, 2020 10:52

Make _api a subpackage

92095c2

Move module cbook.deprecation to _api.deprecation

9330e5c

Re-add a deprecated cbook.deprecation module for backward compatibility

acdd529

Move cbook._warn_external to _api.warn_external.

5a45fc2

This is needed right now to not re-import cbook from _api.deprecation. It's intended anyway, but changing all occurences in the code is left for another time to keep this PR reasonably sized.

Add documentation for _api module

644872b

timhoffm force-pushed the move-deprecation branch from 99de4a6 to 4fee85b Compare October 19, 2020 09:03

jklymak reviewed Oct 23, 2020

View reviewed changes

jklymak approved these changes Oct 23, 2020

View reviewed changes

tacaswell reviewed Oct 23, 2020

View reviewed changes

tacaswell approved these changes Oct 23, 2020

View reviewed changes

Fix import errors and extend module docs

aec6841

timhoffm force-pushed the move-deprecation branch from 4fee85b to aec6841 Compare October 23, 2020 17:26

timhoffm merged commit 2808916 into matplotlib:master Oct 26, 2020

timhoffm deleted the move-deprecation branch October 26, 2020 20:33

timhoffm mentioned this pull request Oct 26, 2020

Move from @cbook.deprecated to @_api.deprecated #18823

Merged

timhoffm mentioned this pull request Nov 6, 2020

Move cbook._suppress_matplotlib_deprecation_warning() from cbook to _api #18903

Merged

timhoffm mentioned this pull request Jan 5, 2021

Move cbook._check_isinstance() to _api.check_isinstance() #19244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move cbook.deprecation to _api.deprecation #18657

Move cbook.deprecation to _api.deprecation #18657

timhoffm commented Oct 4, 2020 •

edited

Loading

ianhi commented Oct 5, 2020

jklymak commented Oct 5, 2020

timhoffm commented Oct 5, 2020

dopplershift commented Oct 5, 2020

tacaswell commented Oct 9, 2020

jklymak Oct 23, 2020

timhoffm Oct 23, 2020

jklymak Oct 23, 2020

timhoffm Oct 23, 2020

tacaswell Oct 23, 2020

tacaswell Oct 23, 2020

jklymak left a comment

tacaswell Oct 23, 2020

timhoffm Oct 23, 2020

tacaswell left a comment

timhoffm commented Oct 26, 2020


		.. warning:

		This module and its submodules are for internal use only.

-    This module and its submodules are for internal use only.
+    This module and its submodules are for internal use only.  We may change
+    the API at any time with no warning.  If you want to use these functions in your
+    own code please vendor them.

Move cbook.deprecation to _api.deprecation #18657

Move cbook.deprecation to _api.deprecation #18657

Conversation

timhoffm commented Oct 4, 2020 • edited Loading

PR Summary

ianhi commented Oct 5, 2020

jklymak commented Oct 5, 2020

timhoffm commented Oct 5, 2020

dopplershift commented Oct 5, 2020

tacaswell commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell left a comment

Choose a reason for hiding this comment

timhoffm commented Oct 26, 2020

timhoffm commented Oct 4, 2020 •

edited

Loading