From e42d9e7b8cb9536b63abc095f69a92e6ea63ab8a Mon Sep 17 00:00:00 2001 From: huong-li-nguyen Date: Thu, 18 Jul 2024 15:12:24 +0200 Subject: [PATCH 01/14] Add validation for components --- vizro-core/examples/scratch_dev/app.py | 70 +++++-------------- .../src/vizro/models/_components/_form.py | 4 +- .../src/vizro/models/_components/container.py | 4 +- vizro-core/src/vizro/models/_models_utils.py | 20 ++++++ vizro-core/src/vizro/models/_page.py | 4 +- 5 files changed, 43 insertions(+), 59 deletions(-) diff --git a/vizro-core/examples/scratch_dev/app.py b/vizro-core/examples/scratch_dev/app.py index 49775d2be..3818b7c95 100644 --- a/vizro-core/examples/scratch_dev/app.py +++ b/vizro-core/examples/scratch_dev/app.py @@ -1,67 +1,31 @@ """Example app to show all features of Vizro.""" -# check out https://github.com/mckinsey/vizro for more info about Vizro -# and checkout https://vizro.readthedocs.io/en/stable/ for documentation - +import pandas as pd import vizro.models as vm -import vizro.plotly.express as px from vizro import Vizro +from vizro.figures import kpi_card + +# data from the demo app +df_kpi = pd.DataFrame( + { + "Actual": [100, 200, 700], + "Reference": [100, 300, 500], + "Category": ["A", "B", "C"], + } +) -df = px.data.iris() -page = vm.Page( - title="Vizro on PyCafe", - layout=vm.Layout( - grid=[[0, 0, 0, 1, 2, 3], [4, 4, 4, 4, 4, 4], [4, 4, 4, 4, 4, 4], [5, 5, 5, 5, 5, 5], [5, 5, 5, 5, 5, 5]], - row_min_height="175px", - ), +home = vm.Page( + title="value_error.discriminated_union.missing_discriminator", components=[ - vm.Card( - text=""" - ### What is Vizro? - - Vizro is a toolkit for creating modular data visualization applications. - """ - ), - vm.Card( - text=""" - ### Github - - Checkout Vizro's github page. - """, - href="https://github.com/mckinsey/vizro", - ), - vm.Card( - text=""" - ### Docs - - Visit the documentation for codes examples, tutorials and API reference. - """, - href="https://vizro.readthedocs.io/", - ), - vm.Card( - text=""" - ### Nav Link - - Click this for page 2. - """, - href="/page2", - ), - vm.Graph(id="scatter_chart", figure=px.scatter(df, x="sepal_length", y="petal_width", color="species")), - vm.Graph(id="hist_chart", figure=px.histogram(df, x="sepal_width", color="species")), - ], - controls=[ - vm.Filter(column="species", selector=vm.Dropdown(value=["ALL"])), - vm.Filter(column="petal_length"), - vm.Filter(column="sepal_width"), + # vm.Card(text="I am a Card"), + kpi_card(data_frame=df_kpi, value_column="Actual", title="KPI with value"), ], ) -page2 = vm.Page( - title="Page2", components=[vm.Graph(id="hist_chart2", figure=px.histogram(df, x="sepal_width", color="species"))] -) -dashboard = vm.Dashboard(pages=[page, page2]) +dashboard = vm.Dashboard(pages=[home]) + if __name__ == "__main__": Vizro().build(dashboard).run() diff --git a/vizro-core/src/vizro/models/_components/_form.py b/vizro-core/src/vizro/models/_components/_form.py index 248f66f80..63cb49136 100644 --- a/vizro-core/src/vizro/models/_components/_form.py +++ b/vizro-core/src/vizro/models/_components/_form.py @@ -12,7 +12,7 @@ from vizro.models import VizroBaseModel from vizro.models._components.form import Checklist, Dropdown, RadioItems, RangeSlider, Slider from vizro.models._layout import set_layout -from vizro.models._models_utils import _log_call, _validate_min_length +from vizro.models._models_utils import _log_call, validate_components from vizro.models.types import _FormComponentType if TYPE_CHECKING: @@ -34,7 +34,7 @@ class Form(VizroBaseModel): layout: Layout = None # type: ignore[assignment] # Re-used validators - _validate_components = validator("components", allow_reuse=True, always=True)(_validate_min_length) + _validate_components = validator("components", allow_reuse=True, always=True, pre=True)(validate_components) _validate_layout = validator("layout", allow_reuse=True, always=True)(set_layout) @_log_call diff --git a/vizro-core/src/vizro/models/_components/container.py b/vizro-core/src/vizro/models/_components/container.py index d42d62d8d..fee0ea3f0 100644 --- a/vizro-core/src/vizro/models/_components/container.py +++ b/vizro-core/src/vizro/models/_components/container.py @@ -11,7 +11,7 @@ from vizro.models import VizroBaseModel from vizro.models._layout import set_layout -from vizro.models._models_utils import _log_call, _validate_min_length +from vizro.models._models_utils import _log_call, validate_components from vizro.models.types import ComponentType if TYPE_CHECKING: @@ -36,7 +36,7 @@ class Container(VizroBaseModel): layout: Layout = None # type: ignore[assignment] # Re-used validators - _validate_components = validator("components", allow_reuse=True, always=True)(_validate_min_length) + _validate_components = validator("components", allow_reuse=True, always=True, pre=True)(validate_components) _validate_layout = validator("layout", allow_reuse=True, always=True)(set_layout) @_log_call diff --git a/vizro-core/src/vizro/models/_models_utils.py b/vizro-core/src/vizro/models/_models_utils.py index 842800d70..f3fd34360 100644 --- a/vizro-core/src/vizro/models/_models_utils.py +++ b/vizro-core/src/vizro/models/_models_utils.py @@ -1,6 +1,8 @@ import logging from functools import wraps +from vizro.models.types import CapturedCallable + logger = logging.getLogger(__name__) @@ -15,8 +17,26 @@ def _wrapper(self, *args, **kwargs): return _wrapper + # Validators for reuse def _validate_min_length(cls, field): if not field: raise ValueError("Ensure this value has at least 1 item.") + +def validate_components(cls, field): + _validate_min_length(cls, field) + + # Validate CapturedCallable has been directly provided. + mode_to_error = { + "figure": "A callable of mode `figure` has been provided. Please wrap it inside the `vm.Figure(figure=...)`.", + "table": "A callable of mode `table` has been provided. Please wrap it inside the `vm.Table(figure=...)`.", + "ag_grid": "A callable of mode `ag_grid` has been provided. Please wrap it inside the `vm.AgGrid(figure=...)`.", + "graph": "A callable of mode `graph` has been provided. Please wrap it inside the `vm.AgGrid(figure=...)`.", + } + + for component in field: + if isinstance(component, CapturedCallable): + error_message = mode_to_error.get(f"{component._mode}", None) + if error_message: + raise ValueError(error_message) return field diff --git a/vizro-core/src/vizro/models/_page.py b/vizro-core/src/vizro/models/_page.py index 558d672c7..067399786 100644 --- a/vizro-core/src/vizro/models/_page.py +++ b/vizro-core/src/vizro/models/_page.py @@ -16,7 +16,7 @@ from vizro.models import Action, Layout, VizroBaseModel from vizro.models._action._actions_chain import ActionsChain, Trigger from vizro.models._layout import set_layout -from vizro.models._models_utils import _log_call, _validate_min_length +from vizro.models._models_utils import _log_call, validate_components from .types import ComponentType, ControlType @@ -52,7 +52,7 @@ class Page(VizroBaseModel): actions: List[ActionsChain] = [] # Re-used validators - _validate_components = validator("components", allow_reuse=True, always=True)(_validate_min_length) + _validate_components = validator("components", allow_reuse=True, always=True, pre=True)(validate_components) _validate_layout = validator("layout", allow_reuse=True, always=True)(set_layout) @root_validator(pre=True) From 9f4481a48926d7029a32fe3230d9eb5ede06a562 Mon Sep 17 00:00:00 2001 From: huong-li-nguyen Date: Thu, 18 Jul 2024 15:28:16 +0200 Subject: [PATCH 02/14] Fix unit tests --- ...ong_li_nguyen_validation_error_captured.md | 48 +++++++++++++++++++ vizro-core/examples/scratch_dev/app.py | 5 +- .../src/vizro/models/_components/_form.py | 5 +- .../src/vizro/models/_components/container.py | 5 +- .../src/vizro/models/_components/tabs.py | 4 +- vizro-core/src/vizro/models/_models_utils.py | 10 ++-- vizro-core/src/vizro/models/_page.py | 5 +- .../unit/vizro/models/test_models_utils.py | 9 +++- 8 files changed, 73 insertions(+), 18 deletions(-) create mode 100644 vizro-core/changelog.d/20240718_151233_huong_li_nguyen_validation_error_captured.md diff --git a/vizro-core/changelog.d/20240718_151233_huong_li_nguyen_validation_error_captured.md b/vizro-core/changelog.d/20240718_151233_huong_li_nguyen_validation_error_captured.md new file mode 100644 index 000000000..a37ae04d3 --- /dev/null +++ b/vizro-core/changelog.d/20240718_151233_huong_li_nguyen_validation_error_captured.md @@ -0,0 +1,48 @@ + + + + + + + + +### Fixed + +- Improve validation error message if `CapturedCallable` is directly provided. ([#587](https://github.com/mckinsey/vizro/pull/587)) + + + diff --git a/vizro-core/examples/scratch_dev/app.py b/vizro-core/examples/scratch_dev/app.py index 3818b7c95..6790994e5 100644 --- a/vizro-core/examples/scratch_dev/app.py +++ b/vizro-core/examples/scratch_dev/app.py @@ -16,10 +16,9 @@ home = vm.Page( - title="value_error.discriminated_union.missing_discriminator", + title="Page Title", components=[ - # vm.Card(text="I am a Card"), - kpi_card(data_frame=df_kpi, value_column="Actual", title="KPI with value"), + vm.Figure(figure=kpi_card(data_frame=df_kpi, value_column="Actual", title="KPI with value")), ], ) diff --git a/vizro-core/src/vizro/models/_components/_form.py b/vizro-core/src/vizro/models/_components/_form.py index 63cb49136..90436b60d 100644 --- a/vizro-core/src/vizro/models/_components/_form.py +++ b/vizro-core/src/vizro/models/_components/_form.py @@ -12,7 +12,7 @@ from vizro.models import VizroBaseModel from vizro.models._components.form import Checklist, Dropdown, RadioItems, RangeSlider, Slider from vizro.models._layout import set_layout -from vizro.models._models_utils import _log_call, validate_components +from vizro.models._models_utils import _log_call, validate_components_type, validate_min_length from vizro.models.types import _FormComponentType if TYPE_CHECKING: @@ -34,7 +34,8 @@ class Form(VizroBaseModel): layout: Layout = None # type: ignore[assignment] # Re-used validators - _validate_components = validator("components", allow_reuse=True, always=True, pre=True)(validate_components) + _validate_components_type = validator("components", allow_reuse=True, always=True, pre=True)(validate_components_type) + _validate_components_length = validator("components", allow_reuse=True, always=True)(validate_min_length) _validate_layout = validator("layout", allow_reuse=True, always=True)(set_layout) @_log_call diff --git a/vizro-core/src/vizro/models/_components/container.py b/vizro-core/src/vizro/models/_components/container.py index fee0ea3f0..7cf1c0de1 100644 --- a/vizro-core/src/vizro/models/_components/container.py +++ b/vizro-core/src/vizro/models/_components/container.py @@ -11,7 +11,7 @@ from vizro.models import VizroBaseModel from vizro.models._layout import set_layout -from vizro.models._models_utils import _log_call, validate_components +from vizro.models._models_utils import _log_call, validate_components_type, validate_min_length from vizro.models.types import ComponentType if TYPE_CHECKING: @@ -36,7 +36,8 @@ class Container(VizroBaseModel): layout: Layout = None # type: ignore[assignment] # Re-used validators - _validate_components = validator("components", allow_reuse=True, always=True, pre=True)(validate_components) + _validate_components_type = validator("components", allow_reuse=True, always=True, pre=True)(validate_components_type) + _validate_components_length = validator("components", allow_reuse=True, always=True)(validate_min_length) _validate_layout = validator("layout", allow_reuse=True, always=True)(set_layout) @_log_call diff --git a/vizro-core/src/vizro/models/_components/tabs.py b/vizro-core/src/vizro/models/_components/tabs.py index e3c3a33f5..763e559cc 100644 --- a/vizro-core/src/vizro/models/_components/tabs.py +++ b/vizro-core/src/vizro/models/_components/tabs.py @@ -11,7 +11,7 @@ from pydantic import validator from vizro.models import VizroBaseModel -from vizro.models._models_utils import _log_call, _validate_min_length +from vizro.models._models_utils import _log_call, validate_min_length if TYPE_CHECKING: from vizro.models._components import Container @@ -29,7 +29,7 @@ class Tabs(VizroBaseModel): type: Literal["tabs"] = "tabs" tabs: List[Container] - _validate_tabs = validator("tabs", allow_reuse=True, always=True)(_validate_min_length) + _validate_tabs = validator("tabs", allow_reuse=True, always=True)(validate_min_length) @_log_call def build(self): diff --git a/vizro-core/src/vizro/models/_models_utils.py b/vizro-core/src/vizro/models/_models_utils.py index f3fd34360..ed299a2e6 100644 --- a/vizro-core/src/vizro/models/_models_utils.py +++ b/vizro-core/src/vizro/models/_models_utils.py @@ -17,21 +17,19 @@ def _wrapper(self, *args, **kwargs): return _wrapper - # Validators for reuse -def _validate_min_length(cls, field): +def validate_min_length(cls, field): if not field: raise ValueError("Ensure this value has at least 1 item.") + return field -def validate_components(cls, field): - _validate_min_length(cls, field) - # Validate CapturedCallable has been directly provided. +def validate_components_type(cls, field): mode_to_error = { "figure": "A callable of mode `figure` has been provided. Please wrap it inside the `vm.Figure(figure=...)`.", "table": "A callable of mode `table` has been provided. Please wrap it inside the `vm.Table(figure=...)`.", "ag_grid": "A callable of mode `ag_grid` has been provided. Please wrap it inside the `vm.AgGrid(figure=...)`.", - "graph": "A callable of mode `graph` has been provided. Please wrap it inside the `vm.AgGrid(figure=...)`.", + "graph": "A callable of mode `graph` has been provided. Please wrap it inside the `vm.Graph(figure=...)`.", } for component in field: diff --git a/vizro-core/src/vizro/models/_page.py b/vizro-core/src/vizro/models/_page.py index 067399786..f5d5fc3d4 100644 --- a/vizro-core/src/vizro/models/_page.py +++ b/vizro-core/src/vizro/models/_page.py @@ -16,7 +16,7 @@ from vizro.models import Action, Layout, VizroBaseModel from vizro.models._action._actions_chain import ActionsChain, Trigger from vizro.models._layout import set_layout -from vizro.models._models_utils import _log_call, validate_components +from vizro.models._models_utils import _log_call, validate_components_type, validate_min_length from .types import ComponentType, ControlType @@ -52,7 +52,8 @@ class Page(VizroBaseModel): actions: List[ActionsChain] = [] # Re-used validators - _validate_components = validator("components", allow_reuse=True, always=True, pre=True)(validate_components) + _validate_components_type = validator("components", allow_reuse=True, always=True, pre=True)(validate_components_type) + _validate_components_length = validator("components", allow_reuse=True, always=True)(validate_min_length) _validate_layout = validator("layout", allow_reuse=True, always=True)(set_layout) @root_validator(pre=True) diff --git a/vizro-core/tests/unit/vizro/models/test_models_utils.py b/vizro-core/tests/unit/vizro/models/test_models_utils.py index c3a1a312e..10edae78a 100644 --- a/vizro-core/tests/unit/vizro/models/test_models_utils.py +++ b/vizro-core/tests/unit/vizro/models/test_models_utils.py @@ -11,10 +11,17 @@ class TestSharedValidators: - def test_set_components_validator(self, model_with_layout): + def test_validate_min_length(self, model_with_layout): with pytest.raises(ValidationError, match="Ensure this value has at least 1 item."): model_with_layout(title="Title", components=[]) + def test_validate_components_type(self, model_with_layout, standard_px_chart): + with pytest.raises( + ValidationError, + match="A callable of mode `graph` has been provided. " "Please wrap it inside the `vm.Graph(figure=...)`.", + ): + model_with_layout(title="Title", components=[standard_px_chart]) + def test_check_for_valid_component_types(self, model_with_layout): with pytest.raises( ValidationError, From 9806db5ad4eb05f3a938c520730f8f7723bb0205 Mon Sep 17 00:00:00 2001 From: huong-li-nguyen Date: Thu, 18 Jul 2024 15:31:40 +0200 Subject: [PATCH 03/14] Rename validator --- vizro-core/src/vizro/models/_components/_form.py | 4 ++-- vizro-core/src/vizro/models/_components/container.py | 4 ++-- vizro-core/src/vizro/models/_models_utils.py | 2 +- vizro-core/src/vizro/models/_page.py | 4 ++-- vizro-core/tests/unit/vizro/models/test_models_utils.py | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/vizro-core/src/vizro/models/_components/_form.py b/vizro-core/src/vizro/models/_components/_form.py index 90436b60d..16bf1a596 100644 --- a/vizro-core/src/vizro/models/_components/_form.py +++ b/vizro-core/src/vizro/models/_components/_form.py @@ -12,7 +12,7 @@ from vizro.models import VizroBaseModel from vizro.models._components.form import Checklist, Dropdown, RadioItems, RangeSlider, Slider from vizro.models._layout import set_layout -from vizro.models._models_utils import _log_call, validate_components_type, validate_min_length +from vizro.models._models_utils import _log_call, check_captured_callable, validate_min_length from vizro.models.types import _FormComponentType if TYPE_CHECKING: @@ -34,7 +34,7 @@ class Form(VizroBaseModel): layout: Layout = None # type: ignore[assignment] # Re-used validators - _validate_components_type = validator("components", allow_reuse=True, always=True, pre=True)(validate_components_type) + _check_captured_callable = validator("components", allow_reuse=True, always=True, pre=True)(check_captured_callable) _validate_components_length = validator("components", allow_reuse=True, always=True)(validate_min_length) _validate_layout = validator("layout", allow_reuse=True, always=True)(set_layout) diff --git a/vizro-core/src/vizro/models/_components/container.py b/vizro-core/src/vizro/models/_components/container.py index 7cf1c0de1..2a9d82b56 100644 --- a/vizro-core/src/vizro/models/_components/container.py +++ b/vizro-core/src/vizro/models/_components/container.py @@ -11,7 +11,7 @@ from vizro.models import VizroBaseModel from vizro.models._layout import set_layout -from vizro.models._models_utils import _log_call, validate_components_type, validate_min_length +from vizro.models._models_utils import _log_call, check_captured_callable, validate_min_length from vizro.models.types import ComponentType if TYPE_CHECKING: @@ -36,7 +36,7 @@ class Container(VizroBaseModel): layout: Layout = None # type: ignore[assignment] # Re-used validators - _validate_components_type = validator("components", allow_reuse=True, always=True, pre=True)(validate_components_type) + _check_captured_callable = validator("components", allow_reuse=True, always=True, pre=True)(check_captured_callable) _validate_components_length = validator("components", allow_reuse=True, always=True)(validate_min_length) _validate_layout = validator("layout", allow_reuse=True, always=True)(set_layout) diff --git a/vizro-core/src/vizro/models/_models_utils.py b/vizro-core/src/vizro/models/_models_utils.py index ed299a2e6..7326148a7 100644 --- a/vizro-core/src/vizro/models/_models_utils.py +++ b/vizro-core/src/vizro/models/_models_utils.py @@ -24,7 +24,7 @@ def validate_min_length(cls, field): return field -def validate_components_type(cls, field): +def check_captured_callable(cls, field): mode_to_error = { "figure": "A callable of mode `figure` has been provided. Please wrap it inside the `vm.Figure(figure=...)`.", "table": "A callable of mode `table` has been provided. Please wrap it inside the `vm.Table(figure=...)`.", diff --git a/vizro-core/src/vizro/models/_page.py b/vizro-core/src/vizro/models/_page.py index f5d5fc3d4..736c84fa2 100644 --- a/vizro-core/src/vizro/models/_page.py +++ b/vizro-core/src/vizro/models/_page.py @@ -16,7 +16,7 @@ from vizro.models import Action, Layout, VizroBaseModel from vizro.models._action._actions_chain import ActionsChain, Trigger from vizro.models._layout import set_layout -from vizro.models._models_utils import _log_call, validate_components_type, validate_min_length +from vizro.models._models_utils import _log_call, check_captured_callable, validate_min_length from .types import ComponentType, ControlType @@ -52,7 +52,7 @@ class Page(VizroBaseModel): actions: List[ActionsChain] = [] # Re-used validators - _validate_components_type = validator("components", allow_reuse=True, always=True, pre=True)(validate_components_type) + _check_captured_callable = validator("components", allow_reuse=True, always=True, pre=True)(check_captured_callable) _validate_components_length = validator("components", allow_reuse=True, always=True)(validate_min_length) _validate_layout = validator("layout", allow_reuse=True, always=True)(set_layout) diff --git a/vizro-core/tests/unit/vizro/models/test_models_utils.py b/vizro-core/tests/unit/vizro/models/test_models_utils.py index 10edae78a..238cab404 100644 --- a/vizro-core/tests/unit/vizro/models/test_models_utils.py +++ b/vizro-core/tests/unit/vizro/models/test_models_utils.py @@ -15,7 +15,7 @@ def test_validate_min_length(self, model_with_layout): with pytest.raises(ValidationError, match="Ensure this value has at least 1 item."): model_with_layout(title="Title", components=[]) - def test_validate_components_type(self, model_with_layout, standard_px_chart): + def test_check_captured_callable(self, model_with_layout, standard_px_chart): with pytest.raises( ValidationError, match="A callable of mode `graph` has been provided. " "Please wrap it inside the `vm.Graph(figure=...)`.", From d99def713da89481b1a0a9f7a88b68bf577160e0 Mon Sep 17 00:00:00 2001 From: huong-li-nguyen Date: Thu, 18 Jul 2024 15:33:07 +0200 Subject: [PATCH 04/14] Lint --- .../20240718_151233_huong_li_nguyen_validation_error_captured.md | 1 - 1 file changed, 1 deletion(-) diff --git a/vizro-core/changelog.d/20240718_151233_huong_li_nguyen_validation_error_captured.md b/vizro-core/changelog.d/20240718_151233_huong_li_nguyen_validation_error_captured.md index a37ae04d3..e32b5397a 100644 --- a/vizro-core/changelog.d/20240718_151233_huong_li_nguyen_validation_error_captured.md +++ b/vizro-core/changelog.d/20240718_151233_huong_li_nguyen_validation_error_captured.md @@ -39,7 +39,6 @@ Uncomment the section that is right (remove the HTML comment wrapper). - Improve validation error message if `CapturedCallable` is directly provided. ([#587](https://github.com/mckinsey/vizro/pull/587)) - - - +