async support

[minrk]

allow handlers to return anything acceptable to gen.maybe_future

this should include tornado/asyncio coroutines and Futures

counterpart to ipython/ipython#11155

closes jupyter/notebook#3397

[minrk]

In action: asyncio integration works by default with asyncio already running:

[Carreau]

The following restart and run all leads to a sql error when storing history

import asyncio
await asyncio.sleep(1)

1+1

ERROR! Session/line number was not unique in database. History logging moved to new session 14506

[Carreau]

That also leads the the busy/idle indicator to be wrong.

[Carreau]

Got also this weird things which looks like killing the kernel:

[(await get_build(i))['@type'] for i in range(first, first+10)]

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
cell_name in async-def-wrapper()

TypeError: must be str, not int

[Carreau]

I'm starting to be convinced that submitting cells should block submission of subsequent cells unless said otherwise i have the following:

# cells 1
from collections import Counter
c = Counter([(await get_build(i))['@type'] for i in range(first,last)])

# cell 2
c

Cell 1 is ran async, thus cell 2 execute immediately and raise an error c is undefined.

[axil]

Hi, @minrk. Trying to launch some async code in jupyter notebook.

Running latest jupyter-notebook (5.5.0) with your version of ipykernel (run-async branch), getting this:

What am I doing wrong?

Update:
Got it. IPython should also be customized (ipython/ipython#11155).

A happy beta-tester so far :)

[Carreau]

@minrk, do you remember the discussion we had at scipy about execution messages being consumed while code is already running ? Could you give me some pointers on how to fix this ?

I've also pushed some compatibility with IPython 7.0.

[Carreau]

I have a solution which I'm sure is wrong:

--- a/ipykernel/kernelbase.py
+++ b/ipykernel/kernelbase.py
@@ -23,6 +23,7 @@ from signal import signal, default_int_handler, SIGINT
 import zmq
 from tornado import ioloop
 from tornado import gen
+from tornado.locks import Semaphore
 from zmq.eventloop.zmqstream import ZMQStream

 from traitlets.config.configurable import SingletonConfigurable
@@ -38,6 +39,13 @@ from jupyter_client.session import Session

 from ._version import kernel_protocol_version

+from collections import defaultdict
+
+# shell handlers are now coroutine (for async await code),
+# so we may not want to consume all the message and block while it yields
+_msg_locks = defaultdict(Semaphore)
+
+
 class Kernel(SingletonConfigurable):

     #---------------------------------------------------------------------------
@@ -233,7 +241,9 @@ class Kernel(SingletonConfigurable):
             self.log.debug("%s: %s", msg_type, msg)
             self.pre_handler_hook()
             try:
-                yield gen.maybe_future(handler(stream, idents, msg))
+                lock = _msg_locks[msg_type]
+                with (yield lock.acquire()):
+                    yield gen.maybe_future(handler(stream, idents, msg))
             except Exception:
                 self.log.error("Exception in message handler:", exc_info=True)
             finally:
@@ -376,7 +386,6 @@ class Kernel(SingletonConfigurable):
     @gen.coroutine
     def execute_request(self, stream, ident, parent):

I'm going to assume the locks will of course not always be release in the order they've been taken right ?

See also the new PR #11265 on IPython side that fix and document a couple of other things.

[Carreau]

One other thing I'd like to figure out when using IPykernel is how to run user code in other eventloops like trio and curio. Which so far I've managed to do this way, but block the tornado eventloop:

diff --git a/ipykernel/ipkernel.py b/ipykernel/ipkernel.py
index 129d163..9a283cc 100644
--- a/ipykernel/ipkernel.py
+++ b/ipykernel/ipkernel.py
@@ -214,7 +214,16 @@ class IPythonKernel(KernelBase):
             def run_cell(*args, **kwargs):
                 return shell.run_cell(*args, **kwargs)
         try:
-            res = yield run_cell(code, store_history=store_history, silent=silent)
+            # TODO: here we need to hook into the right event loop to run user
+            # code using trio/curio.
+            from IPython.core.interactiveshell import _asyncio_runner
+            if shell.loop_runner is _asyncio_runner:
+                res = yield run_cell(code, store_history=store_history, silent=silent)
+            else:
+                @gen.coroutine
+                def run_cell(*args, **kwargs):
+                    return shell.run_cell(*args, **kwargs)
+                res = yield run_cell(code, store_history=store_history, silent=silent)
         finally:
             self._restore_input()

I've heard that trio has some asyncio integration support, so it would be nice to have better integration.

[minrk]

do you remember the discussion we had at scipy about execution messages being consumed while code is already running ?

Yes, it was to switch the message handler triggers from on_recv callbacks to while true coroutines:

while True:
    msg = await socket.recv_multipart()
    await self.dispatch...

This would require pyzmq >= 16 for asyncio support in .recv_multipart.

Alternately, a smaller change could be to keep the on_recv triggers, but hand off to a coroutine, so that message processing is kept in order:

async def on_recv(msg):
    await self.msg_queue.put(msg)

async def process_queue():
    while True:
        msg = await self.msg_queue.get()
        await self.dispatch(msg)

