Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mo.stop propagation breaks #1509

Closed
dmadisetti opened this issue May 28, 2024 · 8 comments
Closed

mo.stop propagation breaks #1509

dmadisetti opened this issue May 28, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@dmadisetti
Copy link
Collaborator

Describe the bug

I'm not able to replicate this without sharing my notebook.

It's a pretty large notebook. I have a stop that should effectively stop the whole notebook, but I start getting name errors at some point since the stop doesn't propagate as far as it should.

image
Name error for projections

image
The cell that defines projections

Rerunning the stop cell then works! It feels like maybe a race condition somewhere

Environment

0.6.11

Code to reproduce

Large notebook with many parts, since stop condition that should halt everything. Restarting my notebook kernel reliably triggers the incorrect name errors

@dmadisetti dmadisetti added the bug Something isn't working label May 28, 2024
@dmadisetti
Copy link
Collaborator Author

OK- I think it's because I reference UI elements in the "stopped" code, which triggers a rerun on load.
Found 2 bugs:

  1. Stop doesn't stop
  2. Stop doesn't resolve deps when UI element is present.

Replication with much smaller notebook:

https://marimo.app/l/6joyrm

I'm not using refresh, but I do have some interdependent UI elements that I think end up triggering change

@akshayka
Copy link
Contributor

I think a fix could be to not run a cell if any of its ancestors are stopped.

We do this with disabled cells, but not for stopped cells.

@dmadisetti
Copy link
Collaborator Author

dmadisetti commented May 29, 2024

I think "stop" itself is fine, but something is up with resolving the dependency graph. In the example I gave, marimo should have realized that a is not accessible and not tried to run the cells. Also, b is not accessible because a is not accessible.

I think more aggressive checking of refs could solve this. Here's a silly example that shows how the dep, ref checking is a little broken (beyond stop):

https://marimo.app/l/tq20qa


edit: Yes, this is a special weird case, but I think it shows lack of guards that would catch the stop case as well

@akshayka
Copy link
Contributor

I think more aggressive checking of refs could solve this. Here's a silly example that shows how the dep, ref checking is a little broken (beyond stop):

What would you expect refs and defs to be in your example?

@dmadisetti
Copy link
Collaborator Author

dmadisetti commented May 29, 2024

I would expect it not to run. Maybe throw an "Unsatisfable context" or even "This cell is in a cycle: cell-0
-> x -> cell-0".

because even in this case (similar to the original example): https://marimo.app/l/pdym22

marimo doesn't check to see if x exists, just that the cell that declares the def has run. It should check:

  • the variable exists
  • the cell has run
  • the cell is not "stopped."

Right? I don't know—these are weird edge cases, but I think they might surface in cases like we're seeing with stop. A sloppy "except all" could also make this pop up.

Or maybe this is getting unrelated to the stop issue (I can open another issue)


edit, simpler case

Even without exceptions, you can do something like:

if False:
    b = 12
mo.defs()

Which is fine by itself (I think), but trying to use b in another cell should raise an error (beyond the NameError)

This was referenced Jun 6, 2024
@mscolnick
Copy link
Contributor

@dmadisetti is this good to close now with this #1580 (again, awesome work!)

@akshayka
Copy link
Contributor

I think this can be solved for the "relaxed" runtime too, without the machinery of strict.

@dmadisetti
Copy link
Collaborator Author

Closing out, because I had a notebook that seems to pretty reliable trip this, but doesn't anymore 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants