Skip to content

refactor: consolidate Runner and app ScriptRunner#9617

Open
dmadisetti wants to merge 8 commits into
dm/executor-implfrom
dm/runner-cleanup
Open

refactor: consolidate Runner and app ScriptRunner#9617
dmadisetti wants to merge 8 commits into
dm/executor-implfrom
dm/runner-cleanup

Conversation

@dmadisetti
Copy link
Copy Markdown
Collaborator

@dmadisetti dmadisetti commented May 19, 2026

📝 Summary

This PR cleans up dataflow::Runner into functions that directly utilize the evaluator pipeline introduced in #9616.
Additionally, it consolidates AppScriptRunner to use this logic as well.

A note, this refactor prevents AppScriptRunner from classifying mo.stop as an exception. The previous behavior would throw and cause a total script stop- the revised behavior (and desired), is that the dependency tree is cancelled while the rest of the notebook can compute.

@vercel
Copy link
Copy Markdown

vercel Bot commented May 19, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
marimo-docs Ready Ready Preview, Comment May 21, 2026 1:46am

Request Review

@dmadisetti dmadisetti added the internal A refactor or improvement that is not user facing label May 19, 2026
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 10 files

Architecture diagram
sequenceDiagram
    participant Client as App/Cell API
    participant by_kwargs as by_kwargs module
    participant Evaluator as Evaluator
    participant Executor as DefaultExecutor
    participant Scheduler as SequentialScheduler
    participant Graph as DirectedGraph

    Note over Client,Graph: NEW: Cell.run(**kwargs) public API path

    Client->>by_kwargs: run_cell_sync(graph, cellId, kwargs)
    by_kwargs->>Graph: cells[cellId].is_coroutine()
    alt Cell is coroutine
        by_kwargs->>by_kwargs: Raise RuntimeError
    else Sync cell
        by_kwargs->>by_kwargs: _validate_kwargs(cell, kwargs)
        by_kwargs->>Graph: Get ancestor closure (pruned by substitutions)
        by_kwargs->>by_kwargs: topological_sort(graph, ancestors)
        by_kwargs->>Evaluator: evaluate_sync(ancestorCell, glbls)
        Evaluator->>Executor: execute_cell(ancestorCell, glbls)
        loop For each ancestor
            Executor-->>Evaluator: glbls updated
        end
        by_kwargs->>by_kwargs: _substitute_refs(cell, glbls, kwargs)
        by_kwargs->>Evaluator: evaluate_sync(targetCell, glbls)
        Evaluator->>Executor: execute_cell(targetCell, glbls)
        Executor-->>Evaluator: output
        by_kwargs-->>Client: (output, definitions)
    end

    Note over Client,Scheduler: NEW: ScriptRunner consolidated flow

    Client->>Scheduler: __init__(cells_to_run, graph)
    Scheduler->>Scheduler: Pop next cell in order
    loop For each cell
        Scheduler-->>Client: pop_cell()
        alt Cell is cancelled
            Client->>Scheduler: cancelled(cid) == True
            Client->>Client: Skip execution
        else Cell not cancelled
            Client->>Evaluator: evaluate_sync(cell, glbls)
            Evaluator->>Executor: execute_cell(cell, glbls)
            alt Execution succeeds
                Executor-->>Evaluator: output
                Evaluator-->>Client: RunResult(output=..., exception=None)
                Client->>Client: _handle_run_result: record output
            else Execution raises MarimoRuntimeException
                Evaluator-->>Client: RunResult(exception=MarimoRuntimeException)
                Client->>Client: unwrap_user_exception()
                alt MarimoStopError
                    Client->>Client: Record stop output
                    Client->>Scheduler: cancel(cid)
                    Scheduler->>Scheduler: Mark descendants cancelled
                else Other exception
                    Client->>Client: Re-raise exception
                end
            end
        end
    end
    Client-->>Client: Return (outputs, glbls)

    Note over Evaluator,Executor: NEW: evaluate_sync & evaluate_interruptible added

    Client->>Evaluator: evaluate_interruptible(cell, glbls)
    alt Cell is coroutine & main thread
        Evaluator->>Evaluator: Create future, wrap with SIGINT handler
        Evaluator->>Evaluator: asyncio.ensure_future(evaluate(...))
        alt SIGINT received
            Evaluator->>Evaluator: Cancel future → CancelledError
        else Normal completion
            Evaluator-->>Client: RunResult
        end
    else Sync cell or not main thread
        Evaluator->>Evaluator: evaluate(cell, glbls) directly
        Evaluator-->>Client: RunResult
    end

    Note over Evaluator,Executor: CHANGED: lifecycle chain extracted to helper methods

    Client->>Evaluator: evaluate/evaluate_sync(cell, glbls)
    Evaluator->>Evaluator: _setup_chain(cell, glbls)
    loop For each lifecycle
        alt Lifecycle returns Skip
            Evaluator->>Evaluator: Record skip decision
        else Lifecycle completes
            Evaluator->>Evaluator: Append to completed list
        end
    end
    alt Skip was returned
        Evaluator->>Evaluator: value = skip.value
    else No skip
        Evaluator->>Executor: execute_cell / execute_cell_async(cell, glbls)
        Executor-->>Evaluator: output value
    end
    Evaluator->>Evaluator: _teardown_chain(cell, glbls, completed, value, exception)
    loop Reversed completed lifecycles
        Evaluator->>Evaluator: teardown(cell, glbls, result)
    end
    Evaluator-->>Client: RunResult
