Support Notebook integration testing by adding APIs & fixing others (#3848)

- Added `runCell` API. Updated runCell button to listen to events on the model so it'll reflect run cell when called from other sources
- Plumbed through kernelspec info to the extension side so when changed, it's updated
- Fixed bug in ConnectionProfile where it didn't copy from options but instead overrode with empty wrapper functions

Here's the rough test code (it's in the sql-vnext extension and will be out in a separate PR)
```ts

    it('Should connect to local notebook server with result 2', async function() {
        this.timeout(60000);
        let pythonNotebook = Object.assign({}, expectedNotebookContent, { metadata: { kernelspec: { name: "python3", display_name: "Python 3" }}});
        let uri = writeNotebookToFile(pythonNotebook);
        await ensureJupyterInstalled();

        let notebook = await sqlops.nb.showNotebookDocument(uri);
        should(notebook.document.cells).have.length(1);
        let ran = await notebook.runCell(notebook.document.cells[0]);
        should(ran).be.true('Notebook runCell failed');
        let cellOutputs = notebook.document.cells[0].contents.outputs;
        should(cellOutputs).have.length(1);
        let result = (<sqlops.nb.IExecuteResult>cellOutputs[0]).data['text/plain'];
        should(result).equal('2');

        try {
            // TODO support closing the editor. Right now this prompts and there's no override for this. Need to fix in core
            // Close the editor using the recommended vscode API
            //await vscode.commands.executeCommand('workbench.action.closeActiveEditor');
        }
        catch (e) {}
    });

    it('Should connect to remote spark server with result 2', async function() {
        this.timeout(240000);
        let uri = writeNotebookToFile(expectedNotebookContent);
        await ensureJupyterInstalled();

        // Given a connection to a server exists
        let connectionId = await connectToSparkIntegrationServer();

        // When I open a Spark notebook and run the cell
        let notebook = await sqlops.nb.showNotebookDocument(uri, {
            connectionId: connectionId
        });
        should(notebook.document.cells).have.length(1);
        let ran = await notebook.runCell(notebook.document.cells[0]);
        should(ran).be.true('Notebook runCell failed');

        // Then I expect to get the output result of 1+1, executed remotely against the Spark endpoint
        let cellOutputs = notebook.document.cells[0].contents.outputs;
        should(cellOutputs).have.length(4);
        let sparkResult = (<sqlops.nb.IStreamResult>cellOutputs[3]).text;
        should(sparkResult).equal('2');

        try {
            // TODO support closing the editor. Right now this prompts and there's no override for this. Need to fix in core
            // Close the editor using the recommended vscode API
            //await vscode.commands.executeCommand('workbench.action.closeActiveEditor');
        }
        catch (e) {}
    });
});
```
This commit is contained in:
Kevin Cunnane
2019-01-30 14:24:14 -08:00
committed by GitHub
parent 3ddc5e7846
commit d1fef24723
21 changed files with 321 additions and 111 deletions

View File

