Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix eventloop integration with anyio #1265

Merged
merged 4 commits into from
Aug 16, 2024
Merged

Conversation

ianthomas23
Copy link
Collaborator

Fixes #1235.

This fixes GUI event loop integration so that it works following the recent switch to using anyio, so that Matplotlib output correctly displays in separate Windows and both the kernel and plot respond to subsequent input. Such functionality has never been explicitly tested in ipykernel, so I have tested it manually on Linux, macOS and Windows with various combinations of qt, tk and osx Matplotlib backends and Jupyter lab, console, qtconsole and spyder.

Here's a screenshot:

Screenshot 2024-08-09 at 15 36 27

The plot can be panned and zoomed for example, which cannot be shown in such a screenshot.

Summary of changes:

  1. Switch from using shell_stream to shell_socket as the former is no longer used.
  2. To check if there are ZMQ messages pending switch from using shell_stream.flush(limit=1) to shell_socket.get(zmq.EVENTS) & zmq.POLLIN) > 0.
  3. If an eventloop is set (via enable_gui) after the kernel has started, a new anyio.Event is used to trigger calling enter_eventloop. This allows the synchronous enable_gui to trigger the asynchronous enter_eventloop.
  4. The same event is used to close the waiting task if the kernel is stopped without an eventloop being set.
  5. The nested calls of advance_eventloop and schedule_next are replaced with a simple async loop.

These changes are intentionally minimal to reduce the danger of breaking downstream code.

I have disabled test_trio_loop as the other trio-based tests are disabled and we don't appear to support it yet, although as it is one of the async backends supported by anyio this shouldn't be too onerous. That is separate work to this though. I also noted that TrioRunner still exists but is not used any where.

@ianthomas23 ianthomas23 added the bug label Aug 9, 2024
@ianthomas23
Copy link
Collaborator Author

Minimum version tests are failing because in anyio prior to 4.2.0 an Event can only be instantiated from within the async loop (agronholm/anyio#651).

@ianthomas23
Copy link
Collaborator Author

I have bumped the minimum anyio version from 4.0.0 (August 2023) to 4.2.0 (December 2023). Now the only test failures are the same as those occurring on the main branch, so this is now ready for review.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ianthomas23
Copy link
Collaborator Author

Thanks @blink1073

@ianthomas23 ianthomas23 merged commit 1ca8f2c into ipython:main Aug 16, 2024
26 of 32 checks passed
@ianthomas23 ianthomas23 deleted the event_loops branch August 16, 2024 08:10
@jdranczewski
Copy link
Contributor

Hey, thanks so much for implementing these changes! I noticed this would be a problem back in #1079 (comment) (and #1079 (comment) to a lesser extent), but couldn't figure out a way to make the sync-async communication work within anyio. Event seems like a good way to do it! Sorry for not making a proper issue, I got too busy at work.

A small problem with this implementation is that AnyIO events are single-use (https://anyio.readthedocs.io/en/stable/synchronization.html#events) - so when previous versions of ipykernel allowed entering and exiting eventloops using the %gui magic, this version will not work if a user tries to exit and re-enter (or change to a different kind of GUI loop). This is not a super common application, but to my understanding this does occur in Spyder when the user changes the graphics backend - Spyder is currently pinned to ipykernel < 7.0, so this wouldn’t be an immediate problem, but illustrates a broken use case.

This feels like it could be an incremental change to this PR, so I didn’t know if I should open a new issue. Not sure how best to solve it - perhaps a loop in _wait_to_enter_eventloop?

On a different note, the tk eventloop seems to be broken in Notebooks on Windows in my testing. Perhaps because it uses a nested asyncio loop? Or it could be some problem with do_one_iteration. This can be tested by running %matplotlib tk or %gui tk - subsequent cell executions don’t work for me. Perhaps this may be worth it’s own issue. The loop works in the IPython Console, but I think that’s because the Console uses different hooks than the ZMQ shell to run these eventloops: https://github.com/ipython/ipython/tree/128bd582b9894b8ae9ff577aafe13f5fdb39c3cb/IPython/terminal/pt_inputhooks

@ianthomas23
Copy link
Collaborator Author

@jdranczewski My apologies, I didn't realise Spyder supported re-entering event loops. The fix isn't quite as simple as looping in _wait_to_enter_eventloop as we're currently using the same Event to tell the event loop that the kernel is stopping, but I will take a look at it.

I'd personally prefer two new issues as then we have better traceability between problem and solution. I can create them but then they will probably be ipykernel/matplotlib biased whereas if you can create them they should be more appropriately spyder-ish.

Do you have a simple workflow to reproduce the re-entrant problem in Spyder? I've tried this:

In [1]: %matplotlib qt
In [2]: import matplotlib.pyplot as plt
In [3]: plt.plot([1,3,2])
<check plot window is displayed and is interactive, then close it>
In [4]: %gui
In [5]: %gui qt
In [6]: plt.plot([2,3,1])
<check plot window is displayed and in interactive>

to confirm it used to work but is currently broken, but you may have something better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AnyIO commit breaks Matplotlib display in separate windows
3 participants