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

[0.6.0 blocker] state: update inherited_vars and tracking dicts when adding vars #2822

Merged
merged 5 commits into from
Sep 17, 2024

Conversation

masenf
Copy link
Collaborator

@masenf masenf commented Mar 9, 2024

Cached vars were not updated when depending on dynamically added vars, because the dependency tracking dicts update substate_var_dependencies based on whether the var is inherited and inherited_vars was not being updated (and not being updated recursively) when vars were added to the state.

Now when a var is added, push the var down into all substates as an inherited var, and recursively (depth-first) update all substate _computed_var_dependencies and _substate_var_dependencies in case any of them were depending on a var that gets added later in the initialization process.

I don't think dynamically added vars get used that much outside of dynamic route args, so this probably doesn't come up in the wild that often.

Repro code

import reflex as rx

class State(rx.State):
    """The app state."""
    @rx.var
    def arg(self) -> int:
        return int(self.arg_str or 0)


class SubState(State):
    @rx.var(cache=True)
    def cached_arg(self) -> str:
        return self.arg

    @rx.var(cache=True)
    def cached_arg_str(self) -> str:
        return self.arg_str

    def on_load(self):
        print(f"SubState loaded: SubState.arg_str={self.arg_str}")
        print(f"State.arg_str={self.parent_state.arg_str}")
        print(f"rx.State.arg_str={self.parent_state.parent_state.arg_str}")


def index() -> rx.Component:
    return rx.vstack(
        rx.data_list.root(
            rx.data_list.item(
                rx.data_list.label("rx.State.arg_str (dynamic)"),
                rx.data_list.value(rx.State.arg_str),
            ),
            rx.data_list.item(
                rx.data_list.label("State.arg"),
                rx.data_list.value(State.arg),
            ),
            rx.data_list.item(
                rx.data_list.label("State.arg_str (dynamic) (inherited)"),
                rx.data_list.value(State.arg_str),
            ),
            rx.data_list.item(
                rx.data_list.label("SubState.arg_str (dynamic) (inherited)"),
                rx.data_list.value(SubState.arg_str),
            ),
            rx.data_list.item(
                rx.data_list.label("SubState.arg (inherited)"),
                rx.data_list.value(SubState.arg),
            ),
            rx.data_list.item(
                rx.data_list.label("SubState.cached_arg"),
                rx.data_list.value(SubState.cached_arg),
            ),
            rx.data_list.item(
                rx.data_list.label("SubState.cached_arg_str"),
                rx.data_list.value(SubState.cached_arg_str),
            ),
        ),
        rx.link("+", href=f"/{State.arg + 1}"),
        align="center",
        height="100vh",
    )


app = rx.App()
app.add_page(index, route="/[arg_str]", on_load=SubState.on_load)
app.add_page(index, on_load=SubState.on_load)

Navigate to http://localhost:3000/0.

rx.State.arg_str (dynamic)    0
State.arg    0
State.arg_str (dynamic) (inherited)
SubState.arg_str (dynamic) (inherited)
SubState.arg (inherited)    0
SubState.cached_arg    0
SubState.cached_arg_str

and in the terminal (with some debug prints added by me)

Getting value arg_str for reflex___state____state.repro_2822___repro_2822____state.repro_2822___repro_2822____sub_state
SubState loaded: SubState.arg_str=
Getting value arg_str for reflex___state____state
State.arg_str=0
Getting value arg_str for reflex___state____state
rx.State.arg_str=0

with some additional prints added to ImmutableComputedVar.__get__, we can see that the problem is unable to determine where the dynamically added arg_str var is defined

Getting Var arg_str for reflex___state____state
Getting Var arg for reflex___state____state.repro_2822___repro_2822____state
Getting Var arg_str for reflex___state____state.repro_2822___repro_2822____state
Getting Var arg_str for reflex___state____state.repro_2822___repro_2822____state.repro_2822___repro_2822____sub_state

The code is assuming the state_where_defined is the state where accessed because of missing inherited_vars tracking.

Clicking the plus should increment all numbers, but without this fix, the arg_str doesn't get rendered correctly when accessed through substates.

Test case and updated implementation forthcoming.

@benedikt-bartscher
Copy link
Contributor

I think we can close this, since #3842 is merged

@picklelo picklelo closed this Sep 10, 2024
@masenf masenf reopened this Sep 11, 2024
@masenf
Copy link
Collaborator Author

masenf commented Sep 11, 2024

i think we still need to update the inherited_vars properly.

updated repro code for 0.6.0

notice how the inherited dynamic route arg doesn't render properly when accessed through a substate. i think this has to do with how the __get__ method of ImmutableComputedVar relies on checking the inherited_vars to find the class where the var is initially defined.

because the dynamically added vars are not added to inherited_vars, the __get__ method always stops at the outer substate.

Ensure that dynamically added vars are accounted for in dependency and
inheritence tree to avoid unrenderable or stale data.
@masenf masenf force-pushed the masenf/track-computed-vars-retrack branch from 381b1ff to 2cd7cbd Compare September 11, 2024 22:05
Copy link
Contributor

@benedikt-bartscher benedikt-bartscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@masenf thanks for the followup
Your fix looks good to me

@masenf masenf marked this pull request as ready for review September 11, 2024 22:42
@masenf masenf changed the title state: update inherited_vars and tracking dicts when adding vars [0.6.0 blocker] state: update inherited_vars and tracking dicts when adding vars Sep 11, 2024
Use the new /api/gen/{hash}/refactored endpoint to get refactored reflex code.
@benedikt-bartscher
Copy link
Contributor

this needs a rebase for the new var system (Var an js_expr)

@masenf masenf merged commit 16d3962 into main Sep 17, 2024
46 checks passed
@masenf masenf deleted the masenf/track-computed-vars-retrack branch September 17, 2024 18:36
masenf added a commit that referenced this pull request Sep 17, 2024
…adding vars (#2822)

* state: update inherited_vars and tracking dicts when adding vars

Ensure that dynamically added vars are accounted for in dependency and
inheritence tree to avoid unrenderable or stale data.

* Regression test for dynamic route args and inherited_vars

* [flexgen] Initialize app from refactored code

Use the new /api/gen/{hash}/refactored endpoint to get refactored reflex code.

* Use _js_expr instead of _var_name
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.

3 participants