From d29a50655213e722a88a1d48a94a74fa3e5dadc8 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 21 Jul 2022 12:13:09 +1000 Subject: [PATCH 1/8] Separate kernel providers --- src/interactive-window/interactiveWindow.ts | 3 +- src/kernels/errors/kernelErrorHandler.ts | 3 +- src/kernels/execution/kernelExecution.ts | 302 +++++++++++------- src/kernels/kernel.ts | 229 +++++++++---- src/kernels/kernelProvider.base.ts | 41 ++- src/kernels/kernelProvider.node.ts | 24 +- src/kernels/kernelProvider.web.ts | 20 +- src/kernels/types.ts | 63 ++-- src/notebooks/controllers/kernelConnector.ts | 49 ++- .../controllers/vscodeNotebookController.ts | 6 +- .../debugger/debuggingManagerBase.ts | 6 +- src/notebooks/notebookCommandListener.ts | 3 +- src/standalone/api/kernelApi.ts | 13 +- ...remoteKernelConnectionHandler.unit.test.ts | 2 +- .../kernels/kernelProvider.node.unit.test.ts | 7 - 15 files changed, 449 insertions(+), 322 deletions(-) diff --git a/src/interactive-window/interactiveWindow.ts b/src/interactive-window/interactiveWindow.ts index dc09b6efdc0..305224b6bc1 100644 --- a/src/interactive-window/interactiveWindow.ts +++ b/src/interactive-window/interactiveWindow.ts @@ -257,10 +257,9 @@ export class InteractiveWindow implements IInteractiveWindowLoadable { // When connecting, we need to update the sys info message this.updateSysInfoMessage(this.getSysInfoMessage(metadata, SysInfoReason.Start), false, sysInfoCell); const kernel = await KernelConnector.connectToNotebookKernel( - controller, metadata, this.serviceContainer, - { resource: this.owner, notebook: this.notebookDocument }, + { resource: this.owner, notebook: this.notebookDocument, controller }, new DisplayOptions(false), this.internalDisposables, 'jupyterExtension', diff --git a/src/kernels/errors/kernelErrorHandler.ts b/src/kernels/errors/kernelErrorHandler.ts index 7477a97c161..2594de8254e 100644 --- a/src/kernels/errors/kernelErrorHandler.ts +++ b/src/kernels/errors/kernelErrorHandler.ts @@ -279,6 +279,7 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle err instanceof InvalidRemoteJupyterServerUriHandleError ? this.extensions.getExtension(err.extensionId)?.packageJSON.displayName || err.extensionId : ''; + const options = actionSource === 'jupyterExtension' ? [DataScience.selectDifferentKernel()] : []; const selection = await this.applicationShell.showErrorMessage( err instanceof InvalidRemoteJupyterServerUriHandleError ? DataScience.remoteJupyterServerProvidedBy3rdPartyExtensionNoLongerValid().format(extensionName) @@ -286,7 +287,7 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle { detail: message, modal: true }, DataScience.removeRemoteJupyterConnectionButtonText(), DataScience.changeRemoteJupyterConnectionButtonText(), - DataScience.selectDifferentKernel() + ...options ); switch (selection) { case DataScience.removeRemoteJupyterConnectionButtonText(): { diff --git a/src/kernels/execution/kernelExecution.ts b/src/kernels/execution/kernelExecution.ts index 68c4f1923bd..4265662a7a2 100644 --- a/src/kernels/execution/kernelExecution.ts +++ b/src/kernels/execution/kernelExecution.ts @@ -18,10 +18,12 @@ import { captureTelemetry, Telemetry } from '../../telemetry'; import { CellOutputDisplayIdTracker } from './cellDisplayIdTracker'; import { IKernelConnectionSession, - IBaseKernel, + IThirdPartyKernel, InterruptResult, ITracebackFormatter, - NotebookCellRunState + NotebookCellRunState, + IKernel, + IBaseKernel } from '../../kernels/types'; import { traceCellMessage } from './helpers'; import { getDisplayPath } from '../../platform/common/platform/fs-paths'; @@ -32,70 +34,17 @@ import { noop } from '../../platform/common/utils/misc'; * Separate class that deals just with kernel execution. * Else the `Kernel` class gets very big. */ -export class KernelExecution implements IDisposable { - private readonly documentExecutions = new WeakMap(); - private readonly executionFactory: CellExecutionFactory; - private readonly disposables: IDisposable[] = []; +export class BaseKernelExecution implements IDisposable { + protected readonly disposables: IDisposable[] = []; private _interruptPromise?: Promise; private _restartPromise?: Promise; - private readonly _onPreExecute = new EventEmitter(); - constructor( - private readonly kernel: IBaseKernel, - appShell: IApplicationShell, - private readonly interruptTimeout: number, - outputTracker: CellOutputDisplayIdTracker, - context: IExtensionContext, - formatters: ITracebackFormatter[] - ) { - const requestListener = new CellExecutionMessageHandlerService( - appShell, - this.kernel.controller, - outputTracker, - context, - formatters - ); - this.disposables.push(requestListener); - this.executionFactory = new CellExecutionFactory(this.kernel.controller, requestListener); + protected get restarting() { + return this._restartPromise || Promise.resolve(); } + constructor(protected readonly kernel: TKernel, private readonly interruptTimeout: number) {} - public get onPreExecute() { - return this._onPreExecute.event; - } - public get queue() { - const notebook = this.kernel.notebook; - return notebook ? this.documentExecutions.get(notebook)?.queue || [] : []; - } - public async executeCell( - sessionPromise: Promise, - cell: NotebookCell, - codeOverride?: string - ): Promise { - traceCellMessage(cell, `KernelExecution.executeCell (1), ${getDisplayPath(cell.notebook.uri)}`); - if (cell.kind == NotebookCellKind.Markup) { - return NotebookCellRunState.Success; - } - - // If we're restarting, wait for it to finish - if (this._restartPromise) { - await this._restartPromise; - } - - traceCellMessage(cell, `KernelExecution.executeCell (2), ${getDisplayPath(cell.notebook.uri)}`); - const executionQueue = this.getOrCreateCellExecutionQueue(cell.notebook, sessionPromise); - executionQueue.queueCell(cell, codeOverride); - const result = await executionQueue.waitForCompletion([cell]); - traceCellMessage(cell, `KernelExecution.executeCell completed (3), ${getDisplayPath(cell.notebook.uri)}`); - return result[0]; - } public async cancel() { - const notebook = this.kernel.notebook; - if (!notebook) { - return; - } - const executionQueue = this.documentExecutions.get(notebook); - if (executionQueue) { - await executionQueue.cancel(true); - } + noop(); } /** @@ -103,63 +52,41 @@ export class KernelExecution implements IDisposable { * If we don't have a kernel (Jupyter Session) available, then just abort all of the cell executions. */ public async interrupt(sessionPromise?: Promise): Promise { - trackKernelResourceInformation(this.kernel.resourceUri, { interruptKernel: true }); - const notebook = this.kernel.notebook; - const executionQueue = notebook ? this.documentExecutions.get(notebook) : undefined; - if (notebook && !executionQueue && this.kernel.kernelConnectionMetadata.kind !== 'connectToLiveRemoteKernel') { - return InterruptResult.Success; - } // Possible we don't have a notebook. const session = sessionPromise ? await sessionPromise.catch(() => undefined) : undefined; + const pendingExecutions = this.cancelPendingExecutions(); traceInfo('Interrupt kernel execution'); - // First cancel all the cells & then wait for them to complete. - // Both must happen together, we cannot just wait for cells to complete, as its possible - // that cell1 has started & cell2 has been queued. If Cell1 completes, then Cell2 will start. - // What we want is, if Cell1 completes then Cell2 should not start (it must be cancelled before hand). - const pendingCells = executionQueue - ? executionQueue.cancel().then(() => executionQueue.waitForCompletion()) - : Promise.resolve(); if (!session) { traceInfo('No notebook to interrupt'); this._interruptPromise = undefined; - await pendingCells; + await pendingExecutions; return InterruptResult.Success; } // Interrupt the active execution const result = this._interruptPromise ? await this._interruptPromise - : await (this._interruptPromise = this.interruptExecution(session, pendingCells)); + : await (this._interruptPromise = this.interruptExecution(session, pendingExecutions)); // Done interrupting, clear interrupt promise this._interruptPromise = undefined; return result; } + protected async cancelPendingExecutions(): Promise { + noop(); + } /** * Restarts the kernel * If we don't have a kernel (Jupyter Session) available, then just abort all of the cell executions. */ public async restart(sessionPromise?: Promise): Promise { - trackKernelResourceInformation(this.kernel.resourceUri, { restartKernel: true }); - const notebook = this.kernel.notebook; - const executionQueue = notebook ? this.documentExecutions.get(notebook) : undefined; - // Possible we don't have a notebook. const session = sessionPromise ? await sessionPromise.catch(() => undefined) : undefined; - traceInfo('Restart kernel execution'); - // First cancel all the cells & then wait for them to complete. - // Both must happen together, we cannot just wait for cells to complete, as its possible - // that cell1 has started & cell2 has been queued. If Cell1 completes, then Cell2 will start. - // What we want is, if Cell1 completes then Cell2 should not start (it must be cancelled before hand). - const pendingCells = executionQueue - ? executionQueue.cancel(true).then(() => executionQueue.waitForCompletion()) - : Promise.resolve(); if (!session) { traceInfo('No notebook to interrupt'); this._restartPromise = undefined; - await pendingCells; return; } @@ -177,44 +104,11 @@ export class KernelExecution implements IDisposable { traceInfoIfCI(`Dispose KernelExecution`); this.disposables.forEach((d) => d.dispose()); } - private getOrCreateCellExecutionQueue( - document: NotebookDocument, - sessionPromise: Promise - ) { - const existingExecutionQueue = this.documentExecutions.get(document); - // Re-use the existing Queue if it can be used. - if (existingExecutionQueue && !existingExecutionQueue.isEmpty && !existingExecutionQueue.failed) { - return existingExecutionQueue; - } - - const newCellExecutionQueue = new CellExecutionQueue( - sessionPromise, - this.executionFactory, - this.kernel.kernelConnectionMetadata - ); - this.disposables.push(newCellExecutionQueue); - - // If the document is closed (user or on CI), then just stop handling the UI updates & cancel cell execution queue. - workspace.onDidCloseNotebookDocument( - async (e: NotebookDocument) => { - if (e === document) { - if (!newCellExecutionQueue.failed || !newCellExecutionQueue.isEmpty) { - await newCellExecutionQueue.cancel(true); - } - } - }, - this, - this.disposables - ); - newCellExecutionQueue.onPreExecute((c) => this._onPreExecute.fire(c), this, this.disposables); - this.documentExecutions.set(document, newCellExecutionQueue); - return newCellExecutionQueue; - } @captureTelemetry(Telemetry.Interrupt) @captureTelemetry(Telemetry.InterruptJupyterTime) private async interruptExecution( session: IKernelConnectionSession, - pendingCells: Promise + pendingExecutions: Promise ): Promise { const restarted = createDeferred(); const stopWatch = new StopWatch(); @@ -240,7 +134,7 @@ export class KernelExecution implements IDisposable { try { // Wait for all of the pending cells to finish or the timeout to fire const result = await waitForPromise( - Promise.race([pendingCells, restarted.promise]), + Promise.race([pendingExecutions, restarted.promise]), this.interruptTimeout ); @@ -292,3 +186,163 @@ export class KernelExecution implements IDisposable { await session.restart(); } } + +export class ThirdPartyKernelExecution extends BaseKernelExecution {} + +/** + * Separate class that deals just with kernel execution. + * Else the `Kernel` class gets very big. + */ +export class KernelExecution extends BaseKernelExecution { + private readonly documentExecutions = new WeakMap(); + private readonly executionFactory: CellExecutionFactory; + private readonly _onPreExecute = new EventEmitter(); + constructor( + kernel: IKernel, + appShell: IApplicationShell, + interruptTimeout: number, + outputTracker: CellOutputDisplayIdTracker, + context: IExtensionContext, + formatters: ITracebackFormatter[] + ) { + super(kernel, interruptTimeout); + const requestListener = new CellExecutionMessageHandlerService( + appShell, + kernel.controller, + outputTracker, + context, + formatters + ); + this.disposables.push(requestListener); + this.executionFactory = new CellExecutionFactory(this.kernel.controller, requestListener); + } + + public get onPreExecute() { + return this._onPreExecute.event; + } + public get queue() { + const notebook = this.kernel.notebook; + return notebook ? this.documentExecutions.get(notebook)?.queue || [] : []; + } + public async executeCell( + sessionPromise: Promise, + cell: NotebookCell, + codeOverride?: string + ): Promise { + traceCellMessage(cell, `KernelExecution.executeCell (1), ${getDisplayPath(cell.notebook.uri)}`); + if (cell.kind == NotebookCellKind.Markup) { + return NotebookCellRunState.Success; + } + + // If we're restarting, wait for it to finish + await this.restarting; + + traceCellMessage(cell, `KernelExecution.executeCell (2), ${getDisplayPath(cell.notebook.uri)}`); + const executionQueue = this.getOrCreateCellExecutionQueue(cell.notebook, sessionPromise); + executionQueue.queueCell(cell, codeOverride); + const result = await executionQueue.waitForCompletion([cell]); + traceCellMessage(cell, `KernelExecution.executeCell completed (3), ${getDisplayPath(cell.notebook.uri)}`); + return result[0]; + } + public override async cancel() { + await super.cancel(); + const notebook = this.kernel.notebook; + if (!notebook) { + return; + } + const executionQueue = this.documentExecutions.get(notebook); + if (executionQueue) { + await executionQueue.cancel(true); + } + } + + /** + * Interrupts the execution of cells. + * If we don't have a kernel (Jupyter Session) available, then just abort all of the cell executions. + */ + public override async interrupt(sessionPromise?: Promise): Promise { + trackKernelResourceInformation(this.kernel.resourceUri, { interruptKernel: true }); + const notebook = this.kernel.notebook; + const executionQueue = notebook ? this.documentExecutions.get(notebook) : undefined; + if (notebook && !executionQueue && this.kernel.kernelConnectionMetadata.kind !== 'connectToLiveRemoteKernel') { + return InterruptResult.Success; + } + return super.interrupt(sessionPromise); + } + protected override async cancelPendingExecutions(): Promise { + const notebook = this.kernel.notebook; + const executionQueue = notebook ? this.documentExecutions.get(notebook) : undefined; + if (notebook && !executionQueue && this.kernel.kernelConnectionMetadata.kind !== 'connectToLiveRemoteKernel') { + return; + } + traceInfo('Interrupt kernel execution'); + // First cancel all the cells & then wait for them to complete. + // Both must happen together, we cannot just wait for cells to complete, as its possible + // that cell1 has started & cell2 has been queued. If Cell1 completes, then Cell2 will start. + // What we want is, if Cell1 completes then Cell2 should not start (it must be cancelled before hand). + const pendingCells = executionQueue + ? executionQueue.cancel().then(() => executionQueue.waitForCompletion()) + : Promise.resolve(); + + await pendingCells; + } + /** + * Restarts the kernel + * If we don't have a kernel (Jupyter Session) available, then just abort all of the cell executions. + */ + public override async restart(sessionPromise?: Promise): Promise { + trackKernelResourceInformation(this.kernel.resourceUri, { restartKernel: true }); + const notebook = this.kernel.notebook; + const executionQueue = notebook ? this.documentExecutions.get(notebook) : undefined; + // Possible we don't have a notebook. + const session = sessionPromise ? await sessionPromise.catch(() => undefined) : undefined; + traceInfo('Restart kernel execution'); + // First cancel all the cells & then wait for them to complete. + // Both must happen together, we cannot just wait for cells to complete, as its possible + // that cell1 has started & cell2 has been queued. If Cell1 completes, then Cell2 will start. + // What we want is, if Cell1 completes then Cell2 should not start (it must be cancelled before hand). + const pendingCells = executionQueue + ? executionQueue.cancel(true).then(() => executionQueue.waitForCompletion()) + : Promise.resolve(); + + if (!session) { + traceInfo('No notebook to interrupt'); + await pendingCells; + } + + return super.restart(sessionPromise); + } + private getOrCreateCellExecutionQueue( + document: NotebookDocument, + sessionPromise: Promise + ) { + const existingExecutionQueue = this.documentExecutions.get(document); + // Re-use the existing Queue if it can be used. + if (existingExecutionQueue && !existingExecutionQueue.isEmpty && !existingExecutionQueue.failed) { + return existingExecutionQueue; + } + + const newCellExecutionQueue = new CellExecutionQueue( + sessionPromise, + this.executionFactory, + this.kernel.kernelConnectionMetadata + ); + this.disposables.push(newCellExecutionQueue); + + // If the document is closed (user or on CI), then just stop handling the UI updates & cancel cell execution queue. + workspace.onDidCloseNotebookDocument( + async (e: NotebookDocument) => { + if (e === document) { + if (!newCellExecutionQueue.failed || !newCellExecutionQueue.isEmpty) { + await newCellExecutionQueue.cancel(true); + } + } + }, + this, + this.disposables + ); + newCellExecutionQueue.onPreExecute((c) => this._onPreExecute.fire(c), this, this.disposables); + this.documentExecutions.set(document, newCellExecutionQueue); + return newCellExecutionQueue; + } +} diff --git a/src/kernels/kernel.ts b/src/kernels/kernel.ts index bc26d8385b2..d5ee71d9b6a 100644 --- a/src/kernels/kernel.ts +++ b/src/kernels/kernel.ts @@ -44,17 +44,18 @@ import { import { sendTelemetryEvent, Telemetry } from '../telemetry'; import { executeSilently, getDisplayNameOrNameOfKernelConnection, isPythonKernelConnection } from './helpers'; import { - IBaseKernel, + IKernel, IKernelConnectionSession, INotebookProvider, InterruptResult, isLocalConnection, IStartupCodeProvider, ITracebackFormatter, - KernelActionSource, KernelConnectionMetadata, KernelSocketInformation, - NotebookCellRunState + NotebookCellRunState, + IBaseKernel, + KernelActionSource } from './types'; import { Cancellation, isCancellationError } from '../platform/common/cancellation'; import { KernelProgressReporter } from '../platform/progress/kernelProgressReporter'; @@ -63,16 +64,19 @@ import { SilentExecutionErrorOptions } from './helpers'; import { IStatusProvider } from '../platform/progress/types'; import { CellOutputDisplayIdTracker } from './execution/cellDisplayIdTracker'; import { traceCellMessage } from './execution/helpers'; -import { KernelExecution } from './execution/kernelExecution'; +import { BaseKernelExecution, KernelExecution, ThirdPartyKernelExecution } from './execution/kernelExecution'; /** * Represents an active kernel process running on the jupyter (or local) machine. */ -export class Kernel implements IBaseKernel { - private readonly disposables: IDisposable[] = []; +abstract class BaseKernel implements IBaseKernel { + protected readonly disposables: IDisposable[] = []; get onStatusChanged(): Event { return this._onStatusChanged.event; } + get notebook(): NotebookDocument | undefined { + return this._notebookDocument; + } get onRestarted(): Event { return this._onRestarted.event; } @@ -82,14 +86,14 @@ export class Kernel implements IBaseKernel { get onDisposed(): Event { return this._onDisposed.event; } - get onPreExecute(): Event { - return this._onPreExecute.event; + get creator(): KernelActionSource { + return this._creator; } get startedAtLeastOnce() { return this._startedAtLeastOnce; } - private _info?: KernelMessage.IInfoReplyMsg['content']; - private _startedAtLeastOnce?: boolean; + protected _info?: KernelMessage.IInfoReplyMsg['content']; + protected _startedAtLeastOnce?: boolean; get info(): KernelMessage.IInfoReplyMsg['content'] | undefined { return this._info; } @@ -118,68 +122,51 @@ export class Kernel implements IBaseKernel { * `unknown` is generally used to indicate jupyter kernel hasn't started. * If a jupyter kernel dies after it has started, then status is set to `dead`. */ - private isKernelDead?: boolean; + protected isKernelDead?: boolean; public get session(): IKernelConnectionSession | undefined { return this._session; } protected _disposed?: boolean; protected _disposing?: boolean; protected _ignoreJupyterSessionDisposedErrors?: boolean; - private readonly _kernelSocket = new ReplaySubject(); + protected readonly _kernelSocket = new ReplaySubject(); protected readonly _onStatusChanged = new EventEmitter(); protected readonly _onRestarted = new EventEmitter(); - private readonly _onStarted = new EventEmitter(); + protected readonly _onStarted = new EventEmitter(); protected readonly _onDisposed = new EventEmitter(); - protected readonly _onPreExecute = new EventEmitter(); protected _jupyterSessionPromise?: Promise; - private readonly hookedSessionForEvents = new WeakSet(); + protected readonly hookedSessionForEvents = new WeakSet(); protected eventHooks: ((ev: 'willInterrupt' | 'willRestart') => Promise)[] = []; protected restarting?: Deferred; protected startCancellation = new CancellationTokenSource(); - private startupUI = new DisplayOptions(true); - private readonly kernelExecution: KernelExecution; - private disposingPromise?: Promise; - private _visibleExecutionCount = 0; + protected startupUI = new DisplayOptions(true); + protected kernelExecution: TKernelExecution; + protected disposingPromise?: Promise; + protected _visibleExecutionCount = 0; constructor( public readonly uri: Uri, public readonly resourceUri: Resource, - public readonly notebook: NotebookDocument | undefined, + private readonly _notebookDocument: NotebookDocument | undefined, public readonly kernelConnectionMetadata: Readonly, - private readonly notebookProvider: INotebookProvider, - private readonly launchTimeout: number, - interruptTimeout: number, + protected readonly notebookProvider: INotebookProvider, + protected readonly launchTimeout: number, + protected readonly interruptTimeout: number, protected readonly appShell: IApplicationShell, - public readonly controller: NotebookController, protected readonly configService: IConfigurationService, - outputTracker: CellOutputDisplayIdTracker, protected readonly workspaceService: IWorkspaceService, - private readonly statusProvider: IStatusProvider, - public readonly creator: KernelActionSource, - context: IExtensionContext, - formatters: ITracebackFormatter[], - private readonly startupCodeProviders: IStartupCodeProvider[], - private readonly sendTelemetryForPythonKernelExecutable: () => Promise + protected readonly statusProvider: IStatusProvider, + protected readonly startupCodeProviders: IStartupCodeProvider[], + private readonly _creator: KernelActionSource, + protected readonly sendTelemetryForPythonKernelExecutable: () => Promise ) { - this.kernelExecution = new KernelExecution( - this, - appShell, - interruptTimeout, - outputTracker, - context, - formatters - ); - this.kernelExecution.onPreExecute((c) => this._onPreExecute.fire(c), this, this.disposables); - this.disposables.push(this.kernelExecution); this.disposables.push(this._onStatusChanged); this.disposables.push(this._onRestarted); this.disposables.push(this._onStarted); this.disposables.push(this._onDisposed); - this.disposables.push(this._onPreExecute); - this.disposables.push(this.kernelExecution); this.disposables.push({ dispose: () => this._kernelSocket.unsubscribe() }); } - private perceivedJupyterStartupTelemetryCaptured?: boolean; + protected perceivedJupyterStartupTelemetryCaptured?: boolean; public addEventHook(hook: (event: 'willRestart' | 'willInterrupt') => Promise): void { this.eventHooks.push(hook); @@ -199,20 +186,6 @@ export class Kernel implements IBaseKernel { public async start(options?: IDisplayOptions): Promise { await this.startJupyterSession(options); } - public get pendingCells(): readonly NotebookCell[] { - return this.kernelExecution.queue; - } - public async executeCell(cell: NotebookCell, codeOverride?: string): Promise { - traceCellMessage(cell, `kernel.executeCell, ${getDisplayPath(cell.notebook.uri)}`); - sendKernelTelemetryEvent(this.resourceUri, Telemetry.ExecuteCell); - const stopWatch = new StopWatch(); - const sessionPromise = this.startJupyterSession(); - const promise = this.kernelExecution.executeCell(sessionPromise, cell, codeOverride); - this.trackNotebookCellPerceivedColdTime(stopWatch, sessionPromise, promise).catch(noop); - promise.finally(() => (this._visibleExecutionCount += 1)); - promise.then((state) => traceInfo(`Cell ${cell.index} executed with state ${state}`), noop); - return promise; - } public async interrupt(): Promise { await Promise.all(this.eventHooks.map((h) => h('willInterrupt'))); trackKernelResourceInformation(this.resourceUri, { interruptKernel: true }); @@ -401,7 +374,7 @@ export class Kernel implements IBaseKernel { return this._jupyterSessionPromise; } - private async createJupyterSession(stopWatch: StopWatch): Promise { + protected async createJupyterSession(stopWatch: StopWatch): Promise { let disposables: Disposable[] = []; try { // No need to block kernel startup on UI updates. @@ -468,7 +441,7 @@ export class Kernel implements IBaseKernel { } } - private createProgressIndicator(disposables: IDisposable[]) { + protected createProgressIndicator(disposables: IDisposable[]) { // Even if we're not supposed to display the progress indicator, // create it and keep it hidden. const progressReporter = KernelProgressReporter.createProgressReporter( @@ -611,7 +584,7 @@ export class Kernel implements IBaseKernel { } } - private async gatherInternalStartupCode(): Promise { + protected async gatherInternalStartupCode(): Promise { // Gather all of the startup code into a giant string array so we // can execute it all at once. const result: string[] = []; @@ -653,7 +626,7 @@ export class Kernel implements IBaseKernel { * Hence we end up waiting indefinitely. * https://github.com/microsoft/vscode-jupyter/issues/9014 */ - private requestEmptyCompletions() { + protected requestEmptyCompletions() { this.session ?.requestComplete({ code: '__file__.', @@ -662,7 +635,7 @@ export class Kernel implements IBaseKernel { .ignoreErrors(); } - private getMatplotLibInitializeCode(): string[] { + protected getMatplotLibInitializeCode(): string[] { const results: string[] = []; const settings = this.configService.getSettings(this.resourceUri); @@ -698,7 +671,7 @@ export class Kernel implements IBaseKernel { return results; } - private getUserStartupCommands(): string[] { + protected getUserStartupCommands(): string[] { const settings = this.configService.getSettings(this.resourceUri); // Run any startup commands that we specified. Support the old form too let setting = settings.runStartupCommands; @@ -716,7 +689,7 @@ export class Kernel implements IBaseKernel { return []; } - private async executeSilently( + protected async executeSilently( session: IKernelConnectionSession | undefined, code: string[], errorOptions?: SilentExecutionErrorOptions @@ -729,6 +702,132 @@ export class Kernel implements IBaseKernel { } } +export class ThirdPartyKernel extends BaseKernel { + public override get creator(): '3rdPartyExtension' { + return '3rdPartyExtension'; + } + constructor( + uri: Uri, + resourceUri: Resource, + notebook: NotebookDocument | undefined, + kernelConnectionMetadata: Readonly, + notebookProvider: INotebookProvider, + launchTimeout: number, + interruptTimeout: number, + appShell: IApplicationShell, + configService: IConfigurationService, + workspaceService: IWorkspaceService, + statusProvider: IStatusProvider, + startupCodeProviders: IStartupCodeProvider[], + sendTelemetryForPythonKernelExecutable: () => Promise + ) { + super( + uri, + resourceUri, + notebook, + kernelConnectionMetadata, + notebookProvider, + launchTimeout, + interruptTimeout, + appShell, + configService, + workspaceService, + statusProvider, + startupCodeProviders, + '3rdPartyExtension', + sendTelemetryForPythonKernelExecutable + ); + this.kernelExecution = new ThirdPartyKernelExecution(this, this.interruptTimeout); + this.disposables.push(this.kernelExecution); + } +} + +/** + * Represents an active kernel process running on the jupyter (or local) machine. + */ +export class Kernel extends BaseKernel implements IKernel { + public override get creator(): 'jupyterExtension' { + return 'jupyterExtension'; + } + + get onPreExecute(): Event { + return this._onPreExecute.event; + } + protected readonly _onPreExecute = new EventEmitter(); + override get notebook(): NotebookDocument { + return this._notebook; + } + + constructor( + uri: Uri, + resourceUri: Resource, + private _notebook: NotebookDocument, + kernelConnectionMetadata: Readonly, + notebookProvider: INotebookProvider, + launchTimeout: number, + interruptTimeout: number, + appShell: IApplicationShell, + public readonly controller: NotebookController, + configService: IConfigurationService, + outputTracker: CellOutputDisplayIdTracker, + workspaceService: IWorkspaceService, + statusProvider: IStatusProvider, + context: IExtensionContext, + formatters: ITracebackFormatter[], + startupCodeProviders: IStartupCodeProvider[], + sendTelemetryForPythonKernelExecutable: () => Promise + ) { + super( + uri, + resourceUri, + _notebook, + kernelConnectionMetadata, + notebookProvider, + launchTimeout, + interruptTimeout, + appShell, + configService, + workspaceService, + statusProvider, + startupCodeProviders, + 'jupyterExtension', + sendTelemetryForPythonKernelExecutable + ); + + this.kernelExecution = new KernelExecution( + this, + appShell, + interruptTimeout, + outputTracker, + context, + formatters + ); + this.kernelExecution.onPreExecute((c) => this._onPreExecute.fire(c), this, this.disposables); + this.disposables.push(this.kernelExecution); + this.disposables.push(this._onStatusChanged); + this.disposables.push(this._onRestarted); + this.disposables.push(this._onStarted); + this.disposables.push(this._onDisposed); + this.disposables.push(this._onPreExecute); + this.disposables.push(this.kernelExecution); + this.disposables.push({ dispose: () => this._kernelSocket.unsubscribe() }); + } + public get pendingCells(): readonly NotebookCell[] { + return this.kernelExecution.queue; + } + public async executeCell(cell: NotebookCell, codeOverride?: string): Promise { + traceCellMessage(cell, `kernel.executeCell, ${getDisplayPath(cell.notebook.uri)}`); + sendKernelTelemetryEvent(this.resourceUri, Telemetry.ExecuteCell); + const stopWatch = new StopWatch(); + const sessionPromise = this.startJupyterSession(); + const promise = this.kernelExecution.executeCell(sessionPromise, cell, codeOverride); + this.trackNotebookCellPerceivedColdTime(stopWatch, sessionPromise, promise).catch(noop); + promise.finally(() => (this._visibleExecutionCount += 1)); + promise.then((state) => traceInfo(`Cell ${cell.index} executed with state ${state}`), noop); + return promise; + } +} + // Wrap a block of python code in try except to make sure hat we have n function wrapPythonStartupBlock(inputCode: string[], pythonMessage: string): string[] { if (!inputCode || inputCode.length === 0) { diff --git a/src/kernels/kernelProvider.base.ts b/src/kernels/kernelProvider.base.ts index 366bdd2db7a..bea04b42f98 100644 --- a/src/kernels/kernelProvider.base.ts +++ b/src/kernels/kernelProvider.base.ts @@ -9,7 +9,14 @@ import { traceInfoIfCI, traceVerbose, traceWarning } from '../platform/logging'; import { getDisplayPath } from '../platform/common/platform/fs-paths'; import { IAsyncDisposable, IAsyncDisposableRegistry, IDisposableRegistry } from '../platform/common/types'; import { isUri, noop } from '../platform/common/utils/misc'; -import { IBaseKernel, IKernelProvider, IKernel, KernelOptions, IThirdPartyKernelProvider } from './types'; +import { + IThirdPartyKernel, + IKernelProvider, + IKernel, + KernelOptions, + IThirdPartyKernelProvider, + ThirdPartyKernelOptions +} from './types'; /** * Provides kernels to the system. Generally backed by a URI or a notebook object. @@ -137,15 +144,15 @@ export abstract class BaseThirdPartyKernelProvider implements IThirdPartyKernelP * The life time of kernels not tied to a notebook will be managed by callers of the API. * Where as if a kernel is tied to a notebook, then the kernel dies along with notebooks. */ - private readonly kernelsByUri = new Map(); + private readonly kernelsByUri = new Map(); private readonly pendingDisposables = new Set(); - protected readonly _onDidRestartKernel = new EventEmitter(); - protected readonly _onDidStartKernel = new EventEmitter(); - protected readonly _onDidCreateKernel = new EventEmitter(); - protected readonly _onDidDisposeKernel = new EventEmitter(); + protected readonly _onDidRestartKernel = new EventEmitter(); + protected readonly _onDidStartKernel = new EventEmitter(); + protected readonly _onDidCreateKernel = new EventEmitter(); + protected readonly _onDidDisposeKernel = new EventEmitter(); protected readonly _onKernelStatusChanged = new EventEmitter<{ status: KernelMessage.Status; - kernel: IBaseKernel; + kernel: IThirdPartyKernel; }>(); public readonly onKernelStatusChanged = this._onKernelStatusChanged.event; public get kernels() { @@ -165,26 +172,26 @@ export abstract class BaseThirdPartyKernelProvider implements IThirdPartyKernelP disposables.push(this._onDidCreateKernel); } - public get onDidDisposeKernel(): Event { + public get onDidDisposeKernel(): Event { return this._onDidDisposeKernel.event; } - public get onDidRestartKernel(): Event { + public get onDidRestartKernel(): Event { return this._onDidRestartKernel.event; } - public get onDidStartKernel(): Event { + public get onDidStartKernel(): Event { return this._onDidStartKernel.event; } - public get onDidCreateKernel(): Event { + public get onDidCreateKernel(): Event { return this._onDidCreateKernel.event; } - public get(uri: Uri): IBaseKernel | undefined { + public get(uri: Uri): IThirdPartyKernel | undefined { return this.kernelsByUri.get(uri.toString())?.kernel; } public getInternal(uri: Uri): | { - options: KernelOptions; - kernel: IBaseKernel; + options: ThirdPartyKernelOptions; + kernel: IThirdPartyKernel; } | undefined { return this.kernelsByUri.get(uri.toString()); @@ -197,8 +204,8 @@ export abstract class BaseThirdPartyKernelProvider implements IThirdPartyKernelP await Promise.all(items); await Promise.all(this.kernels.map((k) => k.dispose())); } - public abstract getOrCreate(uri: Uri, options: KernelOptions): IBaseKernel; - protected storeKernel(uri: Uri, options: KernelOptions, kernel: IBaseKernel) { + public abstract getOrCreate(uri: Uri, options: ThirdPartyKernelOptions): IThirdPartyKernel; + protected storeKernel(uri: Uri, options: ThirdPartyKernelOptions, kernel: IThirdPartyKernel) { this.kernelsByUri.set(uri.toString(), { options, kernel }); this._onDidCreateKernel.fire(kernel); } @@ -206,7 +213,7 @@ export abstract class BaseThirdPartyKernelProvider implements IThirdPartyKernelP /** * If a kernel has been disposed, then remove the mapping of Uri + Kernel. */ - protected deleteMappingIfKernelIsDisposed(uri: Uri, kernel: IBaseKernel) { + protected deleteMappingIfKernelIsDisposed(uri: Uri, kernel: IThirdPartyKernel) { kernel.onDisposed( () => { // If the same kernel is associated with this document & it was disposed, then delete it. diff --git a/src/kernels/kernelProvider.node.ts b/src/kernels/kernelProvider.node.ts index 5e759222c20..f6d6281d332 100644 --- a/src/kernels/kernelProvider.node.ts +++ b/src/kernels/kernelProvider.node.ts @@ -17,14 +17,15 @@ import { BaseCoreKernelProvider, BaseThirdPartyKernelProvider } from './kernelPr import { InteractiveWindowView } from '../platform/common/constants'; import { CellOutputDisplayIdTracker } from './execution/cellDisplayIdTracker'; import { sendTelemetryForPythonKernelExecutable } from './helpers.node'; -import { Kernel } from './kernel'; +import { Kernel, ThirdPartyKernel } from './kernel'; import { - IBaseKernel, + IThirdPartyKernel, IKernel, INotebookProvider, IStartupCodeProvider, ITracebackFormatter, - KernelOptions + KernelOptions, + ThirdPartyKernelOptions } from './types'; /** @@ -61,7 +62,7 @@ export class KernelProvider extends BaseCoreKernelProvider { const resourceUri = notebook.notebookType === InteractiveWindowView ? options.resourceUri : uri; const waitForIdleTimeout = this.configService.getSettings(resourceUri).jupyterLaunchTimeout; const interruptTimeout = this.configService.getSettings(resourceUri).jupyterInterruptTimeout; - const kernel = new Kernel( + const kernel: IKernel = new Kernel( uri, resourceUri, notebook, @@ -75,7 +76,6 @@ export class KernelProvider extends BaseCoreKernelProvider { this.outputTracker, this.workspaceService, this.statusProvider, - options.creator, this.context, this.formatters, this.startupCodeProviders, @@ -91,7 +91,7 @@ export class KernelProvider extends BaseCoreKernelProvider { return Promise.resolve(); } } - ) as IKernel; + ); kernel.onRestarted(() => this._onDidRestartKernel.fire(kernel), this, this.disposables); kernel.onDisposed(() => this._onDidDisposeKernel.fire(kernel), this, this.disposables); kernel.onStarted(() => this._onDidStartKernel.fire(kernel), this, this.disposables); @@ -115,19 +115,16 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { @inject(INotebookProvider) private notebookProvider: INotebookProvider, @inject(IConfigurationService) private configService: IConfigurationService, @inject(IApplicationShell) private readonly appShell: IApplicationShell, - @inject(CellOutputDisplayIdTracker) private readonly outputTracker: CellOutputDisplayIdTracker, @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IVSCodeNotebook) notebook: IVSCodeNotebook, @inject(IPythonExecutionFactory) private readonly pythonExecutionFactory: IPythonExecutionFactory, @inject(IStatusProvider) private readonly statusProvider: IStatusProvider, - @inject(IExtensionContext) private readonly context: IExtensionContext, - @multiInject(ITracebackFormatter) private readonly formatters: ITracebackFormatter[], @multiInject(IStartupCodeProvider) private readonly startupCodeProviders: IStartupCodeProvider[] ) { super(asyncDisposables, disposables, notebook); } - public getOrCreate(uri: Uri, options: KernelOptions): IBaseKernel { + public getOrCreate(uri: Uri, options: ThirdPartyKernelOptions): IThirdPartyKernel { // const notebook = this. const existingKernelInfo = this.getInternal(uri); if (existingKernelInfo && existingKernelInfo.options.metadata.id === options.metadata.id) { @@ -138,7 +135,7 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { const resourceUri = uri; const waitForIdleTimeout = this.configService.getSettings(resourceUri).jupyterLaunchTimeout; const interruptTimeout = this.configService.getSettings(resourceUri).jupyterInterruptTimeout; - let kernel: Kernel = new Kernel( + const kernel: IThirdPartyKernel = new ThirdPartyKernel( uri, resourceUri, undefined, @@ -147,14 +144,9 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { waitForIdleTimeout, interruptTimeout, this.appShell, - options.controller, this.configService, - this.outputTracker, this.workspaceService, this.statusProvider, - options.creator, - this.context, - this.formatters, this.startupCodeProviders, () => { if (kernel.session) { diff --git a/src/kernels/kernelProvider.web.ts b/src/kernels/kernelProvider.web.ts index 617cd2a913e..91ea39bf0cb 100644 --- a/src/kernels/kernelProvider.web.ts +++ b/src/kernels/kernelProvider.web.ts @@ -15,14 +15,15 @@ import { import { BaseCoreKernelProvider, BaseThirdPartyKernelProvider } from './kernelProvider.base'; import { IStatusProvider } from '../platform/progress/types'; import { CellOutputDisplayIdTracker } from './execution/cellDisplayIdTracker'; -import { Kernel } from './kernel'; +import { Kernel, ThirdPartyKernel } from './kernel'; import { - IBaseKernel, + IThirdPartyKernel, IKernel, INotebookProvider, IStartupCodeProvider, ITracebackFormatter, - KernelOptions + KernelOptions, + ThirdPartyKernelOptions } from './types'; /** @@ -72,7 +73,6 @@ export class KernelProvider extends BaseCoreKernelProvider { this.outputTracker, this.workspaceService, this.statusProvider, - options.creator, this.context, this.formatters, this.startupCodeProviders, @@ -102,18 +102,15 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { @inject(INotebookProvider) private notebookProvider: INotebookProvider, @inject(IConfigurationService) private configService: IConfigurationService, @inject(IApplicationShell) private readonly appShell: IApplicationShell, - @inject(CellOutputDisplayIdTracker) private readonly outputTracker: CellOutputDisplayIdTracker, @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IVSCodeNotebook) notebook: IVSCodeNotebook, @inject(IStatusProvider) private readonly statusProvider: IStatusProvider, - @inject(IExtensionContext) private readonly context: IExtensionContext, - @multiInject(ITracebackFormatter) private readonly formatters: ITracebackFormatter[], @multiInject(IStartupCodeProvider) private readonly startupCodeProviders: IStartupCodeProvider[] ) { super(asyncDisposables, disposables, notebook); } - public getOrCreate(uri: Uri, options: KernelOptions): IBaseKernel { + public getOrCreate(uri: Uri, options: ThirdPartyKernelOptions): IThirdPartyKernel { const existingKernelInfo = this.getInternal(uri); if (existingKernelInfo && existingKernelInfo.options.metadata.id === options.metadata.id) { return existingKernelInfo.kernel; @@ -123,7 +120,7 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { const resourceUri = uri; const waitForIdleTimeout = this.configService.getSettings(resourceUri).jupyterLaunchTimeout; const interruptTimeout = this.configService.getSettings(resourceUri).jupyterInterruptTimeout; - const kernel = new Kernel( + const kernel = new ThirdPartyKernel( uri, resourceUri, undefined, @@ -132,14 +129,9 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { waitForIdleTimeout, interruptTimeout, this.appShell, - options.controller, this.configService, - this.outputTracker, this.workspaceService, this.statusProvider, - options.creator, - this.context, - this.formatters, this.startupCodeProviders, () => Promise.resolve() ); diff --git a/src/kernels/types.ts b/src/kernels/types.ts index 3a5befb56a3..35fcb505aa0 100644 --- a/src/kernels/types.ts +++ b/src/kernels/types.ts @@ -132,6 +132,25 @@ export function isRemoteConnection( export interface IKernel extends IBaseKernel { readonly notebook: NotebookDocument; + readonly onPreExecute: Event; + /** + * Cells that are still being executed (or pending). + */ + readonly pendingCells: readonly NotebookCell[]; + /** + * Controller associated with this kernel + */ + readonly controller: NotebookController; + readonly creator: 'jupyterExtension'; + /** + * @param cell Cell to execute + * @param codeOverride Override the code to execute + */ + executeCell(cell: NotebookCell, codeOverride?: string): Promise; + /** + * Executes arbitrary code against the kernel without incrementing the execution count. + */ + executeHidden(code: string): Promise; } export interface IBaseKernel extends IAsyncDisposable { @@ -140,7 +159,6 @@ export interface IBaseKernel extends IAsyncDisposable { */ readonly executionCount: number; readonly uri: Uri; - readonly notebook: NotebookDocument | undefined; /** * In the case of Notebooks, this is the same as the Notebook Uri. * But in the case of Interactive Window, this is the Uri of the file (such as the Python file). @@ -157,16 +175,7 @@ export interface IBaseKernel extends IAsyncDisposable { readonly onDisposed: Event; readonly onStarted: Event; readonly onRestarted: Event; - readonly onPreExecute: Event; readonly status: KernelMessage.Status; - /** - * Who created this kernel, 3rd party extension or our (jupyter) extension. - */ - readonly creator: KernelActionSource; - /** - * Cells that are still being executed (or pending). - */ - readonly pendingCells: readonly NotebookCell[]; readonly disposed: boolean; readonly disposing: boolean; /** @@ -191,38 +200,32 @@ export interface IBaseKernel extends IAsyncDisposable { * This flag will tell us whether a real kernel was or is active. */ readonly startedAtLeastOnce?: boolean; - /** - * Controller associated with this kernel - */ - readonly controller: NotebookController; start(options?: IDisplayOptions): Promise; interrupt(): Promise; restart(): Promise; - /** - * @param cell Cell to execute - * @param codeOverride Override the code to execute - */ - executeCell(cell: NotebookCell, codeOverride?: string): Promise; - /** - * Executes arbitrary code against the kernel without incrementing the execution count. - */ - executeHidden(code: string): Promise; addEventHook(hook: (event: 'willRestart' | 'willInterrupt') => Promise): void; removeEventHook(hook: (event: 'willRestart' | 'willInterrupt') => Promise): void; } +export interface IThirdPartyKernel extends IBaseKernel { + readonly creator: '3rdPartyExtension'; +} -export type KernelOptions = { +export type ThirdPartyKernelOptions = { metadata: KernelConnectionMetadata; - controller: NotebookController; /** * When creating a kernel for an Interactive window, pass the Uri of the Python file here (to set the working directory, file & the like) * In the case of Notebooks, just pass the uri of the notebook. */ resourceUri: Resource; +}; +export type KernelOptions = { + metadata: KernelConnectionMetadata; + controller: NotebookController; /** - * What is initiating this kernel action, is it Jupyter or a 3rd party extension. + * When creating a kernel for an Interactive window, pass the Uri of the Python file here (to set the working directory, file & the like) + * In the case of Notebooks, just pass the uri of the notebook. */ - creator: KernelActionSource; + resourceUri: Resource; }; export const IKernelProvider = Symbol('IKernelProvider'); export interface IKernelProvider extends IBaseKernelProvider { @@ -238,16 +241,16 @@ export interface IKernelProvider extends IBaseKernelProvider { } export const IThirdPartyKernelProvider = Symbol('IThirdPartyKernelProvider'); -export interface IThirdPartyKernelProvider extends IBaseKernelProvider { +export interface IThirdPartyKernelProvider extends IBaseKernelProvider { /** * Get hold of the active kernel for a given resource uri. */ - get(uri: Uri): IBaseKernel | undefined; + get(uri: Uri): IThirdPartyKernel | undefined; /** * Gets or creates a kernel for a given resource uri. * WARNING: If called with different options for same resource uri, old kernel associated with the Uri will be disposed. */ - getOrCreate(uri: Uri, options: KernelOptions): IBaseKernel; + getOrCreate(uri: Uri, options: ThirdPartyKernelOptions): IThirdPartyKernel; } export interface IBaseKernelProvider extends IAsyncDisposable { diff --git a/src/notebooks/controllers/kernelConnector.ts b/src/notebooks/controllers/kernelConnector.ts index a99c0e20978..e8c41003d4c 100644 --- a/src/notebooks/controllers/kernelConnector.ts +++ b/src/notebooks/controllers/kernelConnector.ts @@ -106,7 +106,7 @@ export class KernelConnector { errorContext: KernelAction, resource: Resource, kernel: IBaseKernel, - controller: NotebookController, + controller: NotebookController | undefined, metadata: KernelConnectionMetadata, actionSource: KernelActionSource ) { @@ -233,7 +233,6 @@ export class KernelConnector { } // Kernel is dead and we didn't prompt the user to restart it, hence re-run the code that will prompt the user for a restart. return KernelConnector.wrapKernelMethod( - kernel.controller, kernel.kernelConnectionMetadata, 'start', actionSource, @@ -248,7 +247,6 @@ export class KernelConnector { } public static async wrapKernelMethod( - controller: NotebookController, metadata: KernelConnectionMetadata, initialContext: KernelAction, actionSource: KernelActionSource, @@ -289,7 +287,6 @@ export class KernelConnector { } const promise = KernelConnector.wrapKernelMethodImpl( - controller, metadata, initialContext, serviceContainer, @@ -327,7 +324,7 @@ export class KernelConnector { ); } private static getKernelInfo(notebookResource: NotebookResource) { - return notebookResource.notebook + return 'notebook' in notebookResource ? KernelConnector.connectionsByNotebook.get(notebookResource.notebook) : KernelConnector.connectionsByUri.get(notebookResource.resource.toString()); } @@ -339,7 +336,7 @@ export class KernelConnector { }>, options: IDisplayOptions ) { - if (notebookResource.notebook) { + if ('notebook' in notebookResource) { KernelConnector.connectionsByNotebook.set(notebookResource.notebook, { kernel: deferred as Deferred<{ kernel: IKernel; @@ -359,7 +356,7 @@ export class KernelConnector { }> ) { if (!matchingKernelPromise) { - if (notebookResource.notebook) { + if ('notebook' in notebookResource) { KernelConnector.connectionsByNotebook.delete(notebookResource.notebook); } else { KernelConnector.connectionsByUri.delete(notebookResource.resource.toString()); @@ -367,7 +364,7 @@ export class KernelConnector { return; } if ( - notebookResource.notebook && + 'notebook' in notebookResource && KernelConnector.connectionsByNotebook.get(notebookResource.notebook)?.kernel.promise === matchingKernelPromise ) { @@ -382,7 +379,6 @@ export class KernelConnector { } private static async wrapKernelMethodImpl( - controller: NotebookController, metadata: KernelConnectionMetadata, initialContext: KernelAction, serviceContainer: IServiceContainer, @@ -399,21 +395,20 @@ export class KernelConnector { let kernel: IBaseKernel | undefined; let currentMethod = KernelConnector.convertContextToFunction(initialContext, options); let currentContext = initialContext; + let controller = 'controller' in notebookResource ? notebookResource.controller : undefined; while (kernel === undefined) { // Try to create the kernel (possibly again) - kernel = notebookResource.notebook - ? kernelProvider.getOrCreate(notebookResource.notebook, { - metadata, - controller, - resourceUri: notebookResource.resource, - creator: actionSource - }) - : thirdPartyKernelProvider.getOrCreate(notebookResource.resource, { - metadata, - controller, - resourceUri: notebookResource.resource, - creator: actionSource - }); + kernel = + 'notebook' in notebookResource + ? kernelProvider.getOrCreate(notebookResource.notebook, { + metadata, + controller: controller || notebookResource.controller, + resourceUri: notebookResource.resource + }) + : thirdPartyKernelProvider.getOrCreate(notebookResource.resource, { + metadata, + resourceUri: notebookResource.resource + }); const isKernelDead = (k: IBaseKernel) => k.status === 'dead' || (k.status === 'terminating' && !k.disposed && !k.disposing); @@ -477,17 +472,15 @@ export class KernelConnector { } public static async connectToNotebookKernel( - controller: NotebookController, metadata: KernelConnectionMetadata, serviceContainer: IServiceContainer, - notebookResource: { resource: Resource; notebook: NotebookDocument }, + notebookResource: { resource: Resource; notebook: NotebookDocument; controller: NotebookController }, options: IDisplayOptions, disposables: IDisposable[], actionSource: KernelActionSource = 'jupyterExtension', onAction: (action: KernelAction, kernel: IKernel) => void = () => noop() ): Promise { return KernelConnector.wrapKernelMethod( - controller, metadata, 'start', actionSource, @@ -500,7 +493,6 @@ export class KernelConnector { } public static async connectToKernel( - controller: NotebookController, metadata: KernelConnectionMetadata, serviceContainer: IServiceContainer, resource: { resource: Uri; notebook: undefined }, @@ -510,7 +502,6 @@ export class KernelConnector { onAction: (action: KernelAction, kernel: IBaseKernel) => void = () => noop() ): Promise { return KernelConnector.wrapKernelMethod( - controller, metadata, 'start', actionSource, @@ -523,4 +514,6 @@ export class KernelConnector { } } -type NotebookResource = { resource: Resource; notebook: NotebookDocument } | { resource: Uri; notebook: undefined }; +type NotebookResource = + | { resource: Resource; notebook: NotebookDocument; controller: NotebookController } + | { resource: Uri }; diff --git a/src/notebooks/controllers/vscodeNotebookController.ts b/src/notebooks/controllers/vscodeNotebookController.ts index 19cf686de1d..fb6974cb0a4 100644 --- a/src/notebooks/controllers/vscodeNotebookController.ts +++ b/src/notebooks/controllers/vscodeNotebookController.ts @@ -505,10 +505,9 @@ export class VSCodeNotebookController implements Disposable, IVSCodeNotebookCont private async connectToKernel(doc: NotebookDocument, options: IDisplayOptions): Promise { return KernelConnector.connectToNotebookKernel( - this.controller, this.kernelConnection, this.serviceContainer, - { resource: doc.uri, notebook: doc }, + { resource: doc.uri, notebook: doc, controller: this.controller }, options, this.disposables ); @@ -617,8 +616,7 @@ export class VSCodeNotebookController implements Disposable, IVSCodeNotebookCont const newKernel = this.kernelProvider.getOrCreate(document, { metadata: selectedKernelConnectionMetadata, controller: this.controller, - resourceUri: document.uri, // In the case of interactive window, we cannot pass the Uri of notebook, it must be the Py file or undefined. - creator: 'jupyterExtension' + resourceUri: document.uri // In the case of interactive window, we cannot pass the Uri of notebook, it must be the Py file or undefined. }); traceVerbose(`KernelProvider switched kernel to id = ${newKernel.kernelConnectionMetadata.id}`); diff --git a/src/notebooks/debugger/debuggingManagerBase.ts b/src/notebooks/debugger/debuggingManagerBase.ts index 52931527720..0af82b113de 100644 --- a/src/notebooks/debugger/debuggingManagerBase.ts +++ b/src/notebooks/debugger/debuggingManagerBase.ts @@ -151,8 +151,7 @@ export abstract class DebuggingManagerBase implements IDisposable { kernel = this.kernelProvider.getOrCreate(doc, { metadata: controller.connection, controller: controller?.controller, - resourceUri: doc.uri, - creator: 'jupyterExtension' + resourceUri: doc.uri }); } if (kernel && kernel.status === 'unknown') { @@ -173,8 +172,7 @@ export abstract class DebuggingManagerBase implements IDisposable { kernel = this.kernelProvider.getOrCreate(doc, { metadata: controller.connection, controller: controller?.controller, - resourceUri: doc.uri, - creator: 'jupyterExtension' + resourceUri: doc.uri }); } diff --git a/src/notebooks/notebookCommandListener.ts b/src/notebooks/notebookCommandListener.ts index b01302e5050..235f64c2f28 100644 --- a/src/notebooks/notebookCommandListener.ts +++ b/src/notebooks/notebookCommandListener.ts @@ -254,12 +254,11 @@ export class NotebookCommandListener implements IDataScienceCommandListener { } // Wrap the restart/interrupt in a loop that allows the user to switch await KernelConnector.wrapKernelMethod( - controller.controller, controller.connection, currentContext, kernel.creator, this.serviceContainer, - { resource: kernel.resourceUri, notebook }, + { resource: kernel.resourceUri, notebook, controller: controller.controller }, new DisplayOptions(false), this.disposableRegistry ); diff --git a/src/standalone/api/kernelApi.ts b/src/standalone/api/kernelApi.ts index b5e23994aa6..b88310f9833 100644 --- a/src/standalone/api/kernelApi.ts +++ b/src/standalone/api/kernelApi.ts @@ -6,9 +6,9 @@ import { Disposable, Event, EventEmitter, Uri } from 'vscode'; import { KernelConnectionWrapper } from './kernelConnectionWrapper'; import { IKernelProvider, - IBaseKernel, KernelConnectionMetadata as IKernelKernelConnectionMetadata, - IThirdPartyKernelProvider + IThirdPartyKernelProvider, + IBaseKernel } from '../../kernels/types'; import { disposeAllDisposables } from '../../platform/common/helpers'; import { traceInfo } from '../../platform/logging'; @@ -210,14 +210,13 @@ class JupyterKernelService implements IExportedKernelService { uri: Uri ): Promise { await this.controllerLoader.loadControllers(); - const controllers = this.controllerRegistration.registered; - const controller = controllers.find((item) => item.connection.id === spec.id); - if (!controller) { + const connections = this.controllerRegistration.all; + const connection = connections.find((item) => item.id === spec.id); + if (!connection) { throw new Error('Not found'); } const kernel = await KernelConnector.connectToKernel( - controller.controller, - controller.connection, + connection, this.serviceContainer, { resource: uri, notebook: undefined }, new DisplayOptions(false), diff --git a/src/test/kernels/jupyter/remoteKernelConnectionHandler.unit.test.ts b/src/test/kernels/jupyter/remoteKernelConnectionHandler.unit.test.ts index 8a946c76b35..af5ee7bbd5f 100644 --- a/src/test/kernels/jupyter/remoteKernelConnectionHandler.unit.test.ts +++ b/src/test/kernels/jupyter/remoteKernelConnectionHandler.unit.test.ts @@ -124,7 +124,7 @@ suite('Remote kernel connection handler', async () => { function verifyRemoteKernelTracking(connection: KernelConnectionMetadata, source: KernelActionSource) { const kernel1 = mock(); when(kernel1.kernelConnectionMetadata).thenReturn(connection); - when(kernel1.creator).thenReturn(source); + when(kernel1.creator).thenReturn('jupyterExtension'); const subject = new Subject(); disposables.push(new Disposable(() => subject.unsubscribe())); when(kernel1.kernelSocket).thenReturn(subject); diff --git a/src/test/kernels/kernelProvider.node.unit.test.ts b/src/test/kernels/kernelProvider.node.unit.test.ts index db355e9f579..6150ab7bf1a 100644 --- a/src/test/kernels/kernelProvider.node.unit.test.ts +++ b/src/test/kernels/kernelProvider.node.unit.test.ts @@ -107,13 +107,10 @@ suite('KernelProvider Node', () => { instance(notebookProvider), instance(configService), instance(appShell), - instance(outputTracker), instance(workspaceService), instance(vscNotebook), instance(pythonExecFactory), instance(statusProvider), - instance(context), - [], [] ); }); @@ -127,7 +124,6 @@ suite('KernelProvider Node', () => { when(metadata.id).thenReturn('xyz'); const options: KernelOptions = { controller: instance(mock()), - creator: 'jupyterExtension', metadata: instance(metadata), resourceUri: sampleUri1 }; @@ -169,7 +165,6 @@ suite('KernelProvider Node', () => { when(metadata.id).thenReturn('xyz'); const options: KernelOptions = { controller: instance(mock()), - creator: '3rdPartyExtension', metadata: instance(metadata), resourceUri: uri }; @@ -207,7 +202,6 @@ suite('KernelProvider Node', () => { when(metadata.id).thenReturn('xyz'); const options: KernelOptions = { controller: instance(mock()), - creator: 'jupyterExtension', metadata: instance(metadata), resourceUri: sampleUri1 }; @@ -227,7 +221,6 @@ suite('KernelProvider Node', () => { when(metadata.id).thenReturn('xyz'); const options: KernelOptions = { controller: instance(mock()), - creator: 'jupyterExtension', metadata: instance(metadata), resourceUri: sampleUri1 }; From 598d83a4c8e735a1dcbdea47d87323de8750e0d5 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 21 Jul 2022 16:12:59 +1000 Subject: [PATCH 2/8] Fix tests --- src/notebooks/controllers/remoteKernelConnectionHandler.ts | 3 +-- .../kernels/jupyter/remoteKernelConnectionHandler.unit.test.ts | 3 --- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/notebooks/controllers/remoteKernelConnectionHandler.ts b/src/notebooks/controllers/remoteKernelConnectionHandler.ts index cc93f167541..a20d292dca8 100644 --- a/src/notebooks/controllers/remoteKernelConnectionHandler.ts +++ b/src/notebooks/controllers/remoteKernelConnectionHandler.ts @@ -63,8 +63,7 @@ export class RemoteKernelConnectionHandler implements IExtensionSyncActivationSe } } private onDidStartKernel(kernel: IKernel) { - // TODO@rebornix, IKernel is already created by Jupyter Extension - if (kernel.creator !== 'jupyterExtension' || !kernel.resourceUri) { + if (!kernel.resourceUri) { return; } const resource = kernel.resourceUri; diff --git a/src/test/kernels/jupyter/remoteKernelConnectionHandler.unit.test.ts b/src/test/kernels/jupyter/remoteKernelConnectionHandler.unit.test.ts index af5ee7bbd5f..07bbd79f5b7 100644 --- a/src/test/kernels/jupyter/remoteKernelConnectionHandler.unit.test.ts +++ b/src/test/kernels/jupyter/remoteKernelConnectionHandler.unit.test.ts @@ -199,9 +199,6 @@ suite('Remote kernel connection handler', async () => { test('When starting a remote kernel spec ensure we track this', async () => { verifyRemoteKernelTracking(remoteKernelSpec, 'jupyterExtension'); }); - test('When starting a remote kernel spec from a 3rd party extension ensure we do not track this', async () => { - verifyRemoteKernelTracking(remoteKernelSpec, '3rdPartyExtension'); - }); test('When starting a local kernel spec ensure we do not track this', async () => { verifyRemoteKernelTracking(localKernelSpec, 'jupyterExtension'); }); From bf7518e3478df7077f6c8609e4dde5f1f1704d00 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 22 Jul 2022 02:43:03 +1000 Subject: [PATCH 3/8] Fix tests --- src/kernels/helpers.ts | 4 ++-- src/notebooks/controllers/kernelConnector.ts | 2 +- src/standalone/api/kernelApi.ts | 2 +- src/test/client/api.vscode.test.ts | 9 +++++++-- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/kernels/helpers.ts b/src/kernels/helpers.ts index 42e323ec8b2..6c1c8a5c7b4 100644 --- a/src/kernels/helpers.ts +++ b/src/kernels/helpers.ts @@ -6,7 +6,7 @@ const NamedRegexp = require('named-js-regexp') as typeof import('named-js-regexp import * as path from '../platform/vscode-path/path'; import * as uriPath from '../platform/vscode-path/resources'; import * as nbformat from '@jupyterlab/nbformat'; -import type { KernelSpec } from '@jupyterlab/services'; +import type { Kernel, KernelSpec } from '@jupyterlab/services'; // eslint-disable-next-line @typescript-eslint/no-require-imports import cloneDeep = require('lodash/cloneDeep'); import * as url from 'url-parse'; @@ -1452,7 +1452,7 @@ export type SilentExecutionErrorOptions = { }; export async function executeSilently( - session: IKernelConnectionSession, + session: IKernelConnectionSession | Kernel.IKernelConnection, code: string, errorOptions?: SilentExecutionErrorOptions ): Promise { diff --git a/src/notebooks/controllers/kernelConnector.ts b/src/notebooks/controllers/kernelConnector.ts index e8c41003d4c..b990435c317 100644 --- a/src/notebooks/controllers/kernelConnector.ts +++ b/src/notebooks/controllers/kernelConnector.ts @@ -495,7 +495,7 @@ export class KernelConnector { public static async connectToKernel( metadata: KernelConnectionMetadata, serviceContainer: IServiceContainer, - resource: { resource: Uri; notebook: undefined }, + resource: { resource: Uri }, options: IDisplayOptions, disposables: IDisposable[], actionSource: KernelActionSource = 'jupyterExtension', diff --git a/src/standalone/api/kernelApi.ts b/src/standalone/api/kernelApi.ts index b88310f9833..697098a8cec 100644 --- a/src/standalone/api/kernelApi.ts +++ b/src/standalone/api/kernelApi.ts @@ -218,7 +218,7 @@ class JupyterKernelService implements IExportedKernelService { const kernel = await KernelConnector.connectToKernel( connection, this.serviceContainer, - { resource: uri, notebook: undefined }, + { resource: uri }, new DisplayOptions(false), this.disposables, '3rdPartyExtension' diff --git a/src/test/client/api.vscode.test.ts b/src/test/client/api.vscode.test.ts index 624f840c3db..f1d4250bb7b 100644 --- a/src/test/client/api.vscode.test.ts +++ b/src/test/client/api.vscode.test.ts @@ -19,6 +19,7 @@ import { captureScreenShot, createEventHandler, IExtensionTestApi } from '../com import { IVSCodeNotebook } from '../../platform/common/application/types'; import { IS_REMOTE_NATIVE_TEST } from '../constants.node'; import { workspace } from 'vscode'; +import { executeSilently } from '../../kernels/helpers'; suite('3rd Party Kernel Service API', function () { let api: IExtensionTestApi; @@ -131,10 +132,14 @@ suite('3rd Party Kernel Service API', function () { 'Kernel notebook is not the active notebook' ); - assert.strictEqual(kernels![0].metadata, pythonKernel, 'Kernel Connection is not the same'); + assert.strictEqual(kernels![0].metadata.id, pythonKernel?.id, 'Kernel Connection is not the same'); const kernel = kernelService?.getKernel(nb.uri); - assert.strictEqual(kernels![0].metadata, kernel!.metadata, 'Kernel Connection not same for the document'); + assert.strictEqual(kernels![0].metadata.id, kernel!.metadata.id, 'Kernel Connection not same for the document'); + // Verify we can run some code against this kernel. + const outputs = await executeSilently(kernel?.connection.connection!, '98765'); + assert.strictEqual(outputs.length, 1); + assert.strictEqual((outputs[0]!.data as { 'text/plain': string })['text/plain'].trim(), '98765'); await closeNotebooksAndCleanUpAfterTests(disposables); await onDidChangeKernels.assertFiredExactly(2); From 7030aed8fbed47a1a8637dcf6aa54bf7dfe6c30b Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 22 Jul 2022 02:51:31 +1000 Subject: [PATCH 4/8] Misc --- src/kernels/execution/kernelExecution.ts | 29 ++++++++---------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/src/kernels/execution/kernelExecution.ts b/src/kernels/execution/kernelExecution.ts index 4265662a7a2..9d4f2d7c89f 100644 --- a/src/kernels/execution/kernelExecution.ts +++ b/src/kernels/execution/kernelExecution.ts @@ -52,13 +52,12 @@ export class BaseKernelExecution impl * If we don't have a kernel (Jupyter Session) available, then just abort all of the cell executions. */ public async interrupt(sessionPromise?: Promise): Promise { - // Possible we don't have a notebook. const session = sessionPromise ? await sessionPromise.catch(() => undefined) : undefined; const pendingExecutions = this.cancelPendingExecutions(); traceInfo('Interrupt kernel execution'); if (!session) { - traceInfo('No notebook to interrupt'); + traceInfo('No kernel session to interrupt'); this._interruptPromise = undefined; await pendingExecutions; return InterruptResult.Success; @@ -85,7 +84,7 @@ export class BaseKernelExecution impl const session = sessionPromise ? await sessionPromise.catch(() => undefined) : undefined; if (!session) { - traceInfo('No notebook to interrupt'); + traceInfo('No kernel session to interrupt'); this._restartPromise = undefined; return; } @@ -221,8 +220,7 @@ export class KernelExecution extends BaseKernelExecution { return this._onPreExecute.event; } public get queue() { - const notebook = this.kernel.notebook; - return notebook ? this.documentExecutions.get(notebook)?.queue || [] : []; + return this.documentExecutions.get(this.kernel.notebook)?.queue || []; } public async executeCell( sessionPromise: Promise, @@ -246,11 +244,7 @@ export class KernelExecution extends BaseKernelExecution { } public override async cancel() { await super.cancel(); - const notebook = this.kernel.notebook; - if (!notebook) { - return; - } - const executionQueue = this.documentExecutions.get(notebook); + const executionQueue = this.documentExecutions.get(this.kernel.notebook); if (executionQueue) { await executionQueue.cancel(true); } @@ -262,17 +256,15 @@ export class KernelExecution extends BaseKernelExecution { */ public override async interrupt(sessionPromise?: Promise): Promise { trackKernelResourceInformation(this.kernel.resourceUri, { interruptKernel: true }); - const notebook = this.kernel.notebook; - const executionQueue = notebook ? this.documentExecutions.get(notebook) : undefined; - if (notebook && !executionQueue && this.kernel.kernelConnectionMetadata.kind !== 'connectToLiveRemoteKernel') { + const executionQueue = this.documentExecutions.get(this.kernel.notebook); + if (!executionQueue && this.kernel.kernelConnectionMetadata.kind !== 'connectToLiveRemoteKernel') { return InterruptResult.Success; } return super.interrupt(sessionPromise); } protected override async cancelPendingExecutions(): Promise { - const notebook = this.kernel.notebook; - const executionQueue = notebook ? this.documentExecutions.get(notebook) : undefined; - if (notebook && !executionQueue && this.kernel.kernelConnectionMetadata.kind !== 'connectToLiveRemoteKernel') { + const executionQueue = this.documentExecutions.get(this.kernel.notebook); + if (!executionQueue && this.kernel.kernelConnectionMetadata.kind !== 'connectToLiveRemoteKernel') { return; } traceInfo('Interrupt kernel execution'); @@ -292,8 +284,7 @@ export class KernelExecution extends BaseKernelExecution { */ public override async restart(sessionPromise?: Promise): Promise { trackKernelResourceInformation(this.kernel.resourceUri, { restartKernel: true }); - const notebook = this.kernel.notebook; - const executionQueue = notebook ? this.documentExecutions.get(notebook) : undefined; + const executionQueue = this.documentExecutions.get(this.kernel.notebook); // Possible we don't have a notebook. const session = sessionPromise ? await sessionPromise.catch(() => undefined) : undefined; traceInfo('Restart kernel execution'); @@ -306,7 +297,7 @@ export class KernelExecution extends BaseKernelExecution { : Promise.resolve(); if (!session) { - traceInfo('No notebook to interrupt'); + traceInfo('No kernel session to interrupt'); await pendingCells; } From c629b33548b7f2519bc60e1868943542e60a71dd Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 22 Jul 2022 02:54:38 +1000 Subject: [PATCH 5/8] Remote notebook --- src/kernels/kernel.ts | 13 +------------ src/kernels/kernelProvider.node.ts | 1 - src/kernels/kernelProvider.web.ts | 1 - 3 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/kernels/kernel.ts b/src/kernels/kernel.ts index d5ee71d9b6a..c57074e3c15 100644 --- a/src/kernels/kernel.ts +++ b/src/kernels/kernel.ts @@ -74,9 +74,6 @@ abstract class BaseKernel implemen get onStatusChanged(): Event { return this._onStatusChanged.event; } - get notebook(): NotebookDocument | undefined { - return this._notebookDocument; - } get onRestarted(): Event { return this._onRestarted.event; } @@ -147,7 +144,6 @@ abstract class BaseKernel implemen constructor( public readonly uri: Uri, public readonly resourceUri: Resource, - private readonly _notebookDocument: NotebookDocument | undefined, public readonly kernelConnectionMetadata: Readonly, protected readonly notebookProvider: INotebookProvider, protected readonly launchTimeout: number, @@ -709,7 +705,6 @@ export class ThirdPartyKernel extends BaseKernel { constructor( uri: Uri, resourceUri: Resource, - notebook: NotebookDocument | undefined, kernelConnectionMetadata: Readonly, notebookProvider: INotebookProvider, launchTimeout: number, @@ -724,7 +719,6 @@ export class ThirdPartyKernel extends BaseKernel { super( uri, resourceUri, - notebook, kernelConnectionMetadata, notebookProvider, launchTimeout, @@ -754,14 +748,10 @@ export class Kernel extends BaseKernel implements IKernel { return this._onPreExecute.event; } protected readonly _onPreExecute = new EventEmitter(); - override get notebook(): NotebookDocument { - return this._notebook; - } - constructor( uri: Uri, resourceUri: Resource, - private _notebook: NotebookDocument, + public readonly notebook: NotebookDocument, kernelConnectionMetadata: Readonly, notebookProvider: INotebookProvider, launchTimeout: number, @@ -780,7 +770,6 @@ export class Kernel extends BaseKernel implements IKernel { super( uri, resourceUri, - _notebook, kernelConnectionMetadata, notebookProvider, launchTimeout, diff --git a/src/kernels/kernelProvider.node.ts b/src/kernels/kernelProvider.node.ts index f6d6281d332..b58efc42bfb 100644 --- a/src/kernels/kernelProvider.node.ts +++ b/src/kernels/kernelProvider.node.ts @@ -138,7 +138,6 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { const kernel: IThirdPartyKernel = new ThirdPartyKernel( uri, resourceUri, - undefined, options.metadata, this.notebookProvider, waitForIdleTimeout, diff --git a/src/kernels/kernelProvider.web.ts b/src/kernels/kernelProvider.web.ts index 91ea39bf0cb..f26efc11aef 100644 --- a/src/kernels/kernelProvider.web.ts +++ b/src/kernels/kernelProvider.web.ts @@ -123,7 +123,6 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { const kernel = new ThirdPartyKernel( uri, resourceUri, - undefined, options.metadata, this.notebookProvider, waitForIdleTimeout, From 3f751345bcc3b3e2b0ec0af7bb9de9de394faaa6 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 22 Jul 2022 03:11:13 +1000 Subject: [PATCH 6/8] Misc fixes --- src/kernels/kernel.ts | 122 +++++++++++++++++++++--------------------- src/kernels/types.ts | 8 +-- 2 files changed, 64 insertions(+), 66 deletions(-) diff --git a/src/kernels/kernel.ts b/src/kernels/kernel.ts index c57074e3c15..4ab7423c5be 100644 --- a/src/kernels/kernel.ts +++ b/src/kernels/kernel.ts @@ -89,8 +89,8 @@ abstract class BaseKernel implemen get startedAtLeastOnce() { return this._startedAtLeastOnce; } - protected _info?: KernelMessage.IInfoReplyMsg['content']; - protected _startedAtLeastOnce?: boolean; + private _info?: KernelMessage.IInfoReplyMsg['content']; + private _startedAtLeastOnce?: boolean; get info(): KernelMessage.IInfoReplyMsg['content'] | undefined { return this._info; } @@ -109,37 +109,33 @@ abstract class BaseKernel implemen get kernelSocket(): Observable { return this._kernelSocket.asObservable(); } - get executionCount(): number { - return this._visibleExecutionCount; - } - protected _session?: IKernelConnectionSession; + private _session?: IKernelConnectionSession; /** * If the session died, then ensure the status is set to `dead`. * We need to provide an accurate status. * `unknown` is generally used to indicate jupyter kernel hasn't started. * If a jupyter kernel dies after it has started, then status is set to `dead`. */ - protected isKernelDead?: boolean; + private isKernelDead?: boolean; public get session(): IKernelConnectionSession | undefined { return this._session; } - protected _disposed?: boolean; - protected _disposing?: boolean; - protected _ignoreJupyterSessionDisposedErrors?: boolean; - protected readonly _kernelSocket = new ReplaySubject(); - protected readonly _onStatusChanged = new EventEmitter(); - protected readonly _onRestarted = new EventEmitter(); - protected readonly _onStarted = new EventEmitter(); - protected readonly _onDisposed = new EventEmitter(); - protected _jupyterSessionPromise?: Promise; - protected readonly hookedSessionForEvents = new WeakSet(); - protected eventHooks: ((ev: 'willInterrupt' | 'willRestart') => Promise)[] = []; - protected restarting?: Deferred; - protected startCancellation = new CancellationTokenSource(); - protected startupUI = new DisplayOptions(true); + private _disposed?: boolean; + private _disposing?: boolean; + private _ignoreJupyterSessionDisposedErrors?: boolean; + private readonly _kernelSocket = new ReplaySubject(); + private readonly _onStatusChanged = new EventEmitter(); + private readonly _onRestarted = new EventEmitter(); + private readonly _onStarted = new EventEmitter(); + private readonly _onDisposed = new EventEmitter(); + private _jupyterSessionPromise?: Promise; + private readonly hookedSessionForEvents = new WeakSet(); + private eventHooks: ((ev: 'willInterrupt' | 'willRestart') => Promise)[] = []; + private restarting?: Deferred; + private startCancellation = new CancellationTokenSource(); + private startupUI = new DisplayOptions(true); protected kernelExecution: TKernelExecution; - protected disposingPromise?: Promise; - protected _visibleExecutionCount = 0; + private disposingPromise?: Promise; constructor( public readonly uri: Uri, @@ -162,7 +158,6 @@ abstract class BaseKernel implemen this.disposables.push(this._onDisposed); this.disposables.push({ dispose: () => this._kernelSocket.unsubscribe() }); } - protected perceivedJupyterStartupTelemetryCaptured?: boolean; public addEventHook(hook: (event: 'willRestart' | 'willInterrupt') => Promise): void { this.eventHooks.push(hook); @@ -172,13 +167,6 @@ abstract class BaseKernel implemen this.eventHooks = this.eventHooks.filter((h) => h !== hook); } - public async executeHidden(code: string): Promise { - const stopWatch = new StopWatch(); - const sessionPromise = this.startJupyterSession(); - const promise = sessionPromise.then((session) => executeSilently(session, code)); - this.trackNotebookCellPerceivedColdTime(stopWatch, sessionPromise, promise).catch(noop); - return promise; - } public async start(options?: IDisplayOptions): Promise { await this.startJupyterSession(options); } @@ -294,29 +282,6 @@ abstract class BaseKernel implemen // Indicate a restart occurred if it succeeds this._onRestarted.fire(); } - protected async trackNotebookCellPerceivedColdTime( - stopWatch: StopWatch, - started: Promise, - executionPromise: Promise - ): Promise { - if (this.perceivedJupyterStartupTelemetryCaptured) { - return; - } - const session = await started; - if (!session) { - return; - } - // Setup telemetry - if (!this.perceivedJupyterStartupTelemetryCaptured) { - this.perceivedJupyterStartupTelemetryCaptured = true; - sendTelemetryEvent(Telemetry.PerceivedJupyterStartupNotebook, stopWatch.elapsedTime); - executionPromise - .finally(() => - sendTelemetryEvent(Telemetry.StartExecuteNotebookCellPerceivedCold, stopWatch.elapsedTime) - ) - .catch(noop); - } - } protected async startJupyterSession( options: IDisplayOptions = new DisplayOptions(false) ): Promise { @@ -471,7 +436,6 @@ abstract class BaseKernel implemen traceVerbose('Not running kernel initialization'); return; } - this._visibleExecutionCount = 0; if (!this.hookedSessionForEvents.has(session)) { this.hookedSessionForEvents.add(session); session.kernelSocket.subscribe(this._kernelSocket); @@ -686,7 +650,7 @@ abstract class BaseKernel implemen } protected async executeSilently( - session: IKernelConnectionSession | undefined, + session: IKernelConnectionSession, code: string[], errorOptions?: SilentExecutionErrorOptions ) { @@ -747,7 +711,12 @@ export class Kernel extends BaseKernel implements IKernel { get onPreExecute(): Event { return this._onPreExecute.event; } - protected readonly _onPreExecute = new EventEmitter(); + get executionCount(): number { + return this._visibleExecutionCount; + } + private _visibleExecutionCount = 0; + private readonly _onPreExecute = new EventEmitter(); + private perceivedJupyterStartupTelemetryCaptured?: boolean; constructor( uri: Uri, resourceUri: Resource, @@ -793,13 +762,8 @@ export class Kernel extends BaseKernel implements IKernel { ); this.kernelExecution.onPreExecute((c) => this._onPreExecute.fire(c), this, this.disposables); this.disposables.push(this.kernelExecution); - this.disposables.push(this._onStatusChanged); - this.disposables.push(this._onRestarted); - this.disposables.push(this._onStarted); - this.disposables.push(this._onDisposed); this.disposables.push(this._onPreExecute); this.disposables.push(this.kernelExecution); - this.disposables.push({ dispose: () => this._kernelSocket.unsubscribe() }); } public get pendingCells(): readonly NotebookCell[] { return this.kernelExecution.queue; @@ -815,6 +779,40 @@ export class Kernel extends BaseKernel implements IKernel { promise.then((state) => traceInfo(`Cell ${cell.index} executed with state ${state}`), noop); return promise; } + public async executeHidden(code: string): Promise { + const stopWatch = new StopWatch(); + const sessionPromise = this.startJupyterSession(); + const promise = sessionPromise.then((session) => executeSilently(session, code)); + this.trackNotebookCellPerceivedColdTime(stopWatch, sessionPromise, promise).catch(noop); + return promise; + } + protected async trackNotebookCellPerceivedColdTime( + stopWatch: StopWatch, + started: Promise, + executionPromise: Promise + ): Promise { + if (this.perceivedJupyterStartupTelemetryCaptured) { + return; + } + const session = await started; + if (!session) { + return; + } + // Setup telemetry + if (!this.perceivedJupyterStartupTelemetryCaptured) { + this.perceivedJupyterStartupTelemetryCaptured = true; + sendTelemetryEvent(Telemetry.PerceivedJupyterStartupNotebook, stopWatch.elapsedTime); + executionPromise + .finally(() => + sendTelemetryEvent(Telemetry.StartExecuteNotebookCellPerceivedCold, stopWatch.elapsedTime) + ) + .catch(noop); + } + } + protected override async initializeAfterStart(session: IKernelConnectionSession | undefined) { + this._visibleExecutionCount = 0; + return super.initializeAfterStart(session); + } } // Wrap a block of python code in try except to make sure hat we have n diff --git a/src/kernels/types.ts b/src/kernels/types.ts index 35fcb505aa0..186d4cbbd6f 100644 --- a/src/kernels/types.ts +++ b/src/kernels/types.ts @@ -131,6 +131,10 @@ export function isRemoteConnection( } export interface IKernel extends IBaseKernel { + /** + * Total execution count on this kernel + */ + readonly executionCount: number; readonly notebook: NotebookDocument; readonly onPreExecute: Event; /** @@ -154,10 +158,6 @@ export interface IKernel extends IBaseKernel { } export interface IBaseKernel extends IAsyncDisposable { - /** - * Total execution count on this kernel - */ - readonly executionCount: number; readonly uri: Uri; /** * In the case of Notebooks, this is the same as the Notebook Uri. From 2a38cb9faa1c6da0144f08246522c7be37c1522d Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 22 Jul 2022 03:23:28 +1000 Subject: [PATCH 7/8] Misc --- src/kernels/kernel.ts | 68 ++++++++----------- src/kernels/kernelProvider.node.ts | 21 +----- src/kernels/kernelProvider.web.ts | 9 +-- .../kernels/kernelProvider.node.unit.test.ts | 5 -- 4 files changed, 33 insertions(+), 70 deletions(-) diff --git a/src/kernels/kernel.ts b/src/kernels/kernel.ts index 4ab7423c5be..1dd65b6621d 100644 --- a/src/kernels/kernel.ts +++ b/src/kernels/kernel.ts @@ -18,7 +18,7 @@ import { NotebookDocument } from 'vscode'; import { CodeSnippets, Identifiers } from '../platform/common/constants'; -import { IApplicationShell, IWorkspaceService } from '../platform/common/application/types'; +import { IApplicationShell } from '../platform/common/application/types'; import { WrappedError } from '../platform/errors/types'; import { disposeAllDisposables } from '../platform/common/helpers'; import { traceInfo, traceInfoIfCI, traceError, traceVerbose, traceWarning } from '../platform/logging'; @@ -146,11 +146,9 @@ abstract class BaseKernel implemen protected readonly interruptTimeout: number, protected readonly appShell: IApplicationShell, protected readonly configService: IConfigurationService, - protected readonly workspaceService: IWorkspaceService, protected readonly statusProvider: IStatusProvider, protected readonly startupCodeProviders: IStartupCodeProvider[], - private readonly _creator: KernelActionSource, - protected readonly sendTelemetryForPythonKernelExecutable: () => Promise + private readonly _creator: KernelActionSource ) { this.disposables.push(this._onStatusChanged); this.disposables.push(this._onRestarted); @@ -475,15 +473,6 @@ abstract class BaseKernel implemen // Restart sessions and retries might make this hard to do correctly otherwise. session.registerCommTarget(Identifiers.DefaultCommTarget, noop); - if (isPythonKernelConnection(this.kernelConnectionMetadata)) { - // Request completions to warm up the completion engine. - this.requestEmptyCompletions(); - - if (isLocalConnection(this.kernelConnectionMetadata)) { - await this.sendTelemetryForPythonKernelExecutable(); - } - } - // If this is a live kernel, we shouldn't be changing anything by running startup code. if (this.kernelConnectionMetadata.kind !== 'connectToLiveRemoteKernel') { // Gather all of the startup code at one time and execute as one cell @@ -580,21 +569,6 @@ abstract class BaseKernel implemen return result; } - /** - * Do not wait for completions, - * If the completions request crashes then we don't get a response for this request, - * Hence we end up waiting indefinitely. - * https://github.com/microsoft/vscode-jupyter/issues/9014 - */ - protected requestEmptyCompletions() { - this.session - ?.requestComplete({ - code: '__file__.', - cursor_pos: 9 - }) - .ignoreErrors(); - } - protected getMatplotLibInitializeCode(): string[] { const results: string[] = []; @@ -675,10 +649,8 @@ export class ThirdPartyKernel extends BaseKernel { interruptTimeout: number, appShell: IApplicationShell, configService: IConfigurationService, - workspaceService: IWorkspaceService, statusProvider: IStatusProvider, - startupCodeProviders: IStartupCodeProvider[], - sendTelemetryForPythonKernelExecutable: () => Promise + startupCodeProviders: IStartupCodeProvider[] ) { super( uri, @@ -689,11 +661,9 @@ export class ThirdPartyKernel extends BaseKernel { interruptTimeout, appShell, configService, - workspaceService, statusProvider, startupCodeProviders, - '3rdPartyExtension', - sendTelemetryForPythonKernelExecutable + '3rdPartyExtension' ); this.kernelExecution = new ThirdPartyKernelExecution(this, this.interruptTimeout); this.disposables.push(this.kernelExecution); @@ -729,12 +699,11 @@ export class Kernel extends BaseKernel implements IKernel { public readonly controller: NotebookController, configService: IConfigurationService, outputTracker: CellOutputDisplayIdTracker, - workspaceService: IWorkspaceService, statusProvider: IStatusProvider, context: IExtensionContext, formatters: ITracebackFormatter[], startupCodeProviders: IStartupCodeProvider[], - sendTelemetryForPythonKernelExecutable: () => Promise + private readonly sendTelemetryForPythonKernelExecutable: () => Promise ) { super( uri, @@ -745,11 +714,9 @@ export class Kernel extends BaseKernel implements IKernel { interruptTimeout, appShell, configService, - workspaceService, statusProvider, startupCodeProviders, - 'jupyterExtension', - sendTelemetryForPythonKernelExecutable + 'jupyterExtension' ); this.kernelExecution = new KernelExecution( @@ -811,8 +778,31 @@ export class Kernel extends BaseKernel implements IKernel { } protected override async initializeAfterStart(session: IKernelConnectionSession | undefined) { this._visibleExecutionCount = 0; + if (session && isPythonKernelConnection(this.kernelConnectionMetadata)) { + // Request completions to warm up the completion engine. + this.requestEmptyCompletions(session); + + if (isLocalConnection(this.kernelConnectionMetadata)) { + await this.sendTelemetryForPythonKernelExecutable(); + } + } return super.initializeAfterStart(session); } + + /** + * Do not wait for completions, + * If the completions request crashes then we don't get a response for this request, + * Hence we end up waiting indefinitely. + * https://github.com/microsoft/vscode-jupyter/issues/9014 + */ + private requestEmptyCompletions(session: IKernelConnectionSession) { + session + ?.requestComplete({ + code: '__file__.', + cursor_pos: 9 + }) + .ignoreErrors(); + } } // Wrap a block of python code in try except to make sure hat we have n diff --git a/src/kernels/kernelProvider.node.ts b/src/kernels/kernelProvider.node.ts index b58efc42bfb..1786843f6d2 100644 --- a/src/kernels/kernelProvider.node.ts +++ b/src/kernels/kernelProvider.node.ts @@ -4,7 +4,7 @@ 'use strict'; import { inject, injectable, multiInject } from 'inversify'; import { NotebookDocument, Uri } from 'vscode'; -import { IApplicationShell, IWorkspaceService, IVSCodeNotebook } from '../platform/common/application/types'; +import { IApplicationShell, IVSCodeNotebook } from '../platform/common/application/types'; import { IPythonExecutionFactory } from '../platform/common/process/types.node'; import { IAsyncDisposableRegistry, @@ -40,7 +40,6 @@ export class KernelProvider extends BaseCoreKernelProvider { @inject(IConfigurationService) private configService: IConfigurationService, @inject(IApplicationShell) private readonly appShell: IApplicationShell, @inject(CellOutputDisplayIdTracker) private readonly outputTracker: CellOutputDisplayIdTracker, - @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IVSCodeNotebook) notebook: IVSCodeNotebook, @inject(IPythonExecutionFactory) private readonly pythonExecutionFactory: IPythonExecutionFactory, @inject(IStatusProvider) private readonly statusProvider: IStatusProvider, @@ -74,7 +73,6 @@ export class KernelProvider extends BaseCoreKernelProvider { options.controller, this.configService, this.outputTracker, - this.workspaceService, this.statusProvider, this.context, this.formatters, @@ -115,9 +113,7 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { @inject(INotebookProvider) private notebookProvider: INotebookProvider, @inject(IConfigurationService) private configService: IConfigurationService, @inject(IApplicationShell) private readonly appShell: IApplicationShell, - @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IVSCodeNotebook) notebook: IVSCodeNotebook, - @inject(IPythonExecutionFactory) private readonly pythonExecutionFactory: IPythonExecutionFactory, @inject(IStatusProvider) private readonly statusProvider: IStatusProvider, @multiInject(IStartupCodeProvider) private readonly startupCodeProviders: IStartupCodeProvider[] ) { @@ -144,21 +140,8 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { interruptTimeout, this.appShell, this.configService, - this.workspaceService, this.statusProvider, - this.startupCodeProviders, - () => { - if (kernel.session) { - return sendTelemetryForPythonKernelExecutable( - kernel.session, - kernel.resourceUri, - kernel.kernelConnectionMetadata, - this.pythonExecutionFactory - ); - } else { - return Promise.resolve(); - } - } + this.startupCodeProviders ); kernel.onRestarted(() => this._onDidRestartKernel.fire(kernel), this, this.disposables); kernel.onDisposed(() => this._onDidDisposeKernel.fire(kernel), this, this.disposables); diff --git a/src/kernels/kernelProvider.web.ts b/src/kernels/kernelProvider.web.ts index f26efc11aef..2a40ace1a69 100644 --- a/src/kernels/kernelProvider.web.ts +++ b/src/kernels/kernelProvider.web.ts @@ -3,7 +3,7 @@ 'use strict'; import { inject, injectable, multiInject } from 'inversify'; -import { IApplicationShell, IVSCodeNotebook, IWorkspaceService } from '../platform/common/application/types'; +import { IApplicationShell, IVSCodeNotebook } from '../platform/common/application/types'; import { InteractiveWindowView } from '../platform/common/constants'; import { NotebookDocument, Uri } from 'vscode'; import { @@ -38,7 +38,6 @@ export class KernelProvider extends BaseCoreKernelProvider { @inject(IConfigurationService) private configService: IConfigurationService, @inject(IApplicationShell) private readonly appShell: IApplicationShell, @inject(CellOutputDisplayIdTracker) private readonly outputTracker: CellOutputDisplayIdTracker, - @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IVSCodeNotebook) notebook: IVSCodeNotebook, @inject(IStatusProvider) private readonly statusProvider: IStatusProvider, @inject(IExtensionContext) private readonly context: IExtensionContext, @@ -71,7 +70,6 @@ export class KernelProvider extends BaseCoreKernelProvider { options.controller, this.configService, this.outputTracker, - this.workspaceService, this.statusProvider, this.context, this.formatters, @@ -102,7 +100,6 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { @inject(INotebookProvider) private notebookProvider: INotebookProvider, @inject(IConfigurationService) private configService: IConfigurationService, @inject(IApplicationShell) private readonly appShell: IApplicationShell, - @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IVSCodeNotebook) notebook: IVSCodeNotebook, @inject(IStatusProvider) private readonly statusProvider: IStatusProvider, @multiInject(IStartupCodeProvider) private readonly startupCodeProviders: IStartupCodeProvider[] @@ -129,10 +126,8 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { interruptTimeout, this.appShell, this.configService, - this.workspaceService, this.statusProvider, - this.startupCodeProviders, - () => Promise.resolve() + this.startupCodeProviders ); kernel.onRestarted(() => this._onDidRestartKernel.fire(kernel), this, this.disposables); kernel.onDisposed(() => this._onDidDisposeKernel.fire(kernel), this, this.disposables); diff --git a/src/test/kernels/kernelProvider.node.unit.test.ts b/src/test/kernels/kernelProvider.node.unit.test.ts index 6150ab7bf1a..907f6f4813d 100644 --- a/src/test/kernels/kernelProvider.node.unit.test.ts +++ b/src/test/kernels/kernelProvider.node.unit.test.ts @@ -37,7 +37,6 @@ suite('KernelProvider Node', () => { let configService: IConfigurationService; let appShell: IApplicationShell; let outputTracker: CellOutputDisplayIdTracker; - let workspaceService: IWorkspaceService; let vscNotebook: IVSCodeNotebook; let statusProvider: IStatusProvider; let pythonExecFactory: IPythonExecutionFactory; @@ -72,7 +71,6 @@ suite('KernelProvider Node', () => { configService = mock(); appShell = mock(); outputTracker = mock(); - workspaceService = mock(); vscNotebook = mock(); statusProvider = mock(); context = mock(); @@ -93,7 +91,6 @@ suite('KernelProvider Node', () => { instance(configService), instance(appShell), instance(outputTracker), - instance(workspaceService), instance(vscNotebook), instance(pythonExecFactory), instance(statusProvider), @@ -107,9 +104,7 @@ suite('KernelProvider Node', () => { instance(notebookProvider), instance(configService), instance(appShell), - instance(workspaceService), instance(vscNotebook), - instance(pythonExecFactory), instance(statusProvider), [] ); From b3d29c8df7b5870de466eb77d1aa6e6b6932b113 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 22 Jul 2022 03:26:20 +1000 Subject: [PATCH 8/8] Misc --- src/test/client/api.vscode.test.ts | 3 ++- src/test/kernels/kernelProvider.node.unit.test.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/client/api.vscode.test.ts b/src/test/client/api.vscode.test.ts index f1d4250bb7b..dafdd50e026 100644 --- a/src/test/client/api.vscode.test.ts +++ b/src/test/client/api.vscode.test.ts @@ -20,6 +20,7 @@ import { IVSCodeNotebook } from '../../platform/common/application/types'; import { IS_REMOTE_NATIVE_TEST } from '../constants.node'; import { workspace } from 'vscode'; import { executeSilently } from '../../kernels/helpers'; +import { getPlainTextOrStreamOutput } from '../../kernels/kernel'; suite('3rd Party Kernel Service API', function () { let api: IExtensionTestApi; @@ -139,7 +140,7 @@ suite('3rd Party Kernel Service API', function () { // Verify we can run some code against this kernel. const outputs = await executeSilently(kernel?.connection.connection!, '98765'); assert.strictEqual(outputs.length, 1); - assert.strictEqual((outputs[0]!.data as { 'text/plain': string })['text/plain'].trim(), '98765'); + assert.include(getPlainTextOrStreamOutput(outputs), '98765'); await closeNotebooksAndCleanUpAfterTests(disposables); await onDidChangeKernels.assertFiredExactly(2); diff --git a/src/test/kernels/kernelProvider.node.unit.test.ts b/src/test/kernels/kernelProvider.node.unit.test.ts index 907f6f4813d..f8d5167ee4b 100644 --- a/src/test/kernels/kernelProvider.node.unit.test.ts +++ b/src/test/kernels/kernelProvider.node.unit.test.ts @@ -13,7 +13,7 @@ import { KernelOptions, IKernelProvider } from '../../kernels/types'; -import { IApplicationShell, IVSCodeNotebook, IWorkspaceService } from '../../platform/common/application/types'; +import { IApplicationShell, IVSCodeNotebook } from '../../platform/common/application/types'; import { AsyncDisposableRegistry } from '../../platform/common/asyncDisposableRegistry'; import { JupyterNotebookView } from '../../platform/common/constants'; import { disposeAllDisposables } from '../../platform/common/helpers';