Skip to content

Remove external usages of jupyter.selectjupyteruri #13513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -549,12 +549,6 @@
"category": "Jupyter",
"enablement": "isWorkspaceTrusted && !jupyter.webExtension"
},
{
"command": "jupyter.selectjupyteruri",
"title": "%jupyter.command.jupyter.selectjupyteruri.title%",
"category": "Jupyter",
"enablement": "isWorkspaceTrusted"
},
{
"command": "jupyter.importnotebook",
"title": "%jupyter.command.jupyter.importnotebook.title%",
Expand Down Expand Up @@ -1358,11 +1352,6 @@
"command": "jupyter.interactive.clearAllCells",
"when": "editorFocus && editorLangId == python || activeEditor == 'workbench.editor.interactive'"
},
{
"command": "jupyter.selectjupyteruri",
"title": "%jupyter.command.jupyter.selectjupyteruri.title%",
"when": "isWorkspaceTrusted && config.notebook.kernelPicker.type != mru"
},
{
"command": "jupyter.clearSavedJupyterUris",
"title": "%jupyter.command.jupyter.clearSavedJupyterUris.title%",
Expand Down
1 change: 0 additions & 1 deletion package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@
"jupyter.command.jupyter.gotoNextCellInFile.title": "Go to Next Cell",
"jupyter.command.jupyter.gotoPrevCellInFile.title": "Go to Previous Cell",
"jupyter.command.jupyter.createnewinteractive.title": "Create Interactive Window",
"jupyter.command.jupyter.selectjupyteruri.title": "Specify Jupyter Server for Connections (deprecated)",
"jupyter.command.jupyter.importnotebook.title": {
"message": "Import Jupyter Notebook",
"comment": [
Expand Down
7 changes: 1 addition & 6 deletions src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import { IShowDataViewerFromVariablePanel } from './messageTypes';
import { Commands as DSCommands, CommandSource } from './platform/common/constants';
import { PythonEnvironment } from './platform/pythonEnvironments/info';
import { Channel } from './platform/common/application/types';
import { SelectJupyterUriCommandSource } from './kernels/jupyter/connection/serverSelector';

export type CommandIds = keyof ICommandNameArgumentTypeMapping;

Expand Down Expand Up @@ -178,11 +177,7 @@ export interface ICommandNameArgumentTypeMapping {
[DSCommands.ShowDataViewer]: [IShowDataViewerFromVariablePanel];
[DSCommands.RefreshDataViewer]: [];
[DSCommands.ClearSavedJupyterUris]: [];
[DSCommands.SelectJupyterURI]: [
boolean | undefined,
Uri | SelectJupyterUriCommandSource | undefined,
NotebookDocument | undefined
];
[DSCommands.SelectJupyterURI]: [Uri];
[DSCommands.RunByLine]: [NotebookCell];
[DSCommands.RunAndDebugCell]: [NotebookCell];
[DSCommands.RunByLineNext]: [NotebookCell];
Expand Down
4 changes: 1 addition & 3 deletions src/kernels/errors/kernelErrorHandler.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import { inject, injectable, optional } from 'inversify';
import { Uri } from 'vscode';
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../platform/common/application/types';
import { IApplicationShell, IWorkspaceService } from '../../platform/common/application/types';
import {
IBrowserService,
IConfigurationService,
Expand Down Expand Up @@ -43,7 +43,6 @@ export class DataScienceErrorHandlerNode extends DataScienceErrorHandler {
kernelDependency: IKernelDependencyService | undefined,
@inject(IWorkspaceService) workspaceService: IWorkspaceService,
@inject(IJupyterServerUriStorage) serverUriStorage: IJupyterServerUriStorage,
@inject(ICommandManager) commandManager: ICommandManager,
@inject(IsWebExtension) isWebExtension: boolean,
@inject(IExtensions) extensions: IExtensions,
@inject(IJupyterUriProviderRegistration) jupyterUriProviderRegistration: IJupyterUriProviderRegistration,
Expand All @@ -60,7 +59,6 @@ export class DataScienceErrorHandlerNode extends DataScienceErrorHandler {
workspaceService,
serverUriStorage,
jupyterUriProviderRegistration,
commandManager,
isWebExtension,
extensions,
fs,
Expand Down
9 changes: 1 addition & 8 deletions src/kernels/errors/kernelErrorHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { KernelConnectionTimeoutError } from './kernelConnectionTimeoutError';
import { KernelDiedError } from './kernelDiedError';
import { KernelPortNotUsedTimeoutError } from './kernelPortNotUsedTimeoutError';
import { KernelProcessExitedError } from './kernelProcessExitedError';
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../platform/common/application/types';
import { IApplicationShell, IWorkspaceService } from '../../platform/common/application/types';
import { traceError, traceWarning } from '../../platform/logging';
import {
IBrowserService,
Expand Down Expand Up @@ -88,7 +88,6 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
@inject(IJupyterServerUriStorage) private readonly serverUriStorage: IJupyterServerUriStorage,
@inject(IJupyterUriProviderRegistration)
private readonly jupyterUriProviderRegistration: IJupyterUriProviderRegistration,
@inject(ICommandManager) private readonly commandManager: ICommandManager,
@inject(IsWebExtension) private readonly isWebExtension: boolean,
@inject(IExtensions) private readonly extensions: IExtensions,
@inject(IFileSystem) private readonly fs: IFileSystem,
Expand Down Expand Up @@ -414,12 +413,6 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
return KernelInterpreterDependencyResponse.cancel;
}
case DataScience.changeRemoteJupyterConnectionButtonText: {
await this.commandManager.executeCommand(
Commands.SelectJupyterURI,
true,
'errorHandler',
undefined
);
return KernelInterpreterDependencyResponse.cancel;
}
case DataScience.selectDifferentKernel: {
Expand Down
7 changes: 0 additions & 7 deletions src/kernels/errors/kernelErrorHandler.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import { getDisplayNameOrNameOfKernelConnection } from '../helpers';
import { getOSType, OSType } from '../../platform/common/utils/platform';
import { RemoteJupyterServerConnectionError } from '../../platform/errors/remoteJupyterServerConnectionError';
import { computeServerId, generateUriFromRemoteProvider } from '../jupyter/jupyterUtils';
import { Commands } from '../../platform/common/constants';
import { RemoteJupyterServerUriProviderError } from './remoteJupyterServerUriProviderError';
import { IReservedPythonNamedProvider } from '../../platform/interpreter/types';
import { DataScienceErrorHandlerNode } from './kernelErrorHandler.node';
Expand Down Expand Up @@ -96,7 +95,6 @@ suite('Error Handler Unit Tests', () => {
instance(kernelDependencyInstaller),
instance(workspaceService),
instance(uriStorage),
instance(cmdManager),
false,
instance(extensions),
instance(jupyterUriProviderRegistration),
Expand Down Expand Up @@ -818,7 +816,6 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
DataScience.selectDifferentKernel
)
).once();
verify(cmdManager.executeCommand(Commands.SelectJupyterURI, true, 'errorHandler', undefined)).never();
verify(uriStorage.removeUri(uri)).never();
});
test('Display error when connection to remote jupyter server fails due to 3rd party extension', async () => {
Expand Down Expand Up @@ -858,7 +855,6 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
DataScience.selectDifferentKernel
)
).once();
verify(cmdManager.executeCommand(Commands.SelectJupyterURI, true, 'errorHandler', undefined)).never();
verify(uriStorage.removeUri(uri)).never();
});
test('Remove remote Uri if user choses to do so, when connection to remote jupyter server fails', async () => {
Expand Down Expand Up @@ -892,7 +888,6 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.cancel);
verify(cmdManager.executeCommand(Commands.SelectJupyterURI, true, 'errorHandler', undefined)).never();
verify(uriStorage.removeUri(uri)).once();
verify(uriStorage.getSavedUriList()).atLeast(1);
});
Expand Down Expand Up @@ -924,7 +919,6 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.cancel);
verify(cmdManager.executeCommand(Commands.SelectJupyterURI, true, 'errorHandler', undefined)).once();
verify(uriStorage.removeUri(uri)).never();
});
test('Select different kernel user choses to do so, when connection to remote jupyter server fails', async () => {
Expand Down Expand Up @@ -954,7 +948,6 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.selectDifferentKernel);
verify(cmdManager.executeCommand(Commands.SelectJupyterURI, true, 'errorHandler', undefined)).never();
verify(uriStorage.removeUri(uri)).never();
});
function verifyErrorMessage(message: string, linkInfo?: string) {
Expand Down
12 changes: 2 additions & 10 deletions src/kernels/jupyter/connection/serverSelector.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@
import { anything, capture, instance, mock, verify } from 'ts-mockito';
import { Uri } from 'vscode';
import { CommandManager } from '../../../platform/common/application/commandManager';
import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../../platform/common/application/types';
import { ICommandManager } from '../../../platform/common/application/types';
import { JupyterServerSelector } from './serverSelector';
import { Commands } from '../../../platform/common/constants';
import { JupyterServerSelectorCommand } from './serverSelectorCommand';
import { JupyterServerUriStorage } from './serverUriStorage';
import { IBrowserService } from '../../../platform/common/types';

/* eslint-disable */
suite('Server Selector Command', () => {
Expand All @@ -20,18 +19,12 @@ suite('Server Selector Command', () => {
setup(() => {
commandManager = mock(CommandManager);
serverSelector = mock(JupyterServerSelector);
const appShell = mock<IApplicationShell>();
const browser = mock<IBrowserService>();
const notebook = mock<IVSCodeNotebook>();
const uriStorage = mock(JupyterServerUriStorage);

serverSelectorCommand = new JupyterServerSelectorCommand(
instance(commandManager),
instance(serverSelector),
instance(uriStorage),
instance(notebook),
instance(appShell),
instance(browser)
instance(uriStorage)
);
});

Expand All @@ -48,7 +41,6 @@ suite('Server Selector Command', () => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const handler = (capture(commandManager.registerCommand as any).first()[1] as Function).bind(
serverSelectorCommand,
false,
uri
);

Expand Down
36 changes: 5 additions & 31 deletions src/kernels/jupyter/connection/serverSelectorCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,13 @@

import { inject, injectable } from 'inversify';
import { Uri } from 'vscode';
import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../../platform/common/application/types';
import { ICommandManager } from '../../../platform/common/application/types';
import { Commands } from '../../../platform/common/constants';
import { IBrowserService, IDisposable } from '../../../platform/common/types';
import { IDisposable } from '../../../platform/common/types';
import { traceInfo } from '../../../platform/logging';
import { JupyterServerSelector, SelectJupyterUriCommandSource } from './serverSelector';
import { JupyterServerSelector } from './serverSelector';
import { IJupyterServerUriStorage } from '../types';
import { IExtensionSyncActivationService } from '../../../platform/activation/types';
import { noop } from '../../../platform/common/utils/misc';
import { Common, DataScience } from '../../../platform/common/utils/localize';

/**
* Registers commands to allow the user to set the remote server URI.
Expand All @@ -22,10 +20,7 @@ export class JupyterServerSelectorCommand implements IExtensionSyncActivationSer
constructor(
@inject(ICommandManager) private readonly commandManager: ICommandManager,
@inject(JupyterServerSelector) private readonly serverSelector: JupyterServerSelector,
@inject(IJupyterServerUriStorage) private readonly serverUriStorage: IJupyterServerUriStorage,
@inject(IVSCodeNotebook) private readonly notebook: IVSCodeNotebook,
@inject(IApplicationShell) private readonly appShell: IApplicationShell,
@inject(IBrowserService) private readonly browser: IBrowserService
@inject(IJupyterServerUriStorage) private readonly serverUriStorage: IJupyterServerUriStorage
) {}
public activate() {
this.disposables.push(
Expand All @@ -39,34 +34,13 @@ export class JupyterServerSelectorCommand implements IExtensionSyncActivationSer
this.disposables.forEach((d) => d.dispose());
}

private async selectJupyterUri(
_local: boolean = true, // Legacy - 3rd party extenions may use this
source: Uri | SelectJupyterUriCommandSource = 'commandPalette'
): Promise<void> {
private async selectJupyterUri(source: Uri): Promise<void> {
if (source instanceof Uri) {
traceInfo(`Setting Jupyter Server URI to remote: ${source}`);

// Set the uri directly
await this.serverSelector.setJupyterURIToRemote(source.toString(true));
} else {
this.appShell
.showErrorMessage(DataScience.enterRemoteJupyterUrlsThroughTheKernelPicker, Common.documentation)
.then((selection) => {
if (selection === Common.documentation) {
this.browser.launch(
'https://code.visualstudio.com/docs/datascience/jupyter-kernel-management#_existing-jupyter-server'
);
}
}, noop);
if (this.notebook.activeNotebookEditor) {
await this.commandManager.executeCommand('notebook.selectKernel', {
notebookEditor: this.notebook.activeNotebookEditor
});
}
}

// Picking the 'preferred' kernel for remote should happen in the command handler from the
// kernel picker, not from the command palette command
}

private async clearJupyterUris(): Promise<void> {
Expand Down
6 changes: 3 additions & 3 deletions src/test/common.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,21 +220,21 @@ export function initializeCommonNodeApi() {
}
return { file: Uri.file(tempFile), dispose: () => swallowExceptions(() => fs.unlinkSync(tempFile)) };
},
async startJupyterServer(notebook?: NotebookDocument, useCert: boolean = false): Promise<any> {
async startJupyterServer(_notebook?: NotebookDocument, useCert: boolean = false): Promise<any> {
if (IS_REMOTE_NATIVE_TEST()) {
const uriString = useCert
? await JupyterServer.instance.startJupyterWithCert()
: await JupyterServer.instance.startJupyterWithToken();
console.info(`Jupyter started and listening at ${uriString}`);
try {
await commands.executeCommand('jupyter.selectjupyteruri', false, Uri.parse(uriString), notebook);
await commands.executeCommand('jupyter.selectjupyteruri', Uri.parse(uriString));
} catch (ex) {
console.error('Failed to select jupyter server, retry in 1s', ex);
}
// Todo: Fix in debt week, we need to retry, some changes have caused the first connection attempt to fail on CI.
// Possible we're trying to connect before the server is ready.
await sleep(5_000);
await commands.executeCommand('jupyter.selectjupyteruri', false, Uri.parse(uriString), notebook);
await commands.executeCommand('jupyter.selectjupyteruri', Uri.parse(uriString));
} else {
console.info(`Jupyter not started and set to local`); // This is the default
}
Expand Down