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

Cache JSON when using pydeck local data #7113

Merged
merged 16 commits into from
Aug 9, 2023
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,29 @@
import { DeckGlJsonChart as DeckGlJsonChartProto } from "@streamlit/lib/src/proto"
import { NavigationControl } from "react-map-gl"
import { mockTheme } from "@streamlit/lib/src/mocks/mockTheme"
import { DeckGlJsonChart, PropsWithHeight } from "./DeckGlJsonChart"
import { DeckGlJsonChart, PropsWithHeight, State } from "./DeckGlJsonChart"

const mockInitialViewState = {
bearing: -27.36,
latitude: 52.2323,
longitude: -1.415,
maxZoom: 15,
minZoom: 5,
pitch: 40.5,
zoom: 6,
}

const mockId = "testId"
jest.mock("@streamlit/lib/src/theme", () => ({
hasLightBackgroundColor: jest.fn(() => false),
}))

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

Choose a reason for hiding this comment

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

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

bearing: -27.36,
latitude: 52.2323,
longitude: -1.415,
maxZoom: 15,
minZoom: 5,
pitch: 40.5,
zoom: 6,
},
initialViewState: mockInitialViewState,
layers: [
{
"@@type": "HexagonLayer",
Expand All @@ -62,6 +69,7 @@

return {
element: DeckGlJsonChartProto.create({
id: mockId,
json: JSON.stringify(json),
...elementProps,
}),
Expand Down Expand Up @@ -94,8 +102,10 @@
viewState: { pitch: 5, zoom: 5 },
})

// @ts-expect-error
wrapper.setProps(getProps({}, { pitch: 40.5, zoom: 10 }))
wrapper.setProps(
// @ts-expect-error
getProps({ elementId: "newTestId" }, { pitch: 40.5, zoom: 10 })
)

expect(wrapper.state("viewState")).toStrictEqual({
pitch: 5,
Expand Down Expand Up @@ -142,4 +152,83 @@

expect(createdTooltip).toBe(false)
})

describe("getDeckObject", () => {
const mockJsonParse = JSON.parse

const id = "newTestId"
const newJson = '{"initialViewState": {"height": "100", "width": "100"}}'
const isFullScreen = false
const isLightTheme = false
const initialized = false

const getNewState = (overrides?: Partial<State>): State => {
const defaultState: State = {
pydeckJson: JSON.parse(newJson),
isFullScreen,
viewState: {},
initialized,
initialViewState: mockInitialViewState,
id,
isLightTheme,
}

return { ...defaultState, ...overrides }
}

const originalState: State = {
pydeckJson: JSON.parse(newJson),
isFullScreen,
viewState: {},
initialized,
initialViewState: mockInitialViewState,
id: mockId,
isLightTheme,
}

beforeEach(() => {
JSON.parse = jest.fn(mockJsonParse)
})

afterEach(() => {
JSON.parse = mockJsonParse
})

it("should not call JSON.parse when the element id is the same", () => {
DeckGlJsonChart.getDeckObject(getProps(), originalState)

expect(JSON.parse).not.toHaveBeenCalled()

// state has different elementId from getProps
DeckGlJsonChart.getDeckObject(getProps(), getNewState())

expect(JSON.parse).toHaveBeenCalled()
})

it("should not call JSON.parse when FullScreen state changes", () => {
DeckGlJsonChart.getDeckObject(getProps(), originalState)

expect(JSON.parse).not.toHaveBeenCalled()

Check failure on line 211 in frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.test.tsx

View workflow job for this annotation

GitHub Actions / js_test

Error: expect(jest.fn()).not.toHaveBeenCalled() Expected number of calls: 0 Received number of calls: 1 1: "{\"initialViewState\":{\"bearing\":-27.36,\"latitude\":52.2323,\"longitude\":-1.415,\"maxZoom\":15,\"minZoom\":5,\"pitch\":40.5,\"zoom\":6},\"layers\":[{\"@@type\":\"HexagonLayer\",\"autoHighlight\":true,\"coverage\":1,\"data\":\"https://raw.githubusercontent.com/uber-common/deck.gl-data/master/examples/3d-heatmap/heatmap-data.csv\",\"elevationRange\":[0,3000],\"elevationScale\":50,\"extruded\":true,\"getPosition\":\"@@=[lng, lat]\",\"id\":\"0533490f-fcf9-4dc0-8c94-ae4fbd42eb6f\",\"pickable\":true}],\"mapStyle\":\"mapbox://styles/mapbox/light-v9\",\"views\":[{\"@@type\":\"MapView\",\"controller\":true}]}" at Object.<anonymous> (/home/runner/work/streamlit/streamlit/frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.test.tsx:211:30) at Promise.then.completed (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/utils.js:391:28) at new Promise (<anonymous>) at callAsyncCircusFn (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/utils.js:316:10) at _callCircusTest (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:218:40) at _runTest (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:155:3) at _runTestsForDescribeBlock (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:66:9) at _runTestsForDescribeBlock (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:60:9) at _runTestsForDescribeBlock (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:60:9) at run (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:25:3) at runAndTransformResultsToJestFormat (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:170:21) at jestAdapter (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:68:19) at runTestInternal (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-runner/build/runTest.js:250:16) at runTest (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-runner/build/runTest.js:310:7)

DeckGlJsonChart.getDeckObject(
getProps(),
getNewState({ isFullScreen: true })
)

expect(JSON.parse).toHaveBeenCalled()
})

it("should not call JSON.parse when theme state changes", () => {
DeckGlJsonChart.getDeckObject(getProps(), originalState)

expect(JSON.parse).not.toHaveBeenCalled()

Check failure on line 224 in frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.test.tsx

View workflow job for this annotation

GitHub Actions / js_test

Error: expect(jest.fn()).not.toHaveBeenCalled() Expected number of calls: 0 Received number of calls: 1 1: "{\"initialViewState\":{\"bearing\":-27.36,\"latitude\":52.2323,\"longitude\":-1.415,\"maxZoom\":15,\"minZoom\":5,\"pitch\":40.5,\"zoom\":6},\"layers\":[{\"@@type\":\"HexagonLayer\",\"autoHighlight\":true,\"coverage\":1,\"data\":\"https://raw.githubusercontent.com/uber-common/deck.gl-data/master/examples/3d-heatmap/heatmap-data.csv\",\"elevationRange\":[0,3000],\"elevationScale\":50,\"extruded\":true,\"getPosition\":\"@@=[lng, lat]\",\"id\":\"0533490f-fcf9-4dc0-8c94-ae4fbd42eb6f\",\"pickable\":true}],\"mapStyle\":\"mapbox://styles/mapbox/light-v9\",\"views\":[{\"@@type\":\"MapView\",\"controller\":true}]}" at Object.<anonymous> (/home/runner/work/streamlit/streamlit/frontend/lib/src/components/elements/DeckGlJsonChart/DeckGlJsonChart.test.tsx:224:30) at Promise.then.completed (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/utils.js:391:28) at new Promise (<anonymous>) at callAsyncCircusFn (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/utils.js:316:10) at _callCircusTest (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:218:40) at _runTest (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:155:3) at _runTestsForDescribeBlock (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:66:9) at _runTestsForDescribeBlock (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:60:9) at _runTestsForDescribeBlock (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:60:9) at run (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/run.js:25:3) at runAndTransformResultsToJestFormat (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:170:21) at jestAdapter (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:68:19) at runTestInternal (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-runner/build/runTest.js:250:16) at runTest (/home/runner/work/streamlit/streamlit/frontend/node_modules/jest-runner/build/runTest.js:310:7)

DeckGlJsonChart.getDeckObject(
getProps(),
getNewState({ isLightTheme: true })
)

expect(JSON.parse).toHaveBeenCalled()
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ import { registerLoaders } from "@loaders.gl/core"
import withFullScreenWrapper from "@streamlit/lib/src/hocs/withFullScreenWrapper"
import withMapboxToken from "@streamlit/lib/src/hocs/withMapboxToken"

import { notNullOrUndefined } from "@streamlit/lib/src/util/utils"

import { DeckGlJsonChart as DeckGlJsonChartProto } from "@streamlit/lib/src/proto"
import {
StyledDeckGlChart,
Expand Down Expand Up @@ -72,21 +70,26 @@ registerLoaders([CSVLoader, GLTFLoader])

const jsonConverter = new JSONConverter({ configuration })

interface Props {
export interface DeckGLProps {
width: number
theme: EmotionTheme
mapboxToken: string
element: DeckGlJsonChartProto
isFullScreen?: boolean
}

export interface PropsWithHeight extends Props {
export interface PropsWithHeight extends DeckGLProps {
height?: number
}

interface State {
export interface State {
viewState: Record<string, unknown>
initialized: boolean
initialViewState: Record<string, unknown>
id: string | undefined
pydeckJson: any
isFullScreen: boolean
isLightTheme: boolean
}

export const DEFAULT_DECK_GL_HEIGHT = 500
Expand All @@ -100,6 +103,10 @@ export class DeckGlJsonChart extends PureComponent<PropsWithHeight, State> {
},
initialized: false,
initialViewState: {},
id: undefined,
pydeckJson: undefined,
isFullScreen: false,
isLightTheme: hasLightBackgroundColor(this.props.theme),
}

componentDidMount = (): void => {
Expand All @@ -115,7 +122,7 @@ export class DeckGlJsonChart extends PureComponent<PropsWithHeight, State> {
props: Readonly<PropsWithHeight>,
state: Partial<State>
): Partial<State> | null {
const deck = DeckGlJsonChart.getDeckObject(props)
const deck = DeckGlJsonChart.getDeckObject(props, state)

// If the ViewState on the server has changed, apply the diff to the current state
if (!isEqual(deck.initialViewState, state.initialViewState)) {
Expand Down Expand Up @@ -144,35 +151,50 @@ export class DeckGlJsonChart extends PureComponent<PropsWithHeight, State> {
return null
}

static getDeckObject = (props: PropsWithHeight): DeckObject => {
const { element, width, height, theme } = props
const json = JSON.parse(element.json)
static getDeckObject = (
props: PropsWithHeight,
state: Partial<State>
): DeckObject => {
const { element, width, height, theme, isFullScreen } = props

const currFullScreen = isFullScreen ?? false

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

if (
element.id !== state.id ||
state.isFullScreen !== currFullScreen ||
sfc-gh-wihuang marked this conversation as resolved.
Show resolved Hide resolved
state.isLightTheme !== hasLightBackgroundColor(theme)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

) {
state.pydeckJson = JSON.parse(element.json)
state.id = element.id
}

// If unset, use either the Mapbox light or dark style based on Streamlit's theme
// For Mapbox styles, see https://docs.mapbox.com/api/maps/styles/#mapbox-styles
if (!notNullOrUndefined(json.mapStyle)) {
const mapTheme = hasLightBackgroundColor(theme) ? "light" : "dark"
json.mapStyle = `mapbox://styles/mapbox/${mapTheme}-v9`
if (!state.pydeckJson?.mapStyle) {
state.pydeckJson.mapStyle = `mapbox://styles/mapbox/${
hasLightBackgroundColor(theme) ? "light" : "dark"
}-v9`
}

// The graph dimensions could be set from props ( like withFullscreen ) or
// from the generated element object
if (height) {
json.initialViewState.height = height
json.initialViewState.width = width
// Set width and height based on the fullscreen state
if (isFullScreen) {
Object.assign(state.pydeckJson?.initialViewState, { width, height })
} else {
if (!json.initialViewState.height) {
json.initialViewState.height = DEFAULT_DECK_GL_HEIGHT
if (!state.pydeckJson?.initialViewState?.height) {
state.pydeckJson.initialViewState.height = DEFAULT_DECK_GL_HEIGHT
}

if (element.useContainerWidth) {
json.initialViewState.width = width
state.pydeckJson.initialViewState.width = width
}
}

delete json.views // We are not using views. This avoids a console warning.
state.isFullScreen = isFullScreen
state.isLightTheme = hasLightBackgroundColor(theme)

delete state.pydeckJson?.views // We are not using views. This avoids a console warning.

return jsonConverter.convert(json)
return jsonConverter.convert(state.pydeckJson)
}

createTooltip = (info: PickingInfo): Record<string, unknown> | boolean => {
Expand Down Expand Up @@ -213,9 +235,8 @@ export class DeckGlJsonChart extends PureComponent<PropsWithHeight, State> {
}

render(): ReactNode {
const deck = DeckGlJsonChart.getDeckObject(this.props)
const deck = DeckGlJsonChart.getDeckObject(this.props, this.state)
const { viewState } = this.state

return (
<StyledDeckGlChart
className="stDeckGlJsonChart"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import hoistNonReactStatics from "hoist-non-react-statics"

import FullScreenWrapper from "@streamlit/lib/src/components/shared/FullScreenWrapper"

interface Props {
export interface Props {
willhuang1997 marked this conversation as resolved.
Show resolved Hide resolved
width: number
height?: number
}
Expand Down
7 changes: 7 additions & 0 deletions lib/streamlit/elements/deck_gl_json_chart.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import hashlib
import json
from typing import TYPE_CHECKING, Any, Dict, Mapping, Optional, cast

Expand Down Expand Up @@ -156,12 +157,18 @@ def marshall(
) -> None:
if pydeck_obj is None:
spec = json.dumps(EMPTY_MAP)
id = ""
else:
spec = pydeck_obj.to_json()
json_string = json.dumps(spec)
json_bytes = json_string.encode("utf-8")
id = hashlib.md5(json_bytes).hexdigest()

pydeck_proto.json = spec
pydeck_proto.use_container_width = use_container_width

pydeck_proto.id = id

tooltip = _get_pydeck_tooltip(pydeck_obj)
if tooltip:
pydeck_proto.tooltip = json.dumps(tooltip)
5 changes: 4 additions & 1 deletion proto/streamlit/proto/DeckGlJsonChart.proto
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@
syntax = "proto3";

message DeckGlJsonChart {
// The dataframe that will be used as the chart's main data source.
// The json of the pydeck object (https://deckgl.readthedocs.io/en/latest/deck.html)
string json = 1;

string tooltip = 2;

// If True, will overwrite the chart width spec to fit to container.
bool use_container_width = 4;

// the hash of the json so the the frontend doesn't always have to parse the pydeck json object
string id = 5;
}