Skip to content

Commit daa467d

Browse files
rchiodorchiodo
authored
Fix problems with a python kernel not handling json with numbers (#12293)
* Fix environment values that aren't strings. * Consolidate in one function * Fix unit test * Make unit test verify env * Review feedback
1 parent e066c28 commit daa467d

5 files changed

Lines changed: 53 additions & 11 deletions

File tree

‎news/2 Fixes/11749.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Jupyter can fail to run a kernel if the user's environment contains non string values.

‎src/client/datascience/jupyter/kernels/helpers.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ import { JupyterKernelSpec } from './jupyterKernelSpec';
88
// tslint:disable-next-line: no-var-requires no-require-imports
99
const NamedRegexp = require('named-js-regexp') as typeof import('named-js-regexp');
1010

11+
// tslint:disable-next-line: no-require-imports
12+
import cloneDeep = require('lodash/cloneDeep');
13+
1114
// Helper functions for dealing with kernels and kernelspecs
1215

1316
export const defaultKernelSpecName = 'python_defaultSpec_';
@@ -42,3 +45,24 @@ export function detectDefaultKernelName(name: string) {
4245
const regEx = NamedRegexp('python\\s*(?<version>(\\d+))', 'g');
4346
return regEx.exec(name.toLowerCase());
4447
}
48+
49+
export function cleanEnvironment<T>(spec: T): T {
50+
// tslint:disable-next-line: no-any
51+
const copy = cloneDeep(spec) as { env?: any };
52+
53+
if (copy.env) {
54+
// Scrub the environment of the spec to make sure it has allowed values (they all must be strings)
55+
// See this issue here: https://github.com/microsoft/vscode-python/issues/11749
56+
const keys = Object.keys(copy.env);
57+
keys.forEach((k) => {
58+
if (copy.env) {
59+
const value = copy.env[k];
60+
if (value !== null && value !== undefined) {
61+
copy.env[k] = value.toString();
62+
}
63+
}
64+
});
65+
}
66+
67+
return copy as T;
68+
}

‎src/client/datascience/jupyter/kernels/kernelService.ts‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ import {
3232
IKernelDependencyService,
3333
KernelInterpreterDependencyResponse
3434
} from '../../types';
35-
import { detectDefaultKernelName } from './helpers';
35+
import { cleanEnvironment, detectDefaultKernelName } from './helpers';
3636
import { JupyterKernelSpec } from './jupyterKernelSpec';
3737
import { LiveKernelModel } from './types';
3838

@@ -395,14 +395,15 @@ export class KernelService {
395395
forceWrite?: boolean
396396
) {
397397
const specedKernel = kernel as JupyterKernelSpec;
398-
if (specedKernel.specFile && interpreter) {
399-
const specModel: ReadWrite<Kernel.ISpecModel> = JSON.parse(
398+
if (specedKernel.specFile) {
399+
let specModel: ReadWrite<Kernel.ISpecModel> = JSON.parse(
400400
await this.fileSystem.readFile(specedKernel.specFile)
401401
);
402+
let shouldUpdate = false;
402403

403404
// Make sure the specmodel has an interpreter or already in the metadata or we
404405
// may overwrite a kernel created by the user
405-
if (specModel.metadata?.interpreter || forceWrite) {
406+
if (interpreter && (specModel.metadata?.interpreter || forceWrite)) {
406407
// Ensure we use a fully qualified path to the python interpreter in `argv`.
407408
if (specModel.argv[0].toLowerCase() === 'conda') {
408409
// If conda is the first word, its possible its a conda activation command.
@@ -438,7 +439,19 @@ export class KernelService {
438439
// tslint:disable-next-line: no-any
439440
specModel.metadata.interpreter = interpreter as any;
440441

441-
// Update the kernel.json with our new stuff.
442+
// Indicate we need to write
443+
shouldUpdate = true;
444+
}
445+
446+
// Scrub the environment of the specmodel to make sure it has allowed values (they all must be strings)
447+
// See this issue here: https://github.com/microsoft/vscode-python/issues/11749
448+
if (specModel.env) {
449+
specModel = cleanEnvironment(specModel);
450+
shouldUpdate = true;
451+
}
452+
453+
// Update the kernel.json with our new stuff.
454+
if (shouldUpdate) {
442455
await this.fileSystem.writeFile(specedKernel.specFile, JSON.stringify(specModel, undefined, 2), {
443456
flag: 'w',
444457
encoding: 'utf8'

‎src/client/datascience/kernel-launcher/kernelProcess.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,11 @@ import { noop, swallowExceptions } from '../../common/utils/misc';
1414
import { PythonInterpreter } from '../../pythonEnvironments/discovery/types';
1515
import { captureTelemetry } from '../../telemetry';
1616
import { Telemetry } from '../constants';
17-
import { findIndexOfConnectionFile } from '../jupyter/kernels/helpers';
17+
import { cleanEnvironment, findIndexOfConnectionFile } from '../jupyter/kernels/helpers';
1818
import { IJupyterKernelSpec } from '../types';
1919
import { PythonKernelLauncherDaemon } from './kernelLauncherDaemon';
2020
import { IKernelConnection, IKernelProcess, IPythonKernelDaemon, PythonKernelDiedError } from './types';
2121

22-
// tslint:disable-next-line: no-require-imports
23-
import cloneDeep = require('lodash/cloneDeep');
2422
import { IFileSystem } from '../../common/platform/types';
2523
import { KernelDaemonPool } from './kernelDaemonPool';
2624

@@ -58,7 +56,7 @@ export class KernelProcess implements IKernelProcess {
5856
private readonly interpreter?: PythonInterpreter
5957
) {
6058
this.originalKernelSpec = kernelSpec;
61-
this._kernelSpec = cloneDeep(kernelSpec);
59+
this._kernelSpec = cleanEnvironment(kernelSpec);
6260
}
6361
public async interrupt(): Promise<void> {
6462
if (this.kernelDaemon) {

‎src/test/datascience/jupyter/kernels/kernelService.unit.test.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -585,7 +585,11 @@ suite('Data Science - KernelService', () => {
585585
const kernel = new JupyterKernelSpec(userKernelSpecModel, kernelJsonFile);
586586
when(jupyterInterpreterExecutionService.getKernelSpecs(anything())).thenResolve([kernel]);
587587
when(fs.readFile(kernelJsonFile)).thenResolve(JSON.stringify(userKernelSpecModel));
588-
when(fs.writeFile(kernelJsonFile, anything())).thenResolve();
588+
let contents: string | undefined;
589+
when(fs.writeFile(kernelJsonFile, anything(), anything())).thenCall((_f, c) => {
590+
contents = c;
591+
return Promise.resolve();
592+
});
589593
const envVariables = { MYVAR: '1' };
590594
when(activationHelper.getActivatedEnvironmentVariables(undefined, interpreter, true)).thenResolve(
591595
envVariables
@@ -596,7 +600,9 @@ suite('Data Science - KernelService', () => {
596600

597601
// tslint:disable-next-line: no-any
598602
assert.deepEqual(kernel, installedKernel as any);
599-
verify(fs.writeFile(anything(), anything(), anything())).never();
603+
assert.ok(contents, 'Env not updated');
604+
const obj = JSON.parse(contents!);
605+
assert.notOk(obj.metadata.interpreter, 'MetaData should not have been written');
600606
});
601607
});
602608
});

0 commit comments

Comments
 (0)