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: always run a coroutine and work with tornado #71

Merged

Conversation

maartenbreddels
Copy link
Contributor

This is a port of what I use in vaex

This has been running for quite a while on CI, and seems pretty solid. If an event loop is present it will patch it with nested-asyncio and will also patch tornado, which should solve:

Alternative to #67

See also erdewit/nest_asyncio#23 and tornadoweb/tornado#2753

This also checks to see if it is being run in an IPython context (I'm not sure the check makes sense, I just check if it is imported), and will give an error if not v7+, otherwise cells in a notebook may run out of order (a known? issue with older versions of IPython I believe), see also erdewit/nest_asyncio#9

@maartenbreddels maartenbreddels changed the title fix: always run a coroutine using nested-asyncio if needed and work with tornado fix: always run a coroutine and work with tornado May 27, 2020
@maartenbreddels maartenbreddels force-pushed the fix_auto_run_nested_with_tornado branch from 7a3b4ce to 404ce8b Compare May 27, 2020 09:43
nbclient/util.py Outdated
check_ipython()
import nest_asyncio
nest_asyncio.apply()
check_patch_tornado()
Copy link
Member

Choose a reason for hiding this comment

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

We might want to do that only once if we had a loop, not at each coroutine execution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the patch is not applied when tornado is not imported (this happens in vaex, where I first call this function, then start the server, and call this function again). Also the patch is only applied once (it checks if the Future is in the set).

Copy link
Member

Choose a reason for hiding this comment

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

OK, but what about check_ipython() and nest_asyncio.apply()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For check_ipython the same holds (it can be imported later on), and nest_asyncio has a check build in, in the end, simplicity, otherwise we need to check things that nest_asyncio already checks.

@davidbrochart
Copy link
Member

Awesome work @maartenbreddels!

@maartenbreddels
Copy link
Contributor Author

My problem with this PR is that I don't have a test, I have trouble reproducing the original issue. So lets put it to draft first.

@maartenbreddels maartenbreddels marked this pull request as draft May 27, 2020 09:51
nbclient/util.py Outdated Show resolved Hide resolved
nbclient/util.py Outdated Show resolved Hide resolved
nbclient/util.py Outdated Show resolved Hide resolved
@maartenbreddels maartenbreddels force-pushed the fix_auto_run_nested_with_tornado branch 2 times, most recently from afd8113 to 88beb2f Compare May 27, 2020 14:39
@maartenbreddels maartenbreddels marked this pull request as ready for review May 27, 2020 14:39
@maartenbreddels maartenbreddels marked this pull request as draft May 27, 2020 14:45
@maartenbreddels maartenbreddels marked this pull request as ready for review May 27, 2020 14:54
@maartenbreddels
Copy link
Contributor Author

Using the private API asyncio._get_running_loop() is the most portable way it seems to find out of we have a running ioloop.

@davidbrochart
Copy link
Member

Thanks a lot for the tests and the awesome work!

@maartenbreddels maartenbreddels force-pushed the fix_auto_run_nested_with_tornado branch from 09ce53f to a79ae70 Compare May 27, 2020 15:12
@maartenbreddels maartenbreddels marked this pull request as ready for review May 27, 2020 17:37
Copy link
Contributor

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

LGTM, let's give it a try. I am about to pop out an alpha 0.4 release for folks to try some of the recently merged changes this week anyway.

@MSeal MSeal merged commit 2d5cee2 into jupyter:master May 27, 2020
@maartenbreddels maartenbreddels deleted the fix_auto_run_nested_with_tornado branch May 27, 2020 19:01
@MSeal
Copy link
Contributor

MSeal commented May 27, 2020

0.4.0a0 is released fyi

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

Successfully merging this pull request may close these issues.

3 participants