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

Track state variable and attribute usage #2429

Closed

Conversation

benedikt-bartscher
Copy link
Contributor

@benedikt-bartscher benedikt-bartscher commented Jan 21, 2024

Track if rx.Vars and their attributes are used (directly or via var operations) and only include used variables in the state dict. This should result in reduced initial state payloads and network traffic for var changes, which are not really needed in the frontend.

My plan is to write a second PR based on this one, which includes "used" properties (normal python properties with @property decorator) in the serialization. This allows users to use those properties in the frontend without always computing all properties for a given class. (related to my comment here)

Co-Author: @macmoritz

tests/states/upload.py Outdated Show resolved Hide resolved
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.

I think i fixed the integration test failures, pretty sure we can merge if CI is passing.

reflex/vars.py Outdated Show resolved Hide resolved
@masenf
Copy link
Collaborator

masenf commented Feb 20, 2024

Will have to continue holding off on this one for the time being, because it breaks with the reflex.dev website.

Flexdown currently redefines State classes multiple times per doc file, so they lose their _is_used markers.

@benedikt-bartscher benedikt-bartscher marked this pull request as draft March 11, 2024 10:57
@masenf masenf mentioned this pull request May 1, 2024
@benedikt-bartscher benedikt-bartscher changed the title track variable and attribute usage, only include used variables in state dict Track state variable and attribute usage May 2, 2024
@benedikt-bartscher
Copy link
Contributor Author

Needs #3214 for flexdown

@benedikt-bartscher
Copy link
Contributor Author

Will have to continue holding off on this one for the time being, because it breaks with the reflex.dev website.

Flexdown currently redefines State classes multiple times per doc file, so they lose their _is_used markers.

@masenf I just rebased onto main and tested this locally with flexdown branch benedikt-bartscher/per-block-module, and it seems to work fine. However, reflex-web ci is still failing

@benedikt-bartscher
Copy link
Contributor Author

@masenf did you release a new flexdown version yet?
Shall we stall this one until the new var system is merged? Did you have usage tracking in mind while designing it?

@picklelo
Copy link
Contributor

@benedikt-bartscher I just cut a new version of flexdown 0.1.5a10 off the latest main

@benedikt-bartscher
Copy link
Contributor Author

Thank you, @picklelo. I just raised reflex-dev/reflex-web#855

@benedikt-bartscher
Copy link
Contributor Author

Finally, reflex-web ci is passing, thanks again @picklelo and @masenf.
Now we just need to figure out how we handle this with the new var system in #3743 cc @adhami3310

@benedikt-bartscher benedikt-bartscher marked this pull request as ready for review August 20, 2024 10:24
@benedikt-bartscher benedikt-bartscher marked this pull request as draft August 26, 2024 20:42
@benedikt-bartscher
Copy link
Contributor Author

Converted to draft because #3743 was merged. This needs some love to work with the new Immutable Vars

@benedikt-bartscher
Copy link
Contributor Author

@masenf @adhami3310 do you have any hints/ideas on how to implement this for ImmutableVar's?

@adhami3310
Copy link
Member

ideas on how to implement this for ImmutableVar

Since immutable vars can't have such fields, it could be possible to have a state-level mapping between vars and if they are used or not, although it will be fairly tricky to implement.

I'm not sure however how this will be that strong of an optimization considering that in an ideal application (almost) all vars will be used somewhere in the code, rendering all of the operations we do to figure out if a variable is being used unnecessary. Another issue is that it would slow down compile times (even further). Is there unseen advantages I'm missing?

@benedikt-bartscher
Copy link
Contributor Author

benedikt-bartscher commented Aug 27, 2024

Thanks for your feedback. You are right, it might introduce some additional compile time. But I think it should be possible without too much overhead, and (at least for us) runtime performance is more important.

an ideal application (almost) all vars will be used somewhere in the code
While this should always be the goal, there are many cases where it is not true.

Some examples:

  • Reflex internal vars like router or dynamic router vars can not be disabled and may not be needed at all.
  • Libraries may share reusable State classes or Mixins, and the user might not want to use all vars.

Another advantage of usage tracking is automatic serialization of python/sqla/... properties if they are used in the frontend.

@benedikt-bartscher
Copy link
Contributor Author

benedikt-bartscher commented Aug 27, 2024

@adhami3310 I completely forgot about one of the biggest benefits. Sorry, it has been a long time since I started this PR. Reflex currently always serializes complete root-level vars. Meaning, if you have f.e. a rx.Base class with 10 fields, reflex always serializes all of them if you don't provide a custom serializer.

@benedikt-bartscher
Copy link
Contributor Author

Closing this, continue here: #3845

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.

7 participants