Skip to content
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

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

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 2 additions & 2 deletions src/client/common/process/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions src/client/datascience/activation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ 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));
this.testSavedInterpreter();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: not sure on the placement here. Maybe at the same time as PreWarmDaemonPool?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to just make it part of the getSelectedInterpreter. But if I do that, I'm not sure if we are gaining anything from saving it, since we still would verify the first time that we run it and get the slowdown.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you also get into a problem here if auto start is turned on? Auto start would use the saved interpreter before you reset it?


In reply to: 379103857 [](ancestors = 379103857)

Copy link

@rchiodo rchiodo Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe have the auto start stuff wait for the check too?


In reply to: 379116829 [](ancestors = 379116829,379103857)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I feel like this currently doesn't guarantee a good interpreter. But making things wait for it would reduce / remove the benefit of saving a good server I would think.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the auto start case. Normal startup should not wait.


In reply to: 379118437 [](ancestors = 379118437)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it makes sense to have it in activate

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rchiodo . How about this. The saved interpreter check is a promise that we save (like ensureNotebook) and we fire it off on activate right away. Then getSelectedInterpreter always awaits on that promise before returning its result. This way we ensure that getSelectedInterpreter is returning something valid for all the notebook launch cases as well as stuff like preWarmVariables and the auto start server. If I do this, by the time the user is actually starting a notebook it should have completed, since it launched on activate so we still get the benefit of the saved server.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me 👍

await this.contextService.activate();
}

Expand All @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<void> {
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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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);
Expand Down