Skip to content

Commit

Permalink
Revert to using conda run for fetching activated environment variab…
Browse files Browse the repository at this point in the history 
…les (#19986)

Reverts #19819 Closes
#19967

Until we have a better activation story:
#11039, where we
potentially get rid of using `conda activate` commands, use `conda run`
for fetching environment variables even though it's slower.

Created #19987 for
tackling it in the future.

FYI @DonJayamanne
  • Loading branch information
Kartik Raj authored Oct 12, 2022
1 parent 76f7763 commit 38d8d9c
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 26 deletions.
66 changes: 43 additions & 23 deletions src/client/interpreter/activation/service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { sleep } from '../../common/utils/async';
import { InMemoryCache } from '../../common/utils/cacheUtils';
import { OSType } from '../../common/utils/platform';
import { IEnvironmentVariablesProvider } from '../../common/variables/types';
import { PythonEnvironment } from '../../pythonEnvironments/info';
import { EnvironmentType, PythonEnvironment } from '../../pythonEnvironments/info';
import { captureTelemetry, sendTelemetryEvent } from '../../telemetry';
import { EventName } from '../../telemetry/constants';
import { IInterpreterService } from '../contracts';
Expand All @@ -30,6 +30,7 @@ import {
traceVerbose,
traceWarn,
} from '../../logging';
import { Conda } from '../../pythonEnvironments/common/environmentManagers/conda';

const ENVIRONMENT_PREFIX = 'e8b39361-0157-4923-80e1-22d70d46dee6';
const CACHE_DURATION = 10 * 60 * 1000;
Expand Down Expand Up @@ -169,20 +170,41 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
if (!shellInfo) {
return;
}
let isPossiblyCondaEnv = false;
try {
const activationCommands = await this.helper.getEnvironmentActivationShellCommands(
resource,
shellInfo.shellType,
interpreter,
);
traceVerbose(`Activation Commands received ${activationCommands} for shell ${shellInfo.shell}`);
if (!activationCommands || !Array.isArray(activationCommands) || activationCommands.length === 0) {
return;
let command: string | undefined;
let [args, parse] = internalScripts.printEnvVariables();
args.forEach((arg, i) => {
args[i] = arg.toCommandArgumentForPythonExt();
});
interpreter = interpreter ?? (await this.interpreterService.getActiveInterpreter(resource));
if (interpreter?.envType === EnvironmentType.Conda) {
const conda = await Conda.getConda();
const pythonArgv = await conda?.getRunPythonArgs({
name: interpreter.envName,
prefix: interpreter.envPath ?? '',
});
if (pythonArgv) {
// Using environment prefix isn't needed as the marker script already takes care of it.
command = [...pythonArgv, ...args].map((arg) => arg.toCommandArgumentForPythonExt()).join(' ');
}
}
if (!command) {
const activationCommands = await this.helper.getEnvironmentActivationShellCommands(
resource,
shellInfo.shellType,
interpreter,
);
traceVerbose(`Activation Commands received ${activationCommands} for shell ${shellInfo.shell}`);
if (!activationCommands || !Array.isArray(activationCommands) || activationCommands.length === 0) {
return;
}
// Run the activate command collect the environment from it.
const activationCommand = this.fixActivationCommands(activationCommands).join(' && ');
// In order to make sure we know where the environment output is,
// put in a dummy echo we can look for
command = `${activationCommand} && echo '${ENVIRONMENT_PREFIX}' && python ${args.join(' ')}`;
}
isPossiblyCondaEnv = activationCommands.join(' ').toLowerCase().includes('conda');
// Run the activate command collect the environment from it.
const activationCommand = this.fixActivationCommands(activationCommands).join(' && ');

const processService = await this.processServiceFactory.create(resource);
const customEnvVars = await this.envVarsService.getEnvironmentVariables(resource);
const hasCustomEnvVars = Object.keys(customEnvVars).length;
Expand All @@ -194,14 +216,6 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
env[PYTHON_WARNINGS] = 'ignore';

traceVerbose(`${hasCustomEnvVars ? 'Has' : 'No'} Custom Env Vars`);

// In order to make sure we know where the environment output is,
// put in a dummy echo we can look for
const [args, parse] = internalScripts.printEnvVariables();
args.forEach((arg, i) => {
args[i] = arg.toCommandArgumentForPythonExt();
});
const command = `${activationCommand} && echo '${ENVIRONMENT_PREFIX}' && python ${args.join(' ')}`;
traceVerbose(`Activating Environment to capture Environment variables, ${command}`);

// Do some wrapping of the call. For two reasons:
Expand All @@ -219,7 +233,10 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
result = await processService.shellExec(command, {
env,
shell: shellInfo.shell,
timeout: isPossiblyCondaEnv ? CONDA_ENVIRONMENT_TIMEOUT : ENVIRONMENT_TIMEOUT,
timeout:
interpreter?.envType === EnvironmentType.Conda
? CONDA_ENVIRONMENT_TIMEOUT
: ENVIRONMENT_TIMEOUT,
maxBuffer: 1000 * 1000,
throwOnStdErr: false,
});
Expand Down Expand Up @@ -265,7 +282,7 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
} catch (e) {
traceError('getActivatedEnvironmentVariables', e);
sendTelemetryEvent(EventName.ACTIVATE_ENV_TO_GET_ENV_VARS_FAILED, undefined, {
isPossiblyCondaEnv,
isPossiblyCondaEnv: interpreter?.envType === EnvironmentType.Conda,
terminal: shellInfo.shellType,
});

Expand All @@ -283,6 +300,9 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
@traceDecoratorError('Failed to parse Environment variables')
@traceDecoratorVerbose('parseEnvironmentOutput', TraceOptions.None)
protected parseEnvironmentOutput(output: string, parse: (out: string) => NodeJS.ProcessEnv | undefined) {
if (output.indexOf(ENVIRONMENT_PREFIX) === -1) {
return parse(output);
}
output = output.substring(output.indexOf(ENVIRONMENT_PREFIX) + ENVIRONMENT_PREFIX.length);
const js = output.substring(output.indexOf('{')).trim();
return parse(js);
Expand Down
7 changes: 4 additions & 3 deletions src/test/interpreters/activation/service.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ suite('Interpreters Activation - Python Environment Variables', () => {
architecture: Architecture.x64,
};

function initSetup() {
function initSetup(interpreter: PythonEnvironment | undefined) {
helper = mock(TerminalHelper);
platform = mock(PlatformService);
processServiceFactory = mock(ProcessServiceFactory);
Expand All @@ -71,6 +71,7 @@ suite('Interpreters Activation - Python Environment Variables', () => {
onDidChangeInterpreter = new EventEmitter<void>();
when(envVarsService.onDidEnvironmentVariablesChange).thenReturn(onDidChangeEnvVariables.event);
when(interpreterService.onDidChangeInterpreter).thenReturn(onDidChangeInterpreter.event);
when(interpreterService.getActiveInterpreter(anything())).thenResolve(interpreter);
service = new EnvironmentActivationService(
instance(helper),
instance(platform),
Expand All @@ -89,7 +90,7 @@ suite('Interpreters Activation - Python Environment Variables', () => {
[undefined, Uri.parse('a')].forEach((resource) =>
[undefined, pythonInterpreter].forEach((interpreter) => {
suite(title(resource, interpreter), () => {
setup(initSetup);
setup(() => initSetup(interpreter));
test('Unknown os will return empty variables', async () => {
when(platform.osType).thenReturn(OSType.Unknown);
const env = await service.getActivatedEnvironmentVariables(resource);
Expand All @@ -102,7 +103,7 @@ suite('Interpreters Activation - Python Environment Variables', () => {

osTypes.forEach((osType) => {
suite(osType.name, () => {
setup(initSetup);
setup(() => initSetup(interpreter));
test('getEnvironmentActivationShellCommands will be invoked', async () => {
when(platform.osType).thenReturn(osType.value);
when(
Expand Down

0 comments on commit 38d8d9c

Please sign in to comment.