Skip to content

Commit

Permalink
Display meaningful errors in cells and popups (#9312)
Browse files Browse the repository at this point in the history
  • Loading branch information
@DonJayamanne
DonJayamanne authored Mar 10, 2022
1 parent 98f4b05 commit fb383f3
Show file tree
Hide file tree
Showing 5 changed files with 153 additions and 47 deletions.
88 changes: 46 additions & 42 deletions src/client/datascience/errors/errorHandler.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
import type * as nbformat from '@jupyterlab/nbformat';
import { inject, injectable } from 'inversify';
import { IApplicationShell, IWorkspaceService } from '../../common/application/types';
import { BaseError, BaseKernelError, WrappedError, WrappedKernelError } from '../../common/errors/types';
Expand All @@ -9,8 +8,7 @@ import { Common, DataScience } from '../../common/utils/localize';
import { noop } from '../../common/utils/misc';
import { JupyterInstallError } from './jupyterInstallError';
import { JupyterSelfCertsError } from './jupyterSelfCertsError';
import { getDisplayNameOrNameOfKernelConnection, getLanguageInNotebookMetadata } from '../jupyter/kernels/helpers';
import { isPythonNotebook } from '../notebook/helpers/helpers';
import { getDisplayNameOrNameOfKernelConnection } from '../jupyter/kernels/helpers';
import {
IDataScienceErrorHandler,
IJupyterInterpreterDependencyManager,
Expand Down Expand Up @@ -91,23 +89,22 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {
err instanceof KernelProcessExitedError ||
err instanceof JupyterConnectError
) {
const defaultErrorMessage = getCombinedErrorMessage(
getErrorMessageFromPythonTraceback(err.stdErr) || err.stdErr
);
this.applicationShell.showErrorMessage(defaultErrorMessage).then(noop, noop);
this.applicationShell.showErrorMessage(getUserFriendlyErrorMessage(err)).then(noop, noop);
} else {
// Some errors have localized and/or formatted error messages.
const message = getCombinedErrorMessage(err.message || err.toString());
this.applicationShell.showErrorMessage(message).then(noop, noop);
}
}
public async getErrorMessageForDisplayInCell(error: Error) {
let message: string = error.message;
public async getErrorMessageForDisplayInCell(
error: Error,
context: 'start' | 'restart' | 'interrupt' | 'execution'
) {
error = WrappedError.unwrap(error);
if (error instanceof JupyterKernelDependencyError) {
message = getIPyKernelMissingErrorMessageForCell(error.kernelConnectionMetadata) || message;
return getIPyKernelMissingErrorMessageForCell(error.kernelConnectionMetadata) || error.message;
} else if (error instanceof JupyterInstallError) {
message = getJupyterMissingErrorMessageForCell(error) || message;
return getJupyterMissingErrorMessageForCell(error) || error.message;
} else if (error instanceof VscCancellationError || error instanceof CancellationError) {
// Don't show the message for cancellation errors
traceWarning(`Cancelled by user`, error);
Expand All @@ -121,7 +118,7 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {
) {
// We don't look for ipykernel dependencies before we start a kernel, hence
// its possible the kernel failed to start due to missing dependencies.
message = getIPyKernelMissingErrorMessageForCell(error.kernelConnectionMetadata) || message;
return getIPyKernelMissingErrorMessageForCell(error.kernelConnectionMetadata) || error.message;
} else if (error instanceof BaseKernelError || error instanceof WrappedKernelError) {
const failureInfo = analyzeKernelErrors(
workspace.workspaceFolders || [],
Expand All @@ -135,27 +132,20 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {
failureInfo.reason === KernelFailureReason.moduleNotFoundFailure &&
['ipykernel_launcher', 'ipykernel'].includes(failureInfo.moduleName)
) {
return getIPyKernelMissingErrorMessageForCell(error.kernelConnectionMetadata) || message;
return getIPyKernelMissingErrorMessageForCell(error.kernelConnectionMetadata) || error.message;
}
const messageParts = [failureInfo.message];
if (failureInfo.moreInfoLink) {
messageParts.push(Common.clickHereForMoreInfoWithHtml().format(failureInfo.moreInfoLink));
}
return messageParts.join('\n');
}
return getCombinedErrorMessage(
getErrorMessageFromPythonTraceback(error.stdErr) || error.stdErr || error.message
);
} else if (error instanceof BaseError) {
return getCombinedErrorMessage(
getErrorMessageFromPythonTraceback(error.stdErr) || error.stdErr || error.message
);
}
return message;
return getUserFriendlyErrorMessage(error, context);
}
public async handleKernelError(
err: Error,
purpose: 'start' | 'restart' | 'interrupt' | 'execution',
context: 'start' | 'restart' | 'interrupt' | 'execution',
kernelConnection: KernelConnectionMetadata,
resource: Resource
): Promise<KernelInterpreterDependencyResponse> {
Expand All @@ -166,7 +156,7 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {
if (err instanceof JupyterKernelDependencyError) {
return err.reason;
// Use the kernel dependency service to first determine if this is because dependencies are missing or not
} else if ((purpose === 'start' || purpose === 'restart') && err instanceof JupyterInstallError) {
} else if ((context === 'start' || context === 'restart') && err instanceof JupyterInstallError) {
const response = await this.dependencyManager.installMissingDependencies(err);
return response === JupyterInterpreterDependencyResponse.ok
? KernelInterpreterDependencyResponse.ok
Expand Down Expand Up @@ -196,7 +186,7 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {
traceWarning(`Cancelled by user`, err);
return KernelInterpreterDependencyResponse.cancel;
} else if (
(purpose === 'start' || purpose === 'restart') &&
(context === 'start' || context === 'restart') &&
!(await this.kernelDependency.areDependenciesInstalled(kernelConnection, undefined, true))
) {
const tokenSource = new CancellationTokenSource();
Expand All @@ -220,13 +210,10 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {
);
if (failureInfo) {
void this.showMessageWithMoreInfo(failureInfo?.message, failureInfo?.moreInfoLink);
} else if (err instanceof BaseError) {
const message = getCombinedErrorMessage(
getErrorMessageFromPythonTraceback(err.stdErr) || err.stdErr || err.message
);
void this.showMessageWithMoreInfo(message);
} else {
void this.showMessageWithMoreInfo(err.message);
// These are generic errors, we have no idea what went wrong,
// hence add a descriptive prefix (message), that provides more context to the user.
void this.showMessageWithMoreInfo(getUserFriendlyErrorMessage(err, context));
}
return KernelInterpreterDependencyResponse.failed;
}
Expand All @@ -243,8 +230,35 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {
});
}
}
function getCombinedErrorMessage(message?: string) {
const errorMessage = ['', message || '']
const errorPrefixes = {
restart: DataScience.failedToRestartKernel(),
start: DataScience.failedToStartKernel(),
interrupt: DataScience.failedToInterruptKernel(),
execution: ''
};
/**
* Sometimes the errors thrown don't contain user friendly messages,
* all they contain is some cryptic or stdout or tracebacks.
* For such messages, provide more context on what went wrong.
*/
function getUserFriendlyErrorMessage(error: Error, context?: 'start' | 'restart' | 'interrupt' | 'execution') {
error = WrappedError.unwrap(error);
const errorPrefix = context ? errorPrefixes[context] : '';
if (error instanceof BaseError) {
// These are generic errors, we have no idea what went wrong,
// hence add a descriptive prefix (message), that provides more context to the user.
return getCombinedErrorMessage(
errorPrefix,
getErrorMessageFromPythonTraceback(error.stdErr) || error.stdErr || error.message
);
} else {
// These are generic errors, we have no idea what went wrong,
// hence add a descriptive prefix (message), that provides more context to the user.
return getCombinedErrorMessage(errorPrefix, error.message);
}
}
function getCombinedErrorMessage(prefix?: string, message?: string) {
const errorMessage = [prefix || '', message || '']
.map((line) => line.trim())
.filter((line) => line.length > 0)
.join(' \n');
Expand All @@ -255,16 +269,6 @@ function getCombinedErrorMessage(message?: string) {
}
return errorMessage;
}
export function getKernelNotInstalledErrorMessage(notebookMetadata?: nbformat.INotebookMetadata) {
const language = getLanguageInNotebookMetadata(notebookMetadata);
if (isPythonNotebook(notebookMetadata) || !language) {
return DataScience.pythonNotInstalled();
} else {
const kernelName = notebookMetadata?.kernelspec?.display_name || notebookMetadata?.kernelspec?.name || language;
return DataScience.kernelNotInstalled().format(kernelName);
}
}

function getIPyKernelMissingErrorMessageForCell(kernelConnection: KernelConnectionMetadata) {
if (
kernelConnection.kind === 'connectToLiveKernel' ||
Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/jupyter/kernels/kernelCommandListener.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export class KernelCommandListener implements IDataScienceCommandListener {
displayErrorsInCell(
currentCell,
cellExecution,
await this.errorHandler.getErrorMessageForDisplayInCell(ex)
await this.errorHandler.getErrorMessageForDisplayInCell(ex, context)
);
cellExecution.end(false);
} else {
Expand Down
4 changes: 3 additions & 1 deletion src/client/datascience/notebook/vscodeNotebookController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,8 +382,10 @@ export class VSCodeNotebookController implements Disposable {
void execution.clearOutput(cell);

// Connect to a matching kernel if possible (but user may pick a different one)
let context: 'start' | 'execution' = 'start';
try {
const kernel = await this.connectToKernel(doc);
context = 'execution';
if (kernel.controller.id === this.id) {
this.updateKernelInfoInNotebookWhenAvailable(kernel, doc);
}
Expand All @@ -392,7 +394,7 @@ export class VSCodeNotebookController implements Disposable {
const errorHandler = this.serviceContainer.get<IDataScienceErrorHandler>(IDataScienceErrorHandler);

// If there was a failure connecting or executing the kernel, stick it in this cell
displayErrorsInCell(cell, execution, await errorHandler.getErrorMessageForDisplayInCell(ex));
displayErrorsInCell(cell, execution, await errorHandler.getErrorMessageForDisplayInCell(ex, context));
const isCancelled =
(WrappedError.unwrap(ex) || ex) instanceof CancellationError ||
(WrappedError.unwrap(ex) || ex) instanceof VscCancellationError;
Expand Down
5 changes: 4 additions & 1 deletion src/client/datascience/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,10 @@ export interface IDataScienceErrorHandler {
kernelConnection: KernelConnectionMetadata,
resource: Resource
): Promise<KernelInterpreterDependencyResponse>;
getErrorMessageForDisplayInCell(err: Error): Promise<string>;
getErrorMessageForDisplayInCell(
err: Error,
context: 'start' | 'restart' | 'interrupt' | 'execution'
): Promise<string>;
}

/**
Expand Down
101 changes: 99 additions & 2 deletions src/test/datascience/errorHandler.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
import { getOSType, OSType } from '../common';
import { JupyterInterpreterService } from '../../client/datascience/jupyter/interpreter/jupyterInterpreterService';
import { JupyterConnectError } from '../../client/datascience/errors/jupyterConnectError';
import { PythonEnvironment } from '../../client/pythonEnvironments/info';
import { EnvironmentType, PythonEnvironment } from '../../client/pythonEnvironments/info';
import { JupyterInterpreterDependencyResponse } from '../../client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService';

suite('DataScience Error Handler Unit Tests', () => {
Expand All @@ -34,6 +34,7 @@ suite('DataScience Error Handler Unit Tests', () => {
let browser: IBrowserService;
let configuration: IConfigurationService;
let jupyterInterpreterService: JupyterInterpreterService;
let kernelDependencyInstaller: IKernelDependencyService;
const jupyterInterpreter: PythonEnvironment = {
displayName: 'Hello',
path: 'Some Path',
Expand All @@ -49,7 +50,7 @@ suite('DataScience Error Handler Unit Tests', () => {
jupyterInterpreterService = mock<JupyterInterpreterService>();
when(dependencyManager.installMissingDependencies(anything())).thenResolve();
when(workspaceService.workspaceFolders).thenReturn([]);
const kernelDependencyInstaller = mock<IKernelDependencyService>();
kernelDependencyInstaller = mock<IKernelDependencyService>();
when(kernelDependencyInstaller.areDependenciesInstalled(anything(), anything(), anything())).thenResolve(true);
dataScienceErrorHandler = new DataScienceErrorHandler(
instance(applicationShell),
Expand Down Expand Up @@ -501,6 +502,102 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
verify(dependencyManager.installMissingDependencies(anything())).once();
assert.strictEqual(result, KernelInterpreterDependencyResponse.cancel);
});
test('Verify error message for conda install of ipykernel', async () => {
when(kernelDependencyInstaller.areDependenciesInstalled(anything(), anything(), anything())).thenResolve(
false
);
when(dependencyManager.installMissingDependencies(anything())).thenResolve(
JupyterInterpreterDependencyResponse.cancel
);
const result = await dataScienceErrorHandler.getErrorMessageForDisplayInCell(
new KernelDiedError('Kaboom', 'hello word does not have attribute named abc', undefined, {
...kernelConnection,
interpreter: {
...kernelConnection.interpreter!,
envType: EnvironmentType.Conda,
envName: 'condaEnv1'
}
}),
'start'
);
assert.strictEqual(
result,
[
"Running cells with 'Hello (Some Path)' requires ipykernel package.",
"Run the following command to install 'ipykernel' into the Python environment. ",
`Command: 'conda install -n condaEnv1 ipykernel --update-deps --force-reinstall'`
].join('\n')
);
});
test('Verify error message for pip install of ipykernel', async () => {
when(kernelDependencyInstaller.areDependenciesInstalled(anything(), anything(), anything())).thenResolve(
false
);
when(dependencyManager.installMissingDependencies(anything())).thenResolve(
JupyterInterpreterDependencyResponse.cancel
);
const result = await dataScienceErrorHandler.getErrorMessageForDisplayInCell(
new KernelDiedError(
'Kaboom',
'hello word does not have attribute named abc',
undefined,
kernelConnection
),
'start'
);
assert.strictEqual(
result,
[
"Running cells with 'Hello (Some Path)' requires ipykernel package.",
"Run the following command to install 'ipykernel' into the Python environment. ",
`Command: '"Hello There" -m pip install ipykernel -U --force-reinstall'`
].join('\n')
);
});
test('Ensure we provide some context to startup failures', async () => {
when(dependencyManager.installMissingDependencies(anything())).thenResolve(
JupyterInterpreterDependencyResponse.cancel
);
const result = await dataScienceErrorHandler.getErrorMessageForDisplayInCell(
new KernelDiedError(
'Kaboom',
'hello word does not have attribute named abc',
undefined,
kernelConnection
),
'start'
);
assert.strictEqual(
result,
[
'Failed to start the Kernel. ',
'hello word does not have attribute named abc. ',
'View Jupyter [log](command:jupyter.viewOutput) for further details.'
].join('\n')
);
});
test('Ensure we provide some context to re-start failures', async () => {
when(dependencyManager.installMissingDependencies(anything())).thenResolve(
JupyterInterpreterDependencyResponse.cancel
);
const result = await dataScienceErrorHandler.getErrorMessageForDisplayInCell(
new KernelDiedError(
'Kaboom',
'hello word does not have attribute named abc',
undefined,
kernelConnection
),
'restart'
);
assert.strictEqual(
result,
[
'Failed to restart the Kernel. ',
'hello word does not have attribute named abc. ',
'View Jupyter [log](command:jupyter.viewOutput) for further details.'
].join('\n')
);
});
function verifyErrorMessage(message: string, linkInfo?: string) {
message = message.includes('command:jupyter.viewOutput')
? message
Expand Down

0 comments on commit fb383f3

Please sign in to comment.