Skip to content

Raw kernel auto start fix + #12337

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
IanMatthewHuff merged 6 commits into from
Jun 16, 2020
Merged

Raw kernel auto start fix + #12337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3c267cd
autoStartBasicFix
Jun 12, 2020
57ce443
jupyter execution removed
Jun 12, 2020
328f563
prereview cleanup
Jun 12, 2020
bb7db51
add news
Jun 12, 2020
7107058
remove old comment
Jun 12, 2020
8d27478
review feedback
Jun 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/2 Fixes/12330.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix raw kernel autostart and remove jupyter execution from interactive base.
44 changes: 2 additions & 42 deletions src/client/datascience/interactive-common/interactiveBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ import {
IInteractiveWindowInfo,
IInteractiveWindowListener,
IJupyterDebugger,
IJupyterExecution,
IJupyterKernelSpec,
IJupyterVariableDataProviderFactory,
IJupyterVariables,
Expand Down Expand Up @@ -134,7 +133,6 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
@unmanaged() cssGenerator: ICodeCssGenerator,
@unmanaged() themeFinder: IThemeFinder,
@unmanaged() private statusProvider: IStatusProvider,
@unmanaged() protected jupyterExecution: IJupyterExecution,
@unmanaged() protected fileSystem: IFileSystem,
@unmanaged() protected configuration: IConfigurationService,
@unmanaged() protected jupyterExporter: INotebookExporter,
Expand Down Expand Up @@ -178,9 +176,6 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
const handler = this.documentManager.onDidChangeActiveTextEditor(() => this.activating());
this.disposables.push(handler);

// If our execution changes its liveshare session, we need to close our server
this.jupyterExecution.sessionChanged(() => this.reloadAfterShutdown());

// For each listener sign up for their post events
this.listeners.forEach((l) => l.postMessage((e) => this.postMessageInternal(e.message, e.payload)));
// Channel for listeners to send messages to the interactive base.
Expand All @@ -199,8 +194,8 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
.ignoreErrors();
}, 0);

// When a server starts, make sure we create a notebook if the server matches
jupyterExecution.serverStarted(this.checkForNotebookProviderConnection.bind(this));
// When a notebook provider first makes its connection check it to see if we should create a notebook
this.disposables.push(notebookProvider.onConnectionMade(this.checkForNotebookProviderConnection.bind(this)));

@IanMatthewHuff IanMatthewHuff Jun 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change here is the crux of the fix. Our autostart preload is handled in serverLoad.ts for two main cases.

  1. At activation if Interactive window or notebook editor has been used in the last 7 days
  2. At notebook editor load

After my fix to honor the disableJupyterAutoStart setting raw kernel was respecting case #1 above (which is why we were not initially seeing the issue, for all of us #1 would apply basically all of the time) but we were not honoring #2. Case two is handled by this change here.

But more changes here were for code health cleanup as well. The interactiveBase is supposed to be communicating the the notebook provider for getting a notebook and associated info. It's very crummy coupling for it to have awareness of jupyterExecution and to be referencing it. So in addition to this change I've also made changes to remove jupyterExecution fully from interactiveBase.


// When the variable service requests a refresh, refresh our variable list
this.disposables.push(this.jupyterVariables.refreshRequired(this.refreshVariables.bind(this)));
Expand Down Expand Up @@ -1046,27 +1041,6 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
sendTelemetryEvent(event);
};

private async stopServer(): Promise<void> {
// Finish either of our notebook promises
if (this.connectionAndNotebookPromise) {
await this.connectionAndNotebookPromise;
this.connectionAndNotebookPromise = undefined;
}
if (this.notebookPromise) {
await this.notebookPromise;
this.notebookPromise = undefined;
}
// If we have a notebook dispose of it
if (this._notebook) {
const notebook = this._notebook;
this._notebook = undefined;
await notebook.dispose();
}

// Disconnect from our notebook provider
await this.notebookProvider.disconnect({ getOnly: true });
}

// ensureNotebook can be called apart from ensureNotebookAndServer and it needs
// the same protection to not be called twice
// tslint:disable-next-line: member-ordering
Expand Down Expand Up @@ -1205,20 +1179,6 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
}
}

