-
Notifications
You must be signed in to change notification settings - Fork 504
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 non-existing kernels #309
Handle non-existing kernels #309
Conversation
Another approach would be to make use of the |
What does the classic notebook do? It appears to be quite liberal with kernelspecs. |
It looks like most of the logic for dealing with missing kernel names in the classic notebook is actually handled by the frontend. This is what it appears to be doing:
I might have missed some details but this is basically the outcome of stepping through the code in the dev tools. |
Thanks for looking into this. What do you think is the cleanest thing to do? It seems that jupyterlab pops up a dialog to select a kernel. Or such a logic could be in the backend? |
Finding a match based on the
And in some way it follows the logic from the classic notebook. For the case where there is no match for a given |
The last commits implement finding a spec matching the If there is no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach, my remaining question is: should we do this in voila or nbconvert.
I'm ok with getting this into voila first, then see if we can upstream this.
voila/handler.py
Outdated
] | ||
if matches: | ||
matches.sort(key=lambda name: all_kernel_specs[name]["spec"]["display_name"]) | ||
kernel_name = matches[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we have 0 matches, if we can have a custom template with a proper error msg and http error code: https://en.wikipedia.org/wiki/List_of_HTTP_status_codes
If there are 1 or more matches, I think we should log a warning, so users are aware of it (I can imagine someone expecting it to run with a conda-env-kernel, which cannot be found, leading to surprises).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, a warning would definitely be useful to let the users know the kernel being used is not the one they expect.
Having a custom template with a proper error message is also a good idea, but maybe for a follow-up PR?
voila/handler.py
Outdated
kernel_name = kernelspec.get('name', self.kernel_manager.default_kernel_name) | ||
|
||
# Find a spec matching the language if the kernel name does not exist in the kernelspecs | ||
all_kernel_specs = yield tornado.gen.maybe_future(self.kernel_spec_manager.get_all_specs()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is yield tornado.gen.maybe_future
needed? The current implementation gives a plain dict, or can other implementations return a future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kernel spec manager could be a GatewayKernelSpecManager
, in which case the maybe_future
would be needed: https://github.com/jupyter/notebook/blob/e498de6775b3de57f5ff827e562c179b17893064/notebook/gateway/managers.py#L521
…o reuse We also give logger warnings when the kernel_name is not found (and a match is used)
b6f0a0d
to
a18de94
Compare
Not sure it was discussed before, but do we need to keep supporting Python 2? |
Not really, but we have it now, and it doesn't lead yet to any major hurdles. I'd say the incentive to keep supporting is decaying exponentially though. |
Right, the hurdles are quite minimal for now. |
@maartenbreddels this looks good to me. Returning a 500 when there is no matching kernel is definitely useful. |
Fixes #259. It should also provide a simple solution for #267.
This PR makes use of the
KernelSpecManager
to first check that the requested kernel name (from the notebook metadata) is available. If it isn't then it will default to the default kernel name.I also got hit by that recently while using
xeus-python
and decided to give it a shot.