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.

Sign up for GitHub

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

Remove startup resolving and interpreter refreh #15261

Merged
merged 3 commits into from
Feb 28, 2024
Merged

Remove startup resolving and interpreter refreh #15261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c42e22
Remove resolvedEnvironments property
DonJayamanne Feb 28, 2024
2ae8492
Stop refreshing & resolving all envs on startup
DonJayamanne Feb 28, 2024
2c3e44a
Merge branch 'main' into removeStartupResolvingAndInterpreterRefreh
DonJayamanne Feb 28, 2024
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
17 changes: 15 additions & 2 deletions src/kernels/raw/finder/localPythonAndRelatedNonPythonKernelSpecFinder.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,12 @@ import {
} from './interpreterKernelSpecFinderHelper.node';
import { getDisplayPath } from '../../../platform/common/platform/fs-paths.node';
import { raceCancellation } from '../../../platform/common/cancellation';
import { getCachedEnvironments, resolvedPythonEnvToJupyterEnv } from '../../../platform/interpreter/helpers';
import {
getCachedEnvironment,
getCachedEnvironments,
resolvedPythonEnvToJupyterEnv
} from '../../../platform/interpreter/helpers';
import { sleep } from '../../../platform/common/utils/async';

type InterpreterId = string;

Expand Down Expand Up @@ -88,7 +93,15 @@ export class LocalPythonAndRelatedNonPythonKernelSpecFinder extends LocalKernelS
this.disposables
);
interpreterService.onDidRemoveInterpreter(
(e) => {
async (e) => {
// This is a farily destructive operation, hence lets wait a few seconds.
// In the past Python extension triggered this even incorrectly
// Lets wait a few seconds and see if the env still exists
await sleep(1_000);
if (getCachedEnvironment(e)) {
return;
}

traceVerbose(`Interpreter removed ${e.id}`);
const deletedKernels: LocalKernelConnectionMetadata[] = [];
this._kernels.forEach((k) => {
Expand Down
225 changes: 24 additions & 201 deletions src/platform/api/pythonApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
Event,
Uri,
ExtensionMode,
CancellationTokenSource,
CancellationToken,
workspace,
extensions,
Expand All @@ -27,7 +26,7 @@ import { getDisplayPath, getFilePath } from '../common/platform/fs-paths';
import { IInterpreterService } from '../interpreter/contracts';
import { areInterpreterPathsSame, getInterpreterHash } from '../pythonEnvironments/info/interpreter';
import { EnvironmentType, PythonEnvironment } from '../pythonEnvironments/info';
import { areObjectsWithUrisTheSame, isUri, noop } from '../common/utils/misc';
import { isUri, noop } from '../common/utils/misc';
import { StopWatch } from '../common/utils/stopWatch';
import { Environment, PythonExtension as PythonExtensionApi, ResolvedEnvironment } from '@vscode/python-extension';
import { PromiseMonitor } from '../common/utils/promises';
Expand All @@ -36,11 +35,10 @@ import { PythonExtensionApiNotExportedError } from '../errors/pythonExtApiNotExp
import { getOSType, OSType } from '../common/utils/platform';
import { SemVer } from 'semver';
import {
getCachedEnvironment,
getCachedVersion,
getEnvironmentType,
getPythonEnvDisplayName,
getPythonEnvironmentName,
isCondaEnvironmentWithoutPython,
resolvedPythonEnvToJupyterEnv,
setPythonApi
} from '../interpreter/helpers';
Expand Down Expand Up @@ -297,7 +295,7 @@ export class InterpreterService implements IInterpreterService {
public onDidRemoveInterpreter = this._onDidRemoveInterpreter.event;
public onDidEnvironmentVariablesChange = this._onDidEnvironmentVariablesChange.event;
private eventHandlerAdded?: boolean;
private interpreterListCachePromise: Promise<PythonEnvironment[]> | undefined = undefined;
// private interpreterListCachePromise: Promise<PythonEnvironment[]> | undefined = undefined;
private apiPromise: Promise<PythonExtensionApi | undefined> | undefined;
private _status: 'refreshing' | 'idle' = 'idle';
public get status() {
Expand All @@ -320,34 +318,24 @@ export class InterpreterService implements IInterpreterService {
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
@inject(IExtensionContext) private readonly context: IExtensionContext
) {
if (this.extensionChecker.isPythonExtensionInstalled) {
if (!this.extensionChecker.isPythonExtensionActive) {
// This event may not fire. It only fires if we're the reason for python extension
// activation. VS code does not fire such an event itself if something else activates
this.apiProvider.onDidActivatePythonExtension(
() => {
this.builtListOfInterpretersAtLeastOnce = false;
this.hookupOnDidChangeInterpreterEvent();
this.buildListOfInterpretersForFirstTime();
},
this,
this.disposables
);
}
if (this.extensionChecker.isPythonExtensionInstalled && !this.extensionChecker.isPythonExtensionActive) {
// This event may not fire. It only fires if we're the reason for python extension
// activation. VS code does not fire such an event itself if something else activates
this.apiProvider.onDidActivatePythonExtension(
() => {
this.hookupOnDidChangeInterpreterEvent();
},
this,
this.disposables
);
}
workspace.onDidChangeWorkspaceFolders(this.onDidChangeWorkspaceFolders, this, disposables);
workspace.onDidGrantWorkspaceTrust(() => this.refreshInterpreters(true), this, this.disposables);
this.disposables.push(this._onDidChangeStatus);
this.disposables.push(this.refreshPromises);
this.disposables.push(this.onResumeEnvDetection);
this.refreshPromises.onStateChange(() => {
this.status = this.refreshPromises.isComplete ? 'idle' : 'refreshing';
});
workspace.onDidGrantWorkspaceTrust(
() => this.populateCachedListOfInterpreters(true).catch(noop),
this,
this.disposables
);
}
public async resolveEnvironment(id: string | Environment): Promise<ResolvedEnvironment | undefined> {
return this.getApi().then((api) => {
Expand All @@ -367,21 +355,6 @@ export class InterpreterService implements IInterpreterService {
this.hookupOnDidChangeInterpreterEvent();
return this.didChangeInterpreters.event;
}
private readonly _interpreters = new Map<string, { resolved: PythonEnvironment }>();
private getInterpretersCancellation?: CancellationTokenSource;
private getInterpreters(): Promise<PythonEnvironment[]> {
this.hookupOnDidChangeInterpreterEvent();
// Cache result as it only changes when the interpreter list changes or we add more workspace folders
if (!this.interpreterListCachePromise) {
this.getInterpretersCancellation?.cancel();
this.getInterpretersCancellation?.dispose();
const cancellation = (this.getInterpretersCancellation = new CancellationTokenSource());
this.interpreterListCachePromise = this.getInterpretersImpl(cancellation.token);
this.interpreterListCachePromise.finally(() => cancellation.dispose()).catch(noop);
this.refreshPromises.push(this.interpreterListCachePromise);
}
return this.interpreterListCachePromise;
}
public async refreshInterpreters(forceRefresh: boolean = false) {
const promise = (async () => {
const api = await this.getApi();
Expand All @@ -390,8 +363,7 @@ export class InterpreterService implements IInterpreterService {
}
try {
await api.environments.refreshEnvironments({ forceRefresh });
this.interpreterListCachePromise = undefined;
await this.getInterpreters();
this.hookupOnDidChangeInterpreterEvent();
traceVerbose(`Refreshed Environments`);
} catch (ex) {
traceError(`Failed to refresh the list of interpreters`);
Expand Down Expand Up @@ -559,18 +531,7 @@ export class InterpreterService implements IInterpreterService {
})
.catch(noop);

if (
!this._interpreters.get(env.id) ||
!areObjectsWithUrisTheSame(resolved, this._interpreters.get(env.id)?.resolved)
) {
// Also update the interpreter details in place, so that old references get the latest details
const info = this._interpreters.get(env.id);
if (info?.resolved) {
Object.assign(info.resolved, resolved);
}
this._interpreters.set(env.id, { resolved });
this.triggerEventIfAllowed('interpretersChangeEvent', resolved);
}
this.triggerEventIfAllowed('interpretersChangeEvent', resolved);
return resolved;
}
}
Expand Down Expand Up @@ -614,130 +575,11 @@ export class InterpreterService implements IInterpreterService {
return this.apiPromise;
}

private onDidChangeWorkspaceFolders() {
this.interpreterListCachePromise = undefined;
}
private populateCachedListOfInterpreters(clearCache?: boolean) {
if (clearCache) {
this.interpreterListCachePromise = undefined;
}
const promise = this.getInterpreters().catch(noop);
this.refreshPromises.push(promise);
// Python extension might completely this promise, however this doesn't mean all of the
// events have been triggered,
// I.e. even after we call refresh the python extension could trigger events indicating that there are more changes to the interpreters.
// Hence wait for at least 2 seconds for these events to complete getting triggered.
// Why 2s and not 5 or why not 1s, there's no real reason, just a guess.
// This promise only improves the discovery of kernels, even without this things work,
// but with this things work better as the kernel discovery knows that Python refresh has finished.
this.refreshPromises.push(promise.then(() => sleep(1_000)));
return promise;
}
private async getInterpretersImpl(
cancelToken: CancellationToken,
recursiveCounter = 0
): Promise<PythonEnvironment[]> {
if (!workspace.isTrusted) {
return [];
}

if (this.extensionChecker.isPythonExtensionInstalled) {
this.builtListOfInterpretersAtLeastOnce = true;
}

const allInterpreters: PythonEnvironment[] = [];
let buildListOfInterpretersAgain = false;
await this.getApi().then(async (api) => {
if (!api || cancelToken.isCancellationRequested) {
return [];
}
let previousListOfInterpreters = api.environments.known.length;
try {
await Promise.all(
api.environments.known.map(async (item) => {
try {
const env = await api.environments.resolveEnvironment(item);
const resolved = this.trackResolvedEnvironment(env);
if (resolved) {
allInterpreters.push(resolved);
} else if (item.executable.uri && item.environment?.type !== EnvironmentType.Conda) {
// Ignore cases where we do not have Uri and its a conda env, as those as conda envs without Python.
traceError(
`Failed to get env details from Python API for ${getDisplayPath(
item.id
)} without an error`
);
}
} catch (ex) {
traceError(`Failed to get env details from Python API for ${getDisplayPath(item.id)}`, ex);
}
})
);
// We have updated the list of environments, trigger a change
// Possible one of the environments was resolve even before this method started.
// E.g. we got active interpreter details, and then we came here.
// At this point the env is already resolved, but we did not trigger a change event.
this.triggerEventIfAllowed('interpretersChangeEvent', undefined);
} catch (ex) {
traceError(`Failed to refresh list of interpreters and get their details`, ex);
}

if (previousListOfInterpreters < api.environments.known.length) {
// this means we haven't completed the first refresh of the list of interpreters.
// We've received yet another set of interpreters.
buildListOfInterpretersAgain = true;
}
});
if (cancelToken.isCancellationRequested) {
return [];
}
if (buildListOfInterpretersAgain && recursiveCounter < 10) {
traceVerbose(
`List of interpreters changed after a while, will need to rebuild it again, counter = ${recursiveCounter}`
);
return this.getInterpretersImpl(cancelToken, recursiveCounter++);
}
traceVerbose(
`Full interpreter list is length: ${allInterpreters.length}, ${allInterpreters
.map(
(item) =>
`${item.id}:${getPythonEnvDisplayName(item)}:${getEnvironmentType(item)}:${getDisplayPath(
item.uri
)}`
)
.join(', ')}`
);
return allInterpreters;
}
private builtListOfInterpretersAtLeastOnce?: boolean;
private buildListOfInterpretersForFirstTime() {
if (this.builtListOfInterpretersAtLeastOnce) {
return;
}
// Get latest interpreter list in the background.
if (this.extensionChecker.isPythonExtensionActive) {
this.builtListOfInterpretersAtLeastOnce = true;
this.populateCachedListOfInterpreters().catch(noop);
}
this.extensionChecker.onPythonExtensionInstallationStatusChanged(
(e) => {
if (e !== 'installed') {
return;
}
if (this.extensionChecker.isPythonExtensionActive) {
this.populateCachedListOfInterpreters().catch(noop);
}
},
this,
this.disposables
);
}
private hookupOnDidChangeInterpreterEvent() {
// Only do this once.
if (this.eventHandlerAdded) {
return;
}
this.buildListOfInterpretersForFirstTime();
this.getApi()
.then((api) => {
if (!this.eventHandlerAdded && api) {
Expand All @@ -753,7 +595,6 @@ export class InterpreterService implements IInterpreterService {
api.environments.onDidChangeActiveEnvironmentPath(
() => {
traceVerbose(`Detected change in Active Python environment via Python API`);
this.interpreterListCachePromise = undefined;
this.workspaceCachedActiveInterpreter.clear();
this.triggerEventIfAllowed('interpreterChangeEvent', undefined);
},
Expand All @@ -765,34 +606,16 @@ export class InterpreterService implements IInterpreterService {
traceVerbose(`Python API env change detected, ${e.type} => '${e.env.id}'`);
// Remove items that are no longer valid.
if (e.type === 'remove') {
this._interpreters.delete(e.env.id);
this.triggerEventIfAllowed('interpreterChangeEvent', undefined);
this.triggerEventIfAllowed('interpretersChangeEvent', undefined);
this._onDidRemoveInterpreter.fire({ id: e.env.id });
return;
}
const info = resolvedPythonEnvToJupyterEnv(getCachedEnvironment(e.env));
if (e.type === 'update' && info) {
this.triggerEventIfAllowed('interpreterChangeEvent', info);
this.triggerEventIfAllowed('interpretersChangeEvent', info);
}
// If this is a conda env that was previously resolved,
// & subsequently updated as having python then trigger changes.
const pythonInstalledIntoConda =
e.type === 'update' &&
isCondaEnvironmentWithoutPython(this._interpreters.get(e.env.id)?.resolved) &&
e.env.executable.uri
? true
: false;
this.populateCachedListOfInterpreters(true)
.finally(() => {
const info = this._interpreters.get(e.env.id);
if (e.type === 'remove' && !info) {
this.triggerEventIfAllowed('interpreterChangeEvent', undefined);
this.triggerEventIfAllowed('interpretersChangeEvent', undefined);
this._onDidRemoveInterpreter.fire({ id: e.env.id });
} else if (
e.type === 'update' &&
info &&
pythonInstalledIntoConda &&
!isCondaEnvironmentWithoutPython(info.resolved)
) {
this.triggerEventIfAllowed('interpreterChangeEvent', info.resolved);
this.triggerEventIfAllowed('interpretersChangeEvent', info.resolved);
}
})
.catch(noop);
},
this,
this.disposables
Expand Down
Loading