Async jupyter client #10

davidbrochart · 2020-02-02T11:28:38Z

This is a follow-up of #6. It has to be used with jupyter/jupyter_client#506.

MSeal

Thanks for putting this together! I need to do some more thorough testing, but there are a few things we need to figure out in the meanwhile.

A simple one is that #12 (and other PRs touching execute_cell) will conflict with this PR since it touches most public methods so it'll probably become stale as smaller PRs merge in. If we start something experimental and want more incremental PRs it might make sense for us to work out of a branch.

A larger concern that I'm not sure how to completely bridge is that we need to support python 3.5+ (probably can cut to 3.6+ later in the year). I haven't looked into what the best approach is to support async across such versions when one needs to support synchronous calls back in pre-async python verisons. Perhaps we could build a wheel that has different files imported depending on the python version, though that might be tricky. Another option is to release two modules, nbclient and nbclient-async independently. Open to suggestions on this topic.

MSeal · 2020-02-04T06:23:16Z

nbclient/execute.py

+        loop = get_loop()
+        return loop.run_until_complete(self.async_run_cell(cell, cell_index, store_history))
+
+    async def poll_output_msg(self, parent_msg_id, cell, cell_index, timeout=None):


Will need to do some more manual testing to ensure we correctly timeout, pass execution context, and handle errors cleanly. Some of those patterns are tested in the synchronous path with some assumptions that might not hold for the async version.

Just to explain a bit more how I implemented the asynchronous functionality, there are now two tasks that are run in parallel:

_poll_for_reply: asynchronously awaits a reply on the shell channel (possibly with timeout).

poll_output_msg: asynchronously awaits a message on the IOPub channel for cell execution. We first await as long as no reply is received on the shell channel, and when a reply is received on the shell channel we cancel this task and launch it again with a timeout.

Thanks, that helps!

MSeal · 2020-02-04T06:23:51Z

nbclient/tests/test_execute.py

@@ -270,6 +295,55 @@ def test_many_parallel_notebooks(capfd):
    assert captured.err == ""


+def test_async_parallel_notebooks(capfd, tmpdir):


Nice! Glad these tests are here to validate parallel execution

MSeal · 2020-02-04T06:25:41Z

tox.ini

 deps = .[dev]
-commands = pytest -vv --maxfail=2 --cov=nbclient -W always {posargs}
+commands =
+    /bin/bash -c 'python -m pip install -e git+https://[email protected]/davidbrochart/jupyter_client.git@async_client#egg=jupyter_client'


We can put that in the requirements file to test as a git path without needing to toss it in the commands. That also makes the package install cleaner for local development and indicates the branch is dependent on an unreleased code dependency.

I tried to put it in the requirements file but never managed to make it work, that's why it ended up here (I had an error about pip not being able to parse it).

all good, there's a way to make it happy but we can instead just focus on getting async support merged and released in jupyter_client as a pre-req for this PR to be released.

davidbrochart · 2020-02-04T10:03:31Z

Thanks for reviewing @MSeal. There are a couple of issues related to compatibility of async with older Python versions:

in setup_kernel (which is now async because it awaits start_new_kernel_client which awaits wait_for_ready which awaits get_msg...) there is a yield which leads to a SyntaxError: 'yield' inside async function in Python 3.5. Also in Python 3.5 and 3.6 asynccontextmanager doesn't exist in contextlib.
in the tests we now mock messages asynchronously, but AsyncMock only exists in unittest.mock from Python 3.8.

That being said, I think we should work on fixing these issues and not have a nbclient-async separate module.

SylvainCorlay · 2020-02-04T12:42:09Z

Another option is to release two modules, nbclient and nbclient-async independently.

That seems a bit overkill to me!

davidbrochart · 2020-02-04T14:12:46Z

Trigger CI.

davidbrochart · 2020-02-04T14:20:55Z

@MSeal I managed to get Python compatibility from 3.5 to 3.8, but it looks like the CI dies randomly for test_many_parallel_notebooks. I'm not sure if it's because of low CI resources or if it's a real issue.

davidbrochart · 2020-02-04T14:21:03Z

Trigger CI.

MSeal · 2020-02-04T18:00:05Z

@MSeal I managed to get Python compatibility from 3.5 to 3.8, but it looks like the CI dies randomly for test_many_parallel_notebooks. I'm not sure if it's because of low CI resources or if it's a real issue.

I've seen python 3.5 fail randomly in nbconvert and now in nbclient where the kernel dies unexpectantly due to port conflicts. I believe that's related to ipython no longer releasing 3.5 versions (ipython==7.9.0 vs ipython==7.12.0) and there being some outstanding bugs in ipython 7.9. I think the parallel notebook tests should probably scope to 3.6+ given the ipython issues. However the

E       AssertionError: assert '[IPKernelApp] WARNING | Parent appears to have exited, shutting down.\n' == ''
E         - [IPKernelApp] WARNING | Parent appears to have exited, shutting down.

is new to me -- that looks like something different is happening.

maartenbreddels · 2020-02-06T14:59:55Z

nbclient/execute.py

+        )
+        exec_reply = await self._poll_for_reply(parent_msg_id, cell, exec_timeout)
+        if not task_poll_output_msg.done():
+            task_poll_output_msg.cancel()


Never knew about cancel, can this mean a message can get lost?

I don't think so, because messages are queued anyway in zmq. And we create a new task polling for messages just after that.

But can it cancel, when it's off the zmq queue, say, it's in the json parser. How does it cancel like Ctrl-c.

It can't do anything between two await, so it depends on the implementation. For instance, in zmq.asyncio.Socket.recv_multipart we might await zmq.asyncio.Socket.recv several times and so cancelling in the middle of it can lead to losing a message, you're right.

Another way that I tried out was to leave the first poll_output_msg task running (with no timeout) and launch another one with a timeout when _poll_for_reply is done (and if the first poll_output_msg task is not done), and then asyncio.wait(both_tasks, return_when=asyncio.FIRST_COMPLETED), but it doesn't handle exceptions well (there is a return_when=asyncio.FIRST_EXCEPTION for that).
I can't think of a better solution right now.

SylvainCorlay · 2020-02-06T16:27:19Z

due to port conflicts

@MSeal there is an accurate description of that issue in jupyter/jupyter_client#487 by @JohanMabille.

Basically the whole handshake model of the kernel protocol is an antipattern. The client should not tell the kernel which ports to use. It should only open one socket for the kernel to communicate back the available ports for itself...

davidbrochart · 2020-02-08T12:50:40Z

@maartenbreddels I reworked the polling on the shell and iopub channels. Now the polling on iopub channel task is run only once (with no timeout), and when we have an exec reply we wait_for that task with a timeout, which will cancel the task after the timeout. That way there is no task rescheduling and no risk to loose a message.

maartenbreddels · 2020-02-08T12:52:28Z

Sounds brilliant, I'll review next week! (from mobile phone)

…

On Sat, 8 Feb 2020, 13:50 David Brochart, ***@***.***> wrote: @maartenbreddels <https://github.com/maartenbreddels> I reworked the polling on the shell and iopub channels. Now the polling on iopub channel task is run only once (with no timeout), and when we have an exec reply we wait_for that task with a timeout, which will cancel the task after the timeout. That way there is no task rescheduling and no risk to loose a message. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10?email_source=notifications&email_token=AANPEPM2JSE4K5C5EAYJVL3RB2TCDA5CNFSM4KOZCYWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELFREZI#issuecomment-583733861>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANPEPMG3BQMHVXCYII7SFTRB2TCDANCNFSM4KOZCYWA> .

davidbrochart · 2020-02-12T12:36:40Z

