Resolve backend in rcParams.getitem("backend"). #11896

anntzer · 2018-08-20T07:11:49Z

... to avoid leaking the non-string sentinel.

Changes to backends/__init__ and _backend_selection() were necessary
to avoid import-time cycles.

The fallback order in backend_fallback slightly changed in that the
FooCairo backends now fallback to WxAgg instead of Wx when a wx event
loop is active, but I wouldn't worry too much about it anyways given
that the Wx backend is deprecated and the correct fallback would be
WxCairo to start with.

attn @tacaswell, xref #11844.

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 · 2018-08-20T08:41:33Z

Test failure seems spurious, and I can't repro it locally.

anntzer · 2018-08-20T20:59:32Z

@tacaswell Should be fixed now.
TLDR: now pyplot had already been imported when pytest_configure is being called (due to the resolution system), so that call to use() needed force=True.

... to avoid leaking the non-string sentinel. Changes to `backends/__init__` and `_backend_selection()` were necessary to avoid import-time cycles. The fallback order in `backend_fallback` slightly changed in that the FooCairo backends now fallback to WxAgg instead of Wx when a wx event loop is active, but I wouldn't worry too much about it anyways given that the Wx backend is deprecated and the correct fallback would be WxCairo to start with.

anntzer · 2018-08-21T09:13:46Z

... or perhaps not completely fixed :/

anntzer · 2018-08-21T19:01:14Z

Well, now I'm stuck with not being able to repro the failure locally (again) and rcParams["backend"] not matching the actual backend in the CI (per print-debugging), as feared in the API discussion of the original PR :/

tacaswell · 2018-08-22T19:33:23Z

Let see if that fixes it. I think that some of these fixes are overlapping, but defense in depth seems like a good call here.

Turns out we were keeping state in three places we were keeping the backend state around (rcParams, rcParamsDefault, and rcParamsOrig).

This passes locally.

tacaswell · 2018-08-22T20:03:24Z

Getting closer to the problem, it looks like the rcparam tests are re-setting back to the auto sentinal which is then resolving to qt5agg and never getting reset back to 'agg'.

tacaswell · 2018-08-22T20:20:27Z

This behaves correctly in IPython if you let it do the 'auto' thing

16:16 $ ipython
Python 3.8.0a0 (heads/master:28853a249b, Aug 22 2018, 13:47:23) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.0.0.dev -- An enhanced Interactive Python. Type '?' for help.

In [1]: %matplotlib                                                                                                                                                                                                                                           
Using matplotlib backend: Qt5Agg

In [2]:

and if you ask for a specific backend

16:17 $ ipython
Python 3.8.0a0 (heads/master:28853a249b, Aug 22 2018, 13:47:23) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.0.0.dev -- An enhanced Interactive Python. Type '?' for help.

In [1]: %matplotlib tk                                                                                                                                                                                                                                        

In [2]: import matplotlib                                                                                                                                                                                                                                     

In [3]: matplotlib.get_backend()                                                                                                                                                                                                                              
Out[3]: 'TkAgg'

In [4]:

but I am now having issues with the input hooks.....

tacaswell · 2018-08-22T22:18:27Z

ok, the inputhook issues are either an IPython master-branch or pt 2 issue (down-grading both fixed it).

anntzer · 2018-08-23T07:57:52Z

The whole thing turned out to be more complicated than expected (hehe), but the additional fixes by @tacaswell look good, so I'm "approving" them (well, can't do this on my own PR). Of course that should need at least one external reviewer...

tacaswell · 2018-08-24T14:15:18Z

The complexity we hit is what @timhoffm (correctly) predicted would be a problem, however I think we have no choice but to absorb this complexity as we can not break compatibility with older versions of IPython.

anntzer · 2018-08-24T14:45:44Z

Now that I think of it I think the previous approach (resolve when writing to the rcParams, not when reading from it) was still simpler (even if using the sentinel object rather than a list, because that object essentially never exists in the rcParams, this being the source of the issues). After all the tests did pass on the old PR IIRC. Of course there were the other issue pointed out by @timhoffm re: whether "active" rcParams would be a good API.

In any case things appear to work as they are (thanks for figuring it out).

efiring

Seems OK.
In addition to considering the minor comments, you might want to use this PR to update the list of valid backends in matplotlibrc.template while you are in the neighborhood.

