-
Notifications
You must be signed in to change notification settings - Fork 148
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
Optimize the client-server communication #34
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.
Took a quick look - the example dashboard definitely feels quicker! 🚀 🐎 Great job, Petar!
I have a few open questions, some suggestion but would have to take a detailed look when I am back! If the PR is ready by this week, I'll leave the review to Max or Lingyi :)
Otherwise i'll take a look when I'm back
vizro-core/src/vizro/actions/_action_loop/_build_action_loop_callbacks.py
Outdated
Show resolved
Hide resolved
vizro-core/src/vizro/actions/_action_loop/_build_action_loop_callbacks.py
Outdated
Show resolved
Hide resolved
vizro-core/src/vizro/actions/_action_loop/_build_action_loop_callbacks.py
Outdated
Show resolved
Hide resolved
vizro-core/src/vizro/actions/_action_loop/_build_action_loop_callbacks.py
Show resolved
Hide resolved
vizro-core/src/vizro/actions/_action_loop/_build_action_loop_callbacks.py
Show resolved
Hide resolved
vizro-core/changelog.d/20230918_111226_petar_pejovic_optimise_app_rendering_time.md
Show resolved
Hide resolved
Another future use case relevant to this topic: when a dashboard needs to connect to the database and pull data from it,
https://dash.plotly.com/clientside-callbacks How should we construct the action chain if there is a DB call involved |
Only the internal mechanism (the mechanism that allows the action loop to work) is implemented using clientside-callbacks. If someone needs a DB connection, they can create a custom action or we can release a predefined action for that (either way it's an Action). Remember that Action callbacks are |
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.
Thanks for the optimizations! Started a discussion on a few routes we can take to move forward
vizro-core/changelog.d/20230918_111226_petar_pejovic_optimise_app_rendering_time.md
Outdated
Show resolved
Hide resolved
vizro-core/tests/unit/vizro/models/_components/form/test_range_slider.py
Show resolved
Hide resolved
…r/optimise_app_rendering_time
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.
LGTM!
Have a few very small suggestions that we should do before merging. Anything marked as (for later) please ignore until we have merged.
vizro-core/changelog.d/20230918_111226_petar_pejovic_optimise_app_rendering_time.md
Outdated
Show resolved
Hide resolved
vizro-core/changelog.d/20230918_111226_petar_pejovic_optimise_app_rendering_time.md
Outdated
Show resolved
Hide resolved
vizro-core/changelog.d/20230918_111226_petar_pejovic_optimise_app_rendering_time.md
Outdated
Show resolved
Hide resolved
vizro-core/src/vizro/actions/_action_loop/_build_action_loop_callbacks.py
Show resolved
Hide resolved
…app_rendering_time.md Co-authored-by: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com>
…app_rendering_time.md Co-authored-by: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com>
…app_rendering_time.md Co-authored-by: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com>
Co-authored-by: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com>
Co-authored-by: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com>
Co-authored-by: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com>
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.
LGTM! See great speed improvement
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.
Lgtm now!
Description
By merging some callbacks and rewriting some of them into the clientside_callback form (pure javascript code processed on the user's client browser machine), the rendering time (when someone opens a new page or applies some action like filter, export_data...) is significantly reduced.
Progress:
trigger_to_global_store
as clientside_callback.gateway
,set_remaining
andexecutor
into one single callback.after_action
andcycle_breaker
callbacks into one clientside_callback.jest
(run with command:hatch run test-js
)Speed-up
Screenshot
Action loop architecture for the current system:
Action loop architecture for the optimised system (after we rewrite the gateway callback into the client-side one):
Consequences
To be discussed (not the part of this PR):
vizro.themes
on the frontend side as well? Then to convert theupdate_graph_theme
callback into the clienside_callback too.@lru_cache
on the server functions.ToDo:
Checklist
Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1))
(if applicable)Types of changes