Correctly handle interpreter changes for Interactive and Notebook (#8385)

Author: IanMatthewHuff

news/2 Fixes/8019.md

@@ -0,0 +1 @@
+Correctly restart jupyter sessions when the active interpreter is changed.

src/client/datascience/interactive-common/interactiveBase.ts

@@ -935,7 +935,9 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
 
     private onInterpreterChanged = () => {
         // Update our load promise. We need to restart the jupyter server
-        this.loadPromise = this.reloadWithNew();
+        if (this.loadPromise) {
+            this.loadPromise = this.reloadWithNew();
+        }
     }
 
     private async stopServer(): Promise<void> {

src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts

@@ -63,7 +63,7 @@ export class GuestJupyterExecution extends LiveShareParticipantGuest(JupyterExec
             commandFactory,
             serviceContainer);
         asyncRegistry.push(this);
-        this.serverCache = new ServerCache(configuration, workspace, fileSystem);
+        this.serverCache = new ServerCache(configuration, workspace, fileSystem, interpreterService);
     }
 
     public async dispose(): Promise<void> {

src/client/datascience/jupyter/liveshare/hostJupyterExecution.ts

@@ -64,7 +64,7 @@ export class HostJupyterExecution
             configService,
             commandFactory,
             serviceContainer);
-        this.serverCache = new ServerCache(configService, workspace, fileSys);
+        this.serverCache = new ServerCache(configService, workspace, fileSys, interpreterService);
     }
 
     public async dispose(): Promise<void> {

src/client/datascience/jupyter/liveshare/serverCache.ts

@@ -9,6 +9,7 @@ import * as uuid from 'uuid/v4';
 import { IWorkspaceService } from '../../../common/application/types';
 import { IFileSystem } from '../../../common/platform/types';
 import { IAsyncDisposable, IConfigurationService } from '../../../common/types';
+import { IInterpreterService } from '../../../interpreter/contracts';
 import { INotebookServer, INotebookServerOptions } from '../../types';
 
 export class ServerCache implements IAsyncDisposable {
@@ -18,7 +19,8 @@ export class ServerCache implements IAsyncDisposable {
     constructor(
         private configService: IConfigurationService,
         private workspace: IWorkspaceService,
-        private fileSystem: IFileSystem
+        private fileSystem: IFileSystem,
+        private interpreterService: IInterpreterService
     ) { }
 
     public async get(options?: INotebookServerOptions): Promise<INotebookServer | undefined> {
@@ -60,13 +62,16 @@ export class ServerCache implements IAsyncDisposable {
     }
 
     public async generateDefaultOptions(options?: INotebookServerOptions): Promise<INotebookServerOptions> {
+        const activeInterpreter = await this.interpreterService.getActiveInterpreter();
+        const activeInterpreterPath = activeInterpreter ? activeInterpreter.path : undefined;
         return {
             enableDebugging: options ? options.enableDebugging : false,
             uri: options ? options.uri : undefined,
             useDefaultConfig: options ? options.useDefaultConfig : true, // Default for this is true.
             usingDarkTheme: options ? options.usingDarkTheme : undefined,
             purpose: options ? options.purpose : uuid(),
-            workingDir: options && options.workingDir ? options.workingDir : await this.calculateWorkingDirectory()
+            workingDir: options && options.workingDir ? options.workingDir : await this.calculateWorkingDirectory(),
+            interpreterPath: options && options.interpreterPath ? options.interpreterPath : activeInterpreterPath
         };
     }
 
@@ -76,11 +81,12 @@ export class ServerCache implements IAsyncDisposable {
         } else {
             // combine all the values together to make a unique key
             const uri = options.uri ? options.uri : '';
+            const interpreter = options.interpreterPath ? options.interpreterPath : '';
             const useFlag = options.useDefaultConfig ? 'true' : 'false';
             const debug = options.enableDebugging ? 'true' : 'false';
             // tslint:disable-next-line:no-suspicious-comment
             // TODO: Should there be some separator in the key?
-            return `${options.purpose}${uri}${useFlag}${options.workingDir}${debug}`;
+            return `${options.purpose}${uri}${useFlag}${options.workingDir}${debug}${interpreter}`;
         }
     }
 

src/client/datascience/types.ts

@@ -107,6 +107,7 @@ export interface INotebookServerOptions {
     usingDarkTheme?: boolean;
     useDefaultConfig?: boolean;
     workingDir?: string;
+    interpreterPath?: string;
     purpose: string;
 }
 

src/test/datascience/dataScienceIocContainer.ts

@@ -599,9 +599,7 @@ export class DataScienceIocContainer extends UnitTestIocContainer {
         const interpreterManager = this.serviceContainer.get<IInterpreterService>(IInterpreterService);
         interpreterManager.initialize();
 
-        if (this.mockJupyter) {
-            this.mockJupyter.addInterpreter(this.workingPython, SupportedCommands.all);
-        }
+        this.addInterpreter(this.workingPython, SupportedCommands.all);
     }
 
     // tslint:disable:any
@@ -718,6 +716,12 @@ export class DataScienceIocContainer extends UnitTestIocContainer {
         this.pythonSettings.jediEnabled = enabled;
     }
 
+    public addInterpreter(newInterpreter: PythonInterpreter, commands: SupportedCommands) {
+        if (this.mockJupyter) {
+            this.mockJupyter.addInterpreter(newInterpreter, commands);
+        }
+    }
+
     private findPythonPath(): string {
         try {
             const output = child_process.execFileSync('python', ['-c', 'import sys;print(sys.executable)'], { encoding: 'utf8' });

src/test/datascience/notebook.functional.test.ts

@@ -8,6 +8,7 @@ import * as fs from 'fs-extra';
 import { injectable } from 'inversify';
 import * as os from 'os';
 import * as path from 'path';
+import { SemVer } from 'semver';
 import { Readable, Writable } from 'stream';
 import { anything, instance, mock, when } from 'ts-mockito';
 import * as TypeMoq from 'typemoq';
@@ -23,6 +24,7 @@ import { IFileSystem } from '../../client/common/platform/types';
 import { IProcessServiceFactory, IPythonExecutionFactory, Output } from '../../client/common/process/types';
 import { createDeferred, waitForPromise } from '../../client/common/utils/async';
 import { noop } from '../../client/common/utils/misc';
+import { Architecture } from '../../client/common/utils/platform';
 import { concatMultilineStringInput } from '../../client/datascience/common';
 import { Identifiers } from '../../client/datascience/constants';
 import { JupyterExecutionFactory } from '../../client/datascience/jupyter/jupyterExecutionFactory';
@@ -42,13 +44,15 @@ import {
 import {
     IInterpreterService,
     IKnownSearchPathsForInterpreters,
+    InterpreterType,
     PythonInterpreter
 } from '../../client/interpreter/contracts';
 import { generateTestState, ICellViewModel } from '../../datascience-ui/interactive-common/mainState';
 import { asyncDump } from '../common/asyncDump';
 import { sleep } from '../core';
 import { DataScienceIocContainer } from './dataScienceIocContainer';
 import { getConnectionInfo, getIPConnectionInfo, getNotebookCapableInterpreter } from './jupyterHelpers';
+import { SupportedCommands } from './mockJupyterManager';
 import { MockPythonService } from './mockPythonService';
 
 // tslint:disable:no-any no-multiline-string max-func-body-length no-console max-classes-per-file trailing-comma
@@ -519,6 +523,38 @@ suite('DataScience notebook tests', () => {
         }
     });
 
+    runTest('Change Interpreter', async () => {
+        const isRollingBuild = process.env ? process.env.VSCODE_PYTHON_ROLLING !== undefined : false;
+
+        // Real Jupyter doesn't help this test at all and is tricky to set up for it, so just skip it
+        if (!isRollingBuild) {
+            const server = await createNotebook(true);
+
+            // Create again, we should get the same server from the cache
+            const server2 = await createNotebook(true);
+            assert.equal(server, server2, 'With no settings changed we should return the cached server');
+
+            // Create a new mock interpreter with a different path
+            const newPython: PythonInterpreter = {
+                path: '/foo/bar/baz/python.exe',
+                version: new SemVer('3.6.6-final'),
+                sysVersion: '1.0.0.0',
+                sysPrefix: 'Python',
+                type: InterpreterType.Unknown,
+                architecture: Architecture.x64,
+            };
+
+            // Add interpreter into mock jupyter service and set it as active
+            ioc.addInterpreter(newPython, SupportedCommands.all);
+
+            // Create a new notebook, we should not be the same anymore
+            const server3 = await createNotebook(true);
+            assert.notEqual(server, server3, 'With interpreter changed we should return a new server');
+        } else {
+            console.log(`Skipping Change Interpreter test in non-mocked Jupyter case`);
+        }
+    });
+
     runTest('Restart kernel', async () => {
         addMockData(`a=1${os.EOL}a`, 1);
         addMockData(`a+=1${os.EOL}a`, 2);