From 5ec1a992e15dd3de8eda07b7de80945cc45f4632 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Thu, 13 Feb 2020 12:31:30 -0800 Subject: [PATCH 1/5] initial idea --- src/client/common/process/types.ts | 4 +-- src/client/datascience/activation.ts | 6 ++++ .../interpreter/jupyterInterpreterService.ts | 31 +++++++++++++++++++ .../jupyterInterpreterStateStore.ts | 7 ++--- 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/client/common/process/types.ts b/src/client/common/process/types.ts index 7e85afd69945..5dd9bbc28716 100644 --- a/src/client/common/process/types.ts +++ b/src/client/common/process/types.ts @@ -113,8 +113,8 @@ export interface IPythonExecutionFactory { create(options: ExecutionFactoryCreationOptions): Promise; /** * Creates a daemon Python Process. - * On windows its cheapter to create a daemon and use that than spin up Python Processes everytime. - * If something cannot be executed within the daemin, it will resort to using the stanard IPythonExecutionService. + * On windows it's cheaper to create a daemon and use that than spin up Python Processes everytime. + * If something cannot be executed within the daemon, it will resort to using the standard IPythonExecutionService. * Note: The returned execution service is always using an activated environment. * * @param {ExecutionFactoryCreationOptions} options diff --git a/src/client/datascience/activation.ts b/src/client/datascience/activation.ts index 4ff5b35c8d7c..e23f2f6d40ea 100644 --- a/src/client/datascience/activation.ts +++ b/src/client/datascience/activation.ts @@ -28,6 +28,7 @@ export class Activation implements IExtensionSingleActivationService { public async activate(): Promise { this.disposables.push(this.notebookProvider.onDidOpenNotebookEditor(this.onDidOpenNotebookEditor, this)); this.disposables.push(this.jupyterInterpreterService.onDidChangeInterpreter(this.onDidChangeInterpreter, this)); + this.testSavedInterpreter(); await this.contextService.activate(); } @@ -43,6 +44,11 @@ export class Activation implements IExtensionSingleActivationService { } } + private testSavedInterpreter(): void { + // Do we have a saved interpreter? If so check it out to see if it's still valid + this.jupyterInterpreterService.checkSavedInterpreter().ignoreErrors(); + } + @debounceAsync(500) @swallowExceptions('Failed to pre-warm daemon pool') private async PreWarmDaemonPool() { diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts index 042e931ca3d3..fc19f6c13be5 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts @@ -8,6 +8,7 @@ import { Event, EventEmitter } from 'vscode'; import { CancellationToken } from 'vscode-jsonrpc'; import { createPromiseFromCancellation } from '../../../common/cancellation'; import '../../../common/extensions'; +import { traceInfo } from '../../../common/logger'; import { IInterpreterService, PythonInterpreter } from '../../../interpreter/contracts'; import { sendTelemetryEvent } from '../../../telemetry'; import { Telemetry } from '../../constants'; @@ -89,6 +90,36 @@ export class JupyterInterpreterService { } return interpreterDetails; } + + // Verify if a saved global interpreter is still valid for use + // If not, then clear it out + public async checkSavedInterpreter(): Promise { + if (!this.interpreterSelectionState.selectedPythonPath) { + // None set yet, so no need to check + return; + } + + try { + const interpreterDetails = await this.interpreterService.getInterpreterDetails( + this.interpreterSelectionState.selectedPythonPath, + undefined + ); + + if (interpreterDetails) { + if (await this.interpreterConfiguration.areDependenciesInstalled(interpreterDetails, undefined)) { + // Our saved interpreter was found and has dependencies installed + return; + } + } + } catch (_err) { + traceInfo('Saved Jupyter interpreter invalid'); + } + + // At this point we failed some aspect of our checks regarding our saved interpreter, so clear it out + this._selectedInterpreter = undefined; + this.interpreterSelectionState.updateSelectedPythonPath(undefined); + } + /** * Selects and interpreter to run jupyter server. * Validates and configures the interpreter. diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterStateStore.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterStateStore.ts index d4e3585b2c5c..3fecb01848db 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterStateStore.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterStateStore.ts @@ -11,11 +11,11 @@ import { noop } from '../../../common/utils/misc'; const key = 'INTERPRETER_PATH_SELECTED_FOR_JUPYTER_SERVER'; const keySelected = 'INTERPRETER_PATH_WAS_SELECTED_FOR_JUPYTER_SERVER'; /** - * Keeps track of whether the user ever selected an interpreter to be used as the gloabl jupyter interpreter. + * Keeps track of whether the user ever selected an interpreter to be used as the global jupyter interpreter. * Keeps track of the interpreter path of the interpreter used as the global jupyter interpreter. * * @export - * @class JupyterInterpreterFinderEverSet + * @class JupyterInterpreterStateStore */ @injectable() export class JupyterInterpreterStateStore { @@ -27,7 +27,6 @@ export class JupyterInterpreterStateStore { * * @readonly * @type {Promise} - * @memberof JupyterInterpreterFinderEverSet */ public get interpreterSetAtleastOnce(): boolean { return !!this.selectedPythonPath || this.memento.get(keySelected, false); @@ -35,7 +34,7 @@ export class JupyterInterpreterStateStore { public get selectedPythonPath(): string | undefined { return this._interpreterPath || this.memento.get(key, undefined); } - public updateSelectedPythonPath(value: string) { + public updateSelectedPythonPath(value: string | undefined) { this._interpreterPath = value; this.memento.update(key, value).then(noop, noop); this.memento.update(keySelected, true).then(noop, noop); From 8bc139a026b22d3bcb5ed9783c7a6006b056e183 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Thu, 13 Feb 2020 14:18:52 -0800 Subject: [PATCH 2/5] refactor to save check promise --- src/client/datascience/activation.ts | 7 +-- .../interpreter/jupyterInterpreterService.ts | 48 ++++++++++++++++++- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/client/datascience/activation.ts b/src/client/datascience/activation.ts index e23f2f6d40ea..691f5f7edf95 100644 --- a/src/client/datascience/activation.ts +++ b/src/client/datascience/activation.ts @@ -28,7 +28,7 @@ export class Activation implements IExtensionSingleActivationService { public async activate(): Promise { this.disposables.push(this.notebookProvider.onDidOpenNotebookEditor(this.onDidOpenNotebookEditor, this)); this.disposables.push(this.jupyterInterpreterService.onDidChangeInterpreter(this.onDidChangeInterpreter, this)); - this.testSavedInterpreter(); + this.jupyterInterpreterService.validateSavedInterpreter().ignoreErrors(); await this.contextService.activate(); } @@ -44,11 +44,6 @@ export class Activation implements IExtensionSingleActivationService { } } - private testSavedInterpreter(): void { - // Do we have a saved interpreter? If so check it out to see if it's still valid - this.jupyterInterpreterService.checkSavedInterpreter().ignoreErrors(); - } - @debounceAsync(500) @swallowExceptions('Failed to pre-warm daemon pool') private async PreWarmDaemonPool() { diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts index fc19f6c13be5..3de678d68f98 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts @@ -25,6 +25,7 @@ export class JupyterInterpreterService { private _selectedInterpreter?: PythonInterpreter; private _selectedInterpreterPath?: string; private _onDidChangeInterpreter = new EventEmitter(); + private validateSavedInterpreterPromise: Promise | undefined; public get onDidChangeInterpreter(): Event { return this._onDidChangeInterpreter.event; } @@ -38,6 +39,13 @@ export class JupyterInterpreterService { private readonly interpreterConfiguration: JupyterInterpreterDependencyService, @inject(IInterpreterService) private readonly interpreterService: IInterpreterService ) {} + public async validateSavedInterpreter(): Promise { + if (!this.validateSavedInterpreterPromise) { + this.validateSavedInterpreterPromise = this.validateSavedInterpreterImpl(); + } + + return this.validateSavedInterpreterPromise; + } /** * Gets the selected interpreter configured to run Jupyter. * @@ -65,7 +73,17 @@ export class JupyterInterpreterService { return interpreter; } - const pythonPath = this._selectedInterpreterPath || this.interpreterSelectionState.selectedPythonPath; + let pythonPath = this._selectedInterpreterPath; + + if (!pythonPath && this.interpreterSelectionState.selectedPythonPath) { + // On activate we kick off a check to see if the saved interpreter is still valid + // make sure that has completed before we actually use it as a valid interpreter + if (await this.validateSavedInterpreter()) { + pythonPath = this.interpreterSelectionState.selectedPythonPath; + } + } + + // If nothing saved, then check our current interpreter to see if we can use it if (!pythonPath) { // Check if current interpreter has all of the required dependencies. // If yes, then use that. @@ -183,4 +201,32 @@ export class JupyterInterpreterService { this.interpreterSelectionState.updateSelectedPythonPath((this._selectedInterpreterPath = interpreter.path)); sendTelemetryEvent(Telemetry.SelectJupyterInterpreter, undefined, { result: 'selected' }); } + + private async validateSavedInterpreterImpl(): Promise { + if (!this.interpreterSelectionState.selectedPythonPath) { + // None set yet, so no need to check + return false; + } + + try { + const interpreterDetails = await this.interpreterService.getInterpreterDetails( + this.interpreterSelectionState.selectedPythonPath, + undefined + ); + + if (interpreterDetails) { + if (await this.interpreterConfiguration.areDependenciesInstalled(interpreterDetails, undefined)) { + // Our saved interpreter was found and has dependencies installed + return true; + } + } + } catch (_err) { + traceInfo('Saved Jupyter interpreter invalid'); + } + + // At this point we failed some aspect of our checks regarding our saved interpreter, so clear it out + this._selectedInterpreter = undefined; + this.interpreterSelectionState.updateSelectedPythonPath(undefined); + return false; + } } From b25c387605f3b6c44dfc85e8b0e4759849df1d56 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Thu, 13 Feb 2020 16:28:49 -0800 Subject: [PATCH 3/5] revamp of initial loading --- src/client/datascience/activation.ts | 3 +- .../interpreter/jupyterInterpreterService.ts | 199 +++++++----------- 2 files changed, 79 insertions(+), 123 deletions(-) diff --git a/src/client/datascience/activation.ts b/src/client/datascience/activation.ts index 691f5f7edf95..7e73b10dd1a3 100644 --- a/src/client/datascience/activation.ts +++ b/src/client/datascience/activation.ts @@ -28,7 +28,8 @@ export class Activation implements IExtensionSingleActivationService { public async activate(): Promise { this.disposables.push(this.notebookProvider.onDidOpenNotebookEditor(this.onDidOpenNotebookEditor, this)); this.disposables.push(this.jupyterInterpreterService.onDidChangeInterpreter(this.onDidChangeInterpreter, this)); - this.jupyterInterpreterService.validateSavedInterpreter().ignoreErrors(); + // Warm up our selected interpreter for the extension + this.jupyterInterpreterService.setInitialInterpreter().ignoreErrors(); await this.contextService.activate(); } diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts index 3de678d68f98..ca6cb23d55e0 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts @@ -8,7 +8,6 @@ import { Event, EventEmitter } from 'vscode'; import { CancellationToken } from 'vscode-jsonrpc'; import { createPromiseFromCancellation } from '../../../common/cancellation'; import '../../../common/extensions'; -import { traceInfo } from '../../../common/logger'; import { IInterpreterService, PythonInterpreter } from '../../../interpreter/contracts'; import { sendTelemetryEvent } from '../../../telemetry'; import { Telemetry } from '../../constants'; @@ -23,9 +22,8 @@ import { JupyterInterpreterStateStore } from './jupyterInterpreterStateStore'; @injectable() export class JupyterInterpreterService { private _selectedInterpreter?: PythonInterpreter; - private _selectedInterpreterPath?: string; private _onDidChangeInterpreter = new EventEmitter(); - private validateSavedInterpreterPromise: Promise | undefined; + private setInitialInterpreterPromise: Promise | undefined; public get onDidChangeInterpreter(): Event { return this._onDidChangeInterpreter.event; } @@ -39,13 +37,6 @@ export class JupyterInterpreterService { private readonly interpreterConfiguration: JupyterInterpreterDependencyService, @inject(IInterpreterService) private readonly interpreterService: IInterpreterService ) {} - public async validateSavedInterpreter(): Promise { - if (!this.validateSavedInterpreterPromise) { - this.validateSavedInterpreterPromise = this.validateSavedInterpreterImpl(); - } - - return this.validateSavedInterpreterPromise; - } /** * Gets the selected interpreter configured to run Jupyter. * @@ -54,88 +45,20 @@ export class JupyterInterpreterService { * @memberof JupyterInterpreterService */ public async getSelectedInterpreter(token?: CancellationToken): Promise { - if (this._selectedInterpreter) { - return this._selectedInterpreter; - } - - const resolveToUndefinedWhenCancelled = createPromiseFromCancellation({ - cancelAction: 'resolve', - defaultValue: undefined, - token - }); - // For backwards compatiblity check if we have a cached interpreter (older version of extension). - // If that interpreter has everything we need then use that. - let interpreter = await Promise.race([ - this.getInterpreterFromChangeOfOlderVersionOfExtension(), - resolveToUndefinedWhenCancelled - ]); - if (interpreter) { - return interpreter; - } - - let pythonPath = this._selectedInterpreterPath; + // Before we return _selected interpreter make sure that we have run our initial set interpreter once + await this.setInitialInterpreter(token); - if (!pythonPath && this.interpreterSelectionState.selectedPythonPath) { - // On activate we kick off a check to see if the saved interpreter is still valid - // make sure that has completed before we actually use it as a valid interpreter - if (await this.validateSavedInterpreter()) { - pythonPath = this.interpreterSelectionState.selectedPythonPath; - } - } - - // If nothing saved, then check our current interpreter to see if we can use it - if (!pythonPath) { - // Check if current interpreter has all of the required dependencies. - // If yes, then use that. - interpreter = await this.interpreterService.getActiveInterpreter(undefined); - if (!interpreter) { - return; - } - // Use this interpreter going forward. - if (await this.interpreterConfiguration.areDependenciesInstalled(interpreter)) { - this.setAsSelectedInterpreter(interpreter); - return interpreter; - } - return; - } - - const interpreterDetails = await Promise.race([ - this.interpreterService.getInterpreterDetails(pythonPath, undefined), - resolveToUndefinedWhenCancelled - ]); - if (interpreterDetails) { - this._selectedInterpreter = interpreterDetails; - } - return interpreterDetails; + return this._selectedInterpreter; } - // Verify if a saved global interpreter is still valid for use - // If not, then clear it out - public async checkSavedInterpreter(): Promise { - if (!this.interpreterSelectionState.selectedPythonPath) { - // None set yet, so no need to check - return; - } - - try { - const interpreterDetails = await this.interpreterService.getInterpreterDetails( - this.interpreterSelectionState.selectedPythonPath, - undefined - ); - - if (interpreterDetails) { - if (await this.interpreterConfiguration.areDependenciesInstalled(interpreterDetails, undefined)) { - // Our saved interpreter was found and has dependencies installed - return; - } - } - } catch (_err) { - traceInfo('Saved Jupyter interpreter invalid'); + // To be run one initial time. Check our saved locations and then current interpreter to try to start off + // with a valid jupyter interpreter + public async setInitialInterpreter(token?: CancellationToken): Promise { + if (!this.setInitialInterpreterPromise) { + this.setInitialInterpreterPromise = this.setInitialInterpreterImpl(token); } - // At this point we failed some aspect of our checks regarding our saved interpreter, so clear it out - this._selectedInterpreter = undefined; - this.interpreterSelectionState.updateSelectedPythonPath(undefined); + return this.setInitialInterpreterPromise; } /** @@ -175,58 +98,90 @@ export class JupyterInterpreterService { return this.selectInterpreter(token); } } - private async getInterpreterFromChangeOfOlderVersionOfExtension(): Promise { + + // Check the location that we stored jupyter launch path in the old version + // if it's there, return it and clear the location + private getInterpreterFromChangeOfOlderVersionOfExtension(): string | undefined { const pythonPath = this.oldVersionCacheStateStore.getCachedInterpreterPath(); if (!pythonPath) { return; } - try { - const interpreter = await this.interpreterService.getInterpreterDetails(pythonPath, undefined); - if (!interpreter) { - return; - } - if (await this.interpreterConfiguration.areDependenciesInstalled(interpreter)) { - this.setAsSelectedInterpreter(interpreter); - return interpreter; - } - // If dependencies are not installed, then ignore it. lets continue with the current logic. - } finally { - // Don't perform this check again, just clear the cache. - this.oldVersionCacheStateStore.clearCache().ignoreErrors(); - } + + // Clear the cache to not check again + this.oldVersionCacheStateStore.clearCache().ignoreErrors(); + return pythonPath; } + + // Set the specified interpreter as our current selected interpreter private setAsSelectedInterpreter(interpreter: PythonInterpreter): void { this._selectedInterpreter = interpreter; this._onDidChangeInterpreter.fire(interpreter); - this.interpreterSelectionState.updateSelectedPythonPath((this._selectedInterpreterPath = interpreter.path)); + this.interpreterSelectionState.updateSelectedPythonPath(interpreter.path); sendTelemetryEvent(Telemetry.SelectJupyterInterpreter, undefined, { result: 'selected' }); } - private async validateSavedInterpreterImpl(): Promise { - if (!this.interpreterSelectionState.selectedPythonPath) { - // None set yet, so no need to check - return false; + // For a given python path check if it can run jupyter for us + // if so, return the interpreter + private async validateInterpreterPath( + pythonPath: string, + token?: CancellationToken + ): Promise { + const resolveToUndefinedWhenCancelled = createPromiseFromCancellation({ + cancelAction: 'resolve', + defaultValue: undefined, + token + }); + + // First see if we can get interpreter details + const interpreter = await Promise.race([ + this.interpreterService.getInterpreterDetails(pythonPath, undefined), + resolveToUndefinedWhenCancelled + ]); + if (interpreter) { + // Then check that dependencies are installed + if (await this.interpreterConfiguration.areDependenciesInstalled(interpreter)) { + return interpreter; + } + } + return undefined; + } + + private async setInitialInterpreterImpl(token?: CancellationToken): Promise { + let interpreter: PythonInterpreter | undefined; + + // Check the old version location first, we will clear it if we find it here + const oldVersionPythonPath = this.getInterpreterFromChangeOfOlderVersionOfExtension(); + if (oldVersionPythonPath) { + interpreter = await this.validateInterpreterPath(oldVersionPythonPath, token); } - try { - const interpreterDetails = await this.interpreterService.getInterpreterDetails( - this.interpreterSelectionState.selectedPythonPath, - undefined - ); + // Next check the saved global path + if (!interpreter && this.interpreterSelectionState.selectedPythonPath) { + interpreter = await this.validateInterpreterPath(this.interpreterSelectionState.selectedPythonPath, token); - if (interpreterDetails) { - if (await this.interpreterConfiguration.areDependenciesInstalled(interpreterDetails, undefined)) { - // Our saved interpreter was found and has dependencies installed - return true; + // If we had a global path, but it's not valid, trash it + if (!interpreter) { + this.interpreterSelectionState.updateSelectedPythonPath(undefined); + } + } + + // Nothing saved found, so check our current interpreter + if (!interpreter) { + const currentInterpreter = await this.interpreterService.getActiveInterpreter(undefined); + + if (currentInterpreter) { + // Ask and give a chance to install dependencies in current interpreter + if (await this.interpreterConfiguration.areDependenciesInstalled(currentInterpreter)) { + interpreter = currentInterpreter; } } - } catch (_err) { - traceInfo('Saved Jupyter interpreter invalid'); } - // At this point we failed some aspect of our checks regarding our saved interpreter, so clear it out - this._selectedInterpreter = undefined; - this.interpreterSelectionState.updateSelectedPythonPath(undefined); - return false; + // Set ourselves as a valid interpreter + if (interpreter) { + this.setAsSelectedInterpreter(interpreter); + } + + return interpreter; } } From f0eb2be39c695af2259a77158124bd55c5d30241 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Fri, 14 Feb 2020 09:02:38 -0800 Subject: [PATCH 4/5] unit tests added --- .../interpreter/jupyterInterpreterService.ts | 40 +++++++++++-------- src/test/datascience/activation.unit.test.ts | 1 + .../jupyterInterpreterService.unit.test.ts | 30 +++++++++++++- 3 files changed, 54 insertions(+), 17 deletions(-) diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts index ca6cb23d55e0..f425d146d88c 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts @@ -8,6 +8,7 @@ import { Event, EventEmitter } from 'vscode'; import { CancellationToken } from 'vscode-jsonrpc'; import { createPromiseFromCancellation } from '../../../common/cancellation'; import '../../../common/extensions'; +import { noop } from '../../../common/utils/misc'; import { IInterpreterService, PythonInterpreter } from '../../../interpreter/contracts'; import { sendTelemetryEvent } from '../../../telemetry'; import { Telemetry } from '../../constants'; @@ -88,6 +89,8 @@ export class JupyterInterpreterService { const result = await this.interpreterConfiguration.installMissingDependencies(interpreter, undefined, token); switch (result) { case JupyterInterpreterDependencyResponse.ok: { + // Make sure that we have set our initial path before the user sets it + await this.setInitialInterpreter(token); this.setAsSelectedInterpreter(interpreter); return interpreter; } @@ -126,22 +129,27 @@ export class JupyterInterpreterService { pythonPath: string, token?: CancellationToken ): Promise { - const resolveToUndefinedWhenCancelled = createPromiseFromCancellation({ - cancelAction: 'resolve', - defaultValue: undefined, - token - }); - - // First see if we can get interpreter details - const interpreter = await Promise.race([ - this.interpreterService.getInterpreterDetails(pythonPath, undefined), - resolveToUndefinedWhenCancelled - ]); - if (interpreter) { - // Then check that dependencies are installed - if (await this.interpreterConfiguration.areDependenciesInstalled(interpreter)) { - return interpreter; + try { + const resolveToUndefinedWhenCancelled = createPromiseFromCancellation({ + cancelAction: 'resolve', + defaultValue: undefined, + token + }); + + // First see if we can get interpreter details + const interpreter = await Promise.race([ + this.interpreterService.getInterpreterDetails(pythonPath, undefined), + resolveToUndefinedWhenCancelled + ]); + if (interpreter) { + // Then check that dependencies are installed + if (await this.interpreterConfiguration.areDependenciesInstalled(interpreter, token)) { + return interpreter; + } } + } catch (_err) { + // For any errors we are ok with just returning undefined for an invalid interpreter + noop(); } return undefined; } @@ -171,7 +179,7 @@ export class JupyterInterpreterService { if (currentInterpreter) { // Ask and give a chance to install dependencies in current interpreter - if (await this.interpreterConfiguration.areDependenciesInstalled(currentInterpreter)) { + if (await this.interpreterConfiguration.areDependenciesInstalled(currentInterpreter, token)) { interpreter = currentInterpreter; } } diff --git a/src/test/datascience/activation.unit.test.ts b/src/test/datascience/activation.unit.test.ts index 03cc7f3d166e..d7d996ef258a 100644 --- a/src/test/datascience/activation.unit.test.ts +++ b/src/test/datascience/activation.unit.test.ts @@ -53,6 +53,7 @@ suite('Data Science - Activation', () => { ); when(jupyterInterpreterService.getSelectedInterpreter()).thenResolve(interpreter); when(jupyterInterpreterService.getSelectedInterpreter(anything())).thenResolve(interpreter); + when(jupyterInterpreterService.setInitialInterpreter()).thenResolve(interpreter); await activator.activate(); }); teardown(() => fakeTimer.uninstall()); diff --git a/src/test/datascience/jupyter/interpreter/jupyterInterpreterService.unit.test.ts b/src/test/datascience/jupyter/interpreter/jupyterInterpreterService.unit.test.ts index 84d9d4e98398..4573149a357f 100644 --- a/src/test/datascience/jupyter/interpreter/jupyterInterpreterService.unit.test.ts +++ b/src/test/datascience/jupyter/interpreter/jupyterInterpreterService.unit.test.ts @@ -4,7 +4,7 @@ 'use strict'; import { assert } from 'chai'; -import { anything, instance, mock, verify, when } from 'ts-mockito'; +import { anyString, anything, instance, mock, verify, when } from 'ts-mockito'; import { Memento } from 'vscode'; import { Architecture } from '../../../../client/common/utils/platform'; import { @@ -122,4 +122,32 @@ suite('Data Science - Jupyter Interpreter Service', () => { assert.equal(selectedInterpreter, secondPythonInterpreter); }); + test('setInitialInterpreter if older version is set should use and clear', async () => { + when(oldVersionCacheStateStore.getCachedInterpreterPath()).thenReturn(pythonInterpreter.path); + when(oldVersionCacheStateStore.clearCache()).thenResolve(); + when(interpreterConfiguration.areDependenciesInstalled(pythonInterpreter, anything())).thenResolve(true); + const initialInterpreter = await jupyterInterpreterService.setInitialInterpreter(undefined); + verify(oldVersionCacheStateStore.clearCache()).once(); + assert.equal(initialInterpreter, pythonInterpreter); + }); + test('setInitialInterpreter use saved interpreter if valid', async () => { + when(oldVersionCacheStateStore.getCachedInterpreterPath()).thenReturn(undefined); + when(interpreterSelectionState.selectedPythonPath).thenReturn(pythonInterpreter.path); + when(interpreterConfiguration.areDependenciesInstalled(pythonInterpreter, anything())).thenResolve(true); + const initialInterpreter = await jupyterInterpreterService.setInitialInterpreter(undefined); + assert.equal(initialInterpreter, pythonInterpreter); + }); + test('setInitialInterpreter saved interpreter invalid, clear it and use active interpreter', async () => { + when(oldVersionCacheStateStore.getCachedInterpreterPath()).thenReturn(undefined); + when(interpreterSelectionState.selectedPythonPath).thenReturn(secondPythonInterpreter.path); + when(interpreterConfiguration.areDependenciesInstalled(secondPythonInterpreter, anything())).thenResolve(false); + when(interpreterService.getActiveInterpreter(anything())).thenResolve(pythonInterpreter); + when(interpreterConfiguration.areDependenciesInstalled(pythonInterpreter, anything())).thenResolve(true); + const initialInterpreter = await jupyterInterpreterService.setInitialInterpreter(undefined); + assert.equal(initialInterpreter, pythonInterpreter); + // Make sure we set our saved interpreter to the new active interpreter + // it should have been cleared to undefined, then set to a new value + verify(interpreterSelectionState.updateSelectedPythonPath(undefined)).once(); + verify(interpreterSelectionState.updateSelectedPythonPath(anyString())).once(); + }); }); From 331e7f7ee54c247d3f19cf1d3761b173cf1d154d Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Fri, 14 Feb 2020 11:21:32 -0800 Subject: [PATCH 5/5] slight refactor for setting active jupyter interpreter --- .../interpreter/jupyterInterpreterService.ts | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts index f425d146d88c..d12d56d02749 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts @@ -24,7 +24,7 @@ import { JupyterInterpreterStateStore } from './jupyterInterpreterStateStore'; export class JupyterInterpreterService { private _selectedInterpreter?: PythonInterpreter; private _onDidChangeInterpreter = new EventEmitter(); - private setInitialInterpreterPromise: Promise | undefined; + private getInitialInterpreterPromise: Promise | undefined; public get onDidChangeInterpreter(): Event { return this._onDidChangeInterpreter.event; } @@ -47,6 +47,8 @@ export class JupyterInterpreterService { */ public async getSelectedInterpreter(token?: CancellationToken): Promise { // Before we return _selected interpreter make sure that we have run our initial set interpreter once + // because _selectedInterpreter can be changed by other function and at other times, this promise + // is cached to only run once await this.setInitialInterpreter(token); return this._selectedInterpreter; @@ -55,11 +57,17 @@ export class JupyterInterpreterService { // To be run one initial time. Check our saved locations and then current interpreter to try to start off // with a valid jupyter interpreter public async setInitialInterpreter(token?: CancellationToken): Promise { - if (!this.setInitialInterpreterPromise) { - this.setInitialInterpreterPromise = this.setInitialInterpreterImpl(token); + if (!this.getInitialInterpreterPromise) { + this.getInitialInterpreterPromise = this.getInitialInterpreterImpl(token).then(result => { + // Set ourselves as a valid interpreter if we found something + if (result) { + this.changeSelectedInterpreterProperty(result); + } + return result; + }); } - return this.setInitialInterpreterPromise; + return this.getInitialInterpreterPromise; } /** @@ -89,9 +97,7 @@ export class JupyterInterpreterService { const result = await this.interpreterConfiguration.installMissingDependencies(interpreter, undefined, token); switch (result) { case JupyterInterpreterDependencyResponse.ok: { - // Make sure that we have set our initial path before the user sets it - await this.setInitialInterpreter(token); - this.setAsSelectedInterpreter(interpreter); + await this.setAsSelectedInterpreter(interpreter); return interpreter; } case JupyterInterpreterDependencyResponse.cancel: @@ -116,7 +122,14 @@ export class JupyterInterpreterService { } // Set the specified interpreter as our current selected interpreter - private setAsSelectedInterpreter(interpreter: PythonInterpreter): void { + private async setAsSelectedInterpreter(interpreter: PythonInterpreter): Promise { + // Make sure that our initial set has happened before we allow a set so that + // calculation of the initial interpreter doesn't clobber the existing one + await this.setInitialInterpreter(); + this.changeSelectedInterpreterProperty(interpreter); + } + + private changeSelectedInterpreterProperty(interpreter: PythonInterpreter) { this._selectedInterpreter = interpreter; this._onDidChangeInterpreter.fire(interpreter); this.interpreterSelectionState.updateSelectedPythonPath(interpreter.path); @@ -154,7 +167,7 @@ export class JupyterInterpreterService { return undefined; } - private async setInitialInterpreterImpl(token?: CancellationToken): Promise { + private async getInitialInterpreterImpl(token?: CancellationToken): Promise { let interpreter: PythonInterpreter | undefined; // Check the old version location first, we will clear it if we find it here @@ -185,11 +198,6 @@ export class JupyterInterpreterService { } } - // Set ourselves as a valid interpreter - if (interpreter) { - this.setAsSelectedInterpreter(interpreter); - } - return interpreter; } }