Skip to content

Commit 8bf0209

Browse files
IanMatthewHuffIanMatthewHuff
authored
Check our saved jupyter interpreters before allowing any of them to be used as active interpreters (#10113)
1 parent 25f5af3 commit 8bf0209

File tree

6 files changed

+142
-64
lines changed

6 files changed

+142
-64
lines changed

‎src/client/common/process/types.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,8 @@ export interface IPythonExecutionFactory {
113113
create(options: ExecutionFactoryCreationOptions): Promise<IPythonExecutionService>;
114114
/**
115115
* Creates a daemon Python Process.
116-
* On windows its cheapter to create a daemon and use that than spin up Python Processes everytime.
117-
* If something cannot be executed within the daemin, it will resort to using the stanard IPythonExecutionService.
116+
* On windows it's cheaper to create a daemon and use that than spin up Python Processes everytime.
117+
* If something cannot be executed within the daemon, it will resort to using the standard IPythonExecutionService.
118118
* Note: The returned execution service is always using an activated environment.
119119
*
120120
* @param {ExecutionFactoryCreationOptions} options

‎src/client/datascience/activation.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ export class Activation implements IExtensionSingleActivationService {
2828
public async activate(): Promise<void> {
2929
this.disposables.push(this.notebookProvider.onDidOpenNotebookEditor(this.onDidOpenNotebookEditor, this));
3030
this.disposables.push(this.jupyterInterpreterService.onDidChangeInterpreter(this.onDidChangeInterpreter, this));
31+
// Warm up our selected interpreter for the extension
32+
this.jupyterInterpreterService.setInitialInterpreter().ignoreErrors();
3133
await this.contextService.activate();
3234
}
3335

‎src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts

Lines changed: 105 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { Event, EventEmitter } from 'vscode';
88
import { CancellationToken } from 'vscode-jsonrpc';
99
import { createPromiseFromCancellation } from '../../../common/cancellation';
1010
import '../../../common/extensions';
11+
import { noop } from '../../../common/utils/misc';
1112
import { IInterpreterService, PythonInterpreter } from '../../../interpreter/contracts';
1213
import { sendTelemetryEvent } from '../../../telemetry';
1314
import { Telemetry } from '../../constants';
@@ -22,8 +23,8 @@ import { JupyterInterpreterStateStore } from './jupyterInterpreterStateStore';
2223
@injectable()
2324
export class JupyterInterpreterService {
2425
private _selectedInterpreter?: PythonInterpreter;
25-
private _selectedInterpreterPath?: string;
2626
private _onDidChangeInterpreter = new EventEmitter<PythonInterpreter>();
27+
private getInitialInterpreterPromise: Promise<PythonInterpreter | undefined> | undefined;
2728
public get onDidChangeInterpreter(): Event<PythonInterpreter> {
2829
return this._onDidChangeInterpreter.event;
2930
}
@@ -45,50 +46,30 @@ export class JupyterInterpreterService {
4546
* @memberof JupyterInterpreterService
4647
*/
4748
public async getSelectedInterpreter(token?: CancellationToken): Promise<PythonInterpreter | undefined> {
48-
if (this._selectedInterpreter) {
49-
return this._selectedInterpreter;
50-
}
49+
// Before we return _selected interpreter make sure that we have run our initial set interpreter once
50+
// because _selectedInterpreter can be changed by other function and at other times, this promise
51+
// is cached to only run once
52+
await this.setInitialInterpreter(token);
5153

52-
const resolveToUndefinedWhenCancelled = createPromiseFromCancellation({
53-
cancelAction: 'resolve',
54-
defaultValue: undefined,
55-
token
56-
});
57-
// For backwards compatiblity check if we have a cached interpreter (older version of extension).
58-
// If that interpreter has everything we need then use that.
59-
let interpreter = await Promise.race([
60-
this.getInterpreterFromChangeOfOlderVersionOfExtension(),
61-
resolveToUndefinedWhenCancelled
62-
]);
63-
if (interpreter) {
64-
return interpreter;
65-
}
54+
return this._selectedInterpreter;
55+
}
6656

67-
const pythonPath = this._selectedInterpreterPath || this.interpreterSelectionState.selectedPythonPath;
68-
if (!pythonPath) {
69-
// Check if current interpreter has all of the required dependencies.
70-
// If yes, then use that.
71-
interpreter = await this.interpreterService.getActiveInterpreter(undefined);
72-
if (!interpreter) {
73-
return;
74-
}
75-
// Use this interpreter going forward.
76-
if (await this.interpreterConfiguration.areDependenciesInstalled(interpreter)) {
77-
this.setAsSelectedInterpreter(interpreter);
78-
return interpreter;
79-
}
80-
return;
57+
// To be run one initial time. Check our saved locations and then current interpreter to try to start off
58+
// with a valid jupyter interpreter
59+
public async setInitialInterpreter(token?: CancellationToken): Promise<PythonInterpreter | undefined> {
60+
if (!this.getInitialInterpreterPromise) {
61+
this.getInitialInterpreterPromise = this.getInitialInterpreterImpl(token).then(result => {
62+
// Set ourselves as a valid interpreter if we found something
63+
if (result) {
64+
this.changeSelectedInterpreterProperty(result);
65+
}
66+
return result;
67+
});
8168
}
8269

83-
const interpreterDetails = await Promise.race([
84-
this.interpreterService.getInterpreterDetails(pythonPath, undefined),
85-
resolveToUndefinedWhenCancelled
86-
]);
87-
if (interpreterDetails) {
88-
this._selectedInterpreter = interpreterDetails;
89-
}
90-
return interpreterDetails;
70+
return this.getInitialInterpreterPromise;
9171
}
72+
9273
/**
9374
* Selects and interpreter to run jupyter server.
9475
* Validates and configures the interpreter.
@@ -116,7 +97,7 @@ export class JupyterInterpreterService {
11697
const result = await this.interpreterConfiguration.installMissingDependencies(interpreter, undefined, token);
11798
switch (result) {
11899
case JupyterInterpreterDependencyResponse.ok: {
119-
this.setAsSelectedInterpreter(interpreter);
100+
await this.setAsSelectedInterpreter(interpreter);
120101
return interpreter;
121102
}
122103
case JupyterInterpreterDependencyResponse.cancel:
@@ -126,30 +107,97 @@ export class JupyterInterpreterService {
126107
return this.selectInterpreter(token);
127108
}
128109
}
129-
private async getInterpreterFromChangeOfOlderVersionOfExtension(): Promise<PythonInterpreter | undefined> {
110+
111+
// Check the location that we stored jupyter launch path in the old version
112+
// if it's there, return it and clear the location
113+
private getInterpreterFromChangeOfOlderVersionOfExtension(): string | undefined {
130114
const pythonPath = this.oldVersionCacheStateStore.getCachedInterpreterPath();
131115
if (!pythonPath) {
132116
return;
133117
}
118+
119+
// Clear the cache to not check again
120+
this.oldVersionCacheStateStore.clearCache().ignoreErrors();
121+
return pythonPath;
122+
}
123+
124+
// Set the specified interpreter as our current selected interpreter
125+
private async setAsSelectedInterpreter(interpreter: PythonInterpreter): Promise<void> {
126+
// Make sure that our initial set has happened before we allow a set so that
127+
// calculation of the initial interpreter doesn't clobber the existing one
128+
await this.setInitialInterpreter();
129+
this.changeSelectedInterpreterProperty(interpreter);
130+
}
131+
132+
private changeSelectedInterpreterProperty(interpreter: PythonInterpreter) {
133+
this._selectedInterpreter = interpreter;
134+
this._onDidChangeInterpreter.fire(interpreter);
135+
this.interpreterSelectionState.updateSelectedPythonPath(interpreter.path);
136+
sendTelemetryEvent(Telemetry.SelectJupyterInterpreter, undefined, { result: 'selected' });
137+
}
138+
139+
// For a given python path check if it can run jupyter for us
140+
// if so, return the interpreter
141+
private async validateInterpreterPath(
142+
pythonPath: string,
143+
token?: CancellationToken
144+
): Promise<PythonInterpreter | undefined> {
134145
try {
135-
const interpreter = await this.interpreterService.getInterpreterDetails(pythonPath, undefined);
146+
const resolveToUndefinedWhenCancelled = createPromiseFromCancellation({
147+
cancelAction: 'resolve',
148+
defaultValue: undefined,
149+
token
150+
});
151+
152+
// First see if we can get interpreter details
153+
const interpreter = await Promise.race([
154+
this.interpreterService.getInterpreterDetails(pythonPath, undefined),
155+
resolveToUndefinedWhenCancelled
156+
]);
157+
if (interpreter) {
158+
// Then check that dependencies are installed
159+
if (await this.interpreterConfiguration.areDependenciesInstalled(interpreter, token)) {
160+
return interpreter;
161+
}
162+
}
163+
} catch (_err) {
164+
// For any errors we are ok with just returning undefined for an invalid interpreter
165+
noop();
166+
}
167+
return undefined;
168+
}
169+
170+
private async getInitialInterpreterImpl(token?: CancellationToken): Promise<PythonInterpreter | undefined> {
171+
let interpreter: PythonInterpreter | undefined;
172+
173+
// Check the old version location first, we will clear it if we find it here
174+
const oldVersionPythonPath = this.getInterpreterFromChangeOfOlderVersionOfExtension();
175+
if (oldVersionPythonPath) {
176+
interpreter = await this.validateInterpreterPath(oldVersionPythonPath, token);
177+
}
178+
179+
// Next check the saved global path
180+
if (!interpreter && this.interpreterSelectionState.selectedPythonPath) {
181+
interpreter = await this.validateInterpreterPath(this.interpreterSelectionState.selectedPythonPath, token);
182+
183+
// If we had a global path, but it's not valid, trash it
136184
if (!interpreter) {
137-
return;
185+
this.interpreterSelectionState.updateSelectedPythonPath(undefined);
138186
}
139-
if (await this.interpreterConfiguration.areDependenciesInstalled(interpreter)) {
140-
this.setAsSelectedInterpreter(interpreter);
141-
return interpreter;
187+
}
188+
189+
// Nothing saved found, so check our current interpreter
190+
if (!interpreter) {
191+
const currentInterpreter = await this.interpreterService.getActiveInterpreter(undefined);
192+
193+
if (currentInterpreter) {
194+
// Ask and give a chance to install dependencies in current interpreter
195+
if (await this.interpreterConfiguration.areDependenciesInstalled(currentInterpreter, token)) {
196+
interpreter = currentInterpreter;
197+
}
142198
}
143-
// If dependencies are not installed, then ignore it. lets continue with the current logic.
144-
} finally {
145-
// Don't perform this check again, just clear the cache.
146-
this.oldVersionCacheStateStore.clearCache().ignoreErrors();
147199
}
148-
}
149-
private setAsSelectedInterpreter(interpreter: PythonInterpreter): void {
150-
this._selectedInterpreter = interpreter;
151-
this._onDidChangeInterpreter.fire(interpreter);
152-
this.interpreterSelectionState.updateSelectedPythonPath((this._selectedInterpreterPath = interpreter.path));
153-
sendTelemetryEvent(Telemetry.SelectJupyterInterpreter, undefined, { result: 'selected' });
200+
201+
return interpreter;
154202
}
155203
}

‎src/client/datascience/jupyter/interpreter/jupyterInterpreterStateStore.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,11 @@ import { noop } from '../../../common/utils/misc';
1111
const key = 'INTERPRETER_PATH_SELECTED_FOR_JUPYTER_SERVER';
1212
const keySelected = 'INTERPRETER_PATH_WAS_SELECTED_FOR_JUPYTER_SERVER';
1313
/**
14-
* Keeps track of whether the user ever selected an interpreter to be used as the gloabl jupyter interpreter.
14+
* Keeps track of whether the user ever selected an interpreter to be used as the global jupyter interpreter.
1515
* Keeps track of the interpreter path of the interpreter used as the global jupyter interpreter.
1616
*
1717
* @export
18-
* @class JupyterInterpreterFinderEverSet
18+
* @class JupyterInterpreterStateStore
1919
*/
2020
@injectable()
2121
export class JupyterInterpreterStateStore {
@@ -27,15 +27,14 @@ export class JupyterInterpreterStateStore {
2727
*
2828
* @readonly
2929
* @type {Promise<boolean>}
30-
* @memberof JupyterInterpreterFinderEverSet
3130
*/
3231
public get interpreterSetAtleastOnce(): boolean {
3332
return !!this.selectedPythonPath || this.memento.get<boolean>(keySelected, false);
3433
}
3534
public get selectedPythonPath(): string | undefined {
3635
return this._interpreterPath || this.memento.get<string | undefined>(key, undefined);
3736
}
38-
public updateSelectedPythonPath(value: string) {
37+
public updateSelectedPythonPath(value: string | undefined) {
3938
this._interpreterPath = value;
4039
this.memento.update(key, value).then(noop, noop);
4140
this.memento.update(keySelected, true).then(noop, noop);

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ suite('Data Science - Activation', () => {
5353
);
5454
when(jupyterInterpreterService.getSelectedInterpreter()).thenResolve(interpreter);
5555
when(jupyterInterpreterService.getSelectedInterpreter(anything())).thenResolve(interpreter);
56+
when(jupyterInterpreterService.setInitialInterpreter()).thenResolve(interpreter);
5657
await activator.activate();
5758
});
5859
teardown(() => fakeTimer.uninstall());

‎src/test/datascience/jupyter/interpreter/jupyterInterpreterService.unit.test.ts

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
'use strict';
55

66
import { assert } from 'chai';
7-
import { anything, instance, mock, verify, when } from 'ts-mockito';
7+
import { anyString, anything, instance, mock, verify, when } from 'ts-mockito';
88
import { Memento } from 'vscode';
99
import { Architecture } from '../../../../client/common/utils/platform';
1010
import {
@@ -122,4 +122,32 @@ suite('Data Science - Jupyter Interpreter Service', () => {
122122

123123
assert.equal(selectedInterpreter, secondPythonInterpreter);
124124
});
125+
test('setInitialInterpreter if older version is set should use and clear', async () => {
126+
when(oldVersionCacheStateStore.getCachedInterpreterPath()).thenReturn(pythonInterpreter.path);
127+
when(oldVersionCacheStateStore.clearCache()).thenResolve();
128+
when(interpreterConfiguration.areDependenciesInstalled(pythonInterpreter, anything())).thenResolve(true);
129+
const initialInterpreter = await jupyterInterpreterService.setInitialInterpreter(undefined);
130+
verify(oldVersionCacheStateStore.clearCache()).once();
131+
assert.equal(initialInterpreter, pythonInterpreter);
132+
});
133+
test('setInitialInterpreter use saved interpreter if valid', async () => {
134+
when(oldVersionCacheStateStore.getCachedInterpreterPath()).thenReturn(undefined);
135+
when(interpreterSelectionState.selectedPythonPath).thenReturn(pythonInterpreter.path);
136+
when(interpreterConfiguration.areDependenciesInstalled(pythonInterpreter, anything())).thenResolve(true);
137+
const initialInterpreter = await jupyterInterpreterService.setInitialInterpreter(undefined);
138+
assert.equal(initialInterpreter, pythonInterpreter);
139+
});
140+
test('setInitialInterpreter saved interpreter invalid, clear it and use active interpreter', async () => {
141+
when(oldVersionCacheStateStore.getCachedInterpreterPath()).thenReturn(undefined);
142+
when(interpreterSelectionState.selectedPythonPath).thenReturn(secondPythonInterpreter.path);
143+
when(interpreterConfiguration.areDependenciesInstalled(secondPythonInterpreter, anything())).thenResolve(false);
144+
when(interpreterService.getActiveInterpreter(anything())).thenResolve(pythonInterpreter);
145+
when(interpreterConfiguration.areDependenciesInstalled(pythonInterpreter, anything())).thenResolve(true);
146+
const initialInterpreter = await jupyterInterpreterService.setInitialInterpreter(undefined);
147+
assert.equal(initialInterpreter, pythonInterpreter);
148+
// Make sure we set our saved interpreter to the new active interpreter
149+
// it should have been cleared to undefined, then set to a new value
150+
verify(interpreterSelectionState.updateSelectedPythonPath(undefined)).once();
151+
verify(interpreterSelectionState.updateSelectedPythonPath(anyString())).once();
152+
});
125153
});

0 commit comments

Comments
 (0)