FIX: Don't enable IPython integration if not entering REPL. #14979

Carreau · 2019-08-05T03:53:51Z

IPython can also be used to simply run files; when this is the case we
are not going to be interactive; and we do not want to enable the
eventloop integration. Otherwise someone doing:

$ ipython script.py

Will simply see a window blink, as IPython will exit immediately instead
of showing the window and exit.

I'm not quite sure how to test that are we basically want something to
block forever and wait for user input; so no test added

Closes: ipython/ipython#11837

PR Summary

PR Checklist

Has Pytest style unit tests
Code is Flake 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

IPython can also be used to simply run files; when this is the case we are not going to be interactive; and we do not want to enable the eventloop integration. Otherwise someone doing: $ ipython script.py Will simply see a window blink, as IPython will exit immediately instead of showing the window and exit. I'm not quite sure how to test that are we basically _want_ something to block forever and wait for user input; so no test added Closes: ipython/ipython#11837

Carreau · 2019-08-05T03:54:17Z

This is also related to #14950 so may not be a proper fix.

anntzer · 2019-08-05T07:35:21Z

lib/matplotlib/backend_bases.py

            try:
                mpl.rcParamsOrig["backend"] = mpl.rcParams["backend"]
-                ip.enable_matplotlib()
+                if is_non_interactive_terminal_ipython(ip):


Can you 1) make the function private (leading underscore) and 2) perhaps move the check a bit earlier, e.g. in line 1649 if not ip or _is_non_interactive...(ip)?

anntzer · 2019-08-05T07:35:41Z

thanks! just a minor nitpick.

timhoffm · 2019-08-05T11:24:43Z

lib/matplotlib/backend_bases.py

@@ -1553,6 +1553,20 @@ def _draw(renderer): raise Done(renderer)
            return figure._cachedRenderer


+def is_non_interactive_terminal_ipython(ip):


This does only check attributes of ip, so I feel it should belong to the IPython object and should be implemented there as IPython.is_interactive or similar.

Then one can do:

if ip.is_interactive: ip.enable_matplotlib()

Or maybe even in enable_matplotlib()

def enable_matplotlib(self): if not self.is_interactive: warnings.warn('enable_matplotlib() called on a non-interactive IPython.' 'Matplotlib interactive mode is not enabled.' 'This will raise a RuntimeError in furture versions.') return ...

But then we would only support versions of ipython>=whatever-implements-this.

Which is the reason I did it this way, and also ip.is_interactive make no-sens on non-TerminalInteractiveShell. When running matplotlib in notebook, you will get a different type for ip (ZmqInteractiveShell). And both are subclass of a base class which can be overridden by other implementations.

To be clearer this function test:

is ip created by a terminal IPython.

if 1; is it interactive.

I'm ok with the present approach for back-comatibility.

closes matplotlib#14950

lib/matplotlib/backend_bases.py

ImportanceOfBeingErnest · 2019-08-05T15:32:38Z

Can we just revert the commit that introduced the problem and then rethink this from scratch? I feel like matplotlib should provide the interface and attributes, such that IPython can query all the information it needs to decide what to do; not matplotlib deciding what IPython should do.

tacaswell · 2019-08-05T15:34:32Z

Pushed a commit to address @anntzer's nitpicks and confirmed interacitvely that this fixes #14950

tacaswell · 2019-08-05T16:16:19Z

@ImportanceOfBeingErnest In principle you are correct but we are constrained by some history (as we want IPython and Matplotlib to both work with future / old versions of the other) and the conflict between user expectations and lazyness.

On one hand users want to be able to just import plt and have it "do the right thing" of setting up the input hooks, but when IPython starts we don't know if Matplotlib is going to be used (so we do not want to pre-emptively import Matplotlib and even if we did preemptively import Matplotlib, we don't know which GUI framework the user is going to want so use. Thus, Matplotlib has to tell IPython, as late as possible, "ok now set up the input hook" as that is when we are truely locked into a given backend.

In the past people have had to remember to type %matplotlib to manually enable the connection, but as of 1.5.3 (#6734) we will at pyplot import time poke IPython to set things up, however this does not work for backends that IPyhon does not know about which is what this code is supposed to fix.

The second to last commit changes to using the gentler ip.enable_gui.

The last commit simplifies it further and avoids monkey-patching IPython all together.

Carreau · 2019-08-05T16:52:20Z

Note that the above approach does not preclude to improve and expose more things in IPython; and we also likely want at some point to clear all the old code path.

Matplotlib master is still amazingly workswith IPython 3.0 (released in 2015).

I'm open to remove old compat layer though – maybe we should more carefully comment whcih code path are for older version of IPython :-)

ImportanceOfBeingErnest · 2019-08-05T18:28:33Z

IIUC you want that people can get cairo plots in IPython by just updating matplotlib, without updating IPython? Isn't that a very special case? Like, if I know I need to update matplotlib anyways, I could just update IPython as well.

Carreau · 2019-08-05T18:54:42Z

Like, if I know I need to update matplotlib anyways, I could just update IPython as well.

Maybe you can but not everyboy can; but you can't assume that everyboy will think about this, take the time to do it or has the possibility to do it. IPython also oes have bugs, and we regularly have people that have to get to olde IPython version for some other unrelated bugs. And if you start this way, why not be always up to date will al the libraries ? Why isn't everyone using Python 3.8 two days after it is out ?

This also mean that first we need to get the update in IPython; then release IPython, then release matplotlib.

That also does not help with downstream packagers in Debian and Co. who now may also have to package IPython first.

Packages that works with each other have to work with previous/next versions if you want to have any chance to a be a bit friendly with users.

tacaswell · 2019-08-05T19:20:01Z

This will also be an issues with ipympl.

ImportanceOfBeingErnest · 2019-08-05T19:40:19Z

I really feel I'm missing something here. So all of this is just for the one person on earth who wants to use a non-standard backend and cannot type plt.switch_backend("Qt5Cairo") or whatever they need into the console?

anntzer · 2019-08-05T20:23:33Z

What ipython needs to do is basically read the required_interactive_framework attribute (which is new in mpl3.0 and which I'm proposing, in #14521, to move to somewhere slightly more practical for third-party backend developers (basically they can mostly just ignore it if that PR gets merged)) and enable the correct event loop based on that.

Note that this is not only for qt5cairo, but also for mplcairo and any other backend that may exist out there (of which there may admittedly not be that many).

@ImportanceOfBeingErnest to be honest, it is not clear to me why you think this patch would better live in IPython's codebase rather than in Matplotlib's. For example, if #14521 goes in, IPython would need to carry more or less forever code to handle either location of required_interactive_framework, whereas by having the patch on Matplotlib's side, we can immediately update the IPython integration to use the new location. Also, the complexity of the patch is bascailly the same wherever it lives.

tacaswell · 2019-08-05T21:29:33Z

plt.switch_backend("Qt5Cairo") does not bind the GUI event loop until the first figure is created (so that you can have one GUI in your rc params, but still switch after pyplot is imported and before the first figure is created. This is extremely useful for users who may import things that in turn import pyplot so they do not have to be super careful about import order (and was a very long standing complaint).

Hopefully mplcairo will grow a decent user base and, as I said above, I suspect that this will also be useful for ipympl.

Carreau · 2019-08-05T21:39:11Z

So all of this is just for the one person on earth who wants to use a non-standard backend and cannot type plt.switch_backend("Qt5Cairo") or whatever they need into the console?

Hey, it's not because we have asingle bug report that there is a single person that have the issue. And note the reporter in the IPython issue never said they they couldn't type plt.switch_backend("Qt5Cairo"); they were concerned about teaching python/ipython and matplotlib and reported a difference of behavior between Python and IPython.

And it appear that the same piece of code also trigger issues with users that use plt.ioff(); so it definitively look like there is an issue in the logic.

Also sie note; I'm not a maintainer here and only swing by every now and then, but I'm not quite sure if I need to interprete your comments as a real genuine desire to understand, or a lack of empathy toward other user's needs. I have usually a tough skin and had to fight a lot with the language and cultural barrier when trying to express myself in international projects, but the way you present things can be extremely offputting to anyone reading this thread. New users may not distinguish a contributor from a maintainer; which can make feel like the project just does not care about their use case. Keep in mind that for every people commenting on a thread there might be dozens just lurking.

ImportanceOfBeingErnest · 2019-08-05T22:57:19Z

I am extremely concerned about user's needs. And one of the highest goods is hence the stability of the code-base, i.e. not to break people's code from one version to the next.
Now let's start at the beginning. In matplotlib 3.0.2 the MacOS user who opened ipython/ipython#11837, as well as the Spyder user who opened #14950 were all happy. Up to the point where #12637 got in and was released with 3.1.0. #12637 tried to meet a feature request, namely to have some non-standard backends work in IPython. But it broke the interactive mode.
So my proposal here was to revert that commit that broke things for all the users and maybe rethink about that "one user"'s desire to have Cairo working in IPython. Because that would allow to see it as what it really is: a missing feature. Do we need to provide that feature now? Or can we wait till Matplotlib, as well as IPython provide the necessary framework? Do people want to continue to ride from one makeshift solution to the next or build something sustainable?

Carreau · 2019-08-06T20:56:42Z

And one of the highest goods is hence the stability of the code-base, i.e. not to break people's code from one version to the next.Now let's start at the beginning.[...]. Do we need to provide that feature now? Or can we wait till Matplotlib, as well as IPython provide the necessary framework? Do people want to continue to ride from one makeshift solution to the next or build something sustainable?

I don't disagree that we need stability; but whether or not we had release this in 3.1 or 4.0 it would have been broken; none of us design code to have bugs. Without the experience of this breakage; we would likely had never find this corner case anyway. And we are not going from one makeshift solution to another; we explore what is possible doing what we can; and cleaning up when possible to get better solutions.

Remove the change is also a problem for the same stability reason you give us; if we remove it then user relying on some of those behavior in 3.1 would lose it in 3.2. That's not good either. In the end it's always a question a trade off; maybe reverting will be the solution, but maybe it's not. One thing for sure this is a relatively hard problem, and without experiments we won't get all things right. No plan survives contact with the enemy, and enemy is the rest of the folks that don't work on master.

As usual there is an XKCD for this https://xkcd.com/1172/

tacaswell · 2019-08-09T12:31:11Z

I think this should be merged as-is and I will follow up with a PR that removes the enable_gui logic from pyplot and moves it all to the backend. This will let us not try to install the input hook until we actually need it (which is the first time the user creates the canvas which is when we are really locked to one GUI framework).

anntzer · 2019-08-09T13:11:13Z

Let's give a few days to @ImportanceOfBeingErnest to reply, unless you want to push 3.1.2 out tomorrow...

ImportanceOfBeingErnest · 2019-08-09T13:18:07Z

Oh, I don't think I have anything more to add here. I've made the problem clear. And it seems it's being understood.

anntzer · 2019-08-09T13:38:08Z

ok, let's try this, then :)

lumberbot-app · 2019-08-09T13:38:23Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout v3.1.x
$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 bd98f12d76433dd67c12504a43553ba6e2bf02d5

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am "Backport PR #14979: FIX: Don't enable IPython integration if not entering REPL."

Push to a named branch :

git push YOURFORK v3.1.x:auto-backport-of-pr-14979-on-v3.1.x

Create a PR against branch v3.1.x, I would have named this PR:

"Backport PR #14979 on branch v3.1.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

…f not entering REPL. FIX: Don't enable IPython integration if not entering REPL. (matplotlib#14979) FIX: Don't enable IPython integration if not entering REPL.

…-v3.1.x Backport PR #14979: FIX: Don't enable IPython integration if not ente…

Carreau mentioned this pull request Aug 5, 2019

plt.ioff() not supressing figure generation #14950

Closed

anntzer reviewed Aug 5, 2019

View reviewed changes

anntzer added the third-party integration label Aug 5, 2019

anntzer added this to the v3.1.2 milestone Aug 5, 2019

timhoffm reviewed Aug 5, 2019

View reviewed changes

tacaswell added 2 commits August 5, 2019 08:59

MNT: rename helper to be private

4cde730

FIX: preserve interactive state around ip.enable_matplotlib()

d66a760

closes matplotlib#14950

tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Aug 5, 2019

anntzer reviewed Aug 5, 2019

View reviewed changes

lib/matplotlib/backend_bases.py Outdated Show resolved Hide resolved

MNT: fix spelling of local variable

d762969

tacaswell added 2 commits August 5, 2019 12:00

MNT: try using a simpler way of not messing with mpl state

dc2402d

MNT: simplify and be explicit about what GUI to setup

bc65530

anntzer approved these changes Aug 5, 2019

View reviewed changes

dopplershift approved these changes Aug 5, 2019

View reviewed changes

tacaswell approved these changes Aug 9, 2019

View reviewed changes

anntzer merged commit bd98f12 into matplotlib:master Aug 9, 2019

lumberbot-app bot added the status: needs manual backport label Aug 9, 2019

tacaswell mentioned this pull request Sep 5, 2019

Backport PR #14979: FIX: Don't enable IPython integration if not ente… #15189

Merged

dstansby added a commit that referenced this pull request Sep 5, 2019

Merge pull request #15189 from tacaswell/auto-backport-of-pr-14979-on…

c536fb6

…-v3.1.x Backport PR #14979: FIX: Don't enable IPython integration if not ente…

QuLogic removed the status: needs manual backport label Sep 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Don't enable IPython integration if not entering REPL. #14979

FIX: Don't enable IPython integration if not entering REPL. #14979

Carreau commented Aug 5, 2019

Carreau commented Aug 5, 2019

anntzer Aug 5, 2019

anntzer commented Aug 5, 2019

timhoffm Aug 5, 2019

anntzer Aug 5, 2019

Carreau Aug 5, 2019

timhoffm Aug 5, 2019

ImportanceOfBeingErnest commented Aug 5, 2019

tacaswell commented Aug 5, 2019

tacaswell commented Aug 5, 2019

Carreau commented Aug 5, 2019

ImportanceOfBeingErnest commented Aug 5, 2019

Carreau commented Aug 5, 2019

tacaswell commented Aug 5, 2019

ImportanceOfBeingErnest commented Aug 5, 2019

anntzer commented Aug 5, 2019

tacaswell commented Aug 5, 2019

Carreau commented Aug 5, 2019

ImportanceOfBeingErnest commented Aug 5, 2019

Carreau commented Aug 6, 2019

tacaswell commented Aug 9, 2019

anntzer commented Aug 9, 2019

ImportanceOfBeingErnest commented Aug 9, 2019

anntzer commented Aug 9, 2019

lumberbot-app bot commented Aug 9, 2019

		@@ -1553,6 +1553,20 @@ def _draw(renderer): raise Done(renderer)
		return figure._cachedRenderer


		def is_non_interactive_terminal_ipython(ip):

FIX: Don't enable IPython integration if not entering REPL. #14979

FIX: Don't enable IPython integration if not entering REPL. #14979

Conversation

Carreau commented Aug 5, 2019

PR Summary

PR Checklist

Carreau commented Aug 5, 2019

anntzer Aug 5, 2019

Choose a reason for hiding this comment

anntzer commented Aug 5, 2019

timhoffm Aug 5, 2019

Choose a reason for hiding this comment

anntzer Aug 5, 2019

Choose a reason for hiding this comment

Carreau Aug 5, 2019

Choose a reason for hiding this comment

timhoffm Aug 5, 2019

Choose a reason for hiding this comment

ImportanceOfBeingErnest commented Aug 5, 2019

tacaswell commented Aug 5, 2019

tacaswell commented Aug 5, 2019

Carreau commented Aug 5, 2019

ImportanceOfBeingErnest commented Aug 5, 2019

Carreau commented Aug 5, 2019

tacaswell commented Aug 5, 2019

ImportanceOfBeingErnest commented Aug 5, 2019

anntzer commented Aug 5, 2019

tacaswell commented Aug 5, 2019

Carreau commented Aug 5, 2019

ImportanceOfBeingErnest commented Aug 5, 2019

Carreau commented Aug 6, 2019

tacaswell commented Aug 9, 2019

anntzer commented Aug 9, 2019

ImportanceOfBeingErnest commented Aug 9, 2019

anntzer commented Aug 9, 2019

lumberbot-app bot commented Aug 9, 2019