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

Explicit deps and interval for computed vars #3231

Merged

Conversation

benedikt-bartscher
Copy link
Contributor

@benedikt-bartscher benedikt-bartscher commented May 4, 2024

from datetime import datetime, timedelta

import reflex as rx


class OtherState(rx.State):
    def do_nothing(self) -> None:
        print("Doing nothing")


class State(rx.State):
    @rx.cached_var(interval=5)
    def time1(self) -> str:
        return datetime.now().strftime("%H:%M:%S")

    # same as above, different notation
    @rx.var(interval=5, cache=True)
    def time2(self) -> str:
        return datetime.now().strftime("%H:%M:%S")

    # explicit disabled auto_deps and timedelta interval
    @rx.var(interval=timedelta(minutes=5), cache=True, auto_deps=False)
    def time3(self) -> str:
        # this will not add deps, because auto_deps is False
        print(self.time1)
        print(self.time2)

        return datetime.now().strftime("%H:%M:%S")

    # explicit dependency on time var
    @rx.cached_var(deps=[time1])
    def depends_on_time1(self) -> str:
        return datetime.now().strftime("%H:%M:%S")

    # same as above
    @rx.cached_var(deps=["time1"])
    def depends_on_time1_alt(self) -> str:
        return datetime.now().strftime("%H:%M:%S")

    def do_nothing(self) -> None:
        print("Doing nothing")


def index() -> rx.Component:
    return rx.center(
        rx.vstack(
            rx.button("Do nothing", on_click=State.do_nothing),
            rx.button("Do nothing (OtherState)", on_click=OtherState.do_nothing),
            rx.text(f"Time1: {State.time1}"),
            rx.text(f"Time2: {State.time2}"),
            rx.text(f"Time3: {State.time3}"),
            rx.text(f"Depends on time1: {State.depends_on_time1}"),
        ),
    )


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

@benedikt-bartscher benedikt-bartscher changed the title add static deps for computed vars Explicit deps and interval for computed vars May 5, 2024
@benedikt-bartscher
Copy link
Contributor Author

Ready for initial review. Will add more tests if you like this idea :)

@benedikt-bartscher benedikt-bartscher marked this pull request as ready for review May 6, 2024 08:37
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

This is cool. I like the new features and flexibility.

@benedikt-bartscher benedikt-bartscher force-pushed the computed-var-improvements branch from 308b595 to bb7fc06 Compare May 6, 2024 20:24
@@ -1685,6 +1697,7 @@ def get_delta(self) -> Delta:
# and always dirty computed vars (cache=False)
delta_vars = (
self.dirty_vars.intersection(self.base_vars)
.union(self.dirty_vars.intersection(self.computed_vars))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@masenf did I find a bug here? You mentioned that one could define a computed var without dependencies and manually mark it as dirty. If I am not mistaken, this line is needed for this to work.

@benedikt-bartscher
Copy link
Contributor Author

Needs rebase after #3254 is merged

@benedikt-bartscher benedikt-bartscher marked this pull request as draft May 8, 2024 21:02
@benedikt-bartscher benedikt-bartscher force-pushed the computed-var-improvements branch from 4e31200 to 70bddda Compare May 21, 2024 20:21
@benedikt-bartscher benedikt-bartscher marked this pull request as ready for review May 21, 2024 20:28
Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Awesome - I like having cache as an argument rather than a separate rx.cached_var, perhaps we can move more to that in the future.

@picklelo picklelo merged commit 93de407 into reflex-dev:main May 28, 2024
47 checks passed
@benedikt-bartscher
Copy link
Contributor Author

Awesome - I like having cache as an argument rather than a separate rx.cached_var, perhaps we can move more to that in the future.

Thanks for your feedback and the merge. Maybe we should deprecate rx.cached_var in favor of the cache argument.

benedikt-bartscher added a commit to benedikt-bartscher/reflex that referenced this pull request Jun 3, 2024
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.

4 participants