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

Use threading.Event for _eventloop_set instead of anyio.Event #1293

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidbrochart
Copy link
Collaborator

Fixes #1292.

@minrk minrk enabled auto-merge (squash) November 8, 2024 07:30
@minrk
Copy link
Member

minrk commented Nov 8, 2024

Thanks!

@ianthomas23
Copy link
Collaborator

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.

@minrk
Copy link
Member

minrk commented Nov 8, 2024

@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.

@davidbrochart
Copy link
Collaborator Author

I can reproduce the issue.

@ianthomas23
Copy link
Collaborator

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.

@minrk
Copy link
Member

minrk commented Nov 8, 2024

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.

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.

_eventloop_set event called from non-async context
3 participants