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

Async cell execution #541

Merged
merged 20 commits into from
May 6, 2020
Merged

Conversation

davidbrochart
Copy link
Member

This uses the asynchronous cell execution mode of nbclient, which requires an async client from jupyter_client. Also, nbconvert has to enable async in Jinja.

@davidbrochart
Copy link
Member Author

This can't work with Python 3.5 because of Jinja async support (see pallets/jinja#1058).

This was referenced Feb 28, 2020
@maartenbreddels
Copy link
Member

Running $ time py.test tests/app/ before and after this PR

py.test tests/app/  21.50s user 5.80s system 51% cpu 53.168 total
py.test tests/app/  16.38s user 4.25s system 87% cpu 23.649 total

So less time, and more cpu usage 🚀
(I did rebase on master, since it has an extra test)

@maartenbreddels
Copy link
Member

I cannot seem to break it yet, if you rebase on master, you'll also get the tests/app/many_iopub_messages_test.py test, which is an important one to pass, since it tests one of the issues of #534, but it passes locally, great work!

@davidbrochart
Copy link
Member Author

Trigger CI.

@davidbrochart davidbrochart reopened this Mar 10, 2020
@jf---
Copy link

jf--- commented Mar 12, 2020

can't wait for this to land -- both significant performance gains at the "cost" of considerably cleaner code --- duly impressive

@davidbrochart davidbrochart force-pushed the nbclient branch 7 times, most recently from 5514bab to 52ce077 Compare March 26, 2020 09:31
@davidbrochart
Copy link
Member Author

Trigger CI.

@davidbrochart
Copy link
Member Author

Trigger CI.

@davidbrochart davidbrochart reopened this Apr 23, 2020
@davidbrochart
Copy link
Member Author

davidbrochart commented Apr 23, 2020

The Jupyter server app has to use an AsyncMappingKernelManager. If that is acceptable, this PR is ready to be merged.

@davidbrochart davidbrochart changed the title [WIP] Async cell execution Async cell execution Apr 23, 2020
Copy link
Member

@maartenbreddels maartenbreddels left a 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' -%}
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted to .tpl.

setup.py Show resolved Hide resolved
voila/handler.py Show resolved Hide resolved
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()
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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)

Copy link
Member

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()

.github/workflows/build.yml Outdated Show resolved Hide resolved
@maartenbreddels maartenbreddels mentioned this pull request May 4, 2020
Copy link
Member

@maartenbreddels maartenbreddels left a 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?

setup.py Show resolved Hide resolved
voila/execute.py Outdated Show resolved Hide resolved

@property
def environment(self):
self.enable_async = True
Copy link
Member

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?

@SylvainCorlay
Copy link
Member

Weird, there is a test failure over a new line character on Python 3.8 - and also I cannot restart the build.

@davidbrochart
Copy link
Member Author

Trigger CI.

@davidbrochart davidbrochart reopened this May 6, 2020
@SylvainCorlay
Copy link
Member

Why Can't I restart CI from the travis UI?

@davidbrochart
Copy link
Member Author

No idea.

@SylvainCorlay SylvainCorlay merged commit 1c9ca89 into voila-dashboards:master May 6, 2020
@SylvainCorlay
Copy link
Member

🎉

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.

4 participants