Adds error handling around install_repl_displayhook #25870

turnipseason · 2023-05-12T10:58:23Z

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 · 2023-05-12T14:38:41Z

lib/matplotlib/pyplot.py

-    install_repl_displayhook()
+    try:
+        install_repl_displayhook()
+    except ImportError as err:


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

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

Yes, I think the technical question is if we

eat the error

warn

re-raise ImportError

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 · 2023-05-12T15:01:46Z

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 · 2023-05-14T20:57:32Z

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 · 2023-05-23T18:28:53Z

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 · 2023-05-23T18:54:27Z

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 · 2023-06-08T19:13:05Z

@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 · 2023-09-12T18:06:09Z

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 · 2023-10-18T21:35:43Z

lib/matplotlib/tests/test_backends_interactive.py

+    response = _run_helper(_fallback_check, timeout=_test_timeout)
+    assert response != subprocess.CalledProcessError


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.

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)

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

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

ksunden · 2023-10-27T17:21:20Z

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 · 2024-10-17T23:25:45Z

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

ianthomas23 · 2024-10-18T10:18:32Z

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.

# This is a combination of 3 commits. Fixes the issue 23770 and adds a test in test_backends_interactive.py Changed the test to skip when IPython can't be imported and use _run_helper for the subprocess. Removed the redundant assert statement.

QuLogic · 2024-11-16T01:04:21Z

OK, then I've rebased and squashed.

ianthomas23 · 2024-11-18T09:15:44Z

I'd be happy to approve this but there are some CI failures that may or may not be related.

QuLogic · 2024-11-19T07:04:38Z

It looks like the new test is hitting the long-standing actions/setup-python#649

ianthomas23

Approved as CI failure unrelated.

github-actions bot reviewed May 12, 2023

View reviewed changes

tacaswell reviewed May 12, 2023

View reviewed changes

tacaswell modified the milestones: v3.8.0, v3.7.2 May 12, 2023

QuLogic modified the milestones: v3.7.2, v3.7.3 Jul 5, 2023

github-actions bot added the status: needs rebase label Aug 8, 2023

QuLogic modified the milestones: v3.7.3, v3.8.0 Sep 9, 2023

ksunden force-pushed the bug-fix-for-issue-23770 branch from 673da5e to e98d502 Compare September 12, 2023 18:05

github-actions bot removed the status: needs rebase label Sep 12, 2023

ksunden previously approved these changes Sep 12, 2023

View reviewed changes

ksunden modified the milestones: v3.8.0, v3.8.1 Sep 12, 2023

QuLogic reviewed Oct 18, 2023

View reviewed changes

ksunden added status: needs revision status: needs rebase labels Oct 27, 2023

ksunden modified the milestones: v3.8.1, v3.8.2 Oct 27, 2023

github-actions bot removed the status: needs rebase label Oct 27, 2023

ksunden removed this from the v3.8.2 milestone Nov 17, 2023

ksunden added this to the v3.8.3 milestone Nov 17, 2023

tacaswell added the status: orphaned PR label Jan 25, 2024

tacaswell marked this pull request as draft January 25, 2024 14:48

tacaswell modified the milestones: v3.8.3, future releases Jan 25, 2024

github-actions bot added the status: needs rebase label Feb 5, 2024

QuLogic changed the title ~~Adds error handling around install_repl_displayhook and call to enable_gui in pyplot; adds a test in test_backends_interactive.~~ Adds error handling around install_repl_displayhook Nov 16, 2024

QuLogic force-pushed the bug-fix-for-issue-23770 branch from 5c468c3 to 321b769 Compare November 16, 2024 01:03

github-actions bot added topic: pyplot API and removed status: needs rebase labels Nov 16, 2024

QuLogic removed status: needs revision status: orphaned PR labels Nov 16, 2024

QuLogic marked this pull request as ready for review November 16, 2024 01:04

ianthomas23 approved these changes Nov 20, 2024

View reviewed changes

tacaswell modified the milestones: future releases, v3.11.0 Dec 16, 2024

tacaswell merged commit fa92cf9 into matplotlib:main Dec 16, 2024
43 of 45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds error handling around install_repl_displayhook #25870

Adds error handling around install_repl_displayhook #25870

turnipseason commented May 12, 2023

github-actions bot left a comment

tacaswell May 12, 2023

turnipseason May 12, 2023

tacaswell May 12, 2023

turnipseason May 12, 2023

tacaswell commented May 12, 2023

tacaswell commented May 14, 2023

melissawm commented May 23, 2023

turnipseason commented May 23, 2023

ksunden commented Jun 8, 2023

ksunden commented Sep 12, 2023

QuLogic Oct 18, 2023

turnipseason Oct 19, 2023

QuLogic Oct 20, 2023

turnipseason Oct 20, 2023

ksunden commented Oct 27, 2023

QuLogic commented Oct 17, 2024

ianthomas23 commented Oct 18, 2024

QuLogic commented Nov 16, 2024

ianthomas23 commented Nov 18, 2024

QuLogic commented Nov 19, 2024

ianthomas23 left a comment

		response = _run_helper(_fallback_check, timeout=_test_timeout)
		assert response != subprocess.CalledProcessError

Adds error handling around install_repl_displayhook #25870

Adds error handling around install_repl_displayhook #25870

Conversation

turnipseason commented May 12, 2023

PR summary

PR checklist

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented May 12, 2023

tacaswell commented May 14, 2023

melissawm commented May 23, 2023

turnipseason commented May 23, 2023

ksunden commented Jun 8, 2023

ksunden commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksunden commented Oct 27, 2023

QuLogic commented Oct 17, 2024

ianthomas23 commented Oct 18, 2024

QuLogic commented Nov 16, 2024

ianthomas23 commented Nov 18, 2024

QuLogic commented Nov 19, 2024

ianthomas23 left a comment

Choose a reason for hiding this comment