Shared kernelconnection wrapper #10141
Conversation
Codecov Report
@@ Coverage Diff @@
## main #10141 +/- ##
=====================================
- Coverage 60% 60% -1%
=====================================
Files 203 204 +1
Lines 9240 9286 +46
Branches 1492 1502 +10
=====================================
+ Hits 5632 5650 +18
- Misses 3130 3154 +24
- Partials 478 482 +4
|
| public get kernelSocket(): Observable<KernelSocketInformation | undefined> { | ||
| return this._kernelSocket; | ||
| } | ||
| public get onSessionStatusChanged(): Event<KernelMessage.Status> { | ||
| return this.onStatusChangedEvent.event; | ||
| } | ||
| public get onIOPubMessage(): Event<KernelMessage.IIOPubMessage> { |
There was a problem hiding this comment.
We need a way to hook up to the iopub and anymessage events in the underlyiing Jupyter kernel connection.
This property onIOPubMessage is a custom event we wire up. If I were to introduce anyMessage event then i'd have to handle kernel restarts all over again, tomorrow if we wanted to handle other events, then all over gain etc...
Also, as we expose the underlying jupyter kernel (Kernel.IKenrelConnection) everywhere it makes sense to just hook to those events.
Hence created a kernel wrapper so that we have a single instance, of the Kernel.KernleConnection on which we can monitor the events (basically moved onIOPubMessage & similar events into the kernel.IKernelConnection and have that keep track of all the event handlers even when kernels restart)
| } | ||
| } | ||
| }) | ||
| new Disposable(() => this.jupyterSession.kernel?.iopubMessage.disconnect(this.onIOPubMessage, this)) |
There was a problem hiding this comment.
Now we can hook up to the underlying Jupyter API events.
| import { BaseKernelConnectionWrapper } from '../../kernels/jupyter/baseKernelConnectionWrapper'; | ||
| import { IKernel } from '../../kernels/types'; | ||
|
|
||
| export class KernelConnectionWrapper extends BaseKernelConnectionWrapper { |
There was a problem hiding this comment.
existing wrapper around IKernel for 3rd party API (modifeid to share code)
This wrapper wrap the IKernel object, instead of directly exposing the jupyter API as the IKernel object handles restarts, interrupts, & all of those details.
| @@ -51,9 +51,6 @@ export class RawJupyterSession extends BaseJupyterSession { | |||
| } | |||
| return super.status; | |||
| } | |||
| public get kernelId(): string { | |||
| return this.session?.kernel?.id || ''; | |||
There was a problem hiding this comment.
Duplicate code in both child classes, moved to base class
| @@ -76,15 +69,6 @@ export class JupyterSession extends BaseJupyterSession implements IJupyterServer | |||
| // Wait for idle on this session | |||
| return this.waitForIdleOnSession(this.session, timeout); | |||
| } | |||
|
|
|||
| public override get kernel(): Kernel.IKernelConnection | undefined { | |||
| return this.session?.kernel || undefined; | |||
There was a problem hiding this comment.
Duplicate code in both child classes, moved to base class
DonJayamanne
force-pushed
the
kernelConnectionWrapper
branch
2 times, most recently
from
edb849d to
aafff02
Compare
DonJayamanne
force-pushed
the
kernelConnectionWrapper
branch
from
aafff02 to
13ecb40
Compare
Part of #9503