From ea0dc14716d4bb6dd8f78ed881b4f47c5821a447 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Tue, 5 Nov 2019 15:20:32 -0800 Subject: [PATCH] Correctly handle interpreter changes for Interactive and Notebook (#8385) --- news/2 Fixes/8019.md | 1 + .../interactive-common/interactiveBase.ts | 4 ++- .../liveshare/guestJupyterExecution.ts | 2 +- .../jupyter/liveshare/hostJupyterExecution.ts | 2 +- .../jupyter/liveshare/serverCache.ts | 12 +++++-- src/client/datascience/types.ts | 1 + .../datascience/dataScienceIocContainer.ts | 10 ++++-- .../datascience/notebook.functional.test.ts | 36 +++++++++++++++++++ 8 files changed, 59 insertions(+), 9 deletions(-) create mode 100644 news/2 Fixes/8019.md diff --git a/news/2 Fixes/8019.md b/news/2 Fixes/8019.md new file mode 100644 index 000000000000..66b2505089f1 --- /dev/null +++ b/news/2 Fixes/8019.md @@ -0,0 +1 @@ +Correctly restart jupyter sessions when the active interpreter is changed. \ No newline at end of file diff --git a/src/client/datascience/interactive-common/interactiveBase.ts b/src/client/datascience/interactive-common/interactiveBase.ts index d8a0915a49a3..bc569fc93d14 100644 --- a/src/client/datascience/interactive-common/interactiveBase.ts +++ b/src/client/datascience/interactive-common/interactiveBase.ts @@ -935,7 +935,9 @@ export abstract class InteractiveBase extends WebViewHost { // 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 { diff --git a/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts b/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts index f9427d34192d..0a1a7fddd96f 100644 --- a/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts +++ b/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 { diff --git a/src/client/datascience/jupyter/liveshare/hostJupyterExecution.ts b/src/client/datascience/jupyter/liveshare/hostJupyterExecution.ts index ce66abee96d6..f4dd6d50f17a 100644 --- a/src/client/datascience/jupyter/liveshare/hostJupyterExecution.ts +++ b/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 { diff --git a/src/client/datascience/jupyter/liveshare/serverCache.ts b/src/client/datascience/jupyter/liveshare/serverCache.ts index 714b02287fe7..99597935224f 100644 --- a/src/client/datascience/jupyter/liveshare/serverCache.ts +++ b/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 { @@ -60,13 +62,16 @@ export class ServerCache implements IAsyncDisposable { } public async generateDefaultOptions(options?: INotebookServerOptions): Promise { + 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}`; } } diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index d686c68adeb2..5741d71f7a13 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -107,6 +107,7 @@ export interface INotebookServerOptions { usingDarkTheme?: boolean; useDefaultConfig?: boolean; workingDir?: string; + interpreterPath?: string; purpose: string; } diff --git a/src/test/datascience/dataScienceIocContainer.ts b/src/test/datascience/dataScienceIocContainer.ts index af4c293c34e0..a36f39b99d3b 100644 --- a/src/test/datascience/dataScienceIocContainer.ts +++ b/src/test/datascience/dataScienceIocContainer.ts @@ -599,9 +599,7 @@ export class DataScienceIocContainer extends UnitTestIocContainer { const interpreterManager = this.serviceContainer.get(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' }); diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts index c6d31fbb9a82..483c28a21378 100644 --- a/src/test/datascience/notebook.functional.test.ts +++ b/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,6 +44,7 @@ import { import { IInterpreterService, IKnownSearchPathsForInterpreters, + InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts'; import { generateTestState, ICellViewModel } from '../../datascience-ui/interactive-common/mainState'; @@ -49,6 +52,7 @@ 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);