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
Widget persistence: only create comm for widgets having valid widget ids #62
Conversation
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. |
Is the thinking here that a widget model starts comm-less, until a comm is connected? |
The idea is to have a version of |
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 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 |
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. |
No worries, this feature will remain. |
Besides, I think this should be in 4.0. |
4.0 is okay with me. The other devs will probably have an opinion WRT to the other PRs too. |
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. |
Finished with the items listed. |
8ce63f6
to
07cb46d
Compare
@jdfreder I commited a workaround for the |
0b7202e
to
2c45472
Compare
bb59860
to
67d568f
Compare
@jdfreder @jasongrout I rebased the PR and squashed a few commits. |
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... |
This doesn't merge anymore. Can you rebase? @jdfreder once it's mergeable, can you have another review pass? |
Yes, @SylvainCorlay ping me when it's rebased. |
67d568f
to
9330746
Compare
@jdfreder rebased |
Widget persistence: only create comm for widgets having valid widget ids
Thanks! |
@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. |
Yeah it still bothers me, but I figured we should get the PR in so the code doesn't go stale. |
This PR has two part:
comm_info_[request/reply]
message. The workaround works as follow: instead of sending acomm_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.