private async reloadAfterShutdown(): Promise<void> {
try {
await this.stopServer();
} catch {
// We just switched from host to guest mode. Don't really care
// if closing the host server kills it.
this._notebook = undefined;
} finally {
this.connectionAndNotebookPromise = undefined;
this.notebookPromise = undefined;
}
await this.ensureConnectionAndNotebook();
}

@captureTelemetry(Telemetry.GotoSourceCode, undefined, false)
private gotoCode(args: IGotoCode) {
this.gotoCodeInternal(args.file, args.line).catch((err) => {
Expand Down
19 changes: 17 additions & 2 deletions src/client/datascience/interactive-common/notebookProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
export class NotebookProvider implements INotebookProvider {
private readonly notebooks = new Map<string, Promise<INotebook>>();
private _notebookCreated = new EventEmitter<{ identity: Uri; notebook: INotebook }>();
private _connectionMade = new EventEmitter<void>();
private _type: 'jupyter' | 'raw' = 'jupyter';
public get activeNotebooks() {
return [...this.notebooks.values()];
Expand Down Expand Up @@ -56,6 +57,10 @@ export class NotebookProvider implements INotebookProvider {
return this._notebookCreated.event;
}

public get onConnectionMade() {
return this._connectionMade.event;
}

public get type(): 'jupyter' | 'raw' {
return this._type;
}
Expand All @@ -72,9 +77,15 @@ export class NotebookProvider implements INotebookProvider {
public async connect(options: ConnectNotebookProviderOptions): Promise<INotebookProviderConnection | undefined> {
// Connect to either a jupyter server or a stubbed out raw notebook "connection"
if (await this.rawNotebookProvider.supported()) {
return this.rawNotebookProvider.connect(options);
return this.rawNotebookProvider.connect({
...options,
onConnectionMade: this.fireConnectionMade.bind(this)
});
} else {
return this.jupyterNotebookProvider.connect(options);
return this.jupyterNotebookProvider.connect({
...options,
onConnectionMade: this.fireConnectionMade.bind(this)
});
}
}

Expand Down Expand Up @@ -134,6 +145,10 @@ export class NotebookProvider implements INotebookProvider {
return promise;
}

private fireConnectionMade() {
this._connectionMade.fire();
}

// Cache the promise that will return a notebook
private cacheNotebookPromise(identity: Uri, promise: Promise<INotebook>) {
this.notebooks.set(identity.fsPath, promise);
Expand Down
9 changes: 8 additions & 1 deletion src/client/datascience/interactive-common/notebookServerProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,14 @@ export class NotebookServerProvider implements IJupyterServerProvider {
return this.jupyterExecution.getServer(serverOptions);
} else {
// Otherwise create a new server
return this.createServer(options, token);
return this.createServer(options, token).then((val) => {
// If we created a new server notify of our first time provider connection
if (val && options.onConnectionMade) {
options.onConnectionMade();
}

return val;
});
}
}

Expand Down
3 changes: 0 additions & 3 deletions src/client/datascience/interactive-ipynb/nativeEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ import {
IInteractiveWindowInfo,
IInteractiveWindowListener,
IJupyterDebugger,
IJupyterExecution,
IJupyterKernelSpec,
IJupyterVariableDataProviderFactory,
IJupyterVariables,
Expand Down Expand Up @@ -160,7 +159,6 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
@inject(ICodeCssGenerator) cssGenerator: ICodeCssGenerator,
@inject(IThemeFinder) themeFinder: IThemeFinder,
@inject(IStatusProvider) statusProvider: IStatusProvider,
@inject(IJupyterExecution) jupyterExecution: IJupyterExecution,
@inject(IFileSystem) fileSystem: IFileSystem,
@inject(IConfigurationService) configuration: IConfigurationService,
@inject(ICommandManager) commandManager: ICommandManager,
Expand Down Expand Up @@ -193,7 +191,6 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
cssGenerator,
themeFinder,
statusProvider,
jupyterExecution,
fileSystem,
configuration,
jupyterExporter,
Expand Down
3 changes: 0 additions & 3 deletions src/client/datascience/interactive-ipynb/nativeEditorOldWebView.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import {
IDataScienceErrorHandler,
IInteractiveWindowListener,
IJupyterDebugger,
IJupyterExecution,
IJupyterVariableDataProviderFactory,
IJupyterVariables,
INotebookEditorProvider,
Expand Down Expand Up @@ -81,7 +80,6 @@ export class NativeEditorOldWebView extends NativeEditor {
@inject(ICodeCssGenerator) cssGenerator: ICodeCssGenerator,
@inject(IThemeFinder) themeFinder: IThemeFinder,
@inject(IStatusProvider) statusProvider: IStatusProvider,
@inject(IJupyterExecution) jupyterExecution: IJupyterExecution,
@inject(IFileSystem) fileSystem: IFileSystem,
@inject(IConfigurationService) configuration: IConfigurationService,
@inject(ICommandManager) commandManager: ICommandManager,
Expand Down Expand Up @@ -115,7 +113,6 @@ export class NativeEditorOldWebView extends NativeEditor {
cssGenerator,
themeFinder,
statusProvider,
jupyterExecution,
fileSystem,
configuration,
commandManager,
Expand Down
3 changes: 0 additions & 3 deletions src/client/datascience/interactive-window/interactiveWindow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ import {
IInteractiveWindowListener,
IInteractiveWindowProvider,
IJupyterDebugger,
IJupyterExecution,
IJupyterKernelSpec,
IJupyterVariableDataProviderFactory,
IJupyterVariables,
Expand Down Expand Up @@ -96,7 +95,6 @@ export class InteractiveWindow extends InteractiveBase implements IInteractiveWi
@inject(IDisposableRegistry) disposables: IDisposableRegistry,
@inject(ICodeCssGenerator) cssGenerator: ICodeCssGenerator,
@inject(IThemeFinder) themeFinder: IThemeFinder,
@inject(IJupyterExecution) jupyterExecution: IJupyterExecution,
@inject(IFileSystem) fileSystem: IFileSystem,
@inject(IConfigurationService) configuration: IConfigurationService,
@inject(ICommandManager) commandManager: ICommandManager,
Expand Down Expand Up @@ -127,7 +125,6 @@ export class InteractiveWindow extends InteractiveBase implements IInteractiveWi
cssGenerator,
themeFinder,
statusProvider,
jupyterExecution,
fileSystem,
configuration,
jupyterExporter,
Expand Down
5 changes: 0 additions & 5 deletions src/client/datascience/jupyter/jupyterExecution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ const LocalHosts = ['localhost', '127.0.0.1', '::1'];

export class JupyterExecutionBase implements IJupyterExecution {
private usablePythonInterpreter: PythonInterpreter | undefined;
private eventEmitter: EventEmitter<void> = new EventEmitter<void>();
private startedEmitter: EventEmitter<INotebookServerOptions> = new EventEmitter<INotebookServerOptions>();

@IanMatthewHuff IanMatthewHuff Jun 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In interactiveBase.ts you'll notice that I also removed a usage of jupyterExecution that was watching this event. Turns out this event has never (or at least when I went back a year in the code) been hooked up. It's here, but it's never having .fire() called on it. So I removed it and the associated code in a couple of different places.

This is part of LiveShare, but I checked that the live share tests passed, and as this has never been hooked up I feel ok removing it.

private disposed: boolean = false;
private readonly jupyterInterpreterService: IJupyterSubCommandExecutionService;
Expand Down Expand Up @@ -71,10 +70,6 @@ export class JupyterExecutionBase implements IJupyterExecution {
}
}

public get sessionChanged(): Event<void> {
return this.eventEmitter.event;
}

public get serverStarted(): Event<INotebookServerOptions> {
return this.startedEmitter.event;
}
Expand Down
5 changes: 5 additions & 0 deletions src/client/datascience/raw-kernel/rawNotebookProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ export class RawNotebookProviderBase implements IRawNotebookProvider {
// If not get only, create if needed and return
if (!this.rawConnection) {
this.rawConnection = new RawConnection();

// Fire our optional event that we have created a connection
if (options.onConnectionMade) {
options.onConnectionMade();
}
}
return Promise.resolve(this.rawConnection);
}
Expand Down
8 changes: 7 additions & 1 deletion src/client/datascience/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ export type ConnectNotebookProviderOptions = {
disableUI?: boolean;
localOnly?: boolean;
token?: CancellationToken;
onConnectionMade?(): void; // Optional callback for when the first connection is made
};

export interface INotebookServerOptions {
Expand Down Expand Up @@ -280,7 +281,6 @@ export interface IGatherLogger extends INotebookExecutionLogger {

export const IJupyterExecution = Symbol('IJupyterExecution');
export interface IJupyterExecution extends IAsyncDisposable {
sessionChanged: Event<void>;
serverStarted: Event<INotebookServerOptions | undefined>;
isNotebookSupported(cancelToken?: CancellationToken): Promise<boolean>;
isImportSupported(cancelToken?: CancellationToken): Promise<boolean>;
Expand Down Expand Up @@ -1052,6 +1052,7 @@ export type GetServerOptions = {
disableUI?: boolean;
localOnly?: boolean;
token?: CancellationToken;
onConnectionMade?(): void; // Optional callback for when the first connection is made
};

/**
Expand All @@ -1073,6 +1074,11 @@ export interface INotebookProvider {
*/
onNotebookCreated: Event<{ identity: Uri; notebook: INotebook }>;

/**
* Fired just the first time that this provider connects
*/
onConnectionMade: Event<void>;
Comment thread
IanMatthewHuff marked this conversation as resolved.

/**
* List of all notebooks (active and ones that are being constructed).
*/
Expand Down
3 changes: 0 additions & 3 deletions src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,6 @@ suite('DataScience - Native Editor Provider', () => {
const editorChangeEvent = new EventEmitter<TextEditor | undefined>();
when(docManager.onDidChangeActiveTextEditor).thenReturn(editorChangeEvent.event);

const sessionChangedEvent = new EventEmitter<void>();
when(executionProvider.sessionChanged).thenReturn(sessionChangedEvent.event);

const serverStartedEvent = new EventEmitter<INotebookServerOptions>();
when(executionProvider.serverStarted).thenReturn(serverStartedEvent.event);

Expand Down
3 changes: 0 additions & 3 deletions src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,6 @@ suite('DataScience - Native Editor Storage', () => {
const editorChangeEvent = new EventEmitter<TextEditor | undefined>();
when(docManager.onDidChangeActiveTextEditor).thenReturn(editorChangeEvent.event);

const sessionChangedEvent = new EventEmitter<void>();
when(executionProvider.sessionChanged).thenReturn(sessionChangedEvent.event);

const serverStartedEvent = new EventEmitter<INotebookServerOptions>();
when(executionProvider.serverStarted).thenReturn(serverStartedEvent.event);

Expand Down
8 changes: 4 additions & 4 deletions src/test/datascience/uiTests/ipywidget.ui.functional.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ use(chaiAsPromised);
assert.include(outputHtml, '<input type="text');
});
});
test('Checkox Widget', async () => {
test('Checkbox Widget', async () => {
const { notebookUI } = await openStandardWidgetsIpynb();
await assert.eventually.isFalse(notebookUI.cellHasOutput(2));

Expand Down Expand Up @@ -232,7 +232,7 @@ use(chaiAsPromised);
assert.include(cellOutput, 'World</td>');
});
});
test('Widget renderes after closing and re-opening notebook', async () => {
test('Widget renders after closing and re-opening notebook', async () => {
const result = await openNotebookAndTestSliderWidget();

await result.notebookUI.page?.close();
Expand All @@ -241,7 +241,7 @@ use(chaiAsPromised);
// Open the same notebook again and test.
await openNotebookAndTestSliderWidget();
});
test('Widget renderes after restarting kernel', async () => {
test('Widget renders after restarting kernel', async () => {
const { notebookUI, notebookEditor } = await openNotebookAndTestSliderWidget();

// Clear the output
Expand All @@ -254,7 +254,7 @@ use(chaiAsPromised);
// Execute cell again and verify output is displayed.
await verifySliderWidgetIsAvailableAfterExecution(notebookUI);
});
test('Widget renderes after interrupting kernel', async () => {
test('Widget renders after interrupting kernel', async () => {
const { notebookUI, notebookEditor } = await openNotebookAndTestSliderWidget();

// Clear the output
Expand Down