From d36bee31bfad1e8d8ed19b37439e7ec35dfa120f Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Mon, 30 Oct 2023 11:32:52 -0700 Subject: [PATCH 1/3] Fix default width/height with implicit plot output Previously, default width/height value from render.plot (None) was passed to implicit plot output. With this fix, use MISSING as the default parameter value to make it clear that the default value is not set and shouldn't be passed through. --- shiny/render/_render.py | 19 ++++--- shiny/render/transformer/_transformer.py | 37 ++----------- tests/e2e/plot-sizing/test_plot_sizing.py | 13 ----- tests/pytest/test_plot_sizing.py | 64 +++++++++++++++++++++++ 4 files changed, 81 insertions(+), 52 deletions(-) create mode 100644 tests/pytest/test_plot_sizing.py diff --git a/shiny/render/_render.py b/shiny/render/_render.py index e3f908bc7..73f69b6a7 100644 --- a/shiny/render/_render.py +++ b/shiny/render/_render.py @@ -33,7 +33,7 @@ from .. import _utils from .. import ui as _ui from .._namespaces import ResolvedId -from ..types import ImgData +from ..types import MISSING, MISSING_TYPE, ImgData from ._try_render_plot import ( PlotSizeInfo, try_render_matplotlib, @@ -115,8 +115,8 @@ async def PlotTransformer( _fn: ValueFn[object], *, alt: Optional[str] = None, - width: Optional[float] = None, - height: Optional[float] = None, + width: Optional[float] | MISSING_TYPE = MISSING, + height: Optional[float] | MISSING_TYPE = MISSING, **kwargs: object, ) -> ImgData | None: is_userfn_async = is_async_callable(_fn) @@ -125,6 +125,11 @@ async def PlotTransformer( inputs = session.root_scope().input + if width is MISSING: + width = None + if height is MISSING: + height = None + # We don't have enough information at this point to decide what size the plot should # be. This is because the user's plotting code itself may express an opinion about # the plot size. We'll take the information we will need and stash it in @@ -218,8 +223,8 @@ def container_size(dimension: Literal["width", "height"]) -> float: def plot( *, alt: Optional[str] = None, - width: Optional[float] = None, - height: Optional[float] = None, + width: Optional[float] | MISSING_TYPE = MISSING, + height: Optional[float] | MISSING_TYPE = MISSING, **kwargs: Any, ) -> PlotTransformer.OutputRendererDecorator: ... @@ -234,8 +239,8 @@ def plot( _fn: PlotTransformer.ValueFn | None = None, *, alt: Optional[str] = None, - width: Optional[float] = None, - height: Optional[float] = None, + width: Optional[float] | MISSING_TYPE = MISSING, + height: Optional[float] | MISSING_TYPE = MISSING, **kwargs: Any, ) -> PlotTransformer.OutputRenderer | PlotTransformer.OutputRendererDecorator: """ diff --git a/shiny/render/transformer/_transformer.py b/shiny/render/transformer/_transformer.py index be736ec94..5efbf2e53 100644 --- a/shiny/render/transformer/_transformer.py +++ b/shiny/render/transformer/_transformer.py @@ -45,6 +45,7 @@ from ..._docstring import add_example from ..._typing_extensions import Concatenate, ParamSpec from ..._utils import is_async_callable, run_coro_sync +from ...types import MISSING # Input type for the user-spplied function that is passed to a render.xx IT = TypeVar("IT") @@ -353,10 +354,12 @@ def _render_default(self) -> TagList | Tag | MetadataNode | str: { k: v for k, v in self._params.kwargs.items() - if k in self.default_ui_passthrough_args + if k in self.default_ui_passthrough_args and v is not MISSING } ) - kwargs.update(self.default_ui_kwargs) + kwargs.update( + {k: v for k, v in self.default_ui_kwargs.items() if v is not MISSING} + ) return cast(DefaultUIFn, self.default_ui)( self.__name__, *self.default_ui_args, **kwargs ) @@ -800,33 +803,3 @@ async def resolve_value_fn(value_fn: ValueFn[IT]) -> IT: R = TypeVar("R") - - -def decorator_args_passthrough( - fn: Callable[P, R], passthrough: tuple[str, ...] -) -> Callable[Concatenate[dict[str, object], P], R]: - """ - Modifies a default_ui function so that it can receive certain kwargs that are - passed to the OutputRenderer. For example, @render.plot takes `width` and `height` - arguments that we'd like to pass through to `ui.output_plot`. We can do that by - calling `enrich_default_ui_output_fn(ui.output_plot, ("width", "height"))`. - - This works by returning a wrapped version of the function that has the same - signature, except, with an added `_params` first argument. The OutputRenderer base - class will look for this magic argument and pass it the params it was created with. - The wrapped function will use those to prepopulate the kwargs. - """ - - def inner(_params: dict[str, object], *args: P.args, **kwargs: P.kwargs): - # Filter down to just the params that we care about (`in passthrough`) and did - # not explicitly get overwritten by more explicit argument passing (`in kwargs`) - extra_args = { - k: v for k, v in _params.items() if k in passthrough and k not in kwargs - } - # Theoretically this is mutating a dict that possibly doesn't belong to us. In - # practice, we're the only ones to call default_ui, so it's fine. - kwargs.update(extra_args) - # Call the wrapped function and return the result - return fn(*args, **kwargs) - - return inner diff --git a/tests/e2e/plot-sizing/test_plot_sizing.py b/tests/e2e/plot-sizing/test_plot_sizing.py index eb9ad2962..aef615d6c 100644 --- a/tests/e2e/plot-sizing/test_plot_sizing.py +++ b/tests/e2e/plot-sizing/test_plot_sizing.py @@ -52,16 +52,3 @@ def test_output_image_kitchen(page: Page, local_app: ShinyAppProc) -> None: tolerance += 2 assert abs(rect["width"] - 300) <= tolerance assert abs(rect["height"] - 200) <= tolerance - - -def test_decorator_passthrough_size(): - """Make sure that render.plot width/height are passed through to implicit output""" - from shiny import render - - @render.plot(width=1280, height=960) - def foo(): - ... - - rendered = str(foo.tagify()) - assert "1280px" in rendered - assert "960px" in rendered diff --git a/tests/pytest/test_plot_sizing.py b/tests/pytest/test_plot_sizing.py new file mode 100644 index 000000000..4390f85f2 --- /dev/null +++ b/tests/pytest/test_plot_sizing.py @@ -0,0 +1,64 @@ +from shiny import render, ui +from shiny.express import output_args +from shiny.types import MISSING + + +def test_decorator_plot_sizing(): + """render.plot width/height are passed through to implicit output""" + + @render.plot(width=1280, height=960) + def foo(): + ... + + rendered = str(foo.tagify()) + assert "1280px" in rendered + assert "960px" in rendered + assert rendered == str(ui.output_plot("foo", width=1280, height=960)) + + +def test_decorator_plot_default(): + """render.plot default is the same as ui.output_plot default""" + + @render.plot() + def foo(): + ... + + rendered = str(foo.tagify()) + assert rendered == str(ui.output_plot("foo")) + + +def test_decorator_output_args(): + """@output_args is respected""" + + @output_args(width="640px", height="480px") + @render.plot() + def foo(): + ... + + rendered = str(foo.tagify()) + assert rendered == str(ui.output_plot("foo", width="640px", height="480px")) + + +def test_decorator_output_args_priority(): + """@output_args should override render.plot width/height""" + + @output_args(width="640px", height=480) + @render.plot(width=1280, height=960) + def foo(): + ... + + rendered = str(foo.tagify()) + # Note "640px" => 640 and 480 => "480px" + assert rendered == str(ui.output_plot("foo", width=640, height="480px")) + + +def test_decorator_output_args_MISSING(): + """Not saying we support this, but test how MISSING interacts""" + + @output_args(width=MISSING) + @render.plot(width=1280, height=MISSING) + def foo(): + ... + + rendered = str(foo.tagify()) + assert rendered == str(ui.output_plot("foo", width="1280px")) From dc1fb6a9e79e1b145528e17dbea7b6eb611a5485 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Mon, 30 Oct 2023 11:50:24 -0700 Subject: [PATCH 2/3] Fix pyright error --- shiny/render/_render.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/shiny/render/_render.py b/shiny/render/_render.py index 73f69b6a7..d72ae74f0 100644 --- a/shiny/render/_render.py +++ b/shiny/render/_render.py @@ -125,11 +125,6 @@ async def PlotTransformer( inputs = session.root_scope().input - if width is MISSING: - width = None - if height is MISSING: - height = None - # We don't have enough information at this point to decide what size the plot should # be. This is because the user's plotting code itself may express an opinion about # the plot size. We'll take the information we will need and stash it in @@ -147,12 +142,16 @@ def container_size(dimension: Literal["width", "height"]) -> float: result = inputs[ResolvedId(f".clientdata_output_{name}_{dimension}")]() return typing.cast(float, result) + non_missing_size = ( + cast(Union[float, None], width) if width is not MISSING else None, + cast(Union[float, None], height) if height is not MISSING else None, + ) plot_size_info = PlotSizeInfo( container_size_px_fn=( lambda: container_size("width"), lambda: container_size("height"), ), - user_specified_size_px=(width, height), + user_specified_size_px=non_missing_size, pixelratio=pixelratio, ) From 209cf1a385672888751c1eb2df7be093bc53d695 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Mon, 30 Oct 2023 12:34:29 -0700 Subject: [PATCH 3/3] Code review feedback --- shiny/render/_render.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/shiny/render/_render.py b/shiny/render/_render.py index d72ae74f0..cd782c168 100644 --- a/shiny/render/_render.py +++ b/shiny/render/_render.py @@ -115,8 +115,8 @@ async def PlotTransformer( _fn: ValueFn[object], *, alt: Optional[str] = None, - width: Optional[float] | MISSING_TYPE = MISSING, - height: Optional[float] | MISSING_TYPE = MISSING, + width: float | None | MISSING_TYPE = MISSING, + height: float | None | MISSING_TYPE = MISSING, **kwargs: object, ) -> ImgData | None: is_userfn_async = is_async_callable(_fn) @@ -222,8 +222,8 @@ def container_size(dimension: Literal["width", "height"]) -> float: def plot( *, alt: Optional[str] = None, - width: Optional[float] | MISSING_TYPE = MISSING, - height: Optional[float] | MISSING_TYPE = MISSING, + width: float | None | MISSING_TYPE = MISSING, + height: float | None | MISSING_TYPE = MISSING, **kwargs: Any, ) -> PlotTransformer.OutputRendererDecorator: ... @@ -238,8 +238,8 @@ def plot( _fn: PlotTransformer.ValueFn | None = None, *, alt: Optional[str] = None, - width: Optional[float] | MISSING_TYPE = MISSING, - height: Optional[float] | MISSING_TYPE = MISSING, + width: float | None | MISSING_TYPE = MISSING, + height: float | None | MISSING_TYPE = MISSING, **kwargs: Any, ) -> PlotTransformer.OutputRenderer | PlotTransformer.OutputRendererDecorator: """ @@ -251,15 +251,15 @@ def plot( Alternative text for the image if it cannot be displayed or viewed (i.e., the user uses a screen reader). width - Width of the plot in pixels. If ``None``, the width will be determined by the - size of the corresponding :func:`~shiny.ui.output_plot`. (You should not need to - use this argument in most Shiny apps--set the desired width on - :func:`~shiny.ui.output_plot` instead.) + Width of the plot in pixels. If ``None`` or ``MISSING``, the width will be + determined by the size of the corresponding :func:`~shiny.ui.output_plot`. (You + should not need to use this argument in most Shiny apps--set the desired width + on :func:`~shiny.ui.output_plot` instead.) height - Height of the plot in pixels. If ``None``, the height will be determined by the - size of the corresponding :func:`~shiny.ui.output_plot`. (You should not need to - use this argument in most Shiny apps--set the desired height on - :func:`~shiny.ui.output_plot` instead.) + Height of the plot in pixels. If ``None`` or ``MISSING``, the height will be + determined by the size of the corresponding :func:`~shiny.ui.output_plot`. (You + should not need to use this argument in most Shiny apps--set the desired height + on :func:`~shiny.ui.output_plot` instead.) **kwargs Additional keyword arguments passed to the relevant method for saving the image (e.g., for matplotlib, arguments to ``savefig()``; for PIL and plotnine,