Skip to content

Commit

Permalink
improvement: errors should stay internal in marimo run and logged to …
Browse files Browse the repository at this point in the history
…console (#3150)
  • Loading branch information
mscolnick authored Dec 13, 2024
1 parent de0b3f3 commit 6af7f91
Show file tree
Hide file tree
Showing 21 changed files with 408 additions and 43 deletions.
4 changes: 4 additions & 0 deletions frontend/src/components/editor/output/MarimoErrorOutput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,10 @@ export const MarimoErrorOutput = ({
</Tip>
</div>
);
case "internal":
titleContents = "An internal error occurred";
return <p key={idx}>{error.msg}</p>;

case "ancestor-prevented":
titleContents = "Ancestor prevented from running";
alertVariant = "default";
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/components/ui/toast.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const ToastViewport = React.forwardRef<
<ToastPrimitives.Viewport
ref={ref}
className={cn(
"fixed top-0 z-[100] flex max-h-screen w-full flex-col-reverse p-4 sm:bottom-0 sm:right-0 sm:top-auto sm:flex-col md:max-w-[420px]",
"fixed top-0 z-[100] flex max-h-screen w-full flex-col-reverse p-4 sm:bottom-0 sm:right-0 sm:top-auto sm:flex-col md:w-fit md:max-w-[420px]",
className,
)}
{...props}
Expand Down
16 changes: 16 additions & 0 deletions frontend/src/core/cells/logs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { CellMessage, OutputMessage } from "../kernel/messages";
import type { CellId } from "./ids";
import { fromUnixTime } from "date-fns";
import { isErrorMime } from "../mime";
import { toast } from "@/components/ui/use-toast";

export interface CellLog {
timestamp: number;
Expand All @@ -12,6 +13,8 @@ export interface CellLog {
cellId: CellId;
}

let didAlreadyToastError = false;

export function getCellLogsForMessage(cell: CellMessage): CellLog[] {
const logs: CellLog[] = [];
const consoleOutputs: OutputMessage[] = [cell.console].filter(Boolean).flat();
Expand Down Expand Up @@ -56,6 +59,19 @@ export function getCellLogsForMessage(cell: CellMessage): CellLog[] {
message: JSON.stringify(error),
});
});

const shouldToast = cell.output.data.some(
(error) => error.type === "internal",
);
if (!didAlreadyToastError && shouldToast) {
didAlreadyToastError = true;
toast({
title: "An internal error occurred",
description: "See console for details.",
className:
"text-xs text-background bg-[var(--red-10)] py-2 pl-3 [&>*]:flex [&>*]:gap-3",
});
}
}

return logs;
Expand Down
1 change: 1 addition & 0 deletions marimo/_cli/development/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ def _generate_schema() -> dict[str, Any]:
errors.MultipleDefinitionError,
errors.DeleteNonlocalError,
errors.MarimoInterruptionError,
errors.MarimoInternalError,
errors.MarimoAncestorStoppedError,
errors.MarimoAncestorPreventedError,
errors.MarimoStrictExecutionError,
Expand Down
45 changes: 45 additions & 0 deletions marimo/_messaging/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,50 @@ def describe(self) -> str:
return self.msg


@dataclass
class MarimoInternalError:
"""
An internal error that should be hidden from the user.
The error is logged to the console and then a new error is broadcasted
such that the data is hidden.
They can be linked back to the original error by the error_id.
"""

error_id: str
type: Literal["internal"] = "internal"
msg: str = ""

def __post_init__(self) -> None:
self.msg = f"An internal error occurred: {self.error_id}"

def describe(self) -> str:
return self.msg


def is_unexpected_error(error: Error) -> bool:
"""
These errors are unexpected, in that they are not intentional.
mo.stop and interrupt are intentional.
"""
return error.type not in [
"ancestor-prevented",
"ancestor-stopped",
"interruption",
]


def is_sensitive_error(error: Error) -> bool:
"""
These errors are sensitive, in that they are intentional.
"""
return error.type not in [
"ancestor-prevented",
"ancestor-stopped",
"internal",
]


Error = Union[
CycleError,
MultipleDefinitionError,
Expand All @@ -117,5 +161,6 @@ def describe(self) -> str:
MarimoStrictExecutionError,
MarimoInterruptionError,
MarimoSyntaxError,
MarimoInternalError,
UnknownError,
]
30 changes: 28 additions & 2 deletions marimo/_messaging/ops.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
Union,
cast,
)
from uuid import uuid4

