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

backend switching. #11581

Closed
wants to merge 1 commit into from
Closed

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 5, 2018

Main part of #9795 (but rewritten quite a bit), labeled as release critical following it.

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.

If the API change ("rcParams['backend'] returns a list as long as pyplot
has not been imported") is deemed unacceptable, we could also make
reading rcParams["backend"] force backend resolution (by hooking
__getattr__).

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

@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 5, 2018
@anntzer anntzer added this to the v3.0 milestone Jul 5, 2018
matplotlib.backends._get_running_interactive_framework()
if (current_framework and required_framework
and current_framework != required_framework):
raise ImportError(
Copy link
Member

Choose a reason for hiding this comment

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

I am worried that by this point we are already broken (due to importing the underlying GUI wrappers which may over-write each other's PyOS_EventHook), but I don't see any way around that short of delaying all of the imports (which I am not sure we can do).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PyOS_InputHook should (hopefully) not be overwritten before the toolkit event loop actually starts; e.g. for PyQt:

from PyQt5.QtWidgets import *
print(input("foo? "))
app = QApplication([])
print(input("foo? "))

even after the QApplication is created, input works fine; overwriting likely only occurs in app.exec_().

Copy link
Member

Choose a reason for hiding this comment

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

But if you the import tk will the Qt windows still be responsive while waiting for user input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes (this can easily be checked by, well, doing it). I'd guess tk also only sets up the input hook when actually starting the event loop.

try:
pyplot.switch_backend(backend)
except Exception:
dict.__setitem__(rcParams, "backend", old_backend)
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be done like this to make sure we un-wind any changes that `switch_backend_ makes to the rcparams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because switch_backend will set rcParams["backend"] too (to keep things consistent), but we don't want this to recurse ad infinitum (so we pre-set the entry -- but undo that if at the end of the day the backend switch was unsuccessful).

Really it's just a complication because of the existence of two entry points (setting rcParams["backend"] and switch_backend) to the same info; it would even be simpler if switch_backend itself was inlined into the validator (but then you have funky action at distance with the proper initialization of pyplot happening in some other module...).

@tacaswell
Copy link
Member

I am a bit concerned about the validator having side effects, but overall 👍

The only way to resolve the list to a single value is to import pyplot and implicitly triggering pyplot on access to rcParams seems much worse than the violation of type stability on get_backend and rcParams['backend'].

@anntzer
Copy link
Contributor Author

anntzer commented Jul 5, 2018

Well, you can comment on #11509 re: validator side effects :)

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.

I'm also uneasy about the side effects. My notion of the rcParams is that they are plain config values, that take effect only the next time you work with them, but they don't affect existing stuff. figure.figsize or figure.constrained_layout.use also do not modify the current figure.

I'm afraid that it will cause confusion for the user which commands are plain settings, and which have an immediate effect.

Is it really necessary to be active here? IMO the user should switch using matplotlib.use and not by setting an rcParam. Internally we can work with that too.

There might actually be a slight confusion as to what rcParams['backend'] actually is. Is it a choice of possible backends to try when you need to instantiate a new one (this notion is reflected in the list-type)? Or should it be the current backend (reflected in the active behavior)? I think it's not good to mix them and would opt to only use it in the former notion. This should really be thought through carefully.

It is now possible to set ``rcParams["backend"]`` to a *list* of candidate
backends.

If `.pyplot` has already been imported, Matplotlib will try to load each
Copy link
Member

@jklymak jklymak Jul 6, 2018

Choose a reason for hiding this comment

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

By ".pyplot has already been imported" do you mean matplotlib.use() hasn't been called? Does pyplot fundamentally have anything to do with this, other than its where 99% of us call matplotlib.use() implicitly?

@tacaswell
Copy link
Member

@jklymak The only place Matplotlib has any opinions about what backend you are using is in pyplot (and is the only place this value matters). When you import .pyplot we look at the value in rcparams['backend'] and then import the machinery from the correct backend module and hook it up so plt.figure() does "the right thing". This is why you currently have to call use before you import pyplot for it to matter. I suspect the a few phases down the line of this will be to keep deferring this as much as possible so that you are not locked in until you actually create a figure (@WeatherGod wants this), but getting there in small steps will be easier to review.

@timhoffm Those are fair points, but the backend is very different than the other settings. It makes sense to have many figures with different sizes, however while (theoretically) it is it technically possible to have more than one GUI framework running in the same process (see #4779 for lots of details on the event loop integration problem) so switching the backend is necessarily a global change (one of the steps is to close all open figures!). If we don't let the backend validator have side effects then we can get our selves to a state where the actually backend being used (if you ask the canvas object of a live figure what it's module is), the result of mpl.get_backend and rcParams are out of sync with each other.

I suspect the history of 'backend' being in rcparams was "we should be able to control the backend from a configuration file -> we already have a configuration file -> rcParams['backend'] is created" despite there being some pretty big semantic difference. 'backend' is not the only one in this boat, see

STYLE_BLACKLIST = {
'interactive', 'backend', 'backend.qt4', 'webagg.port', 'webagg.address',
'webagg.port_retries', 'webagg.open_in_browser', 'backend_fallback',
'toolbar', 'timezone', 'datapath', 'figure.max_open_warning',
'savefig.directory', 'tk.window_focus', 'docstring.hardcopy'}
for the other 'global' rcparams that we do not let the style system change.

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.

If the API change ("rcParams['backend'] returns a list as long as pyplot
has not been imported") is deemed unacceptable, we could also make
*reading* rcParams["backend"] force backend resolution (by hooking
`__getattr__`).
@anntzer
Copy link
Contributor Author

anntzer commented Jul 7, 2018

@tacaswell already answered most of the queries but here are a few more points.

@jklymak I suspect the a few phases down the line of this will be to keep deferring this as much as possible so that you are not locked in until you actually create a figure (@WeatherGod wants this).

AFAICT this is already the case with this PR.

@timhoffm Those are fair points, but the backend is very different than the other settings. It makes sense to have many figures with different sizes, however while (theoretically) it is it technically possible to have more than one GUI framework running in the same process (see #4779 for lots of details on the event loop integration problem) so switching the backend is necessarily a global change (one of the steps is to close all open figures!). If we don't let the backend validator have side effects then we can get our selves to a state where the actually backend being used (if you ask the canvas object of a live figure what it's module is), the result of mpl.get_backend and rcParams are out of sync with each other.

Indeed, keeping mpl.get_backend and rcParams["backend"] and the actual backend in sync is the motivation here. However

  1. I don't think we actually need to close the windows anymore in switch_backend, because there's actually no problem switching from, say, qt5agg to qt5cairo, and actually forbidden switches (from one interactive toolkit to another) will fail earlier (by the explicit check). (Of course, instead of changing the behavior of switch_backend, we can move the logic somewhere else and make switch_backend call it, yada yada.)

  2. I agree that the "assign a list to rcParams["backend"]" behavior is quite weird.

  • At least we should make it always immediately resolve the backend: there's no issue with importing pyplot immediately anymore, because it doesn't prevent backend switching. This way, reading from rcParams["backend"] will always return a single backend.

  • The main reason this was added, though, was to make it possible for matplotlib to ship a default rcfile with the contents (e.g.):

backend: macosx, qt5agg, qt4agg, ...

which will try each backend until one can set up (currently, this selection of the default backend is done at build time, which is completely pointless as the toolkits available on the user's machine can be completely different (cf the patching of the default matplotlibrc by many downstream redistributors)). However, this is just a limitation due to the fact that matplotlibrcs are not actual Python files (cf MEP32); if we had python-syntax matplotlibrc, we could just do

for backend in ["macosx", "qt5agg", "qt4agg", ...]:
    try:
        use(backend); break
    except ImportError:
        continue

which is slighly verbose but the logic is easy enough to understand.

In the mean time, another solution (instead of supporting the assignment of lists, which I agree is a bit ugly) would be to support setting rcParams["backend"] to a special value (e.g. "auto" if we want to make it public, or a private singleton object (my preference)), to trigger this automatic resolution.

@@ -2358,6 +2401,9 @@ def _autogen_docstring(base):
# to determine if they should trigger a draw.
install_repl_displayhook()

# Set up the backend.
switch_backend(rcParams["backend"])
Copy link
Member

Choose a reason for hiding this comment

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

This step locks the backend in, @WeatherGod wants

import matplotlib
matplotlib.use('qt5agg')
import matplotlib.pyplot
matplotlib.use('tkagg')  # this does not warn or fail
plt.figure()  # this makes a tk agg figure and actually does the backend import etc. 

Copy link
Contributor Author

@anntzer anntzer Jul 8, 2018

Choose a reason for hiding this comment

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

No it doesn't, because the QApplication isn't created until a figure is created. Indeed, your snippet works as expected.
Sure, the snippet would fail with ImportError if PyQt5 is not installed, but that seems to be asking a bit too much (just wrap it in a try... except yourself); in fact I think a fast fail is less confusing.

Copy link
Member

Choose a reason for hiding this comment

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

I am apparently confused about the details of various hooks get installed or not.... 🐑

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general nothing should happen until you create a widget; we then start the loop lazily.

@timhoffm
Copy link
Member

timhoffm commented Jul 8, 2018

Maybe I've used too much words to state my intention 😄

IMO the sane API for backends would be this:

  1. There should only be rcParams['default_backend']. This defines the backend(s) pyplot should try when it needs a backend. This may be a list.
  2. There should be functions get_backend() and set_backend(name) to query and set the active backend.

I know, we're not there with the existing API, but in case we agree that my proposal is a desireable target state, we can implement a suitable migration strategy.

@jklymak
Copy link
Member

jklymak commented Jul 10, 2018

marking as WIP because I think #11600 is the canonical version of this...

@anntzer
Copy link
Contributor Author

anntzer commented Jul 11, 2018

Let's just close this as the other API is clearly better (regardless of the debate of whether to make assignment to rcParams["backend"] change the backend, allowing assignment of a list is too weird).

@anntzer anntzer closed this Jul 11, 2018
@anntzer anntzer deleted the backend-switching-2 branch July 11, 2018 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. status: work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants