From 0cbe19f52055ad52e9db161e127deafcd321c3e4 Mon Sep 17 00:00:00 2001 From: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com> Date: Fri, 6 Oct 2023 17:55:06 +0200 Subject: [PATCH] Introduce validator to raise ModuleNotFoundError at dashboard creation (#97) Signed-off-by: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com> Co-authored-by: Antony Milne <49395058+antonymilne@users.noreply.github.com> --- ..._openpyxl_library_causes_console_errors.md | 41 +++++++++++++++++++ .../src/vizro/actions/export_data_action.py | 3 +- .../src/vizro/models/_action/_action.py | 21 +++++++++- vizro-core/src/vizro/models/types.py | 4 +- .../vizro/actions/test_export_data_action.py | 25 ----------- 5 files changed, 64 insertions(+), 30 deletions(-) create mode 100644 vizro-core/changelog.d/20231005_145819_maximilian_schulz_86_absence_of_openpyxl_library_causes_console_errors.md diff --git a/vizro-core/changelog.d/20231005_145819_maximilian_schulz_86_absence_of_openpyxl_library_causes_console_errors.md b/vizro-core/changelog.d/20231005_145819_maximilian_schulz_86_absence_of_openpyxl_library_causes_console_errors.md new file mode 100644 index 000000000..82cc7741a --- /dev/null +++ b/vizro-core/changelog.d/20231005_145819_maximilian_schulz_86_absence_of_openpyxl_library_causes_console_errors.md @@ -0,0 +1,41 @@ + + + + +### Added + +- Raise `ModuleNotFoundError` in case the `export_data` action is used with `file_format="xlsx"` and neither `openpyxl` nor `xlsxwriter` are installed ([#97](https://github.com/mckinsey/vizro/pull/97)) + + + + + diff --git a/vizro-core/src/vizro/actions/export_data_action.py b/vizro-core/src/vizro/actions/export_data_action.py index 5fcd57962..3c75df63a 100644 --- a/vizro-core/src/vizro/actions/export_data_action.py +++ b/vizro-core/src/vizro/actions/export_data_action.py @@ -55,8 +55,7 @@ def export_data( writer = data_frames[target_id].to_csv elif file_format == "xlsx": writer = data_frames[target_id].to_excel - else: - raise ValueError(f'Unknown "file_format": {file_format}.' f' Known file formats: "csv", "xlsx".') + # Invalid file_format should be caught by Action validation callback_outputs[f"download-dataframe_{target_id}"] = dcc.send_data_frame( writer=writer, filename=f"{target_id}.{file_format}", index=False diff --git a/vizro-core/src/vizro/models/_action/_action.py b/vizro-core/src/vizro/models/_action/_action.py index 63f7e01a0..d5de2de6a 100644 --- a/vizro-core/src/vizro/models/_action/_action.py +++ b/vizro-core/src/vizro/models/_action/_action.py @@ -1,8 +1,9 @@ +import importlib.util import logging from typing import Any, Dict, List from dash import Input, Output, State, callback, ctx -from pydantic import Field +from pydantic import Field, validator import vizro.actions from vizro.models import VizroBaseModel @@ -35,6 +36,24 @@ class Action(VizroBaseModel): regex="^[a-zA-Z0-9_]+[.][a-zA-Z_]+$", ) + # TODO: Problem: generic Action model shouldn't depend on details of particular actions like export_data. + # Possible solutions: make a generic mapping of action functions to validation functions or the imports they + # require, and make the code here look up the appropriate validation using the function as key + # This could then also involve other validations currently only carried out at run-time in pre-defined actions, such + # as e.g. checking if the correct arguments have been provided to the file_format in export_data. + @validator("function") + def validate_predefined_actions(cls, function): + if function._function.__name__ == "export_data": + file_format = function._arguments.get("file_format") + if file_format not in [None, "csv", "xlsx"]: + raise ValueError(f'Unknown "file_format": {file_format}.' f' Known file formats: "csv", "xlsx".') + if file_format == "xlsx": + if importlib.util.find_spec("openpyxl") is None and importlib.util.find_spec("xlsxwriter") is None: + raise ModuleNotFoundError( + "You must install either openpyxl or xlsxwriter to export to xlsx format." + ) + return function + @_log_call def build(self): from vizro.actions._callback_mapping._get_action_callback_mapping import _get_action_callback_mapping diff --git a/vizro-core/src/vizro/models/types.py b/vizro-core/src/vizro/models/types.py index 6ba038e05..f0d64f7e4 100644 --- a/vizro-core/src/vizro/models/types.py +++ b/vizro-core/src/vizro/models/types.py @@ -68,8 +68,8 @@ def __delitem__(self, arg_name: str): @property def _arguments(self): - # TODO: This is only used once in _get_parametrized_config and should be removed when that reference is - # removed. + # TODO: This is used twice: in _get_parametrized_config and in vm.Action and should be removed when those + # references are removed. return self.__bound_arguments.arguments @classmethod diff --git a/vizro-core/tests/unit/vizro/actions/test_export_data_action.py b/vizro-core/tests/unit/vizro/actions/test_export_data_action.py index b446ecac4..1ca11bd2b 100644 --- a/vizro-core/tests/unit/vizro/actions/test_export_data_action.py +++ b/vizro-core/tests/unit/vizro/actions/test_export_data_action.py @@ -215,28 +215,3 @@ def test_multiple_targets_with_filter_and_filter_interaction( assert result["download-dataframe_box_chart"]["filename"] == "box_chart.csv" assert result["download-dataframe_box_chart"]["content"] == target_box_filtered_pop.to_csv(index=False) - - @pytest.mark.usefixtures("managers_one_page_two_graphs_one_button") - @pytest.mark.parametrize( - "callback_context_export_data", [(["scatter_chart", "box_chart"], None, None)], indirect=True - ) - def test_invalid_file_format( - self, - callback_context_export_data, - ): - # Add action to relevant component - model_manager["button"].actions = [ - vm.Action( - id="test_action", - function=export_data( - targets=["scatter_chart", "box_chart"], - file_format="invalid_file_format", - ), - ) - ] - - with pytest.raises( - ValueError, match='Unknown "file_format": invalid_file_format.' ' Known file formats: "csv", "xlsx".' - ): - # Run action by picking the above added action function and executing it with () - model_manager["test_action"].function()