backend switching -- don't create a public fallback API #11600

anntzer · 2018-07-08T20:52:37Z

Alternative version of #11581, following in particular @timhoffm's comments.

The difference is that instead of allowing assigning a list to rcParams["backend"] and having implicit fallback (an admittedly awkward API), we use a private, magic value as default that triggers the fallback. This should still be enough to fix most issues with downstream packagers.

A public fallback API would come (or not) with python-syntax matplotlibrcs (for backend in ...: try: use(backend); except ImportError: pass).

I think I prefer this version.

See changes documented in the API changes file.

Some followup cleanup (of the now unused old machinery) will come as a
separate PR (left some "FIXME: Remove." comments).

Changes to the build process (namely, getting rid of trying to detect
the default backend in setupext.py) will come as a separate PR.

I inlined pylab_setup into switch_backend (and deprecated the old
version of pylab_setup) because otherwise the typical call stack would
be use() -> set rcParams['backend'] = ... -> switch_backend() ->
pylab_setup(), which is a bit of a mess; at least we can get rid of
one of the layers.

PR Summary

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

tacaswell · 2018-07-08T21:48:05Z

lib/matplotlib/pyplot.py

+                          "tkagg", "wxagg", "agg", "cairo"]:
+            try:
+                switch_backend(candidate)
+            except ImportError:


Does this catch the 'no display' errors you get on a headless server?

It should(?), because _get_running_interactive_framework will return "headless" which is different from what the backend declares. We need the backend import to correctly fail with an ImportError, but I took care of the cases I encountered in the previous PR.

tacaswell · 2018-07-08T21:51:48Z

Modulo fixing the mac tests...

timhoffm · 2018-07-08T22:43:33Z

@anntzer Thank you for considering my comments. It's good not to overload the semantics of rcParams['backend'].

However, I'm afraid I haven't made myself clear there:

I'm not in favor of adding any active functionality to rcParams (i.e. changing the current backend by setting the rcParam).
A list of possible backends as rcParam is basically ok.

Please see my comment #11581 (comment) (which was probably created in parallel to this PR.)

anntzer · 2018-07-08T22:54:37Z

Re: mac failures:
I think they are due to the fact that (unlike all other backends, and following @tacaswell's fears), the OSX backend initializes the NSApp during backend import, rather than totally lazily; then, because we always try to import it first, when we later switch to tkagg, things may go awry. Fixing this shouldn't be too hard, just a matter of moving the initialization to a private function and call it in FigureCanvas_init; @tacaswell do you have time to give it a shot?

@timhoffm I agree that the backend doesn't logically belong in the rcParams, but until python-syntax matplotlibrcs happen (if ever, yada yada), the only thing we can set from matplotlibrc is entries in rcParams, and we definitely want a way to set the backend from matplotlibrc. Once python-syntax matplotlibrc happen, we can always move it out and ask people to use use() (or set_backend, etc.) in their matplotlibrc.py.

Having the entry in the rcParams and the actual backend go out of sync is exactly what I don't want to happen.

timhoffm · 2018-07-08T23:16:46Z

I agree, that we want to set the default backend from matplotlibrc. The current backend should not be part of the rcParams. There's no syncing involved if we make that distinction.

tacaswell · 2018-07-09T02:43:10Z

@tacaswell do you have time to give it a shot?

I don't have a mac to test on....

I agree, that we want to set the default backend from matplotlibrc. The current backend should not be part of the rcParams. There's no syncing involved if we make that distinction.

for better or worse, the current backend being stashed in rcParams in the existing behavior. I think the weird side effects of setting one rcparam (at risk of setting a very poor precedent) is better than breaking that bit of existing behavior.

timhoffm · 2018-07-09T06:35:53Z

I am aware that the current backend being stashed in rcParams is the existing behavior. So far, this is read-only (setting the rcParam when a current backend exists has no effect). This is not great, but both easy to cope with if we want to evolve the API, and likely not used very often.

This PR adds setting-capability through rcParams. IMO it's a movement in the wrong direction. We make more users aware of this poor API. And the added functionality will make it harded to smoothly migrate to a better API later. I strongly suggest not to go down this road.

There are better ways to bring in the backend-switching functionality. Unfortunately, I don't have the time to write the design out now, but I will do so tonight. Please wait with further decisions until then, if at all possible.

anntzer · 2018-07-09T08:08:47Z

@tacaswell You realize that Travis is my main OSX machine? :) Tried to push something, let's see whether it works.
@timhoffm Sure, I'll wait for your design.

anntzer · 2018-07-09T13:30:08Z

Cherry-picked #9993 into this PR as well because the current version of qt_compat fails with NameError (instead of ImportError) when qt bindings are not installed.

timhoffm · 2018-07-09T22:34:42Z

#11612 lays out an alternative API design.

jklymak

This looks good to me. Not really parsing all the backend selection logic, but I assume this works, or will get fixed quickly if it breaks for someone.

jklymak · 2018-07-10T17:06:15Z

lib/matplotlib/backends/qt_compat.py

-
-if 'PySide2' in sys.modules:
-    # user has imported PySide before importing mpl
+# First, check if anything is already imported.


I assume we are prepared to do quick patch releases if this stuff doesn't work on everyone's setup....

jklymak · 2018-07-10T17:08:15Z

Appveyor is of course failing for some reason....

timhoffm

Fine modulo the active rcParams["backend"] stuff.

Side remark: The cherry-pick of #9993 makes reviewing harder - I did not check that parts. Under normal circumstances, it should be reviewed and merged separately.

timhoffm · 2018-07-10T23:21:56Z

doc/api/next_api_changes/2018-06-27-AL.rst

@@ -1,6 +1,9 @@
 Changes to backend loading
 ``````````````````````````

+Assignment to ``rcParams["backend"]`` now sets the backend.  Backends can now


As discussed in #11612 I'm 👎 on making rcParams["backend"] active. This is a new way of API behavior, which we don't want in the long run. There is no loss in functionality, if users still use matplotlib.use() to set the backend.

Can we leave that part out in the PR?

anntzer · 2018-07-11T09:21:39Z

The cherry-pick is due to #11629 (and the fact that this PR explicitly relies on unimportable backends to raise ImportError -- I believe that generically catching any exception is too wide and too likely to hide real bugs). Would be happy for #9993 to be merged in first, or otherwise for #11629 to be fixed separately.

I'm fine with moving the switching logic to use for now. This means that rcParams["backend"] may end up being different from the actual backend in use (if one assigns to it -- note that calling use will still update the entry in the rcParams), which I don't like, but that would still be an improvement wrt. the current situation. Sounds like a reasonable compromise?

@timhoffm Do you want to give a try at implementing this? The pieces are basically all there, just a matter of moving things around.

See changes documented in the API changes file. I inlined pylab_setup into switch_backend (and deprecated the old version of pylab_setup) because otherwise the typical call stack would be `use()` -> `switch_backend()` -> `pylab_setup()`, which is a bit of a mess; at least we can get rid of one of the layers.

To actually make it change backends. This change is required because I reverted some of the more aggressive changes to `mpl.use` (to effectively default to force=True).

It seems to have a bug where `plt.show` hangs when the window is closed.

jklymak · 2018-08-09T16:11:42Z

As noted on gitter, I can't really follow what this is doing, and it seems to need documentation. I won't block if core folks like @tacaswell, @dopplershift and @anntzer are happy with it, and are willing to maintain it.

anntzer · 2018-08-09T16:34:56Z

Basically, when I originally wrote the PR, there were two goals:

don't set the default backend in the build, which causes all sorts of problems to packagers and is usually worked around by the redistributors by hard-wiring some value anyways,
make mpl.use() actually switch backend when it can (i.e. staying within the same GUI toolkit, or to a noninteractive toolkit, or as long as no GUI interactive event loop has started), and throw an exception when it cannot, getting rid of the somewhat nonsensical force and warn kwargs to use.

The second part has been removed after the additional PRs by @tacaswell due to disagreements about the API, which I can't say I'm that happy with (but still thanks for putting together that implementation to move things forward); on the other hand even just having the first part is clearly already an improvement (and not having the second part means that there is less API changes). Moreover, implementing something else in the spirit of the second part remains relatively easy to do now (again, it's mostly a discussion about the API that's needed).

jklymak · 2018-08-09T18:16:19Z

