-
Notifications
You must be signed in to change notification settings - Fork 286
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
Improve logic for restarting kernels with new ports during initial startup #348
base: main
Are you sure you want to change the base?
Conversation
MetaKernelFinder -> KernelFinder
Prototype new kernel discovery machinery
Fix typo in documentation.
The old URL points to a "This page has moved"-page
Updated URL for Jupyter Kernels in other languages
- use IOLoop.current over IOLoop.instance - drop removed `loop` arg from PeriodicCallback - deprecate now-unused IOLoopKernelRestarter.loop
- interrupt_mode="signal" is the default and current behaviour - With interrupt_mode="message", instead of a signal, a `interrupt_request` message on the control port will be sent
prepare for tornado 5
Additional to the actual signal, send a message on the control port
this should allow ipykernel's wheel-installed specs to specify `python3` or `python2` and prevent python2 kernels from launching with sys.executable if the Python version is 3.
A simple lead in to the 'kernel nanny' work, this adds a command so you can do: jupyter kernel --kernel python
…ance Improve performance of get_kernel_spec
Tolerate invalid kernel specs in get_all_specs()
Start writing release notes for 5.2
require tornado
Du to a likely bug in wheel, the conditional dependency on pytest ends up being unconditional. Seem like adding parenthesis fix that (as a work around). See pypa/setuptools#1242 Closes jupyter#324
Parenthesize conditional requirement in setup.py
This shrinks the sdist from 2MB to ~250KB... just realized that after uploading 5.2.1 took way too long. Apparently 5.2.0 alsho shipped built docs.
Exclude build docs from sdist.
to help inqiure on this jupyter#329
more complete error message
Use the ability to exclude branches as describe there: - https://docs.travis-ci.com/user/customizing-the-build/#Safelisting-or-blocklisting-branches Relatively easy as MrMeeseeks push a known branch format. This of course cannot be tested until merged and backported, and another backport triggered.
Tell Travis not to test the push from MrMeeseeks
we could probably avoid this if we registered/unregistered atexit callbacks for instances instead of registering it once for classes at import time
handle classes having been torn down in atexit
…ation. Made kernel_monitor_enabled non configurable
@rolweber - you introduced the 'restart with new ports' machinery in #279. Do you have time to look at this and the accompanying PR jupyter/notebook#3284 ? |
@takluyver: Unfortunately, I don't have the time. But Sergey is sitting in the same office as I am, so it wouldn't be an independent review anyway :-) When I introduced the original machinery, I thought that the liveness check is based on heartbeat messages from the kernel. As I learned later, heartbeat messages are optional, and the liveness check really just indicates that the pipes to the started process are still up. Therefore, the condition I checked was meaningless for the intended purpose. Sergey picked up the task of implementing something that actually works, and this PR is the result of his efforts. |
So with the default settings ( When I set it to actually use the new code, it throws errors in the Qt console because the shell channel is lacking the I like the addition of a way to notify other components that the kernel has restarted with new ports. But if the application can handle that (as the notebook will be able to with your corresponding PR), the extra complexity to decide when to use it seems superfluous - we can just use new ports on every restart. So I'd say:
|
@takluyver Thank you for your review. I will update the pull request regarding your proposals. By default |
b1fe8e4
to
3e8ee4a
Compare
The proposal defines the logic in
restarter.py
for setting_initial_startup
value toFalse
only when the kernel sends kernel_info reply. Introduced changes solve issues described in #347. The introduced changes allow to identify issues with shell channel (e.g. due to theZMQError: Address already in use
issue)The proposal also defines logic for notifying client (Notebook Server, Kernel gateway, etc.) that the kernel was restarted with new ports.
The Proposal includes the following changes:
startup_time
parameter is introduced. This parameter defines the timeout for kernel_reply response. Default value is set to 0 (kernel_info reply monitoring disables). It is disabled by default, because different kernels (e.g. kernels with spark context) require different time before the kernel will be ready.startup_time
>0
thenkernel_info
request is sent to kernel with the first execution of restarter'spoll
functionpoll
function is executed the kernel_info response from shell channel is checked (withinstartup_time
). It is done to prevent application blocking while waiting for the shell messageskernel_info
reply the kernel monitor is disabledkernel_info
reply is not received withinstartup_time
then kernel is restarted with new ports and restart callback withnewports=True
parameter is executed**kwargs
support for registered callback:callback(**kwargs)
. If additional parameters are not supported thencallback()
will be executed.newports
parameter indicating whether new ports are initialised is sent. Client could apply the action based on this informationkernel_manager.restart_kernel
function is executed. It is required because a connection file with new ports is created during kernel restart