Run kernel on cell execution when no kernel #12858
Conversation
|
Thanks for making a pull request to jupyterlab! |
|
I have found that |
|
|
||
| // Always fall back to selecting a kernel | ||
| return true; | ||
| return this.startKernel(); |
There was a problem hiding this comment.
_startIfNecessary already checked preference.shouldStart === false So _startIfNecessary is backward compatible
| const { preference } = options; | ||
| const { shouldStart } = preference; | ||
|
|
||
| if (shouldStart === false) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
I added this to make SessionContext.getDefaultKernel be backward compatible.
Honestly, I don't like this change... It is very hard to understand this code without knowing history.
Previous code is very simple, but it is not straightforward, too. Why Private.getDefaultKernel is needed? We already opened Private.getDefaultKernel by SessionContext.getDefaultKernel.
packages/notebook/src/actions.tsx
Outdated
| if (sessionContext.hasNoKernel) { | ||
| void sessionContextDialogs.selectKernel(sessionContext); | ||
| return Promise.resolve(false); | ||
| } | ||
| const deletedCells = notebook.model?.deletedCells ?? []; | ||
| executionScheduled.emit({ notebook, cell }); | ||
| return CodeCell.execute(cell as CodeCell, sessionContext, { | ||
| deletedCells, | ||
| recordTiming: notebook.notebookConfig.recordTiming | ||
| }) | ||
| .then(reply => { | ||
| deletedCells.splice(0, deletedCells.length); | ||
| if (cell.isDisposed) { | ||
| return false; | ||
| promise = sessionContext.startKernel().then(shouldSelect => { | ||
| if (shouldSelect) { | ||
| return sessionContextDialogs.selectKernel(sessionContext); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Now we conditionally chain a promise to start new default kernel or prompt dialogs.
There was a problem hiding this comment.
Sorry for not looking at this earlier and thanks a lot for the PR. I understand and like the idea. But some organization prefer to be able to pick a kernel. Recently the dialog has been extended to display a checkbox in the footer: see documentation.
Do you think it could be possible to use it on the kernel dialog selection to add an option Always select the preferred kernel? That will be stored in a settings in order to allow your use case A or not. And in your deployed installation, that settings could be set to true to fit your need.
Sorry for my late reply. Your suggestion about checkbox is good. For the first, I added new configurable setting I'm now considering using commands for turning on the Then I will add checkbox and execute the command here: jupyterlab/packages/apputils/src/sessioncontext.tsx Lines 1279 to 1317 in edc294b Unfortunately, CommandRegistry is not available. Also I think passing CommandRegistry to SessionContext, Notebook, ... is a bad for this small feature. Is there any advice? |
|
Hey sorry too for the slow reply. My two cents here is that we should drop the default So the actions are:
It will take the
Does it make sense? |
I finally understand what Token is. It is actually a Dependency Injection system. Thank you for your reply, it helps a lot.
Should |
Indeed this is a good point - we can keep it to ease backporting it. |
|
I have finished dropping old default sessionContextDialogs. And I found that it is somewhat impossible to make it backward compatible. So many functions have to be changed. To make it compatible, We should keep old default. I will check tests now. It looks like Github actions are failing with some other reason during installation. 2023-01-19.20-24-30.mp4 |
This reverts commit bb0df81.
fcollonval
force-pushed
the
feature/run-kernel-on-execute
branch
from
18d8381
to
99ee80d
Compare
Benchmark reportThe execution time (in milliseconds) are grouped by test file, test type and browser. The mean relative comparison is computed with 95% confidence. Results table
Changes are computed with expected as reference.
Waiting for localhost:8888 Cell memory leaksCreate a code cellMemory change: -158 kB Leak detected: NoLeaking objects:
Leaking collections: Create a markdown cellMemory change: -104 kB Leak detected: NoLeaking objects:
Leaking collections: Create a raw cellMemory change: -154 kB Leak detected: NoLeaking objects:
Leaking collections: File editor memory leaksCreate a fileMemory change: -74.5 kB Leak detected: NoLeaking objects:
Notebook memory leaksCreate a notebookMemory change: +26.6 kB Leak detected: YesLeaking objects:
2 passing (6m) |
Background
There are several requests about opening notebook file without kernel.
KernelPreferencealready haveshouldStartboolean flag. This flag can be set through jupyter server argument--LabServerApp.notebook_starts_kernel=False. This flag works as intended, so it prevents kernels to be launched automatically.Nevertheless, we need more improvement here. When users execute a code cell in a notebook without kernel, it shows a dialog:
(It was done by this PR #12379)
When
shouldStartis false, user must select kernels for every notebook file opened. It is very annoying, and bad user experience.So I suggest a new behaviors for launching a new kernel on code execution.
References
This closes #12793 as it set the
autoStartDefault.Code changes
SessionContext
_startIfNecessaryinto a new public methodstartKernelgetDefaultKernelshouldStartstartKerneldepends ongetDefaultKernelto automatically select a kernel.SessionContextDialogsdefault global are removed - you should use the token'@jupyterlab/apputils-extension:sessionDialogs'(This is a similar change to the sanitizer or the content factory to be more token based)autoStartDefaultonly for notebook (with a settings in notebook-extension package) with a check box in the kernel selection dialog, the logic is a complex. But basically, ifautoStartDefaultis notundefineda checkbox will be displayed. The checkbox value will be used to update the session context kernel preference. That update will trigger a newkernelPreferenceChangedinISessionContext. That signal is listen to in the notebook extension to be able to update the settings.Actions
startKernelUser-facing changes
When a notebook is opened without kernel and a user tries to execute a code cell, the user don't have to select a kernel. Additionally, code is executed immediately after the kernel executed.
When the user is opening a new notebook, if the new settings is true, the associated kernel will be started directly.
Before (JLab 4.0.0a34)
After
Backwards-incompatible changes
global
sessionContextDialogsfrom@jupyterlab/apputilsdoes not exist any longer.In my initial draft, the semantic ofgetDefaultKernelchanged. It is not a serious but a backwards-incompatible change. I think it is possible to satify Requirement A without editinggetDefaultKernel, but some level of code duplication is inevitable.I need help to choose the direction, 1) ignoring small level of backwards-incompatibility or 2) accepting a little bit of duplication.(Edited)
getDefaultKernelis a private function. I made every function which callsPrivate.getDefaultKernelkeep old behavior.