from marimo import _loggers as loggers
from marimo._ast.app import _AppConfig
Expand All @@ -31,7 +32,11 @@
from marimo._dependencies.dependencies import DependencyManager
from marimo._messaging.cell_output import CellChannel, CellOutput
from marimo._messaging.completion_option import CompletionOption
from marimo._messaging.errors import Error
from marimo._messaging.errors import (
Error,
MarimoInternalError,
is_sensitive_error,
)
from marimo._messaging.mimetypes import KnownMimeType
from marimo._messaging.streams import OUTPUT_MAX_BYTES
from marimo._messaging.types import Stream
Expand All @@ -41,6 +46,7 @@
from marimo._plugins.ui._core.ui_element import UIElement
from marimo._plugins.ui._impl.tables.utils import get_table_manager_or_none
from marimo._runtime.context import get_context
from marimo._runtime.context.utils import get_mode
from marimo._runtime.layout.layout import LayoutConfig
from marimo._utils.platform import is_pyodide, is_windows

Expand Down Expand Up @@ -248,12 +254,32 @@ def broadcast_error(
cell_id: CellId_t,
) -> None:
console: Optional[list[CellOutput]] = [] if clear_console else None

# In run mode, we don't want to broadcast the error. Instead we want to print the error to the console
# and then broadcast a new error such that the data is hidden.
safe_errors: list[Error] = []
if get_mode() == "run":
for error in data:
# Skip non-sensitive errors
if not is_sensitive_error(error):
safe_errors.append(error)
continue

error_id = uuid4()
LOGGER.error(
f"(error_id={error_id}) {error.describe()}",
extra={"error_id": error_id},
)
safe_errors.append(MarimoInternalError(error_id=str(error_id)))
else:
safe_errors = list(data)

CellOp(
cell_id=cell_id,
output=CellOutput(
channel=CellChannel.MARIMO_ERROR,
mimetype="application/vnd.marimo+error",
data=data,
data=safe_errors,
),
console=console,
status=None,
Expand Down
17 changes: 14 additions & 3 deletions marimo/_runtime/context/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@

from marimo._output.rich_help import mddoc
from marimo._runtime.context import ContextNotInitializedError, get_context
from marimo._runtime.context.kernel_context import KernelRuntimeContext
from marimo._runtime.context.script_context import ScriptRuntimeContext
from marimo._server.model import SessionMode
from marimo._utils.assert_never import assert_never


@mddoc
Expand All @@ -18,6 +18,8 @@ def running_in_notebook() -> bool:
except ContextNotInitializedError:
return False
else:
from marimo._runtime.context.kernel_context import KernelRuntimeContext

return isinstance(ctx, KernelRuntimeContext)


Expand All @@ -29,9 +31,18 @@ def get_mode() -> Optional[Literal["run", "edit", "script"]]:
or None if marimo has no context initialized.
"""
try:
from marimo._runtime.context.kernel_context import KernelRuntimeContext
from marimo._runtime.context.script_context import ScriptRuntimeContext

context = get_context()
if isinstance(context, KernelRuntimeContext):
return context.session_mode # type: ignore
if context.session_mode == SessionMode.RUN:
return "run"
elif context.session_mode == SessionMode.EDIT:
return "edit"
else:
assert_never(context.session_mode)

if isinstance(context, ScriptRuntimeContext):
return "script"
except ContextNotInitializedError:
Expand Down
1 change: 1 addition & 0 deletions marimo/_server/asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,7 @@ def _create_app_for_file(base_url: str, file_path: str) -> ASGIApp:
# to each application
cli_args={},
auth_token=auth_token,
redirect_console_to_browser=False,
)
app = create_starlette_app(
base_url="",
Expand Down
28 changes: 15 additions & 13 deletions marimo/_server/export/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@
import asyncio
import sys
from dataclasses import dataclass
from typing import Any, Callable, Literal, Optional, cast
from typing import Any, Callable, Literal, Optional, Union, cast

from marimo._cli.print import echo
from marimo._config.manager import (
get_default_config_manager,
)
from marimo._messaging.cell_output import CellChannel
from marimo._messaging.errors import Error
from marimo._messaging.errors import Error, is_unexpected_error
from marimo._messaging.ops import MessageOperation
from marimo._messaging.types import KernelMessage
from marimo._output.hypertext import patch_html_for_non_interactive_output
Expand Down Expand Up @@ -109,18 +109,22 @@ def export_as_wasm(


async def run_app_then_export_as_ipynb(
path: MarimoPath,
path_or_file_manager: Union[MarimoPath, AppFileManager],
sort_mode: Literal["top-down", "topological"],
cli_args: SerializedCLIArgs,
) -> ExportResult:
file_router = AppFileRouter.from_filename(path)
file_key = file_router.get_unique_file_key()
assert file_key is not None
file_manager = file_router.get_file_manager(file_key)
if isinstance(path_or_file_manager, AppFileManager):
file_manager = path_or_file_manager
else:
file_router = AppFileRouter.from_filename(path_or_file_manager)
file_key = file_router.get_unique_file_key()
assert file_key is not None
file_manager = file_router.get_file_manager(file_key)

with patch_html_for_non_interactive_output():
(session_view, did_error) = await run_app_until_completion(
file_manager, cli_args
file_manager,
cli_args,
)

result = Exporter().export_as_ipynb(
Expand All @@ -146,7 +150,8 @@ async def run_app_then_export_as_html(

config = get_default_config_manager(current_path=file_manager.path)
session_view, did_error = await run_app_until_completion(
file_manager, cli_args
file_manager,
cli_args,
)
# Export the session as HTML
html, filename = Exporter().export_as_html(
Expand Down Expand Up @@ -222,10 +227,7 @@ class Container:
for err in errors:
parsed = parse_raw({"error": err}, Container)
# Not all errors are fatal
if parsed.error.type not in [
"ancestor-prevented",
"ancestor-stopped",
]:
if is_unexpected_error(parsed.error):
echo(
f"{parsed.error.__class__.__name__}: {parsed.error.describe()}",
file=sys.stderr,
Expand Down
4 changes: 2 additions & 2 deletions marimo/_server/sessions.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ def __init__(
app_metadata: AppMetadata,
user_config_manager: MarimoConfigReader,
virtual_files_supported: bool,
redirect_console_to_browser: bool = False,
redirect_console_to_browser: bool,
) -> None:
self.kernel_task: Optional[threading.Thread] | Optional[mp.Process]
self.queue_manager = queue_manager
Expand Down Expand Up @@ -661,7 +661,7 @@ def __init__(
user_config_manager: MarimoConfigReader,
cli_args: SerializedCLIArgs,
auth_token: Optional[AuthToken],
redirect_console_to_browser: bool = False,
redirect_console_to_browser: bool,
) -> None:
self.file_router = file_router
self.mode = mode
Expand Down
16 changes: 16 additions & 0 deletions openapi/api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ components:
- $ref: '#/components/schemas/MarimoStrictExecutionError'
- $ref: '#/components/schemas/MarimoInterruptionError'
- $ref: '#/components/schemas/MarimoSyntaxError'
- $ref: '#/components/schemas/MarimoInternalError'
- $ref: '#/components/schemas/UnknownError'
ExecuteMultipleRequest:
properties:
Expand Down Expand Up @@ -1269,6 +1270,21 @@ components:
- name
- path
type: object
MarimoInternalError:
properties:
error_id:
type: string
msg:
type: string
type:
enum:
- internal
type: string
required:
- error_id
- type
- msg
type: object
MarimoInterruptionError:
properties:
type:
Expand Down
7 changes: 7 additions & 0 deletions openapi/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2249,6 +2249,7 @@ export interface components {
| components["schemas"]["MarimoStrictExecutionError"]
| components["schemas"]["MarimoInterruptionError"]
| components["schemas"]["MarimoSyntaxError"]
| components["schemas"]["MarimoInternalError"]
| components["schemas"]["UnknownError"];
ExecuteMultipleRequest: {
cellIds: string[];
Expand Down Expand Up @@ -2555,6 +2556,12 @@ export interface components {
path: string;
sessionId?: string | null;
};
MarimoInternalError: {
error_id: string;
msg: string;
/** @enum {string} */
type: "internal";
};
MarimoInterruptionError: {
/** @enum {string} */
type: "interruption";
Expand Down
5 changes: 3 additions & 2 deletions tests/_runtime/test_app_mode.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@
from marimo._runtime.context.kernel_context import KernelRuntimeContext
from marimo._runtime.context.script_context import ScriptRuntimeContext
from marimo._runtime.context.utils import get_mode
from marimo._server.model import SessionMode


def test_get_mode_kernel_run():
"""Test get_mode() returns 'run' when in kernel run mode"""
mock_context = Mock(spec=KernelRuntimeContext)
mock_context.session_mode = "run"
mock_context.session_mode = SessionMode.RUN

with patch(
"marimo._runtime.context.utils.get_context", return_value=mock_context
Expand All @@ -24,7 +25,7 @@ def test_get_mode_kernel_run():
def test_get_mode_kernel_edit():
"""Test get_mode() returns 'edit' when in kernel edit mode"""
mock_context = Mock(spec=KernelRuntimeContext)
mock_context.session_mode = "edit"
mock_context.session_mode = SessionMode.EDIT

with patch(
"marimo._runtime.context.utils.get_context", return_value=mock_context
Expand Down
Loading

0 comments on commit 6af7f91

Please sign in to comment.