Skip to content
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.

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

🛳 exposing any errors during execution in execute return object #601

Merged
merged 1 commit into from
Feb 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 5 additions & 4 deletions packages/core/src/cell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand All @@ -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') {
Expand All @@ -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),
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/cell_noexec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,6 @@ export default class NonExecutableCell implements IThebeCell {

async execute(source?: string): Promise<IThebeCellExecuteReturn | null> {
// 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 };
}
}
14 changes: 4 additions & 10 deletions packages/core/src/notebook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -177,7 +171,7 @@ class ThebeNotebook {
async executeOnly(
cellId: string,
preprocessor?: (s: string) => string,
): Promise<ExecuteReturn | null> {
): Promise<IThebeCellExecuteReturn | null> {
if (!this.cells) return null;
this.events.triggerStatus({
status: NotebookStatusEvent.executing,
Expand All @@ -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,
Expand Down Expand Up @@ -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({
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/types.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -104,7 +104,7 @@ export interface IThebeCellExecuteReturn {
id: string;
height: number;
width: number;
error: boolean;
error?: IError[];
}

export interface ServerRuntime {
Expand Down
71 changes: 52 additions & 19 deletions packages/react/src/hooks/notebook.ts
Original file line number Diff line number Diff line change
@@ -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<IThebeNotebookError[] | null>(
(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<ThebeNotebook | undefined>();
const [refs, setRefs] = useState<((node: HTMLDivElement) => void)[]>([]);
const [sessionAttached, setSessionAttached] = useState(false);
const [executing, setExecuting] = useState<boolean>(false);
const [executed, setExecuted] = useState(false);
const [errors, setErrors] = useState<IThebeNotebookError[] | null>(null);

/**
* When the notebook and session is avaiable, attach to session
Expand All @@ -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 = () => {
Expand All @@ -64,6 +92,7 @@ function useNotebookBase(opts = { refsForWidgetsOnly: true }) {
attached: sessionAttached,
executing,
executed,
errors,
notebook,
setNotebook,
refs,
Expand Down Expand Up @@ -95,6 +124,7 @@ export function useNotebook(
attached,
executing,
executed,
errors,
notebook,
setNotebook,
refs,
Expand All @@ -103,7 +133,7 @@ export function useNotebook(
executeSome,
clear,
session,
} = useNotebookBase(opts);
} = useNotebookBase();

/**
* - set loading flag
Expand Down Expand Up @@ -141,6 +171,7 @@ export function useNotebook(
attached,
executing,
executed,
errors,
notebook,
cellRefs: refs,
cellIds: (opts.refsForWidgetsOnly ? notebook?.widgets ?? [] : notebook?.cells ?? []).map(
Expand All @@ -167,6 +198,7 @@ export function useNotebookFromSource(sourceCode: string[], opts = { refsForWidg
attached,
executing,
executed,
errors,
notebook,
setNotebook,
refs,
Expand All @@ -175,7 +207,7 @@ export function useNotebookFromSource(sourceCode: string[], opts = { refsForWidg
executeSome,
clear,
session,
} = useNotebookBase(opts);
} = useNotebookBase();

useEffect(() => {
if (!core || !config) return;
Expand Down Expand Up @@ -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(
Expand Down