-
Notifications
You must be signed in to change notification settings - Fork 506
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
Async cell execution #541
Async cell execution #541
Conversation
This can't work with Python 3.5 because of Jinja async support (see pallets/jinja#1058). |
Running
So less time, and more cpu usage 🚀 |
I cannot seem to break it yet, if you rebase on master, you'll also get the |
499e6c6
to
86916d8
Compare
Trigger CI. |
can't wait for this to land -- both significant performance gains at the "cost" of considerably cleaner code --- duly impressive |
5514bab
to
52ce077
Compare
Trigger CI. |
Trigger CI. |
The Jupyter server app has to use an AsyncMappingKernelManager. If that is acceptable, this PR is ready to be merged. |
… AsyncKernelClient
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.
Awesome work, really glad to see a lot of code removed. I wonder if we want or should try to go for nbconvert5 compatibility or not, if possible.
Mosty small comments.
@@ -1,4 +1,4 @@ | |||
{%- extends 'display_priority.tpl' -%} |
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.
With jupyter/nbconvert#1173 we could again use the tpl, if we care about backwards compatibility
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 reverted to .tpl
.
km.client_class = 'jupyter_client.asynchronous.AsyncKernelClient' | ||
self.executor = VoilaExecutor(nb, km=km, config=self.traitlet_config) | ||
self.executor.kc = km.client() | ||
self.executor.kc.start_channels() |
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 am/was a bit worried about this. I remember when I just started adding an async client that the jupyter_server part used the same client. Maybe that was indeed because I configured it to be async.
Are you saying that both jupyter_server and voila have their own client to the kernel, jupyter_server does whatever it does, and we do it async?
Did you check if https://github.com/voila-dashboards/voila/blob/master/notebooks/basics.ipynb works with both the voila app and server extension?
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.
Yes it works both with the voila app and the server extension. I'm not sure I understand what you mean, Voila has its own kernel anyway, right?
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.
Well, the kernel is also known to jupyter server, which is responsible for the WebSocket connection. So jupyter server had a connection to the kernel as well (actually it may be in parallel to us)
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.
could we use nbclient's async context manager async with self.async_setup_kernel()
instead of manually doing
self.executor.kc = km.client()
self.executor.kc.start_channels()
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 think the whole PR is 'atomic' enough to be 1 commit, shall we squash and merge?
|
||
@property | ||
def environment(self): | ||
self.enable_async = True |
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.
Maybe add a comment to say what this is?
Weird, there is a test failure over a new line character on Python 3.8 - and also I cannot restart the build. |
Trigger CI. |
Why Can't I restart CI from the travis UI? |
No idea. |
🎉 |
This uses the asynchronous cell execution mode of
nbclient
, which requires an async client fromjupyter_client
. Also,nbconvert
has to enable async in Jinja.