Improvement: annotate cycles with variable names #1304
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@fuenfundachtzig, thank you for doing this; definitely an improvement. I'll plan to review once it's moved out of draft, but let me know if you want eyes earlier. |
Thanks a lot :) There is a failing test, let me look into that first (might have time over the weekend). |
|
The test fails because I am using a set in marimo/marimo/_runtime/dataflow.py Line 22 in 976b324 which becomes part of the cell output if there is a marimo/marimo/_messaging/errors.py Line 13 in 976b324 making the marimo/marimo/_server/templates/templates.py Line 106 in 5886681 I could either
Any preferences? |
|
I'd just use a list instead of a set for simplicity. |
| <li className={liStyle} key={`${edge[0]}-${edge[1]}`}> | ||
| <CellLinkError cellId={edge[0]} /> | ||
| {" -> "} {edge[1].length == 1 ? edge[1] : edge[1].join(", ")}{" "} |
There was a problem hiding this comment.
| {" -> "} {edge[1].length == 1 ? edge[1] : edge[1].join(", ")}{" "} | |
| <span className="text-muted-foreground"> | |
| {" - "} {edge[1].length == 1 ? edge[1] : edge[1].join(", ")} | |
| {" -> "} | |
| </span> |
marimo/_runtime/dataflow.py
Outdated
| @@ -19,6 +19,9 @@ | |||
| from collections.abc import Collection | |||
|
|
|||
| Edge = Tuple[CellId_t, CellId_t] | |||
| # EdgeWithVar uses a list rather than a set for the variables linking the cells | |||
| # as sets are not JSON-serializable (required by static_notebook_template()) | |||
There was a problem hiding this comment.
| # as sets are not JSON-serializable (required by static_notebook_template()) | |
| # as sets are not JSON-serializable (required by static_notebook_template()). | |
| # The first entry is the source node; the second entry is a list of defs from | |
| # the source read by the destination; and the third entry is the destination | |
| # node. |
marimo/_runtime/validate_graph.py
Outdated
| # before reporting the cells in the cycle to the user, | ||
| # we first annotate the cycle with the variable names | ||
| # that link its cells |
There was a problem hiding this comment.
Super small nit on comment formatting.
| # before reporting the cells in the cycle to the user, | |
| # we first annotate the cycle with the variable names | |
| # that link its cells | |
| # before reporting the cells in the cycle to the user, | |
| # we first annotate the cycle with the variable names | |
| # that link its cells |
tests/_runtime/test_runtime.py
Outdated
| for edge in expected_edges: | ||
| assert edge in error.edges or (edge[1], edge[0]) in error.edges | ||
| assert edge in error.edges_with_vars or reversed(edge) in error.edges_with_vars |
There was a problem hiding this comment.
I believe the reversed logic wasn't needed originally; removed it and these tests
still pass on my machine. Checking for the presence of a reversed edge is no longer
correct, since the variables that flow on the edges change when you flip the direction.
| assert edge in error.edges_with_vars or reversed(edge) in error.edges_with_vars | |
| assert edge in error.edges_with_vars |
|
Thanks for the suggestions! I have implemented all of them. You're right about the reversed... I had wondered about that but didn't want to break anything I might be overlooking. I have also decided to sort the variables when printing them. |
sorting variable names in output
fuenfundachtzig
force-pushed
the
feature/annotate-cycles-with-vars
branch
from
10c1f56
to
89cc570
Compare
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.4.11-dev10 |
Fixes #847