Check our saved jupyter interpreters before allowing any of them to be used as active interpreters (#10113)

Author: IanMatthewHuff

src/client/common/process/types.ts

@@ -113,8 +113,8 @@ export interface IPythonExecutionFactory {
     create(options: ExecutionFactoryCreationOptions): Promise<IPythonExecutionService>;
     /**
      * 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

src/client/datascience/activation.ts

@@ -28,6 +28,8 @@ export class Activation implements IExtensionSingleActivationService {
     public async activate(): Promise<void> {
         this.disposables.push(this.notebookProvider.onDidOpenNotebookEditor(this.onDidOpenNotebookEditor, this));
         this.disposables.push(this.jupyterInterpreterService.onDidChangeInterpreter(this.onDidChangeInterpreter, this));
+        // Warm up our selected interpreter for the extension
+        this.jupyterInterpreterService.setInitialInterpreter().ignoreErrors();
         await this.contextService.activate();
     }
 

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';
@@ -19,8 +20,8 @@ import { JupyterInterpreterStateStore } from './jupyterInterpreterStateStore';
 @injectable()
 export class JupyterInterpreterService {
     private _selectedInterpreter?: PythonInterpreter;
-    private _selectedInterpreterPath?: string;
     private _onDidChangeInterpreter = new EventEmitter<PythonInterpreter>();
+    private getInitialInterpreterPromise: Promise<PythonInterpreter | undefined> | undefined;
     public get onDidChangeInterpreter(): Event<PythonInterpreter> {
         return this._onDidChangeInterpreter.event;
     }
@@ -40,40 +41,30 @@ export class JupyterInterpreterService {
      * @memberof JupyterInterpreterService
      */
     public async getSelectedInterpreter(token?: CancellationToken): Promise<PythonInterpreter | undefined> {
-        if (this._selectedInterpreter) {
-            return this._selectedInterpreter;
-        }
+        // 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);
 
-        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;
-        }
+        return this._selectedInterpreter;
+    }
 
-        const pythonPath = this._selectedInterpreterPath || this.interpreterSelectionState.selectedPythonPath;
-        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;
+    // 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<PythonInterpreter | undefined> {
+        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;
+            });
         }
 
-        const interpreterDetails = await Promise.race([this.interpreterService.getInterpreterDetails(pythonPath, undefined), resolveToUndefinedWhenCancelled]);
-        if (interpreterDetails) {
-            this._selectedInterpreter = interpreterDetails;
-        }
-        return interpreterDetails;
+        return this.getInitialInterpreterPromise;
     }
+
     /**
      * Selects and interpreter to run jupyter server.
      * Validates and configures the interpreter.
@@ -94,7 +85,7 @@ export class JupyterInterpreterService {
         const result = await this.interpreterConfiguration.installMissingDependencies(interpreter, undefined, token);
         switch (result) {
             case JupyterInterpreterDependencyResponse.ok: {
-                this.setAsSelectedInterpreter(interpreter);
+                await this.setAsSelectedInterpreter(interpreter);
                 return interpreter;
             }
             case JupyterInterpreterDependencyResponse.cancel:
@@ -104,30 +95,91 @@ export class JupyterInterpreterService {
                 return this.selectInterpreter(token);
         }
     }
-    private async getInterpreterFromChangeOfOlderVersionOfExtension(): Promise<PythonInterpreter | undefined> {
+
+    // 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;
         }
+
+        // Clear the cache to not check again
+        this.oldVersionCacheStateStore.clearCache().ignoreErrors();
+        return pythonPath;
+    }
+
+    // Set the specified interpreter as our current selected interpreter
+    private async setAsSelectedInterpreter(interpreter: PythonInterpreter): Promise<void> {
+        // 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);
+        sendTelemetryEvent(Telemetry.SelectJupyterInterpreter, undefined, { result: 'selected' });
+    }
+
+    // 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<PythonInterpreter | undefined> {
         try {
-            const interpreter = await this.interpreterService.getInterpreterDetails(pythonPath, undefined);
+            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;
+    }
+
+    private async getInitialInterpreterImpl(token?: CancellationToken): Promise<PythonInterpreter | undefined> {
+        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);
+        }
+
+        // Next check the saved global path
+        if (!interpreter && this.interpreterSelectionState.selectedPythonPath) {
+            interpreter = await this.validateInterpreterPath(this.interpreterSelectionState.selectedPythonPath, token);
+
+            // If we had a global path, but it's not valid, trash it
             if (!interpreter) {
-                return;
+                this.interpreterSelectionState.updateSelectedPythonPath(undefined);
             }
-            if (await this.interpreterConfiguration.areDependenciesInstalled(interpreter)) {
-                this.setAsSelectedInterpreter(interpreter);
-                return interpreter;
+        }
+
+        // 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, token)) {
+                    interpreter = currentInterpreter;
+                }
             }
-            // 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();
         }
-    }
-    private setAsSelectedInterpreter(interpreter: PythonInterpreter): void {
-        this._selectedInterpreter = interpreter;
-        this._onDidChangeInterpreter.fire(interpreter);
-        this.interpreterSelectionState.updateSelectedPythonPath((this._selectedInterpreterPath = interpreter.path));
-        sendTelemetryEvent(Telemetry.SelectJupyterInterpreter, undefined, { result: 'selected' });
+
+        return interpreter;
     }
 }

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,15 +27,14 @@ export class JupyterInterpreterStateStore {
      *
      * @readonly
      * @type {Promise<boolean>}
-     * @memberof JupyterInterpreterFinderEverSet
      */
     public get interpreterSetAtleastOnce(): boolean {
         return !!this.selectedPythonPath || this.memento.get<boolean>(keySelected, false);
     }
     public get selectedPythonPath(): string | undefined {
         return this._interpreterPath || this.memento.get<string | undefined>(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);

src/test/datascience/activation.unit.test.ts

@@ -47,6 +47,7 @@ suite('Data Science - Activation', () => {
         activator = new Activation(instance(notebookProvider), instance(jupyterInterpreterService), instance(executionFactory), [], instance(contextService));
         when(jupyterInterpreterService.getSelectedInterpreter()).thenResolve(interpreter);
         when(jupyterInterpreterService.getSelectedInterpreter(anything())).thenResolve(interpreter);
+        when(jupyterInterpreterService.setInitialInterpreter()).thenResolve(interpreter);
         await activator.activate();
     });
     teardown(() => fakeTimer.uninstall());

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 { JupyterInterpreterDependencyResponse, JupyterInterpreterDependencyService } from '../../../../client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService';
@@ -107,4 +107,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();
+    });
 });