From 5b712deb234ff99e48fbd0902a948bfa75f55be8 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 5 Apr 2021 17:39:38 -0700 Subject: [PATCH 1/6] Add caching to command output in discovery component --- src/client/common/utils/decorators.ts | 24 +++++++--- .../common/externalDependencies.ts | 8 ++++ .../discovery/locators/services/conda.ts | 2 + .../discovery/locators/services/poetry.ts | 47 +++++++++++++------ 4 files changed, 59 insertions(+), 22 deletions(-) diff --git a/src/client/common/utils/decorators.ts b/src/client/common/utils/decorators.ts index 36c374689301..cd95b2996e79 100644 --- a/src/client/common/utils/decorators.ts +++ b/src/client/common/utils/decorators.ts @@ -121,7 +121,13 @@ export function makeDebounceAsyncDecorator(wait?: number) { type PromiseFunctionWithAnyArgs = (...any: any) => Promise; const cacheStoreForMethods = getGlobalCacheStore(); -export function cache(expiryDurationMs: number) { +/** + * Caches function value until a specific duration. + * @param expiryDurationMs Duration to cache the result for. If set as '-1', the cache will never expire for the session. + * @param cachePromise If true, cache the promise instead of the promise result. + * @returns + */ +export function cache(expiryDurationMs: number, cachePromise = false) { return function ( target: Object, propertyName: string, @@ -136,16 +142,20 @@ export function cache(expiryDurationMs: number) { } const key = getCacheKeyFromFunctionArgs(keyPrefix, args); const cachedItem = cacheStoreForMethods.get(key); - if (cachedItem && cachedItem.expiry > Date.now()) { + if (cachedItem && (cachedItem.expiry > Date.now() || cachedItem.expiry === -1)) { traceVerbose(`Cached data exists ${key}`); return Promise.resolve(cachedItem.data); } const promise = originalMethod.apply(this, args) as Promise; - promise - .then((result) => - cacheStoreForMethods.set(key, { data: result, expiry: Date.now() + expiryDurationMs }), - ) - .ignoreErrors(); + if (cachePromise) { + cacheStoreForMethods.set(key, { data: promise, expiry: Date.now() + expiryDurationMs }); + } else { + promise + .then((result) => + cacheStoreForMethods.set(key, { data: result, expiry: Date.now() + expiryDurationMs }), + ) + .ignoreErrors(); + } return promise; }; }; diff --git a/src/client/pythonEnvironments/common/externalDependencies.ts b/src/client/pythonEnvironments/common/externalDependencies.ts index a00fc6d5c8d4..41cc445c2128 100644 --- a/src/client/pythonEnvironments/common/externalDependencies.ts +++ b/src/client/pythonEnvironments/common/externalDependencies.ts @@ -61,10 +61,18 @@ export function pathExists(absPath: string): Promise { return fsapi.pathExists(absPath); } +export function pathExistsSync(absPath: string): boolean { + return fsapi.pathExistsSync(absPath); +} + export function readFile(filePath: string): Promise { return fsapi.readFile(filePath, 'utf-8'); } +export function readFileSync(filePath: string): string { + return fsapi.readFileSync(filePath, 'utf-8'); +} + /** * Returns true if given file path exists within the given parent directory, false otherwise. * @param filePath File path to check for diff --git a/src/client/pythonEnvironments/discovery/locators/services/conda.ts b/src/client/pythonEnvironments/discovery/locators/services/conda.ts index ef9201912adc..16c0148c6df6 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/conda.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/conda.ts @@ -10,6 +10,7 @@ import { parseVersion } from '../../../base/info/pythonVersion'; import { getRegistryInterpreters } from '../../../common/windowsUtils'; import { EnvironmentType, PythonEnvironment } from '../../../info'; import { IDisposable } from '../../../../common/types'; +import { cache } from '../../../../common/utils/decorators'; export const AnacondaCompanyNames = ['Anaconda, Inc.', 'Continuum Analytics, Inc.']; @@ -347,6 +348,7 @@ export class Conda { * Retrieves list of Python environments known to this conda. * Corresponds to "conda env list --json", but also computes environment names. */ + @cache(20_000) public async getEnvList(): Promise { const info = await this.getInfo(); const { envs } = info; diff --git a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts index d7d4380f580d..b70c0f4fcb08 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts @@ -9,13 +9,14 @@ import { getOSType, getUserHomeDir, OSType } from '../../../../common/utils/plat import { getPythonSetting, isParentPath, - pathExists, - readFile, + pathExistsSync, + readFileSync, shellExecute, } from '../../../common/externalDependencies'; import { getEnvironmentDirFromPath } from '../../../common/commonUtils'; import { isVirtualenvEnvironment } from './virtualEnvironmentIdentifier'; import { StopWatch } from '../../../../common/utils/stopWatch'; +import { cache } from '../../../../common/utils/decorators'; /** * Global virtual env dir for a project is named as: @@ -59,7 +60,7 @@ async function isLocalPoetryEnvironment(interpreterPath: string): Promise { - if (!(await hasValidPyprojectToml(cwd))) { + if (!hasValidPyprojectToml(cwd)) { // This check is not expensive and may change during a session, so we need not cache it. return undefined; } @@ -128,7 +129,7 @@ export class Poetry { */ private static async locate(cwd: string): Promise { // Produce a list of candidate binaries to be probed by exec'ing them. - async function* getCandidates() { + function* getCandidates() { const customPoetryPath = getPythonSetting('poetryPath'); if (customPoetryPath && customPoetryPath !== 'poetry') { // If user has specified a custom poetry path, use it first. @@ -139,26 +140,21 @@ export class Poetry { const home = getUserHomeDir(); if (home) { const defaultPoetryPath = path.join(home, '.poetry', 'bin', 'poetry'); - if (await pathExists(defaultPoetryPath)) { + if (pathExistsSync(defaultPoetryPath)) { yield defaultPoetryPath; } } } // Probe the candidates, and pick the first one that exists and does what we need. - for await (const poetryPath of getCandidates()) { + for (const poetryPath of getCandidates()) { traceVerbose(`Probing poetry binary for ${cwd}: ${poetryPath}`); const poetry = new Poetry(poetryPath, cwd); - const stopWatch = new StopWatch(); const virtualenvs = await poetry.getEnvList(); if (virtualenvs !== undefined) { traceVerbose(`Found poetry via filesystem probing for ${cwd}: ${poetryPath}`); return poetry; } - traceVerbose( - `Time taken to run ${poetryPath} env list --full-path in ms for ${cwd}`, - stopWatch.elapsedTime, - ); } // Didn't find anything. @@ -176,6 +172,15 @@ export class Poetry { * Corresponds to "poetry env list --full-path". Swallows errors if any. */ public async getEnvList(): Promise { + return this._getEnvList(this.cwd); + } + + /** + * Method created to faciliate caching. The caching decorator uses function arguments as cache key, + * so pass in cwd on which we need to cache. + */ + @cache(30_000, true) + private async _getEnvList(_cwd: string): Promise { const result = await this.safeShellExecute(`${this._command} env list --full-path`); if (!result) { return undefined; @@ -203,6 +208,18 @@ export class Poetry { * Corresponds to "poetry env info -p". Swallows errors if any. */ public async getActiveEnvPath(): Promise { + return this._getActiveEnvPath(this.cwd); + } + + /** + * Method created to faciliate caching. The caching decorator uses function arguments as cache key, + * so pass in cwd on which we need to cache. + * + * Note this cache has no expiry, this is because we often need this method to decide whether to use + * poetry to install a product, after user asks to install something. We need this to be fast always. + */ + @cache(-1, true) + private async _getActiveEnvPath(_cwd: string): Promise { const result = await this.safeShellExecute(`${this._command} env info -p`, true); if (!result) { return undefined; @@ -288,12 +305,12 @@ export async function isPoetryEnvironmentRelatedToFolder( * * @param folder Folder to look for pyproject.toml file in. */ -async function hasValidPyprojectToml(folder: string): Promise { +function hasValidPyprojectToml(folder: string): boolean { const pyprojectToml = path.join(folder, 'pyproject.toml'); - if (!(await pathExists(pyprojectToml))) { + if (!pathExistsSync(pyprojectToml)) { return false; } - const content = await readFile(pyprojectToml); + const content = readFileSync(pyprojectToml); if (!content.includes('[tool.poetry]')) { return false; } From 075b78d983259bf440d5593334078d8b7afbb73f Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 5 Apr 2021 17:53:52 -0700 Subject: [PATCH 2/6] Add doc comments --- .../discovery/locators/services/poetry.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts index b70c0f4fcb08..b3a095232804 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts @@ -112,7 +112,15 @@ export class Poetry { this.fixCwd(); } + /** + * Returns a Poetry instance corresponding to the binary which can be used to run commands for the cwd. + * + * Poetry commands can be slow and so can be bottleneck to overall discovery time. So trigger command + * execution as soon as possible. To do that we need to ensure the operations before the command are + * performed synchronously. + */ public static async getPoetry(cwd: string): Promise { + // Following check should be performed synchronously so we trigger poetry execution as soon as possible. if (!hasValidPyprojectToml(cwd)) { // This check is not expensive and may change during a session, so we need not cache it. return undefined; @@ -124,10 +132,10 @@ export class Poetry { return Poetry._poetryPromise.get(cwd); } - /** - * Returns a Poetry instance corresponding to the binary. - */ private static async locate(cwd: string): Promise { + // First thing this method awaits on should be poetry command execution, hence perform all operations + // before that synchronously. + // Produce a list of candidate binaries to be probed by exec'ing them. function* getCandidates() { const customPoetryPath = getPythonSetting('poetryPath'); From 996122ddca92c6cff3e36b5c7be31dd760d7360e Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 5 Apr 2021 17:56:23 -0700 Subject: [PATCH 3/6] Fix tests --- .../discovery/locators/services/poetry.ts | 10 +++++----- .../discovery/locators/lowLevel/poetry.unit.test.ts | 7 +++---- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts index b3a095232804..7d3513050e0b 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts @@ -180,7 +180,7 @@ export class Poetry { * Corresponds to "poetry env list --full-path". Swallows errors if any. */ public async getEnvList(): Promise { - return this._getEnvList(this.cwd); + return this.getEnvListCached(this.cwd); } /** @@ -188,7 +188,7 @@ export class Poetry { * so pass in cwd on which we need to cache. */ @cache(30_000, true) - private async _getEnvList(_cwd: string): Promise { + private async getEnvListCached(_cwd: string): Promise { const result = await this.safeShellExecute(`${this._command} env list --full-path`); if (!result) { return undefined; @@ -216,7 +216,7 @@ export class Poetry { * Corresponds to "poetry env info -p". Swallows errors if any. */ public async getActiveEnvPath(): Promise { - return this._getActiveEnvPath(this.cwd); + return this.getActiveEnvPathCached(this.cwd); } /** @@ -224,10 +224,10 @@ export class Poetry { * so pass in cwd on which we need to cache. * * Note this cache has no expiry, this is because we often need this method to decide whether to use - * poetry to install a product, after user asks to install something. We need this to be fast always. + * poetry to install a product, after user asks to install something. We need this to be fast. */ @cache(-1, true) - private async _getActiveEnvPath(_cwd: string): Promise { + private async getActiveEnvPathCached(_cwd: string): Promise { const result = await this.safeShellExecute(`${this._command} env info -p`, true); if (!result) { return undefined; diff --git a/src/test/pythonEnvironments/discovery/locators/lowLevel/poetry.unit.test.ts b/src/test/pythonEnvironments/discovery/locators/lowLevel/poetry.unit.test.ts index 21e1de6ba27c..4b1f5a727a38 100644 --- a/src/test/pythonEnvironments/discovery/locators/lowLevel/poetry.unit.test.ts +++ b/src/test/pythonEnvironments/discovery/locators/lowLevel/poetry.unit.test.ts @@ -94,7 +94,6 @@ suite('isPoetryEnvironment Tests', () => { suite('Poetry binary is located correctly', async () => { let shellExecute: sinon.SinonStub; let getPythonSetting: sinon.SinonStub; - let pathExists: sinon.SinonStub; suiteSetup(() => { Poetry._poetryPromise = new Map(); @@ -175,9 +174,9 @@ suite('Poetry binary is located correctly', async () => { return; } const defaultPoetry = path.join(home, '.poetry', 'bin', 'poetry'); - pathExists = sinon.stub(externalDependencies, 'pathExists'); - pathExists.withArgs(defaultPoetry).resolves(true); - pathExists.callThrough(); + const pathExistsSync = sinon.stub(externalDependencies, 'pathExistsSync'); + pathExistsSync.withArgs(defaultPoetry).returns(true); + pathExistsSync.callThrough(); getPythonSetting.returns('poetry'); shellExecute.callsFake((command: string, options: ShellOptions) => { if ( From af4a2c9c5b1b3f8fddd326e24a30d5a9ab627ce7 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 6 Apr 2021 10:18:22 -0700 Subject: [PATCH 4/6] Cache for lesser expiry duration after startup --- src/client/common/utils/decorators.ts | 29 ++++++++++++++----- .../discovery/locators/services/conda.ts | 2 +- .../discovery/locators/services/poetry.ts | 2 +- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/client/common/utils/decorators.ts b/src/client/common/utils/decorators.ts index cd95b2996e79..cb9203bc8f10 100644 --- a/src/client/common/utils/decorators.ts +++ b/src/client/common/utils/decorators.ts @@ -4,6 +4,7 @@ import { traceError, traceVerbose } from '../logger'; import { createDeferred, Deferred } from './async'; import { getCacheKeyFromFunctionArgs, getGlobalCacheStore } from './cacheUtils'; import { TraceInfo, tracing } from './misc'; +import { StopWatch } from './stopWatch'; const _debounce = require('lodash/debounce') as typeof import('lodash/debounce'); @@ -121,13 +122,25 @@ export function makeDebounceAsyncDecorator(wait?: number) { type PromiseFunctionWithAnyArgs = (...any: any) => Promise; const cacheStoreForMethods = getGlobalCacheStore(); + +/** + * Extension start up time is considered the duration until extension is likely to keep running commands in background. + * It is observed on CI it can take upto 3 minutes, so this is an intelligent guess. + */ +const extensionStartUpTime = 200_000; +/** + * Tracks the time since the module was loaded. For caching purposes, we consider this time to approximately signify + * how long extension has been active. + */ +const moduleLoadWatch = new StopWatch(); /** * Caches function value until a specific duration. * @param expiryDurationMs Duration to cache the result for. If set as '-1', the cache will never expire for the session. * @param cachePromise If true, cache the promise instead of the promise result. - * @returns + * @param expiryDurationAfterStartUpMs If specified, this is the duration to cache the result for after extension startup (until extension is likely to + * keep running commands in background) */ -export function cache(expiryDurationMs: number, cachePromise = false) { +export function cache(expiryDurationMs: number, cachePromise = false, expiryDurationAfterStartUpMs?: number) { return function ( target: Object, propertyName: string, @@ -142,18 +155,20 @@ export function cache(expiryDurationMs: number, cachePromise = false) { } const key = getCacheKeyFromFunctionArgs(keyPrefix, args); const cachedItem = cacheStoreForMethods.get(key); - if (cachedItem && (cachedItem.expiry > Date.now() || cachedItem.expiry === -1)) { + if (cachedItem && (cachedItem.expiry > Date.now() || expiryDurationMs === -1)) { traceVerbose(`Cached data exists ${key}`); return Promise.resolve(cachedItem.data); } + const expiryMs = + expiryDurationAfterStartUpMs && moduleLoadWatch.elapsedTime > extensionStartUpTime + ? expiryDurationAfterStartUpMs + : expiryDurationMs; const promise = originalMethod.apply(this, args) as Promise; if (cachePromise) { - cacheStoreForMethods.set(key, { data: promise, expiry: Date.now() + expiryDurationMs }); + cacheStoreForMethods.set(key, { data: promise, expiry: Date.now() + expiryMs }); } else { promise - .then((result) => - cacheStoreForMethods.set(key, { data: result, expiry: Date.now() + expiryDurationMs }), - ) + .then((result) => cacheStoreForMethods.set(key, { data: result, expiry: Date.now() + expiryMs })) .ignoreErrors(); } return promise; diff --git a/src/client/pythonEnvironments/discovery/locators/services/conda.ts b/src/client/pythonEnvironments/discovery/locators/services/conda.ts index 16c0148c6df6..cbd1bb1bba3b 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/conda.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/conda.ts @@ -348,7 +348,7 @@ export class Conda { * Retrieves list of Python environments known to this conda. * Corresponds to "conda env list --json", but also computes environment names. */ - @cache(20_000) + @cache(30_000, true, 10_000) public async getEnvList(): Promise { const info = await this.getInfo(); const { envs } = info; diff --git a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts index 7d3513050e0b..9c44fb9868f3 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts @@ -187,7 +187,7 @@ export class Poetry { * Method created to faciliate caching. The caching decorator uses function arguments as cache key, * so pass in cwd on which we need to cache. */ - @cache(30_000, true) + @cache(30_000, true, 10_000) private async getEnvListCached(_cwd: string): Promise { const result = await this.safeShellExecute(`${this._command} env list --full-path`); if (!result) { From 2bfc88bf61d630696618e7edddb717f37810f263 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 6 Apr 2021 15:29:12 -0700 Subject: [PATCH 5/6] Get python executables using `getInterpreterPathFromDir` instead of finding interpreters in all directories --- .../discovery/locators/services/poetry.ts | 2 +- .../locators/services/poetryLocator.ts | 32 +++++++------------ 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts index 9c44fb9868f3..e4100ed22733 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts @@ -226,7 +226,7 @@ export class Poetry { * Note this cache has no expiry, this is because we often need this method to decide whether to use * poetry to install a product, after user asks to install something. We need this to be fast. */ - @cache(-1, true) + @cache(20_000, true, 10_000) private async getActiveEnvPathCached(_cwd: string): Promise { const result = await this.safeShellExecute(`${this._command} env info -p`, true); if (!result) { diff --git a/src/client/pythonEnvironments/discovery/locators/services/poetryLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/poetryLocator.ts index 33423ad6e185..6bda02974cd3 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/poetryLocator.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/poetryLocator.ts @@ -13,10 +13,9 @@ import { buildEnvInfo } from '../../../base/info/env'; import { IPythonEnvsIterator } from '../../../base/locator'; import { FSWatchingLocator } from '../../../base/locators/lowLevel/fsWatchingLocator'; import { - findInterpretersInDir, getEnvironmentDirFromPath, + getInterpreterPathFromDir, getPythonVersionFromPath, - looksLikeBasicVirtualPython, } from '../../../common/commonUtils'; import { getFileInfo, isParentPath, pathExists } from '../../../common/externalDependencies'; import { isPoetryEnvironment, localPoetryEnvDirName, Poetry } from './poetry'; @@ -114,29 +113,20 @@ export class PoetryLocator extends FSWatchingLocator { traceVerbose(`Searching for poetry virtual envs in: ${envDir}`); const isLocal = isParentPath(envDir, root); - const executables = findInterpretersInDir(envDir, 1); - - for await (const entry of executables) { - const { filename } = entry; - // We only care about python.exe (on windows) and python (on linux/mac) - // Other version like python3.exe or python3.8 are often symlinks to - // python.exe or python in the same directory in the case of virtual - // environments. - if (await looksLikeBasicVirtualPython(entry)) { + const filename = await getInterpreterPathFromDir(envDir); + if (filename !== undefined) { + const kind = PythonEnvKind.Poetry; + if (isLocal && !(await isPoetryEnvironment(filename))) { + // This is not a poetry env. + traceVerbose( + `Poetry Virtual Environment: [skipped] ${filename} (reason: Not poetry environment)`, + ); + } else { // We should extract the kind here to avoid doing is*Environment() // check multiple times. Those checks are file system heavy and // we can use the kind to determine this anyway. - yield buildVirtualEnvInfo( - filename, - // Global environments are fetched using 'poetry env list' so we already - // know they're poetry environments, no need to get kind for them. - isLocal ? await getVirtualEnvKind(filename) : PythonEnvKind.Poetry, - undefined, - isLocal, - ); + yield buildVirtualEnvInfo(filename, kind, undefined, isLocal); traceVerbose(`Poetry Virtual Environment: [added] ${filename}`); - } else { - traceVerbose(`Poetry Virtual Environment: [skipped] ${filename}`); } } } From cd8e02940cd5a2e546bc6b5801a5dde9c53af1c0 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 6 Apr 2021 15:45:42 -0700 Subject: [PATCH 6/6] Update doc comment --- .../pythonEnvironments/discovery/locators/services/poetry.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts index e4100ed22733..520958e71464 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/poetry.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/poetry.ts @@ -222,9 +222,6 @@ export class Poetry { /** * Method created to faciliate caching. The caching decorator uses function arguments as cache key, * so pass in cwd on which we need to cache. - * - * Note this cache has no expiry, this is because we often need this method to decide whether to use - * poetry to install a product, after user asks to install something. We need this to be fast. */ @cache(20_000, true, 10_000) private async getActiveEnvPathCached(_cwd: string): Promise {