Adds error handling around install_repl_displayhook and call to enable_gui in pyplot; adds a test in test_backends_interactive.

[turnipseason]

PR summary

Closes #23770. Starting an ipython session with InteractiveShell led to a crash with NotImplementedError being raised. An ImportError needed to be raised from that, allowing for continuation with a different backend.

PR checklist

• "closes #0000" is in the body of the PR description to link the related issue
• new and changed code is tested
  [N/A] Plotting related features are demonstrated in an example
  [N/A] New Features and API Changes are noted with a directive and release note
  [N/A] Documentation complies with general and docstring guidelines

[github-actions]

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

[tacaswell]

Can we catch the NotImplemented error here instead and make no changes to install_repl_displayhook?

[turnipseason]

Sure, I changed it! In light of your next comment -- is this still relevant in general (should I amend my PR)?

[tacaswell]

Yes, I think the technical question is if we

• eat the error
• warn
• re-raise ImportError

[turnipseason]

Clearly I'm no authority on this, but warning and re-raising seem pretty logical to do because it just crashes otherwise.

I also didn't fully understand your comment -- like why should there be no GUI at all with block=True/sleep enabled. The couple of examples I tried all seemed to work (switching to a different backend and continuing as intended). I'm very likely missing something, but is the expected behavior here something different?

[tacaswell]

Now that I see this implemented as I described I'm no longer sure this is best approach 😞 .

I still think the analysis of the problem in #23770 (comment) is correct, however what I did not consider when I wrote that is that it only applies if the user wants to do plt.ion() and work in "interactive mode". If they are happy to not have windows pop-up and be live while they type (and either use plt.show(block=True) or plt.sleep(10) to get temporarily live figures) then this change will prevent users from getting GUIs at all.

On the other hand if we just catch and eat the NotImplemented exception in switch_backends (maybe printing a warning) then we enable users use GUI backends with this limited IPython shell, even if its usage is degraded.

With the plain Python shell this sort of "un-managed" situation is possible so from a parity point of view preventing it is bad. On the other hand, currently the GUI backends do not work with this version of the IPython shell so we are not actually taking anything away, but are making it so things will automatically import and do something sensible.

I can see arguments both ways and think either one would be acceptable, but we should consider both options.

[tacaswell]

So with this change I think it will fallback to agg because we can get IPython to setup up the input hook so that

plt.ion()
plt.show(block=False)

work. On the other hand, we could just eat the exception and give the user a partially functional GUI.

[melissawm]

Hi @turnipseason @tacaswell do we need more discussion here or is there a clear path forward?

@turnipseason do you need help with the current test failures?

[turnipseason]

Hi @turnipseason @tacaswell do we need more discussion here or is there a clear path forward?

@turnipseason do you need help with the current test failures?

Hi @melissawm ! Sorry for not replying sooner. I tried understanding what's going on but still don't quite get it, so I'm not sure I can contribute much to the discussion, unfortunately. When I tried running it, both (eating/not eating the exception) ways worked just fine and neither defaulted to the non-interactive background. They also never seemed to enter the interactive python mode though (I couldn't type in the console, even though the input was captured and shown after I closed the GUI), so I'm not sure if I just wrote something incorrectly.

With regards to the test failures -- yes, I think I do.
They seem to be coming from the tests I wrote, since it calls for IPython in the subprocess and the tests fail with a ModuleNotFoundError because there's no IPython. Should I check for the existence of IPython in the subprocess and skip the test somehow if is isn't found since this issue concerns IPython in the first place? Or rewrite them completely?

[ksunden]

@turnipseason

yes you can use pytest.importorskip("IPython")

I would also perhaps suggest using the _run_helper for running your code as a subprocess, that will allow the code to look like a normal python function, rather than being included as strings.

[ksunden]

Did the rebase, which was just caused by a comment being reflowed adjacent to some changed lines

Test failures on azure macos are the newly added test

[QuLogic]

The run helper passes check=True, which causes CalledProcessError to be raised; I don't understand why this is checking the return value, which should be a subprocess.CompletedProcess.

[turnipseason]

You're right. I just thought we had to check for it. Should I leave the test at this, then? It seems to do the same thing (fail without the fix, pass with the fix).

def test_fallback_to_different_backend():
    pytest.importorskip("IPython") 
    _run_helper(_fallback_check, timeout=_test_timeout)

[QuLogic]

All exceptions are errors; the assert can never be False, so there's no need for it.

[turnipseason]

Ok then, thank you, I'll leave it without the assert!

[ksunden]

Test failures on Azure MacOS are still the newly added test in the latest revision.

Additionally, needs a rebase for the meson build system change to fix Circle.

[QuLogic]

Do you have any thoughts on this PR, @ianthomas23?

[ianthomas23]

Do you have any thoughts on this PR, @ianthomas23?

I think it is a somewhat unusual use case that would be reasonable to "won't fix" but given that the fix is simple, localised and evidently a good thing to do anyway then I would approve it. It needs a rebase but that looks relatively straightforward.

One complication is that there is another unprotected use of install_repl_displayhook in pyplot.ion():

matplotlib/lib/matplotlib/pyplot.py

Lines 724 to 728 in 9489b93

	stack = ExitStack()
	stack.callback(ion if isinteractive() else ioff)
	matplotlib.interactive(True)
	install_repl_displayhook()
	return stack

and we cannot repeat the fix here as we are not switching a backend. But I don't think it should stop this PR.