matplotlibrc.template

-## If you omit this parameter, it will always default to "Agg", which is a
-## non-interactive backend.
-backend      : $TEMPLATE_BACKEND
+## If you omit this parameter, the backend will be determined by fallback.


"...by fallback" is vague. What will actually happen if this parameter is omitted?

jklymak · 2018-08-09T18:17:02Z

src/_macosx.m

@@ -267,6 +267,39 @@ - (int)index;

 /* ---------------------------- Python classes ---------------------------- */


Not clear what these changes have to do w/ this PR...

It defers creating the main loop until you actually create a figure. This means you should be able to import pyplot and the use switch_backend as much as you want between any backend until you create a figure.

tacaswell · 2018-08-10T04:10:51Z

I added a whats new for the backend selection logic. Did not add one for the backend switching (despite that being the name of this branch!) because as @anntzer points out there is still disagreement of what that API should look like (and where it should live) so lets not advertise it widely (yet).

efiring

You might address one minor comment, but I think the critical thing is to get this merged so that it gets more testing, and so 3.0rc1 can get out the door. This PR doesn't have to be perfect; it just has to work, and constitute an improvement.

efiring · 2018-08-10T19:28:37Z

lib/matplotlib/__init__.py

+
+    force : bool, optional
+        If True, attempt to switch the backend.  This defaults to
+        false and using `.pyplot.switch_backend` is preferred.


I thought this docstring reference to 'switch_backend' was going to be deleted.