@MSeal jupyter_client now has an async API (jupyter/jupyter_client#506 has been merged). How would you like to proceed? I guess we should make a release of jupyter_client before merging this PR.

MSeal

Thanks for getting this all in place and updated!

meeseeksmachine · 2020-02-28T23:02:35Z

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/welcome-new-jupyter-client-nbclient-maintainer-david-brochart/3467/1

davidbrochart added 11 commits January 29, 2020 14:44

Asynchronous cell execution

0d52672

Keep only async versions of execute related methods

12420d7

Split async_run_cell in 2 tasks

145a038

Longer timeout in test_async_parallel_notebooks

69c21a9

Longer timeout

c57f37c

No timeout

31284ba

Fix linter

3d690cd

poll_period=0

1d49ea9

No timeout

fd8ea1f

Add test_many_async_parallel_notebooks

5be3d95

Fully async with async jupyter_client

c4be851

davidbrochart mentioned this pull request Feb 2, 2020

Add async API jupyter/jupyter_client#506

Merged

davidbrochart added 8 commits February 2, 2020 13:16

Get jupyter_client #506

4cd62ec

Fix pip install

4139f26

-

9f233a8

-

d6fa25c

-

d1ff6b9

Python 3.8 only

6f498f6

-

aa9bbff

-

c0b1157

MSeal reviewed Feb 4, 2020

View reviewed changes

davidbrochart added 3 commits February 4, 2020 11:33

Workaround for AsyncMock so that we support python3.7

81fb8dc

Fix linter

323fe5c

Import asynccontextmanager from async_generator for Python < 3.7

55f36e9

davidbrochart added 2 commits February 4, 2020 14:52

python 3.5 compatibility

4aa3466

Add py35 and py36 tests

16b3f52

davidbrochart closed this Feb 4, 2020

davidbrochart reopened this Feb 4, 2020

davidbrochart closed this Feb 4, 2020

davidbrochart reopened this Feb 4, 2020

davidbrochart added 2 commits February 4, 2020 15:38

Decrease multiprocessing pool to 2 workers

146cf26

Decrease number of async notebook run to 4

3265d5e

maartenbreddels reviewed Feb 6, 2020

View reviewed changes

Rework polling on shell and iopub channels

3afc591

wreise mentioned this pull request Feb 12, 2020

Fix/mapper colors giotto-ai/giotto-tda#277

Merged

davidbrochart added 2 commits February 12, 2020 13:01

Merge with master

f24ecba

Fix linter, rename

9c33393

Pin jupyter_client>=6.0.0

f513ee7

MSeal approved these changes Feb 26, 2020

View reviewed changes

MSeal merged commit d4e09db into jupyter:master Feb 26, 2020

davidbrochart mentioned this pull request Mar 3, 2020

[WIP] Asynchronous cell execution #6

Closed

kevin-bates mentioned this pull request Oct 10, 2020

Why is the kernel client class pinned to AsyncKernelClient? #116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async jupyter client #10

Async jupyter client #10

davidbrochart commented Feb 2, 2020

MSeal left a comment

MSeal Feb 4, 2020

davidbrochart Feb 4, 2020

MSeal Feb 4, 2020

MSeal Feb 4, 2020

MSeal Feb 4, 2020

davidbrochart Feb 4, 2020

MSeal Feb 4, 2020

davidbrochart commented Feb 4, 2020

SylvainCorlay commented Feb 4, 2020

davidbrochart commented Feb 4, 2020

davidbrochart commented Feb 4, 2020

davidbrochart commented Feb 4, 2020

MSeal commented Feb 4, 2020

maartenbreddels Feb 6, 2020

davidbrochart Feb 6, 2020

maartenbreddels Feb 6, 2020

davidbrochart Feb 6, 2020

davidbrochart Feb 6, 2020 •

edited

Loading

SylvainCorlay commented Feb 6, 2020

davidbrochart commented Feb 8, 2020

maartenbreddels commented Feb 8, 2020 via email

davidbrochart commented Feb 12, 2020

MSeal left a comment

meeseeksmachine commented Feb 28, 2020

		@@ -270,6 +295,55 @@ def test_many_parallel_notebooks(capfd):
		assert captured.err == ""


		def test_async_parallel_notebooks(capfd, tmpdir):

Async jupyter client #10

Async jupyter client #10

Conversation

davidbrochart commented Feb 2, 2020

MSeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbrochart commented Feb 4, 2020

SylvainCorlay commented Feb 4, 2020

davidbrochart commented Feb 4, 2020

davidbrochart commented Feb 4, 2020

davidbrochart commented Feb 4, 2020

MSeal commented Feb 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbrochart Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

SylvainCorlay commented Feb 6, 2020

davidbrochart commented Feb 8, 2020

maartenbreddels commented Feb 8, 2020 via email

davidbrochart commented Feb 12, 2020

MSeal left a comment

Choose a reason for hiding this comment

meeseeksmachine commented Feb 28, 2020

davidbrochart Feb 6, 2020 •

edited

Loading