Add comm_info_[request/reply] messages

[SylvainCorlay]

Adding a comm_list_[request/reply] shell message.

[minrk]

Since this is a spec change, the version needs to be bumped.

[minrk]

Since it's a new message, that would be 5.1, not 6.0. Sorry for not specifying.

[SylvainCorlay]

Done.

[jasongrout]

"strings"

[SylvainCorlay]

So is this fine with everyone?

[minrk]

Yes, I think so. I just want to bring it up at the dev meeting, first.

[takluyver]

Fernando, Min and I discussed this some more over lunch in Berkeley today. Some notes:

We're a bit concerned that this API (list the things that are there, then use them) might be susceptible to race conditions. In an analogous example, it's frequently a mistake and sometimes a security vulnerability to check whether a file exists, and then do filesystem operations depending on that result. It's normally more reliable to try doing what you want and check for an error.

A few possible alternatives we came up with:

1. Any message to a comm that the recipient doesn't know about could result in an error message, either comm_close or a new message type, so that the sender knows it failed. This should already happen on comm_open if the target name isn't recognised, so it would be a relatively small change to do it on all comm messages.
2. Comms could have a 'connected' state, and an explicit reconnection mechanism. So when you recreate a comm, it initially starts off disconnected until it has done some handshake with its counterpart.
3. We could say that comms are transient things, and persistence happens at the widget layer. So reconnecting a widget would work by creating a new comm, giving the widget ID. This is by analogy with websockets, where you can't reconnect, only create a new connection. It has the advantage that the reconnection logic is only needed at the widget layer, which can evolve more freely than the general message spec.

Making comms more analogous with websockets may have other benefits - e.g. mpl has an abstraction layer over a comm or a websocket for the webagg backend. And by sticking close to an existing design, we get the benefits of the thought and discussion that has already gone into that, and we can draw inspiration from the ways people are using it.

[takluyver]

Oh, and the postscript to all that: we think there's quite a bit to think about here, so we think that we should hold off any message specs changes for 4.0.

[SylvainCorlay]

1. regarding the possibility of a race condition with this implementation: if you request the list of comms and the list changes before you use it, you may send a few comm messages to closed targets before you receive the comm_close events. In that case, the comm messages are simply disregarded on the Python side. c.f. code.

   (Actually, we were sending a lot of messages to non-existing comms when moving disconnected sliders before Save changes should only save if comm is live jupyter-widgets/ipywidgets#60.) I don't see a security risk here.

2. Rather than a connected attribute, there could be a _closed to be symmetrical. Although with the change that I propose in widget, a comm would simply not be created if there is no back-end counterpart for it (unless you are initiating the creation, with a comm open).

3. Well, this is just reconsidering completely the mapping comm - widget, and would be a major change in the way of thinking about it!

[jasongrout]

Re: 3 - wait, so we're saying that we're reimplementing sockets on top of the IPython message spec on top of websockets on top of html on top of tcp sockets? This is getting pretty crazy...

[takluyver]

Well, this is just reconsidering completely the mapping comm - widget, and would be a major change in the way of thinking about it!

I don't think we really considered reconnection in the design of comms at all, so it's worth thinking about what level that should live at. One thing that came up at lunch is that the relationship between comms and widgets is more where we've ended up as we've explored the concept of widgets, not the result of deliberate design. That shouldn't stop us from thinking about changes like this.

Also, I think that if reconnection was done at the widget layer by creating new comms, you could implement whatever you need for that with no message spec changes, since we've said that the widget protocol is not yet stable. That's a pretty big advantage at the moment.

Re: 3 - wait, so we're saying that we're reimplementing sockets on top of the IPython message spec on top of websockets on top of html on top of tcp sockets? This is getting pretty crazy...

Comms are absolutely another socket-like abstraction, we had talked about this even before we implemented them. None of this discussion is changing that, and this proposal actually keeps them simpler than other ideas we're discussing, because there's no need for them to handle reconnection.

Reasons for building YATLS (yet another thing like sockets):

• We can multiplex many comms without tying up file descriptors or tcp connections.
• They can be tunnelled from the frontend to the kernel without the server needing to know about individual comms.

[SylvainCorlay]

Well, this is just reconsidering completely the mapping comm - widget, and would be a major change in the way of thinking about it!

I don't think we really considered reconnection in the design of comms at all, so it's worth thinking about what level that should live at. One thing that came up at lunch is that the relationship between comms and widgets is more where we've ended up as we've explored the concept of widgets, not the result of deliberate design. That shouldn't stop us from thinking about changes like this.

The way we handle reconnection for the current persistence, is to recreate a new Comm object, and give it the same id. In any case, since the page was reloaded or just opened, a new object must be created anyway. Hence it think that it is the right approach. It is not so much about allowing the same Comm object to be connected / disconnected.

[takluyver]

The way we handle reconnection for the current persistence, is to recreate a new Comm object, and give it the same id. In any case, since the page was reloaded or just opened, a new object must be created anyway. Hence it think that it is the right approach. It is not so much about allowing the same Comm object to be connected / disconnected.

Conceptually, this is still reconnecting to an existing comm, using the ID, not opening a new comm. With what I'm talking about, you wouldn't save comm IDs at all, only widget IDs.

[SylvainCorlay]

The way we handle reconnection for the current persistence, is to recreate a new Comm object, and give it the same id. In any case, since the page was reloaded or just opened, a new object must be created anyway. Hence it think that it is the right approach. It is not so much about allowing the same Comm object to be connected / disconnected.

Conceptually, this is still reconnecting to an existing comm, using the ID, not opening a new comm. With what I'm talking about, you wouldn't save comm IDs at all, only widget IDs.

OK, then, similarly, you need a list of valid widgets when refreshing the page in the same way as you needed a list of valid comms. (The workaround I was going to make is to request this to the widget manager comm target.)

[takluyver]

OK, then, similarly, you need a list of valid widgets when refresing the page in the same way as you needed a list of valid comms.

Yes, or possibly the same kind of logic we were talking about (comm target fails if the widget ID supplied doesn't exist, so a comm_close message comes back). But, crucially, you can define all this in the widget layer, which we've said is unstable, whereas there's a higher barrier to doing it for comms because that involves message spec changes.

[SylvainCorlay]

OK, then, similarly, you need a list of valid widgets when refresing the page in the same way as you needed a list of valid comms.

Yes, or possibly the same kind of logic we were talking about (comm target fails if the widget ID supplied doesn't exist, so a comm_close message comes back). But, crucially, you can define all this in the widget layer, which we've said is unstable, whereas there's a higher barrier to doing it for comms because that involves message spec changes.

The method with the comm_close message back is not what we want IMO, because we want dead widgets, which are part of the document to remain with the "broken link" icon until we have executed the corresponding cell and no widget view remains.. A comm close destroys the model and all the views.

Besides, to get a list of valid widgets, you will need a permanent comm with a fixed id to respond to that kind of widget list request. I find it absurd to go through such contortions while the comm_list message was a clean way to get the information.

[takluyver]

The method with the comm_close message back is not what we want IMO, because we want dead widgets, which are part of the document to remain with the "broken link" icon until we have executed the corresponding cell and no widget view remains.. A comm close destroys the model and all the views.

Well, widgets define what happens on comm_close. If the widget can be reconnected using a new comm, it would seem sensible for 'remove this widget from the page' to be an explicit message, not an implicit action when the comm is closed.

We can go back and forth on specifics all day, but I'm trying to highlight that the way widgets work at the moment is not the way they have to work. We've explicitly said that there's no stability guarantee on them, so that we can keep thinking about how else they might work. So instead of starting from the current architecture of widgets and adding to the message spec to make it work, let's treat the message spec as relatively fixed and think about how best to build widgets on top of it.

If we need to make changes at the comm layer, we can, but that requires a much higher bar of justification than changes to widgets at this point. After yesterday's discussions on the matter, it looks like we need to think more carefully about what abstraction comms provide before we make any changes to them to facilitate reconnecting them.

[SylvainCorlay]

If we need to make changes at the comm layer, we can, but that requires a much higher bar of justification than changes to widgets at this point. After yesterday's discussions on the matter, it looks like we need to think more carefully about what abstraction comms provide before we make any changes to them to facilitate reconnecting them.

The only usecase of comms are widgets so far. Disregarding a change on comm because the motivation is widgets does not make any sense.

[takluyver]

The only usecase of comms are widgets so far.

No, matplotlib uses them, as I mentioned above. But even if that wasn't the case, comms are intended to be a general purpose API that other code can build on. Widgets are the first consumer of that API, but that doesn't mean comms should just do whatever makes things easiest for widgets.

[SylvainCorlay]

Which is essentially a widget.

[jdfreder]

Interjecting, matplotlib should probably use the widget framework, even if it's not synchronizing state, because a big piece of the convenience of the widget framework is the handling of cells and routing of messages. They've probably had to rewrite a lot of things that the widget framework does well.

[SylvainCorlay]

An addition we could do to this could be to add an optional target_name argument to the comm_info_request. When provided, the reply would only contain the comm info for the specified target name.

[jdfreder]

An addition we could do to this could be to add an optional target_name argument to the comm_info_request. When provided, the reply would only contain the comm info for the specified target name.

Do you have a use case for this right now?
Widgets

[SylvainCorlay]

@jdfreder yes, I am redoing a pass on persistence. I think it would be better to filter on the target name beforehand rather than after receiving all the comms.

[SylvainCorlay]

@minrk, as discussed on gitter, I added the ability to filter by target name.

Message type: comm_info_request::

content = {
    # optional, the target name for which the comm info is requested.
    'target_name': str,
}

Message type: comm_info_reply::

content = {
    # A dictionary of the comms, indexed by uuids
    'comms': {
        comm_id: {
            'target_name': str,
        },
    },
}

[ellisonbg]

I haven't been following the technical discussion on this, but I think it is important for this to start moving forward. Originally we told @SylvainCorlay that we would be releasing in a few weeks and then we could merge this right afterwards. The release took longer (just fine) but now that 4.0 is out I think we should move forward with it.

[SylvainCorlay]

Thanks!
Now back from Europe and following up on this. Is this ok to be merged or should I change anything?

[minrk]

Let's see how it goes...

[minrk]

Thanks for your patience, @SylvainCorlay

[SylvainCorlay]

Thanks!

[SylvainCorlay]

How should it go for the PR to the other repos?
I guess we need to wait for jupyter-client 4.1 to be released and and then me to require jupyter-client 4.1 in the ipykernel PR?

[minrk]

Good question. Maybe we want to make dev-kernel run tests with dev-client for a while, again. I know that I want to get a release of kernel out with other fixes before this lands (I probably want to get another release of client without this as well).

[SylvainCorlay]

Let me know what you decide. This may take some time before this trickles down to notebook and widgets.

[minrk]

Since we want to get this tested, I think it makes sense to make ipykernel tests run from client-dev for a bit. If we are going to do releases before this is done, we'll just make a backport branch.

There are other spec changes we probably want to bundle (completions currently underway, possibly a debug discussion) in the same release.

[SylvainCorlay]

OK, I guess you will want to switch to client-dev in ipykernel
independently of the open PR, and I will rebase then.

On Mon, Aug 24, 2015 at 6:43 PM, Min RK [email protected] wrote:

Since we want to get this tested, I think it makes sense to make ipykernel
tests run from client-dev for a bit. If we are going to do releases before
this is done, we'll just make a backport branch.

There are other spec changes we probably want to bundle (completions
currently underway, possibly a debug discussion) in the same release.

—
Reply to this email directly or view it on GitHub
#34 (comment)
.