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

Handle Iopub welcome message for jep#65 #1303

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

Conversation

Hind-M
Copy link
Contributor

@Hind-M Hind-M commented Jul 24, 2023

Update jupyter_server to complete the JEP #65 implementation.

@welcome
Copy link

welcome bot commented Jul 24, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

iopub_future.set_result(None)

# Resolve with a timeout if we get no response
timeout_future = gen.with_timeout(loop.time() + self.kernel_info_timeout, iopub_future)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we should use kernel_info_timeout or something else for the timeout?

@Hind-M
Copy link
Contributor Author

Hind-M commented Jul 24, 2023

Should we add a test, maybe something similar to this? I am not sure how yet...

@Hind-M
Copy link
Contributor Author

Hind-M commented Jul 25, 2023

I think the Check Links failure in the CI is unrelated to this PR (see #1304).
Regarding the jupyterlab_server failure, is it related to some version incompatibility with jsonschema?

@Zsailer Zsailer self-requested a review July 27, 2023 15:06
@Zsailer Zsailer self-assigned this Aug 10, 2023
@Zsailer
Copy link
Member

Zsailer commented Sep 21, 2023

Hi @Hind-M 👋

Apologies for the long delay here. I'm finally getting some time to review here. Overall, I think the approach here looks good. Is there a kernel that implements the protocol version >=5.4? I couldn't seem to trigger the handshake here using the latest versions of xeus-python or ipykernel.

@Hind-M
Copy link
Contributor Author

Hind-M commented Sep 21, 2023

Hey @Zsailer!
No worries, thanks for your time!
Well, IIUC bumping to a protocol version >= 5.4 would be appreciated after getting some other features in (in addition to the XPUB one).
For now, xeus-zmq has already the XPUB corresponding PR merged (jupyter-xeus/xeus-zmq#31).
Next step would be to update the protocol version to have it in the latest version of xeus-python.
That's why I was hoping to add a test in jupyter-server with something similar to this? and check the tests locally (using the right version of xeus-zmq) while waiting for the protocol version to be out?

@Zsailer
Copy link
Member

Zsailer commented Oct 12, 2023

That's why I was hoping to add a test in jupyter-server with something similar to this?

That makes sense.

I think we should split the test_websocket_connection unit test into two different tests; and instead of calling nudge directly, we switch to using is_subscribed.

Maybe name the unit tests:

  1. def test_is_subscribed_via_nudge(...)
  2. def test_is_subscribed_via_xpub(...)

The rest of the unit tests are equivalent, but the configuration going into the unit tests are different to trigger the nudge/xpub flow.

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.

2 participants