From 99106433299110f41473037b2c69479e72f0e851 Mon Sep 17 00:00:00 2001 From: Steve Purves Date: Thu, 9 Feb 2023 20:43:06 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=B3=20exposing=20any=20errors=20during?= =?UTF-8?q?=20execution=20in=20execute=20return=20object=20(#601)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/core/src/cell.ts | 9 ++-- packages/core/src/cell_noexec.ts | 2 +- packages/core/src/notebook.ts | 14 ++---- packages/core/src/types.ts | 4 +- packages/react/src/hooks/notebook.ts | 71 ++++++++++++++++++++-------- 5 files changed, 64 insertions(+), 36 deletions(-) diff --git a/packages/core/src/cell.ts b/packages/core/src/cell.ts index 4f69f618..e4a0e5be 100644 --- a/packages/core/src/cell.ts +++ b/packages/core/src/cell.ts @@ -130,7 +130,7 @@ class ThebeCell extends PassiveCellRenderer implements IThebeCell { console.debug(`thebe:renderer:execute ${this.id}`); if (!this.isBusy) this.setAsBusy(); - const useShadow = true; + const useShadow = true; // TODO expose as option if (useShadow) { // Use a shadow output area for the execute request const model = new OutputAreaModel({ trusted: true }); @@ -151,7 +151,7 @@ class ThebeCell extends PassiveCellRenderer implements IThebeCell { await this.area.future.done; } - let hasExecuteErrors = false; + let executeErrors: IError[] | undefined; for (let i = 0; i < this.model.length; i++) { const out = this.model.get(i); if (out.type === 'error') { @@ -162,7 +162,8 @@ class ThebeCell extends PassiveCellRenderer implements IThebeCell { message: errorToMessage(json), }); } else { - hasExecuteErrors = true; + if (!executeErrors) executeErrors = [json]; + else executeErrors?.push(json); this.events.triggerError({ status: ErrorStatusEvent.executeError, message: errorToMessage(json), @@ -176,7 +177,7 @@ class ThebeCell extends PassiveCellRenderer implements IThebeCell { id: this.id, height: this.area.node.offsetHeight, width: this.area.node.offsetWidth, - error: hasExecuteErrors, + error: executeErrors, }; } catch (err: any) { console.error('thebe:renderer:execute Error:', err); diff --git a/packages/core/src/cell_noexec.ts b/packages/core/src/cell_noexec.ts index ce2d17d0..19d312b9 100644 --- a/packages/core/src/cell_noexec.ts +++ b/packages/core/src/cell_noexec.ts @@ -110,6 +110,6 @@ export default class NonExecutableCell implements IThebeCell { async execute(source?: string): Promise { // could potentially allow for markdown rendering here - return { id: this.id, height: 0, width: 0, error: false }; + return { id: this.id, height: 0, width: 0 }; } } diff --git a/packages/core/src/notebook.ts b/packages/core/src/notebook.ts index bb5ba1bc..446c649d 100644 --- a/packages/core/src/notebook.ts +++ b/packages/core/src/notebook.ts @@ -10,12 +10,6 @@ import { EventEmitter } from './emitter'; import type { ICodeCell, INotebookContent, INotebookMetadata } from '@jupyterlab/nbformat'; import NonExecutableCell from './cell_noexec'; -export interface ExecuteReturn { - id: string; - height: number; - width: number; -} - export interface CodeBlock { id: string; source: string; @@ -150,7 +144,7 @@ class ThebeNotebook { cellId: string, stopOnError = false, preprocessor?: (s: string) => string, - ): Promise<(ExecuteReturn | null)[]> { + ): Promise<(IThebeCellExecuteReturn | null)[]> { if (!this.cells) return []; this.events.triggerStatus({ status: NotebookStatusEvent.executing, @@ -177,7 +171,7 @@ class ThebeNotebook { async executeOnly( cellId: string, preprocessor?: (s: string) => string, - ): Promise { + ): Promise { if (!this.cells) return null; this.events.triggerStatus({ status: NotebookStatusEvent.executing, @@ -196,7 +190,7 @@ class ThebeNotebook { cellIds: string[], stopOnError = false, preprocessor?: (s: string) => string, - ): Promise<(ExecuteReturn | null)[]> { + ): Promise<(IThebeCellExecuteReturn | null)[]> { if (!this.cells) return []; this.events.triggerStatus({ status: NotebookStatusEvent.executing, @@ -238,7 +232,7 @@ class ThebeNotebook { async executeAll( stopOnError = false, preprocessor?: (s: string) => string, - ): Promise<(ExecuteReturn | null)[]> { + ): Promise<(IThebeCellExecuteReturn | null)[]> { if (!this.cells) return []; this.events.triggerStatus({ diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 62ed0464..70fd65e0 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -1,6 +1,6 @@ import type { KernelSpecAPI, ServerConnection, Session } from '@jupyterlab/services'; import type ThebeSession from './session'; -import type { IOutput } from '@jupyterlab/nbformat'; +import type { IOutput, IError } from '@jupyterlab/nbformat'; import type { IRenderMimeRegistry } from '@jupyterlab/rendermime'; import type ThebeServer from './server'; import type { ServerStatusEvent } from './events'; @@ -104,7 +104,7 @@ export interface IThebeCellExecuteReturn { id: string; height: number; width: number; - error: boolean; + error?: IError[]; } export interface ServerRuntime { diff --git a/packages/react/src/hooks/notebook.ts b/packages/react/src/hooks/notebook.ts index 013bb5f0..6c64f018 100644 --- a/packages/react/src/hooks/notebook.ts +++ b/packages/react/src/hooks/notebook.ts @@ -1,23 +1,40 @@ import { createRef, useEffect, useState } from 'react'; -import type { ThebeNotebook, ThebeSession, IThebeCell } from 'thebe-core'; +import type { ThebeNotebook, ThebeSession, IThebeCell, IThebeCellExecuteReturn } from 'thebe-core'; import { useThebeConfig } from '../ThebeServerProvider'; import { useThebeCore } from '../ThebeCoreProvider'; import type { INotebookContent } from '@jupyterlab/nbformat'; import { useThebeSession } from '../ThebeSessionProvider'; -interface ExecuteOptions { +export interface NotebookExecuteOptions { + stopOnError?: boolean; before?: () => void; after?: () => void; preprocessor?: (s: string) => string; } -function useNotebookBase(opts = { refsForWidgetsOnly: true }) { +export type IThebeNotebookError = IThebeCellExecuteReturn & { index: number }; + +function findErrors(execReturns: (IThebeCellExecuteReturn | null)[]) { + return execReturns.reduce( + (acc, retval: IThebeCellExecuteReturn | null, index) => { + if (retval?.error) { + if (acc == null) return [{ ...retval, index }]; + else return [...acc, { ...retval, index }]; + } + return acc; + }, + null, + ); +} + +function useNotebookBase() { const { session } = useThebeSession(); const [notebook, setNotebook] = useState(); const [refs, setRefs] = useState<((node: HTMLDivElement) => void)[]>([]); const [sessionAttached, setSessionAttached] = useState(false); const [executing, setExecuting] = useState(false); const [executed, setExecuted] = useState(false); + const [errors, setErrors] = useState(null); /** * When the notebook and session is avaiable, attach to session @@ -28,29 +45,40 @@ function useNotebookBase(opts = { refsForWidgetsOnly: true }) { setSessionAttached(true); }, [notebook, session]); - const executeAll = (options?: ExecuteOptions) => { + const executeAll = (options?: NotebookExecuteOptions) => { if (!notebook) throw new Error('executeAll called before notebook available'); options?.before?.(); setExecuting(true); - return notebook.executeAll(true, options?.preprocessor).then((exec_return) => { - options?.after?.(); - setExecuted(true); - setExecuting(false); - return exec_return; - }); + return notebook + .executeAll(options?.stopOnError ?? true, options?.preprocessor) + .then((execReturns) => { + options?.after?.(); + const errs = findErrors(execReturns); + if (errs != null) setErrors(errs); + setExecuted(true); + setExecuting(false); + return execReturns; + }); }; - const executeSome = (predicate: (cell: IThebeCell) => boolean, options?: ExecuteOptions) => { + const executeSome = ( + predicate: (cell: IThebeCell) => boolean, + options?: NotebookExecuteOptions, + ) => { if (!notebook) throw new Error('executeSome called before notebook available'); options?.before?.(); setExecuting(true); const filteredCells = notebook.cells.filter(predicate).map((c) => c.id); - return notebook.executeCells(filteredCells, true, options?.preprocessor).then((exec_return) => { - options?.after?.(); - setExecuted(true); - setExecuting(false); - return exec_return; - }); + return notebook + .executeCells(filteredCells, options?.stopOnError ?? true, options?.preprocessor) + .then((execReturns) => { + options?.after?.(); + const errs = findErrors(execReturns); + if (errs != null) setErrors(errs); + setExecuted(true); + setExecuting(false); + return execReturns; + }); }; const clear = () => { @@ -64,6 +92,7 @@ function useNotebookBase(opts = { refsForWidgetsOnly: true }) { attached: sessionAttached, executing, executed, + errors, notebook, setNotebook, refs, @@ -95,6 +124,7 @@ export function useNotebook( attached, executing, executed, + errors, notebook, setNotebook, refs, @@ -103,7 +133,7 @@ export function useNotebook( executeSome, clear, session, - } = useNotebookBase(opts); + } = useNotebookBase(); /** * - set loading flag @@ -141,6 +171,7 @@ export function useNotebook( attached, executing, executed, + errors, notebook, cellRefs: refs, cellIds: (opts.refsForWidgetsOnly ? notebook?.widgets ?? [] : notebook?.cells ?? []).map( @@ -167,6 +198,7 @@ export function useNotebookFromSource(sourceCode: string[], opts = { refsForWidg attached, executing, executed, + errors, notebook, setNotebook, refs, @@ -175,7 +207,7 @@ export function useNotebookFromSource(sourceCode: string[], opts = { refsForWidg executeSome, clear, session, - } = useNotebookBase(opts); + } = useNotebookBase(); useEffect(() => { if (!core || !config) return; @@ -203,6 +235,7 @@ export function useNotebookFromSource(sourceCode: string[], opts = { refsForWidg attached, executing, executed, + errors, notebook, cellRefs: refs, cellIds: (opts.refsForWidgetsOnly ? notebook?.widgets ?? [] : notebook?.cells ?? []).map(