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

Fix activity tracking and nudge issues when kernel ports change on restarts #482

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

kevin-bates
Copy link
Member

This change adds the ability for the ZMQChannelsHandler and activity watching logic to determine if ports have changed when restoring connections and restarting kernels, respectively.

By determining that ports have changed during restarts, the MappingKernelManager can also restart the activity monitor since it listens on the iopub channel. This will preserve proper functionality for culling behaviors as well as allow the "nudge" functionality to determine the kernel is busy - bypassing the time-based attempts to send/recieve a kernel-info request.

ZMQChannelsHandler.open() makes use of the fact that the kernel's ports have changed when restoring a connection (which essentially consists of loading state from previously recorded buffers). Prior to this change, the nudge functionality within this branch would use the kernel's previous iopub channel to listen for replies and time out. It will now use the kernel's current iopub channel when port changes are detected.

I made an attempt to write a test for this but this requires buffering to occur and I'm not certain what else the front end is doing. However, use of the tests's NewPortsMappingKernelManager and NewPortsKernelManager subclasses make introducing port-changes on restarts rather simple and I decided to retain this class's use in the tests in cases other port-related issues are encountered.

With these subclasses, this issue can be reproduced by running the following command:

jupyter lab --debug --ServerApp.kernel_manager_class=jupyter_server.tests.services.sessions.test_api.NewPortsMappingKernelManager

You will also need to change the result of the new internal method MappingKernelManager._get_changed_ports() to return None to disable port-change detection.

Resolved #481

@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@41e837f). Click here to learn what that means.
The diff coverage is 83.95%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #482   +/-   ##
=========================================
  Coverage          ?   77.76%           
=========================================
  Files             ?      106           
  Lines             ?     9255           
  Branches          ?      996           
=========================================
  Hits              ?     7197           
  Misses            ?     1703           
  Partials          ?      355           
Impacted Files Coverage Δ
jupyter_server/services/kernels/handlers.py 62.96% <0.00%> (ø)
jupyter_server/services/kernels/kernelmanager.py 80.43% <73.07%> (ø)
jupyter_server/tests/services/sessions/test_api.py 98.85% <94.23%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41e837f...2e36f8a. Read the comment docs.

Copy link
Contributor

@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.

Thanks!

@blink1073 blink1073 merged commit aa26367 into jupyter-server:master Apr 19, 2021
@kevin-bates kevin-bates deleted the handle-port-changes branch April 19, 2021 16:29
hMED22 pushed a commit to hMED22/jupyter_server that referenced this pull request Jan 23, 2023
…anges

Fix activity tracking and nudge issues when kernel ports change on restarts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kernel activity tracking stops and "nudge" times out when kernel ports change on restarts
3 participants