Loading

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread marimo/_runtime/executor/evaluator.py Outdated
Comment thread marimo/_ast/app.py
Comment thread marimo/_runtime/runner/by_kwargs.py Outdated
Comment thread marimo/_runtime/runner/by_kwargs.py Outdated
@dmadisetti dmadisetti force-pushed the dm/runner-cleanup branch from 6d9c8c1 to 0500e7d Compare May 20, 2026 18:27
@dmadisetti dmadisetti marked this pull request as ready for review May 20, 2026 18:28
dmadisetti added a commit that referenced this pull request May 20, 2026
## 📝 Summary

This fix is a followup to #9617 and #9616 and restores the truncated
stacktrace to the user's notebook.

---

Before fix:

<img width="1086" height="405" alt="image"
src="https://github.com/user-attachments/assets/39918a95-a4ec-4ca5-b701-d77fef42169f"
/>

After fix:

<img width="1088" height="397" alt="image"
src="https://github.com/user-attachments/assets/f73e98ba-7bfd-4559-bd34-ef0ea1515bbd"
/>
if exc is None:
return None
if isinstance(exc, MarimoRuntimeException) and isinstance(
exc.__cause__, MarimoStopError
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there ever a plain MarimoStopError that is not wrapped? maybe we can be defensive?

Comment thread marimo/_runtime/executor/executor.py Outdated
# Strip our own frame so user-facing tracebacks start at user
# code. Runners classify via ``__cause__``.
if e.__traceback__ is not None:
e.__traceback__ = e.__traceback__.tb_next
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we check if e.__traceback__.tb_next is not none either?
maybe we can pull out to a util up_one_traceback(e)


return self._teardown_chain(cell, glbls, completed, result)

async def evaluate_interruptible(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be tested

Comment thread marimo/_ast/cell.py
# Refresh the async decision with the caller's substitutions —
# an unsubstituted ancestor may have been async but isn't on
# this call's ancestor closure.
if by_kwargs.is_coroutine(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is no longer cached. intentional?

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 4 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread marimo/_runtime/executor/executor.py
dmadisetti and others added 7 commits May 20, 2026 16:56
## 📝 Summary

This fix is a followup to #9617 and #9616 and restores the truncated
stacktrace to the user's notebook.

---

Before fix:

<img width="1086" height="405" alt="image"
src="https://github.com/user-attachments/assets/39918a95-a4ec-4ca5-b701-d77fef42169f"
/>

After fix:

<img width="1088" height="397" alt="image"
src="https://github.com/user-attachments/assets/f73e98ba-7bfd-4559-bd34-ef0ea1515bbd"
/>
Adds behavior tests for four untested surfaces of the executor refactor,
and includes the one production-fix uncovered while planning.

Production fix (``marimo/_runtime/executor/executor.py``): let
``asyncio.CancelledError`` propagate unwrapped from
``DefaultExecutor.execute_cell{,_async}``. Wrapping it in
``MarimoRuntimeException`` defeated the bare-``CancelledError`` check
in ``Runner._finalize_run_result`` and prevented
``runner.interrupted`` from flipping on async SIGINT.

Tests:
- ``StrictLifecycle.setup`` ``Skip(result=...)`` path: 5 cases
  covering undefined ref, ref-before-def, graph-resolved blamed cell,
  private-var ``unmangle_local`` fallback, and the state invariant
  that the Skip early-return must not mutate globals or stash a
  backup (teardown then no-ops).
- ``unwrap_user_exception``: 6 cases, including the
  ``NameError.name is None`` guard. Construction note: bare
  ``NameError("name 'x' is not defined")`` does NOT set ``.name == 'x'``
  — only interpreter-raised NameErrors do. Tests set ``.name`` explicitly.
- Plugin executor dispatch via ``Runner``: registers a sentinel
  factory against ``_EXECUTOR_REGISTRY`` (fully isolated — both
  ``_plugins`` and ``names`` monkeypatched so installed third-party
  entry points can't shadow), asserts the runner runs the sentinel.
- SIGINT and ``runner.interrupted``: deterministic ``_cancel_on_sigint``
  tests via ``signal.signal``/``signal.getsignal`` monkeypatching
  (real signals are unsafe in pytest — the wrapper chains to the
  ``previously installed`` handler, which in tests is pytest's, not
  marimo's), plus the executor-level CancelledError-propagation test
  and runner-level tests for both sync ``MarimoInterrupt`` and async
  cancellation flipping the flag.

Resolves cubic's "this should be tested" comment on
``evaluator.py:97``.
- Inline ``Evaluator(...)`` at the two remaining ``build_evaluator``
  call sites (``by_kwargs._new_evaluator`` and
  ``AppScriptRunner.__init__``) so the helper removal on
  ``dm/executor-impl`` doesn't leave dangling imports here.
- Extract ``_strip_frame(e, count=1)`` util in ``executor.py`` for
  the executor-frame elision logic. Iterates ``count`` times and
  stops if the traceback runs out, never stripping the only frame
  we have — fixes the subtle issue where ``tb_next`` being ``None``
  would set ``__traceback__`` to ``None`` outright.
- ``_classify`` in ``by_kwargs`` now short-circuits on a bare
  ``MarimoStopError`` before the ``MarimoRuntimeException`` unwrap
  branch — any caller bypassing the default wrap (e.g. a custom
  ``Executor`` that raises ``MarimoStopError`` directly) still gets
  stop-control-flow handling.
## 📝 Summary

Replaces `dict[str, Any]` in the executor pipeline with `MutableGlobals`
(alias), and `Globals` (`Mapping[str, Any]`).

Layering on top to prevent rebase churn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal A refactor or improvement that is not user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants