From ba6ac95e0204047a884e390f70b28ad73e458766 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 15 Mar 2023 16:30:23 -0700 Subject: [PATCH 1/5] Change resolveEnvironment API to invalidate cache for conda envs without python --- src/client/interpreter/interpreterService.ts | 30 +++++++++++++++---- .../locators/composite/envsCollectionCache.ts | 28 +++++++++++++---- 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/src/client/interpreter/interpreterService.ts b/src/client/interpreter/interpreterService.ts index 2fee9aaec22e..9f386247f4d8 100644 --- a/src/client/interpreter/interpreterService.ts +++ b/src/client/interpreter/interpreterService.ts @@ -9,6 +9,7 @@ import { ProgressLocation, ProgressOptions, Uri, + WorkspaceFolder, } from 'vscode'; import '../common/extensions'; import { IApplicationShell, IDocumentManager, IWorkspaceService } from '../common/application/types'; @@ -96,7 +97,13 @@ export class InterpreterService implements Disposable, IInterpreterService { public async refresh(resource?: Uri): Promise { const interpreterDisplay = this.serviceContainer.get(IInterpreterDisplay); await interpreterDisplay.refresh(resource); - this.ensureEnvironmentContainsPython(this.configService.getSettings(resource).pythonPath).ignoreErrors(); + const workspaceFolder = this.serviceContainer + .get(IWorkspaceService) + .getWorkspaceFolder(resource); + this.ensureEnvironmentContainsPython( + this.configService.getSettings(resource).pythonPath, + workspaceFolder, + ).ignoreErrors(); } public initialize(): void { @@ -227,18 +234,21 @@ export class InterpreterService implements Disposable, IInterpreterService { if (this._pythonPathSetting === '' || this._pythonPathSetting !== pySettings.pythonPath) { this._pythonPathSetting = pySettings.pythonPath; this.didChangeInterpreterEmitter.fire(resource); + const workspaceFolder = this.serviceContainer + .get(IWorkspaceService) + .getWorkspaceFolder(resource); reportActiveInterpreterChanged({ path: pySettings.pythonPath, - resource: this.serviceContainer.get(IWorkspaceService).getWorkspaceFolder(resource), + resource: workspaceFolder, }); const interpreterDisplay = this.serviceContainer.get(IInterpreterDisplay); interpreterDisplay.refresh().catch((ex) => traceError('Python Extension: display.refresh', ex)); - await this.ensureEnvironmentContainsPython(this._pythonPathSetting); + await this.ensureEnvironmentContainsPython(this._pythonPathSetting, workspaceFolder); } } @cache(-1, true) - private async ensureEnvironmentContainsPython(pythonPath: string) { + private async ensureEnvironmentContainsPython(pythonPath: string, workspaceFolder: WorkspaceFolder | undefined) { const installer = this.serviceContainer.get(IInstaller); if (!(await installer.isInstalled(Product.python))) { // If Python is not installed into the environment, install it. @@ -251,7 +261,17 @@ export class InterpreterService implements Disposable, IInterpreterService { traceLog('Conda envs without Python are known to not work well; fixing conda environment...'); const promise = installer.install(Product.python, await this.getInterpreterDetails(pythonPath)); shell.withProgress(progressOptions, () => promise); - promise.then(() => this.triggerRefresh().ignoreErrors()); + promise + .then(async () => { + // Fetch interpreter details so the cache is updated to include the newly installed Python. + await this.getInterpreterDetails(pythonPath); + this.didChangeInterpreterEmitter.fire(workspaceFolder?.uri); + reportActiveInterpreterChanged({ + path: pythonPath, + resource: workspaceFolder, + }); + }) + .ignoreErrors(); } } } diff --git a/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts b/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts index acf7626e6935..c49715fb5c61 100644 --- a/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts +++ b/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts @@ -5,13 +5,14 @@ import { Event } from 'vscode'; import { isTestExecution } from '../../../../common/constants'; import { traceInfo, traceVerbose } from '../../../../logging'; import { arePathsSame, getFileInfo, pathExists } from '../../../common/externalDependencies'; -import { PythonEnvInfo } from '../../info'; +import { PythonEnvInfo, PythonEnvKind } from '../../info'; import { areEnvsDeepEqual, areSameEnv, getEnvPath } from '../../info/env'; import { BasicPythonEnvCollectionChangedEvent, PythonEnvCollectionChangedEvent, PythonEnvsWatcher, } from '../../watcher'; +import { getCondaInterpreterPath } from '../../../common/environmentManagers/conda'; export interface IEnvsCollectionCache { /** @@ -146,15 +147,21 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher areSameEnv(e, env)); + if (!found) { + this.envs.push(env); + this.fire({ new: env }); + } else if (hasLatestInfo) { + const isValidated = this.validatedEnvs.has(env.id!); + if (!isValidated) { + // Update cache only if we have latest info and the env is not already validated. + this.updateEnv(found, env, true); + } + } if (hasLatestInfo) { traceVerbose(`Flushing env to cache ${env.id}`); this.validatedEnvs.add(env.id!); this.flush(env).ignoreErrors(); // If we have latest info, flush it so it can be saved. } - if (!found) { - this.envs.push(env); - this.fire({ new: env }); - } } public updateEnv(oldValue: PythonEnvInfo, newValue: PythonEnvInfo | undefined, forceUpdate = false): void { @@ -177,6 +184,17 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher { // `path` can either be path to environment or executable path const env = this.envs.find((e) => arePathsSame(e.location, path)) ?? this.envs.find((e) => areSameEnv(e, path)); + if ( + env?.kind === PythonEnvKind.Conda && + getEnvPath(env.executable.filename, env.location).pathType === 'envFolderPath' + ) { + if (await pathExists(getCondaInterpreterPath(env.location))) { + // This is a conda env without python in cache which actually now has a valid python, so return + // `undefined` and delete value from cache as cached value is not the latest anymore. + this.validatedEnvs.delete(env.id!); + return undefined; + } + } if (env) { if (this.validatedEnvs.has(env.id!)) { traceVerbose(`Found cached env for ${path}`); From 4330a98fa8fa72d6bafa3dfad931ad24709a61e6 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 15 Mar 2023 16:37:19 -0700 Subject: [PATCH 2/5] Add comments --- src/client/interpreter/interpreterService.ts | 1 + .../base/locators/composite/envsCollectionCache.ts | 9 +++------ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/client/interpreter/interpreterService.ts b/src/client/interpreter/interpreterService.ts index 9f386247f4d8..7bb6f9c2edce 100644 --- a/src/client/interpreter/interpreterService.ts +++ b/src/client/interpreter/interpreterService.ts @@ -265,6 +265,7 @@ export class InterpreterService implements Disposable, IInterpreterService { .then(async () => { // Fetch interpreter details so the cache is updated to include the newly installed Python. await this.getInterpreterDetails(pythonPath); + // Fire an event as the executable for the environment has changed. this.didChangeInterpreterEmitter.fire(workspaceFolder?.uri); reportActiveInterpreterChanged({ path: pythonPath, diff --git a/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts b/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts index c49715fb5c61..f44866c18d97 100644 --- a/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts +++ b/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts @@ -150,12 +150,9 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher Date: Thu, 16 Mar 2023 14:20:50 -0700 Subject: [PATCH 3/5] Add unit tests --- .../locators/composite/envsCollectionCache.ts | 3 + src/test/pythonEnvironments/base/common.ts | 5 +- .../envsCollectionService.unit.test.ts | 93 ++++++++++++++++--- .../base/locators/envTestUtils.ts | 18 +++- 4 files changed, 101 insertions(+), 18 deletions(-) diff --git a/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts b/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts index f44866c18d97..d3ece41f163d 100644 --- a/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts +++ b/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts @@ -191,6 +191,9 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher extends Locator { constructor( private envs: I[], public callbacks: { - resolve?: null | ((env: PythonEnvInfo) => Promise); + resolve?: null | ((env: PythonEnvInfo | string) => Promise); before?(): Promise; after?(): Promise; onUpdated?: Event | ProgressNotificationEvent>; @@ -112,6 +112,7 @@ export class SimpleLocator extends Locator { afterEach?(e: I): Promise; onQuery?(query: PythonLocatorQuery | undefined, envs: I[]): Promise; } = {}, + private options?: { resolveAsString?: boolean }, ) { super(); } @@ -172,7 +173,7 @@ export class SimpleLocator extends Locator { if (this.callbacks?.resolve === null) { return undefined; } - return this.callbacks.resolve(envInfo); + return this.callbacks.resolve(this.options?.resolveAsString ? env : envInfo); } } diff --git a/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts b/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts index ac7157365f02..2470d08dd975 100644 --- a/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts +++ b/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. @@ -22,13 +23,29 @@ import * as externalDependencies from '../../../../../client/pythonEnvironments/ import { noop } from '../../../../core'; import { TEST_LAYOUT_ROOT } from '../../../common/commonTestConstants'; import { SimpleLocator } from '../../common'; -import { assertEnvEqual, assertEnvsEqual } from '../envTestUtils'; +import { assertEnvEqual, assertEnvsEqual, createFile, deleteFile } from '../envTestUtils'; suite('Python envs locator - Environments Collection', async () => { let collectionService: EnvsCollectionService; let storage: PythonEnvInfo[]; const updatedName = 'updatedName'; + const condaEnvWithoutPython = createEnv( + 'python', + undefined, + undefined, + path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython'), + PythonEnvKind.Conda, + path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', 'python.exe'), + ); + const condaEnvWithPython = createEnv( + path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', 'python.exe'), + undefined, + undefined, + path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython'), + PythonEnvKind.Conda, + path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', 'python.exe'), + ); function applyChangeEventToEnvList(envs: PythonEnvInfo[], event: PythonEnvCollectionChangedEvent) { const env = event.old ?? event.new; @@ -49,8 +66,17 @@ suite('Python envs locator - Environments Collection', async () => { return envs; } - function createEnv(executable: string, searchLocation?: Uri, name?: string, location?: string) { - return buildEnvInfo({ executable, searchLocation, name, location }); + function createEnv( + executable: string, + searchLocation?: Uri, + name?: string, + location?: string, + kind?: PythonEnvKind, + id?: string, + ) { + const env = buildEnvInfo({ executable, searchLocation, name, location, kind }); + env.id = id ?? env.id; + return env; } function getLocatorEnvs() { @@ -77,12 +103,7 @@ suite('Python envs locator - Environments Collection', async () => { path.join(TEST_LAYOUT_ROOT, 'pipenv', 'project1', '.venv', 'Scripts', 'python.exe'), Uri.file(TEST_LAYOUT_ROOT), ); - const envCached3 = createEnv( - 'python', - undefined, - undefined, - path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython'), - ); + const envCached3 = condaEnvWithoutPython; return [cachedEnvForWorkspace, envCached1, envCached2, envCached3]; } @@ -123,7 +144,8 @@ suite('Python envs locator - Environments Collection', async () => { collectionService = new EnvsCollectionService(cache, parentLocator); }); - teardown(() => { + teardown(async () => { + await deleteFile(condaEnvWithPython.executable.filename); // Restore to the original state sinon.restore(); }); @@ -404,7 +426,7 @@ suite('Python envs locator - Environments Collection', async () => { env.executable.mtime = 100; sinon.stub(externalDependencies, 'getFileInfo').resolves({ ctime: 100, mtime: 100 }); const parentLocator = new SimpleLocator([], { - resolve: async (e: PythonEnvInfo) => { + resolve: async (e: any) => { if (env.executable.filename === e.executable.filename) { return resolvedViaLocator; } @@ -434,7 +456,7 @@ suite('Python envs locator - Environments Collection', async () => { waitDeferred.resolve(); await deferred.promise; }, - resolve: async (e: PythonEnvInfo) => { + resolve: async (e: any) => { if (env.executable.filename === e.executable.filename) { return resolvedViaLocator; } @@ -464,7 +486,7 @@ suite('Python envs locator - Environments Collection', async () => { env.executable.mtime = 90; sinon.stub(externalDependencies, 'getFileInfo').resolves({ ctime: 100, mtime: 100 }); const parentLocator = new SimpleLocator([], { - resolve: async (e: PythonEnvInfo) => { + resolve: async (e: any) => { if (env.executable.filename === e.executable.filename) { return resolvedViaLocator; } @@ -483,7 +505,7 @@ suite('Python envs locator - Environments Collection', async () => { test('resolveEnv() adds env to cache after resolving using downstream locator', async () => { const resolvedViaLocator = buildEnvInfo({ executable: 'Resolved via locator' }); const parentLocator = new SimpleLocator([], { - resolve: async (e: PythonEnvInfo) => { + resolve: async (e: any) => { if (resolvedViaLocator.executable.filename === e.executable.filename) { return resolvedViaLocator; } @@ -500,6 +522,49 @@ suite('Python envs locator - Environments Collection', async () => { assertEnvsEqual(envs, [resolved]); }); + test('resolveEnv() uses underlying locator once conda envs without python get a python installed', async () => { + const cachedEnvs = [condaEnvWithoutPython]; + const parentLocator = new SimpleLocator( + [], + { + resolve: async (e) => { + if (condaEnvWithoutPython.location === (e as string)) { + return condaEnvWithPython; + } + return undefined; + }, + }, + { resolveAsString: true }, + ); + const cache = await createCollectionCache({ + get: () => cachedEnvs, + store: async () => noop(), + }); + collectionService = new EnvsCollectionService(cache, parentLocator); + let resolved = await collectionService.resolveEnv(condaEnvWithoutPython.location); + assertEnvEqual(resolved, condaEnvWithoutPython); // Ensure cache is used to resolve such envs. + + condaEnvWithPython.executable.ctime = 100; + condaEnvWithPython.executable.mtime = 100; + sinon.stub(externalDependencies, 'getFileInfo').resolves({ ctime: 100, mtime: 100 }); + + const events: PythonEnvCollectionChangedEvent[] = []; + collectionService.onChanged((e) => { + events.push(e); + }); + + await createFile(condaEnvWithPython.executable.filename); // Install Python into the env + + resolved = await collectionService.resolveEnv(condaEnvWithoutPython.location); + assertEnvEqual(resolved, condaEnvWithPython); // Ensure it resolves latest info. + + // Verify conda env without python in cache is replaced with updated info. + const envs = collectionService.getEnvs(); + assertEnvsEqual(envs, [condaEnvWithPython]); + + expect(events.length).to.equal(1, 'Update event should be fired'); + }); + test('Ensure events from downstream locators do not trigger new refreshes if a refresh is already scheduled', async () => { const refreshDeferred = createDeferred(); let refreshCount = 0; diff --git a/src/test/pythonEnvironments/base/locators/envTestUtils.ts b/src/test/pythonEnvironments/base/locators/envTestUtils.ts index 64f8f9558d3c..d1099ee4f840 100644 --- a/src/test/pythonEnvironments/base/locators/envTestUtils.ts +++ b/src/test/pythonEnvironments/base/locators/envTestUtils.ts @@ -1,9 +1,10 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +import * as fsapi from 'fs-extra'; import * as assert from 'assert'; import { exec } from 'child_process'; -import { zip } from 'lodash'; +import { cloneDeep, zip } from 'lodash'; import { promisify } from 'util'; import { PythonEnvInfo, PythonVersion, UNKNOWN_PYTHON_VERSION } from '../../../../client/pythonEnvironments/base/info'; import { getEmptyVersion } from '../../../../client/pythonEnvironments/base/info/pythonVersion'; @@ -40,17 +41,30 @@ export function assertVersionsEqual(actual: PythonVersion | undefined, expected: assert.deepStrictEqual(actual, expected); } +export async function createFile(filename: string, text = ''): Promise { + await fsapi.writeFile(filename, text); + return filename; +} + +export async function deleteFile(filename: string): Promise { + await fsapi.remove(filename); +} + export function assertEnvEqual(actual: PythonEnvInfo | undefined, expected: PythonEnvInfo | undefined): void { assert.notStrictEqual(actual, undefined); assert.notStrictEqual(expected, undefined); if (actual) { + // Make sure to clone so we do not alter the original object + actual = cloneDeep(actual); + expected = cloneDeep(expected); // No need to match these, so reset them actual.executable.ctime = -1; actual.executable.mtime = -1; - actual.version = normalizeVersion(actual.version); if (expected) { + expected.executable.ctime = -1; + expected.executable.mtime = -1; expected.version = normalizeVersion(expected.version); delete expected.id; } From 063a1b9394c0448c803a248a0061fdefb12e9136 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 16 Mar 2023 14:58:28 -0700 Subject: [PATCH 4/5] Add tests for ubuntu --- .../locators/composite/envsCollectionService.unit.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts b/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts index 2470d08dd975..f48d91cf24ae 100644 --- a/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts +++ b/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts @@ -24,27 +24,29 @@ import { noop } from '../../../../core'; import { TEST_LAYOUT_ROOT } from '../../../common/commonTestConstants'; import { SimpleLocator } from '../../common'; import { assertEnvEqual, assertEnvsEqual, createFile, deleteFile } from '../envTestUtils'; +import { OSType, getOSType } from '../../../../common'; suite('Python envs locator - Environments Collection', async () => { let collectionService: EnvsCollectionService; let storage: PythonEnvInfo[]; const updatedName = 'updatedName'; + const pathToCondaPython = getOSType() === OSType.Windows ? 'python.exe' : path.join('bin', 'python'); const condaEnvWithoutPython = createEnv( 'python', undefined, undefined, path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython'), PythonEnvKind.Conda, - path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', 'python.exe'), + path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', pathToCondaPython), ); const condaEnvWithPython = createEnv( - path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', 'python.exe'), + path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', pathToCondaPython), undefined, undefined, path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython'), PythonEnvKind.Conda, - path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', 'python.exe'), + path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', pathToCondaPython), ); function applyChangeEventToEnvList(envs: PythonEnvInfo[], event: PythonEnvCollectionChangedEvent) { From bf3ed30db8f681423d57d754461f99a9bd157aef Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 16 Mar 2023 15:07:59 -0700 Subject: [PATCH 5/5] Add tests for ubuntu --- .../envlayouts/envsWithoutPython/condaLackingPython/bin/dummy | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 src/test/pythonEnvironments/common/envlayouts/envsWithoutPython/condaLackingPython/bin/dummy diff --git a/src/test/pythonEnvironments/common/envlayouts/envsWithoutPython/condaLackingPython/bin/dummy b/src/test/pythonEnvironments/common/envlayouts/envsWithoutPython/condaLackingPython/bin/dummy new file mode 100644 index 000000000000..e69de29bb2d1