Use different method for checking if kernelspec is available (#10114)

* Use different method for checking if kernelspec is available

* Fix unit tests

Author: rchiodo

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()

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 }
+        );
     }
 }
 

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<boolean>}
      * @memberof JupyterInterpreterConfigurationService
      */
-    private async isKernelSpecAvailable(interpreter: PythonInterpreter, token?: CancellationToken): Promise<boolean> {
-        const execService = await this.pythonExecFactory.createActivatedEnvironment({
+    private async isKernelSpecAvailable(interpreter: PythonInterpreter, _token?: CancellationToken): Promise<boolean> {
+        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(() => {
                 sendTelemetryEvent(Telemetry.KernelSpecNotFound);
@@ -326,9 +325,10 @@ export class JupyterInterpreterDependencyService {
         token?: CancellationToken
     ): Promise<JupyterInterpreterDependencyResponse> {
         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;
         }

src/test/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.unit.test.ts

@@ -4,29 +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 {
+    InterpreterJupyterKernelSpecCommand,
+    JupyterCommandFactory
+} from '../../../../client/datascience/jupyter/interpreter/jupyterCommand';
 import {
     JupyterInterpreterDependencyResponse,
     JupyterInterpreterDependencyService
 } from '../../../../client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService';
+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 pythonExecService: IPythonExecutionService;
+    let commandFactory: IJupyterCommandFactory;
+    let command: IJupyterCommand;
     const pythonInterpreter: PythonInterpreter = {
         path: '',
         architecture: Architecture.Unknown,
@@ -37,19 +40,19 @@ suite('Data Science - Jupyter Interpreter Configuration', () => {
     setup(() => {
         appShell = mock(ApplicationShell);
         installer = mock(ProductInstaller);
-        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: ''
-        });
+        commandFactory = mock(JupyterCommandFactory);
+        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),
             instance(installer),
-            instance(pythonExecFactory)
+            instance(commandFactory)
         );
     });
     test('Return ok if all dependencies are installed', async () => {
@@ -91,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);