Skip to content

Prevent missing IOPub upon restart #5904

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Prevent missing IOPub upon restart #5904

wants to merge 1 commit into from

Conversation

@SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Dec 10, 2020

Upon restart, nedge the kernel with info requests until we get something on IOPub, to ensure it is properly connected.

CF jupyter/jupyter_client#593

SylvainCorlay
SylvainCorlay commented Dec 10, 2020
View reviewed changes
notebook/services/kernels/kernelmanager.py
# return a Future that will resolve when the kernel has successfully restarted
channel = kernel.connect_shell()
shell_channel = kernel.connect_shell()
iopub_channel = kernel.connect_iopub()
Copy link
Member Author

@SylvainCorlay SylvainCorlay Dec 10, 2020

Choose a reason for hiding this comment

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

Really, we should use the iopub channel from zmqhandler.channels, which we don't have access to here.

@SylvainCorlay SylvainCorlay mentioned this pull request Dec 10, 2020
Closed
@kevin-bates
Copy link
Member

kevin-bates commented Dec 10, 2020

Hi Sylvain - thanks for submitting this pull request. Using these changes, I am unable to complete a general restart of the kernel. The state stays at restarting and no commands can be performed. It does appear that the kernel manager is completing its restart of the process, but the setup of the kernel messaging is having issues.

I also notice that SIGINT (multiple times) is not terminating the application (SIGQUIT does) implying that the process is probably tied up in lower levels.

I've left a similar comment on the jupyter-server PR as well (once the method name is changed).

@kevin-bates
Copy link
Member

kevin-bates commented Dec 10, 2020

I'm sorry. It just dawned on me that this PR requires jupyter/jupyter_client#593, so please disregard the restart failure behavior. I'll need to pull that PR as well.

@kevin-bates kevin-bates mentioned this pull request Dec 10, 2020
Merged
@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Dec 11, 2020

Closing. This is not the way to go.

We need to do this in pre_get / open when opening the web socket connection.

@SylvainCorlay SylvainCorlay deleted the restart branch December 11, 2020 09:32
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

status:resolved-locked

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SylvainCorlay @kevin-bates