diff --git a/changelogs/current_changelog.md b/changelogs/current_changelog.md index 2a9a12d2a5..35da9f6483 100644 --- a/changelogs/current_changelog.md +++ b/changelogs/current_changelog.md @@ -110,6 +110,7 @@ Provide a bulleted list of bug fixes and a reference to the PR(s) containing the #### Breaking Changes Provide a bulleted list of breaking changes and a reference to the PR(s) containing those changes. +- execute cell is now synchronized with kernel execute_reply returned status and will not stop following cells while the kernel still executes. ([PR5561](https://github.com/nteract/nteract/pull/5561)) #### New Features @@ -166,10 +167,12 @@ Provide a bulleted list of bug fixes and a reference to the PR(s) containing the #### Breaking Changes Provide a bulleted list of breaking changes and a reference to the PR(s) containing those changes. +- executeRequest uses Jupyter default for stop_on_error = True. ([PR5561](https://github.com/nteract/nteract/pull/5561)) #### New Features Provide a bulleted list of new features or improvements and a reference to the PR(s) containing these changes. +- Add support for retrieving executionErrors including aborted requests ([PR5561](https://github.com/nteract/nteract/pull/5561)) #### Bug Fixes diff --git a/packages/actions/__tests__/actions-spec.ts b/packages/actions/__tests__/actions-spec.ts index 8459723e8e..2d84451f86 100644 --- a/packages/actions/__tests__/actions-spec.ts +++ b/packages/actions/__tests__/actions-spec.ts @@ -317,6 +317,23 @@ describe("updateCellStatus", () => { }); }); +describe("updateCellExecutionResult", () => { + test("creates an UPDATE_CELL_EXECUTION_RESULT action", () => { + const contentRef = createContentRef(); + expect( + actions.updateCellExecutionResult({ id: "1234", result: "test", contentRef }) + ).toEqual({ + type: actionTypes.UPDATE_CELL_EXECUTION_RESULT, + payload: { + id: "1234", + contentRef, + result: "test" + } + }); + }); +}); + + describe("moveCell", () => { test("creates a MOVE_CELL action", () => { const contentRef = createContentRef(); diff --git a/packages/actions/src/actionTypes/cell_state.ts b/packages/actions/src/actionTypes/cell_state.ts index b977268b39..0474acc6aa 100644 --- a/packages/actions/src/actionTypes/cell_state.ts +++ b/packages/actions/src/actionTypes/cell_state.ts @@ -2,27 +2,30 @@ import { CellType } from "@nteract/commutable"; import { Action, HasCell, makeActionFunction, MaybeHasCell } from "../utils"; -export const TOGGLE_TAG_IN_CELL = "CORE/TOGGLE_TAG_IN_CELL"; -export const CHANGE_CELL_TYPE = "CHANGE_CELL_TYPE"; -export const UPDATE_CELL_STATUS = "UPDATE_CELL_STATUS"; -export const MARK_CELL_AS_DELETING = "MARK_CELL_AS_DELETING"; -export const UNMARK_CELL_AS_DELETING = "UNMARK_CELL_AS_DELETING"; -export const SET_IN_CELL = "SET_IN_CELL"; +export const TOGGLE_TAG_IN_CELL = "CORE/TOGGLE_TAG_IN_CELL"; +export const CHANGE_CELL_TYPE = "CHANGE_CELL_TYPE"; +export const UPDATE_CELL_STATUS = "UPDATE_CELL_STATUS"; +export const MARK_CELL_AS_DELETING = "MARK_CELL_AS_DELETING"; +export const UNMARK_CELL_AS_DELETING = "UNMARK_CELL_AS_DELETING"; +export const SET_IN_CELL = "SET_IN_CELL"; +export const UPDATE_CELL_EXECUTION_RESULT = "UPDATE_CELL_EXECUTION_RESULT"; -export type ChangeCellType = Action; -export type UpdateCellStatus = Action; -export type ToggleTagInCell = Action; -export type MarkCellAsDeleting = Action; -export type UnmarkCellAsDeleting = Action; -export type SetInCell = Action; +export type ChangeCellType = Action; +export type UpdateCellStatus = Action; +export type ToggleTagInCell = Action; +export type MarkCellAsDeleting = Action; +export type UnmarkCellAsDeleting = Action; +export type SetInCell = Action; +export type UpdateCellExecutionResult = Action; + +export const changeCellType = makeActionFunction (CHANGE_CELL_TYPE); +export const updateCellStatus = makeActionFunction (UPDATE_CELL_STATUS); +export const toggleTagInCell = makeActionFunction (TOGGLE_TAG_IN_CELL); +export const markCellAsDeleting = makeActionFunction (MARK_CELL_AS_DELETING); +export const unmarkCellAsDeleting = makeActionFunction (UNMARK_CELL_AS_DELETING); +export const updateCellExecutionResult = makeActionFunction (UPDATE_CELL_EXECUTION_RESULT); -export const changeCellType = makeActionFunction (CHANGE_CELL_TYPE); -export const updateCellStatus = makeActionFunction (UPDATE_CELL_STATUS); -export const toggleTagInCell = makeActionFunction (TOGGLE_TAG_IN_CELL); -export const markCellAsDeleting = makeActionFunction (MARK_CELL_AS_DELETING); -export const unmarkCellAsDeleting = makeActionFunction(UNMARK_CELL_AS_DELETING); - -export const setInCell = (payload: SetInCell["payload"]) => makeActionFunction>(SET_IN_CELL)(payload); -export const toggleParameterCell = (payload: HasCell) => toggleTagInCell({...payload, tag: "parameters"}); // Tag comes via Papermill -export const updateCellSource = (payload: HasCell & { value: string }) => setInCell({...payload, path: ["source"]}); -export const updateCellExecutionCount = (payload: HasCell & { value: number }) => setInCell({...payload, path: ["execution_count"]}); +export const setInCell = (payload: SetInCell["payload"]) => makeActionFunction>(SET_IN_CELL)(payload); +export const toggleParameterCell = (payload: HasCell) => toggleTagInCell({...payload, tag: "parameters"}); // Tag comes via Papermill +export const updateCellSource = (payload: HasCell & { value: string }) => setInCell({...payload, path: ["source"]}); +export const updateCellExecutionCount = (payload: HasCell & { value: number }) => setInCell({...payload, path: ["execution_count"]}); diff --git a/packages/actions/src/actionTypes/kernel_execution.ts b/packages/actions/src/actionTypes/kernel_execution.ts index c79d36614e..89532c92b2 100644 --- a/packages/actions/src/actionTypes/kernel_execution.ts +++ b/packages/actions/src/actionTypes/kernel_execution.ts @@ -18,7 +18,7 @@ export type ExecuteCell = Action ; export type ExecuteAllCellsBelow = Action ; export type ExecuteFocusedCell = Action ; -export type ExecuteCanceled = Action ; +export type ExecuteCanceled = Action ; export type ExecuteFailed = ErrorAction; export type SetExecutionStateAction = Action ; export type EnqueueAction = Action ; diff --git a/packages/epics/__tests__/execute/execute.spec.ts b/packages/epics/__tests__/execute/execute.spec.ts index f76037f8fd..c0fbc83926 100644 --- a/packages/epics/__tests__/execute/execute.spec.ts +++ b/packages/epics/__tests__/execute/execute.spec.ts @@ -60,7 +60,11 @@ describe("executeCellStream", () => { msg_type: "execute_reply" }, content: { - execution_count: 0 + execution_count: 1, + status: "error", + ename: "TestException", + evalue: "testEvalue", + traceback: ["1", "2"] } } ]; @@ -124,6 +128,62 @@ describe("executeCellStream", () => { ) ); + expect(emittedActions).toContainEqual( + expect.objectContaining( + actions.updateCellStatus({ + id: "0", + contentRef: "fakeContentRef", + status: "idle" + }) + ) + ); + + expect(emittedActions).toContainEqual( + expect.objectContaining( + actions.updateCellStatus({ + id: "0", + contentRef: "fakeContentRef", + status: "busy" + }) + ) + ); + + expect(emittedActions).toContainEqual( + expect.objectContaining( + actions.updateCellExecutionResult({ + id: "0", + contentRef: "fakeContentRef", + result: "error" + }) + ) + ); + + expect(emittedActions).toContainEqual( + expect.objectContaining( + actions.updateCellExecutionCount({ + id: "0", + contentRef: "fakeContentRef", + value: 1 + }) + ) + ); + + expect(emittedActions).toContainEqual( + expect.objectContaining( + actions.executeCanceled({ + id: "0", + contentRef: "fakeContentRef", + code: "EXEC_CELL_RUNTIME_ERROR", + error: { + execution_count: 1, + status: "error", + ename: "TestException", + evalue: "testEvalue", + traceback: ["1", "2"] + } + }) + ) + ); done(); }); }); diff --git a/packages/epics/src/execute/execute.ts b/packages/epics/src/execute/execute.ts index fc8b682694..92ed9cd999 100644 --- a/packages/epics/src/execute/execute.ts +++ b/packages/epics/src/execute/execute.ts @@ -4,6 +4,8 @@ import { executeRequest, ExecuteRequest, executionCounts, + executionStatuses, + executionErrors, inputRequests, JupyterMessage, kernelStatuses, @@ -35,11 +37,11 @@ import { ContentRef, InputRequestMessage, KernelStatus, - PayloadMessage, + PayloadMessage, errors } from "@nteract/types"; -const EXECUTE_CANCEL_ALL = "all"; +import { ExecuteReplyError } from "@nteract/messaging"; /** * Observe all the reactions to running code for cell with id. @@ -131,6 +133,14 @@ export function executeCellStream( ) ), + // Update the cell execution result from execute_reply.content.status + cellMessages.pipe( + executionStatuses() as any, + map((result: string) => + actions.updateCellExecutionResult({ id, result, contentRef }) + ) + ), + // Update the input numbering: `[ ]` cellMessages.pipe( executionCounts() as any, @@ -148,8 +158,10 @@ export function executeCellStream( ), cellMessages.pipe( - ofMessageType("error"), - map(() => actions.executeCanceled({ contentRef, id: EXECUTE_CANCEL_ALL })) + executionErrors() as any, + map((error: ExecuteReplyError) => + actions.executeCanceled({ contentRef, id, code: errors.EXEC_CELL_RUNTIME_ERROR, error }) + ) ), // clear_output display message @@ -226,9 +238,7 @@ export function createExecuteCellStream( action$.pipe( ofType(actions.EXECUTE_CANCELED, actions.DELETE_CELL), filter( - (action: ExecuteStreamActions) => - (action as PerCellStopStopExecutionActions).payload.id === id || - (action as PerCellStopStopExecutionActions).payload.id === EXECUTE_CANCEL_ALL + (action: ExecuteStreamActions) => (action as PerCellStopStopExecutionActions).payload.id === id ) ), action$.pipe( diff --git a/packages/messaging/__tests__/messaging-spec.ts b/packages/messaging/__tests__/messaging-spec.ts index 7d2bb96f98..f8fc2059a3 100644 --- a/packages/messaging/__tests__/messaging-spec.ts +++ b/packages/messaging/__tests__/messaging-spec.ts @@ -15,10 +15,13 @@ import { kernelStatuses, ofMessageType, outputs, - payloads + payloads, + executionStatuses, + executionErrors } from "../src"; import { displayData, + error, executeInput, executeReply, message, @@ -366,6 +369,63 @@ describe("executionCounts", () => { }); }); +describe("executionStatuses", () => { + it("extracts all execution status from a session", () => { + return of( + status(KernelStatus.Starting), + status(KernelStatus.Idle), + status(KernelStatus.Busy), + executeReply({ + status: "ok", + execution_count: 0 + }), + displayData({ data: { "text/plain": "woo" } }), + displayData({ data: { "text/plain": "hoo" } }), + executeReply({ + status: "aborted", + execution_count: 1 + }), + status(KernelStatus.Idle) + ) + .pipe(executionStatuses(), toArray()) + .toPromise() + .then(arr => { + expect(arr).toEqual(["ok", "aborted"]); + }); + }); +}); + +describe("error", () => { + it("extracts all error from a session", () => { + const errorContent = { + ename: "TestException", + evalue: "testEvalue", + traceback: ["1", "2"] + }; + return of( + status(KernelStatus.Starting), + status(KernelStatus.Idle), + status(KernelStatus.Busy), + executeReply({ + status: "error", + execution_count: 0, + ...errorContent + }), + error(errorContent), + status(KernelStatus.Idle) + ) + .pipe(executionErrors(), toArray()) + .toPromise() + .then(arr => { + expect(arr).toEqual([{ + status: "error", + execution_count: 0, + ...errorContent + }]); + }); + }); +}); + describe("kernelStatuses", () => { it("extracts all the execution states from status messages", () => { return of( diff --git a/packages/messaging/src/index.ts b/packages/messaging/src/index.ts index 979e324783..dcc587aabe 100644 --- a/packages/messaging/src/index.ts +++ b/packages/messaging/src/index.ts @@ -245,6 +245,26 @@ export const executionCounts = () => (source: Observable) => map(entry => entry.content.execution_count) ); +/** + * Get all the execution status from the observable of the jupyter messages + * One of: 'ok' OR 'error' OR 'aborted' from the messaging protocol: https://jupyter-client.readthedocs.io/en/stable/messaging.html#execution-results + */ +export const executionStatuses = () => (source: Observable) => +source.pipe( + ofMessageType("execute_reply"), + map(entry => entry.content.status) +); + +/** + * Get all execution errors from the observable of the jupyter messages + */ +export const executionErrors = () => (source: Observable) => +source.pipe( + ofMessageType("execute_reply"), + filter((entry) => entry.content.status !== "ok"), + map(entry => entry.content) +); + /** * Get all statuses of all running kernels. */ diff --git a/packages/messaging/src/messages.ts b/packages/messaging/src/messages.ts index ae1d376a39..a3da0b3787 100644 --- a/packages/messaging/src/messages.ts +++ b/packages/messaging/src/messages.ts @@ -125,6 +125,7 @@ function createHeader( /** * An execute request creator * + * ref: https://jupyter-client.readthedocs.io/en/stable/messaging.html#execute * > executeRequest('print("hey")', { 'silent': true }) * { header: * { msg_id: 'f344cc6b-4308-4405-a8e8-a166b0345579', @@ -141,7 +142,7 @@ function createHeader( * store_history: true, * user_expressions: {}, * allow_stdin: true, - * stop_on_error: false } } + * stop_on_error: true } } * * @param code The code to execute * @param options The options for the execute request @@ -170,7 +171,7 @@ export function executeRequest( store_history: true, user_expressions: {}, allow_stdin: true, - stop_on_error: false, + stop_on_error: true, ...options }, channel, @@ -270,11 +271,14 @@ export function executeResult(content: { * * http://jupyter-client.readthedocs.io/en/stable/messaging.html#execution-errors */ -export function error(content: { + +export interface ExecuteError { ename?: string; evalue?: string; traceback?: string[]; -}) { +} + +export function error(content: ExecuteError) { return message( { msg_type: "error" diff --git a/packages/reducers/src/core/entities/contents/index.ts b/packages/reducers/src/core/entities/contents/index.ts index 6b7bb08fd7..763a468d82 100644 --- a/packages/reducers/src/core/entities/contents/index.ts +++ b/packages/reducers/src/core/entities/contents/index.ts @@ -310,6 +310,7 @@ export const byRef = ( case actionTypes.TOGGLE_CELL_INPUT_VISIBILITY: case actionTypes.ACCEPT_PAYLOAD_MESSAGE: case actionTypes.UPDATE_CELL_STATUS: + case actionTypes.UPDATE_CELL_EXECUTION_RESULT: case actionTypes.SET_LANGUAGE_INFO: case actionTypes.SET_KERNEL_METADATA: case actionTypes.OVERWRITE_METADATA_FIELD: diff --git a/packages/reducers/src/core/entities/contents/notebook.ts b/packages/reducers/src/core/entities/contents/notebook.ts index 2e56236b9d..80f7e20a79 100644 --- a/packages/reducers/src/core/entities/contents/notebook.ts +++ b/packages/reducers/src/core/entities/contents/notebook.ts @@ -673,6 +673,14 @@ function updateCellStatus( return state.setIn(["transient", "cellMap", id, "status"], status); } +function updateCellExecutionResult( + state: NotebookModel, + action: actionTypes.UpdateCellExecutionResult +): RecordOf { + const { id, result } = action.payload; + return state.setIn(["transient", "cellMap", id, "executionResult"], result); +} + function updateOutputMetadata( state: NotebookModel, action: actionTypes.UpdateOutputMetadata @@ -921,6 +929,7 @@ type DocumentAction = | actionTypes.ToggleCellOutputVisibility | actionTypes.ToggleCellInputVisibility | actionTypes.UpdateCellStatus + | actionTypes.UpdateCellExecutionResult | actionTypes.UpdateOutputMetadata | actionTypes.SetLanguageInfo | actionTypes.SetKernelMetadata @@ -999,6 +1008,8 @@ export function notebook( return acceptPayloadMessage(state, action); case actionTypes.UPDATE_CELL_STATUS: return updateCellStatus(state, action); + case actionTypes.UPDATE_CELL_EXECUTION_RESULT: + return updateCellExecutionResult(state, action); case actionTypes.UPDATE_OUTPUT_METADATA: return updateOutputMetadata(state, action); case actionTypes.SET_LANGUAGE_INFO: diff --git a/packages/types/src/errors/execute.ts b/packages/types/src/errors/execute.ts index 1ebdcaf18d..f8649fa36f 100644 --- a/packages/types/src/errors/execute.ts +++ b/packages/types/src/errors/execute.ts @@ -51,3 +51,7 @@ export const EXEC_ERROR_IN_CELL_STREAM = "EXEC_ERROR_IN_CELL_STREAM"; * that were sent in the meantime might fail. */ export const EXEC_EPIC_ERROR = "EXEC_EPIC_ERROR"; +/** + * An error returned by the kernel on cell run as an "error" message type. + */ +export const EXEC_CELL_RUNTIME_ERROR = "EXEC_CELL_RUNTIME_ERROR"