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

Independent States #3206

Conversation

benedikt-bartscher
Copy link
Contributor

Closes #3204

@masenf
Copy link
Collaborator

masenf commented May 1, 2024

i think we need #2429 to solve this properly. the _was_touched marker is reset for every event (in redis), just because a particular substate wasn't touched, doesn't necessarily mean that it doesn't have computed vars that depend on vars in the parent states that may have changed.

@benedikt-bartscher
Copy link
Contributor Author

@masenf thanks for looking into this. While my issue description with the unused page looks like it is related to #2429 it really isn't. This is a completely different issue. Maybe my last commit makes things more clear?

@benedikt-bartscher
Copy link
Contributor Author

If my assumptions are correct, this should improve event handling performance for reflex-web quite decent. There are over 20 ComputedVars in reflex-web, even one which establishes a database connection. With reflex's current behavior, this should cause every event to recompute all those ComputedVars.

@benedikt-bartscher benedikt-bartscher marked this pull request as ready for review May 1, 2024 19:57
@masenf
Copy link
Collaborator

masenf commented May 1, 2024

import reflex as rx


class StateA(rx.State):
    a: str = ""

    async def handle_click(self):
        self.a = "foo"
        state_b = await self.get_state(StateB)
        state_b.b = "bar"


class StateB(rx.State):
    b: str = ""


def index() -> rx.Component:
    return rx.vstack(
        rx.hstack(
            StateA.a,
            StateB.b
        ),
        rx.button("Click me", on_click=StateA.handle_click)
    )


app = rx.App()
app.add_page(index)

In reflex main branch, the page displays "foobar" after clicking the button.

With this proposed patch, the page only displays "foo".

This is primarily why the delta is calculated from the root state, so that any states which are patched into the tree during processing of an event have their deltas also calculated.

I definitely see what you're trying to do here; ComputedVar is re-calculated in a somewhat overzealous manner, but the reason for that is to retain the semantics of ensuring they always have the latest value after every event. My main motivation for introducing the @rx.cached_var decorator last year, was to avoid the cost of recomputing these vars when the vars they depend on have not been changed.

If I just have an @rx.var decorated function, reflex has no way of knowing what it depends on. Since it's in a state class that is ultimately a descendent of rx.State, it might depend on router vars or something else that could change when an event is processed. I don't see a way around recomputing it without opting in to @rx.cached_var semantics... so maybe it's time to make @rx.var be a cached var by default?

@benedikt-bartscher benedikt-bartscher marked this pull request as draft May 1, 2024 20:21
@benedikt-bartscher
Copy link
Contributor Author

Thanks, I completely missed get_state. I already thought about the changing router vars...
Shouldn't it be possible to extend the computed var dependency tracking to respect get_state?

@masenf
Copy link
Collaborator

masenf commented May 1, 2024

Shouldn't it be possible to extend the computed var dependency tracking to respect get_state?

Thankfully because computed vars use property semantics, they can only access vars in their immediate ancestors (without using hacks that we do not support). So the dependency tracking is already working i think. The problem is that for regular @rx.var we don't do any dependency tracking; an @rx.var is always considered dirty by definition.

@benedikt-bartscher
Copy link
Contributor Author

Thanks a lot, I should have looked into the dependency tracking implementation earlier...
If the tracking works fine, is there any reason not promoting it to the default?

@masenf
Copy link
Collaborator

masenf commented May 1, 2024

If the tracking works fine, is there any reason not promoting it to the default?

There have been several bugs initially in the dependency tracking that have since been fixed. It's possible that other bugs could be lurking, but i have not uncovered them yet and use @rx.cached_var in all of my projects. Initially the intent was to have cached var be the default behavior, but we were hesitant to bring it in at first without some more bake time.

I think it's ready to become the default, but that means we're relying on users to report bugs that might pop up related to computed vars in the next release.

Additionally, it notably cannot track vars outside of self, so if someone is using a computed var to access/render data that originates from outside the state, a cached_var will essentially never update in that case.

@benedikt-bartscher
Copy link
Contributor Author

I see, makes sense. Considering the possible performance impacts of @rx.var I would support changing the default to _cache = True. I somehow was convinced that @rx.vars only should get updated if the associated state is dirty. But as you pointed out nicely, it's always considered dirty if it has an @rx.var defined.

I just migrated some codebases to cached var. If any problems arise, I will report them as usual.

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

Successfully merging this pull request may close these issues.

Independent States
2 participants