Cache JSON when using pydeck local data #7113

willhuang1997 · 2023-08-03T22:43:58Z

Describe your changes

add a protobuf key named element id which is a hash of the pydeck json
add logic to not parse the json again where the json is stored in state
- if the hash string is different, then parse it again
- if it’s not, use the cached json
add fullScreen and theme to state
- if these change, we need to parse the json again to rerender the new map
these changes make the loading slightly longer when rerunning or running the script; however, the experience is significantly better so it's worth to do this.

GitHub Issue Link (if applicable)

#5532

Testing Plan

Explanation of why no additional tests are needed
Unit Tests (JS and/or Python)
will need to add these
E2E Tests
very difficult to add these so will not be adding these.
Any manual testing needed?
yes, i manually tested the test case within the issue 5532. I will grab other use cases from pydeck and make sure they're responsive.

This branch:
https://github.com/streamlit/streamlit/assets/16749069/130d21ca-efe4-437b-aa6e-fb1a04c6805b

Develop:
https://github.com/streamlit/streamlit/assets/16749069/275b4e00-4f1d-444e-bf69-f84040c9b86f

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

willhuang1997 · 2023-08-07T18:03:55Z

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.test.tsx


 const getProps = (
  elementProps: Partial<DeckGlJsonChartProto> = {},
  initialViewStateProps: Record<string, unknown> = {}
 ): PropsWithHeight => {
  const json = {
-    initialViewState: {


just moved this to top of file so I can reuse later in the tests

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.test.tsx

willhuang1997 · 2023-08-07T18:17:04Z

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.tsx

+
+    const isFullScreen = Boolean(height)
+
+    // Only parse JSON when not transitioning to/from fullscreen, the element id changes, or theme changes


maybe i should remove this comment since it's not necessary?

You could also add one more sentence here why we need to reparse with these changes

proto/streamlit/proto/DeckGlJsonChart.proto

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.tsx

frontend/lib/src/hocs/withFullScreenWrapper/withFullScreenWrapper.tsx

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.tsx

LukasMasuch · 2023-08-07T22:19:58Z

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.tsx

+    if (
+      element.id !== id ||
+      state.isFullScreen !== currFullScreen ||
+      state.isLightTheme !== hasLightBackgroundColor(theme)


Why do we need to reparse this when layout changes? Aren't we always adapting the mapStyle below anyways based on the theme?

lib/streamlit/elements/deck_gl_json_chart.py

LukasMasuch

LGTM if the comments above are checked 👍 This component could probably benefit from a rewrite to functional API sometime later since this is a perfect usecase for useState with dependencies.

sfc-gh-wihuang · 2023-08-07T22:59:28Z

LGTM if the comments above are checked 👍 This component could probably benefit from a rewrite to functional API sometime later since this is a perfect usecase for useState with dependencies.

Will fix them. Currently in progress! thanks!

LukasMasuch · 2023-08-07T23:14:18Z

Ahh, and maybe this would be a good opportunity to also rewrite the unit test of deckgl component to use RTL instead of enzyme (shallow).

sfc-gh-wihuang · 2023-08-07T23:19:52Z

Ahh, and maybe this would be a good opportunity to also rewrite the unit test of deckgl component to use RTL instead of enzyme (shallow).

Sounds good. I'll add that to things to do this week

…oing to make these plain unit tests

willhuang1997 · 2023-08-08T20:43:07Z

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.test.tsx

-    deckGL.prop("onViewStateChange")({
-      viewState: { pitch: 5, zoom: 5 },
-    })
+    const state = {


Changed this test to not involve calling DeckGl methods because that seems odd. So I just changed this test to test our getDerivedStateFromProps function.

Testing the changing of pydeck with unit testing and RTL is turning out to be a little too difficult as we can click the zoom button but it's not immediately obvious how to check the map changed. That should likely be tested with e2e tests and thus changing this test seems like a reasonable thing from my perspective.

willhuang1997 · 2023-08-08T20:43:23Z

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.test.tsx

-  it("should render tooltip", () => {
-    const props = getProps({
-      tooltip: `{"html": "<b>Elevation Value:</b> {elevationValue}", "style": {"color": "white"}}`,
+  describe("createTooltip", () => {


Changed this test to not involve calling DeckGl methods because that seems odd. So I just changed this test to test our createTooltip function.

Testing the changing of pydeck with unit testing and RTL is turning out to be a little too difficult as we can hover but it's not immediately obvious how to check the tooltip existing. That should likely be tested with e2e tests and thus changing this test seems like a reasonable thing from my perspective.

## Describe your changes - add a protobuf key named element id which is a hash of the pydeck json - add logic to not parse the json again where the json is stored in state - if the hash string is different, then parse it again - if it’s not, use the cached json - add fullScreen and theme to state - if these change, we need to parse the json again to rerender the new map - these changes make the loading slightly longer when rerunning or running the script; however, the experience is significantly better so it's worth to do this. ## GitHub Issue Link (if applicable) streamlit#5532 ## Testing Plan - Explanation of why no additional tests are needed - Unit Tests (JS and/or Python) - will need to add these - E2E Tests - very difficult to add these so will not be adding these. - Any manual testing needed? - yes, i manually tested the test case within the issue 5532. I will grab other use cases from pydeck and make sure they're responsive. This branch: https://github.com/streamlit/streamlit/assets/16749069/130d21ca-efe4-437b-aa6e-fb1a04c6805b Develop: https://github.com/streamlit/streamlit/assets/16749069/275b4e00-4f1d-444e-bf69-f84040c9b86f --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

willhuang1997 added 4 commits August 3, 2023 14:30

Moving the map works but full screen broke

c148941

Fix fullscreen problem

bfb51f6

Cleanup

9facb03

more cleanup

e4e6f44

willhuang1997 added change:bugfix impact:users security-assessment-completed labels Aug 7, 2023

willhuang1997 added 3 commits August 7, 2023 10:12

Fix theming changes

15999f6

Fix tests

a6270ef

Fix tests

69d30c4

willhuang1997 commented Aug 7, 2023

View reviewed changes

github-advanced-security bot found potential problems Aug 7, 2023

View reviewed changes

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.test.tsx Fixed Show resolved Hide resolved

Remove unused import

af51b14

willhuang1997 marked this pull request as ready for review August 7, 2023 18:10

willhuang1997 requested review from vdonato, LukasMasuch and sfc-gh-mnowotka as code owners August 7, 2023 18:10

willhuang1997 commented Aug 7, 2023

View reviewed changes

LukasMasuch reviewed Aug 7, 2023

View reviewed changes

proto/streamlit/proto/DeckGlJsonChart.proto Outdated Show resolved Hide resolved

LukasMasuch reviewed Aug 7, 2023

View reviewed changes

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.tsx Outdated Show resolved Hide resolved

willhuang1997 added 4 commits August 7, 2023 11:28

Change element_id to id to be same as rest of codebase

c2eed5c

Fix Test Error

ac78cf9

Remove accidentally added files

2cc5f97

Fix comment about isFullScreen

426d73d

willhuang1997 commented Aug 7, 2023

View reviewed changes

frontend/lib/src/hocs/withFullScreenWrapper/withFullScreenWrapper.tsx Show resolved Hide resolved

LukasMasuch reviewed Aug 7, 2023

View reviewed changes

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.tsx Outdated Show resolved Hide resolved

LukasMasuch reviewed Aug 7, 2023

View reviewed changes

frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.tsx Show resolved Hide resolved

LukasMasuch reviewed Aug 7, 2023

View reviewed changes

lib/streamlit/elements/deck_gl_json_chart.py Outdated Show resolved Hide resolved

Minor fixes from comments

a08deb9

LukasMasuch approved these changes Aug 7, 2023

View reviewed changes

willhuang1997 added 2 commits August 8, 2023 13:36

Change tests to not test DeckGL. I found RTL harder to implement so g…

d0dafef

…oing to make these plain unit tests

Remove unused imports

bfecde4

willhuang1997 commented Aug 8, 2023

View reviewed changes

Readd react import that was deleted by VSCode

498a117

willhuang1997 merged commit 1ef8b83 into develop Aug 9, 2023
49 checks passed

willhuang1997 mentioned this pull request Aug 10, 2023

Streamlit+PyDeck Performance Issues (compared to non-streamlit usage of Pydeck with local data object) #5532

Closed

vdonato deleted the fix/5532 branch August 19, 2023 01:54

LukasMasuch mentioned this pull request Jan 23, 2024

st.map is narrow inside expander unless another map is added #8004

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache JSON when using pydeck local data #7113

Cache JSON when using pydeck local data #7113

willhuang1997 commented Aug 3, 2023 •

edited

willhuang1997 Aug 7, 2023

willhuang1997 Aug 7, 2023

LukasMasuch Aug 7, 2023 •

edited

LukasMasuch Aug 7, 2023

LukasMasuch left a comment •

edited

sfc-gh-wihuang commented Aug 7, 2023

LukasMasuch commented Aug 7, 2023 •

edited

sfc-gh-wihuang commented Aug 7, 2023

willhuang1997 Aug 8, 2023 •

edited

willhuang1997 Aug 8, 2023 •

edited


		const isFullScreen = Boolean(height)

		// Only parse JSON when not transitioning to/from fullscreen, the element id changes, or theme changes

Cache JSON when using pydeck local data #7113

Cache JSON when using pydeck local data #7113

Conversation

willhuang1997 commented Aug 3, 2023 • edited

Describe your changes

GitHub Issue Link (if applicable)

Testing Plan

willhuang1997 Aug 7, 2023

Choose a reason for hiding this comment

willhuang1997 Aug 7, 2023

Choose a reason for hiding this comment

LukasMasuch Aug 7, 2023 • edited

Choose a reason for hiding this comment

LukasMasuch Aug 7, 2023

Choose a reason for hiding this comment

LukasMasuch left a comment • edited

Choose a reason for hiding this comment

sfc-gh-wihuang commented Aug 7, 2023

LukasMasuch commented Aug 7, 2023 • edited

sfc-gh-wihuang commented Aug 7, 2023

willhuang1997 Aug 8, 2023 • edited

Choose a reason for hiding this comment

willhuang1997 Aug 8, 2023 • edited

Choose a reason for hiding this comment

willhuang1997 commented Aug 3, 2023 •

edited

LukasMasuch Aug 7, 2023 •

edited

LukasMasuch left a comment •

edited

LukasMasuch commented Aug 7, 2023 •

edited

willhuang1997 Aug 8, 2023 •

edited

willhuang1997 Aug 8, 2023 •

edited