From 9361e7d5c722718da5bf5f662c41544810a66621 Mon Sep 17 00:00:00 2001 From: Antony Milne Date: Mon, 13 Nov 2023 11:06:06 +0000 Subject: [PATCH 1/8] Remove dead code --- vizro-core/src/vizro/actions/__init__.py | 9 --------- vizro-core/src/vizro/models/_dashboard.py | 9 ++------- vizro-core/tests/unit/vizro/models/test_dashboard.py | 7 ------- 3 files changed, 2 insertions(+), 23 deletions(-) diff --git a/vizro-core/src/vizro/actions/__init__.py b/vizro-core/src/vizro/actions/__init__.py index f3411d31f..acc394f56 100644 --- a/vizro-core/src/vizro/actions/__init__.py +++ b/vizro-core/src/vizro/actions/__init__.py @@ -9,12 +9,3 @@ # Please keep alphabetically ordered __all__ = ["export_data", "filter_interaction"] - -# Actions lookup dictionary to facilitate function comparison -action_functions: Dict[Callable[[Any], Dict[str, Any]], str] = { - export_data.__wrapped__: "export_data", - filter_interaction.__wrapped__: "filter_interaction", - _filter.__wrapped__: "filter", - _parameter.__wrapped__: "parameter", - _on_page_load.__wrapped__: "on_page_load", -} diff --git a/vizro-core/src/vizro/models/_dashboard.py b/vizro-core/src/vizro/models/_dashboard.py index 11d482706..c4f55250c 100644 --- a/vizro-core/src/vizro/models/_dashboard.py +++ b/vizro-core/src/vizro/models/_dashboard.py @@ -23,10 +23,6 @@ logger = logging.getLogger(__name__) -def update_theme(on: bool): - return "vizro_dark" if on else "vizro_light" - - class Dashboard(VizroBaseModel): """Vizro Dashboard to be used within [`Vizro`][vizro._vizro.Vizro.build]. @@ -65,6 +61,7 @@ def __init__(self, *args, **kwargs): # pio is the backend global state and shouldn't be changed while # the app is running. This limitation leads to the case that Graphs blink # on page load if user previously has changed theme_selector. + # AM: Did I fix this? No need to set this any more? pio.templates.default = self.theme @_log_call @@ -103,9 +100,7 @@ def _make_page_layout(self, page: Page): if self.title else html.Div(hidden=True, id="dashboard_title_outer") ) - theme_switch = daq.BooleanSwitch( - id="theme_selector", on=True if self.theme == "vizro_dark" else False, persistence=True - ) + theme_switch = daq.BooleanSwitch(id="theme_selector", on=self.theme == "vizro_dark", persistence=True) # Shared across pages but slightly differ in content page_title = html.H2(children=page.title, id="page_title") diff --git a/vizro-core/tests/unit/vizro/models/test_dashboard.py b/vizro-core/tests/unit/vizro/models/test_dashboard.py index 43353b85d..8f2937153 100644 --- a/vizro-core/tests/unit/vizro/models/test_dashboard.py +++ b/vizro-core/tests/unit/vizro/models/test_dashboard.py @@ -12,7 +12,6 @@ import vizro import vizro.models as vm from vizro.actions._action_loop._action_loop import ActionLoop -from vizro.models._dashboard import update_theme @pytest.fixture() @@ -163,9 +162,3 @@ def test_dashboard_build(self, dashboard_container, dashboard): result = json.loads(json.dumps(dashboard.build(), cls=plotly.utils.PlotlyJSONEncoder)) expected = json.loads(json.dumps(dashboard_container, cls=plotly.utils.PlotlyJSONEncoder)) assert result == expected - - -@pytest.mark.parametrize("on, expected", [(True, "vizro_dark"), (False, "vizro_light")]) -def test_update_theme(on, expected): - result = update_theme(on) - assert result == expected From 3e9d7eab27c6422500d1d954bded262aa358739c Mon Sep 17 00:00:00 2001 From: Antony Milne Date: Mon, 13 Nov 2023 11:31:33 +0000 Subject: [PATCH 2/8] Move update_layout code --- vizro-core/src/vizro/actions/_actions_utils.py | 3 --- .../src/vizro/models/_components/graph.py | 8 ++++++++ vizro-core/src/vizro/models/_dashboard.py | 15 ++++++--------- vizro-core/src/vizro/models/_page.py | 17 ++++++++++++----- 4 files changed, 26 insertions(+), 17 deletions(-) diff --git a/vizro-core/src/vizro/actions/_actions_utils.py b/vizro-core/src/vizro/actions/_actions_utils.py index e82756e03..759418c10 100644 --- a/vizro-core/src/vizro/actions/_actions_utils.py +++ b/vizro-core/src/vizro/actions/_actions_utils.py @@ -267,7 +267,4 @@ def _get_modified_page_figures( outputs[target] = model_manager[target]( # type: ignore[operator] data_frame=filtered_data[target], **parameterized_config[target] ) - if hasattr(outputs[target], "update_layout"): - outputs[target].update_layout(template="vizro_dark" if ctd_theme["value"] else "vizro_light") - return outputs diff --git a/vizro-core/src/vizro/models/_components/graph.py b/vizro-core/src/vizro/models/_components/graph.py index cb792615e..3dd3889df 100644 --- a/vizro-core/src/vizro/models/_components/graph.py +++ b/vizro-core/src/vizro/models/_components/graph.py @@ -56,6 +56,14 @@ def __call__(self, **kwargs): # Remove top margin if title is provided if fig.layout.title.text is None: fig.update_layout(margin_t=24) + + try: + fig.update_layout(template="vizro_dark" if ctx.states["theme_selector.on"] else "vizro_light") + except MissingCallbackContextException: + # Possibly we should enforce that __call__ can only be used within the context of a callback, but it's easy + # to just swallow up the error here as it doesn't cause any problems. + logger.info("fig.update_layout called outside of callback context.") + return fig # Convenience wrapper/syntactic sugar. diff --git a/vizro-core/src/vizro/models/_dashboard.py b/vizro-core/src/vizro/models/_dashboard.py index c4f55250c..d966cb3c7 100644 --- a/vizro-core/src/vizro/models/_dashboard.py +++ b/vizro-core/src/vizro/models/_dashboard.py @@ -80,7 +80,12 @@ def pre_build(self): def build(self): for page in self.pages: page.build() # TODO: ideally remove, but necessary to register slider callbacks - self._update_theme() + + clientside_callback( + ClientsideFunction(namespace="clientside", function_name="update_dashboard_theme"), + Output("dashboard_container_outer", "className"), + Input("theme_selector", "on"), + ) return dbc.Container( id="dashboard_container_outer", @@ -122,14 +127,6 @@ def _make_page_layout(self, page: Page): right_side = html.Div(children=[header, component_container], className="right_side", id="right_side_outer") return html.Div([left_side, right_side], className="page_container", id="page_container_outer") - @staticmethod - def _update_theme(): - clientside_callback( - ClientsideFunction(namespace="clientside", function_name="update_dashboard_theme"), - Output("dashboard_container_outer", "className"), - Input("theme_selector", "on"), - ) - @staticmethod def _make_page_404_layout(): return html.Div( diff --git a/vizro-core/src/vizro/models/_page.py b/vizro-core/src/vizro/models/_page.py index e8e0a8245..701ccb4d0 100644 --- a/vizro-core/src/vizro/models/_page.py +++ b/vizro-core/src/vizro/models/_page.py @@ -136,6 +136,17 @@ def build(self): return html.Div([control_panel, components_container]) def _update_graph_theme(self): + # The obvious way to do this would be to alter pio.templates.default, but this changes global state and so is + # not good. + # Putting graphs as inputs here would be a nice way to trigger the theme change automatically so that we don't + # need the update_layout call in Graph.__call__, but this results in an extra callback and the graph + # flickering. + # TODO: consider making this clientside callback and then possibly we can remove the update_layout in + # Graph.__call__ without any flickering. + # TODO: consider putting this as dashboard level like with _update_theme (though possibly pointless because you + # don't see graphs on other pages and then when you change page it always redraws graphs anyway). + # TODO: consider putting the Graph-specific logic here in the Graph model itself (whether clientside or + # serverside) to keep the code here abstract. outputs = [ Output(component.id, "figure", allow_duplicate=True) for component in self.components @@ -143,11 +154,7 @@ def _update_graph_theme(self): ] if outputs: - @callback( - outputs, - Input("theme_selector", "on"), - prevent_initial_call="initial_duplicate", - ) + @callback(outputs, Input("theme_selector", "on"), prevent_initial_call="initial_duplicate") def update_graph_theme(theme_selector_on: bool): patched_figure = Patch() patched_figure["layout"]["template"] = themes.dark if theme_selector_on else themes.light From 9f9dd45163958efeeb226572ab7f5264f35e4a41 Mon Sep 17 00:00:00 2001 From: Antony Milne Date: Mon, 13 Nov 2023 11:53:44 +0000 Subject: [PATCH 3/8] Fix flickering graph behaviour --- .../src/vizro/models/_components/graph.py | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/vizro-core/src/vizro/models/_components/graph.py b/vizro-core/src/vizro/models/_components/graph.py index 3dd3889df..3822a90e2 100644 --- a/vizro-core/src/vizro/models/_components/graph.py +++ b/vizro-core/src/vizro/models/_components/graph.py @@ -1,7 +1,8 @@ import logging from typing import List, Literal -from dash import dcc +from dash import dcc, ctx +from dash.exceptions import MissingCallbackContextException from plotly import graph_objects as go from pydantic import Field, PrivateAttr, validator @@ -16,18 +17,6 @@ logger = logging.getLogger(__name__) -def create_empty_fig(message: str) -> go.Figure: - """Creates empty go.Figure object with a display message.""" - fig = go.Figure() - fig.add_trace(go.Scatter(x=[None], y=[None], showlegend=False, hoverinfo="none")) - fig.update_layout( - xaxis={"visible": False}, - yaxis={"visible": False}, - annotations=[{"text": message, "showarrow": False, "font": {"size": 16}}], - ) - return fig - - class Graph(VizroBaseModel): """Wrapper for `dcc.Graph` to visualize charts in dashboard. @@ -58,6 +47,8 @@ def __call__(self, **kwargs): fig.update_layout(margin_t=24) try: + # if "theme_selector" in ctx.states # Probably want this here given code should work outside Dashboard + # for now fig.update_layout(template="vizro_dark" if ctx.states["theme_selector.on"] else "vizro_light") except MissingCallbackContextException: # Possibly we should enforce that __call__ can only be used within the context of a callback, but it's easy @@ -76,14 +67,21 @@ def __getitem__(self, arg_name: str): @_log_call def build(self): + # The empty figure here is just a placeholder designed to be replaced by the actual figure when the filters + # etc. are applied. It only appears on the screen for a brief instant, but we need to make sure it's + # transparent and has no axes so it doesn't draw anything on the screen which would flicker away when the + # graph callback is executed to make the dcc.Loading icon appear. return dcc.Loading( dcc.Graph( id=self.id, - # We don't do self.__call__() until the Graph is actually built. This ensures that lazy data is not - # loaded until the graph is first shown on the screen. At the moment, we eagerly run page.build() for - # all pages in Dashboard.build in order to register all the callbacks in advance. In future this should - # no longer be the case so that we achieve true lazy loading. - figure=create_empty_fig(""), + figure=go.Figure( + layout={ + "paper_bgcolor": "rgba(0,0,0,0)", + "plot_bgcolor": "rgba(0,0,0,0)", + "xaxis": {"visible": False}, + "yaxis": {"visible": False}, + } + ), config={ "autosizable": True, "frameMargins": 0, From cf4ead19896e89706e60a01447bb784aab2554d1 Mon Sep 17 00:00:00 2001 From: Antony Milne Date: Mon, 13 Nov 2023 12:25:10 +0000 Subject: [PATCH 4/8] Remove pio.default setting --- vizro-core/src/vizro/models/_dashboard.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/vizro-core/src/vizro/models/_dashboard.py b/vizro-core/src/vizro/models/_dashboard.py index d966cb3c7..1cf23d638 100644 --- a/vizro-core/src/vizro/models/_dashboard.py +++ b/vizro-core/src/vizro/models/_dashboard.py @@ -56,14 +56,6 @@ def validate_navigation(cls, navigation, values): return Navigation(pages=[page.id for page in values["pages"]]) return navigation - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - # pio is the backend global state and shouldn't be changed while - # the app is running. This limitation leads to the case that Graphs blink - # on page load if user previously has changed theme_selector. - # AM: Did I fix this? No need to set this any more? - pio.templates.default = self.theme - @_log_call def pre_build(self): # Setting order here ensures that the pages in dash.page_registry preserves the order of the List[Page]. From 6cdf2d78998ca3a13959ff4cd2ce9381b1f9a768 Mon Sep 17 00:00:00 2001 From: Antony Milne Date: Mon, 13 Nov 2023 12:36:29 +0000 Subject: [PATCH 5/8] Add changelog --- ...231113_122927_antony.milne_update_theme.md | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 vizro-core/changelog.d/20231113_122927_antony.milne_update_theme.md diff --git a/vizro-core/changelog.d/20231113_122927_antony.milne_update_theme.md b/vizro-core/changelog.d/20231113_122927_antony.milne_update_theme.md new file mode 100644 index 000000000..6aeea568a --- /dev/null +++ b/vizro-core/changelog.d/20231113_122927_antony.milne_update_theme.md @@ -0,0 +1,48 @@ + + + + + + + + + From 772fefb9c49b844f8e86c594310a11aa1b1e0ad4 Mon Sep 17 00:00:00 2001 From: Antony Milne Date: Tue, 14 Nov 2023 15:54:25 +0000 Subject: [PATCH 6/8] Add changelog --- .../changelog.d/20231113_122927_antony.milne_update_theme.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/vizro-core/changelog.d/20231113_122927_antony.milne_update_theme.md b/vizro-core/changelog.d/20231113_122927_antony.milne_update_theme.md index 6aeea568a..6e15f2555 100644 --- a/vizro-core/changelog.d/20231113_122927_antony.milne_update_theme.md +++ b/vizro-core/changelog.d/20231113_122927_antony.milne_update_theme.md @@ -34,12 +34,10 @@ Uncomment the section that is right (remove the HTML comment wrapper). - A bullet item for the Deprecated category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1)) --> - + ### Fixed - Remove graph flickering on page load with Vizro light theme ([#166](https://github.com/mckinsey/vizro/pull/166)) diff --git a/vizro-core/src/vizro/actions/__init__.py b/vizro-core/src/vizro/actions/__init__.py index acc394f56..39224154f 100644 --- a/vizro-core/src/vizro/actions/__init__.py +++ b/vizro-core/src/vizro/actions/__init__.py @@ -1,6 +1,3 @@ -# Redundant aliases here to prevent ruff from removing unused imports. -from typing import Any, Callable, Dict - from vizro.actions._filter_action import _filter from vizro.actions._on_page_load_action import _on_page_load from vizro.actions._parameter_action import _parameter diff --git a/vizro-core/src/vizro/models/_dashboard.py b/vizro-core/src/vizro/models/_dashboard.py index 1cf23d638..f28b08f22 100644 --- a/vizro-core/src/vizro/models/_dashboard.py +++ b/vizro-core/src/vizro/models/_dashboard.py @@ -7,7 +7,6 @@ import dash import dash_bootstrap_components as dbc import dash_daq as daq -import plotly.io as pio from dash import ClientsideFunction, Input, Output, clientside_callback, get_relative_path, html from pydantic import Field, validator diff --git a/vizro-core/tests/unit/vizro/models/_components/test_graph.py b/vizro-core/tests/unit/vizro/models/_components/test_graph.py index e45609321..9694febe9 100644 --- a/vizro-core/tests/unit/vizro/models/_components/test_graph.py +++ b/vizro-core/tests/unit/vizro/models/_components/test_graph.py @@ -11,7 +11,6 @@ import vizro.plotly.express as px from vizro.managers import data_manager from vizro.models._action._action import Action -from vizro.models._components.graph import create_empty_fig @pytest.fixture @@ -44,7 +43,14 @@ def expected_graph(): return dcc.Loading( dcc.Graph( id="text_graph", - figure=create_empty_fig(""), + figure=go.Figure( + layout={ + "paper_bgcolor": "rgba(0,0,0,0)", + "plot_bgcolor": "rgba(0,0,0,0)", + "xaxis": {"visible": False}, + "yaxis": {"visible": False}, + } + ), config={ "autosizable": True, "frameMargins": 0, @@ -136,15 +142,8 @@ def test_process_figure_data_frame_df(self, standard_px_chart, gapminder): class TestBuild: - def test_create_empty_fig(self, expected_empty_chart): - result = create_empty_fig("NO DATA") - assert result == expected_empty_chart - def test_graph_build(self, standard_px_chart, expected_graph): - graph = vm.Graph( - id="text_graph", - figure=standard_px_chart, - ) + graph = vm.Graph(id="text_graph", figure=standard_px_chart) result = json.loads(json.dumps(graph.build(), cls=plotly.utils.PlotlyJSONEncoder)) expected = json.loads(json.dumps(expected_graph, cls=plotly.utils.PlotlyJSONEncoder))