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

Execute dependent cells in notebook order #3182

Open
gabrielgrant opened this issue Dec 16, 2024 · 5 comments
Open

Execute dependent cells in notebook order #3182

gabrielgrant opened this issue Dec 16, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@gabrielgrant
Copy link
Contributor

Description

When changes in one cell trigger recomputation in multiple downstream cells with no inter-dependencies, my impression is that right now the recomputation computation of those downstream cells isn't well defined, but seems to happen in the order in which they were first created?

Not sure about that, but it does seem that they don't execute in the same order as they appear in the notebook. It would be nice if they did

Suggested solution

Have cells that could theoretically be executed concurrently execute in the order in which they appear in the notebook

Alternative

In an ideal world these might actually be executed concurrently. But that seems likely to be a bigger lift (and likely to have lots of edge cases around side-effects)

Additional context

No response

@gabrielgrant gabrielgrant added the enhancement New feature or request label Dec 16, 2024
@mscolnick
Copy link
Contributor

mscolnick commented Dec 16, 2024

Are you able to repro this consistently? I did update our logic to do exactly this (#3060) (although it shouldn't be something to depend on, because, as you said, we may move to run cells concurrently. that will also come with their edge-cases, but it's on our roadmap to explore)

@gabrielgrant
Copy link
Contributor Author

Ah, great minds lol

That's interest, tho. Didn't really try to build repro test case for it, just (thought I?) noticed it a couple times, and thought it was worth marking down as a (fairly minor) quality-of-life improvement. This quick test seems to behave as expected tho:

Image

(added bottom sleep() cell first, then the one above. re-running the import cell causes them to run in top-to-bottom order)

May be that I loaded one cell from disk and added the other after it was already running? Unfortunately have now closed that notebook, and it would be dependent on internal kernel state, so just re-opening it isn't going to work to track down the issue.

Given how minor an issue this is, and that you already seem to have mostly fixed it, it hardly seems worth wasting time tracking down a (possible) tricky-to-repro corner-case, so gonna just close.

Thanks for the quick response!

@mscolnick
Copy link
Contributor

Sounds good. It could be more complex DAGs that the tie-breaker is quite as expected. We can revisit if we get a good repro for that.

@gabrielgrant
Copy link
Contributor Author

gabrielgrant commented Dec 18, 2024

Just had this happen again. The particular case seems to be:

A
|__B
|  |__D
|
|__C

(plz excuse the medicore ascii art)

Letters represent cells in notebook order top to bottom, with lines indicating deps (B & C depend on A; D depends on B). When A is changed, cell recalculation order seems to be B, C, D rather than B, D, C

Image

Full repro notebook source
import marimo

__generated_with = "0.10.5"
app = marimo.App(width="full")


@app.cell
def _():
    import importlib
    import random
    return importlib, random


@app.cell
def _(importlib):
    import time
    importlib.reload(time)
    return (time,)


@app.cell
def _(random, time):
    time.sleep(10)
    a = random.random()
    return (a,)


@app.cell
def _(a, random, time):
    time.sleep(10)
    b = a + random.random()
    return (b,)


@app.cell
def _(b, random, time):
    time.sleep(10)
    d = b + random.random()
    return (d,)


@app.cell
def _(a, random, time):
    time.sleep(10)
    c = a + random.random()
    return (c,)


if __name__ == "__main__":
    app.run()

Gonna reopen since this at least seems tractable -- AFAICT this doesn't actually seem to depend on internal kernel state.

Not sure the intended behavior, but seems that the execution-in-notebook-order ordering is maybe just for direct siblings, rather than descendants?

@gabrielgrant gabrielgrant reopened this Dec 18, 2024
@mscolnick
Copy link
Contributor

Thanks for the repro - it should be easy to work with this setup to test/fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants