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

fix var dependency dicts #3842

Merged
merged 8 commits into from
Sep 9, 2024
Merged

Conversation

benedikt-bartscher
Copy link
Contributor

@benedikt-bartscher benedikt-bartscher commented Aug 27, 2024

Closes #3595

@benedikt-bartscher benedikt-bartscher marked this pull request as ready for review August 27, 2024 11:21
@benedikt-bartscher benedikt-bartscher marked this pull request as draft August 27, 2024 11:21
@benedikt-bartscher benedikt-bartscher marked this pull request as ready for review August 27, 2024 13:46
@benedikt-bartscher
Copy link
Contributor Author

I missed the failing redis integration tests, going back to draft

@benedikt-bartscher benedikt-bartscher marked this pull request as draft August 27, 2024 18:55
@benedikt-bartscher
Copy link
Contributor Author

benedikt-bartscher commented Aug 27, 2024

Tests are fixed ✔️
However, I am thinking about delaying the initialization of the var dependency dicts to compile, to ensure they are only executed once. Currently, all dependencies are recalculated if you add a dynamic var (State.add_var) or a page with a dynamic route. They are reloaded in integration tests as well (reload_state_module). Moving this to compile should simplify the code and make it faster. WDYT @picklelo @masenf ?

Edit: I just tested it, works fine. All integration tests pass. But we need to call _init_var_dependency_dicts in some (19) unit tests which are not compiling the app and test var deps.

diff --git a/reflex/app.py b/reflex/app.py
index 5be0ef04..6e9c044b 100644
--- a/reflex/app.py
+++ b/reflex/app.py
@@ -799,6 +799,13 @@ class App(MiddlewareMixin, LifespanMixin, Base):
         for render, kwargs in DECORATED_PAGES[get_config().app_name]:
             self.add_page(render, **kwargs)
 
+    def _init_var_dependency_dicts(self):
+        """Recursively initialize the var dependency dictionaries for all state subclasses of the app."""
+        if not self.state:
+            return
+
+        self.state._init_var_dependency_dicts()
+
     def _validate_var_dependencies(
         self, state: Optional[Type[BaseState]] = None
     ) -> None:
@@ -853,6 +860,7 @@ class App(MiddlewareMixin, LifespanMixin, Base):
         if not self._should_compile():
             return
 
+        self._init_var_dependency_dicts()
         self._validate_var_dependencies()
         self._setup_overlay_component()
         self._setup_error_boundary()
diff --git a/reflex/state.py b/reflex/state.py
index 8fcaa7e1..a98acc2c 100644
--- a/reflex/state.py
+++ b/reflex/state.py
@@ -592,7 +592,6 @@ class BaseState(Base, ABC, extra=pydantic.Extra.allow):
         # Initialize per-class var dependency tracking.
         cls._computed_var_dependencies = defaultdict(set)
         cls._substate_var_dependencies = defaultdict(set)
-        cls._init_var_dependency_dicts()
 
     @staticmethod
     def _copy_fn(fn: Callable) -> Callable:
@@ -653,7 +652,7 @@ class BaseState(Base, ABC, extra=pydantic.Extra.allow):
 
     @classmethod
     def _init_var_dependency_dicts(cls):
-        """Initialize the var dependency tracking dicts.
+        """Recursively initialize the var dependency tracking dicts for the state and all substates.
 
         Allows the state to know which vars each ComputedVar depends on and
         whether a ComputedVar depends on a var in its parent state.
@@ -701,6 +700,9 @@ class BaseState(Base, ABC, extra=pydantic.Extra.allow):
                     parent_state.get_parent_state(),
                 )
 
+        for substate in cls.get_substates():
+            substate._init_var_dependency_dicts()
+
     @classmethod
     def _check_overridden_methods(cls):
         """Check for shadow methods and raise error if any.
@@ -918,9 +920,6 @@ class BaseState(Base, ABC, extra=pydantic.Extra.allow):
         for substate_class in cls.class_subclasses:
             substate_class.vars.setdefault(name, var)
 
-        # Reinitialize dependency tracking dicts.
-        cls._init_var_dependency_dicts()
-
     @classmethod
     def _set_var(cls, prop: ImmutableVar):
         """Set the var as a class member.
@@ -1037,9 +1036,6 @@ class BaseState(Base, ABC, extra=pydantic.Extra.allow):
             cls.vars[param] = cls.computed_vars[param] = func._var_set_state(cls)  # type: ignore
             setattr(cls, param, func)
 
-        # Reinitialize dependency tracking dicts.
-        cls._init_var_dependency_dicts()
-
     def __getattribute__(self, name: str) -> Any:
         """Get the state var.
 
@@ -3365,5 +3361,4 @@ def reload_state_module(
             state._always_dirty_substates.discard(subclass.get_name())
     state._computed_var_dependencies = defaultdict(set)
     state._substate_var_dependencies = defaultdict(set)
-    state._init_var_dependency_dicts()
     state.get_class_substate.cache_clear()

As a bonus point, this will only calculate var dependencies for state subclasses of the app's main state, instead of all state classes which are imported.

@benedikt-bartscher
Copy link
Contributor Author

@masenf I just noticed you already tried something similar here: #2822

@benedikt-bartscher benedikt-bartscher marked this pull request as ready for review August 27, 2024 23:05
@benedikt-bartscher
Copy link
Contributor Author

However, I am thinking about delaying the initialization of the var dependency dicts to compile, to ensure they are only executed once.

Implemented in f3a98f3

@benedikt-bartscher
Copy link
Contributor Author

I think we are safe to adjust the cache to be True by default after merging this one

btw: failing example-counter tests seem not related to this PR

@benedikt-bartscher
Copy link
Contributor Author

Is there anything else I can do to get this fix in?

@masenf
Copy link
Collaborator

masenf commented Sep 5, 2024

@benedikt-bartscher reviewing today. i think we can bring it in, thanks for the nudge

reflex/app.py Outdated Show resolved Hide resolved
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.

nice catch - the example is working for me

@picklelo picklelo merged commit 99ffbc8 into reflex-dev:main Sep 9, 2024
46 checks passed
adhami3310 pushed a commit that referenced this pull request Sep 9, 2024
adhami3310 added a commit that referenced this pull request Sep 10, 2024
* use serializer system

* add checks for unsupported operands

* and and or are now supported

* format

* remove unnecessary call to JSON

* put base before rest

* fix failing testcase

* add hinting to get static analysis to complain

* damn

* big changes

* get typeguard from extensions

* please darglint

* dangit darglint

* remove one from vars

* add without data and use it in plotly

* DARGLINT

* change format for special props

* add pyi

* delete instances of Var.create

* modify client state to work

* fixed so much

* remove every Var.create

* delete all basevar stuff

* checkpoint

* fix pyi

* get older python to work

* dangit darglint

* add simple fix to last failing testcase

* remove var name unwrapped and put client state on immutable var

* fix older python

* fox event issues

* change forms pyi

* make test less strict

* use rx state directly

* add typeignore to page_id

* implement foreach

* delete .web states folder silly

* update reflex chakra

* fix issue when on mount or on unmount is not set

* nuke Var

* run pyi

* import immutablevar in critical location

* delete unwrap vars

* bring back array ref

* fix style props in app

* /health endpoint for K8 Liveness and Readiness probes (#3855)

* Added API Endpoint

* Added API Endpoint

* Added Unit Tests

* Added Unit Tests

* main

* Apply suggestions from Code Review

* Fix Ruff Formatting

* Update Socket Events

* Async Functions

* Update find_replace (#3886)

* [REF-3592]Promote `rx.progress` from radix themes (#3878)

* Promote `rx.progress` from radix themes

* fix pyi

* add warning when accessing `rx._x.progress`

* Use correct flexgen backend URL (#3891)

* Remove demo template (#3888)

* gitignore .web (#3885)

* update overflowY in AUTO_HEIGHT_JS from hidden to scroll (#3882)

* Retain mutability inside `async with self` block (#3884)

When emitting a state update, restore `_self_mutable` to the value it had
previously so that `yield` in the middle of `async with self` does not result
in an immutable StateProxy.

Fix #3869

* Include child imports in markdown component_map (#3883)

If a component in the markdown component_map contains children components, use
`_get_all_imports` to recursively enumerate them.

Fix #3880

* [REF-3570] Remove deprecated REDIS_URL syntax (#3892)

* mixin computed vars should only be applied to highest level state (#3833)

* improve state hierarchy validation, drop old testing special case (#3894)

* fix var dependency dicts (#3842)

* Adding array to array pluck operation. (#3868)

* fix initial state without cv fallback (#3670)

* add fragment to foreach (#3877)

* Update docker-example (#3324)

* /health endpoint for K8 Liveness and Readiness probes (#3855)

* Added API Endpoint

* Added API Endpoint

* Added Unit Tests

* Added Unit Tests

* main

* Apply suggestions from Code Review

* Fix Ruff Formatting

* Update Socket Events

* Async Functions

* /health endpoint for K8 Liveness and Readiness probes (#3855)

* Added API Endpoint

* Added API Endpoint

* Added Unit Tests

* Added Unit Tests

* main

* Apply suggestions from Code Review

* Fix Ruff Formatting

* Update Socket Events

* Async Functions

* Merge branch 'main' into use-old-serializer-in-literalvar

* [REF-3570] Remove deprecated REDIS_URL syntax (#3892)

* /health endpoint for K8 Liveness and Readiness probes (#3855)

* Added API Endpoint

* Added API Endpoint

* Added Unit Tests

* Added Unit Tests

* main

* Apply suggestions from Code Review

* Fix Ruff Formatting

* Update Socket Events

* Async Functions

* [REF-3570] Remove deprecated REDIS_URL syntax (#3892)

* remove extra var

Co-authored-by: Masen Furer <m_github@0x26.net>

* resolve typo

* write better doc for var.create

* return var value when we know it's literal var

* fix unit test

* less bloat for ToOperations

* simplify ImmutableComputedVar.__get__ (#3902)

* simplify ImmutableComputedVar.__get__

* ruff it

---------

Co-authored-by: Samarth Bhadane <samarthbhadane119@gmail.com>
Co-authored-by: Elijah Ahianyo <elijahahianyo@gmail.com>
Co-authored-by: Masen Furer <m_github@0x26.net>
Co-authored-by: benedikt-bartscher <31854409+benedikt-bartscher@users.noreply.github.com>
Co-authored-by: Vishnu Deva <vishnu.deva12@gmail.com>
Co-authored-by: abulvenz <a.eismann@senbax.de>
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.

[REF-3239] Var dependency bug
3 participants