From 708004b02e926181d43e18f3b308d31c0e4525fd Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 12 Jul 2022 11:04:54 -0700 Subject: [PATCH] Fix problems with context keys and strings for remote connections (#10768) * Fix context problems in kernel selection * Add news --- TELEMETRY.md | 16 +++++----- news/2 Fixes/10672.md | 1 + package.nls.json | 2 +- .../jupyter/launcher/serverUriStorage.ts | 20 ++++++++----- src/kernels/jupyter/serverSelector.ts | 22 ++++++++------ src/kernels/jupyter/types.ts | 1 + .../serverConnectionControllerCommands.ts | 30 ++++++++++++++----- 7 files changed, 59 insertions(+), 33 deletions(-) create mode 100644 news/2 Fixes/10672.md diff --git a/TELEMETRY.md b/TELEMETRY.md index 2882435c56e..3910eaf86bd 100644 --- a/TELEMETRY.md +++ b/TELEMETRY.md @@ -903,7 +903,7 @@ No properties for event @captureTelemetry(Telemetry.EnterJupyterURI) @traceDecoratorError('Failed to enter Jupyter Uri') - public async setJupyterURIToRemote(userURI: string, ignoreValidation?: boolean): Promise { + public async setJupyterURIToRemote(userURI: string | undefined, ignoreValidation?: boolean): Promise { // Double check this server can be connected to. Might need a password, might need a allowUnauthorized ``` @@ -4315,13 +4315,13 @@ No description provided [src/kernels/jupyter/serverSelector.ts](https://github.com/microsoft/vscode-jupyter/tree/main/src/kernels/jupyter/serverSelector.ts) ```typescript - await this.serverUriStorage.setUriToRemote(userURI, connection.displayName); + await this.serverUriStorage.setUriToRemote(userURI, connection.displayName); - // Indicate setting a jupyter URI to a remote setting. Check if an azure remote or not - sendTelemetryEvent(Telemetry.SetJupyterURIToUserSpecified, undefined, { - azure: userURI.toLowerCase().includes('azure') - }); - } + // Indicate setting a jupyter URI to a remote setting. Check if an azure remote or not + sendTelemetryEvent(Telemetry.SetJupyterURIToUserSpecified, undefined, { + azure: userURI.toLowerCase().includes('azure') + }); + } else { ``` @@ -8894,7 +8894,7 @@ No properties for event [src/kernels/jupyter/serverSelector.ts](https://github.com/microsoft/vscode-jupyter/tree/main/src/kernels/jupyter/serverSelector.ts) ```typescript } - } else { + } else if (userURI) { if (err.message.includes('Failed to fetch') && this.isWebExtension) { sendTelemetryEvent(Telemetry.FetchError, undefined, { currentTask: 'connecting' }); } diff --git a/news/2 Fixes/10672.md b/news/2 Fixes/10672.md new file mode 100644 index 00000000000..dea55080fd9 --- /dev/null +++ b/news/2 Fixes/10672.md @@ -0,0 +1 @@ +Fix language of picking a jupyter server when in the web extension. diff --git a/package.nls.json b/package.nls.json index 9019679f0fe..b629219ea53 100644 --- a/package.nls.json +++ b/package.nls.json @@ -780,7 +780,7 @@ "comment": ["{Locked='python -m jupyter notebook --version'}"] }, "DataScience.savePngTitle": "Save Image", - "DataScience.jupyterSelectURIQuickPickTitle": "Pick how to connect to Jupyter", + "DataScience.jupyterSelectURIQuickPickTitle": "Enter the URI of the running Jupyter server", "DataScience.jupyterSelectURIQuickPickPlaceholder": "Choose an option", "DataScience.jupyterSelectURIQuickPickCurrent": "Current: {0}", "DataScience.jupyterSelectURINoneLabel": "None", diff --git a/src/kernels/jupyter/launcher/serverUriStorage.ts b/src/kernels/jupyter/launcher/serverUriStorage.ts index 5da394297dd..80782fcf386 100644 --- a/src/kernels/jupyter/launcher/serverUriStorage.ts +++ b/src/kernels/jupyter/launcher/serverUriStorage.ts @@ -25,7 +25,7 @@ export class JupyterServerUriStorage implements IJupyterServerUriStorage, IServe private lastSavedList?: Promise< { uri: string; serverId: string; time: number; displayName?: string | undefined }[] >; - private currentUriPromise: Promise | undefined; + private currentUriPromise: Promise | undefined; private _currentServerId: string | undefined; private _localOnly: boolean = false; private _onDidChangeUri = new EventEmitter(); @@ -190,7 +190,7 @@ export class JupyterServerUriStorage implements IJupyterServerUriStorage, IServe }) ); } - public getUri(): Promise { + public getUri(): Promise { if (!this.currentUriPromise) { this.currentUriPromise = this.getUriInternal(); } @@ -226,18 +226,22 @@ export class JupyterServerUriStorage implements IJupyterServerUriStorage, IServe await this.setUri(uri); } - public async setUri(uri: string) { + public async setUriToNone(): Promise { + return this.setUri(undefined); + } + + public async setUri(uri: string | undefined) { // Set the URI as our current state this.currentUriPromise = Promise.resolve(uri); - this._currentServerId = await computeServerId(uri); - this._localOnly = uri === Settings.JupyterServerLocalLaunch || uri === undefined; + this._currentServerId = uri ? await computeServerId(uri) : undefined; + this._localOnly = (uri === Settings.JupyterServerLocalLaunch || uri === undefined) && !this.isWebExtension; this._onDidChangeUri.fire(); // Needs to happen as soon as we change so that dependencies update synchronously // No update the async parts await this.globalMemento.update(mementoKeyToIndicateIfConnectingToLocalKernelsOnly, this._localOnly); await this.globalMemento.update(currentServerHashKey, this._currentServerId); - if (!this._localOnly) { + if (!this._localOnly && uri) { await this.addToUriList(uri, Date.now(), uri); // Save in the storage (unique account per workspace) @@ -245,7 +249,7 @@ export class JupyterServerUriStorage implements IJupyterServerUriStorage, IServe await this.encryptedStorage.store(Settings.JupyterServerRemoteLaunchService, key, uri); } } - private async getUriInternal(): Promise { + private async getUriInternal(): Promise { if (this.isLocalLaunch) { return Settings.JupyterServerLocalLaunch; } else { @@ -258,7 +262,7 @@ export class JupyterServerUriStorage implements IJupyterServerUriStorage, IServe this._currentServerId = await computeServerId(storedUri); } - return storedUri || Settings.JupyterServerLocalLaunch; + return storedUri; } } diff --git a/src/kernels/jupyter/serverSelector.ts b/src/kernels/jupyter/serverSelector.ts index 4a64d337225..477a170708c 100644 --- a/src/kernels/jupyter/serverSelector.ts +++ b/src/kernels/jupyter/serverSelector.ts @@ -87,10 +87,10 @@ export class JupyterServerSelector { @captureTelemetry(Telemetry.EnterJupyterURI) @traceDecoratorError('Failed to enter Jupyter Uri') - public async setJupyterURIToRemote(userURI: string, ignoreValidation?: boolean): Promise { + public async setJupyterURIToRemote(userURI: string | undefined, ignoreValidation?: boolean): Promise { // Double check this server can be connected to. Might need a password, might need a allowUnauthorized try { - if (!ignoreValidation) { + if (!ignoreValidation && userURI) { await this.jupyterConnection.validateRemoteUri(userURI); } } catch (err) { @@ -106,7 +106,7 @@ export class JupyterServerSelector { if (!handled) { return; } - } else { + } else if (userURI) { if (err.message.includes('Failed to fetch') && this.isWebExtension) { sendTelemetryEvent(Telemetry.FetchError, undefined, { currentTask: 'connecting' }); } @@ -117,13 +117,17 @@ export class JupyterServerSelector { } } - const connection = await this.jupyterConnection.createConnectionInfo({ uri: userURI }); - await this.serverUriStorage.setUriToRemote(userURI, connection.displayName); + if (userURI) { + const connection = await this.jupyterConnection.createConnectionInfo({ uri: userURI }); + await this.serverUriStorage.setUriToRemote(userURI, connection.displayName); - // Indicate setting a jupyter URI to a remote setting. Check if an azure remote or not - sendTelemetryEvent(Telemetry.SetJupyterURIToUserSpecified, undefined, { - azure: userURI.toLowerCase().includes('azure') - }); + // Indicate setting a jupyter URI to a remote setting. Check if an azure remote or not + sendTelemetryEvent(Telemetry.SetJupyterURIToUserSpecified, undefined, { + azure: userURI.toLowerCase().includes('azure') + }); + } else { + await this.serverUriStorage.setUriToNone(); + } } private async startSelectingURI(input: IMultiStepInput<{}>, _state: {}): Promise | void> { diff --git a/src/kernels/jupyter/types.ts b/src/kernels/jupyter/types.ts index 0bd96668830..92d053a72a0 100644 --- a/src/kernels/jupyter/types.ts +++ b/src/kernels/jupyter/types.ts @@ -241,6 +241,7 @@ export interface IJupyterServerUriStorage { getRemoteUri(): Promise; setUriToLocal(): Promise; setUriToRemote(uri: string, displayName: string): Promise; + setUriToNone(): Promise; } export interface IBackupFile { diff --git a/src/notebooks/controllers/commands/serverConnectionControllerCommands.ts b/src/notebooks/controllers/commands/serverConnectionControllerCommands.ts index 55ba15b1ae5..f9930ae7060 100644 --- a/src/notebooks/controllers/commands/serverConnectionControllerCommands.ts +++ b/src/notebooks/controllers/commands/serverConnectionControllerCommands.ts @@ -84,11 +84,11 @@ export class ServerConnectionControllerCommands implements IExtensionSingleActiv this.disposables.push( this.commandManager.registerCommand(Commands.SwitchToAnotherRemoteKernels, this.switchToRemoteKernels, this) ); - this.disposables.push(this.serverConnectionType.onDidChange(this.onConnectionTypeChanged, this)); - this.onConnectionTypeChanged().ignoreErrors; + this.disposables.push(this.serverConnectionType.onDidChange(this.updateContextKeys, this)); + this.updateContextKeys().ignoreErrors; } - private async onConnectionTypeChanged() { + private async updateContextKeys() { const isLocal = this.serverConnectionType.isLocalLaunch; await (this.isWeb ? this.controllerLoader.loaded : Promise.resolve(true)); @@ -96,12 +96,15 @@ export class ServerConnectionControllerCommands implements IExtensionSingleActiv .set(isLocal || (this.isWeb && this.controllerRegistration.values.length === 0)) .ignoreErrors(); this.showingRemoteNotWebContext.set(!isLocal && !this.isWeb).ignoreErrors(); - this.showingRemoteContext.set(!isLocal).ignoreErrors(); + this.showingRemoteContext.set(!isLocal && this.controllerRegistration.values.length > 0).ignoreErrors(); } private async showVsCodeKernelPicker() { const activeEditor = this.notebooks.activeNotebookEditor; if (activeEditor) { + // Need to wait for controller to reupdate after + // switching local/remote + await this.controllerLoader.loaded; this.commandManager .executeCommand('notebook.selectKernel', { notebookEditor: activeEditor }) .then(noop, noop); @@ -111,9 +114,13 @@ export class ServerConnectionControllerCommands implements IExtensionSingleActiv private async switchToRemoteKernels() { const activeNotebookType = this.notebooks.activeNotebookEditor ? JupyterNotebookView : InteractiveWindowView; const startingLocal = this.serverConnectionType.isLocalLaunch; + const startingUri = await this.serverUriStorage.getRemoteUri(); const multiStep = this.multiStepFactory.create<{}>(); return multiStep.run( - this.startSwitchRun.bind(this, this.startSwitchingToRemote.bind(this, activeNotebookType, startingLocal)), + this.startSwitchRun.bind( + this, + this.startSwitchingToRemote.bind(this, activeNotebookType, startingLocal, startingUri) + ), {} ); } @@ -126,11 +133,12 @@ export class ServerConnectionControllerCommands implements IExtensionSingleActiv private async startSwitchingToRemote( activeNotebookType: typeof JupyterNotebookView | typeof InteractiveWindowView, startedLocal: boolean, + startingUri: string | undefined, input: IMultiStepInput<{}> ): Promise | void> { try { await this.serverSelector.selectJupyterURI('nonUser', input); - return this.showRemoteKernelPicker(activeNotebookType, startedLocal, input); + return this.showRemoteKernelPicker(activeNotebookType, startedLocal, startingUri, input); } catch (e) { if (e === InputFlowAction.back) { return this.showVsCodeKernelPicker(); @@ -141,6 +149,7 @@ export class ServerConnectionControllerCommands implements IExtensionSingleActiv private async showRemoteKernelPicker( activeNotebookType: typeof JupyterNotebookView | typeof InteractiveWindowView, startedLocal: boolean, + startedUri: string | undefined, input: IMultiStepInput<{}> ): Promise | void> { try { @@ -155,6 +164,9 @@ export class ServerConnectionControllerCommands implements IExtensionSingleActiv // They backed out. Put back to local if (startedLocal) { await this.serverSelector.setJupyterURIToLocal(); + } else { + // Web case is never local but we might have an empty URI + await this.serverSelector.setJupyterURIToRemote(startedUri); } throw e; } @@ -199,7 +211,7 @@ export class ServerConnectionControllerCommands implements IExtensionSingleActiv input ); } catch (e) { - // They backed out. Put back to local + // They backed out. Put back to remote if (!startedLocal && startedUri) { await this.serverSelector.setJupyterURIToRemote(startedUri, true); } @@ -302,6 +314,10 @@ export class ServerConnectionControllerCommands implements IExtensionSingleActiv id: controller.id, extension: JVSC_EXTENSION_ID }); + + // Update our context keys as they might have changed when + // moving to a new kernel + await this.updateContextKeys(); } } }