From 9edd908ecb099bf4d27332d9576ca39639b3103e Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 18 Oct 2024 09:32:38 -0700 Subject: [PATCH 01/16] event validated --- src/api.d.ts | 7 +++++ src/kernels/kernel.ts | 5 ++++ src/kernels/kernelProvider.base.ts | 10 +++++++ src/kernels/kernelProvider.node.ts | 14 ++++++++++ src/kernels/types.ts | 2 ++ src/standalone/api/kernels/index.ts | 43 +++++++++++++++++++---------- 6 files changed, 67 insertions(+), 14 deletions(-) diff --git a/src/api.d.ts b/src/api.d.ts index 64dad363d6e..b7f49391625 100644 --- a/src/api.d.ts +++ b/src/api.d.ts @@ -233,6 +233,13 @@ export interface Kernel { executeCode(code: string, token: CancellationToken): AsyncIterable; } export interface Kernels { + /** + * Event fired when a kernel becomes available for execution. + */ + onDidInitialize: Event<{ + uri: Uri; + }>; + /** * Gets an the kernel associated with a given resource. * For instance if the resource is a notebook, then get the kernel associated with the given Notebook document. diff --git a/src/kernels/kernel.ts b/src/kernels/kernel.ts index b6a552e9c49..303dcb50aa1 100644 --- a/src/kernels/kernel.ts +++ b/src/kernels/kernel.ts @@ -129,6 +129,9 @@ abstract class BaseKernel implements IBaseKernel { get onStarted(): Event { return this._onStarted.event; } + get onPostInitialized(): Event { + return this._onPostInitialized.event; + } get onDisposed(): Event { return this._onDisposed.event; } @@ -176,6 +179,7 @@ abstract class BaseKernel implements IBaseKernel { private readonly _onStatusChanged = new EventEmitter(); private readonly _onRestarted = new EventEmitter(); private readonly _onStarted = new EventEmitter(); + private readonly _onPostInitialized = new EventEmitter(); private readonly _onDisposed = new EventEmitter(); private _jupyterSessionPromise?: Promise; private readonly hookedSessionForEvents = new WeakSet(); @@ -788,6 +792,7 @@ abstract class BaseKernel implements IBaseKernel { logger.trace('End running kernel initialization, session is idle'); } kernelIdle?.stop(); + this._onPostInitialized.fire(); } finally { postInitialization?.stop(); } diff --git a/src/kernels/kernelProvider.base.ts b/src/kernels/kernelProvider.base.ts index 67705529d20..3218ec61574 100644 --- a/src/kernels/kernelProvider.base.ts +++ b/src/kernels/kernelProvider.base.ts @@ -36,6 +36,7 @@ export abstract class BaseCoreKernelProvider implements IKernelProvider { protected readonly _onDidCreateKernel = new EventEmitter(); protected readonly _onDidDisposeKernel = new EventEmitter(); protected readonly _onKernelStatusChanged = new EventEmitter<{ status: KernelMessage.Status; kernel: IKernel }>(); + protected readonly _onDidPostInitializeKernel = new EventEmitter(); public readonly onKernelStatusChanged = this._onKernelStatusChanged.event; public get kernels() { const kernels = new Set(); @@ -58,6 +59,7 @@ export abstract class BaseCoreKernelProvider implements IKernelProvider { disposables.push(this._onKernelStatusChanged); disposables.push(this._onDidStartKernel); disposables.push(this._onDidCreateKernel); + disposables.push(this._onDidPostInitializeKernel); } public get onDidDisposeKernel(): Event { @@ -73,6 +75,9 @@ export abstract class BaseCoreKernelProvider implements IKernelProvider { public get onDidCreateKernel(): Event { return this._onDidCreateKernel.event; } + public get onDidPostInitializeKernel(): Event { + return this._onDidPostInitializeKernel.event; + } public get(uriOrNotebook: Uri | NotebookDocument | string): IKernel | undefined { if (isUri(uriOrNotebook)) { const notebook = workspace.notebookDocuments.find( @@ -185,6 +190,7 @@ export abstract class BaseThirdPartyKernelProvider implements IThirdPartyKernelP protected readonly _onDidStartKernel = new EventEmitter(); protected readonly _onDidCreateKernel = new EventEmitter(); protected readonly _onDidDisposeKernel = new EventEmitter(); + protected readonly _onDidPostInitializeKernel = new EventEmitter(); protected readonly _onKernelStatusChanged = new EventEmitter<{ status: KernelMessage.Status; kernel: IThirdPartyKernel; @@ -211,6 +217,7 @@ export abstract class BaseThirdPartyKernelProvider implements IThirdPartyKernelP disposables.push(this._onKernelStatusChanged); disposables.push(this._onDidStartKernel); disposables.push(this._onDidCreateKernel); + disposables.push(this._onDidPostInitializeKernel); } public get onDidDisposeKernel(): Event { @@ -225,6 +232,9 @@ export abstract class BaseThirdPartyKernelProvider implements IThirdPartyKernelP public get onDidCreateKernel(): Event { return this._onDidCreateKernel.event; } + public get onDidPostInitializeKernel(): Event { + return this._onDidPostInitializeKernel.event; + } public get(uri: Uri | string): IThirdPartyKernel | undefined { return this.kernelsByUri.get(uri.toString())?.kernel || this.kernelsById.get(uri.toString())?.kernel; } diff --git a/src/kernels/kernelProvider.node.ts b/src/kernels/kernelProvider.node.ts index 7b6849ef9e3..703769a6efd 100644 --- a/src/kernels/kernelProvider.node.ts +++ b/src/kernels/kernelProvider.node.ts @@ -88,6 +88,13 @@ export class KernelProvider extends BaseCoreKernelProvider { this, this.disposables ); + kernel.onPostInitialized( + () => { + this._onDidPostInitializeKernel.fire(kernel); + }, + this, + this.disposables + ); this.executions.set(kernel, new NotebookKernelExecution(kernel, this.context, this.formatters, notebook)); this.asyncDisposables.push(kernel); @@ -143,6 +150,13 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { this, this.disposables ); + kernel.onPostInitialized( + () => { + this._onDidPostInitializeKernel.fire(kernel); + }, + this, + this.disposables + ); this.asyncDisposables.push(kernel); this.storeKernel(uri, options, kernel); this.deleteMappingIfKernelIsDisposed(uri, kernel); diff --git a/src/kernels/types.ts b/src/kernels/types.ts index 9d8d40dc727..2feb2546063 100644 --- a/src/kernels/types.ts +++ b/src/kernels/types.ts @@ -355,6 +355,7 @@ export interface IBaseKernel extends IAsyncDisposable { readonly onDisposed: Event; readonly onStarted: Event; readonly onRestarted: Event; + readonly onPostInitialized: Event; readonly restarting: Promise; readonly status: KernelMessage.Status; readonly disposed: boolean; @@ -506,6 +507,7 @@ export interface IBaseKernelProvider extends IAsyncDispos onDidRestartKernel: Event; onDidDisposeKernel: Event; onKernelStatusChanged: Event<{ status: KernelMessage.Status; kernel: T }>; + onDidPostInitializeKernel: Event; } /** diff --git a/src/standalone/api/kernels/index.ts b/src/standalone/api/kernels/index.ts index c0277c8e438..81d094ad2bc 100644 --- a/src/standalone/api/kernels/index.ts +++ b/src/standalone/api/kernels/index.ts @@ -1,19 +1,46 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -import { Uri, workspace } from 'vscode'; +import { Uri, workspace, EventEmitter } from 'vscode'; import { Kernel, Kernels } from '../../../api'; import { ServiceContainer } from '../../../platform/ioc/container'; -import { IKernel, IKernelProvider, isRemoteConnection } from '../../../kernels/types'; +import { IKernel, IKernelProvider } from '../../../kernels/types'; import { createKernelApiForExtension as createKernelApiForExtension } from './kernel'; import { Telemetry, sendTelemetryEvent } from '../../../telemetry'; import { JVSC_EXTENSION_ID } from '../../../platform/common/constants'; import { initializeInteractiveOrNotebookTelemetryBasedOnUserAction } from '../../../kernels/telemetry/helper'; +import { IDisposableRegistry } from '../../../platform/common/types'; const kernelCache = new WeakMap(); +const _onDidInitialize = new EventEmitter<{ uri: Uri }>(); +let isKernelProviderHookRegistered = false; export function getKernelsApi(extensionId: string): Kernels { return { + get onDidInitialize() { + let accessAllowed: boolean | undefined = undefined; + + if (!isKernelProviderHookRegistered) { + const disposableRegistry = ServiceContainer.instance.get(IDisposableRegistry); + const kernelProvider = ServiceContainer.instance.get(IKernelProvider); + disposableRegistry.push( + kernelProvider.onDidPostInitializeKernel((e) => { + _onDidInitialize.fire({ + uri: e.uri + }); + }) + ); + isKernelProviderHookRegistered = true; + } + + sendTelemetryEvent(Telemetry.NewJupyterKernelsApiUsage, undefined, { + extensionId, + pemUsed: 'onDidInitialize', + accessAllowed + }); + + return _onDidInitialize.event; + }, async getKernel(uri: Uri) { let accessAllowed: boolean | undefined = undefined; @@ -29,18 +56,6 @@ export function getKernelsApi(extensionId: string): Kernels { }); return; } - const execution = kernelProvider.getKernelExecution(kernel); - if (!isRemoteConnection(kernel.kernelConnectionMetadata) && execution.executionCount === 0) { - // For local kernels, execution count must be greater than 0, - // As we pre-warms kernels (i.e. we start kernels even though the user may not have executed any code). - // The only way to determine whether users executed code is to look at the execution count - sendTelemetryEvent(Telemetry.NewJupyterKernelsApiUsage, undefined, { - extensionId, - pemUsed: 'getKernel', - accessAllowed - }); - return; - } if (extensionId !== JVSC_EXTENSION_ID) { void initializeInteractiveOrNotebookTelemetryBasedOnUserAction( kernel.resourceUri, From a63276e309d56e9b6bfbfb06dac492f850a0c22d Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 8 Nov 2024 12:34:18 -0800 Subject: [PATCH 02/16] update --- src/api.d.ts | 4 +- src/kernels/kernel.ts | 2 + src/kernels/kernelProvider.node.unit.test.ts | 11 +++++ src/kernels/kernelProvider.web.unit.test.ts | 11 +++++ src/kernels/types.ts | 1 + src/platform/common/constants.ts | 1 + src/standalone/api/kernels/index.ts | 45 +++++++++----------- 7 files changed, 47 insertions(+), 28 deletions(-) diff --git a/src/api.d.ts b/src/api.d.ts index b7f49391625..18666cb78dc 100644 --- a/src/api.d.ts +++ b/src/api.d.ts @@ -234,7 +234,7 @@ export interface Kernel { } export interface Kernels { /** - * Event fired when a kernel becomes available for execution. + * Event fired when a kernel becomes available for execution on a resource. */ onDidInitialize: Event<{ uri: Uri; @@ -243,7 +243,7 @@ export interface Kernels { /** * Gets an the kernel associated with a given resource. * For instance if the resource is a notebook, then get the kernel associated with the given Notebook document. - * Only kernels which have already been started by the user and belonging to Notebooks that are currently opened will be returned. + * Only kernels which are ready for code execution and belonging to Notebooks that are currently opened will be returned. */ getKernel(uri: Uri): Thenable; } diff --git a/src/kernels/kernel.ts b/src/kernels/kernel.ts index 303dcb50aa1..6e45b20598d 100644 --- a/src/kernels/kernel.ts +++ b/src/kernels/kernel.ts @@ -792,6 +792,8 @@ abstract class BaseKernel implements IBaseKernel { logger.trace('End running kernel initialization, session is idle'); } kernelIdle?.stop(); + + // Post initialization event is only emitted on successful initialization this._onPostInitialized.fire(); } finally { postInitialization?.stop(); diff --git a/src/kernels/kernelProvider.node.unit.test.ts b/src/kernels/kernelProvider.node.unit.test.ts index 81138ccd7d6..f5ba56cb83d 100644 --- a/src/kernels/kernelProvider.node.unit.test.ts +++ b/src/kernels/kernelProvider.node.unit.test.ts @@ -101,20 +101,24 @@ suite('Jupyter Session', () => { const kernelStarted = createEventHandler(kernelProvider, 'onDidStartKernel', disposables); const kernelDisposed = createEventHandler(kernelProvider, 'onDidDisposeKernel', disposables); const kernelRestarted = createEventHandler(kernelProvider, 'onDidRestartKernel', disposables); + const kernelPostInitialized = createEventHandler(kernelProvider, 'onDidPostInitializeKernel', disposables); const kernelStatusChanged = createEventHandler(kernelProvider, 'onKernelStatusChanged', disposables); const notebook = new TestNotebookDocument(undefined, 'jupyter-notebook'); const onStarted = new EventEmitter(); const onStatusChanged = new EventEmitter(); const onRestartedEvent = new EventEmitter(); + const onPostInitializedEvent = new EventEmitter(); const onDisposedEvent = new EventEmitter(); disposables.push(onStatusChanged); disposables.push(onRestartedEvent); + disposables.push(onPostInitializedEvent); disposables.push(onStarted); disposables.push(onDisposedEvent); if (kernelProvider instanceof KernelProvider) { sinon.stub(Kernel.prototype, 'onStarted').get(() => onStarted.event); sinon.stub(Kernel.prototype, 'onStatusChanged').get(() => onStatusChanged.event); sinon.stub(Kernel.prototype, 'onRestarted').get(() => onRestartedEvent.event); + sinon.stub(Kernel.prototype, 'onPostInitialized').get(() => onPostInitializedEvent.event); sinon.stub(Kernel.prototype, 'onDisposed').get(() => onDisposedEvent.event); const kernel = kernelProvider.getOrCreate(notebook, { controller, @@ -126,6 +130,7 @@ suite('Jupyter Session', () => { sinon.stub(ThirdPartyKernel.prototype, 'onStarted').get(() => onStarted.event); sinon.stub(ThirdPartyKernel.prototype, 'onStatusChanged').get(() => onStatusChanged.event); sinon.stub(ThirdPartyKernel.prototype, 'onRestarted').get(() => onRestartedEvent.event); + sinon.stub(ThirdPartyKernel.prototype, 'onPostInitialized').get(() => onPostInitializedEvent.event); sinon.stub(ThirdPartyKernel.prototype, 'onDisposed').get(() => onDisposedEvent.event); const kernel = kernelProvider.getOrCreate(notebook.uri, { metadata: instance(metadata), @@ -138,6 +143,10 @@ suite('Jupyter Session', () => { assert.isFalse(kernelStarted.fired, 'IKernelProvider.onDidStartKernel should not be fired'); assert.isFalse(kernelStatusChanged.fired, 'IKernelProvider.onKernelStatusChanged should not be fired'); assert.isFalse(kernelRestarted.fired, 'IKernelProvider.onDidRestartKernel should not have fired'); + assert.isFalse( + kernelPostInitialized.fired, + 'IKernelProvider.onDidPostInitializeKernel should not have fired' + ); assert.isFalse(kernelDisposed.fired, 'IKernelProvider.onDidDisposeKernel should not have fired'); onStarted.fire(); @@ -146,6 +155,8 @@ suite('Jupyter Session', () => { assert.isTrue(kernelStatusChanged.fired, 'IKernelProvider.onKernelStatusChanged not fired'); onRestartedEvent.fire(); assert.isTrue(kernelRestarted.fired, 'IKernelProvider.onKernelRestarted not fired'); + onPostInitializedEvent.fire(); + assert.isTrue(kernelPostInitialized.fired, 'IKernelProvider.onDidPostInitializeKernel not fired'); onDisposedEvent.fire(); assert.isTrue(kernelDisposed.fired, 'IKernelProvider.onDisposedEvent not fired'); } diff --git a/src/kernels/kernelProvider.web.unit.test.ts b/src/kernels/kernelProvider.web.unit.test.ts index 05d5789f7c8..6c8cc09cda2 100644 --- a/src/kernels/kernelProvider.web.unit.test.ts +++ b/src/kernels/kernelProvider.web.unit.test.ts @@ -82,20 +82,24 @@ suite('Jupyter Session', () => { const kernelStarted = createEventHandler(kernelProvider, 'onDidStartKernel', disposables); const kernelDisposed = createEventHandler(kernelProvider, 'onDidDisposeKernel', disposables); const kernelRestarted = createEventHandler(kernelProvider, 'onDidRestartKernel', disposables); + const kernelPostInitialized = createEventHandler(kernelProvider, 'onDidPostInitializeKernel', disposables); const kernelStatusChanged = createEventHandler(kernelProvider, 'onKernelStatusChanged', disposables); const notebook = new TestNotebookDocument(undefined, 'jupyter-notebook'); const onStarted = new EventEmitter(); const onStatusChanged = new EventEmitter(); const onRestartedEvent = new EventEmitter(); + const onPostInitializedEvent = new EventEmitter(); const onDisposedEvent = new EventEmitter(); disposables.push(onStatusChanged); disposables.push(onRestartedEvent); + disposables.push(onPostInitializedEvent); disposables.push(onStarted); disposables.push(onDisposedEvent); if (kernelProvider instanceof KernelProvider) { sinon.stub(Kernel.prototype, 'onStarted').get(() => onStarted.event); sinon.stub(Kernel.prototype, 'onStatusChanged').get(() => onStatusChanged.event); sinon.stub(Kernel.prototype, 'onRestarted').get(() => onRestartedEvent.event); + sinon.stub(Kernel.prototype, 'onPostInitialized').get(() => onPostInitializedEvent.event); sinon.stub(Kernel.prototype, 'onDisposed').get(() => onDisposedEvent.event); const kernel = kernelProvider.getOrCreate(notebook, { controller, @@ -107,6 +111,7 @@ suite('Jupyter Session', () => { sinon.stub(ThirdPartyKernel.prototype, 'onStarted').get(() => onStarted.event); sinon.stub(ThirdPartyKernel.prototype, 'onStatusChanged').get(() => onStatusChanged.event); sinon.stub(ThirdPartyKernel.prototype, 'onRestarted').get(() => onRestartedEvent.event); + sinon.stub(ThirdPartyKernel.prototype, 'onPostInitialized').get(() => onPostInitializedEvent.event); sinon.stub(ThirdPartyKernel.prototype, 'onDisposed').get(() => onDisposedEvent.event); const kernel = kernelProvider.getOrCreate(notebook.uri, { metadata: instance(metadata), @@ -119,6 +124,10 @@ suite('Jupyter Session', () => { assert.isFalse(kernelStarted.fired, 'IKernelProvider.onDidStartKernel should not be fired'); assert.isFalse(kernelStatusChanged.fired, 'IKernelProvider.onKernelStatusChanged should not be fired'); assert.isFalse(kernelRestarted.fired, 'IKernelProvider.onDidRestartKernel should not have fired'); + assert.isFalse( + kernelPostInitialized.fired, + 'IKernelProvider.onDidPostInitializeKernel should not have fired' + ); assert.isFalse(kernelDisposed.fired, 'IKernelProvider.onDidDisposeKernel should not have fired'); onStarted.fire(); @@ -127,6 +136,8 @@ suite('Jupyter Session', () => { assert.isTrue(kernelStatusChanged.fired, 'IKernelProvider.onKernelStatusChanged not fired'); onRestartedEvent.fire(); assert.isTrue(kernelRestarted.fired, 'IKernelProvider.onKernelRestarted not fired'); + onPostInitializedEvent.fire(); + assert.isTrue(kernelPostInitialized.fired, 'IKernelProvider.onDidPostInitializeKernel not fired'); onDisposedEvent.fire(); assert.isTrue(kernelDisposed.fired, 'IKernelProvider.onDisposedEvent not fired'); } diff --git a/src/kernels/types.ts b/src/kernels/types.ts index 2feb2546063..53a49939b51 100644 --- a/src/kernels/types.ts +++ b/src/kernels/types.ts @@ -508,6 +508,7 @@ export interface IBaseKernelProvider extends IAsyncDispos onDidDisposeKernel: Event; onKernelStatusChanged: Event<{ status: KernelMessage.Status; kernel: T }>; onDidPostInitializeKernel: Event; + onDidDispose: Event; } /** diff --git a/src/platform/common/constants.ts b/src/platform/common/constants.ts index 79b04f69ad9..a49a475bf57 100644 --- a/src/platform/common/constants.ts +++ b/src/platform/common/constants.ts @@ -433,6 +433,7 @@ export enum Telemetry { NotebookFirstKernelAutoSelectionBreakDown = 'DATASCIENCE.NOTEBOOK_FIRST_KERNEL_AUTO_SELECTION_BREAKDOWN', NotebookInterrupt = 'DATASCIENCE.NOTEBOOK_INTERRUPT', NotebookRestart = 'DATASCIENCE.NOTEBOOK_RESTART', + NotebookPostInitializationFailed = 'DATASCIENCE.NOTEBOOK_POST_INITIALIZATION_FAILED', SwitchKernel = 'DS_INTERNAL.SWITCH_KERNEL', KernelCount = 'DS_INTERNAL.KERNEL_COUNT', ExecuteCell = 'DATASCIENCE.EXECUTE_CELL', diff --git a/src/standalone/api/kernels/index.ts b/src/standalone/api/kernels/index.ts index 81d094ad2bc..0d0ead18183 100644 --- a/src/standalone/api/kernels/index.ts +++ b/src/standalone/api/kernels/index.ts @@ -12,35 +12,10 @@ import { initializeInteractiveOrNotebookTelemetryBasedOnUserAction } from '../.. import { IDisposableRegistry } from '../../../platform/common/types'; const kernelCache = new WeakMap(); -const _onDidInitialize = new EventEmitter<{ uri: Uri }>(); -let isKernelProviderHookRegistered = false; +let _onDidInitialize: EventEmitter<{ uri: Uri }> | undefined = undefined; export function getKernelsApi(extensionId: string): Kernels { return { - get onDidInitialize() { - let accessAllowed: boolean | undefined = undefined; - - if (!isKernelProviderHookRegistered) { - const disposableRegistry = ServiceContainer.instance.get(IDisposableRegistry); - const kernelProvider = ServiceContainer.instance.get(IKernelProvider); - disposableRegistry.push( - kernelProvider.onDidPostInitializeKernel((e) => { - _onDidInitialize.fire({ - uri: e.uri - }); - }) - ); - isKernelProviderHookRegistered = true; - } - - sendTelemetryEvent(Telemetry.NewJupyterKernelsApiUsage, undefined, { - extensionId, - pemUsed: 'onDidInitialize', - accessAllowed - }); - - return _onDidInitialize.event; - }, async getKernel(uri: Uri) { let accessAllowed: boolean | undefined = undefined; @@ -65,6 +40,24 @@ export function getKernelsApi(extensionId: string): Kernels { let wrappedKernel = kernelCache.get(kernel) || createKernelApiForExtension(extensionId, kernel); kernelCache.set(kernel, wrappedKernel); return wrappedKernel; + }, + get onDidInitialize() { + // We can cache the event emitter for subsequent calls. + if (!_onDidInitialize) { + const kernelProvider = ServiceContainer.instance.get(IKernelProvider); + const disposableRegistry = ServiceContainer.instance.get(IDisposableRegistry); + _onDidInitialize = new EventEmitter<{ uri: Uri }>(); + + disposableRegistry.push( + kernelProvider.onDidPostInitializeKernel((e) => { + _onDidInitialize?.fire({ uri: e.uri }); + }), + _onDidInitialize, + { dispose: () => (_onDidInitialize = undefined) } + ); + } + + return _onDidInitialize.event; } }; } From f21eb3c085ede6534b5e331c552e1ef412891446 Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 8 Nov 2024 12:35:52 -0800 Subject: [PATCH 03/16] update --- src/platform/common/constants.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/platform/common/constants.ts b/src/platform/common/constants.ts index a49a475bf57..79b04f69ad9 100644 --- a/src/platform/common/constants.ts +++ b/src/platform/common/constants.ts @@ -433,7 +433,6 @@ export enum Telemetry { NotebookFirstKernelAutoSelectionBreakDown = 'DATASCIENCE.NOTEBOOK_FIRST_KERNEL_AUTO_SELECTION_BREAKDOWN', NotebookInterrupt = 'DATASCIENCE.NOTEBOOK_INTERRUPT', NotebookRestart = 'DATASCIENCE.NOTEBOOK_RESTART', - NotebookPostInitializationFailed = 'DATASCIENCE.NOTEBOOK_POST_INITIALIZATION_FAILED', SwitchKernel = 'DS_INTERNAL.SWITCH_KERNEL', KernelCount = 'DS_INTERNAL.KERNEL_COUNT', ExecuteCell = 'DATASCIENCE.EXECUTE_CELL', From 99aab138c7f3ed2f9e46ba3b5462ffd9dc6bd67c Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 8 Nov 2024 14:12:14 -0800 Subject: [PATCH 04/16] remove unused event --- src/kernels/types.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/kernels/types.ts b/src/kernels/types.ts index 53a49939b51..2feb2546063 100644 --- a/src/kernels/types.ts +++ b/src/kernels/types.ts @@ -508,7 +508,6 @@ export interface IBaseKernelProvider extends IAsyncDispos onDidDisposeKernel: Event; onKernelStatusChanged: Event<{ status: KernelMessage.Status; kernel: T }>; onDidPostInitializeKernel: Event; - onDidDispose: Event; } /** From f5274178562859e9e3223430a8d5f081415d3944 Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Thu, 14 Nov 2024 10:53:52 -0800 Subject: [PATCH 05/16] PR --- src/api.d.ts | 9 +-------- src/api.proposed.kernelCreateHook.d.ts | 16 ++++++++++++++++ src/kernels/kernel.ts | 17 +++++++++++++---- src/standalone/api/kernels/index.ts | 6 +++++- 4 files changed, 35 insertions(+), 13 deletions(-) create mode 100644 src/api.proposed.kernelCreateHook.d.ts diff --git a/src/api.d.ts b/src/api.d.ts index 18666cb78dc..64dad363d6e 100644 --- a/src/api.d.ts +++ b/src/api.d.ts @@ -233,17 +233,10 @@ export interface Kernel { executeCode(code: string, token: CancellationToken): AsyncIterable; } export interface Kernels { - /** - * Event fired when a kernel becomes available for execution on a resource. - */ - onDidInitialize: Event<{ - uri: Uri; - }>; - /** * Gets an the kernel associated with a given resource. * For instance if the resource is a notebook, then get the kernel associated with the given Notebook document. - * Only kernels which are ready for code execution and belonging to Notebooks that are currently opened will be returned. + * Only kernels which have already been started by the user and belonging to Notebooks that are currently opened will be returned. */ getKernel(uri: Uri): Thenable; } diff --git a/src/api.proposed.kernelCreateHook.d.ts b/src/api.proposed.kernelCreateHook.d.ts new file mode 100644 index 00000000000..fcd6ccd1b59 --- /dev/null +++ b/src/api.proposed.kernelCreateHook.d.ts @@ -0,0 +1,16 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import type { Event, Uri } from 'vscode'; + +declare module './api' { + export interface Kernels { + /** + * Event fired when a kernel is created (by user execution) or restarted on a resource. + */ + onDidCreateOrRestart: Event<{ + uri: Uri; + kernel: Kernel; + }>; + } +} diff --git a/src/kernels/kernel.ts b/src/kernels/kernel.ts index 6e45b20598d..844c86d5f37 100644 --- a/src/kernels/kernel.ts +++ b/src/kernels/kernel.ts @@ -175,6 +175,7 @@ abstract class BaseKernel implements IBaseKernel { private _disposed?: boolean; private _disposing?: boolean; private _ignoreJupyterSessionDisposedErrors?: boolean; + private _postInitializedOnStart?: boolean; private readonly _onDidKernelSocketChange = new EventEmitter(); private readonly _onStatusChanged = new EventEmitter(); private readonly _onRestarted = new EventEmitter(); @@ -250,7 +251,15 @@ abstract class BaseKernel implements IBaseKernel { this.startCancellation.dispose(); this.startCancellation = new CancellationTokenSource(); } - return this.startJupyterSession(options); + return this.startJupyterSession(options).then((result) => { + // If we started and the UI is no longer disabled (ie., a user executed a cell) + // then we can signal that the kernel was created and can be used by third-party extensions. + // We also only want to fire off a single event here. + if (!options?.disableUI && !this._postInitializedOnStart) { + this._onPostInitialized.fire(); + } + return result; + }); } /** * Interrupts the execution of cells. @@ -413,6 +422,9 @@ abstract class BaseKernel implements IBaseKernel { // Indicate a restart occurred if it succeeds this._onRestarted.fire(); + + // Also signal that the kernel post initialization completed. + this._onPostInitialized.fire(); } catch (ex) { logger.error(`Failed to restart kernel ${getDisplayPath(this.uri)}`, ex); throw ex; @@ -792,9 +804,6 @@ abstract class BaseKernel implements IBaseKernel { logger.trace('End running kernel initialization, session is idle'); } kernelIdle?.stop(); - - // Post initialization event is only emitted on successful initialization - this._onPostInitialized.fire(); } finally { postInitialization?.stop(); } diff --git a/src/standalone/api/kernels/index.ts b/src/standalone/api/kernels/index.ts index 0d0ead18183..a3a3097a016 100644 --- a/src/standalone/api/kernels/index.ts +++ b/src/standalone/api/kernels/index.ts @@ -7,7 +7,7 @@ import { ServiceContainer } from '../../../platform/ioc/container'; import { IKernel, IKernelProvider } from '../../../kernels/types'; import { createKernelApiForExtension as createKernelApiForExtension } from './kernel'; import { Telemetry, sendTelemetryEvent } from '../../../telemetry'; -import { JVSC_EXTENSION_ID } from '../../../platform/common/constants'; +import { DATA_WRANGLER_EXTENSION_ID, JVSC_EXTENSION_ID } from '../../../platform/common/constants'; import { initializeInteractiveOrNotebookTelemetryBasedOnUserAction } from '../../../kernels/telemetry/helper'; import { IDisposableRegistry } from '../../../platform/common/types'; @@ -42,6 +42,10 @@ export function getKernelsApi(extensionId: string): Kernels { return wrappedKernel; }, get onDidInitialize() { + if (![JVSC_EXTENSION_ID, DATA_WRANGLER_EXTENSION_ID].includes(extensionId)) { + throw new Error(`Proposed API is not supported for extension ${extensionId}`); + } + // We can cache the event emitter for subsequent calls. if (!_onDidInitialize) { const kernelProvider = ServiceContainer.instance.get(IKernelProvider); From 7dbd1c1f7d09b50478950caa38ca38ac1dff30fb Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Thu, 14 Nov 2024 16:07:58 -0800 Subject: [PATCH 06/16] WIP --- ...d.ts => api.proposed.kernelStartHook.d.ts} | 4 +- src/extension.node.ts | 5 +- src/extension.web.ts | 5 +- src/kernels/kernel.ts | 1 + .../api/kernels/api.vscode.common.test.ts | 98 ++++++++++++++++++- src/standalone/api/kernels/index.ts | 28 +++--- 6 files changed, 124 insertions(+), 17 deletions(-) rename src/{api.proposed.kernelCreateHook.d.ts => api.proposed.kernelStartHook.d.ts} (66%) diff --git a/src/api.proposed.kernelCreateHook.d.ts b/src/api.proposed.kernelStartHook.d.ts similarity index 66% rename from src/api.proposed.kernelCreateHook.d.ts rename to src/api.proposed.kernelStartHook.d.ts index fcd6ccd1b59..5377ee50f08 100644 --- a/src/api.proposed.kernelCreateHook.d.ts +++ b/src/api.proposed.kernelStartHook.d.ts @@ -6,9 +6,9 @@ import type { Event, Uri } from 'vscode'; declare module './api' { export interface Kernels { /** - * Event fired when a kernel is created (by user execution) or restarted on a resource. + * Event fired when a kernel is started or restarted by a user on a resource. */ - onDidCreateOrRestart: Event<{ + onDidStart: Event<{ uri: Uri; kernel: Kernel; }>; diff --git a/src/extension.node.ts b/src/extension.node.ts index 1857d5e46ce..4e519ab38f3 100644 --- a/src/extension.node.ts +++ b/src/extension.node.ts @@ -124,7 +124,10 @@ export async function activate(context: IExtensionContext): Promise { throw new Error('Not Implemented'); }, - kernels: { getKernel: () => Promise.resolve(undefined) } + kernels: { + getKernel: () => Promise.resolve(undefined), + onDidStart: () => ({ dispose: noop }) + } }; } } diff --git a/src/extension.web.ts b/src/extension.web.ts index add9063bd7c..9ffca564c40 100644 --- a/src/extension.web.ts +++ b/src/extension.web.ts @@ -117,7 +117,10 @@ export async function activate(context: IExtensionContext): Promise { throw new Error('Not Implemented'); }, - kernels: { getKernel: () => Promise.resolve(undefined) } + kernels: { + getKernel: () => Promise.resolve(undefined), + onDidStart: () => ({ dispose: noop }) + } }; } } diff --git a/src/kernels/kernel.ts b/src/kernels/kernel.ts index 844c86d5f37..8bee207b249 100644 --- a/src/kernels/kernel.ts +++ b/src/kernels/kernel.ts @@ -257,6 +257,7 @@ abstract class BaseKernel implements IBaseKernel { // We also only want to fire off a single event here. if (!options?.disableUI && !this._postInitializedOnStart) { this._onPostInitialized.fire(); + this._postInitializedOnStart = true; } return result; }); diff --git a/src/standalone/api/kernels/api.vscode.common.test.ts b/src/standalone/api/kernels/api.vscode.common.test.ts index d29e011f4fc..0e311e343cc 100644 --- a/src/standalone/api/kernels/api.vscode.common.test.ts +++ b/src/standalone/api/kernels/api.vscode.common.test.ts @@ -2,7 +2,15 @@ // Licensed under the MIT License. import { assert } from 'chai'; -import { CancellationTokenSource, NotebookCellOutputItem, NotebookDocument, commands, window, workspace } from 'vscode'; +import { + CancellationTokenSource, + NotebookCell, + NotebookCellOutputItem, + NotebookDocument, + commands, + window, + workspace +} from 'vscode'; import { logger } from '../../../platform/logging'; import { IDisposable } from '../../../platform/common/types'; import { @@ -32,6 +40,7 @@ import { KernelError } from '../../../kernels/errors/kernelError'; import { JVSC_EXTENSION_ID } from '../../../platform/common/constants'; import { escapeStringToEmbedInPythonCode } from '../../../kernels/chat/generator'; import { notebookCellExecutions } from '../../../platform/notebooks/cellExecutionStateService'; +import { createKernelApiForExtension } from './kernel'; suiteMandatory('Kernel API Tests @typescript', function () { const disposables: IDisposable[] = []; @@ -185,6 +194,93 @@ suiteMandatory('Kernel API Tests @typescript', function () { 'Traceback does not contain original error' ); }); + testMandatory('Kernel start event is not triggered by silent executions', async function () { + let startEventCounter = 0; + // Register event listener to track invocations + disposables.push( + kernels.onDidStart(() => { + startEventCounter++; + }) + ); + + await realKernel.start(); + const kernel = createKernelApiForExtension(JVSC_EXTENSION_ID_FOR_TESTS, realKernel); + + logger.info(`Execute code silently`); + const expectedMime = NotebookCellOutputItem.stdout('').mime; + const token = new CancellationTokenSource(); + await waitForOutput(kernel.executeCode('console.log(1234)', token.token), '1234', expectedMime); + logger.info(`Execute code silently completed`); + // Wait for kernel to be idle. + await waitForCondition( + () => kernel.status === 'idle', + 5_000, + `Kernel did not become idle, current status is ${kernel.status}` + ); + assert.equal(startEventCounter, 0); + }); + testMandatory('Kernel start event is triggered before first user execution and on restart', async function () { + // Register event listener to track invocations + const source = new CancellationTokenSource(); + let startEventCounter = 0; + disposables.push( + kernels.onDidStart(({ kernel }) => { + startEventCounter++; + kernel.executeCode(`foo = ${startEventCounter}`, source.token); + }) + ); + await insertCodeCell('console.log(foo)', { index: 0, language: 'typescript' }); + + await realKernel.start(); + const cell = notebook.cellAt(0)!; + const executionOrderSet = createDeferred(); + const eventHandler = notebookCellExecutions.onDidChangeNotebookCellExecutionState((e) => { + if (e.cell === cell && e.cell.executionSummary?.executionOrder) { + executionOrderSet.resolve(); + } + }); + disposables.push(eventHandler); + await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); + + // Validate the cell execution output is equal to the expected value of "foo = 1" + const expectedMime = NotebookCellOutputItem.stdout('').mime; + assert.isTrue(cellHasOutput(cell, '1', expectedMime)); + + const kernel = await kernels.getKernel(notebook.uri); + if (!kernel) { + throw new Error('Kernel not found'); + } + + // Start event counter should only be 1 after the initial user cell execution + assert.equal(startEventCounter, 1); + + // Running the same cell again should not fire additional events + await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); + assert.isTrue(cellHasOutput(cell, '1', expectedMime)); + assert.equal(startEventCounter, 1); + + // Start event should be triggered on restart + await realKernel.restart(); + assert.equal(startEventCounter, 2); + + await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); + assert.equal(startEventCounter, 2); + assert.isTrue(cellHasOutput(cell, '2', expectedMime)); + }); + + async function cellHasOutput(cell: NotebookCell, expectedOutput: string, expectedMimetype: string) { + return Boolean( + cell.outputs.find((output) => + output.items.some((item) => { + if (item.mime === expectedMimetype) { + const output = new TextDecoder().decode(item.data).trim(); + return output === expectedOutput; + } + return false; + }) + ) + ); + } async function waitForOutput( executionResult: AsyncIterable, diff --git a/src/standalone/api/kernels/index.ts b/src/standalone/api/kernels/index.ts index a3a3097a016..d375e9e8114 100644 --- a/src/standalone/api/kernels/index.ts +++ b/src/standalone/api/kernels/index.ts @@ -12,7 +12,13 @@ import { initializeInteractiveOrNotebookTelemetryBasedOnUserAction } from '../.. import { IDisposableRegistry } from '../../../platform/common/types'; const kernelCache = new WeakMap(); -let _onDidInitialize: EventEmitter<{ uri: Uri }> | undefined = undefined; +let _onDidStart: EventEmitter<{ uri: Uri; kernel: Kernel }> | undefined = undefined; + +function getWrappedKernel(kernel: IKernel, extensionId: string) { + let wrappedKernel = kernelCache.get(kernel) || createKernelApiForExtension(extensionId, kernel); + kernelCache.set(kernel, wrappedKernel); + return wrappedKernel; +} export function getKernelsApi(extensionId: string): Kernels { return { @@ -37,31 +43,29 @@ export function getKernelsApi(extensionId: string): Kernels { kernel.kernelConnectionMetadata ); } - let wrappedKernel = kernelCache.get(kernel) || createKernelApiForExtension(extensionId, kernel); - kernelCache.set(kernel, wrappedKernel); - return wrappedKernel; + return getWrappedKernel(kernel, extensionId); }, - get onDidInitialize() { + get onDidStart() { if (![JVSC_EXTENSION_ID, DATA_WRANGLER_EXTENSION_ID].includes(extensionId)) { throw new Error(`Proposed API is not supported for extension ${extensionId}`); } // We can cache the event emitter for subsequent calls. - if (!_onDidInitialize) { + if (!_onDidStart) { const kernelProvider = ServiceContainer.instance.get(IKernelProvider); const disposableRegistry = ServiceContainer.instance.get(IDisposableRegistry); - _onDidInitialize = new EventEmitter<{ uri: Uri }>(); + _onDidStart = new EventEmitter<{ uri: Uri; kernel: Kernel }>(); disposableRegistry.push( - kernelProvider.onDidPostInitializeKernel((e) => { - _onDidInitialize?.fire({ uri: e.uri }); + kernelProvider.onDidPostInitializeKernel(async (kernel) => { + _onDidStart?.fire({ uri: kernel.uri, kernel: getWrappedKernel(kernel, extensionId) }); }), - _onDidInitialize, - { dispose: () => (_onDidInitialize = undefined) } + _onDidStart, + { dispose: () => (_onDidStart = undefined) } ); } - return _onDidInitialize.event; + return _onDidStart.event; } }; } From bea5bd6821eb4b69e096d8cce69a370afadfc0c5 Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Thu, 14 Nov 2024 16:58:41 -0800 Subject: [PATCH 07/16] test WIP --- src/kernels/kernelProvider.web.ts | 2 ++ src/kernels/kernelProvider.web.unit.test.ts | 2 +- .../api/kernels/api.vscode.common.test.ts | 16 ++++++++++++++-- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/kernels/kernelProvider.web.ts b/src/kernels/kernelProvider.web.ts index 299ec64b4ec..6a65b3f8c00 100644 --- a/src/kernels/kernelProvider.web.ts +++ b/src/kernels/kernelProvider.web.ts @@ -80,6 +80,7 @@ export class KernelProvider extends BaseCoreKernelProvider { this.workspaceStorage ) as IKernel; kernel.onRestarted(() => this._onDidRestartKernel.fire(kernel), this, this.disposables); + kernel.onPostInitialized(() => this._onDidPostInitializeKernel.fire(kernel), this, this.disposables); kernel.onDisposed(() => this._onDidDisposeKernel.fire(kernel), this, this.disposables); kernel.onStarted(() => this._onDidStartKernel.fire(kernel), this, this.disposables); kernel.onStatusChanged( @@ -132,6 +133,7 @@ export class ThirdPartyKernelProvider extends BaseThirdPartyKernelProvider { this.workspaceStorage ); kernel.onRestarted(() => this._onDidRestartKernel.fire(kernel), this, this.disposables); + kernel.onPostInitialized(() => this._onDidPostInitializeKernel.fire(kernel), this, this.disposables); kernel.onDisposed(() => this._onDidDisposeKernel.fire(kernel), this, this.disposables); kernel.onStarted(() => this._onDidStartKernel.fire(kernel), this, this.disposables); kernel.onStatusChanged( diff --git a/src/kernels/kernelProvider.web.unit.test.ts b/src/kernels/kernelProvider.web.unit.test.ts index 6c8cc09cda2..1e90cfd0962 100644 --- a/src/kernels/kernelProvider.web.unit.test.ts +++ b/src/kernels/kernelProvider.web.unit.test.ts @@ -17,7 +17,7 @@ import { ThirdPartyKernelProvider } from './kernelProvider.node'; import { dispose } from '../platform/common/utils/lifecycle'; import { noop } from '../test/core'; -suite('Jupyter Session', () => { +suite.only('Jupyter Session', () => { suite('Web Kernel Provider', function () { let disposables: IDisposable[] = []; const asyncDisposables: { dispose: () => Promise }[] = []; diff --git a/src/standalone/api/kernels/api.vscode.common.test.ts b/src/standalone/api/kernels/api.vscode.common.test.ts index 0e311e343cc..345621d1f87 100644 --- a/src/standalone/api/kernels/api.vscode.common.test.ts +++ b/src/standalone/api/kernels/api.vscode.common.test.ts @@ -41,6 +41,7 @@ import { JVSC_EXTENSION_ID } from '../../../platform/common/constants'; import { escapeStringToEmbedInPythonCode } from '../../../kernels/chat/generator'; import { notebookCellExecutions } from '../../../platform/notebooks/cellExecutionStateService'; import { createKernelApiForExtension } from './kernel'; +import { noop } from '../../../test/core'; suiteMandatory('Kernel API Tests @typescript', function () { const disposables: IDisposable[] = []; @@ -203,7 +204,14 @@ suiteMandatory('Kernel API Tests @typescript', function () { }) ); - await realKernel.start(); + await realKernel.start({ + disableUI: true, + onDidChangeDisableUI: () => ({ + dispose: noop + }) + }); + assert.equal(startEventCounter, 0); + const kernel = createKernelApiForExtension(JVSC_EXTENSION_ID_FOR_TESTS, realKernel); logger.info(`Execute code silently`); @@ -226,12 +234,16 @@ suiteMandatory('Kernel API Tests @typescript', function () { disposables.push( kernels.onDidStart(({ kernel }) => { startEventCounter++; - kernel.executeCode(`foo = ${startEventCounter}`, source.token); + const codeToRun = + startEventCounter === 0 ? `let foo = ${startEventCounter}` : `foo = ${startEventCounter}`; + kernel.executeCode(codeToRun, source.token); }) ); await insertCodeCell('console.log(foo)', { index: 0, language: 'typescript' }); await realKernel.start(); + assert.equal(startEventCounter, 1); + const cell = notebook.cellAt(0)!; const executionOrderSet = createDeferred(); const eventHandler = notebookCellExecutions.onDidChangeNotebookCellExecutionState((e) => { From 9e267473ac7f20d139ca6b62c920a2c2533769fa Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Thu, 14 Nov 2024 17:04:03 -0800 Subject: [PATCH 08/16] test WIP --- src/kernels/kernelProvider.web.unit.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kernels/kernelProvider.web.unit.test.ts b/src/kernels/kernelProvider.web.unit.test.ts index 1e90cfd0962..6c8cc09cda2 100644 --- a/src/kernels/kernelProvider.web.unit.test.ts +++ b/src/kernels/kernelProvider.web.unit.test.ts @@ -17,7 +17,7 @@ import { ThirdPartyKernelProvider } from './kernelProvider.node'; import { dispose } from '../platform/common/utils/lifecycle'; import { noop } from '../test/core'; -suite.only('Jupyter Session', () => { +suite('Jupyter Session', () => { suite('Web Kernel Provider', function () { let disposables: IDisposable[] = []; const asyncDisposables: { dispose: () => Promise }[] = []; From dc3b5ae5af1958bc0d9a5b2dd8b7051138609554 Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 15 Nov 2024 10:22:35 -0800 Subject: [PATCH 09/16] PR --- src/kernels/kernel.ts | 3 + src/kernels/types.ts | 6 ++ .../api/kernels/api.vscode.common.test.ts | 67 ++++++++++++------- src/standalone/api/kernels/index.ts | 3 +- 4 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/kernels/kernel.ts b/src/kernels/kernel.ts index 8bee207b249..139353dd7bd 100644 --- a/src/kernels/kernel.ts +++ b/src/kernels/kernel.ts @@ -141,6 +141,9 @@ abstract class BaseKernel implements IBaseKernel { get startedAtLeastOnce() { return this._startedAtLeastOnce; } + get userStartedKernel() { + return this.startupUI.disableUI; + } private _info?: KernelMessage.IInfoReplyMsg['content']; private _startedAtLeastOnce?: boolean; get info(): KernelMessage.IInfoReplyMsg['content'] | undefined { diff --git a/src/kernels/types.ts b/src/kernels/types.ts index 2feb2546063..b9bc32cf8a4 100644 --- a/src/kernels/types.ts +++ b/src/kernels/types.ts @@ -382,6 +382,12 @@ export interface IBaseKernel extends IAsyncDisposable { * This flag will tell us whether a real kernel was or is active. */ readonly startedAtLeastOnce?: boolean; + /** + * A kernel might be started (e.g., for pre-warming or other internal reasons) but this does not + * necessarily correlate with whether it was started by a user. + * This flag will tell us whether the kernel has been started explicitly through user action from the UI. + */ + readonly userStartedKernel: boolean; start(options?: IDisplayOptions): Promise; interrupt(): Promise; restart(): Promise; diff --git a/src/standalone/api/kernels/api.vscode.common.test.ts b/src/standalone/api/kernels/api.vscode.common.test.ts index 345621d1f87..76efd749eea 100644 --- a/src/standalone/api/kernels/api.vscode.common.test.ts +++ b/src/standalone/api/kernels/api.vscode.common.test.ts @@ -210,38 +210,63 @@ suiteMandatory('Kernel API Tests @typescript', function () { dispose: noop }) }); - assert.equal(startEventCounter, 0); - - const kernel = createKernelApiForExtension(JVSC_EXTENSION_ID_FOR_TESTS, realKernel); + assert.equal(startEventCounter, 0, 'Kernel start event was triggered for a non-user kernel start'); + }); + testMandatory('Kernel start event is triggered when user executes code', async function () { + let startEventCounter = 0; + // Register event listener to track invocations + disposables.push( + kernels.onDidStart(() => { + startEventCounter++; + }) + ); - logger.info(`Execute code silently`); - const expectedMime = NotebookCellOutputItem.stdout('').mime; - const token = new CancellationTokenSource(); - await waitForOutput(kernel.executeCode('console.log(1234)', token.token), '1234', expectedMime); - logger.info(`Execute code silently completed`); - // Wait for kernel to be idle. - await waitForCondition( - () => kernel.status === 'idle', - 5_000, - `Kernel did not become idle, current status is ${kernel.status}` + await realKernel.start({ + disableUI: false, + onDidChangeDisableUI: () => ({ + dispose: noop + }) + }); + assert.equal(startEventCounter, 1, 'Kernel start event was not triggered for a user kernel start'); + }); + testMandatory('Kernel start event is triggered when kernel restarts', async function () { + let startEventCounter = 0; + // Register event listener to track invocations + disposables.push( + kernels.onDidStart(() => { + startEventCounter++; + }) ); - assert.equal(startEventCounter, 0); + + await realKernel.start({ + disableUI: true, + onDidChangeDisableUI: () => ({ + dispose: noop + }) + }); + await realKernel.restart(); + assert.equal(startEventCounter, 1, 'Kernel start event should be fired exactly once after restarting'); }); - testMandatory('Kernel start event is triggered before first user execution and on restart', async function () { + testMandatory('Kernel start event is triggered before first user execution', async function () { // Register event listener to track invocations const source = new CancellationTokenSource(); let startEventCounter = 0; disposables.push( kernels.onDidStart(({ kernel }) => { - startEventCounter++; const codeToRun = startEventCounter === 0 ? `let foo = ${startEventCounter}` : `foo = ${startEventCounter}`; + startEventCounter++; kernel.executeCode(codeToRun, source.token); }) ); await insertCodeCell('console.log(foo)', { index: 0, language: 'typescript' }); - await realKernel.start(); + await realKernel.start({ + disableUI: false, + onDidChangeDisableUI: () => ({ + dispose: noop + }) + }); assert.equal(startEventCounter, 1); const cell = notebook.cellAt(0)!; @@ -270,14 +295,6 @@ suiteMandatory('Kernel API Tests @typescript', function () { await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); assert.isTrue(cellHasOutput(cell, '1', expectedMime)); assert.equal(startEventCounter, 1); - - // Start event should be triggered on restart - await realKernel.restart(); - assert.equal(startEventCounter, 2); - - await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); - assert.equal(startEventCounter, 2); - assert.isTrue(cellHasOutput(cell, '2', expectedMime)); }); async function cellHasOutput(cell: NotebookCell, expectedOutput: string, expectedMimetype: string) { diff --git a/src/standalone/api/kernels/index.ts b/src/standalone/api/kernels/index.ts index d375e9e8114..e286069b4c1 100644 --- a/src/standalone/api/kernels/index.ts +++ b/src/standalone/api/kernels/index.ts @@ -29,7 +29,8 @@ export function getKernelsApi(extensionId: string): Kernels { const notebook = workspace.notebookDocuments.find((item) => item.uri.toString() === uri.toString()); const kernel = kernelProvider.get(notebook || uri); // We are only interested in returning kernels that have been started by the user. - if (!kernel || !kernel.startedAtLeastOnce) { + // Returning started kernels is not sufficient as we also pre-warm kernels (i.e. we start kernels even though the user may not have executed any code). + if (!kernel || !kernel.startedAtLeastOnce || !kernel.userStartedKernel) { sendTelemetryEvent(Telemetry.NewJupyterKernelsApiUsage, undefined, { extensionId, pemUsed: 'getKernel', From 1abef06b36641f5c90e981a58c533de115da00ec Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 15 Nov 2024 10:27:30 -0800 Subject: [PATCH 10/16] PR --- .../api/kernels/api.vscode.common.test.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/standalone/api/kernels/api.vscode.common.test.ts b/src/standalone/api/kernels/api.vscode.common.test.ts index 76efd749eea..0e2bdaf94c8 100644 --- a/src/standalone/api/kernels/api.vscode.common.test.ts +++ b/src/standalone/api/kernels/api.vscode.common.test.ts @@ -262,12 +262,12 @@ suiteMandatory('Kernel API Tests @typescript', function () { await insertCodeCell('console.log(foo)', { index: 0, language: 'typescript' }); await realKernel.start({ - disableUI: false, + disableUI: true, onDidChangeDisableUI: () => ({ dispose: noop }) }); - assert.equal(startEventCounter, 1); + assert.equal(startEventCounter, 0, 'Kernel start event was triggered for a non-user kernel start'); const cell = notebook.cellAt(0)!; const executionOrderSet = createDeferred(); @@ -281,7 +281,10 @@ suiteMandatory('Kernel API Tests @typescript', function () { // Validate the cell execution output is equal to the expected value of "foo = 1" const expectedMime = NotebookCellOutputItem.stdout('').mime; - assert.isTrue(cellHasOutput(cell, '1', expectedMime)); + assert.isTrue( + cellHasOutput(cell, '1', expectedMime), + 'Invalid output, kernel start hook should execute code first' + ); const kernel = await kernels.getKernel(notebook.uri); if (!kernel) { @@ -289,12 +292,12 @@ suiteMandatory('Kernel API Tests @typescript', function () { } // Start event counter should only be 1 after the initial user cell execution - assert.equal(startEventCounter, 1); + assert.equal(startEventCounter, 1, 'Kernel start event was not triggered for a user kernel start'); // Running the same cell again should not fire additional events await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); assert.isTrue(cellHasOutput(cell, '1', expectedMime)); - assert.equal(startEventCounter, 1); + assert.equal(startEventCounter, 1, 'Start event should not be triggered more than once'); }); async function cellHasOutput(cell: NotebookCell, expectedOutput: string, expectedMimetype: string) { From ae766b686adc182ecde73b904c130925fe6d591a Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 15 Nov 2024 10:28:08 -0800 Subject: [PATCH 11/16] PR --- src/standalone/api/kernels/api.vscode.common.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/standalone/api/kernels/api.vscode.common.test.ts b/src/standalone/api/kernels/api.vscode.common.test.ts index 0e2bdaf94c8..74eaae284e9 100644 --- a/src/standalone/api/kernels/api.vscode.common.test.ts +++ b/src/standalone/api/kernels/api.vscode.common.test.ts @@ -246,6 +246,8 @@ suiteMandatory('Kernel API Tests @typescript', function () { }); await realKernel.restart(); assert.equal(startEventCounter, 1, 'Kernel start event should be fired exactly once after restarting'); + await realKernel.restart(); + assert.equal(startEventCounter, 2, 'Kernel start event should be fired more than once for restarts'); }); testMandatory('Kernel start event is triggered before first user execution', async function () { // Register event listener to track invocations From f8b3d8020d665874c5e763ce7accedc760fa6a81 Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 15 Nov 2024 10:31:36 -0800 Subject: [PATCH 12/16] PR --- src/standalone/api/kernels/api.vscode.common.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/standalone/api/kernels/api.vscode.common.test.ts b/src/standalone/api/kernels/api.vscode.common.test.ts index 74eaae284e9..e365e26ae8e 100644 --- a/src/standalone/api/kernels/api.vscode.common.test.ts +++ b/src/standalone/api/kernels/api.vscode.common.test.ts @@ -40,7 +40,6 @@ import { KernelError } from '../../../kernels/errors/kernelError'; import { JVSC_EXTENSION_ID } from '../../../platform/common/constants'; import { escapeStringToEmbedInPythonCode } from '../../../kernels/chat/generator'; import { notebookCellExecutions } from '../../../platform/notebooks/cellExecutionStateService'; -import { createKernelApiForExtension } from './kernel'; import { noop } from '../../../test/core'; suiteMandatory('Kernel API Tests @typescript', function () { From a792cc43e66af52e53e5b07bc6bb2ffdbe32c3e9 Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 15 Nov 2024 13:05:01 -0800 Subject: [PATCH 13/16] fix tests --- src/kernels/kernel.ts | 2 +- .../api/kernels/api.vscode.common.test.ts | 18 +++++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/kernels/kernel.ts b/src/kernels/kernel.ts index 139353dd7bd..a63893ad1fb 100644 --- a/src/kernels/kernel.ts +++ b/src/kernels/kernel.ts @@ -142,7 +142,7 @@ abstract class BaseKernel implements IBaseKernel { return this._startedAtLeastOnce; } get userStartedKernel() { - return this.startupUI.disableUI; + return !this.startupUI.disableUI; } private _info?: KernelMessage.IInfoReplyMsg['content']; private _startedAtLeastOnce?: boolean; diff --git a/src/standalone/api/kernels/api.vscode.common.test.ts b/src/standalone/api/kernels/api.vscode.common.test.ts index e365e26ae8e..334c6414dac 100644 --- a/src/standalone/api/kernels/api.vscode.common.test.ts +++ b/src/standalone/api/kernels/api.vscode.common.test.ts @@ -115,7 +115,12 @@ suiteMandatory('Kernel API Tests @typescript', function () { // Even after starting a kernel the API should not return anything, // as no code has been executed against this kernel. - await realKernel.start(); + await realKernel.start({ + disableUI: true, + onDidChangeDisableUI: () => ({ + dispose: noop + }) + }); assert.isUndefined(await kernels.getKernel(notebook.uri)); // Ensure user has executed some code against this kernel. @@ -253,7 +258,7 @@ suiteMandatory('Kernel API Tests @typescript', function () { const source = new CancellationTokenSource(); let startEventCounter = 0; disposables.push( - kernels.onDidStart(({ kernel }) => { + kernels.onDidStart(async ({ kernel }) => { const codeToRun = startEventCounter === 0 ? `let foo = ${startEventCounter}` : `foo = ${startEventCounter}`; startEventCounter++; @@ -280,10 +285,10 @@ suiteMandatory('Kernel API Tests @typescript', function () { disposables.push(eventHandler); await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); - // Validate the cell execution output is equal to the expected value of "foo = 1" + // Validate the cell execution output is equal to the expected value of "foo = 0" const expectedMime = NotebookCellOutputItem.stdout('').mime; assert.isTrue( - cellHasOutput(cell, '1', expectedMime), + await cellHasOutput(cell, '0', expectedMime), 'Invalid output, kernel start hook should execute code first' ); @@ -297,7 +302,10 @@ suiteMandatory('Kernel API Tests @typescript', function () { // Running the same cell again should not fire additional events await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); - assert.isTrue(cellHasOutput(cell, '1', expectedMime)); + assert.isTrue( + await cellHasOutput(cell, '0', expectedMime), + 'Invalid output, kernel start hook should only execute once' + ); assert.equal(startEventCounter, 1, 'Start event should not be triggered more than once'); }); From 55401d8c90288e08a57dd9e17b37cf6e3b1c95ea Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 15 Nov 2024 13:16:51 -0800 Subject: [PATCH 14/16] proposed fix --- src/kernels/execution/cellExecutionMessageHandler.ts | 1 + src/standalone/api/kernels/api.vscode.common.test.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/kernels/execution/cellExecutionMessageHandler.ts b/src/kernels/execution/cellExecutionMessageHandler.ts index a110728f1e4..0745ddfa311 100644 --- a/src/kernels/execution/cellExecutionMessageHandler.ts +++ b/src/kernels/execution/cellExecutionMessageHandler.ts @@ -1078,6 +1078,7 @@ export class CellExecutionMessageHandler implements IDisposable { const cellExecution = CellExecutionCreator.get(this.cell); if (cellExecution && msg.content.ename !== 'KeyboardInterrupt') { cellExecution.errorInfo = { + name: msg.content.ename, message: `${msg.content.ename}: ${msg.content.evalue}`, location: findErrorLocation(msg.content.traceback, this.cell), uri: this.cell.document.uri, diff --git a/src/standalone/api/kernels/api.vscode.common.test.ts b/src/standalone/api/kernels/api.vscode.common.test.ts index 334c6414dac..ebbd01c2ee1 100644 --- a/src/standalone/api/kernels/api.vscode.common.test.ts +++ b/src/standalone/api/kernels/api.vscode.common.test.ts @@ -258,7 +258,7 @@ suiteMandatory('Kernel API Tests @typescript', function () { const source = new CancellationTokenSource(); let startEventCounter = 0; disposables.push( - kernels.onDidStart(async ({ kernel }) => { + kernels.onDidStart(({ kernel }) => { const codeToRun = startEventCounter === 0 ? `let foo = ${startEventCounter}` : `foo = ${startEventCounter}`; startEventCounter++; From 28113a338dcb30179f0cb6bef7d5e91bb607b077 Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 15 Nov 2024 14:05:02 -0800 Subject: [PATCH 15/16] fix tests --- .../api/kernels/api.vscode.common.test.ts | 126 ++++++++++-------- 1 file changed, 67 insertions(+), 59 deletions(-) diff --git a/src/standalone/api/kernels/api.vscode.common.test.ts b/src/standalone/api/kernels/api.vscode.common.test.ts index ebbd01c2ee1..1200bb93156 100644 --- a/src/standalone/api/kernels/api.vscode.common.test.ts +++ b/src/standalone/api/kernels/api.vscode.common.test.ts @@ -216,7 +216,7 @@ suiteMandatory('Kernel API Tests @typescript', function () { }); assert.equal(startEventCounter, 0, 'Kernel start event was triggered for a non-user kernel start'); }); - testMandatory('Kernel start event is triggered when user executes code', async function () { + testMandatory('Kernel start event is triggered when started with UI enabled', async function () { let startEventCounter = 0; // Register event listener to track invocations disposables.push( @@ -253,73 +253,81 @@ suiteMandatory('Kernel API Tests @typescript', function () { await realKernel.restart(); assert.equal(startEventCounter, 2, 'Kernel start event should be fired more than once for restarts'); }); - testMandatory('Kernel start event is triggered before first user execution', async function () { - // Register event listener to track invocations - const source = new CancellationTokenSource(); - let startEventCounter = 0; - disposables.push( - kernels.onDidStart(({ kernel }) => { - const codeToRun = - startEventCounter === 0 ? `let foo = ${startEventCounter}` : `foo = ${startEventCounter}`; - startEventCounter++; - kernel.executeCode(codeToRun, source.token); - }) - ); - await insertCodeCell('console.log(foo)', { index: 0, language: 'typescript' }); - - await realKernel.start({ - disableUI: true, - onDidChangeDisableUI: () => ({ - dispose: noop - }) - }); - assert.equal(startEventCounter, 0, 'Kernel start event was triggered for a non-user kernel start'); + testMandatory( + 'Kernel start event is triggered when user executes code and the event execution runs first', + async function () { + // Register event listener to track invocations + const source = new CancellationTokenSource(); + let startEventCounter = 0; + disposables.push( + kernels.onDidStart(async ({ kernel }) => { + const codeToRun = + startEventCounter === 0 ? `let foo = ${startEventCounter}` : `foo = ${startEventCounter}`; + startEventCounter++; + + // This is needed for the async generator to get executed. + // eslint-disable-next-line @typescript-eslint/no-unused-vars + for await (const _out of kernel.executeCode(codeToRun, source.token)) { + } + }) + ); + await insertCodeCell('console.log(foo)', { index: 0, language: 'typescript' }); - const cell = notebook.cellAt(0)!; - const executionOrderSet = createDeferred(); - const eventHandler = notebookCellExecutions.onDidChangeNotebookCellExecutionState((e) => { - if (e.cell === cell && e.cell.executionSummary?.executionOrder) { - executionOrderSet.resolve(); + await realKernel.start({ + disableUI: true, + onDidChangeDisableUI: () => ({ + dispose: noop + }) + }); + assert.equal(startEventCounter, 0, 'Kernel start event was triggered for a non-user kernel start'); + const cell = notebook.cellAt(0)!; + const executionOrderSet = createDeferred(); + const eventHandler = notebookCellExecutions.onDidChangeNotebookCellExecutionState((e) => { + if (e.cell === cell && e.cell.executionSummary?.executionOrder) { + executionOrderSet.resolve(); + } + }); + disposables.push(eventHandler); + await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); + + // Validate the cell execution output is equal to the expected value of "foo = 0" + const expectedMime = NotebookCellOutputItem.stdout('').mime; + assert.equal( + await decodeFirstOutput(cell, expectedMime), + '0', + 'Invalid output, kernel start hook should execute code first' + ); + + const kernel = await kernels.getKernel(notebook.uri); + if (!kernel) { + throw new Error('Kernel not found'); } - }); - disposables.push(eventHandler); - await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); - - // Validate the cell execution output is equal to the expected value of "foo = 0" - const expectedMime = NotebookCellOutputItem.stdout('').mime; - assert.isTrue( - await cellHasOutput(cell, '0', expectedMime), - 'Invalid output, kernel start hook should execute code first' - ); - const kernel = await kernels.getKernel(notebook.uri); - if (!kernel) { - throw new Error('Kernel not found'); + // Start event counter should only be 1 after the initial user cell execution + assert.equal(startEventCounter, 1, 'Kernel start event was not triggered for a user kernel start'); + + // Running the same cell again should not fire additional events + await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); + assert.equal( + await decodeFirstOutput(cell, expectedMime), + '0', + 'Invalid output, kernel start hook should only execute once' + ); + assert.equal(startEventCounter, 1, 'Start event should not be triggered more than once'); } + ); - // Start event counter should only be 1 after the initial user cell execution - assert.equal(startEventCounter, 1, 'Kernel start event was not triggered for a user kernel start'); - - // Running the same cell again should not fire additional events - await Promise.all([runCell(cell), waitForExecutionCompletedSuccessfully(cell), executionOrderSet.promise]); - assert.isTrue( - await cellHasOutput(cell, '0', expectedMime), - 'Invalid output, kernel start hook should only execute once' - ); - assert.equal(startEventCounter, 1, 'Start event should not be triggered more than once'); - }); - - async function cellHasOutput(cell: NotebookCell, expectedOutput: string, expectedMimetype: string) { - return Boolean( - cell.outputs.find((output) => - output.items.some((item) => { + async function decodeFirstOutput(cell: NotebookCell, expectedMimetype: string) { + return ( + cell.outputs + .flatMap((output) => output.items) + .map((item) => { if (item.mime === expectedMimetype) { const output = new TextDecoder().decode(item.data).trim(); - return output === expectedOutput; + return output; } - return false; }) - ) + .find((item) => item !== undefined) ?? '' ); } From 828b767ccb212838b27be94a6173ab61ddf804cb Mon Sep 17 00:00:00 2001 From: Paul Wang Date: Fri, 15 Nov 2024 14:16:36 -0800 Subject: [PATCH 16/16] update test --- src/standalone/api/kernels/api.vscode.common.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/standalone/api/kernels/api.vscode.common.test.ts b/src/standalone/api/kernels/api.vscode.common.test.ts index 1200bb93156..9236e2e68ff 100644 --- a/src/standalone/api/kernels/api.vscode.common.test.ts +++ b/src/standalone/api/kernels/api.vscode.common.test.ts @@ -232,6 +232,15 @@ suiteMandatory('Kernel API Tests @typescript', function () { }) }); assert.equal(startEventCounter, 1, 'Kernel start event was not triggered for a user kernel start'); + + // If we call start again with UI enabled, we shouldn't fire additional events + await realKernel.start({ + disableUI: false, + onDidChangeDisableUI: () => ({ + dispose: noop + }) + }); + assert.equal(startEventCounter, 1, 'Multiple start calls should not fire more events'); }); testMandatory('Kernel start event is triggered when kernel restarts', async function () { let startEventCounter = 0;