@@ -61,7 +61,7 @@ export class AddCellFromContextAction extends CellActionBase {
super(id, label, undefined, notificationService);
}
runCellAction(context: CellContext): Promise<void> {
doRun(context: CellContext): Promise<void> {
try {
let model = context.model;
let index = model.cells.findIndex((cell) => cell.id === context.cell.id);
@@ -88,7 +88,7 @@ export class DeleteCellAction extends CellActionBase {
super(id, label, undefined, notificationService);
}
runCellAction(context: CellContext): Promise<void> {
doRun(context: CellContext): Promise<void> {
try {
context.model.deleteCell(context.cell);
} catch (error) {
@@ -110,7 +110,7 @@ export class ClearCellOutputAction extends CellActionBase {
super(id, label, undefined, notificationService);
}
runCellAction(context: CellContext): Promise<void> {
doRun(context: CellContext): Promise<void> {
try {
(context.model.activeCell as CellModel).clearOutputs();
} catch (error) {

View File

@@ -153,7 +153,7 @@ export class CodeComponent extends AngularDisposable implements OnInit, OnChange
protected initActionBar() {
let context = new CellContext(this.model, this.cellModel);
let runCellAction = this._instantiationService.createInstance(RunCellAction);
let runCellAction = this._instantiationService.createInstance(RunCellAction, context);
let taskbar = <HTMLElement>this.toolbarElement.nativeElement;
this._actionBar = new Taskbar(taskbar, this.contextMenuService);

View File

@@ -8,12 +8,12 @@ import { nb } from 'sqlops';
import { Action } from 'vs/base/common/actions';
import { TPromise } from 'vs/base/common/winjs.base';
import { localize } from 'vs/nls';
import { CellType } from 'sql/parts/notebook/models/contracts';
import { NotebookModel } from 'sql/parts/notebook/models/notebookModel';
import { getErrorMessage } from 'sql/parts/notebook/notebookUtils';
import { INotificationService, Severity } from 'vs/platform/notification/common/notification';
import { ICellModel, FutureInternal } from 'sql/parts/notebook/models/modelInterfaces';
import { ICellModel } from 'sql/parts/notebook/models/modelInterfaces';
import { ToggleableAction } from 'sql/parts/notebook/notebookActions';
import { IDisposable } from 'vs/base/common/lifecycle';
let notebookMoreActionMsg = localize('notebook.failed', "Please select active cell and try again");
@@ -48,90 +48,62 @@ export abstract class CellActionBase extends Action {
public run(context: CellContext): TPromise<boolean> {
if (hasModelAndCell(context, this.notificationService)) {
return TPromise.wrap(this.runCellAction(context).then(() => true));
return TPromise.wrap(this.doRun(context).then(() => true));
}
return TPromise.as(true);
}
abstract runCellAction(context: CellContext): Promise<void>;
abstract doRun(context: CellContext): Promise<void>;
}
export class RunCellAction extends ToggleableAction {
public static ID = 'notebook.runCell';
public static LABEL = 'Run cell';
constructor(@INotificationService private notificationService: INotificationService) {
private _executionChangedDisposable: IDisposable;
private _context: CellContext;
constructor(context: CellContext, @INotificationService private notificationService: INotificationService) {
super(RunCellAction.ID, {
shouldToggleTooltip: true,
toggleOnLabel: localize('runCell', 'Run cell'),
toggleOnClass: 'toolbarIconRun',
toggleOffLabel: localize('stopCell', 'Cancel execution'),
toggleOffClass: 'toolbarIconStop',
isOn: true
toggleOffLabel: localize('runCell', 'Run cell'),
toggleOffClass: 'toolbarIconRun',
toggleOnLabel: localize('stopCell', 'Cancel execution'),
toggleOnClass: 'toolbarIconStop',
// On == running
isOn: false
});
this.ensureContextIsUpdated(context);
}
public run(context: CellContext): TPromise<boolean> {
if (hasModelAndCell(context, this.notificationService)) {
return TPromise.wrap(this.runCellAction(context).then(() => true));
private _handleExecutionStateChange(running: boolean): void {
this.toggle(running);
}
public run(context?: CellContext): TPromise<boolean> {
return TPromise.wrap(this.doRun(context).then(() => true));
}
public async doRun(context: CellContext): Promise<void> {
this.ensureContextIsUpdated(context);
if (!this._context) {
// TODO should we error?
return;
}
try {
await this._context.cell.runCell(this.notificationService);
} catch (error) {
let message = getErrorMessage(error);
this.notificationService.error(message);
}
return TPromise.as(true);
}
public async runCellAction(context: CellContext): Promise<void> {
try {
let model = context.model;
let cell = context.cell;
let kernel = await this.getOrStartKernel(model);
if (!kernel) {
return;
private ensureContextIsUpdated(context: CellContext) {
if (context && context !== this._context) {
if (this._executionChangedDisposable) {
this._executionChangedDisposable.dispose();
}
// If cell is currently running and user clicks the stop/cancel button, call kernel.interrupt()
// This matches the same behavior as JupyterLab
if (cell.future && cell.future.inProgress) {
cell.future.inProgress = false;
await kernel.interrupt();
} else {
// TODO update source based on editor component contents
let content = cell.source;
if (content) {
this.toggle(false);
let future = await kernel.requestExecute({
code: content,
stop_on_error: true
}, false);
cell.setFuture(future as FutureInternal);
// For now, await future completion. Later we should just track and handle cancellation based on model notifications
let reply = await future.done;
}
}
} catch (error) {
let message = getErrorMessage(error);
this.notificationService.error(message);
} finally {
this.toggle(true);
this._context = context;
this.toggle(context.cell.isRunning);
this._executionChangedDisposable = this._context.cell.onExecutionStateChange(this._handleExecutionStateChange, this);
}
}
private async getOrStartKernel(model: NotebookModel): Promise<nb.IKernel> {
let clientSession = model && model.clientSession;
if (!clientSession) {
this.notificationService.error(localize('notebookNotReady', 'The session for this notebook is not yet ready'));
return undefined;
} else if (!clientSession.isReady || clientSession.status === 'dead') {
this.notificationService.info(localize('sessionNotReady', 'The session for this notebook will start momentarily'));
await clientSession.kernelChangeCompleted;
}
if (!clientSession.kernel) {
let defaultKernel = model && model.defaultKernel && model.defaultKernel.name;
if (!defaultKernel) {
this.notificationService.error(localize('noDefaultKernel', 'No kernel is available for this notebook'));
return undefined;
}
await clientSession.changeKernel({
name: defaultKernel
});
}
return clientSession.kernel;
}
}
}

View File

@@ -6,16 +6,18 @@
'use strict';
import { nb } from 'sqlops';
import { Event, Emitter } from 'vs/base/common/event';
import URI from 'vs/base/common/uri';
import { localize } from 'vs/nls';
import { nb } from 'sqlops';
import { ICellModelOptions, IModelFactory, FutureInternal } from './modelInterfaces';
import * as notebookUtils from '../notebookUtils';
import { CellTypes, CellType, NotebookChangeType } from 'sql/parts/notebook/models/contracts';
import { ICellModel } from 'sql/parts/notebook/models/modelInterfaces';
import { NotebookModel } from 'sql/parts/notebook/models/notebookModel';
import { INotificationService, Severity } from 'vs/platform/notification/common/notification';
let modelId = 0;
@@ -28,6 +30,7 @@ export class CellModel implements ICellModel {
private _isEditMode: boolean;
private _onOutputsChanged = new Emitter<ReadonlyArray<nb.ICellOutput>>();
private _onCellModeChanged = new Emitter<boolean>();
private _onExecutionStateChanged = new Emitter<boolean>();
public id: string;
private _isTrusted: boolean;
private _active: boolean;
@@ -131,6 +134,89 @@ export class CellModel implements ICellModel {
this._language = newLanguage;
}
public get onExecutionStateChange(): Event<boolean> {
return this._onExecutionStateChanged.event;
}
public get isRunning(): boolean {
return !!(this._future && this._future.inProgress);
}
public async runCell(notificationService?: INotificationService): Promise<boolean> {
try {
if (this.cellType !== CellTypes.Code) {
// TODO should change hidden state to false if we add support
// for this property
return false;
}
let kernel = await this.getOrStartKernel(notificationService);
if (!kernel) {
return false;
}
// If cell is currently running and user clicks the stop/cancel button, call kernel.interrupt()
// This matches the same behavior as JupyterLab
if (this.future && this.future.inProgress) {
this.future.inProgress = false;
await kernel.interrupt();
} else {
// TODO update source based on editor component contents
let content = this.source;
if (content) {
this._onExecutionStateChanged.fire(true);
let future = await kernel.requestExecute({
code: content,
stop_on_error: true
}, false);
this.setFuture(future as FutureInternal);
// For now, await future completion. Later we should just track and handle cancellation based on model notifications
let result: nb.IExecuteReplyMsg = <nb.IExecuteReplyMsg><any> await future.done;
return result && result.content.status === 'ok' ? true : false;
}
}
} catch (error) {
if (error.message === 'Canceled') {
// swallow the error
}
let message = notebookUtils.getErrorMessage(error);
this.sendNotification(notificationService, Severity.Error, message);
throw error;
} finally {
this._onExecutionStateChanged.fire(false);
}
return true;
}
private async getOrStartKernel(notificationService: INotificationService): Promise<nb.IKernel> {
let model = this.options.notebook;
let clientSession = model && model.clientSession;
if (!clientSession) {
this.sendNotification(notificationService, Severity.Error, localize('notebookNotReady', 'The session for this notebook is not yet ready'));
return undefined;
} else if (!clientSession.isReady || clientSession.status === 'dead') {
this.sendNotification(notificationService, Severity.Info, localize('sessionNotReady', 'The session for this notebook will start momentarily'));
await clientSession.kernelChangeCompleted;
}
if (!clientSession.kernel) {
let defaultKernel = model && model.defaultKernel && model.defaultKernel.name;
if (!defaultKernel) {
this.sendNotification(notificationService, Severity.Error, localize('noDefaultKernel', 'No kernel is available for this notebook'));
return undefined;
}
await clientSession.changeKernel({
name: defaultKernel
});
}
return clientSession.kernel;
}
private sendNotification(notificationService: INotificationService, severity: Severity, message: string): void {
if (notificationService) {
notificationService.notify({ severity: severity, message: message});
}
}
/**
* Sets the future which will be used to update the output
* area for this cell
@@ -178,6 +264,11 @@ export class CellModel implements ICellModel {
// TODO #931 we should process this. There can be a payload attached which should be added to outputs.
// In all other cases, it is a no-op
let output: nb.ICellOutput = msg.content as nb.ICellOutput;
if (!this._future.inProgress) {
this._future.dispose();
this._future = undefined;
}
}
private handleIOPub(msg: nb.IIOPubMessage): void {
@@ -223,9 +314,6 @@ export class CellModel implements ICellModel {
this._outputs.push(this.rewriteOutputUrls(output));
this.fireOutputsChanged();
}
if (!this._future.inProgress) {
this._future.dispose();
}
}
private rewriteOutputUrls(output: nb.ICellOutput): nb.ICellOutput {

View File

@@ -20,6 +20,7 @@ import * as notebookUtils from '../notebookUtils';
import { INotebookManager } from 'sql/workbench/services/notebook/common/notebookService';
import { IConnectionProfile } from 'sql/platform/connection/common/interfaces';
type KernelChangeHandler = (kernel: nb.IKernelChangedArgs) => Promise<void>;
/**
* Implementation of a client session. This is a model over session operations,
* which may come from the session manager or a specific session.
@@ -41,6 +42,9 @@ export class ClientSession implements IClientSession {
private _kernelPreference: IKernelPreference;
private _kernelDisplayName: string;
private _errorMessage: string;
private _cachedKernelSpec: nb.IKernelSpec;
private _kernelChangeHandlers: KernelChangeHandler[] = [];
//#endregion
private _serverLoadFinished: Promise<void>;
@@ -62,6 +66,7 @@ export class ClientSession implements IClientSession {
this._serverLoadFinished = this.startServer();
await this._serverLoadFinished;
await this.initializeSession();
await this.updateCachedKernelSpec();
} catch (err) {
this._errorMessage = notebookUtils.getErrorMessage(err);
}
@@ -150,6 +155,12 @@ export class ClientSession implements IClientSession {
public get kernelChanged(): Event<nb.IKernelChangedArgs> {
return this._kernelChangedEmitter.event;
}
public onKernelChanging(changeHandler: (kernel: nb.IKernelChangedArgs) => Promise<void>): void {
if (changeHandler) {
this._kernelChangeHandlers.push(changeHandler);
}
}
public get statusChanged(): Event<nb.ISession> {
return this._statusChangedEmitter.event;
}
@@ -204,6 +215,10 @@ export class ClientSession implements IClientSession {
public get isInErrorState(): boolean {
return !!this._errorMessage;
}
public get cachedKernelSpec(): nb.IKernelSpec {
return this._cachedKernelSpec;
}
//#endregion
//#region Not Yet Implemented
@@ -227,15 +242,28 @@ export class ClientSession implements IClientSession {
}
newKernel = this._session ? kernel : this._session.kernel;
this._isReady = kernel.isReady;
await this.updateCachedKernelSpec();
// Send resolution events to listeners
this._kernelChangeCompleted.resolve();
this._kernelChangedEmitter.fire({
let changeArgs: nb.IKernelChangedArgs = {
oldValue: oldKernel,
newValue: newKernel
});
};
let changePromises = this._kernelChangeHandlers.map(handler => handler(changeArgs));
await Promise.all(changePromises);
// Wait on connection configuration to complete before resolving full kernel change
this._kernelChangeCompleted.resolve();
this._kernelChangedEmitter.fire(changeArgs);
return kernel;
}
private async updateCachedKernelSpec(): Promise<void> {
this._cachedKernelSpec = undefined;
let kernel = this.kernel;
if (kernel && kernel.isReady) {
this._cachedKernelSpec = await this.kernel.getSpec();
}
}
/**
* Helper method to either call ChangeKernel on current session, or start a new session
* @param options

View File

@@ -43,5 +43,6 @@ export enum NotebookChangeType {
CellDeleted,
CellSourceUpdated,
CellOutputUpdated,
DirtyStateChanged
DirtyStateChanged,
KernelChanged
}

View File

@@ -127,6 +127,8 @@ export interface IClientSession extends IDisposable {
*/
readonly kernelDisplayName: string;
readonly cachedKernelSpec: nb.IKernelSpec;
/**
* Initializes the ClientSession, by starting the server and
* connecting to the SessionManager.
@@ -200,7 +202,13 @@ export interface IClientSession extends IDisposable {
/**
* Updates the connection
*/
updateConnection(connection: IConnectionProfile): void;
updateConnection(connection: IConnectionProfile): Promise<void>;
/**
* Supports registering a handler to run during kernel change and implement any calls needed to configure
* the kernel before actions such as run should be allowed
*/
onKernelChanging(changeHandler: ((kernel: nb.IKernelChangedArgs) => Promise<void>)): void;
}
export interface IDefaultConnection {
@@ -322,7 +330,7 @@ export interface INotebookModel {
/**
* Change the current context (if applicable)
*/
changeContext(host: string, connection?: IConnectionProfile): void;
changeContext(host: string, connection?: IConnectionProfile): Promise<void>;
/**
* Find a cell's index given its model
@@ -400,7 +408,10 @@ export interface ICellModel {
readonly future: FutureInternal;
readonly outputs: ReadonlyArray<nb.ICellOutput>;
readonly onOutputsChanged: Event<ReadonlyArray<nb.ICellOutput>>;
readonly onExecutionStateChange: Event<boolean>;
setFuture(future: FutureInternal): void;
readonly isRunning: boolean;
runCell(notificationService?: INotificationService): Promise<boolean>;
equals(cellModel: ICellModel): boolean;
toJSON(): nb.ICellContents;
}

View File

@@ -255,7 +255,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
}
}
public findCellIndex(cellModel: CellModel): number {
public findCellIndex(cellModel: ICellModel): number {
return this._cells.findIndex((cell) => cell.equals(cellModel));
}
@@ -420,7 +420,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
return Promise.resolve();
}
public changeContext(server: string, newConnection?: IConnectionProfile): void {
public async changeContext(server: string, newConnection?: IConnectionProfile): Promise<void> {
try {
if (!newConnection) {
newConnection = this._activeContexts.otherConnections.find((connection) => connection.serverName === server);
@@ -431,7 +431,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
let newConnectionProfile = new ConnectionProfile(this.notebookOptions.capabilitiesService, newConnection);
this._activeConnection = newConnectionProfile;
this.refreshConnections(newConnectionProfile);
this._activeClientSession.updateConnection(this._activeConnection);
await this._activeClientSession.updateConnection(this._activeConnection);
} catch (err) {
let msg = notebookUtils.getErrorMessage(err);
this.notifyError(localize('changeContextFailed', 'Changing context failed: {0}', msg));
@@ -454,7 +454,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
private loadKernelInfo(): void {
this._clientSessions.forEach(clientSession => {
clientSession.kernelChanged(async (e) => {
clientSession.onKernelChanging(async (e) => {
await this.loadActiveContexts(e);
});
});
@@ -550,7 +550,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
this._activeContexts = await NotebookContexts.getContextsForKernel(this.notebookOptions.connectionService, this.getApplicableConnectionProviderIds(kernelDisplayName), kernelChangedArgs, this.connectionProfile);
this._contextsChangedEmitter.fire();
if (this.contexts.defaultConnection !== undefined && this.contexts.defaultConnection.serverName !== undefined) {
this.changeContext(this.contexts.defaultConnection.serverName);
await this.changeContext(this.contexts.defaultConnection.serverName);
}
}
}

View File

@@ -123,7 +123,6 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
ngOnInit() {
this._register(this.themeService.onDidColorThemeChange(this.updateTheme, this));
this.updateTheme(this.themeService.getColorTheme());
this.notebookService.addNotebookEditor(this);
this.initActionBar();
this.doLoad();
}
@@ -135,7 +134,7 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
}
}
public get model(): NotebookModel {
public get model(): NotebookModel | null {
return this._model;
}
@@ -222,6 +221,9 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
this.setViewInErrorState(localize('displayFailed', 'Could not display contents: {0}', error));
this.setLoading(false);
this._modelReadyDeferred.reject(error);
} finally {
// Always add the editor for now to close loop, even if loading contents failed
this.notebookService.addNotebookEditor(this);
}
}
@@ -538,4 +540,15 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
this._model.pushEditOperations(edits);
return true;
}
public async runCell(cell: ICellModel): Promise<boolean> {
await this.modelReady;
let uriString = cell.cellUri.toString();
if (this._model.cells.findIndex(c => c.cellUri.toString() === uriString) > -1) {
return cell.runCell(this.notificationService);
} else {
return Promise.reject<boolean>(new Error(localize('cellNotFound', 'cell with URI {0} was not found in this model', uriString)));
}
}
}

View File

@@ -230,7 +230,8 @@ export class AttachToDropdown extends SelectBox {
this.model = model;
model.contextsChanged(() => {
if (this.model.clientSession.kernel && this.model.clientSession.kernel.name) {
let currentKernelSpec = this.model.specs.kernels.find(kernel => kernel.name === this.model.clientSession.kernel.name);
let nameLower = this.model.clientSession.kernel.name.toLowerCase();
let currentKernelSpec = this.model.specs.kernels.find(kernel => kernel.name && kernel.name.toLowerCase() === nameLower);
this.loadAttachToDropdown(this.model, currentKernelSpec.display_name);
}
});
@@ -282,7 +283,7 @@ export class AttachToDropdown extends SelectBox {
if (this.value === msgAddNewConnection) {
this.openConnectionDialog();
} else {
this.model.changeContext(this.value, connection);
this.model.changeContext(this.value, connection).then(ok => undefined, err => this._notificationService.error(getErrorMessage(err)));
}
}