From 8f48e79b736d5524406a6dd872c50ae138230ef6 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 7 Nov 2018 11:11:22 -0800 Subject: [PATCH 1/4] Eliminate functional tests from travis --- .travis.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.travis.yml b/.travis.yml index 30d83511d11e..98a8f51633e0 100644 --- a/.travis.yml +++ b/.travis.yml @@ -24,9 +24,6 @@ matrix: - os: linux python: "3.6-dev" env: DEBUGGER_TEST_RELEASE=true - - os: linux - python: "3.6-dev" - env: FUNCTIONAL_TEST=true - os: linux python: "3.6-dev" env: SINGLE_WORKSPACE_TEST=true @@ -42,9 +39,6 @@ matrix: - os: linux python: "3.7-dev" env: DEBUGGER_TEST_RELEASE=true - - os: linux - python: "3.7-dev" - env: FUNCTIONAL_TEST=true - os: linux python: "3.7-dev" env: SINGLE_WORKSPACE_TEST=true From 9525fd74490841f7f105e198c2e789eac9681cea Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 7 Nov 2018 17:04:37 -0800 Subject: [PATCH 2/4] Fix markdown coming in out of order --- package-lock.json | 7 +- src/client/datascience/jupyterServer.ts | 192 +++++++++--------------- 2 files changed, 76 insertions(+), 123 deletions(-) diff --git a/package-lock.json b/package-lock.json index 4873a8d318f6..92b44a77f158 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "python", - "version": "2018.10.0-rc", + "version": "2018.11.0-alpha", "lockfileVersion": 1, "requires": true, "dependencies": { @@ -5487,7 +5487,7 @@ }, "event-stream": { "version": "3.3.4", - "resolved": "http://registry.npmjs.org/event-stream/-/event-stream-3.3.4.tgz", + "resolved": "https://registry.npmjs.org/event-stream/-/event-stream-3.3.4.tgz", "integrity": "sha1-SrTJoPWlTbkzi0w02Gv86PSzVXE=", "dev": true, "requires": { @@ -6309,7 +6309,8 @@ "inherits": { "version": "2.0.3", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "ini": { "version": "1.3.5", diff --git a/src/client/datascience/jupyterServer.ts b/src/client/datascience/jupyterServer.ts index fd5910eb190b..dfb3ad5bac14 100644 --- a/src/client/datascience/jupyterServer.ts +++ b/src/client/datascience/jupyterServer.ts @@ -203,13 +203,8 @@ export class JupyterServer implements INotebookServer { const request = this.generateRequest(code, true); return new Promise((resolve, reject) => { - // Just wait for our observable to finish - const observable = this.generateExecuteObservable(code, 'file', -1, '0', request); - // tslint:disable-next-line:no-empty - observable.subscribe(() => { - }, - reject, - resolve); + // Just wait for our request to finish + request.done.then(() => resolve).catch(reject); }); } @@ -422,68 +417,6 @@ export class JupyterServer implements INotebookServer { }); } - private changeDirectoryObservable = (file: string) : Observable => { - return new Observable(subscriber => { - // Execute some code and when its done, finish our subscriber - const dir = path.dirname(file); - this.executeSilently(`%cd "${dir}"`) - .then(() => { - subscriber.next(true); - subscriber.complete(); - }) - .catch(err => subscriber.error(err)); - }); - } - - private chainObservables(first : Observable, second : () => Observable) : Observable { - return new Observable(subscriber => { - first.subscribe( - () => { return; }, - (err) => subscriber.error(err), - () => { - // When the first completes, tell the second to go - second().subscribe((cell : ICell) => { - subscriber.next(cell); - }, - (err) => { - subscriber.error(err); - }, - () => { - subscriber.complete(); - }); - } - ); - }); - } - - private executeCodeObservable = (code: string, file: string, line: number) : Observable => { - - if (this.session) { - // Send a magic that changes the current directory if we aren't already sending a magic - if (line >= 0 && fs.existsSync(file)) { - return this.chainObservables( - this.changeDirectoryObservable(file), - () => this.executeCodeObservableInternal(code, file, line)); - } else { - // We're inside of an execute silently already, don't recurse - return this.executeCodeObservableInternal(code, file, line); - } - } - - return new Observable(subscriber => { - subscriber.error(new Error(localize.DataScience.sessionDisposed())); - subscriber.complete(); - }); - } - - private executeCodeObservableInternal = (code: string, file: string, line: number) : Observable => { - // Send an execute request with this code - const id = uuid(); - const request = this.session ? this.generateRequest(code, false) : undefined; - - return this.generateExecuteObservable(code, file, line, id, request); - } - private appendLineFeed(arr : string[], modifier? : (s : string) => string) { return arr.map((s: string, i: number) => { const out = modifier ? modifier(s) : s; @@ -514,7 +447,7 @@ export class JupyterServer implements INotebookServer { }); } - private generateExecuteObservable(code: string, file: string, line: number, id: string, request: Kernel.IFuture | undefined) : Observable { + private executeCodeObservable(code: string, file: string, line: number) : Observable { return new Observable(subscriber => { // Start out empty; const cell: ICell = { @@ -525,7 +458,7 @@ export class JupyterServer implements INotebookServer { metadata: {}, execution_count: 0 }, - id: id, + id: uuid(), file: file, line: line, state: CellState.init @@ -534,64 +467,83 @@ export class JupyterServer implements INotebookServer { // Keep track of when we started. const startTime = Date.now(); - // Tell our listener. + // Tell our listener. NOTE: have to do this asap so that markdown cells don't get + // run before our cells. subscriber.next(cell); - // Transition to the busy stage - cell.state = CellState.executing; - - // Listen to the reponse messages and update state as we go - if (request) { - request.onIOPub = (msg: KernelMessage.IIOPubMessage) => { - try { - if (KernelMessage.isExecuteResultMsg(msg)) { - this.handleExecuteResult(msg as KernelMessage.IExecuteResultMsg, cell); - } else if (KernelMessage.isExecuteInputMsg(msg)) { - this.handleExecuteInput(msg as KernelMessage.IExecuteInputMsg, cell); - } else if (KernelMessage.isStatusMsg(msg)) { - this.handleStatusMessage(msg as KernelMessage.IStatusMsg); - } else if (KernelMessage.isStreamMsg(msg)) { - this.handleStreamMesssage(msg as KernelMessage.IStreamMsg, cell); - } else if (KernelMessage.isDisplayDataMsg(msg)) { - this.handleDisplayData(msg as KernelMessage.IDisplayDataMsg, cell); - } else if (KernelMessage.isErrorMsg(msg)) { - this.handleError(msg as KernelMessage.IErrorMsg, cell); - } else { - this.logger.logWarning(`Unknown message ${msg.header.msg_type} : hasData=${'data' in msg.content}`); + // Create a handler we'll run after the change directory or not. + const executeHandler = () => { + const request = this.generateRequest(code, false); + + // Transition to the busy stage + cell.state = CellState.executing; + + // Listen to the reponse messages and update state as we go + if (request) { + request.onIOPub = (msg: KernelMessage.IIOPubMessage) => { + try { + if (KernelMessage.isExecuteResultMsg(msg)) { + this.handleExecuteResult(msg as KernelMessage.IExecuteResultMsg, cell); + } else if (KernelMessage.isExecuteInputMsg(msg)) { + this.handleExecuteInput(msg as KernelMessage.IExecuteInputMsg, cell); + } else if (KernelMessage.isStatusMsg(msg)) { + this.handleStatusMessage(msg as KernelMessage.IStatusMsg); + } else if (KernelMessage.isStreamMsg(msg)) { + this.handleStreamMesssage(msg as KernelMessage.IStreamMsg, cell); + } else if (KernelMessage.isDisplayDataMsg(msg)) { + this.handleDisplayData(msg as KernelMessage.IDisplayDataMsg, cell); + } else if (KernelMessage.isErrorMsg(msg)) { + this.handleError(msg as KernelMessage.IErrorMsg, cell); + } else { + this.logger.logWarning(`Unknown message ${msg.header.msg_type} : hasData=${'data' in msg.content}`); + } + + // Set execution count, all messages should have it + if (msg.content.execution_count) { + cell.data.execution_count = msg.content.execution_count as number; + } + + // Show our update if any new output + subscriber.next(cell); + } catch (err) { + // If not a restart error, then tell the subscriber + if (startTime > this.sessionStartTime) { + this.logger.logError(`Error during message ${msg.header.msg_type}`); + subscriber.error(err); + } } + }; - // Set execution count, all messages should have it - if (msg.content.execution_count) { - cell.data.execution_count = msg.content.execution_count as number; - } - - // Show our update if any new output - subscriber.next(cell); - } catch (err) { - // If not a restart error, then tell the subscriber + // Create completion and error functions so we can bind our cell object + const completion = (error: boolean) => { + cell.state = error ? CellState.error : CellState.finished; + // Only do this if start time is still valid if (startTime > this.sessionStartTime) { - this.logger.logError(`Error during message ${msg.header.msg_type}`); - subscriber.error(err); + subscriber.next(cell); } - } - }; - - // Create completion and error functions so we can bind our cell object - const completion = (error : boolean) => { - cell.state = error ? CellState.error : CellState.finished; - // Only do this if start time is still valid - if (startTime > this.sessionStartTime) { - subscriber.next(cell); - } - subscriber.complete(); - }; + subscriber.complete(); + }; + + // When the request finishes we are done + request.done.then(() => completion(false), () => completion(true)); + } else { + subscriber.error(new Error(localize.DataScience.sessionDisposed())); + } + }; - // When the request finishes we are done - request.done.then(() => completion(false), () => completion(true)); + // Change directory if necessary + if (line >= 0 && fs.existsSync(file)) { + const dir = path.dirname(file); + const cdrequest = this.generateRequest(`%cd "${dir}"`, true); + if (cdrequest) { + cdrequest.done.then(executeHandler).catch(err => subscriber.error(err)); + } else { + subscriber.error(new Error(localize.DataScience.sessionDisposed())); + } } else { - subscriber.error(new Error(localize.DataScience.sessionDisposed())); + // Otherwise just continue normally. + executeHandler(); } - }); } From a887641b254d5c85098668d7978bb5eb82c0c63c Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 8 Nov 2018 09:24:33 -0800 Subject: [PATCH 3/4] Update based on review feedback --- src/client/datascience/jupyterServer.ts | 193 +++++++++++++----------- 1 file changed, 108 insertions(+), 85 deletions(-) diff --git a/src/client/datascience/jupyterServer.ts b/src/client/datascience/jupyterServer.ts index dfb3ad5bac14..21c1d8a4b427 100644 --- a/src/client/datascience/jupyterServer.ts +++ b/src/client/datascience/jupyterServer.ts @@ -9,6 +9,7 @@ import * as fs from 'fs-extra'; import { inject, injectable } from 'inversify'; import * as path from 'path'; import { Observable } from 'rxjs/Observable'; +import { Subscriber } from 'rxjs/Subscriber'; import * as uuid from 'uuid/v4'; import * as vscode from 'vscode'; @@ -148,7 +149,7 @@ export class JupyterServer implements INotebookServer { output = cells; }, (error) => { - deferred.resolve(output); + deferred.reject(error); }, () => { deferred.resolve(output); @@ -197,18 +198,34 @@ export class JupyterServer implements INotebookServer { } public executeSilently = (code: string) : Promise => { - // If we have a session, execute the code now. - if (this.session) { - // Generate a new request and wrap it in a promise as we wait for it to finish - const request = this.generateRequest(code, true); + return new Promise((resolve, reject) => { + // If we have a session, execute the code now. + if (this.session) { + // Generate a new request and resolve when it's done. + const request = this.generateRequest(code, true); - return new Promise((resolve, reject) => { - // Just wait for our request to finish - request.done.then(() => resolve).catch(reject); - }); - } + if (request) { - return Promise.reject(new Error(localize.DataScience.sessionDisposed())); + // // For debugging purposes when silently is failing. + // request.onIOPub = (msg: KernelMessage.IIOPubMessage) => { + // try { + // this.logger.logInformation(`Execute silently message ${msg.header.msg_type} : hasData=${'data' in msg.content}`); + // } catch (err) { + // this.logger.logError(err); + // } + // }; + + request.done.then(() => { + this.logger.logInformation(`Execute for ${code} silently finished.`); + resolve(); + }).catch(reject); + } else { + reject(new Error(localize.DataScience.sessionDisposed())); + } + } else { + reject(new Error(localize.DataScience.sessionDisposed())); + } + }); } public get onStatusChanged() : vscode.Event { @@ -295,7 +312,7 @@ export class JupyterServer implements INotebookServer { return this.session.kernel.requestExecute( { // Replace windows line endings with unix line endings. - code: code.replace('\r\n', '\n'), + code: code.replace(/\r\n/g, '\n'), stop_on_error: false, allow_stdin: false, silent: silent @@ -447,6 +464,75 @@ export class JupyterServer implements INotebookServer { }); } + private changeDirectoryIfPossible = async (file: string, line: number) : Promise => { + if (line >= 0 && await fs.pathExists(file)) { + const dir = path.dirname(file); + await this.executeSilently(`%cd "${dir}"`); + } + } + + private handleCodeRequest = (subscriber: Subscriber, startTime: number, cell: ICell, code: string) => { + // Generate a new request. + const request = this.generateRequest(code, false); + + // Transition to the busy stage + cell.state = CellState.executing; + + // Listen to the reponse messages and update state as we go + if (request) { + request.onIOPub = (msg: KernelMessage.IIOPubMessage) => { + try { + if (KernelMessage.isExecuteResultMsg(msg)) { + this.handleExecuteResult(msg as KernelMessage.IExecuteResultMsg, cell); + } else if (KernelMessage.isExecuteInputMsg(msg)) { + this.handleExecuteInput(msg as KernelMessage.IExecuteInputMsg, cell); + } else if (KernelMessage.isStatusMsg(msg)) { + this.handleStatusMessage(msg as KernelMessage.IStatusMsg); + } else if (KernelMessage.isStreamMsg(msg)) { + this.handleStreamMesssage(msg as KernelMessage.IStreamMsg, cell); + } else if (KernelMessage.isDisplayDataMsg(msg)) { + this.handleDisplayData(msg as KernelMessage.IDisplayDataMsg, cell); + } else if (KernelMessage.isErrorMsg(msg)) { + this.handleError(msg as KernelMessage.IErrorMsg, cell); + } else { + this.logger.logWarning(`Unknown message ${msg.header.msg_type} : hasData=${'data' in msg.content}`); + } + + // Set execution count, all messages should have it + if (msg.content.execution_count) { + cell.data.execution_count = msg.content.execution_count as number; + } + + // Show our update if any new output + subscriber.next(cell); + } catch (err) { + // If not a restart error, then tell the subscriber + if (startTime > this.sessionStartTime) { + this.logger.logError(`Error during message ${msg.header.msg_type}`); + subscriber.error(err); + } + } + }; + + // Create completion and error functions so we can bind our cell object + // tslint:disable-next-line:no-any + const completion = (error?: any) => { + cell.state = error ? CellState.error : CellState.finished; + // Only do this if start time is still valid. Dont log an error to the subscriber. Error + // state should end up in the cell output. + if (startTime > this.sessionStartTime) { + subscriber.next(cell); + } + subscriber.complete(); + }; + + // When the request finishes we are done + request.done.then(completion).catch(completion); + } else { + subscriber.error(new Error(localize.DataScience.sessionDisposed())); + } + } + private executeCodeObservable(code: string, file: string, line: number) : Observable { return new Observable(subscriber => { // Start out empty; @@ -471,79 +557,16 @@ export class JupyterServer implements INotebookServer { // run before our cells. subscriber.next(cell); - // Create a handler we'll run after the change directory or not. - const executeHandler = () => { - const request = this.generateRequest(code, false); - - // Transition to the busy stage - cell.state = CellState.executing; - - // Listen to the reponse messages and update state as we go - if (request) { - request.onIOPub = (msg: KernelMessage.IIOPubMessage) => { - try { - if (KernelMessage.isExecuteResultMsg(msg)) { - this.handleExecuteResult(msg as KernelMessage.IExecuteResultMsg, cell); - } else if (KernelMessage.isExecuteInputMsg(msg)) { - this.handleExecuteInput(msg as KernelMessage.IExecuteInputMsg, cell); - } else if (KernelMessage.isStatusMsg(msg)) { - this.handleStatusMessage(msg as KernelMessage.IStatusMsg); - } else if (KernelMessage.isStreamMsg(msg)) { - this.handleStreamMesssage(msg as KernelMessage.IStreamMsg, cell); - } else if (KernelMessage.isDisplayDataMsg(msg)) { - this.handleDisplayData(msg as KernelMessage.IDisplayDataMsg, cell); - } else if (KernelMessage.isErrorMsg(msg)) { - this.handleError(msg as KernelMessage.IErrorMsg, cell); - } else { - this.logger.logWarning(`Unknown message ${msg.header.msg_type} : hasData=${'data' in msg.content}`); - } - - // Set execution count, all messages should have it - if (msg.content.execution_count) { - cell.data.execution_count = msg.content.execution_count as number; - } - - // Show our update if any new output - subscriber.next(cell); - } catch (err) { - // If not a restart error, then tell the subscriber - if (startTime > this.sessionStartTime) { - this.logger.logError(`Error during message ${msg.header.msg_type}`); - subscriber.error(err); - } - } - }; - - // Create completion and error functions so we can bind our cell object - const completion = (error: boolean) => { - cell.state = error ? CellState.error : CellState.finished; - // Only do this if start time is still valid - if (startTime > this.sessionStartTime) { - subscriber.next(cell); - } - subscriber.complete(); - }; - - // When the request finishes we are done - request.done.then(() => completion(false), () => completion(true)); - } else { - subscriber.error(new Error(localize.DataScience.sessionDisposed())); - } - }; - - // Change directory if necessary - if (line >= 0 && fs.existsSync(file)) { - const dir = path.dirname(file); - const cdrequest = this.generateRequest(`%cd "${dir}"`, true); - if (cdrequest) { - cdrequest.done.then(executeHandler).catch(err => subscriber.error(err)); - } else { - subscriber.error(new Error(localize.DataScience.sessionDisposed())); - } - } else { - // Otherwise just continue normally. - executeHandler(); - } + // Attempt to change to the current directory. When that finishes + // send our real request + this.changeDirectoryIfPossible(file, line) + .then(() => { + this.handleCodeRequest(subscriber, startTime, cell, code); + }) + .catch(() => { + // Ignore errors if they occur. Just execute normally + this.handleCodeRequest(subscriber, startTime, cell, code); + }); }); } From 14fae2bd11e48f1f34bd4467e111d48fa5b1c60a Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 8 Nov 2018 10:24:31 -0800 Subject: [PATCH 4/4] Still having travis problems with 3.7 --- .travis.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index d0fe53f6c655..0e16d2917a5f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -39,9 +39,6 @@ matrix: - os: linux python: "3.7-dev" env: DEBUGGER_TEST=true - - os: linux - python: "3.7-dev" - env: FUNCTIONAL_TEST=true - os: linux python: "3.7-dev" env: DEBUGGER_TEST_RELEASE=true