From 34d36c1de1e9b5b79e31dea9806d804ba4ecc9aa Mon Sep 17 00:00:00 2001 From: Cory Rivera Date: Wed, 13 Mar 2019 18:44:54 -0700 Subject: [PATCH] Prompt for Python installation after choosing a Jupyter kernel in notebook (#4453) --- .../src/dialog/configurePythonDialog.ts | 51 ++++++++++++------- .../notebookIntegration.test.ts | 2 +- .../notebook/src/jupyter/jupyterController.ts | 42 ++++----------- .../src/jupyter/jupyterServerInstallation.ts | 31 ++++++++--- .../src/jupyter/jupyterServerManager.ts | 7 +-- .../src/test/model/serverManager.test.ts | 27 +++++----- 6 files changed, 86 insertions(+), 74 deletions(-) diff --git a/extensions/notebook/src/dialog/configurePythonDialog.ts b/extensions/notebook/src/dialog/configurePythonDialog.ts index 539430050a..7bc2c89451 100644 --- a/extensions/notebook/src/dialog/configurePythonDialog.ts +++ b/extensions/notebook/src/dialog/configurePythonDialog.ts @@ -11,8 +11,9 @@ import * as azdata from 'azdata'; import * as fs from 'fs'; import * as utils from '../common/utils'; -import { AppContext } from '../common/appContext'; import JupyterServerInstallation from '../jupyter/jupyterServerInstallation'; +import { ApiWrapper } from '../common/apiWrapper'; +import { Deferred } from '../common/promise'; const localize = nls.loadMessageBundle(); @@ -31,27 +32,44 @@ export class ConfigurePythonDialog { private pythonLocationTextBox: azdata.InputBoxComponent; private browseButton: azdata.ButtonComponent; - constructor(private appContext: AppContext, private outputChannel: vscode.OutputChannel, private jupyterInstallation: JupyterServerInstallation) { + private _setupComplete: Deferred; + + constructor(private apiWrapper: ApiWrapper, private outputChannel: vscode.OutputChannel, private jupyterInstallation: JupyterServerInstallation) { + this._setupComplete = new Deferred(); } - public async showDialog() { + /** + * Opens a dialog to configure python installation for notebooks. + * @param rejectOnCancel Specifies whether an error should be thrown after clicking Cancel. + * @returns A promise that is resolved when the python installation completes. + */ + public showDialog(rejectOnCancel: boolean = false): Promise { this.dialog = azdata.window.createModelViewDialog(this.DialogTitle); this.initializeContent(); this.dialog.okButton.label = this.OkButtonText; this.dialog.cancelButton.label = this.CancelButtonText; + this.dialog.cancelButton.onClick(() => { + if (rejectOnCancel) { + this._setupComplete.reject(localize('pythonInstallDeclined', 'Python installation was declined.')); + } else { + this._setupComplete.resolve(); + } + }); this.dialog.registerCloseValidator(() => this.handleInstall()); azdata.window.openDialog(this.dialog); + + return this._setupComplete.promise; } - private initializeContent() { + private initializeContent(): void { this.dialog.registerContent(async view => { this.pythonLocationTextBox = view.modelBuilder.inputBox() .withProperties({ - value: JupyterServerInstallation.getPythonInstallPath(this.appContext.apiWrapper), + value: JupyterServerInstallation.getPythonInstallPath(this.apiWrapper), width: '100%' }).component(); @@ -106,14 +124,18 @@ export class ConfigurePythonDialog { return false; } } catch (err) { - this.appContext.apiWrapper.showErrorMessage(utils.getErrorMessage(err)); + this.apiWrapper.showErrorMessage(utils.getErrorMessage(err)); return false; } // Don't wait on installation, since there's currently no Cancel functionality - this.jupyterInstallation.startInstallProcess(pythonLocation).catch(err => { - this.appContext.apiWrapper.showErrorMessage(utils.getErrorMessage(err)); - }); + this.jupyterInstallation.startInstallProcess(pythonLocation) + .then(() => { + this._setupComplete.resolve(); + }) + .catch(err => { + this._setupComplete.reject(utils.getErrorMessage(err)); + }); return true; } @@ -149,20 +171,13 @@ export class ConfigurePythonDialog { openLabel: this.SelectFileLabel }; - let fileUris: vscode.Uri[] = await this.appContext.apiWrapper.showOpenDialog(options); + let fileUris: vscode.Uri[] = await this.apiWrapper.showOpenDialog(options); if (fileUris && fileUris[0]) { this.pythonLocationTextBox.value = fileUris[0].fsPath; } } - private showInfoMessage(message: string) { - this.dialog.message = { - text: message, - level: azdata.window.MessageLevel.Information - }; - } - - private showErrorMessage(message: string) { + private showErrorMessage(message: string): void { this.dialog.message = { text: message, level: azdata.window.MessageLevel.Error diff --git a/extensions/notebook/src/integrationTest/notebookIntegration.test.ts b/extensions/notebook/src/integrationTest/notebookIntegration.test.ts index bd9d5d7f47..d55aadb14a 100644 --- a/extensions/notebook/src/integrationTest/notebookIntegration.test.ts +++ b/extensions/notebook/src/integrationTest/notebookIntegration.test.ts @@ -127,6 +127,6 @@ function writeNotebookToFile(pythonNotebook: INotebook): vscode.Uri { async function ensureJupyterInstalled(): Promise { let jupterControllerExports = vscode.extensions.getExtension('Microsoft.sql-vnext').exports; let jupyterController = jupterControllerExports.getJupterController() as JupyterController; - await jupyterController.jupyterInstallation; + await jupyterController.jupyterInstallation.installReady; } diff --git a/extensions/notebook/src/jupyter/jupyterController.ts b/extensions/notebook/src/jupyter/jupyterController.ts index 860702bce0..75a95ce7ba 100644 --- a/extensions/notebook/src/jupyter/jupyterController.ts +++ b/extensions/notebook/src/jupyter/jupyterController.ts @@ -30,7 +30,7 @@ import CodeAdapter from '../prompts/adapter'; let untitledCounter = 0; export class JupyterController implements vscode.Disposable { - private _jupyterInstallation: Promise; + private _jupyterInstallation: JupyterServerInstallation; private _notebookInstances: IServerInstance[] = []; private outputChannel: vscode.OutputChannel; @@ -55,31 +55,11 @@ export class JupyterController implements vscode.Disposable { // PUBLIC METHODS ////////////////////////////////////////////////////// public async activate(): Promise { - // Prompt for install if the python installation path is not defined - let jupyterInstaller = new JupyterServerInstallation( + this._jupyterInstallation = new JupyterServerInstallation( this.extensionContext.extensionPath, this.outputChannel, this.apiWrapper); - if (JupyterServerInstallation.isPythonInstalled(this.apiWrapper)) { - this._jupyterInstallation = Promise.resolve(jupyterInstaller); - } else { - this._jupyterInstallation = new Promise(resolve => { - jupyterInstaller.onInstallComplete(err => { - if (!err) { - resolve(jupyterInstaller); - } - }); - }); - } - let notebookProvider = undefined; - - notebookProvider = this.registerNotebookProvider(); - azdata.nb.onDidOpenNotebookDocument(notebook => { - if (!JupyterServerInstallation.isPythonInstalled(this.apiWrapper)) { - this.doConfigurePython(jupyterInstaller); - } - }); // Add command/task handlers this.apiWrapper.registerTaskHandler(constants.jupyterOpenNotebookTask, (profile: azdata.IConnectionProfile) => { return this.handleOpenNotebookTask(profile); @@ -96,11 +76,12 @@ export class JupyterController implements vscode.Disposable { this.apiWrapper.registerCommand(constants.jupyterReinstallDependenciesCommand, () => { return this.handleDependenciesReinstallation(); }); this.apiWrapper.registerCommand(constants.jupyterInstallPackages, () => { return this.doManagePackages(); }); - this.apiWrapper.registerCommand(constants.jupyterConfigurePython, () => { return this.doConfigurePython(jupyterInstaller); }); + this.apiWrapper.registerCommand(constants.jupyterConfigurePython, () => { return this.doConfigurePython(this._jupyterInstallation); }); let supportedFileFilter: vscode.DocumentFilter[] = [ { scheme: 'untitled', language: '*' } ]; + let notebookProvider = this.registerNotebookProvider(); this.extensionContext.subscriptions.push(this.apiWrapper.registerCompletionItemProvider(supportedFileFilter, new NotebookCompletionItemProvider(notebookProvider))); return true; @@ -195,7 +176,7 @@ export class JupyterController implements vscode.Disposable { private async handleDependenciesReinstallation(): Promise { if (await this.confirmReinstall()) { - this._jupyterInstallation = JupyterServerInstallation.getInstallation( + this._jupyterInstallation = await JupyterServerInstallation.getInstallation( this.extensionContext.extensionPath, this.outputChannel, this.apiWrapper, @@ -225,14 +206,11 @@ export class JupyterController implements vscode.Disposable { } } - public async doConfigurePython(jupyterInstaller: JupyterServerInstallation): Promise { - try { - let pythonDialog = new ConfigurePythonDialog(this.appContext, this.outputChannel, jupyterInstaller); - await pythonDialog.showDialog(); - } catch (error) { - let message = utils.getErrorMessage(error); - this.apiWrapper.showErrorMessage(message); - } + public doConfigurePython(jupyterInstaller: JupyterServerInstallation): void { + let pythonDialog = new ConfigurePythonDialog(this.apiWrapper, this.outputChannel, jupyterInstaller); + pythonDialog.showDialog().catch(err => { + this.apiWrapper.showErrorMessage(utils.getErrorMessage(err)); + }); } public getTextToSendToTerminal(shellType: any): string { diff --git a/extensions/notebook/src/jupyter/jupyterServerInstallation.ts b/extensions/notebook/src/jupyter/jupyterServerInstallation.ts index cc3a1a5d08..95fa6d672c 100644 --- a/extensions/notebook/src/jupyter/jupyterServerInstallation.ts +++ b/extensions/notebook/src/jupyter/jupyterServerInstallation.ts @@ -16,7 +16,9 @@ import * as request from 'request'; import { ApiWrapper } from '../common/apiWrapper'; import * as constants from '../common/constants'; import * as utils from '../common/utils'; -import { OutputChannel, ConfigurationTarget, Event, EventEmitter, window } from 'vscode'; +import { OutputChannel, ConfigurationTarget, window } from 'vscode'; +import { Deferred } from '../common/promise'; +import { ConfigurePythonDialog } from '../dialog/configurePythonDialog'; const localize = nls.loadMessageBundle(); const msgPythonInstallationProgress = localize('msgPythonInstallationProgress', 'Python installation is in progress'); @@ -53,7 +55,7 @@ export default class JupyterServerInstallation { private static readonly DefaultPythonLocation = path.join(utils.getUserHome(), 'azuredatastudio-python'); - private _installCompleteEmitter = new EventEmitter(); + private _installReady: Deferred; constructor(extensionPath: string, outputChannel: OutputChannel, apiWrapper: ApiWrapper, pythonInstallationPath?: string, forceInstall?: boolean) { this.extensionPath = extensionPath; @@ -63,10 +65,15 @@ export default class JupyterServerInstallation { this._forceInstall = !!forceInstall; this.configurePackagePaths(); + + this._installReady = new Deferred(); + if (JupyterServerInstallation.isPythonInstalled(this.apiWrapper)) { + this._installReady.resolve(); + } } - public get onInstallComplete(): Event { - return this._installCompleteEmitter.event; + public get installReady(): Promise { + return this._installReady.promise; } public static async getInstallation( @@ -232,7 +239,7 @@ export default class JupyterServerInstallation { }; } - public async startInstallProcess(pythonInstallationPath?: string): Promise { + public startInstallProcess(pythonInstallationPath?: string): Promise { if (pythonInstallationPath) { this._pythonInstallationPath = pythonInstallationPath; this.configurePackagePaths(); @@ -249,23 +256,31 @@ export default class JupyterServerInstallation { operation: op => { this.installDependencies(op) .then(() => { - this._installCompleteEmitter.fire(); + this._installReady.resolve(); updateConfig(); }) .catch(err => { let errorMsg = msgDependenciesInstallationFailed(err); op.updateStatus(azdata.TaskStatus.Failed, errorMsg); this.apiWrapper.showErrorMessage(errorMsg); - this._installCompleteEmitter.fire(errorMsg); + this._installReady.reject(errorMsg); }); } }); } else { // Python executable already exists, but the path setting wasn't defined, // so update it here - this._installCompleteEmitter.fire(); + this._installReady.resolve(); updateConfig(); } + return this._installReady.promise; + } + + public async promptForPythonInstall(): Promise { + if (!JupyterServerInstallation.isPythonInstalled(this.apiWrapper)) { + let pythonDialog = new ConfigurePythonDialog(this.apiWrapper, this.outputChannel, this); + return pythonDialog.showDialog(true); + } } private async installJupyterProsePackage(): Promise { diff --git a/extensions/notebook/src/jupyter/jupyterServerManager.ts b/extensions/notebook/src/jupyter/jupyterServerManager.ts index 6cbfd41824..f5ae01455c 100644 --- a/extensions/notebook/src/jupyter/jupyterServerManager.ts +++ b/extensions/notebook/src/jupyter/jupyterServerManager.ts @@ -20,7 +20,7 @@ import { PerNotebookServerInstance, IInstanceOptions } from './serverInstance'; export interface IServerManagerOptions { documentPath: string; - jupyterInstallation: Promise; + jupyterInstallation: JupyterServerInstallation; extensionContext: vscode.ExtensionContext; apiWrapper?: ApiWrapper; factory?: ServerInstanceFactory; @@ -62,7 +62,7 @@ export class LocalJupyterServerManager implements nb.ServerManager, vscode.Dispo this._onServerStarted.fire(); } catch (error) { - this.apiWrapper.showErrorMessage(localize('startServerFailed', 'Starting local Notebook server failed with error {0}', utils.getErrorMessage(error))); + this.apiWrapper.showErrorMessage(localize('startServerFailed', 'Starting local Notebook server failed with error: {0}', utils.getErrorMessage(error))); throw error; } } @@ -102,7 +102,8 @@ export class LocalJupyterServerManager implements nb.ServerManager, vscode.Dispo } private async doStartServer(): Promise { // We can't find or create servers until the installation is complete - let installation = await this.options.jupyterInstallation; + let installation = this.options.jupyterInstallation; + await installation.promptForPythonInstall(); // Calculate the path to use as the notebook-dir for Jupyter based on the path of the uri of the // notebook to open. This will be the workspace folder if the notebook uri is inside a workspace diff --git a/extensions/notebook/src/test/model/serverManager.test.ts b/extensions/notebook/src/test/model/serverManager.test.ts index 3f5ab6eae1..4c88fc8d8c 100644 --- a/extensions/notebook/src/test/model/serverManager.test.ts +++ b/extensions/notebook/src/test/model/serverManager.test.ts @@ -23,7 +23,7 @@ import { MockExtensionContext } from '../common/stubs'; describe('Local Jupyter Server Manager', function (): void { let expectedPath = 'my/notebook.ipynb'; let serverManager: LocalJupyterServerManager; - let deferredInstall: Deferred; + let deferredInstall: Deferred; let mockApiWrapper: TypeMoq.IMock; let mockExtensionContext: MockExtensionContext; let mockFactory: TypeMoq.IMock; @@ -33,10 +33,14 @@ describe('Local Jupyter Server Manager', function (): void { mockApiWrapper.setup(a => a.showErrorMessage(TypeMoq.It.isAny())); mockApiWrapper.setup(a => a.getWorkspacePathFromUri(TypeMoq.It.isAny())).returns(() => undefined); mockFactory = TypeMoq.Mock.ofType(ServerInstanceFactory); - deferredInstall = new Deferred(); + + deferredInstall = new Deferred(); + let mockInstall = TypeMoq.Mock.ofType(JupyterServerInstallation, undefined, undefined, '/root'); + mockInstall.setup(j => j.promptForPythonInstall()).returns(() => deferredInstall.promise); + serverManager = new LocalJupyterServerManager({ documentPath: expectedPath, - jupyterInstallation: deferredInstall.promise, + jupyterInstallation: mockInstall.object, extensionContext: mockExtensionContext, apiWrapper: mockApiWrapper.object, factory: mockFactory.object @@ -58,8 +62,8 @@ describe('Local Jupyter Server Manager', function (): void { it('Should configure and start install', async function (): Promise { // Given an install and instance that start with no issues let expectedUri = vscode.Uri.parse('http://localhost:1234?token=abcdefghijk'); - let [mockInstall, mockServerInstance] = initInstallAndInstance(expectedUri); - deferredInstall.resolve(mockInstall.object); + let mockServerInstance = initInstallAndInstance(expectedUri); + deferredInstall.resolve(); // When I start the server let notified = false; @@ -83,9 +87,9 @@ describe('Local Jupyter Server Manager', function (): void { it('Should call stop on server instance', async function (): Promise { // Given an install and instance that start with no issues let expectedUri = vscode.Uri.parse('http://localhost:1234?token=abcdefghijk'); - let [mockInstall, mockServerInstance] = initInstallAndInstance(expectedUri); + let mockServerInstance = initInstallAndInstance(expectedUri); mockServerInstance.setup(s => s.stop()).returns(() => Promise.resolve()); - deferredInstall.resolve(mockInstall.object); + deferredInstall.resolve(); // When I start and then the server await serverManager.startServer(); @@ -98,9 +102,9 @@ describe('Local Jupyter Server Manager', function (): void { it('Should call stop when extension is disposed', async function (): Promise { // Given an install and instance that start with no issues let expectedUri = vscode.Uri.parse('http://localhost:1234?token=abcdefghijk'); - let [mockInstall, mockServerInstance] = initInstallAndInstance(expectedUri); + let mockServerInstance = initInstallAndInstance(expectedUri); mockServerInstance.setup(s => s.stop()).returns(() => Promise.resolve()); - deferredInstall.resolve(mockInstall.object); + deferredInstall.resolve(); // When I start and then dispose the extension await serverManager.startServer(); @@ -111,13 +115,12 @@ describe('Local Jupyter Server Manager', function (): void { mockServerInstance.verify(s => s.stop(), TypeMoq.Times.once()); }); - function initInstallAndInstance(uri: vscode.Uri): [TypeMoq.IMock, TypeMoq.IMock] { - let mockInstall = TypeMoq.Mock.ofType(JupyterServerInstallation, undefined, undefined, '/root'); + function initInstallAndInstance(uri: vscode.Uri): TypeMoq.IMock { let mockServerInstance = TypeMoq.Mock.ofType(JupyterServerInstanceStub); mockFactory.setup(f => f.createInstance(TypeMoq.It.isAny())).returns(() => mockServerInstance.object); mockServerInstance.setup(s => s.configure()).returns(() => Promise.resolve()); mockServerInstance.setup(s => s.start()).returns(() => Promise.resolve()); mockServerInstance.setup(s => s.uri).returns(() => uri); - return [mockInstall, mockServerInstance]; + return mockServerInstance; } });