From 57f958f8dd71c13d694ddd4e15857d8cfc1f3aee Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 8 Dec 2021 09:52:30 -0800 Subject: [PATCH] Show modal error when remote jupyter connect fails (#8476) --- news/3 Code Health/8474.md | 1 + package.nls.json | 2 + src/client/common/utils/localize.ts | 8 ++++ .../notebookServerProvider.ts | 11 ++++- .../datascience/jupyter/serverSelector.ts | 3 +- .../notebook/notebookControllerManager.ts | 39 +++++++++++++-- src/client/telemetry/index.ts | 8 +++- .../notebookServerProvider.unit.test.ts | 5 +- .../notebook/kernelSelection.vscode.test.ts | 48 +++++++++++-------- 9 files changed, 97 insertions(+), 28 deletions(-) create mode 100644 news/3 Code Health/8474.md diff --git a/news/3 Code Health/8474.md b/news/3 Code Health/8474.md new file mode 100644 index 00000000000..4ada2692ab7 --- /dev/null +++ b/news/3 Code Health/8474.md @@ -0,0 +1 @@ +Display error message when failing to connect to the remote Jupyter server. diff --git a/package.nls.json b/package.nls.json index 95004e04852..c1c65c55c95 100644 --- a/package.nls.json +++ b/package.nls.json @@ -278,6 +278,8 @@ "DataScience.jupyterNotebookConnectFailed": "Failed to connect to Jupyter notebook. \r\n{0}\r\n{1}", "DataScience.jupyterNotebookRemoteConnectFailed": "Failed to connect to remote Jupyter notebook.\r\nCheck that the Jupyter Server URI setting has a valid running server specified.\r\n{0}\r\n{1}", "DataScience.jupyterNotebookRemoteConnectSelfCertsFailed": "Failed to connect to remote Jupyter notebook.\r\nSpecified server is using self signed certs. Enable Allow Unauthorized Remote Connection setting to connect anyways\r\n{0}\r\n{1}", + "DataScience.jupyterRemoteConnectFailedModalMessage": "Failed to connect to the remote Jupyter Server. View Jupyter log for further details.", + "DataScience.changeJupyterRemoteConnection": "Change Jupyter Server connection.", "DataScience.notebookVersionFormat": "Jupyter Notebook Version: {0}", "DataScience.jupyterKernelSpecNotFound": "Cannot create a Jupyter kernel spec and none are available for use", "DataScience.jupyterKernelSpecModuleNotFound": "'Kernelspec' module not installed in the selected interpreter ({0}).\n Please re-install or update 'jupyter'.", diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 994b00977f1..5fc88b907c2 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -521,6 +521,14 @@ export namespace DataScience { 'DataScience.jupyterNotebookRemoteConnectFailed', 'Failed to connect to remote Jupyter notebook.\r\nCheck that the Jupyter Server URI setting has a valid running server specified.\r\n{0}\r\n{1}' ); + export const jupyterRemoteConnectFailedModalMessage = localize( + 'DataScience.jupyterRemoteConnectFailedModalMessage', + 'Failed to connect to the remote Jupyter Server. View Jupyter log for further details.' + ); + export const changeJupyterRemoteConnection = localize( + 'DataScience.changeJupyterRemoteConnection', + 'Change Jupyter Server connection.' + ); export const jupyterNotebookRemoteConnectSelfCertsFailed = localize( 'DataScience.jupyterNotebookRemoteConnectSelfCertsFailed', 'Failed to connect to remote Jupyter notebook.\r\nSpecified server is using self signed certs. Enable Allow Unauthorized Remote Connection setting to connect anyways\r\n{0}\r\n{1}' diff --git a/src/client/datascience/interactive-common/notebookServerProvider.ts b/src/client/datascience/interactive-common/notebookServerProvider.ts index f979834024d..4439759047e 100644 --- a/src/client/datascience/interactive-common/notebookServerProvider.ts +++ b/src/client/datascience/interactive-common/notebookServerProvider.ts @@ -39,7 +39,16 @@ export class NotebookServerProvider implements IJupyterServerProvider { @inject(IJupyterServerUriStorage) private readonly serverUriStorage: IJupyterServerUriStorage, @inject(JupyterServerSelector) private serverSelector: JupyterServerSelector, @inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry - ) {} + ) { + serverUriStorage.onDidChangeUri( + () => { + // Possible user selected another Server. + this.serverPromise.remote = undefined; + }, + this, + this.disposables + ); + } public async getOrCreateServer(options: GetServerOptions): Promise { const serverOptions = await this.getNotebookServerOptions(options.resource, options.localJupyter === true); diff --git a/src/client/datascience/jupyter/serverSelector.ts b/src/client/datascience/jupyter/serverSelector.ts index 9804a9142c1..2a8fed70806 100644 --- a/src/client/datascience/jupyter/serverSelector.ts +++ b/src/client/datascience/jupyter/serverSelector.ts @@ -37,7 +37,8 @@ export type SelectJupyterUriCommandSource = | 'toolbar' | 'commandPalette' | 'nativeNotebookStatusBar' - | 'nativeNotebookToolbar'; + | 'nativeNotebookToolbar' + | 'prompt'; @injectable() export class JupyterServerSelector { private readonly localLabel = `$(zap) ${DataScience.jupyterSelectURILocalLabel()}`; diff --git a/src/client/datascience/notebook/notebookControllerManager.ts b/src/client/datascience/notebook/notebookControllerManager.ts index 68886c42723..7fd15f3cdec 100644 --- a/src/client/datascience/notebook/notebookControllerManager.ts +++ b/src/client/datascience/notebook/notebookControllerManager.ts @@ -57,6 +57,8 @@ import { IInterpreterService } from '../../interpreter/contracts'; import { KernelFilterService } from './kernelFilter/kernelFilterService'; import { getDisplayPath } from '../../common/platform/fs-paths'; import { DisplayOptions } from '../displayOptions'; +import { JupyterServerSelector } from '../jupyter/serverSelector'; +import { DataScience } from '../../common/utils/localize'; /** * This class tracks notebook documents that are open and the provides NotebookControllers for @@ -77,6 +79,7 @@ export class NotebookControllerManager implements INotebookControllerManager, IE private selectedControllers = new Map(); private readonly allKernelConnections = new Set(); private _controllersLoaded?: boolean; + private failedToFetchRemoteKernels?: boolean; public get onNotebookControllerSelectionChanged() { return this._onNotebookControllerSelectionChanged.event; } @@ -119,6 +122,7 @@ export class NotebookControllerManager implements INotebookControllerManager, IE @inject(IApplicationShell) private readonly appShell: IApplicationShell, @inject(KernelFilterService) private readonly kernelFilter: KernelFilterService, @inject(IBrowserService) private readonly browser: IBrowserService, + @inject(JupyterServerSelector) private readonly jupyterServerSelector: JupyterServerSelector, @inject(IJupyterServerUriStorage) private readonly serverUriStorage: IJupyterServerUriStorage ) { this._onNotebookControllerSelected = new EventEmitter<{ @@ -453,7 +457,32 @@ export class NotebookControllerManager implements INotebookControllerManager, IE // Prep so that we can track the selected controller for this document traceInfoIfCI(`Clear controller mapping for ${getDisplayPath(document.uri)}`); const loadControllersPromise = this.loadNotebookControllers(); - + if (!this.isLocalLaunch) { + void loadControllersPromise.finally(() => { + if (this.isLocalLaunch) { + return; + } + if (this.failedToFetchRemoteKernels) { + void this.appShell + .showErrorMessage( + DataScience.jupyterRemoteConnectFailedModalMessage(), + { modal: true }, + DataScience.changeJupyterRemoteConnection(), + DataScience.showJupyterLogs() + ) + .then((selection) => { + switch (selection) { + case DataScience.changeJupyterRemoteConnection(): + void this.jupyterServerSelector.selectJupyterURI(true, 'prompt'); + break; + case DataScience.showJupyterLogs(): + void this.commandManager.executeCommand('jupyter.viewOutput'); + break; + } + }); + } + }); + } if ( isPythonNotebook(getNotebookMetadata(document)) && this.extensionChecker.isPythonExtensionInstalled && @@ -724,11 +753,11 @@ export class NotebookControllerManager implements INotebookControllerManager, IE token }); - return this.remoteKernelFinder.listKernels(undefined, connection, token).catch((ex) => { - traceError('Failed to get remote kernel connections', ex); - return [] as KernelConnectionMetadata[]; - }); + const kernels = await this.remoteKernelFinder.listKernels(undefined, connection, token); + this.failedToFetchRemoteKernels = false; + return kernels; } catch (ex) { + this.failedToFetchRemoteKernels = true; traceError('Failed to get remote kernel connections', ex); return [] as KernelConnectionMetadata[]; } finally { diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index d15785d971a..f9ca8690507 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -870,7 +870,13 @@ export interface IEventNamePropertyMapping { * nativeNotebookStatusBar - Invoked from Native notebook statusbar. * nativeNotebookToolbar - Invoked from Native notebook toolbar. */ - commandSource: 'nonUser' | 'commandPalette' | 'toolbar' | 'nativeNotebookStatusBar' | 'nativeNotebookToolbar'; + commandSource: + | 'nonUser' + | 'commandPalette' + | 'toolbar' + | 'nativeNotebookStatusBar' + | 'nativeNotebookToolbar' + | 'prompt'; }; [Telemetry.SetJupyterURIToLocal]: never | undefined; [Telemetry.SetJupyterURIToUserSpecified]: { diff --git a/src/test/datascience/interactive-common/notebookServerProvider.unit.test.ts b/src/test/datascience/interactive-common/notebookServerProvider.unit.test.ts index 62362f1dda4..da4df7f0423 100644 --- a/src/test/datascience/interactive-common/notebookServerProvider.unit.test.ts +++ b/src/test/datascience/interactive-common/notebookServerProvider.unit.test.ts @@ -4,7 +4,7 @@ import { expect } from 'chai'; import { SemVer } from 'semver'; import { anything, instance, mock, verify, when } from 'ts-mockito'; import * as typemoq from 'typemoq'; -import { CancellationTokenSource, Disposable } from 'vscode'; +import { CancellationTokenSource, Disposable, EventEmitter } from 'vscode'; import { disposeAllDisposables } from '../../../client/common/helpers'; import { IConfigurationService, IWatchableJupyterSettings } from '../../../client/common/types'; import { DisplayOptions } from '../../../client/datascience/displayOptions'; @@ -52,6 +52,9 @@ suite('DataScience - NotebookServerProvider', () => { const serverStorage = mock(JupyterServerUriStorage); when(serverStorage.getUri()).thenResolve('local'); const serverSelector = mock(JupyterServerSelector); + const eventEmitter = new EventEmitter(); + disposables.push(eventEmitter); + when(serverStorage.onDidChangeUri).thenReturn(eventEmitter.event); when((jupyterExecution as any).then).thenReturn(undefined); when((serverSelector as any).then).thenReturn(undefined); when((serverStorage as any).then).thenReturn(undefined); diff --git a/src/test/datascience/notebook/kernelSelection.vscode.test.ts b/src/test/datascience/notebook/kernelSelection.vscode.test.ts index 135173271b9..88e29ef8537 100644 --- a/src/test/datascience/notebook/kernelSelection.vscode.test.ts +++ b/src/test/datascience/notebook/kernelSelection.vscode.test.ts @@ -31,7 +31,8 @@ import { waitForKernelToChange, waitForKernelToGetAutoSelected, waitForOutputs, - waitForTextOutput + waitForTextOutput, + defaultNotebookTestTimeout } from './helper'; /* eslint-disable no-invalid-this, , , @typescript-eslint/no-explicit-any */ @@ -61,7 +62,7 @@ suite('DataScience - VSCode Notebook - Kernel Selection', function () { const venvNoKernelSearchString = '.venvnokernel'; const venvKernelSearchString = '.venvkernel'; const venvNoRegSearchString = '.venvnoreg'; - let activeIntepreterSearchString = ''; + let activeInterpreterSearchString = ''; let vscodeNotebook: IVSCodeNotebook; this.timeout(120_000); // Slow test, we need to uninstall/install ipykernel. /* @@ -110,7 +111,7 @@ suite('DataScience - VSCode Notebook - Kernel Selection', function () { venvKernelPythonPath = interpreter2.path; venvNoRegPythonPath = interpreter3.path; venvNoKernelDisplayName = interpreter1.displayName || '.venvnokernel'; - activeIntepreterSearchString = + activeInterpreterSearchString = activeInterpreter.displayName === interpreter1.displayName ? venvNoKernelSearchString : activeInterpreter.displayName === interpreter2.displayName @@ -165,21 +166,30 @@ suite('DataScience - VSCode Notebook - Kernel Selection', function () { const cell = vscodeNotebook.activeNotebookEditor?.document.cellAt(0)!; await Promise.all([runAllCellsInActiveNotebook(), waitForExecutionCompletedSuccessfully(cell)]); - // Confirm the executable printed as a result of code in cell `import sys;sys.executable` - const output = getTextOutputValue(cell.outputs[0]); - if ( - !output.includes(activeIntepreterSearchString) && - !output.includes(getNormalizedInterpreterPath(activeInterpreterPath)) && - !output.includes(activeInterpreterPath) - ) { - assert.fail( - output, - `Expected ${getNormalizedInterpreterPath(activeInterpreterPath)} or ${activeInterpreterPath}`, - `Interpreter does not match for ${activeIntepreterSearchString}: expected ${getNormalizedInterpreterPath( - activeInterpreterPath - )} or ${activeInterpreterPath}, but go ${output}` - ); - } + await waitForCondition( + async () => { + // Confirm the executable printed as a result of code in cell `import sys;sys.executable` + const output = getTextOutputValue(cell.outputs[0]); + if ( + !output.includes(activeInterpreterSearchString) && + !output.includes(getNormalizedInterpreterPath(activeInterpreterPath)) && + !output.includes(activeInterpreterPath) + ) { + assert.fail( + output, + `Expected ${getNormalizedInterpreterPath(activeInterpreterPath)} or ${activeInterpreterPath}`, + `Interpreter does not match for ${activeInterpreterSearchString}: expected ${getNormalizedInterpreterPath( + activeInterpreterPath + )} or ${activeInterpreterPath}, but go ${output}` + ); + } + return true; + }, + defaultNotebookTestTimeout, + `Interpreter does not match for ${activeInterpreterSearchString}: expected ${getNormalizedInterpreterPath( + activeInterpreterPath + )} or ${activeInterpreterPath}, but go ${getTextOutputValue(cell.outputs[0])}` + ); }); test('Ensure kernel is auto selected and interpreter is as expected', async function () { if (IS_REMOTE_NATIVE_TEST) { @@ -265,7 +275,7 @@ suite('DataScience - VSCode Notebook - Kernel Selection', function () { const outputText = getTextOutputValue(cell.outputs[0]).trim(); // venvkernel might be the active one (if this test is run more than once) - if (activeIntepreterSearchString !== venvKernelSearchString) { + if (activeInterpreterSearchString !== venvKernelSearchString) { assert.equal(outputText.toLowerCase().indexOf(venvKernelSearchString), -1); }