Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Resolve backend in rcParams.__getitem__("backend").#11896
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
7ca6805 to4b2be5bCompareTest failure seems spurious, and I can't repro it locally. |
4b2be5b to81ddb0aCompare@tacaswell Should be fixed now. |
f639964 toaa25a25Compare... to avoid leaking the non-string sentinel.Changes to `backends/__init__` and `_backend_selection()` were necessaryto avoid import-time cycles.The fallback order in `backend_fallback` slightly changed in that theFooCairo backends now fallback to WxAgg instead of Wx when a wx eventloop is active, but I wouldn't worry too much about it anyways giventhat the Wx backend is deprecated and the correct fallback would beWxCairo to start with.
aa25a25 to036d788Compare... or perhaps not completely fixed :/ |
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 :/ |
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 ( This passes locally. |
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'. |
This behaves correctly in IPython if you let it do the 'auto' thing and if you ask for a specific backend but I am now having issues with the input hooks..... |
ok, the inputhook issues are either an IPython master-branch or pt 2 issue (down-grading both fixed it). |
anntzer commentedAug 23, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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... |
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. |
Now that I think of it Ithink the previous approach (resolve whenwriting 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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seems OK.
In addition to considering the minor comments, you might want to use this PR to update the list of valid backends inmatplotlibrc.template while you are in the neighborhood.
lib/matplotlib/__init__.py Outdated
| 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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@tacaswell you made this change I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this block can be dropped.
| @@ -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 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The docstring formpl.use says the use ofplt.switch_backend is preferred overforce=True; so why is the discouraged form being used in the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removing that note from the docstring is something on my to-do list that I did not get to.
lib/matplotlib/tests/test_text.py Outdated
| print(dict.__getitem__(plt.rcParams, "backend")) | ||
| print(plt.rcParams["backend"]) | ||
| print(plt._backend_mod) | ||
| print(fig.canvas) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that was for debugging the failure on travis, removing it (rebased it out).
This is to ensure that when IPython get the backend inIPython.core.pylabtools.find_gui_and_backend the backend resolution istriggered.
In either case (resolved or not) pass the backend value through. Thismaintains the behavior prior to changing the type of rcParamsOrig to RcParams.
If the context block triggers the auto-resolution of the backendkeep than information around.
This is to prevent the auto resolution of the backend from beingreverted.
571fd0b toaaab933CompareThe 'backend' rcParam is already skipped by the STYLE_BLACKLIST.
7bd83e8 toad91933Compare@anntzer I force pushed an ammended last commit. |
I plan to merge this when CI passes. |
…896-on-v3.0.xBackport PR#11896 on branch v3.0.x
... to avoid leaking the non-string sentinel.
Changes to
backends/__init__and_backend_selection()were necessaryto avoid import-time cycles.
The fallback order in
backend_fallbackslightly changed in that theFooCairo 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