From 1cfa2470556397c73fa7c3e0244f6e122b6db2df Mon Sep 17 00:00:00 2001 From: Antony Milne <49395058+antonymilne@users.noreply.github.com> Date: Wed, 25 Oct 2023 09:52:03 +0100 Subject: [PATCH 1/2] Fix CapturedCallable to work better with kwargs (#121) --- ...4_101744_antony.milne_captured_callable.md | 41 +++++ vizro-core/src/vizro/models/types.py | 70 +++++--- .../tests/unit/vizro/models/test_types.py | 158 +++++++++--------- 3 files changed, 170 insertions(+), 99 deletions(-) create mode 100644 vizro-core/changelog.d/20231024_101744_antony.milne_captured_callable.md diff --git a/vizro-core/changelog.d/20231024_101744_antony.milne_captured_callable.md b/vizro-core/changelog.d/20231024_101744_antony.milne_captured_callable.md new file mode 100644 index 000000000..d87e93c4c --- /dev/null +++ b/vizro-core/changelog.d/20231024_101744_antony.milne_captured_callable.md @@ -0,0 +1,41 @@ + + + + + + + +### Fixed + +- `CapturedCallable` now handles variadic keywords arguments (`**kwargs`) correctly ([#121](https://github.com/mckinsey/vizro/pull/121)) + + diff --git a/vizro-core/src/vizro/models/types.py b/vizro-core/src/vizro/models/types.py index f0d64f7e4..ba4ae9eb9 100644 --- a/vizro-core/src/vizro/models/types.py +++ b/vizro-core/src/vizro/models/types.py @@ -4,7 +4,6 @@ import functools import inspect -from copy import deepcopy from typing import Any, Dict, List, Literal, Protocol, Union, runtime_checkable from pydantic import Field, StrictBool @@ -40,37 +39,72 @@ def __init__(self, function, /, *args, **kwargs): """Creates a new CapturedCallable object that will be able to re-run `function`. Partially binds *args and **kwargs to the function call. + + Raises: + ValueError if `function` contains positional-only or variadic positional parameters (*args). """ + # It is difficult to get positional-only and variadic positional arguments working at the same time as + # variadic keyword arguments. Ideally we would do the __call__ as + # self.__function(*bound_arguments.args, **bound_arguments.kwargs) as in the + # Python documentation. This would handle positional-only and variadic positional arguments better but makes + # it more difficult to handle variadic keyword arguments due to https://bugs.python.org/issue41745. + # Hence we abandon bound_arguments.args and bound_arguments.kwargs in favor of just using + # self.__function(**bound_arguments.arguments). + parameters = inspect.signature(function).parameters + invalid_params = { + param.name + for param in parameters.values() + if param.kind in [inspect.Parameter.POSITIONAL_ONLY, inspect.Parameter.VAR_POSITIONAL] + } + + if invalid_params: + raise ValueError( + f"Invalid parameter {', '.join(invalid_params)}. CapturedCallable does not accept functions with " + f"positional-only or variadic positional parameters (*args)." + ) + self.__function = function - self.__bound_arguments = inspect.signature(function).bind_partial(*args, **kwargs) + self.__arguments = inspect.signature(function).bind_partial(*args, **kwargs).arguments + + # A function can only ever have one variadic keyword parameter. {""} is just here so that var_keyword_param + # is always unpacking a one element set. + (var_keyword_param,) = { + param.name for param in parameters.values() if param.kind == inspect.Parameter.VAR_KEYWORD + } or {""} + + # Since we do __call__ as self.__function(**bound_arguments.arguments), we need to restructure the arguments + # a bit to put the kwargs in the right place. + # For a function with parameter **kwargs this converts self.__arguments = {"kwargs": {"a": 1}} into + # self.__arguments = {"a": 1}. + if var_keyword_param in self.__arguments: + self.__arguments.update(self.__arguments[var_keyword_param]) + del self.__arguments[var_keyword_param] def __call__(self, **kwargs): """Run the `function` with the initial arguments overridden by **kwargs. Note *args are not possible here, but you can still override positional arguments using argument name. """ - if not kwargs: - return self.__function(*self.__bound_arguments.args, **self.__bound_arguments.kwargs) - - bound_arguments = deepcopy(self.__bound_arguments) - bound_arguments.arguments.update(kwargs) - # This looks like it should be self.__function(*bound_arguments.args, **bound_arguments.kwargs) as in the - # Python documentation, but that leads to problems due to https://bugs.python.org/issue41745. - return self.__function(**bound_arguments.arguments) + return self.__function(**{**self.__arguments, **kwargs}) def __getitem__(self, arg_name: str): """Gets the value of a bound argument.""" - return self.__bound_arguments.arguments[arg_name] + return self.__arguments[arg_name] def __delitem__(self, arg_name: str): """Deletes a bound argument.""" - del self.__bound_arguments.arguments[arg_name] + del self.__arguments[arg_name] @property def _arguments(self): # 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 + return self.__arguments + + # TODO-actions: Find a way how to compare CapturedCallable and function + @property + def _function(self): + return self.__function @classmethod def __get_validators__(cls): @@ -137,11 +171,6 @@ def _parse_json( else: raise ValueError(f"_target_={function_name} must be wrapped in the @capture decorator.") - # TODO-actions: Find a way how to compare CapturedCallable and function - @property - def _function(self): - return self.__function - class capture: """Captures a function call to create a [`CapturedCallable`][vizro.models.types.CapturedCallable]. @@ -175,6 +204,8 @@ def __call__(self, func, /): # The more difficult case, where we need to still have a valid plotly figure that renders in a notebook. # Hence we attach the CapturedCallable as a property instead of returning it directly. # TODO: move point of checking that data_frame argument exists earlier on. + # TODO: also would be nice to raise errors in CapturedCallable.__init__ at point of function definition + # rather than point of calling if possible. @functools.wraps(func) def wrapped(*args, **kwargs) -> _DashboardReadyFigure: if "data_frame" not in inspect.signature(func).parameters: @@ -278,7 +309,8 @@ class OptionsDictType(TypedDict): NavigationPagesType = Annotated[ Union[List[str], Dict[str, List[str]]], Field( - None, description="List of Page IDs or dict mapping of Page IDs and titles (for hierarchical sub-navigation)" + None, + description="List of Page IDs or dict mapping of Page IDs and titles (for hierarchical sub-navigation)", ), ] """Permissible value types for page attribute. Values are displayed as default.""" diff --git a/vizro-core/tests/unit/vizro/models/test_types.py b/vizro-core/tests/unit/vizro/models/test_types.py index 455c0e9ed..3315d2654 100644 --- a/vizro-core/tests/unit/vizro/models/test_types.py +++ b/vizro-core/tests/unit/vizro/models/test_types.py @@ -8,113 +8,111 @@ from vizro.models.types import CapturedCallable, capture -@pytest.fixture -def varargs_function(): - def function(*args, b=2): - return args[0] + b - - return CapturedCallable(function, 1) - +def positional_only_function(a, /): + pass -@pytest.mark.xfail -# Known bug: *args doesn't work properly. Fix while keeping the more important test_varkwargs -# passing due to https://bugs.python.org/issue41745. -# Error raised is IndexError: tuple index out of range -def test_varargs(varargs_function): - assert varargs_function(b=2) == 1 + 2 +def var_positional_function(*args): + pass -@pytest.fixture -def positional_only_function(): - def function(a, /, b): - return a + b - - return CapturedCallable(function, 1) +@pytest.mark.parametrize("function", [positional_only_function, var_positional_function]) +def test_invalid_parameter_kind(function): + with pytest.raises( + ValueError, + match="CapturedCallable does not accept functions with positional-only or variadic positional parameters", + ): + CapturedCallable(function) -@pytest.mark.xfail -# Known bug: position-only argument doesn't work properly. Fix while keeping the more important -# test_varkwargs passing due to https://bugs.python.org/issue41745. -# Error raised is TypeError: function got some positional-only arguments passed as keyword arguments: 'a' -def test_positional_only(positional_only_function): - assert positional_only_function(b=2) == 1 + 2 +def positional_or_keyword_function(a, b, c): + return a + b + c -@pytest.fixture -def keyword_only_function(): - def function(a, *, b): - return a + b - return CapturedCallable(function, 1) +def keyword_only_function(a, *, b, c): + return a + b + c -def test_keyword_only(keyword_only_function): - assert keyword_only_function(b=2) == 1 + 2 +def var_keyword_function(a, **kwargs): + return a + kwargs["b"] + kwargs["c"] @pytest.fixture -def varkwargs_function(): - def function(a, b=2, **kwargs): - return a + b + kwargs["c"] - - return CapturedCallable(function, 1) - - -def test_varkwargs(varkwargs_function): - varkwargs_function(c=3, d=4) == 1 + 2 + 3 - - -@pytest.fixture -def simple_function(): - def function(a, b, c, d=4): - return a + b + c + d - - return CapturedCallable(function, 1, b=2) +def captured_callable(request): + return CapturedCallable(request.param, 1, b=2) +@pytest.mark.parametrize( + "captured_callable", + [positional_or_keyword_function, keyword_only_function, var_keyword_function], + indirect=True, +) class TestCall: - def test_call_missing_argument(self, simple_function): - with pytest.raises(TypeError, match="missing 1 required positional argument"): - simple_function() - - def test_call_needs_keyword_arguments(self, simple_function): + def test_call_needs_keyword_arguments(self, captured_callable): with pytest.raises(TypeError, match="takes 1 positional argument but 2 were given"): - simple_function(2) - - def test_call_provide_required_argument(self, simple_function): - assert simple_function(c=3) == 1 + 2 + 3 + 4 + captured_callable(2) - def test_call_override_existing_arguments(self, simple_function): - assert simple_function(a=5, b=2, c=6) == 5 + 2 + 6 + 4 + def test_call_provide_required_argument(self, captured_callable): + assert captured_callable(c=3) == 1 + 2 + 3 - def test_call_is_memoryless(self, simple_function): - simple_function(c=3) - - with pytest.raises(TypeError, match="missing 1 required positional argument"): - simple_function() - - def test_call_unknown_argument(self, simple_function): - with pytest.raises(TypeError, match="got an unexpected keyword argument"): - simple_function(e=1) + def test_call_override_existing_arguments(self, captured_callable): + assert captured_callable(a=5, b=2, c=6) == 5 + 2 + 6 +@pytest.mark.parametrize( + "captured_callable", + [positional_or_keyword_function, keyword_only_function, var_keyword_function], + indirect=True, +) class TestDunderMethods: - def test_getitem_known_args(self, simple_function): - assert simple_function["a"] == 1 - assert simple_function["b"] == 2 - - def test_getitem_unknown_args(self, simple_function): - with pytest.raises(KeyError): - simple_function["c"] + def test_getitem_known_args(self, captured_callable): + assert captured_callable["a"] == 1 + assert captured_callable["b"] == 2 + def test_getitem_unknown_args(self, captured_callable): with pytest.raises(KeyError): - simple_function["d"] + captured_callable["c"] - def test_delitem(self, simple_function): - del simple_function["a"] + def test_delitem(self, captured_callable): + del captured_callable["a"] with pytest.raises(KeyError): - simple_function["a"] + captured_callable["a"] + + +@pytest.mark.parametrize( + "captured_callable, expectation", + [ + (positional_or_keyword_function, pytest.raises(TypeError, match="missing 1 required positional argument: 'c'")), + (keyword_only_function, pytest.raises(TypeError, match="missing 1 required keyword-only argument: 'c'")), + (var_keyword_function, pytest.raises(KeyError, match="'c'")), + ], + indirect=["captured_callable"], +) +class TestCallMissingArgument: + def test_call_missing_argument(self, captured_callable, expectation): + with expectation: + captured_callable() + + def test_call_is_memoryless(self, captured_callable, expectation): + captured_callable(c=3) + + with expectation: + captured_callable() + + +@pytest.mark.parametrize( + "captured_callable, expectation", + [ + (positional_or_keyword_function, pytest.raises(TypeError, match="got an unexpected keyword argument")), + (keyword_only_function, pytest.raises(TypeError, match="got an unexpected keyword argument")), + (var_keyword_function, pytest.raises(KeyError, match="'c'")), + ], + indirect=["captured_callable"], +) +def test_call_unknown_argument(captured_callable, expectation): + with expectation: + captured_callable(e=1) def undecorated_function(a, b, c, d=4): From bc3a57d7283e9feb812195b2666377fc1be8e4bd Mon Sep 17 00:00:00 2001 From: Li Nguyen <90609403+huong-li-nguyen@users.noreply.github.com> Date: Wed, 25 Oct 2023 13:52:55 +0200 Subject: [PATCH 2/2] Add disclaimer for custom created components and integrations (#126) --- ...guyen_add_disclaimer_for_custom_created.md | 42 +++++++++++++++++++ .../docs/pages/user_guides/custom_charts.md | 7 ++++ .../pages/user_guides/custom_components.md | 13 +++--- .../docs/pages/user_guides/integration.md | 8 ++++ 4 files changed, 65 insertions(+), 5 deletions(-) create mode 100644 vizro-core/changelog.d/20231025_125539_huong_li_nguyen_add_disclaimer_for_custom_created.md diff --git a/vizro-core/changelog.d/20231025_125539_huong_li_nguyen_add_disclaimer_for_custom_created.md b/vizro-core/changelog.d/20231025_125539_huong_li_nguyen_add_disclaimer_for_custom_created.md new file mode 100644 index 000000000..d57e34cc2 --- /dev/null +++ b/vizro-core/changelog.d/20231025_125539_huong_li_nguyen_add_disclaimer_for_custom_created.md @@ -0,0 +1,42 @@ + + + + + + + + diff --git a/vizro-core/docs/pages/user_guides/custom_charts.md b/vizro-core/docs/pages/user_guides/custom_charts.md index 337b6424b..0c8219be6 100644 --- a/vizro-core/docs/pages/user_guides/custom_charts.md +++ b/vizro-core/docs/pages/user_guides/custom_charts.md @@ -153,3 +153,10 @@ The below examples shows a more involved use-case. We create and style a waterfa [![Graph3]][Graph3] [Graph3]: ../../assets/user_guides/custom_charts/custom_chart_waterfall.png + + +???+ warning + + Please note that users of this package are responsible for the content of any custom-created component, + function or integration they write - especially with regard to leaking any sensitive information or exposing to + any security threat during implementation. diff --git a/vizro-core/docs/pages/user_guides/custom_components.md b/vizro-core/docs/pages/user_guides/custom_components.md index 315c19484..dc3fdc805 100644 --- a/vizro-core/docs/pages/user_guides/custom_components.md +++ b/vizro-core/docs/pages/user_guides/custom_components.md @@ -7,11 +7,6 @@ In general, you can create a custom component based on any dash-compatible compo [dash-bootstrap-components](https://dash-bootstrap-components.opensource.faculty.ai/), [dash-html-components](https://github.com/plotly/dash/tree/dev/components/dash-html-components), etc.). -!!!warning - When creating your own custom components, you are responsible for the security of your component (e.g. prevent setting HTML from code which might expose users to cross-site scripting). Vizro cannot guarantee - the security of custom created components, so make sure you keep this in mind when publicly deploying your dashboard. - - Vizro's public API is deliberately kept small in order to facilitate quick and easy configuration of a dashboard. However, at the same time, Vizro is easily extensible, so that you can tweak any component to your liking or even create entirely new ones. @@ -354,3 +349,11 @@ vm.Page.add_type("components", Jumbotron) [![CustomComponent2]][CustomComponent2] [CustomComponent2]: ../../assets/user_guides/custom_components/customcomponent_2.png + + + +???+ warning + + Please note that users of this package are responsible for the content of any custom-created component, + function or integration they write - especially with regard to leaking any sensitive information or exposing to + any security threat during implementation. diff --git a/vizro-core/docs/pages/user_guides/integration.md b/vizro-core/docs/pages/user_guides/integration.md index 202a5ba1c..c987f9091 100644 --- a/vizro-core/docs/pages/user_guides/integration.md +++ b/vizro-core/docs/pages/user_guides/integration.md @@ -45,3 +45,11 @@ register the datasets with [`kedro_datasets.pandas`](https://docs.kedro.org/en/s for dataset_name, dataset in kedro_integration.datasets_from_catalog(catalog).items(): data_manager[dataset_name] = dataset ``` + + + +???+ warning + + Please note that users of this package are responsible for the content of any custom-created component, + function or integration they write - especially with regard to leaking any sensitive information or exposing to + any security threat during implementation.