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.

Sign up for GitHub

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

Fixes related to loading of env variables #10843

Merged
merged 6 commits into from
Jul 20, 2022
Merged

Fixes related to loading of env variables #10843

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6707207
Fixes
DonJayamanne Jul 18, 2022
d929c61
Misc
DonJayamanne Jul 18, 2022
9a5a623
Misc
DonJayamanne Jul 18, 2022
7943a0c
Fixed tests
DonJayamanne Jul 18, 2022
15e446d
Fix tests
DonJayamanne Jul 18, 2022
8f24f02
Misc
DonJayamanne Jul 18, 2022
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
86 changes: 22 additions & 64 deletions src/kernels/jupyter/jupyterKernelService.node.ts
View file
Open in desktop
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
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,6 @@ export class JupyterKernelService implements IJupyterKernelService {
let specModel: ReadWrite<KernelSpec.ISpecModel> = JSON.parse(
await this.fs.readFile(Uri.file(kernelSpecFilePath))
);
let shouldUpdate = false;

// 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 +271,38 @@ 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed duplicate code in favor of kernelEnvVars.getEnvironmentVariables
Now both raw and jupyter kernels (both) use the exact same code to generate env variables for kernels.

.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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This duplicate code can be replaced with KernelEnvironmentVariablesService.getEnvironmentVariables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now both raw kernels and jupyter kernels will use the same set of code to generate env variables for staring of kernels.

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;
}
if (Cancellation.isCanceled(cancelToken)) {
return;
}

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
44 changes: 18 additions & 26 deletions src/kernels/raw/launcher/kernelEnvVarsService.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,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 @@ -37,13 +44,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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will always return the env variables in this method, never undefined
i.e. merge process.env with kernelspec.env and the like.

}
if (!interpreter && kernelSpec.interpreterPath) {
interpreter = await this.interpreterService
.getInterpreterDetails(Uri.file(kernelSpec.interpreterPath))
.catch((ex) => {
Expand All @@ -52,7 +53,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 @@ -63,26 +64,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));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't necessary as we already do this further below (exact same code)

Copy link
Contributor

Choose a reason for hiding this comment

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

But below doesn't return early? It doesn't look the same to me?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this part of the bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats correct, however if you check the code below its the same, as kernelEnv and customEnvVars will be empty, hence most of the code below will be noops, but at least this way its consistent, else we have a lot of branching if conditions, which just complicates it.

return env;
}
return kernelEnv;
}
// Merge the env variables with that of the kernel env.
interpreterEnv = interpreterEnv || {};
const mergedVars = { ...process.env };
const mergedVars = { ...process.env }; // clone the vars, do not update the original.;
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 @@ -96,16 +88,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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to make is clear that these are custom Environment variables. I am guessing the Edit was a typo from my end.

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
Open in desktop
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
Open in desktop
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