efiring · 2018-08-26T20:32:50Z

lib/matplotlib/__init__.py

-                         if k not in STYLE_BLACKLIST})
+        rcParams.update({k: rcParamsOrig[k] for k in rcParamsOrig
+                         if k not in STYLE_BLACKLIST and k != 'backend'})
+        rcParams['backend'] = dict.__getitem__(rcParamsOrig, 'backend')


I think this is contrary to what the docstring says; if so, the docstring needs an update, and either there or in a comment it would be good to say why this exception is being made.

@tacaswell you made this change I think?

I think this block can be dropped.

efiring · 2018-08-26T20:40:18Z

lib/matplotlib/testing/__init__.py

@@ -36,7 +36,7 @@ def setup():
                "Could not set locale to English/United States. "
                "Some date-related tests may fail.")

-    mpl.use('Agg', warn=False)  # use Agg backend for these tests
+    mpl.use('Agg', force=True, warn=False)  # use Agg backend for these tests


The docstring for mpl.use says the use of plt.switch_backend is preferred over force=True; so why is the discouraged form being used in the tests?

The docstring should be updated (really there is no (actual) difference between use and switch_backend anymore after this PR (other than whether we silently or loudly fail or emit a warning) so I'd rather not point to switch_backend, but I know @tacaswell did add some pointers to it at some point so I'll let him decide.

Removing that note from the docstring is something on my to-do list that I did not get to.

efiring · 2018-08-26T20:42:20Z

lib/matplotlib/tests/test_text.py

+    print(dict.__getitem__(plt.rcParams, "backend"))
+    print(plt.rcParams["backend"])
+    print(plt._backend_mod)
+    print(fig.canvas)


We don't usually include print calls in the tests, do we? Are these left over from debugging, or are they intentional? If the latter, given that they are unusual, do they merit a comment?

that was for debugging the failure on travis, removing it (rebased it out).

This is to ensure that when IPython get the backend in IPython.core.pylabtools.find_gui_and_backend the backend resolution is triggered.

In either case (resolved or not) pass the backend value through. This maintains the behavior prior to changing the type of rcParamsOrig to RcParams.

If the context block triggers the auto-resolution of the backend keep than information around.

This is to prevent the auto resolution of the backend from being reverted.

The 'backend' rcParam is already skipped by the STYLE_BLACKLIST.

tacaswell · 2018-08-27T23:25:10Z

@anntzer I force pushed an ammended last commit.

tacaswell · 2018-08-27T23:25:27Z

I plan to merge this when CI passes.

…("backend").

…896-on-v3.0.x Backport PR #11896 on branch v3.0.x

anntzer force-pushed the auto-backend-resolution branch from 7ca6805 to 4b2be5b Compare August 20, 2018 07:45

tacaswell added this to the v3.0 milestone Aug 20, 2018

anntzer mentioned this pull request Aug 20, 2018

tests should force $MPLBACKEND to be unset and $MATPLOTLIBRC to point to an empty file (loading the default rc) #11902

Closed

anntzer force-pushed the auto-backend-resolution branch from 4b2be5b to 81ddb0a Compare August 20, 2018 20:58

anntzer force-pushed the auto-backend-resolution branch 3 times, most recently from f639964 to aa25a25 Compare August 21, 2018 07:46

anntzer force-pushed the auto-backend-resolution branch from aa25a25 to 036d788 Compare August 21, 2018 08:14

tacaswell approved these changes Aug 22, 2018

View reviewed changes

efiring approved these changes Aug 26, 2018

View reviewed changes

tacaswell added 8 commits August 26, 2018 23:00

TST: add a force to a use call to be safe

ce0c5e0

API: change type of matplotlib.rcParamsOrig from dict -> RcParams

adf696f

This is to ensure that when IPython get the backend in IPython.core.pylabtools.find_gui_and_backend the backend resolution is triggered.

FIX: when resolving the auto-backend also update rcParamsOrig

1e8788d

API: Do not have rc_file_defaults import resolve backends

ce04937

In either case (resolved or not) pass the backend value through. This maintains the behavior prior to changing the type of rcParamsOrig to RcParams.

MNT: special case the handling of 'backend' in rc_context

7dffd5b

If the context block triggers the auto-resolution of the backend keep than information around.

MNT: RcParams.__setitem__ refuses to set the backend sentinel

07b6c90

This is to prevent the auto resolution of the backend from being reverted.

FIX: when calling use do not trigger auto-backend resolution

2c1f52c

MNT: special case 'backend' in rc_file

75cc617

TST: add check at end of auto-fixture that backend is still agg

aaab933

anntzer force-pushed the auto-backend-resolution branch from 571fd0b to aaab933 Compare August 26, 2018 21:00

tacaswell added 2 commits August 27, 2018 15:09

DOC: do not point to pyplot from __init__.py

a2bc1b9

MNT: do not special-case backend in rc_file_defaults or rc_file

ad91933

The 'backend' rcParam is already skipped by the STYLE_BLACKLIST.

tacaswell force-pushed the auto-backend-resolution branch from 7bd83e8 to ad91933 Compare August 27, 2018 23:20

tacaswell merged commit 32ad86f into matplotlib:master Aug 28, 2018

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Aug 28, 2018

Backport PR matplotlib#11896: Resolve backend in rcParams.__getitem__…

46614d3

…("backend").

meeseeksmachine mentioned this pull request Aug 28, 2018

Backport PR #11896 on branch v3.0.x #11953

Merged

tacaswell added a commit that referenced this pull request Aug 28, 2018

Merge pull request #11953 from meeseeksmachine/auto-backport-of-pr-11…

18ac317

…896-on-v3.0.x Backport PR #11896 on branch v3.0.x

anntzer deleted the auto-backend-resolution branch August 28, 2018 06:17

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

Resolve backend in rcParams.getitem("backend"). #11896

Resolve backend in rcParams.getitem("backend"). #11896

anntzer commented Aug 20, 2018

anntzer commented Aug 20, 2018

anntzer commented Aug 20, 2018

anntzer commented Aug 21, 2018

anntzer commented Aug 21, 2018

tacaswell commented Aug 22, 2018

tacaswell commented Aug 22, 2018

tacaswell commented Aug 22, 2018

tacaswell commented Aug 22, 2018

anntzer commented Aug 23, 2018 •

edited

Loading

tacaswell commented Aug 24, 2018

anntzer commented Aug 24, 2018

efiring left a comment

efiring Aug 26, 2018

anntzer Aug 26, 2018

tacaswell Aug 27, 2018

efiring Aug 26, 2018

anntzer Aug 26, 2018

tacaswell Aug 27, 2018

efiring Aug 26, 2018

anntzer Aug 26, 2018 •

edited

Loading

tacaswell commented Aug 27, 2018

tacaswell commented Aug 27, 2018

Resolve backend in rcParams.__getitem__("backend"). #11896

Resolve backend in rcParams.__getitem__("backend"). #11896

Conversation

anntzer commented Aug 20, 2018

PR Summary

PR Checklist

anntzer commented Aug 20, 2018

anntzer commented Aug 20, 2018

anntzer commented Aug 21, 2018

anntzer commented Aug 21, 2018

tacaswell commented Aug 22, 2018

tacaswell commented Aug 22, 2018

tacaswell commented Aug 22, 2018

tacaswell commented Aug 22, 2018

anntzer commented Aug 23, 2018 • edited Loading

tacaswell commented Aug 24, 2018

anntzer commented Aug 24, 2018

efiring left a comment

Choose a reason for hiding this comment

efiring Aug 26, 2018

Choose a reason for hiding this comment

anntzer Aug 26, 2018

Choose a reason for hiding this comment

tacaswell Aug 27, 2018

Choose a reason for hiding this comment

efiring Aug 26, 2018

Choose a reason for hiding this comment

anntzer Aug 26, 2018

Choose a reason for hiding this comment

tacaswell Aug 27, 2018

Choose a reason for hiding this comment

efiring Aug 26, 2018

Choose a reason for hiding this comment

anntzer Aug 26, 2018 • edited Loading

Choose a reason for hiding this comment

tacaswell commented Aug 27, 2018

tacaswell commented Aug 27, 2018

Resolve backend in rcParams.getitem("backend"). #11896

Resolve backend in rcParams.getitem("backend"). #11896

anntzer commented Aug 23, 2018 •

edited

Loading

anntzer Aug 26, 2018 •

edited

Loading