[GH-ISSUE #1347] refactor labs to remove widget-in-gated-cell pattern across all 33 labs #4380

New Issue

GiteaMirror · 2026-04-19T12:24:07-05:00

GiteaMirror commented

2026-04-19 12:24:07 -05:00

Originally created by @profvjreddi on GitHub (Apr 16, 2026).
Original GitHub issue: https://github.com/harvard-edge/cs249r_book/issues/1347

context

an ast audit of all 33 labs (via the new TestMarimoDataflow.test_no_widget_defined_in_gated_cell check in #1346) finds that 32 of 33 labs have the widget-in-gated-cell anti-pattern. the one clean lab, vol2/lab_05_dist_train.py, is the canonical reference for the proper pattern.

the pattern looks like this:

@app.cell
def _(mo, partA_prediction):
    mo.stop(partA_prediction.value is None, mo.md("..."))
    partB_prediction = mo.ui.radio(...)  # defined inside gated cell
    return (partB_prediction,)

when partA_prediction is unanswered, mo.stop() halts the cell. partB_prediction never gets defined. every cell that depends on it cascades into an undefined state. in lab_01's pre-fix state, this cascade was severe enough to block all instrument widgets; #1339 fixed lab_01 specifically. other labs tolerate the pattern only because users answer predictions in order.

the proper pattern

from vol2/lab_05_dist_train.py:

@app.cell
def _(mo):
    partB_prediction = mo.ui.radio(...)  # own cell, no gate
    return (partB_prediction,)

@app.cell
def _(mo, partB_prediction):
    mo.stop(partB_prediction.value is None, mo.md("..."))
    # this cell is a pure gate; no widget definitions here

each widget lives in its own un-gated cell. gate cells are pure mo.stop() statements with no side-effect widget definitions.

scope of work

per the audit:

volume	labs affected	severity
vol1	17 labs (all except reference)	mostly minor (next-prediction leak); lab_01 was severe until #1339
vol2	15 labs	mix; lab_06-08 have severe multi-widget leaks

each lab needs gate cells split from widget-definition cells. this is mechanical per cell but the right structure per lab depends on UX (should users see all predictions at once, or unlock sequentially?).

acceptance

pytest labs/tests/test_static.py::TestMarimoDataflow passes with no xpass/xfail mismatch
remove the xfail mark in #1346's test_no_widget_defined_in_gated_cell
marimo check remains clean across all 33 labs

suggested approach

one PR per lab (or batched by volume) so reviews stay manageable
use vol2/lab_05_dist_train as the template
run the xfail-marked test per-lab to verify the fix before removing the xfail mark

this is a larger cleanup sprint, not urgent for today's issues. the check landed in #1346 will prevent new labs from introducing the pattern while this refactor proceeds.

Originally created by @profvjreddi on GitHub (Apr 16, 2026). Original GitHub issue: https://github.com/harvard-edge/cs249r_book/issues/1347 ## context an ast audit of all 33 labs (via the new `TestMarimoDataflow.test_no_widget_defined_in_gated_cell` check in #1346) finds that 32 of 33 labs have the widget-in-gated-cell anti-pattern. the one clean lab, `vol2/lab_05_dist_train.py`, is the canonical reference for the proper pattern. the pattern looks like this: ```python @app.cell def _(mo, partA_prediction): mo.stop(partA_prediction.value is None, mo.md("...")) partB_prediction = mo.ui.radio(...) # defined inside gated cell return (partB_prediction,) ``` when `partA_prediction` is unanswered, `mo.stop()` halts the cell. `partB_prediction` never gets defined. every cell that depends on it cascades into an undefined state. in lab_01's pre-fix state, this cascade was severe enough to block all instrument widgets; #1339 fixed lab_01 specifically. other labs tolerate the pattern only because users answer predictions in order. ## the proper pattern from `vol2/lab_05_dist_train.py`: ```python @app.cell def _(mo): partB_prediction = mo.ui.radio(...) # own cell, no gate return (partB_prediction,) @app.cell def _(mo, partB_prediction): mo.stop(partB_prediction.value is None, mo.md("...")) # this cell is a pure gate; no widget definitions here ``` each widget lives in its own un-gated cell. gate cells are pure `mo.stop()` statements with no side-effect widget definitions. ## scope of work per the audit: | volume | labs affected | severity | |---|---|---| | vol1 | 17 labs (all except reference) | mostly minor (next-prediction leak); lab_01 was severe until #1339 | | vol2 | 15 labs | mix; lab_06-08 have severe multi-widget leaks | each lab needs gate cells split from widget-definition cells. this is mechanical per cell but the right structure per lab depends on UX (should users see all predictions at once, or unlock sequentially?). ## acceptance - `pytest labs/tests/test_static.py::TestMarimoDataflow` passes with no xpass/xfail mismatch - remove the `xfail` mark in #1346's `test_no_widget_defined_in_gated_cell` - `marimo check` remains clean across all 33 labs ## suggested approach - one PR per lab (or batched by volume) so reviews stay manageable - use `vol2/lab_05_dist_train` as the template - run the xfail-marked test per-lab to verify the fix before removing the xfail mark this is a larger cleanup sprint, not urgent for today's issues. the check landed in #1346 will prevent new labs from introducing the pattern while this refactor proceeds.

GiteaMirror added the area: book type: bug type: improvement labels 2026-04-19 12:24:07 -05:00

GiteaMirror closed this issue

2026-04-19 12:24:09 -05:00

GiteaMirror commented

2026-04-19 12:24:10 -05:00

@profvjreddi commented on GitHub (Apr 16, 2026):

scope correction after more careful analysis.

the original framing treated any widget-in-gated-cell as a violation (32 of 33 labs). but the sequential-unlock idiom (one widget per gated cell) actually works correctly in practice: test_engine.py passes 70/70 against dev, and lab_02 through lab_16 use this pattern as-designed.

the REAL bug this tracks is gated cells that leak MULTIPLE widgets. when one such cell's gate fires, all its widgets go undefined and downstream cells cascade. that's what broke lab_01 pre-#1339.

per the refined test_no_multi_widget_leak_in_gated_cell check landing in #1350, these 14 labs are the actual debt:

vol1/lab_00_introduction.py
vol1/lab_01_ml_intro.py
vol2/lab_01_introduction.py
vol2/lab_06_fault_tolerance.py
vol2/lab_07_fleet_orch.py
vol2/lab_08_inference.py
vol2/lab_09_perf_engineering.py
vol2/lab_10_edge_intelligence.py
vol2/lab_11_ops_scale.py
vol2/lab_12_security_privacy.py
vol2/lab_13_robust_ai.py
vol2/lab_14_sustainable_ai.py
vol2/lab_15_responsible_ai.py
vol2/lab_16_fleet_synthesis.py

(vol1/lab_01 already fixed by #1339 but still flagged here because other cells still leak multiple widgets.)

the refactor template is #1339: split each gated cell so it defines at most ONE new widget (usually the next prediction). bundle all instruments into the final gated cell, gated on the last prediction, or move them entirely into the tabs cell.

the remaining 19 labs are already clean by this criterion. the grandfather set in the test will shrink as each of these 14 gets refactored; when empty, the bug class is closed and _KNOWN_MULTI_LEAK_LABS can be deleted.

re #1349: i closed that PR without merging. it was the wrong direction (removed gates entirely rather than splitting widgets per cell).

@profvjreddi commented on GitHub (Apr 16, 2026): scope correction after more careful analysis. the original framing treated any widget-in-gated-cell as a violation (32 of 33 labs). but the sequential-unlock idiom (one widget per gated cell) actually works correctly in practice: test_engine.py passes 70/70 against dev, and lab_02 through lab_16 use this pattern as-designed. the REAL bug this tracks is gated cells that leak MULTIPLE widgets. when one such cell's gate fires, all its widgets go undefined and downstream cells cascade. that's what broke lab_01 pre-#1339. per the refined `test_no_multi_widget_leak_in_gated_cell` check landing in #1350, these 14 labs are the actual debt: - vol1/lab_00_introduction.py - vol1/lab_01_ml_intro.py - vol2/lab_01_introduction.py - vol2/lab_06_fault_tolerance.py - vol2/lab_07_fleet_orch.py - vol2/lab_08_inference.py - vol2/lab_09_perf_engineering.py - vol2/lab_10_edge_intelligence.py - vol2/lab_11_ops_scale.py - vol2/lab_12_security_privacy.py - vol2/lab_13_robust_ai.py - vol2/lab_14_sustainable_ai.py - vol2/lab_15_responsible_ai.py - vol2/lab_16_fleet_synthesis.py (vol1/lab_01 already fixed by #1339 but still flagged here because other cells still leak multiple widgets.) the refactor template is #1339: split each gated cell so it defines at most ONE new widget (usually the next prediction). bundle all instruments into the final gated cell, gated on the last prediction, or move them entirely into the tabs cell. the remaining 19 labs are already clean by this criterion. the grandfather set in the test will shrink as each of these 14 gets refactored; when empty, the bug class is closed and `_KNOWN_MULTI_LEAK_LABS` can be deleted. re #1349: i closed that PR without merging. it was the wrong direction (removed gates entirely rather than splitting widgets per cell).

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/cs249r_book#4380