Widget persistence: only create comm for widgets having valid widget ids

[SylvainCorlay]

This PR has two part:

• it implements a workaround only based on comms to the absence of comm_info_[request/reply] message. The workaround works as follow: instead of sending a comm_info_request, the front-end opens a comm whose target is a widget that sends back the list of live comms and commits suicide.

This initially required the following PRs related to the comm_info_[request/reply] messages: jupyter/jupyter_client#34, ipython/ipykernel#25, jupyter/notebook#166. The code using this proper version is committed and commented out.

• then it deals with persistence:
  • comms are only created for valid ids
  • request_state is not called for invalid ids.
  • create_model is decomposed into the creation of the comm and new_model, which does not create a comm when it is not provided with one.

[minrk]

I know the multi-PR nature of changes like this is annoying, but I think it appropriately reflects the significance of such changes, since there are many consumers of these interfaces that aren't us, now.

[SylvainCorlay]

Until last week, widget persistence did for containers like HBox([Button(), Button()]), or for custom models. (cf #49 and #58), hence I am not too concerned about breaking backward compatibility for particular case of the persistence API.

[jdfreder]

The comm:dead event is not triggered anymore when creating a comm-less model. This event is meant to the case where the comm dies (like for a kernel restart).

Is the thinking here that a widget model starts comm-less, until a comm is connected?

[SylvainCorlay]

The comm:dead event is not triggered anymore when creating a comm-less model. This event is meant to the case where the comm dies (like for a kernel restart).

Is the thinking here that a widget model starts comm-less, until a comm is connected?

The idea is to have a version of create_model that takes a model_id but does not create a comm for it. In the persistence, widget models with model ids that are not in the comm list for the comm_list request will not get a comm.

[SylvainCorlay]

To me, this PR is more of a bug fix / cleanup. The current behavior in case of reload, is that all models are created with the assumption that they have a valid comm, which is not true. Then after they are constructed, comm_live is forcibly set to false, and back to true after the state request...

It causes a lot of bugs for things like widgets that create children in their constructors, because thy recreate new widgets, then receive the old children from the backend state request etc...

The comm_list_[request/reply] thing is really required for widgets is really required to correctly implement persistence.

[jdfreder]

I was thinking from an aesthetic stand point. There will need to be a convenient way for someone to programmatically detect if the widget is connected to the back-end, for the purpose of the broken link icon that indicates a severed connection.

[SylvainCorlay]

No worries, this feature will remain.

[SylvainCorlay]

Besides, I think this should be in 4.0.

[jdfreder]

4.0 is okay with me. The other devs will probably have an opinion WRT to the other PRs too.

[SylvainCorlay]

I think that Min was in favor of the new message. It is a minor bump in the protocol, and getting a list of open comms from the backend is really needed for anything that would use the comm API to be persistent.

[SylvainCorlay]

Finished with the items listed.

[SylvainCorlay]

@jdfreder I commited a workaround for the comm_list. I could open a PR with the first two commits, (without the comm_list function) which are very simple to remove some noise.

[SylvainCorlay]

@jdfreder @jasongrout I rebased the PR and squashed a few commits.

[SylvainCorlay]

One big issue IMO is that even when the new message gets in, we will need to keep using workarounds to be backward compatible with 4.0...

[minrk]

This doesn't merge anymore. Can you rebase? @jdfreder once it's mergeable, can you have another review pass?

[jdfreder]

Yes, @SylvainCorlay ping me when it's rebased.

[SylvainCorlay]

@jdfreder rebased

[jdfreder]

Thanks!

[SylvainCorlay]

@jdfreder you were not sure about the names for new_model / create_model. If you have suggestions, let me know. create_model is the only one we must keep for backward compat.

[jdfreder]

Yeah it still bothers me, but I figured we should get the PR in so the code doesn't go stale.