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

Make handling comm messages optional in a kernel connection. #6929

Merged
merged 5 commits into from Aug 2, 2019

Conversation

jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 1, 2019

References

Code changes

The comm message protocol currently has implicit assumptions that only
one kernel connection is handling comm messages. This option allows a
kernel connection to opt out of handling comms.

See jupyter/jupyter_client#263

This should fix the error where ipywidgets stops working if you open a notebook and a console to the same kernel, since the console automatically closes all comms since it does not have the comm target registered.

User-facing changes

The first connection to a kernel will handle comm messages, and subsequent connections will ignore comm connections by default. This is better than the status quo, where often subsequent connections would often close comm connections for everyone.

This also prevents many errors in the console from comm messages not being processed in a kernel connection without comm targets. For example, you could see these from the help menu kernel connection, or the console kernel connection.

Backwards-incompatible changes

None. I think all parameters introduced are optional with sensible defaults, and the test suite utility deletions are from a private package.

The connectToComm function does throw a new error if the kernel does not handle comms. But its signature hasn't changed in a backwards-incompatible way.

The comm message protocol currently has implicit assumptions that only
one kernel connection is handling comm messages. This option allows a
kernel connection to opt out of handling comms.

See jupyter/jupyter_client#263

This fixes the error where ipywidgets stops working if you open a notebook and a console to the same kernel, since the console automatically closes all comms since it does not have the comm target registered.
@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor Author

Should we additionally have console kernel connections always disable comms? We could do this so that if you opened a console, then a notebook associated with the console, the notebook would have comms enabled.

@jasongrout
Copy link
Contributor Author

Should we additionally have console kernel connections always disable comms? We could do this so that if you opened a console, then a notebook associated with the console, the notebook would have comms enabled.

I think that would be a much more invasive change. This change as-is basically lets the first kernel connection deal with comms, and others have comms disabled.

@jasongrout
Copy link
Contributor Author

I've also consolidated some of the testing utilities in this PR.

@jasongrout
Copy link
Contributor Author

CC @blink1073 @ivanov @afshin for review (or anyone else that wants to review this)

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@blink1073 blink1073 merged commit 4a6dae3 into jupyterlab:master Aug 2, 2019
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Sep 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants