Spurious cycle detection when classes are only used in annotations

[mwchase]

Describe the bug

I was defining some classes that have methods that depend on each other, in different cells, and marimo claimed that there were cyclic dependencies between cells. This makes it somewhat less convenient than I'd like to use annotations.

It appears that the code is parsed with annotations as strings, so any value that's only used in annotations shouldn't factor into dependency ordering.

Environment

{
  "marimo": "0.1.58",
  "OS": "Linux",
  "OS Version": "5.15.0-89-generic",
  "Python Version": "3.12.0",
  "Binaries": {
    "Chrome": "--",
    "Node": "v19.8.1"
  },
  "Requirements": {
    "black": "23.11.0",
    "click": "8.1.7",
    "jedi": "0.19.1",
    "pymdown-extensions": "10.4",
    "tomlkit": "0.12.2",
    "tornado": "6.3.3",
    "typing_extensions": "4.8.0"
  }
}

Code to reproduce

import marimo

__generated_with = "0.1.58"
app = marimo.App()


@app.cell
def __(B):
    class A:
        def use_b(self, b: B) -> None:
            ...
    return A,


@app.cell
def __(A):
    class B:
        def use_a(self, a: A) -> None:
            ...
    return B,


@app.cell
def __():
    return


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

[akshayka]

Thanks for the bug report! We should be able to fix this. Will write back with updates.

[akshayka]

@mwchase, I have a fix, but I just want to make sure this is a bug, instead of intended behavior. Can you help me think through it?

It's possible to use type annotations at runtime. Popular libraries such as Pydantic do this. In such cases, could it be that marimo should indeed treat type annotations as names, not strings? In your specific case, you could replace the type annotations with actual strings, in which case the Python AST will contain Constant nodes for "A" and "B", not Name nodes.

For example, consider this toy notebook. In this case it could be argued that the cell defining B has a legitimate dependency on the cell defining A.

import marimo

__generated_with = "0.1.56"
app = marimo.App()


@app.cell
def __():
    class A:
        """hello world"""
    return A,


@app.cell
def __(A, typing):
    class B:
        def use_type_annotations(self, a: A) -> None:
            print(typing.get_type_hints(B.use_type_annotations)["a"].__doc__)
    return B,


@app.cell
def __(B):
    b = B()
    return b,


@app.cell
def __(A, b):
    b.use_type_annotations(A())
    return


@app.cell
def __():
    import typing
    return typing,


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

Given this complication I am leaning toward "intended behavior", but I am not certain. Let me know what you think?

[mwchase]

Huh. I took that code, started messing with it, and got very confused. Taking a closer look, here's what happens if I change the example you gave to use "A" in the annotation:

Cell 1 no longer depends on Cell 0, but Cell 3 still depends on Cell 0, so the code in Cell 1 can properly resolve the types, when Cell 3 calls it.

If I break the dependency from Cell 3 to Cell 0, I get undesirable behavior in the form of changes not propagating through the dependency graph.

From a quick glance at Pydantic, I can see that it's not unreasonable to end up in a scenario analogous to "there is no dependency between Cell 3 and Cell 0".

Unless someone comes up with a really compelling reason, probably the behavior should stay as-is (for maximum expressiveness within Python), but this probably merits some documentation.
In terms of, "on the one hand, using strings can break cycles, but on the other hand, using strings can break dependencies".

[akshayka]

@mwchase , thanks for taking a look and for the thoughtful experiment.

Unless someone comes up with a really compelling reason, probably the behavior should stay as-is (for maximum expressiveness within Python), but this probably merits some documentation.

Great, we're on the same page.

In terms of, "on the one hand, using strings can break cycles, but on the other hand, using strings can break dependencies".

I like this suggestion — it's a good way to think about it. If dependencies are really needed, then best to not use strings, and declare the two dependent types/classes/functions in the same cell when possible.

I will add documentation to this effect somewhere on docs.marimo.io. Perhaps in the FAQ, and/or the reactivity guide. Once that's done I'll close this issue.

Thanks again for the thoughtful and fun discussion!