This would have the slight possible downside that messages would be popped off the zmq queue into Python before they are handled. Not sure if that's a real issue, though. This pattern would give us the option to run the message handlers in an IO thread, since handoff could be threadsafe. This might be valuable if we want to switch runners.

One other thing I'd like to figure out when using IPykernel is how to run user code in other eventloops like trio and curio.

I think we ought to be able to use the existing eventloop integration for qt, etc. to do this. That's where I would start.

[Carreau]

I've updated this to work with the latest iteration of ipython/ipython#11265

[minrk]

@Carreau I've re-added the queue and fixed some handling of ExecutionResult objects, and now it seems to work with my testing for asyncio and trio, at least.

[Carreau]

Thanks ! I'll have a look ok later. Hopping for an IPython 7.0 beta in the next couple of weeks. Do we want to release IPykernel same time as IPython ? We could do a joint beta.

…

On Tue, Sep 4, 2018, 17:28 Min RK ***@***.***> wrote: @Carreau <https://github.com/Carreau> I've re-added the queue and fixed some handling of ExecutionResult objects, and now it seems to work with my testing for asyncio and trio, at least. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#323 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUez_4M4gWRnk61D-qo1HceHo2Xdthnks5uXpwngaJpZM4UMXqc> .

[Carreau]

Thanks I had a chance to look at the code. Should the aborts also flush the newly created queue instead of just looking at ZMQ ? I suppose that's what the current failure are from.

[minrk]

ok, I think the last hurdle is dealing with eventloop integration and the message queue. Now that handling messages is async, we cannot allow the eventloop integration to work the way it does, witch pausing the main eventloop and pumping kernel.do_one_iteration(). Instead, we need eventloop integration to work the other way - tornado is unconditionally and always running, and we pump the app eventloop while tornado is idle. That should eliminate the flushing issues.

[minrk]

This now relies on ipython/ipython#11289

[Carreau]

TODO: remove I guess ?

[Carreau]

The test failure seem to appear at the time of @takluyver's input transformer pr, more especially
( ipython/ipython@f55a4a5 ; ipython/ipython#11041)

A bit tricky to bisect as one need to switch PTK version.

It may be a miss-test, as I'm not sure why range? would send a set_next_input.

Payload on 6.0.0, where the set_next_input is wrong anyway as it sets only one line:

{
  "status": "ok",
  "execution_count": 1,
  "user_expressions": {},
  "payload": [
    {
      "source": "set_next_input",
      "text": "   x=range",
      "replace": false
    },
    {
      "source": "page",
      "data": {
        "text/plain": "\u001b[0;31mInit signature:...           type\n"
      },
      "start": 0
    }
  ]
}

Payload on master:

{
  "status": "ok",
  "execution_count": 1,
  "user_expressions": {},
  "payload": [
    {
      "source": "page",
      "data": {
        "text/plain": "\u001b[0;31mInit signature:...           type\n"
      },
      "start": 0
    }
  ]
}

[Carreau]

Test was added pre big split, currently at commit 71fca97 (used to be ipython/ipython@e9db6d5), in PR ipython/ipython#4363, see issue ipython/ipython#1349 for background of why this was done, and why set_next_input was send originally.

[Carreau]

Also I've started to draft some Blog post (https://medium.com/@mbussonn/ipython-7-0-async-repl-a35ce050f7f7) still really early, and many parts to re-write, in particular if we want to make IPython/ipykernel release at the same time.

[Carreau]

The change in behavior of ? is very much on purpose if you look at the code.
That is to say, the set_next_input is called only iif the ? is alone on its line.

I'm not sure if this really make sens (@takluyver ?), I'm tempted to say that the set_next_input makes lots of sens, especially in multiline....

[Carreau]

I'm not sure if this really make sens (@takluyver ?), I'm tempted to say that the set_next_input makes lots of sens, especially in multiline....

Meh, it's broken also on 6.4 and before in a different way, and only insert the current line, so InputTransformer2 behavior suits me. I've push a commit that fixes the test (test the actual behavior instead of testing via the fact that ? use the behavior)

[Carreau]

Test are now passing !

[minrk]

Change in set_next_input multi-call behavior seems fine to me, I'm happy with the updated test. Shall we land this and get ready to make prereleases?

[Carreau]

Sounds like a plan. There is a couple more PR in IPython I want to land. Mostly the autoreload one, and matplotlib DISPLAY one, but I'd like for it to be tested. I'm just refrainig from making a release (of IPython) as apparently @ivanov never made one, so I want to leave him the horror.

…

On Mon, Sep 10, 2018, 10:56 Min RK ***@***.***> wrote: Change in set_next_input multi-call behavior seems fine to me, I'm happy with the updated test. Shall we land this and get ready to make prereleases? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#323 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUez-s4oDYz9jBl0lBjwb9t-X_8qJikks5uZilAgaJpZM4UMXqc> .

[minrk]

OK! Landing this one, and we can make 5.0 beta to go with the IPython 7.0 beta

[Carreau]

Side, dumb question. Should we leapfrog 5.0 and 6.0 and do a 7.0 to sync the version number of IPython and IPykernel ?