From 1e749ce4b76eac6da576c358ee6cc1a84201692b Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 13 Feb 2020 12:41:22 -0800 Subject: [PATCH 1/2] Use different method for checking if kernelspec is available --- .../getJupyterKernelspecVersion.py | 7 ++ .../jupyter/interpreter/jupyterCommand.ts | 64 +++++++++++++------ .../jupyterInterpreterDependencyService.ts | 28 ++++---- ...rInterpreterDependencyService.unit.test.ts | 6 +- 4 files changed, 69 insertions(+), 36 deletions(-) create mode 100644 pythonFiles/datascience/getJupyterKernelspecVersion.py diff --git a/pythonFiles/datascience/getJupyterKernelspecVersion.py b/pythonFiles/datascience/getJupyterKernelspecVersion.py new file mode 100644 index 000000000000..1dbcd7694bfc --- /dev/null +++ b/pythonFiles/datascience/getJupyterKernelspecVersion.py @@ -0,0 +1,7 @@ +# Check whether kernelspec module exists. +import sys +import jupyter_client +import jupyter_client.kernelspec + +sys.stdout.write(jupyter_client.__version__) +sys.stdout.flush() diff --git a/src/client/datascience/jupyter/interpreter/jupyterCommand.ts b/src/client/datascience/jupyter/interpreter/jupyterCommand.ts index 4b888be21dd5..567fefe25218 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterCommand.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterCommand.ts @@ -222,37 +222,59 @@ export class InterpreterJupyterKernelSpecCommand extends InterpreterJupyterComma return output; } - // We're only interested in `python -m jupyter kernelspec list --json` + const defaultAction = () => { + if (exception) { + throw exception; + } + return output; + }; + + // We're only interested in `python -m jupyter kernelspec` const interpreter = await this.interpreter(); if ( !interpreter || this.moduleName.toLowerCase() !== 'jupyter' || - this.args.join(' ').toLowerCase() !== `-m jupyter ${JupyterCommands.KernelSpecCommand}`.toLowerCase() || - args.join(' ').toLowerCase() !== 'list --json' + this.args.join(' ').toLowerCase() !== `-m jupyter ${JupyterCommands.KernelSpecCommand}`.toLowerCase() ) { - if (exception) { - throw exception; - } - return output; + return defaultAction(); } + + // Otherwise try running a script instead. try { - // Try getting kernels using python script, if that fails (even if there's output in stderr) rethrow original exception. - const activatedEnv = await this.pythonExecutionFactory.createActivatedEnvironment({ - interpreter, - bypassCondaExecution: true - }); - return activatedEnv.exec( - [path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'datascience', 'getJupyterKernels.py')], - { ...options, throwOnStdErr: true } - ); + if (args.join(' ').toLowerCase() === 'list --json') { + // Try getting kernels using python script, if that fails (even if there's output in stderr) rethrow original exception. + return this.getKernelSpecList(interpreter, options); + } else if (args.join(' ').toLowerCase() === '--version') { + // Try getting kernelspec version using python script, if that fails (even if there's output in stderr) rethrow original exception. + return this.getKernelSpecVersion(interpreter, options); + } } catch (innerEx) { traceError('Failed to get a list of the kernelspec using python script', innerEx); - // Rethrow original exception. - if (exception) { - throw exception; - } - return output; } + return defaultAction(); + } + + private async getKernelSpecList(interpreter: PythonInterpreter, options: SpawnOptions) { + // Try getting kernels using python script, if that fails (even if there's output in stderr) rethrow original exception. + const activatedEnv = await this.pythonExecutionFactory.createActivatedEnvironment({ + interpreter, + bypassCondaExecution: true + }); + return activatedEnv.exec( + [path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'datascience', 'getJupyterKernels.py')], + { ...options, throwOnStdErr: true } + ); + } + private async getKernelSpecVersion(interpreter: PythonInterpreter, options: SpawnOptions) { + // Try getting kernels using python script, if that fails (even if there's output in stderr) rethrow original exception. + const activatedEnv = await this.pythonExecutionFactory.createActivatedEnvironment({ + interpreter, + bypassCondaExecution: true + }); + return activatedEnv.exec( + [path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'datascience', 'getJupyterKernelspecVersion.py')], + { ...options, throwOnStdErr: true } + ); } } diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts index 43555442abe8..a9c97e21229b 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts @@ -8,13 +8,13 @@ import { CancellationToken } from 'vscode'; import { IApplicationShell } from '../../../common/application/types'; import { Cancellation, createPromiseFromCancellation, wrapCancellationTokens } from '../../../common/cancellation'; import { ProductNames } from '../../../common/installer/productNames'; -import { IPythonExecutionFactory } from '../../../common/process/types'; import { IInstaller, InstallerResponse, Product } from '../../../common/types'; import { Common, DataScience } from '../../../common/utils/localize'; import { noop } from '../../../common/utils/misc'; import { PythonInterpreter } from '../../../interpreter/contracts'; import { sendTelemetryEvent } from '../../../telemetry'; -import { HelpLinks, Telemetry } from '../../constants'; +import { HelpLinks, JupyterCommands, Telemetry } from '../../constants'; +import { IJupyterCommandFactory } from '../../types'; import { JupyterInstallError } from '../jupyterInstallError'; export enum JupyterInterpreterDependencyResponse { @@ -113,7 +113,7 @@ export class JupyterInterpreterDependencyService { constructor( @inject(IApplicationShell) private readonly applicationShell: IApplicationShell, @inject(IInstaller) private readonly installer: IInstaller, - @inject(IPythonExecutionFactory) private readonly pythonExecFactory: IPythonExecutionFactory + @inject(IJupyterCommandFactory) private readonly commandFactory: IJupyterCommandFactory ) {} /** * Configures the python interpreter to ensure it can run Jupyter server by installing any missing dependencies. @@ -291,17 +291,16 @@ export class JupyterInterpreterDependencyService { * @returns {Promise} * @memberof JupyterInterpreterConfigurationService */ - private async isKernelSpecAvailable(interpreter: PythonInterpreter, token?: CancellationToken): Promise { - const execService = await this.pythonExecFactory.createActivatedEnvironment({ + private async isKernelSpecAvailable(interpreter: PythonInterpreter, _token?: CancellationToken): Promise { + const command = this.commandFactory.createInterpreterCommand( + JupyterCommands.KernelSpecCommand, + 'jupyter', + ['-m', 'jupyter', 'kernelspec'], interpreter, - allowEnvironmentFetchExceptions: true, - bypassCondaExecution: true - }); - if (Cancellation.isCanceled(token)) { - return false; - } - return execService - .execModule('jupyter', ['kernelspec', '--version'], { throwOnStdErr: true }) + false + ); + return command + .exec(['--version'], { throwOnStdErr: true }) .then(() => true) .catch(() => false); } @@ -323,9 +322,10 @@ export class JupyterInterpreterDependencyService { token?: CancellationToken ): Promise { if (await this.isKernelSpecAvailable(interpreter)) { - sendTelemetryEvent(Telemetry.JupyterInstalledButNotKernelSpecModule); return JupyterInterpreterDependencyResponse.ok; } + // Indicate no kernel spec module. + sendTelemetryEvent(Telemetry.JupyterInstalledButNotKernelSpecModule); if (Cancellation.isCanceled(token)) { return JupyterInterpreterDependencyResponse.cancel; } diff --git a/src/test/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.unit.test.ts b/src/test/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.unit.test.ts index 667cab3fa017..36b8b3bce931 100644 --- a/src/test/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.unit.test.ts +++ b/src/test/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.unit.test.ts @@ -14,10 +14,12 @@ import { IPythonExecutionService } from '../../../../client/common/process/types import { IInstaller, InstallerResponse, Product } from '../../../../client/common/types'; import { DataScience } from '../../../../client/common/utils/localize'; import { Architecture } from '../../../../client/common/utils/platform'; +import { JupyterCommandFactory } from '../../../../client/datascience/jupyter/interpreter/jupyterCommand'; import { JupyterInterpreterDependencyResponse, JupyterInterpreterDependencyService } from '../../../../client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService'; +import { IJupyterCommandFactory } from '../../../../client/datascience/types'; import { InterpreterType, PythonInterpreter } from '../../../../client/interpreter/contracts'; // tslint:disable: max-func-body-length @@ -26,6 +28,7 @@ suite('Data Science - Jupyter Interpreter Configuration', () => { let configuration: JupyterInterpreterDependencyService; let appShell: IApplicationShell; let installer: IInstaller; + let commandFactory: IJupyterCommandFactory; let pythonExecService: IPythonExecutionService; const pythonInterpreter: PythonInterpreter = { path: '', @@ -37,6 +40,7 @@ suite('Data Science - Jupyter Interpreter Configuration', () => { setup(() => { appShell = mock(ApplicationShell); installer = mock(ProductInstaller); + commandFactory = mock(JupyterCommandFactory); pythonExecService = mock(PythonExecutionService); const pythonExecFactory = mock(PythonExecutionFactory); when(pythonExecFactory.createActivatedEnvironment(anything())).thenResolve(instance(pythonExecService)); @@ -49,7 +53,7 @@ suite('Data Science - Jupyter Interpreter Configuration', () => { configuration = new JupyterInterpreterDependencyService( instance(appShell), instance(installer), - instance(pythonExecFactory) + instance(commandFactory) ); }); test('Return ok if all dependencies are installed', async () => { From 9f82705efb49c5010de3c532ee7a5598fc20767d Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 13 Feb 2020 14:09:20 -0800 Subject: [PATCH 2/2] Fix unit tests --- ...rInterpreterDependencyService.unit.test.ts | 35 +++++++++---------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/src/test/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.unit.test.ts b/src/test/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.unit.test.ts index 36b8b3bce931..fc5ff3e1bd9c 100644 --- a/src/test/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.unit.test.ts +++ b/src/test/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.unit.test.ts @@ -4,32 +4,32 @@ 'use strict'; import { assert } from 'chai'; -import { anything, deepEqual, instance, mock, verify, when } from 'ts-mockito'; +import { anything, instance, mock, verify, when } from 'ts-mockito'; import { ApplicationShell } from '../../../../client/common/application/applicationShell'; import { IApplicationShell } from '../../../../client/common/application/types'; import { ProductInstaller } from '../../../../client/common/installer/productInstaller'; -import { PythonExecutionFactory } from '../../../../client/common/process/pythonExecutionFactory'; -import { PythonExecutionService } from '../../../../client/common/process/pythonProcess'; -import { IPythonExecutionService } from '../../../../client/common/process/types'; import { IInstaller, InstallerResponse, Product } from '../../../../client/common/types'; import { DataScience } from '../../../../client/common/utils/localize'; import { Architecture } from '../../../../client/common/utils/platform'; -import { JupyterCommandFactory } from '../../../../client/datascience/jupyter/interpreter/jupyterCommand'; +import { + InterpreterJupyterKernelSpecCommand, + JupyterCommandFactory +} from '../../../../client/datascience/jupyter/interpreter/jupyterCommand'; import { JupyterInterpreterDependencyResponse, JupyterInterpreterDependencyService } from '../../../../client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService'; -import { IJupyterCommandFactory } from '../../../../client/datascience/types'; +import { IJupyterCommand, IJupyterCommandFactory } from '../../../../client/datascience/types'; import { InterpreterType, PythonInterpreter } from '../../../../client/interpreter/contracts'; -// tslint:disable: max-func-body-length +// tslint:disable: max-func-body-length no-any suite('Data Science - Jupyter Interpreter Configuration', () => { let configuration: JupyterInterpreterDependencyService; let appShell: IApplicationShell; let installer: IInstaller; let commandFactory: IJupyterCommandFactory; - let pythonExecService: IPythonExecutionService; + let command: IJupyterCommand; const pythonInterpreter: PythonInterpreter = { path: '', architecture: Architecture.Unknown, @@ -41,14 +41,13 @@ suite('Data Science - Jupyter Interpreter Configuration', () => { appShell = mock(ApplicationShell); installer = mock(ProductInstaller); commandFactory = mock(JupyterCommandFactory); - pythonExecService = mock(PythonExecutionService); - const pythonExecFactory = mock(PythonExecutionFactory); - when(pythonExecFactory.createActivatedEnvironment(anything())).thenResolve(instance(pythonExecService)); - // tslint:disable-next-line: no-any - instance(pythonExecService as any).then = undefined; - when(pythonExecService.execModule('jupyter', deepEqual(['kernelspec', '--version']), anything())).thenResolve({ - stdout: '' - }); + command = mock(InterpreterJupyterKernelSpecCommand); + instance(commandFactory as any).then = undefined; + instance(command as any).then = undefined; + when( + commandFactory.createInterpreterCommand(anything(), anything(), anything(), anything(), anything()) + ).thenReturn(instance(command)); + when(command.exec(anything(), anything())).thenResolve({ stdout: '' }); configuration = new JupyterInterpreterDependencyService( instance(appShell), @@ -95,9 +94,7 @@ suite('Data Science - Jupyter Interpreter Configuration', () => { // tslint:disable-next-line: no-any DataScience.jupyterInstall() as any ); - when(pythonExecService.execModule('jupyter', deepEqual(['kernelspec', '--version']), anything())).thenReject( - new Error('Not found') - ); + when(command.exec(anything(), anything())).thenReject(new Error('Not found')); when(installer.install(anything(), anything(), anything())).thenResolve(InstallerResponse.Installed); const response = await configuration.installMissingDependencies(pythonInterpreter);