From 762e416eab9ddc5814225f356959c044509b4467 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 11 Jun 2020 11:40:59 -0700 Subject: [PATCH 1/5] Fix environment values that aren't strings. --- news/2 Fixes/11749.md | 1 + .../jupyter/kernels/kernelService.ts | 27 ++++++++++++++++--- .../kernel-launcher/kernelProcess.ts | 21 ++++++++++++++- 3 files changed, 45 insertions(+), 4 deletions(-) create mode 100644 news/2 Fixes/11749.md diff --git a/news/2 Fixes/11749.md b/news/2 Fixes/11749.md new file mode 100644 index 000000000000..aadd3d2e81f5 --- /dev/null +++ b/news/2 Fixes/11749.md @@ -0,0 +1 @@ +Jupyter can fail to run a kernel if the user's environment contains non string values. \ No newline at end of file diff --git a/src/client/datascience/jupyter/kernels/kernelService.ts b/src/client/datascience/jupyter/kernels/kernelService.ts index 4aad1d4128e3..657c93e0067b 100644 --- a/src/client/datascience/jupyter/kernels/kernelService.ts +++ b/src/client/datascience/jupyter/kernels/kernelService.ts @@ -395,14 +395,15 @@ export class KernelService { forceWrite?: boolean ) { const specedKernel = kernel as JupyterKernelSpec; - if (specedKernel.specFile && interpreter) { + if (specedKernel.specFile) { const specModel: ReadWrite = JSON.parse( await this.fileSystem.readFile(specedKernel.specFile) ); + 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 - if (specModel.metadata?.interpreter || forceWrite) { + if (interpreter && (specModel.metadata?.interpreter || forceWrite)) { // Ensure we use a fully qualified path to the python interpreter in `argv`. if (specModel.argv[0].toLowerCase() === 'conda') { // If conda is the first word, its possible its a conda activation command. @@ -438,7 +439,27 @@ export class KernelService { // tslint:disable-next-line: no-any specModel.metadata.interpreter = interpreter as any; - // Update the kernel.json with our new stuff. + // Indicate we need to write + shouldUpdate = true; + } + + // 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) { + const keys = Object.keys(specModel.env); + keys.forEach((k) => { + if (specModel.env) { + const value = specModel.env[k]; + if (value !== null && value !== undefined) { + specModel.env[k] = value.toString(); + } + } + }); + shouldUpdate = true; + } + + // Update the kernel.json with our new stuff. + if (shouldUpdate) { await this.fileSystem.writeFile(specedKernel.specFile, JSON.stringify(specModel, undefined, 2), { flag: 'w', encoding: 'utf8' diff --git a/src/client/datascience/kernel-launcher/kernelProcess.ts b/src/client/datascience/kernel-launcher/kernelProcess.ts index 0f277ece37c2..e54243cc57ae 100644 --- a/src/client/datascience/kernel-launcher/kernelProcess.ts +++ b/src/client/datascience/kernel-launcher/kernelProcess.ts @@ -58,7 +58,7 @@ export class KernelProcess implements IKernelProcess { private readonly interpreter?: PythonInterpreter ) { this.originalKernelSpec = kernelSpec; - this._kernelSpec = cloneDeep(kernelSpec); + this._kernelSpec = this.clean(kernelSpec); } public async interrupt(): Promise { if (this.kernelDaemon) { @@ -259,4 +259,23 @@ export class KernelProcess implements IKernelProcess { this._process = exeObs.proc; return exeObs; } + + private clean(kernelSpec: IJupyterKernelSpec): IJupyterKernelSpec { + const copy = cloneDeep(kernelSpec); + if (copy.env) { + // Scrub the environment of the spec to make sure it has allowed values (they all must be strings) + // See this issue here: https://github.com/microsoft/vscode-python/issues/11749 + const keys = Object.keys(copy.env); + keys.forEach((k) => { + if (copy.env) { + const value = copy.env[k]; + if (value !== null && value !== undefined) { + copy.env[k] = value.toString(); + } + } + }); + } + + return copy; + } } From 711ba9a5725d9d7889ff65d53e17f2236dfc6e19 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 11 Jun 2020 11:53:28 -0700 Subject: [PATCH 2/5] Consolidate in one function --- .../datascience/jupyter/kernels/helpers.ts | 25 +++++++++++++++++++ .../jupyter/kernels/kernelService.ts | 14 +++-------- .../kernel-launcher/kernelProcess.ts | 25 ++----------------- 3 files changed, 30 insertions(+), 34 deletions(-) diff --git a/src/client/datascience/jupyter/kernels/helpers.ts b/src/client/datascience/jupyter/kernels/helpers.ts index 432308adb0b8..1cd475a7ea8a 100644 --- a/src/client/datascience/jupyter/kernels/helpers.ts +++ b/src/client/datascience/jupyter/kernels/helpers.ts @@ -8,6 +8,11 @@ import { JupyterKernelSpec } from './jupyterKernelSpec'; // tslint:disable-next-line: no-var-requires no-require-imports const NamedRegexp = require('named-js-regexp') as typeof import('named-js-regexp'); +import { JSONObject } from '@phosphor/coreutils'; + +// tslint:disable-next-line: no-require-imports +import cloneDeep = require('lodash/cloneDeep'); + // Helper functions for dealing with kernels and kernelspecs export const defaultKernelSpecName = 'python_defaultSpec_'; @@ -42,3 +47,23 @@ export function detectDefaultKernelName(name: string) { const regEx = NamedRegexp('python\\s*(?(\\d+))', 'g'); return regEx.exec(name.toLowerCase()); } + +export function cleanEnvironment(spec: T): T { + const copy = cloneDeep(spec) as { env?: JSONObject }; + + if (copy.env) { + // Scrub the environment of the spec to make sure it has allowed values (they all must be strings) + // See this issue here: https://github.com/microsoft/vscode-python/issues/11749 + const keys = Object.keys(copy.env); + keys.forEach((k) => { + if (copy.env) { + const value = copy.env[k]; + if (value !== null && value !== undefined) { + copy.env[k] = value.toString(); + } + } + }); + } + + return copy as T; +} diff --git a/src/client/datascience/jupyter/kernels/kernelService.ts b/src/client/datascience/jupyter/kernels/kernelService.ts index 657c93e0067b..bdcc78ee807b 100644 --- a/src/client/datascience/jupyter/kernels/kernelService.ts +++ b/src/client/datascience/jupyter/kernels/kernelService.ts @@ -32,7 +32,7 @@ import { IKernelDependencyService, KernelInterpreterDependencyResponse } from '../../types'; -import { detectDefaultKernelName } from './helpers'; +import { cleanEnvironment, detectDefaultKernelName } from './helpers'; import { JupyterKernelSpec } from './jupyterKernelSpec'; import { LiveKernelModel } from './types'; @@ -396,7 +396,7 @@ export class KernelService { ) { const specedKernel = kernel as JupyterKernelSpec; if (specedKernel.specFile) { - const specModel: ReadWrite = JSON.parse( + let specModel: ReadWrite = JSON.parse( await this.fileSystem.readFile(specedKernel.specFile) ); let shouldUpdate = false; @@ -446,15 +446,7 @@ export class KernelService { // 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) { - const keys = Object.keys(specModel.env); - keys.forEach((k) => { - if (specModel.env) { - const value = specModel.env[k]; - if (value !== null && value !== undefined) { - specModel.env[k] = value.toString(); - } - } - }); + specModel = cleanEnvironment(specModel); shouldUpdate = true; } diff --git a/src/client/datascience/kernel-launcher/kernelProcess.ts b/src/client/datascience/kernel-launcher/kernelProcess.ts index e54243cc57ae..01044b7c27d0 100644 --- a/src/client/datascience/kernel-launcher/kernelProcess.ts +++ b/src/client/datascience/kernel-launcher/kernelProcess.ts @@ -14,13 +14,11 @@ import { noop, swallowExceptions } from '../../common/utils/misc'; import { PythonInterpreter } from '../../pythonEnvironments/discovery/types'; import { captureTelemetry } from '../../telemetry'; import { Telemetry } from '../constants'; -import { findIndexOfConnectionFile } from '../jupyter/kernels/helpers'; +import { cleanEnvironment, findIndexOfConnectionFile } from '../jupyter/kernels/helpers'; import { IJupyterKernelSpec } from '../types'; import { PythonKernelLauncherDaemon } from './kernelLauncherDaemon'; import { IKernelConnection, IKernelProcess, IPythonKernelDaemon, PythonKernelDiedError } from './types'; -// tslint:disable-next-line: no-require-imports -import cloneDeep = require('lodash/cloneDeep'); import { IFileSystem } from '../../common/platform/types'; import { KernelDaemonPool } from './kernelDaemonPool'; @@ -58,7 +56,7 @@ export class KernelProcess implements IKernelProcess { private readonly interpreter?: PythonInterpreter ) { this.originalKernelSpec = kernelSpec; - this._kernelSpec = this.clean(kernelSpec); + this._kernelSpec = cleanEnvironment(kernelSpec); } public async interrupt(): Promise { if (this.kernelDaemon) { @@ -259,23 +257,4 @@ export class KernelProcess implements IKernelProcess { this._process = exeObs.proc; return exeObs; } - - private clean(kernelSpec: IJupyterKernelSpec): IJupyterKernelSpec { - const copy = cloneDeep(kernelSpec); - if (copy.env) { - // Scrub the environment of the spec to make sure it has allowed values (they all must be strings) - // See this issue here: https://github.com/microsoft/vscode-python/issues/11749 - const keys = Object.keys(copy.env); - keys.forEach((k) => { - if (copy.env) { - const value = copy.env[k]; - if (value !== null && value !== undefined) { - copy.env[k] = value.toString(); - } - } - }); - } - - return copy; - } } From 66eee23cf8da75d08878e2199af4beb58a5cc773 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 11 Jun 2020 12:39:43 -0700 Subject: [PATCH 3/5] Fix unit test --- src/test/datascience/jupyter/kernels/kernelService.unit.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/datascience/jupyter/kernels/kernelService.unit.test.ts b/src/test/datascience/jupyter/kernels/kernelService.unit.test.ts index c098a5e71c57..dec153e60654 100644 --- a/src/test/datascience/jupyter/kernels/kernelService.unit.test.ts +++ b/src/test/datascience/jupyter/kernels/kernelService.unit.test.ts @@ -596,7 +596,6 @@ suite('Data Science - KernelService', () => { // tslint:disable-next-line: no-any assert.deepEqual(kernel, installedKernel as any); - verify(fs.writeFile(anything(), anything(), anything())).never(); }); }); }); From 6d688fae7fe6f23affc69d48620699395d379d52 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 11 Jun 2020 12:51:18 -0700 Subject: [PATCH 4/5] Make unit test verify env --- .../jupyter/kernels/kernelService.unit.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/test/datascience/jupyter/kernels/kernelService.unit.test.ts b/src/test/datascience/jupyter/kernels/kernelService.unit.test.ts index dec153e60654..04df9371d4e7 100644 --- a/src/test/datascience/jupyter/kernels/kernelService.unit.test.ts +++ b/src/test/datascience/jupyter/kernels/kernelService.unit.test.ts @@ -585,7 +585,11 @@ suite('Data Science - KernelService', () => { const kernel = new JupyterKernelSpec(userKernelSpecModel, kernelJsonFile); when(jupyterInterpreterExecutionService.getKernelSpecs(anything())).thenResolve([kernel]); when(fs.readFile(kernelJsonFile)).thenResolve(JSON.stringify(userKernelSpecModel)); - when(fs.writeFile(kernelJsonFile, anything())).thenResolve(); + let contents: string | undefined; + when(fs.writeFile(kernelJsonFile, anything(), anything())).thenCall((_f, c) => { + contents = c; + return Promise.resolve(); + }); const envVariables = { MYVAR: '1' }; when(activationHelper.getActivatedEnvironmentVariables(undefined, interpreter, true)).thenResolve( envVariables @@ -596,6 +600,9 @@ suite('Data Science - KernelService', () => { // tslint:disable-next-line: no-any assert.deepEqual(kernel, installedKernel as any); + assert.ok(contents, 'Env not updated'); + const obj = JSON.parse(contents!); + assert.notOk(obj.metadata.interpreter, 'MetaData should not have been written'); }); }); }); From cf5ef59bbd5989636e460e0396814c9d2a9f124e Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 11 Jun 2020 16:27:30 -0700 Subject: [PATCH 5/5] Review feedback --- src/client/datascience/jupyter/kernels/helpers.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/client/datascience/jupyter/kernels/helpers.ts b/src/client/datascience/jupyter/kernels/helpers.ts index 1cd475a7ea8a..379dfe1fd5ee 100644 --- a/src/client/datascience/jupyter/kernels/helpers.ts +++ b/src/client/datascience/jupyter/kernels/helpers.ts @@ -8,8 +8,6 @@ import { JupyterKernelSpec } from './jupyterKernelSpec'; // tslint:disable-next-line: no-var-requires no-require-imports const NamedRegexp = require('named-js-regexp') as typeof import('named-js-regexp'); -import { JSONObject } from '@phosphor/coreutils'; - // tslint:disable-next-line: no-require-imports import cloneDeep = require('lodash/cloneDeep'); @@ -49,7 +47,8 @@ export function detectDefaultKernelName(name: string) { } export function cleanEnvironment(spec: T): T { - const copy = cloneDeep(spec) as { env?: JSONObject }; + // tslint:disable-next-line: no-any + const copy = cloneDeep(spec) as { env?: any }; if (copy.env) { // Scrub the environment of the spec to make sure it has allowed values (they all must be strings)