ipykernel icon indicating copy to clipboard operation
ipykernel copied to clipboard

Use `threading.Event` for `_eventloop_set` instead of `anyio.Event`

Open davidbrochart opened this issue 1 year ago • 6 comments

Fixes #1292.

davidbrochart avatar Nov 07 '24 21:11 davidbrochart

Thanks!

minrk avatar Nov 08 '24 07:11 minrk

You will have to test this manually as much of the event loop code isn't tested in CI. There is a screenshot of a simple workflow in #1265.

ianthomas23 avatar Nov 08 '24 08:11 ianthomas23

@ianthomas23 thanks for the pointer! When I run the test from #1265:

In [1]: 1
Out[1]: 1

In [2]: %matplotlib qt
...
/Users/minrk/.virtualenvs/kernel7/lib/python3.11/site-packages/jupyter_console/ptshell.py:787: UserWarning: The kernel did not respond to an is_complete_request. Setting `use_kernel_is_complete` to False.
  warn('The kernel did not respond to an is_complete_request. '

I get a hang, both before and after this PR.

I think this comment means that it not working is a known issue, is that right? Do we have an open Issue to track fixing %gui? Shall we reopen #1235? I'll start investigating a test so we can hopefully keep this from being broken again in the future.

minrk avatar Nov 08 '24 09:11 minrk

I can reproduce the issue.

davidbrochart avatar Nov 08 '24 09:11 davidbrochart

I think this comment means that it not working is a known issue, is that right? Do we have an open Issue to track fixing %gui?

It was working fine for the single-shot use case that nearly all end users want of setting the event loop either at the start or once later via e.g. %matplotlib <whatever>. So we should be able to git bisect back to the new breaking change.

For the more complicated use case of changing the event loop, yes that is a known issue but not recorded as a separate github issue as I was waiting for clarification with a reproducer, and lacking that I have not looked at it further.

Shall we reopen #1235? I'll start investigating a test so we can hopefully keep this from being broken again in the future.

Sure.

Testing is a problem. We can add a test to catch the current failure mode as that is pretty fundamental. But testing that both ipykernel and matplotlib are responsive after a plot window is shown is complicated as we'd have to grab an external window on headless CI, etc, and this is probably not something that anyone wants in the ipykernel repo. I have been thinking about how we test the grey area between matplotlib and ipython/ipykernel/jupyter longer term and I have some ideas, but it is not something we can quickly add now.

ianthomas23 avatar Nov 08 '24 09:11 ianthomas23

Yeah, directly testing that the plot window is responsive is hard. But we should be able to test:

  1. the kernel is responsive after entering the event loop (not true now), and
  2. the gui event loop is responsive by launching e.g. a Qt Timer or some such and checking that it fired while the kernel was idle

I think timers won't fire in the cases where the window is unresponsive. That should get the basics that neither event loop is totally blocking the other which is the usual failure mode when we have a regression here, even if it isn't total coverage.

minrk avatar Nov 08 '24 10:11 minrk