Skip to content

Commit 054730f

Browse files
author
Kartik Raj
authored
Use conda run to validate conda interpreters only if attempt to validate it normally fails (#18195)
* Fallback on validating conda env in a normal way if conda run fails * Only build VSIX * Oops * Force kill processes after shell exec is complete * Fallback on conda run * Add conda working pool for validating conda envs * Fix env info service * Revert "Oops" This reverts commit db6dbd8. * Revert "Only build VSIX" This reverts commit 930707c. * Simplify * If validating conda envs normally fails, do not log as error
1 parent b00a568 commit 054730f

File tree

5 files changed

+112
-86
lines changed

5 files changed

+112
-86
lines changed

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33
import { EventEmitter } from 'events';
4+
import { traceError } from '../../logging';
45

56
import { IDisposable } from '../types';
67
import { EnvironmentVariables } from '../variables/types';
@@ -59,6 +60,16 @@ export class ProcessService extends EventEmitter implements IProcessService {
5960

6061
public shellExec(command: string, options: ShellOptions = {}): Promise<ExecutionResult<string>> {
6162
this.emit('exec', command, undefined, options);
62-
return shellExec(command, options, this.env, this.processesToKill);
63+
const disposables = new Set<IDisposable>();
64+
return shellExec(command, options, this.env, disposables).finally(() => {
65+
// Ensure the process we started is cleaned up.
66+
disposables.forEach((p) => {
67+
try {
68+
p.dispose();
69+
} catch {
70+
traceError(`Unable to kill process for ${command}`);
71+
}
72+
});
73+
});
6374
}
6475
}

‎src/client/pythonEnvironments/base/info/environmentInfoService.ts‎

Lines changed: 82 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,10 @@ import { createDeferred, Deferred } from '../../../common/utils/async';
66
import { createRunningWorkerPool, IWorkerPool, QueuePosition } from '../../../common/utils/workerPool';
77
import { getInterpreterInfo, InterpreterInformation } from './interpreter';
88
import { buildPythonExecInfo } from '../../exec';
9-
import { traceError } from '../../../logging';
10-
import { Conda, CONDA_RUN_TIMEOUT, CONDA_RUN_SCRIPT } from '../../common/environmentManagers/conda';
9+
import { traceError, traceInfo } from '../../../logging';
10+
import { Conda, CONDA_RUN_TIMEOUT, isCondaEnvironment } from '../../common/environmentManagers/conda';
1111
import { PythonEnvInfo, PythonEnvKind } from '.';
12+
import { normCasePath } from '../../common/externalDependencies';
1213

1314
export enum EnvironmentInfoServiceQueuePriority {
1415
Default,
@@ -20,30 +21,28 @@ export interface IEnvironmentInfoService {
2021
env: PythonEnvInfo,
2122
priority?: EnvironmentInfoServiceQueuePriority,
2223
): Promise<InterpreterInformation | undefined>;
23-
isInfoProvided(interpreterPath: string): boolean;
2424
}
2525

2626
async function buildEnvironmentInfo(env: PythonEnvInfo): Promise<InterpreterInformation | undefined> {
27-
let python = [env.executable.filename];
28-
const isCondaEnv = env.kind === PythonEnvKind.Conda;
29-
if (isCondaEnv) {
30-
const conda = await Conda.getConda();
31-
const runArgs = await conda?.getRunArgs({ name: env.name, prefix: env.location });
32-
if (runArgs) {
33-
python = [...runArgs, 'python', CONDA_RUN_SCRIPT];
34-
}
35-
}
36-
const interpreterInfo = await getInterpreterInfo(
37-
buildPythonExecInfo(python, undefined, env.executable.filename),
38-
isCondaEnv ? CONDA_RUN_TIMEOUT : undefined,
39-
).catch((reason) => {
40-
traceError(reason);
41-
return undefined;
42-
});
27+
const python = [env.executable.filename];
28+
const interpreterInfo = await getInterpreterInfo(buildPythonExecInfo(python, undefined, env.executable.filename));
29+
return interpreterInfo;
30+
}
4331

44-
if (interpreterInfo === undefined || interpreterInfo.version === undefined) {
32+
async function buildEnvironmentInfoUsingCondaRun(env: PythonEnvInfo): Promise<InterpreterInformation | undefined> {
33+
const conda = await Conda.getConda();
34+
const condaEnv = await conda?.getCondaEnvironment(env.executable.filename);
35+
if (!condaEnv) {
36+
return undefined;
37+
}
38+
const python = await conda?.getRunPythonArgs(condaEnv);
39+
if (!python) {
4540
return undefined;
4641
}
42+
const interpreterInfo = await getInterpreterInfo(
43+
buildPythonExecInfo(python, undefined, env.executable.filename),
44+
CONDA_RUN_TIMEOUT,
45+
);
4746
return interpreterInfo;
4847
}
4948

@@ -59,47 +58,96 @@ class EnvironmentInfoService implements IEnvironmentInfoService {
5958

6059
private workerPool?: IWorkerPool<PythonEnvInfo, InterpreterInformation | undefined>;
6160

61+
private condaRunWorkerPool?: IWorkerPool<PythonEnvInfo, InterpreterInformation | undefined>;
62+
6263
public dispose(): void {
6364
if (this.workerPool !== undefined) {
6465
this.workerPool.stop();
6566
this.workerPool = undefined;
6667
}
68+
if (this.condaRunWorkerPool !== undefined) {
69+
this.condaRunWorkerPool.stop();
70+
this.condaRunWorkerPool = undefined;
71+
}
6772
}
6873

6974
public async getEnvironmentInfo(
7075
env: PythonEnvInfo,
7176
priority?: EnvironmentInfoServiceQueuePriority,
7277
): Promise<InterpreterInformation | undefined> {
7378
const interpreterPath = env.executable.filename;
74-
const result = this.cache.get(interpreterPath);
79+
const result = this.cache.get(normCasePath(interpreterPath));
7580
if (result !== undefined) {
76-
// Another call for this environment has already been made, return its result
81+
// Another call for this environment has already been made, return its result.
7782
return result.promise;
7883
}
7984

85+
const deferred = createDeferred<InterpreterInformation>();
86+
this.cache.set(normCasePath(interpreterPath), deferred);
87+
this._getEnvironmentInfo(env, priority)
88+
.then((r) => {
89+
deferred.resolve(r);
90+
})
91+
.catch((ex) => {
92+
deferred.reject(ex);
93+
});
94+
return deferred.promise;
95+
}
96+
97+
public async _getEnvironmentInfo(
98+
env: PythonEnvInfo,
99+
priority?: EnvironmentInfoServiceQueuePriority,
100+
): Promise<InterpreterInformation | undefined> {
80101
if (this.workerPool === undefined) {
81102
this.workerPool = createRunningWorkerPool<PythonEnvInfo, InterpreterInformation | undefined>(
82103
buildEnvironmentInfo,
83104
);
84105
}
85106

86-
const deferred = createDeferred<InterpreterInformation>();
87-
this.cache.set(interpreterPath, deferred);
88-
return (priority === EnvironmentInfoServiceQueuePriority.High
89-
? this.workerPool.addToQueue(env, QueuePosition.Front)
90-
: this.workerPool.addToQueue(env, QueuePosition.Back)
91-
).then((r) => {
92-
deferred.resolve(r);
93-
return r;
107+
let reason: unknown;
108+
let r = await addToQueue(this.workerPool, env, priority).catch((err) => {
109+
reason = err;
110+
return undefined;
94111
});
95-
}
96112

97-
public isInfoProvided(interpreterPath: string): boolean {
98-
const result = this.cache.get(interpreterPath);
99-
return !!(result && result.completed);
113+
if (r === undefined) {
114+
// Even though env kind is not conda, it can still be a conda environment
115+
// as complete env info may not be available at this time.
116+
const isCondaEnv = env.kind === PythonEnvKind.Conda || (await isCondaEnvironment(env.executable.filename));
117+
if (isCondaEnv) {
118+
traceInfo(
119+
`Validating ${env.executable.filename} normally failed with error, falling back to using conda run: (${reason})`,
120+
);
121+
if (this.condaRunWorkerPool === undefined) {
122+
// Create a separate queue for validation using conda, so getting environment info for
123+
// other types of environment aren't blocked on conda.
124+
this.condaRunWorkerPool = createRunningWorkerPool<
125+
PythonEnvInfo,
126+
InterpreterInformation | undefined
127+
>(buildEnvironmentInfoUsingCondaRun);
128+
}
129+
r = await addToQueue(this.condaRunWorkerPool, env, priority).catch((err) => {
130+
traceError(err);
131+
return undefined;
132+
});
133+
} else if (reason) {
134+
traceError(reason);
135+
}
136+
}
137+
return r;
100138
}
101139
}
102140

141+
function addToQueue(
142+
workerPool: IWorkerPool<PythonEnvInfo, InterpreterInformation | undefined>,
143+
env: PythonEnvInfo,
144+
priority: EnvironmentInfoServiceQueuePriority | undefined,
145+
) {
146+
return priority === EnvironmentInfoServiceQueuePriority.High
147+
? workerPool.addToQueue(env, QueuePosition.Front)
148+
: workerPool.addToQueue(env, QueuePosition.Back);
149+
}
150+
103151
let envInfoService: IEnvironmentInfoService | undefined;
104152
export function getEnvironmentInfoService(disposables?: IDisposableRegistry): IEnvironmentInfoService {
105153
if (envInfoService === undefined) {

‎src/client/pythonEnvironments/common/environmentManagers/conda.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import * as fsapi from 'fs-extra';
22
import * as path from 'path';
33
import { lt, parse, SemVer } from 'semver';
44
import { getEnvironmentVariable, getOSType, getUserHomeDir, OSType } from '../../../common/utils/platform';
5-
import { arePathsSame, exec, getPythonSetting, pathExists, readFile } from '../externalDependencies';
5+
import { arePathsSame, exec, getPythonSetting, isParentPath, pathExists, readFile } from '../externalDependencies';
66

77
import { PythonVersion, UNKNOWN_PYTHON_VERSION } from '../../base/info';
88
import { parseVersion } from '../../base/info/pythonVersion';
@@ -400,7 +400,12 @@ export class Conda {
400400
}));
401401
}
402402

403-
public async getRunArgs(env: CondaEnvInfo): Promise<string[] | undefined> {
403+
public async getCondaEnvironment(executable: string): Promise<CondaEnvInfo | undefined> {
404+
const envList = await this.getEnvList();
405+
return envList.find((e) => isParentPath(executable, e.prefix));
406+
}
407+
408+
public async getRunPythonArgs(env: CondaEnvInfo): Promise<string[] | undefined> {
404409
const condaVersion = await this.getCondaVersion();
405410
if (condaVersion && lt(condaVersion, CONDA_RUN_VERSION)) {
406411
return undefined;
@@ -411,7 +416,7 @@ export class Conda {
411416
} else {
412417
args.push('-p', env.prefix);
413418
}
414-
return [this.command, 'run', ...args, '--no-capture-output'];
419+
return [this.command, 'run', ...args, '--no-capture-output', 'python', CONDA_RUN_SCRIPT];
415420
}
416421

417422
/**

‎src/test/pythonEnvironments/base/info/environmentInfoService.functional.test.ts‎

Lines changed: 0 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import {
1818
getEnvironmentInfoService,
1919
} from '../../../../client/pythonEnvironments/base/info/environmentInfoService';
2020
import { buildEnvInfo } from '../../../../client/pythonEnvironments/base/info/env';
21-
import { PythonEnvKind } from '../../../../client/pythonEnvironments/base/info';
2221
import { Conda, CONDA_RUN_VERSION } from '../../../../client/pythonEnvironments/common/environmentManagers/conda';
2322

2423
suite('Environment Info Service', () => {
@@ -110,45 +109,4 @@ suite('Environment Info Service', () => {
110109
});
111110
assert.ok(stubShellExec.calledOnce);
112111
});
113-
114-
test('isInfoProvided() returns true for items already processed', async () => {
115-
const envService = getEnvironmentInfoService(disposables);
116-
let result: boolean;
117-
const promises: Promise<InterpreterInformation | undefined>[] = [];
118-
const path1 = 'any-path1';
119-
const path2 = 'any-path2';
120-
121-
promises.push(envService.getEnvironmentInfo(buildEnvInfo({ executable: path1 })));
122-
promises.push(envService.getEnvironmentInfo(buildEnvInfo({ executable: path2 })));
123-
124-
await Promise.all(promises);
125-
result = envService.isInfoProvided(path1);
126-
assert.strictEqual(result, true);
127-
result = envService.isInfoProvided(path2);
128-
assert.strictEqual(result, true);
129-
});
130-
131-
test('isInfoProvided() returns false otherwise', async () => {
132-
const envService = getEnvironmentInfoService(disposables);
133-
const promises: Promise<InterpreterInformation | undefined>[] = [];
134-
const path1 = 'any-path1';
135-
const path2 = 'any-path2';
136-
137-
promises.push(envService.getEnvironmentInfo(buildEnvInfo({ executable: path1 })));
138-
promises.push(envService.getEnvironmentInfo(buildEnvInfo({ executable: path2 })));
139-
140-
let result = envService.isInfoProvided(path1);
141-
assert.strictEqual(result, false);
142-
result = envService.isInfoProvided(path2);
143-
assert.strictEqual(result, false);
144-
result = envService.isInfoProvided('some-random-path');
145-
assert.strictEqual(result, false);
146-
});
147-
148-
test('Returns expected item if interpreter type is conda', async () => {
149-
const envService = getEnvironmentInfoService(disposables);
150-
const path = `any-path`;
151-
const info = await envService.getEnvironmentInfo(buildEnvInfo({ executable: path, kind: PythonEnvKind.Conda }));
152-
assert.deepEqual(info, createExpectedEnvInfo(path));
153-
});
154112
});

‎src/test/pythonEnvironments/common/environmentManagers/conda.unit.test.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,11 @@ import { PythonEnvKind } from '../../../../client/pythonEnvironments/base/info';
1010
import { getEnvs } from '../../../../client/pythonEnvironments/base/locatorUtils';
1111
import * as externalDependencies from '../../../../client/pythonEnvironments/common/externalDependencies';
1212
import * as windowsUtils from '../../../../client/pythonEnvironments/common/windowsUtils';
13-
import { Conda, CondaInfo } from '../../../../client/pythonEnvironments/common/environmentManagers/conda';
13+
import {
14+
Conda,
15+
CondaInfo,
16+
CONDA_RUN_SCRIPT,
17+
} from '../../../../client/pythonEnvironments/common/environmentManagers/conda';
1418
import { CondaEnvironmentLocator } from '../../../../client/pythonEnvironments/base/locators/lowLevel/condaLocator';
1519
import { createBasicEnv } from '../../base/common';
1620
import { assertBasicEnvsEqual } from '../../base/locators/envTestUtils';
@@ -467,7 +471,7 @@ suite('Conda and its environments are located correctly', () => {
467471
conda: JSON.stringify(condaInfo('4.8.0')),
468472
};
469473
const conda = await Conda.getConda();
470-
const args = await conda?.getRunArgs({ name: 'envName', prefix: 'envPrefix' });
474+
const args = await conda?.getRunPythonArgs({ name: 'envName', prefix: 'envPrefix' });
471475
expect(args).to.equal(undefined);
472476
});
473477

@@ -476,18 +480,18 @@ suite('Conda and its environments are located correctly', () => {
476480
conda: JSON.stringify(condaInfo('4.9.0')),
477481
};
478482
const conda = await Conda.getConda();
479-
let args = await conda?.getRunArgs({ name: 'envName', prefix: 'envPrefix' });
483+
let args = await conda?.getRunPythonArgs({ name: 'envName', prefix: 'envPrefix' });
480484
expect(args).to.not.equal(undefined);
481485
assert.deepStrictEqual(
482486
args,
483-
['conda', 'run', '-n', 'envName', '--no-capture-output'],
487+
['conda', 'run', '-n', 'envName', '--no-capture-output', 'python', CONDA_RUN_SCRIPT],
484488
'Incorrect args for case 1',
485489
);
486490

487-
args = await conda?.getRunArgs({ name: '', prefix: 'envPrefix' });
491+
args = await conda?.getRunPythonArgs({ name: '', prefix: 'envPrefix' });
488492
assert.deepStrictEqual(
489493
args,
490-
['conda', 'run', '-p', 'envPrefix', '--no-capture-output'],
494+
['conda', 'run', '-p', 'envPrefix', '--no-capture-output', 'python', CONDA_RUN_SCRIPT],
491495
'Incorrect args for case 2',
492496
);
493497
});

0 commit comments

Comments
 (0)