Skip to content

Commit

Permalink
Fixes related to loading of env variables (#10843)
Browse files Browse the repository at this point in the history 
* Fixes

* Misc

* Misc

* Fixed tests

* Fix tests

* Misc
  • Loading branch information
@DonJayamanne
DonJayamanne authored Jul 20, 2022
1 parent eb4348c commit 7308db8
Show file tree
Hide file tree
Showing 7 changed files with 237 additions and 272 deletions.
10 changes: 5 additions & 5 deletions TELEMETRY.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5589,13 +5589,13 @@ No properties for event
[src/kernels/jupyter/jupyterKernelService.node.ts](https://github.com/microsoft/vscode-jupyter/tree/main/src/kernels/jupyter/jupyterKernelService.node.ts)
```typescript
await this.fs.writeFile(Uri.file(kernelSpecFilePath), JSON.stringify(specModel, undefined, 2));
} catch (ex) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
sendTelemetryEvent(Telemetry.FailedToUpdateKernelSpec, undefined, undefined, ex as any, true);
throw ex;
return;
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
sendTelemetryEvent(Telemetry.FailedToUpdateKernelSpec, undefined, undefined, ex as any, true);
throw ex;
}

```
</details>
Expand Down
86 changes: 22 additions & 64 deletions src/kernels/jupyter/jupyterKernelService.node.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,11 @@ import {
} from '../../platform/logging';
import { getDisplayPath, getFilePath } from '../../platform/common/platform/fs-paths';
import { IFileSystemNode } from '../../platform/common/platform/types.node';
import { Resource, ReadWrite, IDisplayOptions, IConfigurationService } from '../../platform/common/types';
import { noop } from '../../platform/common/utils/misc';
import { IEnvironmentVariablesService } from '../../platform/common/variables/types';
import { IEnvironmentActivationService } from '../../platform/interpreter/activation/types';
import { Resource, ReadWrite, IDisplayOptions } from '../../platform/common/types';
import { PythonEnvironment } from '../../platform/pythonEnvironments/info';
import { captureTelemetry, sendTelemetryEvent, Telemetry } from '../../telemetry';
import { JupyterKernelDependencyError } from '../errors/jupyterKernelDependencyError';
import { getKernelRegistrationInfo, cleanEnvironment } from '../helpers';
import { cleanEnvironment } from '../helpers';
import { JupyterPaths } from '../raw/finder/jupyterPaths.node';
import {
IJupyterKernelSpec,
Expand All @@ -40,6 +37,7 @@ import { JupyterKernelSpec } from './jupyterKernelSpec';
import { serializePythonEnvironment } from '../../platform/api/pythonApi';
import { IJupyterKernelService } from './types';
import { arePathsSame } from '../../platform/common/platform/fileUtils';
import { KernelEnvironmentVariablesService } from '../raw/launcher/kernelEnvVarsService.node';

/**
* Responsible for registering and updating kernels in a non ZMQ situation (kernel specs)
Expand All @@ -52,10 +50,8 @@ export class JupyterKernelService implements IJupyterKernelService {
constructor(
@inject(IKernelDependencyService) private readonly kernelDependencyService: IKernelDependencyService,
@inject(IFileSystemNode) private readonly fs: IFileSystemNode,
@inject(IEnvironmentActivationService) private readonly activationHelper: IEnvironmentActivationService,
@inject(IEnvironmentVariablesService) private readonly envVarsService: IEnvironmentVariablesService,
@inject(JupyterPaths) private readonly jupyterPaths: JupyterPaths,
@inject(IConfigurationService) private readonly configService: IConfigurationService
@inject(KernelEnvironmentVariablesService) private readonly kernelEnvVars: KernelEnvironmentVariablesService
) {}

/**
Expand Down Expand Up @@ -261,7 +257,9 @@ export class JupyterKernelService implements IJupyterKernelService {
let specModel: ReadWrite<KernelSpec.ISpecModel> = JSON.parse(
await this.fs.readFile(Uri.file(kernelSpecFilePath))
);
let shouldUpdate = false;
if (Cancellation.isCanceled(cancelToken)) {
return;
}

// Make sure the specmodel has an interpreter or already in the metadata or we
// may overwrite a kernel created by the user
Expand All @@ -276,75 +274,35 @@ export class JupyterKernelService implements IJupyterKernelService {
);
specModel.argv[0] = interpreter.uri.fsPath;
}
// Get the activated environment variables (as a work around for `conda run` and similar).
// This ensures the code runs within the context of an activated environment.
const interpreterEnv = await this.activationHelper
.getActivatedEnvironmentVariables(resource, interpreter, true)
.catch(noop)
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.then((env) => (env || {}) as any);

// Ensure we inherit env variables from the original kernelspec file.
let envInKernelSpecJson =
getKernelRegistrationInfo(kernel) === 'registeredByNewVersionOfExtForCustomKernelSpec'
? specModel.env || {}
: {};

// Give preferences to variables in the env file (except for `PATH`).
envInKernelSpecJson = Object.assign({ ...interpreterEnv }, envInKernelSpecJson);
if (interpreterEnv['PATH']) {
envInKernelSpecJson['PATH'] = interpreterEnv['PATH'];
}
if (interpreterEnv['Path']) {
envInKernelSpecJson['Path'] = interpreterEnv['Path'];
}
specModel.env = Object.assign(envInKernelSpecJson, specModel.env);

// Ensure the python env folder is always at the top of the PATH, this way all executables from that env are used.
// This way shell commands such as `!pip`, `!python` end up pointing to the right executables.
// Also applies to `!java` where java could be an executable in the conda bin directory.
if (specModel.env) {
this.envVarsService.prependPath(specModel.env as {}, path.dirname(interpreter.uri.fsPath));
}

// If user asks us to, set PYTHONNOUSERSITE
// For more details see here https://github.com/microsoft/vscode-jupyter/issues/8553#issuecomment-997144591
// https://docs.python.org/3/library/site.html#site.ENABLE_USER_SITE
if (this.configService.getSettings(undefined).excludeUserSitePackages) {
specModel.env.PYTHONNOUSERSITE = 'True';
}

if (Cancellation.isCanceled(cancelToken)) {
return;
}

// Ensure we update the metadata to include interpreter stuff as well (we'll use this to search kernels that match an interpreter).
// We'll need information such as interpreter type, display name, path, etc...
// Its just a JSON file, and the information is small, hence might as well store everything.
specModel.metadata = specModel.metadata || {};
// eslint-disable-next-line @typescript-eslint/no-explicit-any
specModel.metadata.interpreter = interpreter as any;

// Indicate we need to write
shouldUpdate = true;
}

specModel.env = await this.kernelEnvVars.getEnvironmentVariables(resource, interpreter, specedKernel);

// Scrub the environment of the specmodel to make sure it has allowed values (they all must be strings)
// See this issue here: https://github.com/microsoft/vscode-python/issues/11749
if (specModel.env) {
specModel = cleanEnvironment(specModel);
shouldUpdate = true;
}
specModel = cleanEnvironment(specModel);

// Update the kernel.json with our new stuff.
if (shouldUpdate) {
try {
await this.fs.writeFile(Uri.file(kernelSpecFilePath), JSON.stringify(specModel, undefined, 2));
} catch (ex) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
sendTelemetryEvent(Telemetry.FailedToUpdateKernelSpec, undefined, undefined, ex as any, true);
throw ex;
try {
await this.fs.writeFile(Uri.file(kernelSpecFilePath), JSON.stringify(specModel, undefined, 2));
} catch (ex) {
if (Cancellation.isCanceled(cancelToken)) {
return;
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
sendTelemetryEvent(Telemetry.FailedToUpdateKernelSpec, undefined, undefined, ex as any, true);
throw ex;
}

if (Cancellation.isCanceled(cancelToken)) {
return;
}

// Always update the metadata for the original kernel.
Expand Down
42 changes: 17 additions & 25 deletions src/kernels/raw/launcher/kernelEnvVarsService.node.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ export class KernelEnvironmentVariablesService {
@inject(IConfigurationService) private readonly configService: IConfigurationService
) {}
/**
* Generates the environment variables for the kernel.
*
* Merge env variables from the following 3 sources:
* 1. kernelspec.json file
* 2. process.env
* 3. .env file in workspace folder
*
* If the kernel belongs to a conda environment, then use the env variables of the conda environment and merge that with the env variables of the kernel spec.
* In the case of some kernels such as java, the kernel spec contains the cli as such `argv = ['java', 'xyz']`.
* The first item in the kernelspec argv is an kernel executable, and it might not in the current path (e.g. `java`).
Expand All @@ -40,13 +47,7 @@ export class KernelEnvironmentVariablesService {
let kernelEnv = kernelSpec.env && Object.keys(kernelSpec.env).length > 0 ? kernelSpec.env : undefined;

// If an interpreter was not explicitly passed in, check for an interpreter path in the kernelspec to use
if (!interpreter) {
if (!kernelSpec.interpreterPath) {
traceInfo(
`No custom variables for Kernel as interpreter path is not defined for kernel ${kernelSpec.display_name}`
);
return kernelEnv;
}
if (!interpreter && kernelSpec.interpreterPath) {
interpreter = await this.interpreterService
.getInterpreterDetails(Uri.file(kernelSpec.interpreterPath))
.catch((ex) => {
Expand All @@ -55,7 +56,7 @@ export class KernelEnvironmentVariablesService {
});
}

let [customEditVars, interpreterEnv] = await Promise.all([
let [customEnvVars, interpreterEnv] = await Promise.all([
this.customEnvVars.getCustomEnvironmentVariables(resource).catch(noop),
interpreter
? this.envActivation
Expand All @@ -66,26 +67,17 @@ export class KernelEnvironmentVariablesService {
})
: undefined
]);
if (!interpreterEnv && Object.keys(customEditVars || {}).length === 0) {
if (!interpreterEnv && Object.keys(customEnvVars || {}).length === 0) {
traceInfo('No custom variables nor do we have a conda environment');
// Ensure the python env folder is always at the top of the PATH, this way all executables from that env are used.
// This way shell commands such as `!pip`, `!python` end up pointing to the right executables.
// Also applies to `!java` where java could be an executable in the conda bin directory.
if (interpreter) {
const env = kernelEnv || process.env;
this.envVarsService.prependPath(env, path.dirname(interpreter.uri.fsPath));
return env;
}
return kernelEnv;
}
// Merge the env variables with that of the kernel env.
interpreterEnv = interpreterEnv || {};
const mergedVars = { ...process.env };
kernelEnv = kernelEnv || {};
customEditVars = customEditVars || {};
customEnvVars = customEnvVars || {};
this.envVarsService.mergeVariables(interpreterEnv, mergedVars); // interpreter vars win over proc.
this.envVarsService.mergeVariables(kernelEnv, mergedVars); // kernels vars win over interpreter.
this.envVarsService.mergeVariables(customEditVars, mergedVars); // custom vars win over all.
this.envVarsService.mergeVariables(customEnvVars, mergedVars); // custom vars win over all.
// Reinitialize the PATH variables.
// The values in `PATH` found in the interpreter trumps everything else.
// If we have more PATHS, they need to be appended to this PATH.
Expand All @@ -99,16 +91,16 @@ export class KernelEnvironmentVariablesService {
if (interpreterEnv['PYTHONPATH']) {
mergedVars['PYTHONPATH'] = interpreterEnv['PYTHONPATH'];
}
otherEnvPathKey = Object.keys(customEditVars).find((k) => k.toLowerCase() == 'path');
if (otherEnvPathKey && customEditVars[otherEnvPathKey]) {
this.envVarsService.appendPath(mergedVars, customEditVars[otherEnvPathKey]!);
otherEnvPathKey = Object.keys(customEnvVars).find((k) => k.toLowerCase() == 'path');
if (otherEnvPathKey && customEnvVars[otherEnvPathKey]) {
this.envVarsService.appendPath(mergedVars, customEnvVars[otherEnvPathKey]!);
}
otherEnvPathKey = Object.keys(kernelEnv).find((k) => k.toLowerCase() == 'path');
if (otherEnvPathKey && kernelEnv[otherEnvPathKey]) {
this.envVarsService.appendPath(mergedVars, kernelEnv[otherEnvPathKey]!);
}
if (customEditVars.PYTHONPATH) {
this.envVarsService.appendPythonPath(mergedVars, customEditVars.PYTHONPATH);
if (customEnvVars.PYTHONPATH) {
this.envVarsService.appendPythonPath(mergedVars, customEnvVars.PYTHONPATH);
}
if (kernelEnv.PYTHONPATH) {
this.envVarsService.appendPythonPath(mergedVars, kernelEnv.PYTHONPATH);
Expand Down
80 changes: 0 additions & 80 deletions src/platform/common/process/environmentActivationService.node.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,29 +318,6 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
return;
}

// If this is a conda environment that supports conda run, then we don't need conda activation commands.
const [activationCommands, customEnvVars] = await Promise.all([
interpreter.envType === EnvironmentType.Conda
? Promise.resolve([])
: this.getActivationCommands(resource, interpreter),
this.envVarsService.getCustomEnvironmentVariables(resource)
]);

// Check cache.
const customEnvVariablesHash = getTelemetrySafeHashedString(JSON.stringify(customEnvVars));
const cachedVariables = this.getActivatedEnvVariablesFromCache(
resource,
interpreter,
customEnvVariablesHash,
activationCommands
);
if (cachedVariables) {
traceVerbose(`Got activation Env Vars from cache`);
return cachedVariables && customEnvVars
? { ...cachedVariables, ...customEnvVars }
: cachedVariables || customEnvVars;
}

if (this.activatedEnvVariablesCache.has(key)) {
traceVerbose(`Got activation Env Vars from cached promise with key ${key}`);
return this.activatedEnvVariablesCache.get(key);
Expand Down Expand Up @@ -529,53 +506,6 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
);
}

/**
* We cache activated environment variables.
* When activating environments, all activation scripts update environment variables, nothing else (after all they don't start a process).
* The env variables can change based on the activation script, current env variables on the machine & python interpreter information.
* If any of these change, then the env variables are invalidated.
*/
private getActivatedEnvVariablesFromCache(
resource: Resource,
@logValue<PythonEnvironment>('uri') interpreter: PythonEnvironment,
customEnvVariablesHash: string,
activationCommandsForNonCondaEnvironments: string[] = []
) {
const workspaceKey = this.workspace.getWorkspaceFolderIdentifier(resource);
const key = ENVIRONMENT_ACTIVATED_ENV_VARS_KEY_PREFIX.format(
`${workspaceKey}_${interpreter && getInterpreterHash(interpreter)}`
);
const interpreterVersion = `${interpreter.sysVersion || ''}#${interpreter.version?.raw || ''}`;
const cachedData = this.memento.get<EnvironmentVariablesCacheInformation>(key);
if (!cachedData || !cachedData.activatedEnvVariables) {
return;
}
if (
cachedData.interpreterVersion !== interpreterVersion ||
cachedData.customEnvVariablesHash !== customEnvVariablesHash
) {
return;
}
// We're interested in activation commands only for non-conda environments.
// For conda environments, we don't care about the activation commands (as we activate either using conda activation commands
// Or use conda run).
// Hence for purposes of caching we don't care about the commands.
if (
interpreter.envType !== EnvironmentType.Conda &&
cachedData.activationCommands.join(',').toLowerCase() !==
(activationCommandsForNonCondaEnvironments || []).join(',').toLowerCase()
) {
return;
}
if (
cachedData.originalProcEnvVariablesHash !==
getTelemetrySafeHashedString(JSON.stringify(this.sanitizedCurrentProcessEnvVars))
) {
return;
}
this.updateWithLatestVSCodeVariables(cachedData.activatedEnvVariables);
return cachedData.activatedEnvVariables;
}
private async storeActivatedEnvVariablesInCache(
resource: Resource,
@logValue<PythonEnvironment>('uri') interpreter: PythonEnvironment,
Expand Down Expand Up @@ -619,16 +549,6 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
});
return vars;
}
private updateWithLatestVSCodeVariables(envVars: EnvironmentVariables) {
// Restore the env vars we removed.
const vars = JSON.parse(JSON.stringify(this.currentProcess.env));
Object.keys(vars).forEach((key) => {
if (key.startsWith('VSCODE_')) {
envVars[key] = vars[key];
}
});
}

@traceDecoratorVerbose('getCondaEnvVariables', TraceOptions.BeforeCall)
public async getCondaEnvVariables(
resource: Resource,
Expand Down
6 changes: 6 additions & 0 deletions src/platform/common/variables/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ export const IEnvironmentVariablesProvider = Symbol('IEnvironmentVariablesProvid

export interface IEnvironmentVariablesProvider {
onDidEnvironmentVariablesChange: Event<Uri | undefined>;
/**
* Gets merged result of process.env and env variables defined in the .env file.
*/
getEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables>;
/**
* Gets the env variables defined in the .env file.
*/
getCustomEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables | undefined>;
}
Loading

0 comments on commit 7308db8

Please sign in to comment.