Add new Jupyter kernel supervisor #4910
Conversation
There was a problem hiding this comment.
Overall looking great! I added some minor review comments. The only major thing was some weirdness around restarting runtimes. I ran some Python apps, plotted some things in R and Python and used the Data Explorer and everything seemed to work as usual.
Some notes:
- I am seeing some weirdness around restarting runtimes
Python:R:print('hi') # Click restart button and wait for restart to succeed print('hi') # Click restart button and see `Failed to start session` error in kcserver terminal output # We can continue to use the Python Console, but we can't restart it anymore
print('hi') # Click restart button and wait for restart to succeed print('hi') # Click restart button and see `Failed to start session` error in kcserver terminal output print('hi') # R exits, but doesn't restart # Click power button to start the R runtime and it will finish the restart, but it looks like it starts a new session
- We could add the kallichore generated code directories to our top-level
.eslintignoreif the red squigglies are looking too unfriendly
| path: `/repos/posit-dev/kallichore/releases` | ||
| }; | ||
|
|
||
| const response = await httpsGetAsync(requestOptions as any) as any; |
There was a problem hiding this comment.
Suggested nit to add types/checking in place of any around uses of httpsGetAsync
| readonly session: JupyterLanguageRuntimeSession) { | ||
|
|
||
| // Generate 8 random hex characters for the message stem | ||
| this._msgStem = Math.random().toString(16).substr(2, 8); |
There was a problem hiding this comment.
Alternative to substr which seems to be deprecated
| this._msgStem = Math.random().toString(16).substr(2, 8); | |
| this._msgStem = Math.random().toString(16).slice(2, 10); |
| const path = require('path'); | ||
| const fs = require('fs'); | ||
|
|
There was a problem hiding this comment.
Do we still need these imports here? Looks like we import them at the top of the file
|
|
||
| import * as vscode from 'vscode'; | ||
|
|
||
| import path = require('path'); |
There was a problem hiding this comment.
Looks to be unused
| import path = require('path'); |
|
thank you for the review, @sharon-wang! I was able to reproduce the restart issue and have fixed it in the latest change (the problem was in the supervisor). |
This change adds a new and currently optional Jupyter kernel supervisor to Positron.
When enabled, all R and Python kernels are run under a supervisor process (codenamed 'Kallichore'). This process is a native binary that takes care of the minutiae of kernel lifecyle management (process output, startup, shutdown, restart, status reporting, etc.). It also takes care of the low-level ZeroMQ socket connectivity and exposes a simple WebSocket based interface that proxies messages to and from the ZeroMQ sockets.
The implementation is a drop-in replacement for the Jupyter Adapter and currently exposes an API that is fully compatible with the Jupyter Adapter; the only changes to the R and Python extensions are switches that determine which extension the Jupyter Adapter API is loaded from. The change is otherwise totally transparent to the R and Python extensions since they already are behind a layer of abstraction from the Jupyter protocol.
The current plan of record is for the Jupyter Adapter to be fully deprecated in favor of this new supervisor when it reaches feature parity, stability, and performance goals. This will allow us to get rid of our troublesome ZeroMQ bindings in Node and will also improve stability and performance around session management.
The goal of this PR is to start exposing the new supervisor to builds and to make it possible to start collaborating on work associated with the supervisor. While most everything works today with the new supervisor, there are a lot of edge cases that aren't finished yet, so the supervisor is disabled by default and will continue to be until it's ready to take over.
Lint-level issues are aggregated here: #4912
QA Notes
This change isn't ready for QA yet. It tries very hard to avoid touching any code outside the new adapter extension; the only thing worth testing is probably R and Python restarts.