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

Issue #12786: Create hook for dispatching GUI messages asynchronously #589

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sonthonaxrk
Copy link

@sonthonaxrk sonthonaxrk commented Feb 1, 2021

This fixes this issue I raised: ipython/ipython#12786

And allows you to use async and await on GUI events. Like await button.click()

image


It's just experimental so far. But I haven't found any major issues yet. The only possible problem is where the output from Comm messages gets posted to. If you have a Comm.on_msg callback, that has a print statement, it will probably print to the last modified cell, and not the Cell which the comm was created in.

I don't think this will introduce any regressions.

I've split this into two commits, one introduces the hook. The other implements it. I'd personally be happy merging fist the first commit as it makes doing this in a subclass easier.

This changes the method signatures of the dispatch_shell and dispatch_control method. I hope that's okay.

@sonthonaxrk sonthonaxrk changed the title Issue #12786: Create hook for dispatching messages out of order Issue #12786: Create hook for dispatching GUI messages asynchronously Feb 1, 2021
@sonthonaxrk sonthonaxrk force-pushed the feature/async-comms branch 3 times, most recently from b81f7e2 to 3194e85 Compare February 1, 2021 13:41
@sonthonaxrk
Copy link
Author

Having some trouble reproducing the ubuntu 3.6 failure. I'm on a Macintosh, but I doubt it's OS specific.

Any ideas what's going on there? Exact same requirements. Could it be flakey?

@SylvainCorlay
Copy link
Member

Hey @sonthonaxrk thank you for opening this. This issue has been dicussed a couple of times, but your approach is original, and may be interesting to discuss.

This proposal would change the guarantee that shell messages are processed in order (even between comm messages), which may not be what we want. I think it is interesting to discuss how we could enable that usecase.

@sonthonaxrk
Copy link
Author

sonthonaxrk commented Feb 1, 2021

Thanks for the review @SylvainCorlay.

I've been running this code myself and I haven't found any glaring issues with it.

From a very high level point of view, I think it's worthwhile discussing a design change of the protocol: to allow comms to be executed out of order (like what happens in this PR).

I think this is really useful, and could open up some really cool possibilities with Jupyter. It's really incredible how many non-programmers use Jupyter now, and at a previous job I found myself implementing GUIs in Jupyter to ease their pain. I could absolutely see this becoming a pattern.


Perhaps this could in the interim a hook, like in my first commit, where nothing is actually changed about how the basic kernel works. But users who really wanted this could do:

class AsyncGIUKernel:
    def should_dispatch_immediately(self, msg, *args):
        try:
            msg_type = msg['header']['msg_type']
            if msg_type in self.comm_msg_types:
                return True
        except ValueError:
            pass

        return False

That would at least make subclassing to do this easier.

