-
Notifications
You must be signed in to change notification settings - Fork 58
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
[WIP] Asynchronous cell execution #6
Conversation
nbclient/execute.py
Outdated
polling_exec_reply = True | ||
|
||
while more_output or polling_exec_reply: | ||
await asyncio.sleep(0) |
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.
Out of curiosity, what is the purpose of this await?
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.
It just yields, so that the event loop can run other tasks.
I don't quite understand how this works: the part where it makes sense to await is the kernel client events, but we're not actually awaiting on anything there, as far as I can see. |
The purpose of this PR is just to be able to execute several notebooks in parallel in an async context. Previously it could only be done using threads, because this while loop is blocking. |
nbclient/execute.py
Outdated
|
||
# Avoid exceeding the execution timeout (deadline), but stop | ||
# after at most 1s so we can poll output from iopub_channel. | ||
timeout = self._timeout_with_deadline(1, deadline) |
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.
@akhmerov I think I see what you mean. In practice it allows for some degree of concurrency, because of this 1s timeout, so other cells can be run every second.
The user-facing |
Trigger CI. |
@akhmerov although the tests sometimes fail in the CI (due to very slow machines I guess), I think this is ready to be reviewed. Because |
Closing as #10 has been merged. |
This PR allows for asynchronous cell execution. I just duplicated (with an
async_
prefix) and made asynchronous therun_cell
method and all its callers for now.An option would be to keep only the
async
version of the methods and provide a user-facing (non-async)execute
method that would accept aasynchronous=False
parameter. IfFalse
,execute
would under the hood emulate a blocking call by runningasync_execute
in the event loop withrun_until_complete()
.