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

Fix flickering graphs #166

Merged
merged 10 commits into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ warn_untyped_fields = true
ignore = [
"D104" # undocumented-public-package
]
ignore-init-module-imports = true
line-length = 120
select = [
"E", # pycodestyle errors
Expand All @@ -70,7 +71,7 @@ known-first-party = ["vizro"]
# Tests can use magic values, assertions, and relative imports, ignore missing docstrings
"**/tests/**/*" = ["PLR2004", "S101", "TID252", "D100", "D101", "D102", "D103"]
# Ignore import violations in all __init__.py files
"__init__.py" = ["E402"]
"__init__.py" = ["E402", "F401"]

[tool.ruff.pydocstyle]
convention = "google"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<!--
A new scriv changelog fragment.

Uncomment the section that is right (remove the HTML comment wrapper).
-->

<!--
### Highlights ✨

- A bullet item for the Highlights ✨ 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))

-->
<!--
### Removed

- A bullet item for the Removed 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))

-->
<!--
### Added

- A bullet item for the Added 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))

-->
<!--
### Changed

- A bullet item for the Changed 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))

-->
<!--
### Deprecated

- 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))

<!--
### Security

- A bullet item for the Security 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))

-->
12 changes: 0 additions & 12 deletions vizro-core/src/vizro/actions/__init__.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -9,12 +6,3 @@

# Please keep alphabetically ordered
__all__ = ["export_data", "filter_interaction"]

# Actions lookup dictionary to facilitate function comparison
petar-qb marked this conversation as resolved.
Show resolved Hide resolved
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",
}
29 changes: 12 additions & 17 deletions vizro-core/src/vizro/models/_components/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,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.

Expand Down Expand Up @@ -68,14 +56,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,
Expand Down
31 changes: 7 additions & 24 deletions vizro-core/src/vizro/models/_dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -23,10 +22,6 @@
logger = logging.getLogger(__name__)


def update_theme(on: bool):
petar-qb marked this conversation as resolved.
Show resolved Hide resolved
return "vizro_dark" if on else "vizro_light"


class Dashboard(VizroBaseModel):
"""Vizro Dashboard to be used within [`Vizro`][vizro._vizro.Vizro.build].

Expand Down Expand Up @@ -60,13 +55,6 @@ def validate_navigation(cls, navigation, values):
return Navigation(pages=[page.id for page in values["pages"]])
return navigation

def __init__(self, *args, **kwargs):
petar-qb marked this conversation as resolved.
Show resolved Hide resolved
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.
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].
Expand All @@ -83,7 +71,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",
Expand All @@ -103,9 +96,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")
Expand All @@ -127,14 +118,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(
Expand Down
15 changes: 10 additions & 5 deletions vizro-core/src/vizro/models/_page.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,18 +136,23 @@ 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 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
if isinstance(component, Graph)
]
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
Expand Down
19 changes: 9 additions & 10 deletions vizro-core/tests/unit/vizro/models/_components/test_graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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))
Expand Down
7 changes: 0 additions & 7 deletions vizro-core/tests/unit/vizro/models/test_dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
Loading