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

Modify environment info worker to support new type and to work with resolver #13997

Merged
merged 8 commits into from
Sep 22, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
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
2 changes: 1 addition & 1 deletion pythonFiles/interpreterInfo.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
obj = {}
obj["versionInfo"] = tuple(sys.version_info)
obj["sysPrefix"] = sys.prefix
obj["version"] = sys.version
obj["sysVersion"] = sys.version
Copy link
Author

Choose a reason for hiding this comment

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

The JSON expected in the code contains sysVersion field.

export type PythonEnvInfo = {
versionInfo: PythonVersionInfo;
sysPrefix: string;
sysVersion: string;
is64Bit: boolean;
};
export function interpreterInfo(): [string[], (out: string) => PythonEnvInfo] {
const script = path.join(SCRIPTS_DIR, 'interpreterInfo.py');
const args = [ISOLATED, script];
function parse(out: string): PythonEnvInfo {
let json: PythonEnvInfo;
try {
json = JSON.parse(out);
} catch (ex) {
throw Error(`python ${args} returned bad JSON (${out}) (${ex})`);
}
return json;
}

This bug existed since 3 years ago lol, when we first added interpreterInfo.py.

Copy link

@kimadeline kimadeline Sep 22, 2020

Choose a reason for hiding this comment

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

How come it hadn't been caught until now, we never needed this field?

Copy link
Author

Choose a reason for hiding this comment

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

Looks like we actually don't. I'll create a separate issue to remove it.

obj["is64Bit"] = sys.maxsize > 2 ** 32

print(json.dumps(obj))
89 changes: 89 additions & 0 deletions src/client/pythonEnvironments/base/info/interpreter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import { PythonExecutableInfo, PythonVersion } from '.';
import { interpreterInfo as getInterpreterInfoCommand, PythonEnvInfo } from '../../../common/process/internal/scripts';
import { Architecture } from '../../../common/utils/platform';
import { copyPythonExecInfo, PythonExecInfo } from '../../exec';
import { parseVersion } from './pythonVersion';

export type InterpreterInformation = {

Choose a reason for hiding this comment

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

There's already an InterpreterInformation type being exported from src/client/pythonEnvironments/info/index.ts. I think it would be more clear if this type had a different name to minimize future auto-import mix-ups.

Not part of this PR, but we also have multiple PythonEnvInfo types, so.... 😕

Up to you.

Copy link
Author

Choose a reason for hiding this comment

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

We'll be getting rid of the old InterpreterInformation type, and I didn't want to choose a new name because InterpreterInformation is the most correct name I can think of.

I'll take care of the PythonEnvInfo type.

arch: Architecture;
executable: PythonExecutableInfo;
version: PythonVersion;
};

/**
* Compose full interpreter information based on the given data.
*
* The data format corresponds to the output of the `interpreterInfo.py` script.
*
* @param python - the path to the Python executable
* @param raw - the information returned by the `interpreterInfo.py` script
*/
function extractInterpreterInfo(python: string, raw: PythonEnvInfo): InterpreterInformation {
const rawVersion = `${raw.versionInfo.slice(0, 3).join('.')}-${raw.versionInfo[3]}`;
return {
arch: raw.is64Bit ? Architecture.x64 : Architecture.x86,
executable: {
filename: python,
sysPrefix: raw.sysPrefix,
mtime: -1,
ctime: -1,
},
version: {
...parseVersion(rawVersion),
sysVersion: raw.sysVersion,
},
};
}

type ShellExecResult = {
stdout: string;
stderr?: string;
};
type ShellExecFunc = (command: string, timeout: number) => Promise<ShellExecResult>;

type Logger = {
info(msg: string): void;
error(msg: string): void;
};

/**
* Collect full interpreter information from the given Python executable.
*
* @param python - the information to use when running Python
* @param shellExec - the function to use to exec Python
* @param logger - if provided, used to log failures or other info
*/
export async function getInterpreterInfo(
Copy link
Author

@karrtikr karrtikr Sep 21, 2020

Choose a reason for hiding this comment

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

Most of it is copied from src\client\pythonEnvironments\info\interpreter.ts which is to be removed later. All the new types are being put inside base folder.

python: PythonExecInfo,
shellExec: ShellExecFunc,
logger?: Logger,
): Promise<InterpreterInformation | undefined> {
const [args, parse] = getInterpreterInfoCommand();
const info = copyPythonExecInfo(python, args);
const argv = [info.command, ...info.args];

// Concat these together to make a set of quoted strings
const quoted = argv.reduce((p, c) => (p ? `${p} "${c}"` : `"${c.replace('\\', '\\\\')}"`), '');

// Try shell execing the command, followed by the arguments. This will make node kill the process if it
// takes too long.
// Sometimes the python path isn't valid, timeout if that's the case.
// See these two bugs:
// https://github.com/microsoft/vscode-python/issues/7569
// https://github.com/microsoft/vscode-python/issues/7760
const result = await shellExec(quoted, 15000);
if (result.stderr) {
if (logger) {
logger.error(`Failed to parse interpreter information for ${argv} stderr: ${result.stderr}`);
}
return undefined;
}
const json = parse(result.stdout);
if (logger) {
logger.info(`Found interpreter for ${argv}`);
}
return extractInterpreterInfo(python.pythonExecutable, json);
}
35 changes: 35 additions & 0 deletions src/client/pythonEnvironments/base/info/pythonVersion.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import { PythonReleaseLevel, PythonVersion } from '.';
import { EMPTY_VERSION, parseBasicVersionInfo } from '../../../common/utils/version';

export function parseVersion(versionStr: string): PythonVersion {
const parsed = parseBasicVersionInfo<PythonVersion>(versionStr);
if (!parsed) {
if (versionStr === '') {
return EMPTY_VERSION as PythonVersion;
}
throw Error(`invalid version ${versionStr}`);
}
const { version, after } = parsed;
const match = after.match(/^(a|b|rc)(\d+)$/);
if (match) {
const [, levelStr, serialStr] = match;
let level: PythonReleaseLevel;
if (levelStr === 'a') {
level = PythonReleaseLevel.Alpha;
} else if (levelStr === 'b') {
level = PythonReleaseLevel.Beta;
} else if (levelStr === 'rc') {
level = PythonReleaseLevel.Candidate;
} else {
throw Error('unreachable!');
}
version.release = {
level,
serial: parseInt(serialStr, 10),
};
}
return version;
}
58 changes: 22 additions & 36 deletions src/client/pythonEnvironments/info/environmentInfoService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
// Licensed under the MIT License.

import { injectable } from 'inversify';
import { EnvironmentType, PythonEnvironment } from '.';
import { createDeferred, Deferred } from '../../common/utils/async';
import { createWorkerPool, IWorkerPool, QueuePosition } from '../../common/utils/workerPool';
import { getInterpreterInfo, InterpreterInformation } from '../base/info/interpreter';
import { shellExecute } from '../common/externalDependencies';
import { buildPythonExecInfo } from '../exec';
import { getInterpreterInfo } from './interpreter';

export enum EnvironmentInfoServiceQueuePriority {
Default,
Expand All @@ -18,37 +18,17 @@ export interface IEnvironmentInfoService {
getEnvironmentInfo(
interpreterPath: string,
priority?: EnvironmentInfoServiceQueuePriority
): Promise<PythonEnvironment | undefined>;
): Promise<InterpreterInformation | undefined>;
}

async function buildEnvironmentInfo(interpreterPath: string): Promise<PythonEnvironment | undefined> {
const interpreterInfo = await getInterpreterInfo(buildPythonExecInfo(interpreterPath), shellExecute);
async function buildEnvironmentInfo(interpreterPath: string): Promise<InterpreterInformation | undefined> {
const interpreterInfo = await getInterpreterInfo(buildPythonExecInfo(interpreterPath), shellExecute).catch(
() => undefined,
);
if (interpreterInfo === undefined || interpreterInfo.version === undefined) {
return undefined;
}
return {
path: interpreterInfo.path,
// Have to do this because the type returned by getInterpreterInfo is SemVer
// But we expect this to be PythonVersion
version: {
raw: interpreterInfo.version.raw,
major: interpreterInfo.version.major,
minor: interpreterInfo.version.minor,
patch: interpreterInfo.version.patch,
build: interpreterInfo.version.build,
prerelease: interpreterInfo.version.prerelease,
},
sysVersion: interpreterInfo.sysVersion,
architecture: interpreterInfo.architecture,
sysPrefix: interpreterInfo.sysPrefix,
pipEnvWorkspaceFolder: interpreterInfo.pipEnvWorkspaceFolder,
companyDisplayName: '',
displayName: '',
envType: EnvironmentType.Unknown, // Code to handle This will be added later.
envName: '',
envPath: '',
cachedEntry: false,
};
return interpreterInfo;
}

@injectable()
Expand All @@ -57,29 +37,35 @@ export class EnvironmentInfoService implements IEnvironmentInfoService {
// path again and again in a given session. This information will likely not change in a given
// session. There are definitely cases where this will change. But a simple reload should address
// those.
private readonly cache: Map<string, PythonEnvironment> = new Map<string, PythonEnvironment>();
private readonly cache: Map<string, Deferred<InterpreterInformation>> = new Map<
string,
Deferred<InterpreterInformation>
>();

private readonly workerPool: IWorkerPool<string, PythonEnvironment | undefined>;
private readonly workerPool: IWorkerPool<string, InterpreterInformation | undefined>;

public constructor() {
this.workerPool = createWorkerPool<string, PythonEnvironment | undefined>(buildEnvironmentInfo);
this.workerPool = createWorkerPool<string, InterpreterInformation | undefined>(buildEnvironmentInfo);
}

public async getEnvironmentInfo(
interpreterPath: string,
priority?: EnvironmentInfoServiceQueuePriority,
): Promise<PythonEnvironment | undefined> {
): Promise<InterpreterInformation | undefined> {
const result = this.cache.get(interpreterPath);
if (result !== undefined) {
return result;
// Another call for this environment has already been made, return its result
return result.promise;
}

const deferred = createDeferred<InterpreterInformation>();
this.cache.set(interpreterPath, deferred);
return (priority === EnvironmentInfoServiceQueuePriority.High
? this.workerPool.addToQueue(interpreterPath, QueuePosition.Front)
: this.workerPool.addToQueue(interpreterPath, QueuePosition.Back)
).then((r) => {
if (r !== undefined) {
this.cache.set(interpreterPath, r);
deferred.resolve(r);
if (r === undefined) {
this.cache.delete(interpreterPath);
}
return r;
});
Expand Down
34 changes: 1 addition & 33 deletions src/test/pythonEnvironments/base/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,11 @@

import { createDeferred, flattenIterator, iterable, mapToIterator } from '../../../client/common/utils/async';
import { Architecture } from '../../../client/common/utils/platform';
import { EMPTY_VERSION, parseBasicVersionInfo } from '../../../client/common/utils/version';
import {
PythonEnvInfo,
PythonEnvKind,
PythonReleaseLevel,
PythonVersion
} from '../../../client/pythonEnvironments/base/info';
import { parseVersion } from '../../../client/pythonEnvironments/base/info/pythonVersion';
import { IPythonEnvsIterator, Locator, PythonLocatorQuery } from '../../../client/pythonEnvironments/base/locator';
import { PythonEnvsChangedEvent } from '../../../client/pythonEnvironments/base/watcher';

Expand Down Expand Up @@ -45,36 +43,6 @@ export function createEnv(
};
}

function parseVersion(versionStr: string): PythonVersion {
const parsed = parseBasicVersionInfo<PythonVersion>(versionStr);
if (!parsed) {
if (versionStr === '') {
return EMPTY_VERSION as PythonVersion;
}
throw Error(`invalid version ${versionStr}`);
}
const { version, after } = parsed;
const match = after.match(/^(a|b|rc)(\d+)$/);
if (match) {
const [, levelStr, serialStr ] = match;
let level: PythonReleaseLevel;
if (levelStr === 'a') {
level = PythonReleaseLevel.Alpha;
} else if (levelStr === 'b') {
level = PythonReleaseLevel.Beta;
} else if (levelStr === 'rc') {
level = PythonReleaseLevel.Candidate;
} else {
throw Error('unreachable!');
}
version.release = {
level,
serial: parseInt(serialStr, 10)
};
}
return version;
}

export function createLocatedEnv(
location: string,
versionStr: string,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import * as sinon from 'sinon';
import { ImportMock } from 'ts-mock-imports';
import { ExecutionResult } from '../../../client/common/process/types';
import { Architecture } from '../../../client/common/utils/platform';
import { InterpreterInformation } from '../../../client/pythonEnvironments/base/info/interpreter';
import { parseVersion } from '../../../client/pythonEnvironments/base/info/pythonVersion';
import * as ExternalDep from '../../../client/pythonEnvironments/common/externalDependencies';
import { EnvironmentType, PythonEnvironment } from '../../../client/pythonEnvironments/info';
import {
EnvironmentInfoService,
EnvironmentInfoServiceQueuePriority,
Expand All @@ -18,27 +19,16 @@ import {
suite('Environment Info Service', () => {
let stubShellExec: sinon.SinonStub;

function createExpectedEnvInfo(path: string): PythonEnvironment {
function createExpectedEnvInfo(executable: string): InterpreterInformation {
return {
path,
architecture: Architecture.x64,
sysVersion: undefined,
sysPrefix: 'path',
pipEnvWorkspaceFolder: undefined,
version: {
build: [],
major: 3,
minor: 8,
patch: 3,
prerelease: ['final'],
raw: '3.8.3-final',
version: { ...parseVersion('3.8.3-final'), sysVersion: '3.8.3 (tags/v3.8.3:6f8c832, May 13 2020, 22:37:02) [MSC v.1924 64 bit (AMD64)]' },
arch: Architecture.x64,
executable: {
filename: executable,
sysPrefix: 'path',
mtime: -1,
ctime: -1,
},
companyDisplayName: '',
displayName: '',
envType: EnvironmentType.Unknown,
envName: '',
envPath: '',
cachedEntry: false,
};
}

Expand All @@ -49,7 +39,7 @@ suite('Environment Info Service', () => {
new Promise<ExecutionResult<string>>((resolve) => {
resolve({
stdout:
'{"versionInfo": [3, 8, 3, "final", 0], "sysPrefix": "path", "version": "3.8.3 (tags/v3.8.3:6f8c832, May 13 2020, 22:37:02) [MSC v.1924 64 bit (AMD64)]", "is64Bit": true}',
'{"versionInfo": [3, 8, 3, "final", 0], "sysPrefix": "path", "sysVersion": "3.8.3 (tags/v3.8.3:6f8c832, May 13 2020, 22:37:02) [MSC v.1924 64 bit (AMD64)]", "is64Bit": true}',
});
}),
);
Expand All @@ -59,8 +49,8 @@ suite('Environment Info Service', () => {
});
test('Add items to queue and get results', async () => {
const envService = new EnvironmentInfoService();
const promises: Promise<PythonEnvironment | undefined>[] = [];
const expected: PythonEnvironment[] = [];
const promises: Promise<InterpreterInformation | undefined>[] = [];
const expected: InterpreterInformation[] = [];
for (let i = 0; i < 10; i = i + 1) {
const path = `any-path${i}`;
if (i < 5) {
Expand All @@ -82,8 +72,8 @@ suite('Environment Info Service', () => {

test('Add same item to queue', async () => {
const envService = new EnvironmentInfoService();
const promises: Promise<PythonEnvironment | undefined>[] = [];
const expected: PythonEnvironment[] = [];
const promises: Promise<InterpreterInformation | undefined>[] = [];
const expected: InterpreterInformation[] = [];

const path = 'any-path';
// Clear call counts
Expand Down