-
Notifications
You must be signed in to change notification settings - Fork 285
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 asyncio error when opening notebooks #718
Conversation
This is a fix for jupyter/notebook#6164 We are only applying `nest_asyncio` when `run_sync` has been called which means there could be tasks queued that are unpatched e.g. from the original issue: ``` <Task pending coro=<HTTP1ServerConnection._server_request_loop() running at /apps/python3/lib/python3.7/site-packages/tornado/http1connection.py:823> ``` which originates from Tornado. A similar issue was reported in `nest-asyncio`: erdewit/nest_asyncio#22 where the solution is to call `apply` on import so that unpatched tasks do not get created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I see is that we still want to use nest-asyncio only when absolutely needed, because it patches the event loop (it cannot work with e.g. uvloop, and can potentially have side effects...).
@@ -15,7 +19,6 @@ def wrapped(*args, **kwargs): | |||
except RuntimeError: | |||
loop = asyncio.new_event_loop() | |||
asyncio.set_event_loop(loop) | |||
import nest_asyncio # type: ignore | |||
|
|||
nest_asyncio.apply(loop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this needs to be moved into the exception handling so the new loop is patched (assuming the global apply
is retained).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's still needed, but it could have probably gone inside the exception handling
I find this issue has a resemblance to the Windows event loop policy issue that existed for a couple of tornado releases. In #686 @minrk makes a point that this kind of (my words) "global patch" mechanism should be applied at the application layer and not as a side-effect of an import - which makes sense. |
Right, but because it is a patch, we wish we didn't have to use it, and we try to use it only when necessary. The proper solution is to not have to use |
I understand and agree, although the "proper solution is to not have to use At any rate, it sounds like this issue (assuming I'm understanding things) should be applied at the application level and not here. |
I agree that this should be applied at the application level. Let me look into how that might be done. Is it fair to say that notebook is the only application that relies on this functionality? Maybe we should consider removing |
I don't think so, all applications that don't use the async API need this functionality. |
Closing in favor of making the fix in the application instead of the client: jupyter/notebook#6221 |
I don't understand why, but we're seeing this jupyter_client/jupyter_client/utils.py Lines 13 to 20 in 490298e
and we're hitting an exception matching erdewit/nest_asyncio#22 is it not dangerous for this code in
|
Where are you seeing the issue? You should try a similar approach to: jupyter/notebook#6221 @qci-amos And I agree with your points. That's why we had to change approach to patch the loop in the application (in the case in my PR it was notebooks). If we could patch all clients then this code could be removed. |
Unfortunately, I don't have a lot of background to offer right now -- I'm just trying to help others debug their code. I don't understand the context in which this problem arose let alone have a reproducible case. All I can say about my case is they are making an attempt to not use
You don't consider this to be an option?
|
Just to be clear: by initializing I was able to reproduce this issue easily enough using If you aren't using
to your application code to initialize it as early as possible: before launching any tasks. Even better would be migrating your application to only use the asynchronous methods of
I'm not an expert on this either, but my understanding is that this
It would require a long deprecation cycle I believe:
|
Ok, thank you for your comments @dleen! |
This is a fix for jupyter/notebook#6164
We are only applying
nest_asyncio
whenrun_sync
has been calledwhich means there could be tasks queued that are unpatched e.g. from
the original issue:
which originates from Tornado.
A similar issue was reported in
nest-asyncio
: erdewit/nest_asyncio#22where the solution is to call
apply
on import so that unpatched tasksdo not get created.