Skip to content

Commit

Permalink
fix operator execution error being suppressed
Browse files Browse the repository at this point in the history
  • Loading branch information
imanjra committed Aug 10, 2024
1 parent 7af4330 commit 51e3579
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 28 deletions.
16 changes: 14 additions & 2 deletions app/packages/operators/src/operators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ export class OperatorResult {
public result: object = {},
public executor: Executor = null,
public error: string,
public delegated: boolean = false
public delegated: boolean = false,
public errorMessage: string = null
) {}
hasOutputContent() {
if (this.delegated) return false;
Expand Down Expand Up @@ -657,6 +658,7 @@ export async function executeOperatorWithContext(

let result;
let error;
let errorMessage;
let executor;
let delegated = false;

Expand All @@ -667,13 +669,15 @@ export async function executeOperatorWithContext(
result = serverResult.result;
delegated = serverResult.delegated;
error = serverResult.error;
errorMessage = serverResult.error_message;
} catch (e) {
const isAbortError =
e.name === "AbortError" || e instanceof DOMException;
if (!isAbortError) {
error = e;
console.error(`Error executing operator ${operatorURI}:`);
console.error(error);
errorMessage = error.message;
}
}
} else {
Expand All @@ -700,6 +704,7 @@ export async function executeOperatorWithContext(
);
result = serverResult.result;
error = serverResult.error;
errorMessage = serverResult.error_message;
executor = serverResult.executor;
delegated = serverResult.delegated;
}
Expand Down Expand Up @@ -741,7 +746,14 @@ export async function executeOperatorWithContext(
error,
});

return new OperatorResult(operator, result, executor, error, delegated);
return new OperatorResult(
operator,
result,
executor,
error,
delegated,
errorMessage
);
}

type CurrentContext = {
Expand Down
3 changes: 2 additions & 1 deletion app/packages/operators/src/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -921,13 +921,14 @@ export function useOperatorExecutor(uri, handlers: any = {}) {
callback?.(new OperatorResult(operator, null, ctx.executor, e, false));
const isAbortError =
e.name === "AbortError" || e instanceof DOMException;
const msg = e.message || "Failed to execute an operation";
if (!isAbortError) {
setError(e);
setResult(null);
handlers.onError?.(e);
console.error("Error executing operator", operator, ctx);
console.error(e);
notify({ msg: e.message, variant: "error" });
notify({ msg, variant: "error" });
}
}
setHasExecuted(true);
Expand Down
38 changes: 20 additions & 18 deletions app/packages/operators/src/useCustomPanelHooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ export function useCustomPanelHooks(props: CustomPanelProps): CustomPanelHooks {
data: panelStateLocal?.data,
});
const panelSchema = panelStateLocal?.schema;
const view = useRecoilValue(fos.view);
const panelsStateUpdatesCount = useRecoilValue(panelsStateUpdatesCountAtom);
const lastPanelLoadState = useRef({
count: panelsStateUpdatesCount,
Expand All @@ -82,6 +81,7 @@ export function useCustomPanelHooks(props: CustomPanelProps): CustomPanelHooks {
const isLoaded: boolean = useMemo(() => {
return panelStateLocal?.loaded;
}, [panelStateLocal?.loaded]);
const triggerPanelEvent = usePanelEvent();

const onLoad = useCallback(() => {
if (props.onLoad && !isLoaded) {
Expand Down Expand Up @@ -140,11 +140,22 @@ export function useCustomPanelHooks(props: CustomPanelProps): CustomPanelHooks {

useEffect(() => {
onLoad();
}, [
panelId,
onLoad,
props.onUnLoad,
isLoaded,
setPanelStateLocal,
triggerPanelEvent,
]);

useEffect(() => {
return () => {
if (props.onUnLoad)
executeOperator(props.onUnLoad, { panel_id: panelId });
if (props.onUnLoad) {
triggerPanelEvent(panelId, { operator: props.onUnLoad });
}
};
}, [panelId, props.onLoad, props.onUnLoad, isLoaded, setPanelStateLocal]);
}, []); // eslint-disable-line react-hooks/exhaustive-deps

// Trigger panel "onLoad" operator when panel state changes externally
useEffect(() => {
Expand All @@ -170,22 +181,15 @@ export function useCustomPanelHooks(props: CustomPanelProps): CustomPanelHooks {
const handlePanelStateChangeOpDebounced = useMemo(() => {
return debounce(
(state, onChange, panelId) => {
if (onChange && state)
executeOperator(onChange, { panel_id: panelId, panel_state: state });
if (onChange && state) {
triggerPanelEvent(panelId, { operator: onChange });
}
},
PANEL_STATE_CHANGE_DEBOUNCE,
{ leading: true }
);
}, []);

useEffect(() => {
if (props.onViewChange)
executeOperator(props.onViewChange, {
panel_id: panelId,
panel_state: panelState?.state,
});
}, [view]);

useEffect(() => {
handlePanelStateChangeOpDebounced(
panelState?.state,
Expand All @@ -199,8 +203,6 @@ export function useCustomPanelHooks(props: CustomPanelProps): CustomPanelHooks {
handlePanelStateChangeOpDebounced,
]);

const triggerPanelPropertyChange = usePanelEvent();

const handlePanelStateChange = (newState) => {
setCustomPanelState((state: any) => {
return merge({}, state, newState);
Expand All @@ -213,14 +215,14 @@ export function useCustomPanelHooks(props: CustomPanelProps): CustomPanelHooks {
// This timeout allows the change to be applied before executing the operator
// it might make sense to do this for all operator executions
setTimeout(() => {
triggerPanelPropertyChange(panelId, {
triggerPanelEvent(panelId, {
operator: schema.onChange,
params: { path, value },
});
}, 0);
}
},
[triggerPanelPropertyChange, panelId]
[triggerPanelEvent, panelId]
);

return {
Expand Down
18 changes: 16 additions & 2 deletions app/packages/operators/src/usePanelEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { usePanelStateByIdCallback } from "@fiftyone/spaces";
import { executeOperator } from "./operators";
import { usePromptOperatorInput } from "./state";
import { ExecutionCallback } from "./types-internal";
import { useNotification } from "@fiftyone/state";

type HandlerOptions = {
params: any;
Expand All @@ -13,6 +14,7 @@ type HandlerOptions = {

export default function usePanelEvent() {
const promptForOperator = usePromptOperatorInput();
const notify = useNotification();
return usePanelStateByIdCallback((panelId, panelState, args) => {
const options = args[0] as HandlerOptions;
const { params, operator, prompt } = options;
Expand All @@ -21,10 +23,22 @@ export default function usePanelEvent() {
panel_id: panelId,
panel_state: panelState?.state || {},
};

const eventCallback = (result) => {
const msg =
result.errorMessage || result.error || "Failed to execute operation";
const computedMsg = `${msg} (operation: ${operator})`;
if (result?.error) {
notify({ msg: computedMsg, variant: "error" });
console.error(result?.error);
}
options?.callback?.(result);
};

if (prompt) {
promptForOperator(operator, actualParams, { callback: options.callback });
promptForOperator(operator, actualParams, { callback: eventCallback });
} else {
executeOperator(operator, actualParams, { callback: options.callback });
executeOperator(operator, actualParams, { callback: eventCallback });
}
});
}
18 changes: 13 additions & 5 deletions fiftyone/operators/executor.py
Original file line number Diff line number Diff line change
Expand Up @@ -281,16 +281,20 @@ async def execute_or_delegate_operator(
else None
)
return execution
except:
except Exception as error:
return ExecutionResult(
executor=executor, error=traceback.format_exc()
executor=executor,
error=traceback.format_exc(),
error_message=str(error),
)
else:
try:
result = await do_execute_operator(operator, ctx, exhaust=exhaust)
except:
except Exception as error:
return ExecutionResult(
executor=executor, error=traceback.format_exc()
executor=executor,
error=traceback.format_exc(),
error_message=str(error),
)

return ExecutionResult(result=result, executor=executor)
Expand Down Expand Up @@ -839,7 +843,8 @@ class ExecutionResult(object):
Args:
result (None): the execution result
executor (None): an :class:`Executor`
error (None): an error message
error (None): an error traceback, if an error occurred
error_message (None): an error message, if an error occurred
validation_ctx (None): a :class:`ValidationContext`
delegated (False): whether execution was delegated
outputs_schema (None): a JSON dict representing the output schema of the operator
Expand All @@ -850,13 +855,15 @@ def __init__(
result=None,
executor=None,
error=None,
error_message=None,
validation_ctx=None,
delegated=False,
outputs_schema=None,
):
self.result = result
self.executor = executor
self.error = error
self.error_message = error_message
self.validation_ctx = validation_ctx
self.delegated = delegated
self.outputs_schema = outputs_schema
Expand Down Expand Up @@ -904,6 +911,7 @@ def to_json(self):
"result": self.result,
"executor": self.executor.to_json() if self.executor else None,
"error": self.error,
"error_message": self.error_message,
"delegated": self.delegated,
"validation_ctx": (
self.validation_ctx.to_json() if self.validation_ctx else None
Expand Down

0 comments on commit 51e3579

Please sign in to comment.