Skip to content

Commit ba12764

Browse files
VishakBaddurpre-commit-ci[bot]dmadisetti
authored
fix: clear error state on disabled-transitively cells when ancestor recovers (#8784)
Fixes #8072 ## Root Cause When a disabled-transitively cell's ancestor had an error and then recovered, the disabled cell permanently showed the ancestor's error state. `run_stale_cells()` in `runtime.py` only re-queues non-disabled cells: ```python if cell_impl.stale and not self.graph.is_disabled(cid): cells_to_run.add(cid) ``` So disabled-transitively cells never got re-queued and never had a chance to reset their `run_result_status` from `"exception"` to `"disabled"`. ## Fix - Added `is_any_ancestor_errored()` to `DirectedGraph` - In `run_stale_cells()`, after building `cells_to_run`, reset `run_result_status` to `"disabled"` for any disabled-transitively cell whose ancestor no longer has an error ## Testing Added `test_is_any_ancestor_errored` to `tests/_runtime/test_dataflow.py` verifying the new graph method correctly detects and clears ancestor error states. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: dmadisetti <dmadisetti@coreweave.com>
1 parent 0f934c1 commit ba12764

5 files changed

Lines changed: 138 additions & 1 deletion

File tree

frontend/playwright.config.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ const appToOptions = {
3838
"bugs.py": { command: "edit" },
3939
"cells.py": { command: "edit" },
4040
"disabled_cells.py": { command: "edit" },
41+
"disabled_ancestor_error.py": { command: "edit" },
4142
"kitchen_sink.py": { command: "edit" },
4243
"layout_grid.py": { command: "edit" },
4344
"stdin.py": { command: "edit" },

marimo/_runtime/dataflow/graph.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,14 @@ def is_any_ancestor_disabled(self, cell_id: CellId_t) -> bool:
142142
for cid in self.ancestors(cell_id)
143143
)
144144

145+
def is_any_ancestor_errored(self, cell_id: CellId_t) -> bool:
146+
"""Check if any ancestor of a cell has an error."""
147+
return any(
148+
self.topology.cells[cid].run_result_status
149+
in ("exception", "marimo-error")
150+
for cid in self.ancestors(cell_id)
151+
)
152+
145153
def disable_cell(self, cell_id: CellId_t) -> None:
146154
"""Disables a cell in the graph.
147155

marimo/_runtime/runtime.py

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
from uuid import uuid4
2626

2727
from marimo import _loggers
28-
from marimo._ast.cell import CellConfig, CellImpl
28+
from marimo._ast.cell import CellConfig, CellImpl, RuntimeStateType
2929
from marimo._ast.compiler import _build_source_position_map, compile_cell
3030
from marimo._ast.errors import ImportStarError
3131
from marimo._ast.names import SETUP_CELL_NAME
@@ -1401,12 +1401,39 @@ async def _run_cells(self, cell_ids: set[CellId_t]) -> None:
14011401
# common cases. We could also be more aggressive and run this before
14021402
# every cell, or even before pickle.dump/pickle.dumps()
14031403
with patches.patch_main_module_context(self._module):
1404+
# Snapshot disabled cells that are in an error/cancelled state
1405+
# BEFORE running, so we can clear them after the run if their
1406+
# ancestor recovered.
1407+
pre_run_errored_disabled = {
1408+
cid
1409+
for cid, cell in self.graph.cells.items()
1410+
if self.graph.is_disabled(cid)
1411+
and cell.run_result_status
1412+
in ("exception", "marimo-error", "cancelled")
1413+
}
14041414
while cell_ids := await self._run_cells_internal(cell_ids):
14051415
LOGGER.debug("Running state updates ...")
14061416
if self.lazy() and cell_ids:
14071417
self.graph.set_stale(cell_ids, prune_imports=True)
14081418
break
14091419
LOGGER.debug("Finished run.")
1420+
# Clear stale error state from disabled cells whose ancestor
1421+
# recovered. Uses pre-run snapshot since run_result_status is
1422+
# updated during the run.
1423+
for cid in pre_run_errored_disabled:
1424+
cell_impl = self.graph.cells[cid]
1425+
if not self.graph.is_any_ancestor_errored(cid):
1426+
cell_impl.set_run_result_status("disabled")
1427+
status = cast(
1428+
RuntimeStateType,
1429+
"idle"
1430+
if cell_impl.config.disabled
1431+
else "disabled-transitively",
1432+
)
1433+
cell_impl.set_runtime_state(status)
1434+
CellNotificationUtils.broadcast_empty_output(
1435+
cell_id=cid, status=status
1436+
)
14101437

14111438
async def _if_autorun_then_run_cells(
14121439
self, cell_ids: set[CellId_t]
@@ -1814,6 +1841,7 @@ async def run_stale_cells(self) -> None:
18141841
relatives=dataflow.get_import_block_relatives(self.graph),
18151842
)
18161843
)
1844+
18171845
if self.module_watcher is not None:
18181846
self.module_watcher.run_is_processed.set()
18191847

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
# /// script
2+
# [tool.marimo.runtime]
3+
# auto_instantiate = true
4+
# ///
5+
6+
import marimo
7+
8+
__generated_with = "0.23.1"
9+
app = marimo.App()
10+
11+
12+
@app.cell
13+
def _():
14+
import marimo as mo
15+
16+
toggle = mo.ui.checkbox(label="Trigger ancestor error")
17+
toggle
18+
return (toggle,)
19+
20+
21+
@app.cell
22+
def _(toggle):
23+
if toggle.value:
24+
raise ValueError("ancestor error")
25+
26+
x = 1
27+
return (x,)
28+
29+
30+
@app.cell(disabled=True)
31+
def _(x):
32+
y = x + 1
33+
return (y,)
34+
35+
36+
@app.cell
37+
def _(y):
38+
z = y + 1
39+
return
40+
41+
42+
@app.cell
43+
def _():
44+
return
45+
46+
47+
if __name__ == "__main__":
48+
app.run()

tests/_runtime/test_dataflow.py

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1579,3 +1579,55 @@ def test_prune_cells_for_overrides_preserves_order() -> None:
15791579
graph, execution_order, {"b": 100}
15801580
)
15811581
assert result == ["0", "2", "3"]
1582+
1583+
1584+
def test_is_any_ancestor_errored() -> None:
1585+
"""Test that is_any_ancestor_errored correctly detects ancestor errors."""
1586+
graph = dataflow.DirectedGraph()
1587+
# Create a chain: 0 -> 1 -> 2
1588+
code = "x = 0"
1589+
first_cell = parse_cell(code)
1590+
graph.register_cell("0", first_cell)
1591+
code = "y = x"
1592+
second_cell = parse_cell(code)
1593+
graph.register_cell("1", second_cell)
1594+
code = "z = y"
1595+
third_cell = parse_cell(code)
1596+
graph.register_cell("2", third_cell)
1597+
1598+
# No errors initially
1599+
assert not graph.is_any_ancestor_errored("0")
1600+
assert not graph.is_any_ancestor_errored("1")
1601+
assert not graph.is_any_ancestor_errored("2")
1602+
1603+
# Set cell 0 to exception state
1604+
graph.cells["0"].set_run_result_status("exception")
1605+
assert not graph.is_any_ancestor_errored("0") # no ancestors
1606+
assert graph.is_any_ancestor_errored("1") # parent 0 has error
1607+
assert graph.is_any_ancestor_errored("2") # grandparent 0 has error
1608+
1609+
# Fix cell 0 - clear the error
1610+
graph.cells["0"].set_run_result_status("success")
1611+
assert not graph.is_any_ancestor_errored("0")
1612+
assert not graph.is_any_ancestor_errored("1")
1613+
assert not graph.is_any_ancestor_errored("2")
1614+
1615+
1616+
def test_is_any_ancestor_errored_marimo_error() -> None:
1617+
"""Test that is_any_ancestor_errored detects marimo-error status too."""
1618+
graph = dataflow.DirectedGraph()
1619+
code = "x = 0"
1620+
first_cell = parse_cell(code)
1621+
graph.register_cell("0", first_cell)
1622+
code = "y = x"
1623+
second_cell = parse_cell(code)
1624+
graph.register_cell("1", second_cell)
1625+
1626+
# Set cell 0 to marimo-error state (e.g. registration/syntax error)
1627+
graph.cells["0"].set_run_result_status("marimo-error")
1628+
assert not graph.is_any_ancestor_errored("0") # no ancestors
1629+
assert graph.is_any_ancestor_errored("1") # parent 0 has marimo-error
1630+
1631+
# Fix cell 0
1632+
graph.cells["0"].set_run_result_status("success")
1633+
assert not graph.is_any_ancestor_errored("1")

0 commit comments

Comments
 (0)