-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[REF-2821]Improve Dynamic Imports #3345
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this: https://pypi.org/project/lazy-loader/ it looks like lazy_loader is only recommended for Python 3.11+ - is it okay to use it for our case?
I think for our case it is, lazy_supports python >= 3.8, however they added the recommendation in their latest release due to a bug in python which affects the use of |
c0de2be
to
563e641
Compare
Integration tests need reflex-dev/reflex-web#722 to pass for reflex-web |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Changes look good to me, I see a big performance improvement locally when testing.
unordered_list = list_ns.unordered | ||
|
||
|
||
def __getattr__(name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that special case still needed despite using the tuple in __init__.py
to set the alias of list_ns
to list
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of having of having aliases in tuples came about primarily as a result of the list component which shadows python’s list object when explicitly defined. This messed up the pyi generator as well. This was a fix to generate pyi files properly for cases similar to that of rx.list
. However, list
still needs to be resolved at runtime (reflex.components.radix.themes.layout.list.list
) and in reflex.utils.lazy_loader.attach
, we only pick the alias than the actual ref
To explain this further:
The mapping in relex.__init__.py
contains:
"components.radix.themes.layouts.list": [("list_ns", "list")]
}
In list.py
, assuming list_ns
is defined and list
is not, the submod_attrs arg of lazy_loader.attach
does not accept tuples as list values (only accepts dict[str, list[str]]).
In reflex.utils.lazy_loader
we flatten the submod_list
as a result keeping the alias(list
) leaving out the actual ref (list_ns
) as a result, this is also because in lazy_loader.attach
implementation, they only dynamically import the module when it(list
) exists in the submod_attrs keys(which is recomputed). The issue here is when rx.list
is being resolved, the name list
instead of list_ns
will be passed as the __getattrs__
name argument which will not be found if we added list_ns
instead of list
in submod_attrs
Which means list
still needed to be defined to be accessible(https://github.com/scientific-python/lazy_loader/blob/2031223913c7e592c1c59aec72f296e9d08171fa/lazy_loader/__init__.py#L78).
It may look a bit clunky and confusing(which maybe needs a bit more documentation in the docstrings) considering the “alias” is used primarily for pyi generation but still requires that the attribute be defined as well, but I left it this way since it’s a special case for the list
component and didn’t want to over-engineer the approach just yet.
I think another approach will be to have the lazy_loader.attach
function implementation defined in reflex.utils.lazy_loader, and then modifying it to work with tuples for __getattr__
and __all__
.
83f7094
to
5fcccda
Compare
* Improve import times * add lazy loading to rx.el * add lazy loading to reflex core components * minor refactor * Get imports working with reflex web * get imports to work with all reflex examples * refactor to define imports only in the root. * lint * deadcode remove * update poetry deps * unit tests fix * app_harness fix * app_harness fix * pyi file generate * pyi file generate * sort pyi order * fix pyi * fix docker ci * rework pyi-generator * generate pyi for __init__ files * test pyright * test pyright ci * partial pyright fix * more pyright fix * pyright fix * fix pyi_generator * add rx.serializer and others * add future annotation import which fixes container CI, then also load recharts lazily * add new pyi files * pyright fix * minor fixes for reflex-web and flexdown * forward references for py38 * ruff fix * pyi fix * unit tests fix * reduce coverage to 68% * reduce coverage to 67% * reduce coverage to 66%as a workaround to coverage's rounding issue * reduce coverage to 66%as a workaround to coverage's rounding issue * exclude lazy_loader dependency review checks. * its lazy-loader * Add docstrings and regenerate pyi files * add link * address Pr comments * CI fix * partially address PR comments. * edit docstrings and fix integration tests * fix typo in docstring * pyi fix
@ElijahAhianyo Thanks for the huge performance improvement! Is this wanted? Edit: Another finding: |
@benedikt-bartscher good catch on Pycharm seem to handle this well |
This Pr is a big refactor of reflex imports. Much time is spent making imports when components are accessed in reflex. The motivation is to load imports lazily or dynamically only when needed and do this once.
Dynamic Imports
Reflex utilizes dynamic imports, or lazy loading, to reduce startup/import times.
With this approach, imports are delayed until they are needed. We use
the
lazy_loader
library to achieve this.How it works
lazy_loader.attach
takes two optional arguments:submodules
andsubmod_attrs
.submodules
typically points to directories or files to be accessed.submod_attrs
defines a mapping of directory or file names as keys with a listof attributes or modules to access.
Example directory structure:
To add
box
under therx
namespace (rx.box
), add the relative path tosubmod_attrs
inreflex/__init__.py
(this file):This implies that
box
will be imported fromreflex/components/radix/themes/components/box.py
.To add box under the
rx.radix
namespace (rx.radix.box
), add the relative path to thesubmod_attrs argument in
reflex/components/radix/__init__.py
:Note: It is important to specify the immediate submodules of a directory in the submodules
argument to ensure they are registered at runtime. For example, 'components' for reflex,
'radix' for components, 'themes' for radix, etc.
Pyi_generator
To generate
.pyi
files for__init__.py
files, we read the_SUBMODULES
and_SUBMOD_ATTRS
attributes to generate the import statements. It is highly recommended to define these with
the provided annotations to facilitate their generation.
Aliases
This is a special case to specify an alias for a component.
As an example, we use this typically for
rx.list
where defininglist
attribute in the list.pyovershadows python's list object which messes up the pyi generation for
list.pyi
. As a result, aliasesshould be used for similar cases like this. Note that this logic is employed to fix the pyi generation and alias
should still be defined or accessible. Check out the getattr logic in
reflex/components/radix/themes/layouts/list.py
In the example above, you will be able to do
rx.list
Benchmarking
The following reflex code was tested on mac and windows using hyperfine:
Mac
main
branchThis PR
Windows
main
branchThis PR: