Propagate execution failure (#14918)

* In Kernel API exec return output metadata along with output items * oops * wip * Propagate Kernel Exec errors upto 3rd party extension * oops * Found * Misc * Misc * Misc * Fix formatting * Fix tests * Fix tests * Misc * Revert changes * Fixes * Fixes * wip * Fixes * Fix tests * oops
microsoft · Dec 21, 2023 · 9c38e5b · 9c38e5b
1 parent f89536b
commit 9c38e5b
Show file tree

Hide file tree

Showing 12 changed files with 274 additions and 217 deletions.
diff --git a/src/interactive-window/interactiveWindow.ts b/src/interactive-window/interactiveWindow.ts
@@ -25,16 +25,9 @@ import { Commands, MARKDOWN_LANGUAGE, PYTHON_LANGUAGE, isWebExtension } from '..
 import { traceInfo, traceInfoIfCI, traceVerbose, traceWarning } from '../platform/logging';
 import { IFileSystem } from '../platform/common/platform/types';
 import uuid from 'uuid/v4';
-
 import { IConfigurationService, InteractiveWindowMode, Resource } from '../platform/common/types';
 import { noop } from '../platform/common/utils/misc';
-import {
-    IKernel,
-    IKernelProvider,
-    isLocalConnection,
-    KernelConnectionMetadata,
-    NotebookCellRunState
-} from '../kernels/types';
+import { IKernel, IKernelProvider, isLocalConnection, KernelConnectionMetadata } from '../kernels/types';
 import { chainable } from '../platform/common/utils/decorators';
 import { InteractiveCellResultError } from '../platform/errors/interactiveCellResultError';
 import { DataScience } from '../platform/common/utils/localize';
@@ -219,7 +212,7 @@ export class InteractiveWindow implements IInteractiveWindow {
         if (this.controller.controller) {
             this.controller.startKernel().catch(noop);
         } else {
-            traceInfo('No controller selected for Interactive Window initilization');
+            traceInfo('No controller selected for Interactive Window initialization');
             this.controller.setInfoMessageCell(DataScience.selectKernelForEditor);
         }
     }
@@ -437,8 +430,10 @@ export class InteractiveWindow implements IInteractiveWindow {
             traceInfoIfCI('InteractiveWindow.ts.createExecutionPromise.kernel.executeCell');
             const iwCellMetadata = getInteractiveCellMetadata(cell);
             const execution = this.kernelProvider.getKernelExecution(kernel!);
-            success =
-                (await execution.executeCell(cell, iwCellMetadata?.generatedCode?.code)) !== NotebookCellRunState.Error;
+            success = await execution.executeCell(cell, iwCellMetadata?.generatedCode?.code).then(
+                () => true,
+                () => false
+            );
             traceInfoIfCI('InteractiveWindow.ts.createExecutionPromise.kernel.executeCell.finished');
         } finally {
             await detachKernel();

diff --git a/src/kernels/api/api.vscode.common.test.ts b/src/kernels/api/api.vscode.common.test.ts
@@ -29,6 +29,7 @@ import { IKernel, IKernelProvider } from '../types';
 import { IControllerRegistration, IVSCodeNotebookController } from '../../notebooks/controllers/types';
 import { Kernels, Output } from '../../api';
 import { JVSC_EXTENSION_ID_FOR_TESTS } from '../../test/constants';
+import { KernelError } from '../errors/kernelError';
 
 suiteMandatory('Kernel API Tests @python', function () {
     const disposables: IDisposable[] = [];
@@ -132,9 +133,16 @@ suiteMandatory('Kernel API Tests @python', function () {
 
         // When we execute code that fails, we should get the error information.
         const errorOutputs: Output[] = [];
-        for await (const output of kernel.executeCode('Kaboom', token.token)) {
-            errorOutputs.push(output);
+        let exceptionThrown = false;
+        try {
+            for await (const output of kernel.executeCode('Kaboom', token.token)) {
+                errorOutputs.push(output);
+            }
+        } catch (ex) {
+            exceptionThrown = true;
+            assert.instanceOf(ex, KernelError, 'Error thrown is not a kernel error');
         }
+        assert.isTrue(exceptionThrown, 'Kernel Execution should fail with an error');
         assert.strictEqual(errorOutputs.length, 1, 'Incorrect number of outputs');
         assert.strictEqual(errorOutputs[0].items.length, 1, 'Incorrect number of output items');
         assert.strictEqual(

diff --git a/src/kernels/api/kernel.ts b/src/kernels/api/kernel.ts
@@ -25,7 +25,7 @@ import { Telemetry, sendTelemetryEvent } from '../../telemetry';
 import { StopWatch } from '../../platform/common/utils/stopWatch';
 import { Deferred, createDeferred, sleep } from '../../platform/common/utils/async';
 import { once } from '../../platform/common/utils/events';
-import { traceError, traceVerbose } from '../../platform/logging';
+import { traceVerbose } from '../../platform/logging';
 import { PYTHON_LANGUAGE } from '../../platform/common/constants';
 
 /**
@@ -215,59 +215,44 @@ class WrappedKernelPerExtension extends DisposableBase implements Kernel {
         }
 
         const disposables: IDisposable[] = [];
-        const done = createDeferred<void>();
         disposables.push({
             dispose: () => {
                 measures.duration = stopwatch.elapsedTime;
                 properties.mimeTypes = Array.from(mimeTypes).join(',');
                 completed = true;
-                done.resolve();
                 sendApiExecTelemetry(this.kernel, measures, properties).catch(noop);
             }
         });
         const kernelExecution = ServiceContainer.instance
             .get<IKernelProvider>(IKernelProvider)
             .getKernelExecution(this.kernel);
-        const outputs: Output[] = [];
-        let outputsReceived = createDeferred<void>();
-        kernelExecution
-            .executeCode(code, this.extensionId, token)
-            .then((codeExecution) => {
-                codeExecution.result.finally(() => dispose(disposables)).catch(noop);
-                codeExecution.onRequestSent(
-                    () => {
-                        properties.requestSent = true;
-                        measures.requestSentAfter = stopwatch.elapsedTime;
-                        if (!token.isCancellationRequested) {
-                            const progress = (this.previousProgress = this.progress.show());
-                            disposables.push(progress);
-                        }
-                    },
-                    this,
-                    disposables
-                );
-                codeExecution.onRequestAcknowledged(
-                    () => {
-                        properties.requestAcknowledged = true;
-                        measures.requestAcknowledgedAfter = stopwatch.elapsedTime;
-                    },
-                    this,
-                    disposables
-                );
-                codeExecution.onDidEmitOutput(
-                    (e) => {
-                        e.items.forEach((item) => mimeTypes.add(item.mime));
-                        outputs.push(e);
-                        outputsReceived.resolve();
-                        outputsReceived = createDeferred<void>();
-                    },
-                    this,
-                    disposables
-                );
-            })
-            .catch((ex) => {
-                traceError(`Extension ${this.extensionId} failed to execute code in kernel ${this.extensionId}`, ex);
-            });
+
+        const events = {
+            started: new EventEmitter<void>(),
+            executionAcknowledged: new EventEmitter<void>()
+        };
+
+        events.started.event(
+            () => {
+                properties.requestSent = true;
+                measures.requestSentAfter = stopwatch.elapsedTime;
+                if (!token.isCancellationRequested) {
+                    const progress = (this.previousProgress = this.progress.show());
+                    disposables.push(progress);
+                }
+            },
+            this,
+            disposables
+        );
+        events.executionAcknowledged.event(
+            () => {
+                properties.requestAcknowledged = true;
+                measures.requestAcknowledgedAfter = stopwatch.elapsedTime;
+            },
+            this,
+            disposables
+        );
+
         token.onCancellationRequested(
             () => {
                 if (completed) {
@@ -282,17 +267,14 @@ class WrappedKernelPerExtension extends DisposableBase implements Kernel {
             this,
             disposables
         );
-        while (true) {
-            await Promise.race([outputsReceived.promise, done.promise]);
-            if (outputsReceived.completed) {
-                outputsReceived = createDeferred<void>();
-            }
-            while (outputs.length) {
-                yield outputs.shift()!;
-            }
-            if (done.completed) {
-                break;
+
+        try {
+            for await (const output of kernelExecution.executeCode(code, this.extensionId, events, token)) {
+                output.items.forEach((output) => mimeTypes.add(output.mime));
+                yield output;
             }
+        } finally {
+            dispose(disposables);
         }
     }
 }

diff --git a/src/kernels/errors/kernelError.ts b/src/kernels/errors/kernelError.ts
@@ -0,0 +1,16 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+import type { IErrorMsg, IReplyErrorContent } from '@jupyterlab/services/lib/kernel/messages';
+
+export class KernelError extends Error {
+    public readonly ename: string;
+    public readonly evalue: string;
+    public readonly traceback: string[];
+    constructor(kernelError: IReplyErrorContent | IErrorMsg['content']) {
+        super(kernelError.evalue || kernelError.ename);
+        this.ename = kernelError.ename;
+        this.evalue = kernelError.evalue;
+        this.traceback = kernelError.traceback;
+    }
+}