Sorry, I missed that (but did remove it from warning message.

efiring · 2018-08-10T19:33:03Z

lib/matplotlib/rcsetup.py

@@ -242,13 +243,14 @@ def validate_fonttype(s):

 _validate_standard_backends = ValidateInStrings(
    'backend', all_backends, ignorecase=True)
+_auto_backend_sentinel = object()


I'm curious as to why this used as the sentinel, instead of the more typical 'auto' or None.

Because we don't want to expose this as a public API (... at this point yet, at least).

jklymak · 2018-08-10T22:52:06Z

OK, well, four of the most senior devs approved or authored it, and I can't see anything bad in it, so I'm merging. Thanks for the "Whats New" that definitely helped. The documentation issue noted above can be handled post merge if a follow-up PR if deemed necessary before 3.0 is fully released.

timhoffm · 2018-08-13T16:11:52Z

Thanks for taking this step by step and deferring the API discussion to 3.1. I've been on holidays and couldn't take part in the final discussion. However, I fully agree with the approach taken so far. 😄

anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 8, 2018

anntzer added this to the v3.0 milestone Jul 8, 2018

tacaswell reviewed Jul 8, 2018

View reviewed changes

tacaswell previously approved these changes Jul 8, 2018

View reviewed changes

tacaswell mentioned this pull request Jul 8, 2018

Backend switching #9795

Closed

anntzer force-pushed the backend-switching-3 branch from e8f0b22 to 77ba47c Compare July 8, 2018 22:07

anntzer force-pushed the backend-switching-3 branch 5 times, most recently from 04a0de8 to 1d9d0ed Compare July 9, 2018 12:32

anntzer force-pushed the backend-switching-3 branch 3 times, most recently from 93967ac to c0bae18 Compare July 9, 2018 14:59

timhoffm mentioned this pull request Jul 9, 2018

TST: Backend switching API (don't merge) #11612

Closed

jklymak added the status: needs rebase label Jul 10, 2018

jklymak previously approved these changes Jul 10, 2018

View reviewed changes

jklymak mentioned this pull request Jul 10, 2018

backend switching. #11581

Closed

6 tasks

timhoffm reviewed Jul 10, 2018

View reviewed changes

tacaswell force-pushed the backend-switching-3 branch from 64a34ab to 44eb749 Compare August 6, 2018 22:14

anntzer added 3 commits August 6, 2018 18:28

Lazy-init the OSX event loop.

ec77ca2

Don't fail Qt tests if bindings not installed.

d5576e2

tacaswell force-pushed the backend-switching-3 branch from 44eb749 to d5576e2 Compare August 6, 2018 22:29

tacaswell added 2 commits August 7, 2018 22:06

TST: pass force to mpl.use

22508d8

To actually make it change backends. This change is required because I reverted some of the more aggressive changes to `mpl.use` (to effectively default to force=True).

TST: skip wx backend

0ccc19e

It seems to have a bug where `plt.show` hangs when the window is closed.

tacaswell force-pushed the backend-switching-3 branch from b2087dd to 0ccc19e Compare August 8, 2018 03:05

TST: put back wx and qt5cairo tests

29dd9d4

dopplershift approved these changes Aug 8, 2018

View reviewed changes

jklymak reviewed Aug 9, 2018

View reviewed changes

DOC: add whats new

97683a5

efiring approved these changes Aug 10, 2018

View reviewed changes

jklymak merged commit 3479481 into matplotlib:master Aug 10, 2018

anntzer deleted the backend-switching-3 branch August 11, 2018 08:33

anntzer mentioned this pull request Aug 12, 2018

Backend related issues with matplotlib 3.0.0rc1 #11844

Closed

This was referenced Aug 12, 2018

macosx backend won't load #11846

Closed

FIX: macosx framework check #11850

Merged

anntzer mentioned this pull request Aug 13, 2018

get_backend() backward compatibility #11852

Closed

anntzer mentioned this pull request Aug 30, 2018

Propagate changes to backend loading to setup/setupext. #11962

Merged

6 tasks

marcelm mentioned this pull request Sep 19, 2018

Document preference order for backend auto selection #12166

Merged

anntzer mentioned this pull request Oct 22, 2018

In 3.0.0 backend cannot be set if 'get_backend()' is run first #12362

Closed

jklymak mentioned this pull request Nov 12, 2018

Setting macosx as the default backend causes an ImportError MacPython/matplotlib-wheels#6

Closed

anntzer mentioned this pull request May 12, 2020

Add a public backend fallback API #17386

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backend switching -- don't create a public fallback API #11600

backend switching -- don't create a public fallback API #11600

anntzer commented Jul 8, 2018

tacaswell Jul 8, 2018

anntzer Jul 8, 2018

tacaswell commented Jul 8, 2018

timhoffm commented Jul 8, 2018 •

edited

Loading

anntzer commented Jul 8, 2018

timhoffm commented Jul 8, 2018

tacaswell commented Jul 9, 2018

timhoffm commented Jul 9, 2018

anntzer commented Jul 9, 2018

anntzer commented Jul 9, 2018

timhoffm commented Jul 9, 2018

jklymak left a comment

jklymak Jul 10, 2018

jklymak commented Jul 10, 2018

timhoffm left a comment •

edited

Loading

timhoffm Jul 10, 2018

anntzer commented Jul 11, 2018 •

edited

Loading

jklymak commented Aug 9, 2018

anntzer commented Aug 9, 2018

jklymak Aug 9, 2018

jklymak Aug 9, 2018

tacaswell Aug 9, 2018

tacaswell commented Aug 10, 2018

efiring left a comment

efiring Aug 10, 2018

tacaswell Aug 11, 2018

efiring Aug 10, 2018

anntzer Aug 10, 2018

jklymak commented Aug 10, 2018

timhoffm commented Aug 13, 2018

		@@ -267,6 +267,39 @@ - (int)index;

		/* ---------------------------- Python classes ---------------------------- */

backend switching -- don't create a public fallback API #11600

backend switching -- don't create a public fallback API #11600

Conversation

anntzer commented Jul 8, 2018

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Jul 8, 2018

timhoffm commented Jul 8, 2018 • edited Loading

anntzer commented Jul 8, 2018

timhoffm commented Jul 8, 2018

tacaswell commented Jul 9, 2018

timhoffm commented Jul 9, 2018

anntzer commented Jul 9, 2018

anntzer commented Jul 9, 2018

timhoffm commented Jul 9, 2018

jklymak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Jul 10, 2018

timhoffm left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Jul 11, 2018 • edited Loading

jklymak commented Aug 9, 2018

anntzer commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Aug 10, 2018

efiring left a 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 commented Aug 10, 2018

timhoffm commented Aug 13, 2018

timhoffm commented Jul 8, 2018 •

edited

Loading

timhoffm left a comment •

edited

Loading

anntzer commented Jul 11, 2018 •

edited

Loading