Another option could be to add an experimental flag, where a user could do the following (and it'd raise a warning for good measure:

>>> from IPython import get_ipython
>>> get_ipython().kernel.set_async_comm = True
UserWarning: Async comms are an experimental feature, use at your own risk.

From my perspective, I would be happy.

@SylvainCorlay
Copy link
Member

FYI @jasongrout.

@SylvainCorlay
Copy link
Member

I have rebased the PR on top of master (which simplifies things a bit since there isn't a priority queue anymore).

@sonthonaxrk
Copy link
Author

sonthonaxrk commented Mar 31, 2021

Hey @SylvainCorlay, thanks for rebasing this. Does this mean we might be able to merge this in some form sometime soon?

@SylvainCorlay
Copy link
Member

@sonthonaxrk if we decide to get this in, it would be a good time, because the next ipykernel release will be a major one.

However, I think this deserves a discussion with others familar with the protocol and widgets, like @minrk and @jasongrout.

@blink1073 blink1073 added this to the 6.0 milestone Mar 31, 2021
@sonthonaxrk
Copy link
Author

If it doesn't get in, I'll probably have to investigate using greenlet to achieve this (but that might be a bit of an esoteric solution to my problem).

@SylvainCorlay
Copy link
Member

This issue was discussed at the Jupyter widgets meeting yesterday, and @jasongrout has concerns about re-ordering messages.

There was further discussion about using the same mechanism as the input request to make blocking calls to the front-end.

cc @minrk - you might be interested in this.

@sonthonaxrk
Copy link
Author

I remember we touched on that, but we couldn't quite think of the risk beyond it not being in the specification. And that's a completely valid concern IMO. However, I do think that some kind of mechanism for async controls might be a good thing. IPython is such a great platform for interactive computing, and it would be great if we could move on from some of the limitations that old UNIX style terminals usually have.

Perhaps I should change the design of this PR. Instead of a boolean to change comms to be asynchronous, how about I create a new queue specifically for asynchronous messaging to the kernel. This would give us the notion of sync and async comms.

The advantage of this is libraries would explicitly have to opt into the improved system.

Perhaps the API for using the asynchronous queue could be as simple as this:

from ipykernel.comm import Comm

# Use comm to send a message from the kernel - async
my_comm = Comm(target_name='my_comm_target', data={'foo': 1}, async=True)

@SylvainCorlay @jasongrout @minrk Do you think that would be more amicable to the goals of the IPython project? :)

@tacaswell
Copy link
Contributor

If I'm reading this right it looks like the comm messages would be executed on the thread that is servicing the zmq socket? That seems worrying to me as if you get badly behaved callbacks you can lock up the zmq socket!

Additionally, promoting all com traffic to running on a background thread also seems like it is going to cause chaos (I would bet a beverage of choice that there is someone out there with an ipywidget that has a callback that is doing something to a Qt window). Adding a second queue + thread for comm messages would solve the "lock zmq" problem, but not this problem.

I suspect that there will either need to be a way for the user code to temorarily give up control back to the loop (I guess you could use await input_from_frontend() and make every thing an async function?) or a way for particualar widgets to opt-into being executed on a background thread.

@Carreau Carreau removed this from the 6.0 milestone Jul 1, 2021
@davidbrochart
Copy link
Collaborator

I suspect that there will either need to be a way for the user code to temorarily give up control back to the loop (I guess you could use await input_from_frontend() and make every thing an async function?)

This is actually what akernel is doing.

@davidbrochart davidbrochart marked this pull request as draft July 15, 2021 11:00
@davidbrochart
Copy link
Collaborator

I just rebased on master.
What I can see using @ilyabo's widget is that this code now works:

# cell 1
from unfolded.map_sdk import UnfoldedMap
unfolded_map = UnfoldedMap(
  mapUUID='ae4f5345-8507-49ec-85f6-a1f8c7bc2219'
)
unfolded_map

# cell 2
layers = await unfolded_map.get_layers()

# cell 3
layers
# shows [Layer(label='Point', id='25smxo6', is_visible=True)]

Cell 2 doesn't hang, and cell 3 displays the right thing. However, in the notebook cell 2 appears as still executing (with [*]), although I can execute cell 3. Indeed, the parent used to publish the idle execution status by the kernel is the one from the comm_msg the widget sent to the kernel (and I can see the comm_msg receiving two idle statuses).
I'm not sure how to fix that, because the parent used is always the one from the last message received on the shell channel.

@davidbrochart
Copy link
Collaborator

I'm not sure how to fix that, because the parent used is always the one from the last message received on the shell channel.

I think this limitation is deeply buried inside ipykernel's architecture, and I don't see a way to work around it.
In akernel, each cell execution is launched as a task, and the parent header is embedded in the task, so we can reply to the correct parent even if several requests are interleaved.

@davidbrochart
Copy link
Collaborator

davidbrochart commented Dec 9, 2021

I just rebased on master. It seems to work fine now, maybe recent changes in ipykernel solve the issue I mentioned earlier.

cc @ilyabo

@davidbrochart
Copy link
Collaborator

Oh, that's because of this commit: 22bd412
I didn't mean to push it because I was experimenting with it, but it is the change that solves the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants