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

Optimize the client-server communication #34

Merged
merged 47 commits into from
Sep 25, 2023

Conversation

petar-qb
Copy link
Contributor

@petar-qb petar-qb commented Sep 13, 2023

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:

  • Implement action loop trigger_to_global_store as clientside_callback.
  • Merge gateway , set_remaining and executor into one single callback.
  • Rewrite the merged (new gateway) callback as clientside_callback.
  • Merge after_action and cycle_breaker callbacks into one clientside_callback.
  • Overwrite RangeSlider, Slider, ThemeSelector and other callbacks in the system to clientside_callback.
  • Try to merge two callbacks where input is theme_selector. (IMPOSIBLE because Patch object isn't supported in the clientside_callback) - actually it this fix isn’t needed.
  • Add tests for client-side callbacks written in Node.js tool called jest (run with command: hatch run test-js)

Speed-up

opening a new page filer action export_data action
Current system 4050 ms 3100 ms 3300 ms
Optimised system 850 ms 490 ms 470 ms
Speed-up 4.75x 6.3x 7.0 x

Screenshot

Action loop architecture for the current system:
image

Action loop architecture for the optimised system (after we rewrite the gateway callback into the client-side one):
image

Consequences

  • There's no more logs on the server-side that show which component is triggered and what actions chain will be executed.
  • Since many callbacks have been moved to javascript code, we can no longer unit test them using the pytest.

To be discussed (not the part of this PR):

  • It's probably possible to save the vizro.themes on the frontend side as well? Then to convert the update_graph_theme callback into the clienside_callback too.
  • Try to get rid of dash.pages / or wrap (decorate) it.
  • At the user's request, enable keeping original (or filtered) data on the frontend, instead of in the DataManager.
  • Put some kind of @lru_cache on the server functions.

ToDo:

  • Handle when a new actions chain is started before the previous one has finished.

Checklist

  • I have not referenced individuals, products or companies in any commits, directly or indirectly
  • I have not added data or restricted code in any commits, directly or indirectly
  • I have updated the docstring of any public function/class/model changed
  • I have added the PR number to the change description in the changelog fragment, e.g. Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1)) (if applicable)

Types of changes

  • Docs/refactoring (non-breaking change which improves codebase)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a 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

@petar-qb petar-qb self-assigned this Sep 15, 2023
@petar-qb petar-qb marked this pull request as ready for review September 18, 2023 11:24
@lingyielia
Copy link
Contributor

Another future use case relevant to this topic: when a dashboard needs to connect to the database and pull data from it,

Clientside callbacks are not possible if you need to refer to global variables on the server or a DB call is required.

https://dash.plotly.com/clientside-callbacks

How should we construct the action chain if there is a DB call involved

@petar-qb
Copy link
Contributor Author

@lingyielia

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 blue rectangles in the architecture sketch (look Description) which means that they are not client-side callbacks, but regular dash.callbacks that "live in the server".

Copy link
Contributor

@maxschulz-COL maxschulz-COL left a 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/hatch.toml Outdated Show resolved Hide resolved
@petar-qb petar-qb removed the request for review from huong-li-nguyen September 25, 2023 11:37
Copy link
Contributor

@maxschulz-COL maxschulz-COL left a 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/docs/pages/development/contributing.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/development/contributing.md Outdated Show resolved Hide resolved
vizro-core/hatch.toml Outdated Show resolved Hide resolved
vizro-core/src/vizro/static/js/clientside_functions.js Outdated Show resolved Hide resolved
vizro-core/package.json Outdated Show resolved Hide resolved
petar-qb and others added 6 commits September 25, 2023 13:58
…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>
.gitignore Outdated Show resolved Hide resolved
vizro-core/hatch.toml Show resolved Hide resolved
Copy link
Contributor

@lingyielia lingyielia left a 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

Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

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

Lgtm now!

@huong-li-nguyen huong-li-nguyen changed the title Optimizing the client-server communication Optimize the client-server communication Sep 25, 2023
@maxschulz-COL maxschulz-COL merged commit 3da50d1 into main Sep 25, 2023
21 checks passed
@maxschulz-COL maxschulz-COL deleted the refactor/optimise_app_rendering_time branch September 25, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Ready for Review ☑️ Issue/PR is ready for review - all tests have passed Tidy/Refactoring 🧹 Issue/PR that refactor existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants