diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 4ceb0e35..18f6acfe 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -18,7 +18,7 @@ import datetime import logging from collections import deque -from functools import partial, wraps +from functools import partial import inspect from threading import Thread, Lock import uuid @@ -27,9 +27,11 @@ Annotated, Any, Callable, + Concatenate, + Generic, Optional, - Literal, - Union, + ParamSpec, + TypeVar, overload, ) from weakref import WeakSet @@ -37,10 +39,9 @@ from fastapi import FastAPI, HTTPException, Request, Body, BackgroundTasks from pydantic import BaseModel, create_model -from labthings_fastapi.logs import add_thing_log_destination - +from .base_descriptor import BaseDescriptor +from .logs import add_thing_log_destination from .utilities import model_to_dict -from .thing_description._model import LinkElement from .invocations import InvocationModel, InvocationStatus, LogRecordModel from .dependencies.invocation import NonWarningInvocationID from .exceptions import ( @@ -55,13 +56,11 @@ EmptyInput, StrictEmptyInput, fastapi_dependency_params, - get_docstring, - get_summary, input_model_from_signature, return_type, ) from .thing_description import type_to_dataschema -from .thing_description._model import ActionAffordance, ActionOp, Form +from .thing_description._model import ActionAffordance, ActionOp, Form, LinkElement from .utilities import labthings_data @@ -622,7 +621,15 @@ def delete_invocation(id: uuid.UUID) -> None: """ -class ActionDescriptor: +ActionParams = ParamSpec("ActionParams") +ActionReturn = TypeVar("ActionReturn") +OwnerT = TypeVar("OwnerT", bound="Thing") + + +class ActionDescriptor( + BaseDescriptor[Callable[ActionParams, ActionReturn]], + Generic[ActionParams, ActionReturn, OwnerT], +): """Wrap actions to enable them to be run over HTTP. This class is responsible for generating the action description for @@ -640,7 +647,7 @@ class ActionDescriptor: def __init__( self, - func: Callable, + func: Callable[Concatenate[OwnerT, ActionParams], ActionReturn], response_timeout: float = 1, retention_time: float = 300, ) -> None: @@ -662,7 +669,12 @@ def __init__( :param retention_time: how long, in seconds, the action should be kept for after it has completed. """ + super().__init__() self.func = func + if func.__doc__ is not None: + # Use the docstring from the function, if there is one. + self.__doc__ = func.__doc__ + name = func.__name__ # this is checked in __set_name__ self.response_timeout = response_timeout self.retention_time = retention_time self.dependency_params = fastapi_dependency_params(func) @@ -673,63 +685,47 @@ def __init__( ) self.output_model = return_type(func) self.invocation_model = create_model( - f"{self.name}_invocation", + f"{name}_invocation", __base__=InvocationModel, input=(self.input_model, ...), output=(Optional[self.output_model], None), ) - self.invocation_model.__name__ = f"{self.name}_invocation" + self.invocation_model.__name__ = f"{name}_invocation" - @overload - def __get__(self, obj: Literal[None], type: type[Thing]) -> ActionDescriptor: # noqa: D105 - ... + def __set_name__(self, owner: type[Thing], name: str) -> None: + """Ensure the action name matches the function name. - @overload - def __get__(self, obj: Thing, type: type[Thing] | None = None) -> Callable: # noqa: D105 - ... + It's assumed in a few places that the function name and the + descriptor's name are the same. This should always be the case + if it's used as a decorator. + + :param owner: The class owning the descriptor. + :param name: The name of the descriptor in the class. + :raises ValueError: if the action name does not match the function name. + """ + super().__set_name__(owner, name) + if self.name != self.func.__name__: + raise ValueError( + f"Action name '{self.name}' does not match function name " + f"'{self.func.__name__}'", + ) - def __get__( - self, obj: Optional[Thing], type: Optional[type[Thing]] = None - ) -> Union[ActionDescriptor, Callable]: + def instance_get(self, obj: Thing) -> Callable[ActionParams, ActionReturn]: """Return the function, bound to an object as for a normal method. This currently doesn't validate the arguments, though it may do so in future. In its present form, this is equivalent to a regular Python method, i.e. all we do is supply the first argument, `self`. - If `obj` is None, the descriptor is returned, so we can get - the descriptor conveniently as an attribute of the class. - :param obj: the `.Thing` to which we are attached. This will be the first argument supplied to the function wrapped by this descriptor. - :param type: the class of the `.Thing` to which we are attached. - If the descriptor is accessed via the class it is returned - directly. - :return: the action function, bound to ``obj`` (when accessed - via an instance), or the descriptor (accessed via the class). + :return: the action function, bound to ``obj``. """ - if obj is None: - return self - # TODO: do we attempt dependency injection here? I think not. - # If we want dependency injection, we should be calling the action - # via some sort of client object. - return partial(self.func, obj) - - @property - def name(self) -> str: - """The name of the wrapped function.""" - return self.func.__name__ - - @property - def title(self) -> str: - """A human-readable title.""" - return get_summary(self.func) or self.name - - @property - def description(self) -> str | None: - """A description of the action.""" - return get_docstring(self.func, remove_summary=True) + # `obj` should be of type `OwnerT`, but `BaseDescriptor` currently + # isn't generic in the type of the owning Thing, so we can't express + # that here. + return partial(self.func, obj) # type: ignore[arg-type] def _observers_set(self, obj: Thing) -> WeakSet: """Return a set used to notify changes. @@ -926,27 +922,10 @@ def action_affordance( ) -def mark_action(func: Callable, **kwargs: Any) -> ActionDescriptor: - r"""Mark a method of a Thing as an Action. - - We replace the function with a descriptor that's a - subclass of `.ActionDescriptor` - - :param func: The function to be decorated. - :param \**kwargs: Additional keyword arguments are passed to the constructor - of `.ActionDescriptor`. - - :return: An `.ActionDescriptor` wrapping the method. - """ - - class ActionDescriptorSubclass(ActionDescriptor): - pass - - return ActionDescriptorSubclass(func, **kwargs) - - @overload -def action(func: Callable, **kwargs: Any) -> ActionDescriptor: ... +def action( + func: Callable[Concatenate[OwnerT, ActionParams], ActionReturn], **kwargs: Any +) -> ActionDescriptor[ActionParams, ActionReturn, OwnerT]: ... @overload @@ -954,22 +933,22 @@ def action( **kwargs: Any, ) -> Callable[ [ - Callable, + Callable[Concatenate[OwnerT, ActionParams], ActionReturn], ], - ActionDescriptor, + ActionDescriptor[ActionParams, ActionReturn, OwnerT], ]: ... -@wraps(mark_action) def action( - func: Optional[Callable] = None, **kwargs: Any + func: Callable[Concatenate[OwnerT, ActionParams], ActionReturn] | None = None, + **kwargs: Any, ) -> ( - ActionDescriptor + ActionDescriptor[ActionParams, ActionReturn, OwnerT] | Callable[ [ - Callable, + Callable[Concatenate[OwnerT, ActionParams], ActionReturn], ], - ActionDescriptor, + ActionDescriptor[ActionParams, ActionReturn, OwnerT], ] ): r"""Mark a method of a `.Thing` as a LabThings Action. @@ -996,6 +975,6 @@ def action( # return a partial object, which then calls the # wrapped function once. if func is not None: - return mark_action(func, **kwargs) + return ActionDescriptor(func, **kwargs) else: - return partial(mark_action, **kwargs) + return partial(ActionDescriptor, **kwargs) diff --git a/tests/test_actions.py b/tests/test_actions.py index b0b8f688..6994701c 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -206,3 +206,83 @@ def decorated( # Check we can make the thing and it has a valid TD example = create_thing_without_server(Example) example.validate_thing_description() + + +def test_action_docs(): + """Check that action documentation is included in the Thing Description. + + This test was added to check that the generated documentation is correct, + after some refactoring of `lt.action`. + + `name`, `title` and `description` attributes are now handled by `BaseDescriptor` + and are tested more extensively there - but it seemed worthwhile to have some + tests of them in the context of actions. + """ + + class DocThing(lt.Thing): + @lt.action + def documented_action(self) -> None: + """This is the action docstring.""" + pass + + @lt.action + def convert_type(self, a: int) -> float: + """Convert an integer to a float.""" + return float(a) + + @lt.action + def no_doc_action(self) -> None: + pass + + @lt.action + def long_docstring(self) -> None: + """Do something with a very long docstring. + + It has multiple paragraphs. + + Here is the second paragraph. + + And here is the third. + """ + pass + + # Create a Thing, and generate the Thing Description. This uses `BaseDescriptor` + # functionality to extract the name, title, and description. + thing = create_thing_without_server(DocThing) + td = thing.thing_description() + actions = td.actions + # The various `assert is not None` statements are mostly for type + # checking/autocompletion while writing the tests. + assert actions is not None + + # The function docstring should propagate through as the description. + assert actions["documented_action"].description == "This is the action docstring." + + # It's important that we check more than one action, to ensure there is no + # "leakage" between instances. Previous implementations always subclassed + # `ActionDescriptor` to avoid leakage when methods were manipulated at runtime. + # This is no longer done, so we can instantiate `ActionDescriptor` directly - but + # it's good to make sure we can have multiple actions with different docstrings. + assert actions["convert_type"].description == "Convert an integer to a float." + + # convert_type also allows us to check that the action inputs and outputs are + # correctly represented in the thing description. + input = actions["convert_type"].input + assert input is not None + input_properties = input.properties + assert input_properties is not None + assert input_properties["a"].type.value == "integer" + output = actions["convert_type"].output + assert output is not None + assert output.type.value == "number" + + # An action with no docstring should have no description, and a default title. + assert actions["no_doc_action"].description is None + assert actions["no_doc_action"].title == "no_doc_action" + + # An action with a long docstring should have the docstring body as description, + # and the first line as title. + assert actions["long_docstring"].title == "Do something with a very long docstring." + assert actions["long_docstring"].description.startswith( + "It has multiple paragraphs." + ) diff --git a/typing_tests/README.md b/typing_tests/README.md index 109d79cc..53d09a67 100644 --- a/typing_tests/README.md +++ b/typing_tests/README.md @@ -1,11 +1,7 @@ # Typing tests: check `labthings_fastapi` plays nicely with `mypy`. -The codebase is type-checked with `mypy src/` and tested with `pytest`, however neither of these explicitly check that `mypy` can infer the correct types for `Thing` attributes like properties and actions. The Python files in this folder are intended to be checked using: +The codebase is type-checked with `mypy` and tested with `pytest`, however neither of these explicitly check that `mypy` can infer the correct types for `Thing` attributes like properties and actions. The Python files in this folder are checked when we run `mypy`, and are intended to test that `Thing` code may be statically type-checked. -```terminal -mypy --warn-unused-ignores typing_tests -``` - -The files include valid code that's accompanied by `assert_type` statements (which check the inferred types are what we expect them to be) as well as invalid code where the expected `mypy` errors are ignored. This tests for expected errors - if an expected error is not thrown, it will cause an `unused-ignore` error. +The files include valid code that's accompanied by `assert_type` statements (which check the inferred types are what we expect them to be) as well as invalid code where the expected `mypy` errors are ignored. This tests for expected errors - if an expected error is not thrown, it will cause an `unused-ignore` error as per the configuration in `pyproject.toml`. There are more elaborate type testing solutions available, but all of them add dependencies and require us to learn a new tool. This folder of "tests" feels like a reasonable way to test that the package plays well with static type checking, without too much added complication. \ No newline at end of file diff --git a/typing_tests/thing_actions.py b/typing_tests/thing_actions.py new file mode 100644 index 00000000..cb678273 --- /dev/null +++ b/typing_tests/thing_actions.py @@ -0,0 +1,94 @@ +"""Check actions are typed correctly. + +This module will be checked by `mypy` and is intended to ensure that methods +decorated as `lt.action` have the correct type signatures. +""" + +import labthings_fastapi as lt +from labthings_fastapi.actions import ActionDescriptor + +from typing_extensions import assert_type + +from labthings_fastapi.testing import create_thing_without_server + + +class ThingWithActions(lt.Thing): + """A Thing with various actions for testing.""" + + @lt.action + def no_args_no_return(self) -> None: + """An action with no arguments and no return value.""" + pass + + @lt.action + def with_args_no_return(self, x: int, y: str) -> None: + """An action with arguments and no return value.""" + pass + + @lt.action + def no_args_with_return(self) -> float: + """An action with no arguments and a return value.""" + return 3.14 + + @lt.action + def with_args_with_return(self, a: int, b: str) -> float: + """An action with arguments and a return value.""" + return 3.14 + + +# What we really care about is that the instance attributes are right. +# The lines below create an instance, then check each of the four actions +# has the correct input parameters and return type. +# Note that this test will fail if there are unused ignores, so the +# lines with ignore comments assert that errors are raised. +thing = create_thing_without_server(ThingWithActions) + +# Check the function returns the expected type, when called with the expected args +assert_type(thing.no_args_no_return(), None) +# Check that using arguments causes mypy to raise an error. +thing.no_args_no_return("arg") # type: ignore[call-arg] +thing.no_args_no_return(unexpected=123) # type: ignore[call-arg] + +# Check the return type is None, when called with the expected args +assert_type(thing.with_args_no_return(1, "test"), None) +# Check that missing arguments are caught by mypy +thing.with_args_no_return() # type: ignore[call-arg] +# Check that wrong argument types are caught by mypy +thing.with_args_no_return(1, 2) # type: ignore[arg-type] + +# Check the return type is correct when called without arguments +assert_type(thing.no_args_with_return(), float) +# Check that using arguments causes mypy to raise an error. +thing.with_args_no_return("unexpected") # type: ignore[arg-type, call-arg] +thing.with_args_no_return(unexpected=123) # type: ignore[call-arg] + +# Check the return type is correct when called with the expected args +assert_type(thing.with_args_with_return(10, "data"), float) +# Check that missing arguments are caught by mypy +thing.with_args_no_return() # type: ignore[call-arg] +# Check that wrong argument types are caught by mypy +thing.with_args_no_return(10, 20) # type: ignore[arg-type] + + +# We should also make sure the attribute is a correctly-typed descriptor +assert_type( + ThingWithActions.no_args_no_return, ActionDescriptor[[], None, ThingWithActions] +) +# assert_type doesn't work well with the ParamSpec for arguments, so we use an +# assignment instead to check the type is compatible. +with_args_no_return_descriptor: ActionDescriptor[[int, str], None, ThingWithActions] = ( + ThingWithActions.with_args_no_return +) +assert_type( + ThingWithActions.no_args_with_return, ActionDescriptor[[], float, ThingWithActions] +) +with_args_with_return_descriptor: ActionDescriptor[ + [int, str], float, ThingWithActions +] = ThingWithActions.with_args_with_return + +# Check the documentation-related properties are correctly typed +# There's no need to check all four actions here, as they should all be the same. +assert_type(ThingWithActions.with_args_with_return.__doc__, str | None) +assert_type(ThingWithActions.with_args_with_return.description, str | None) +assert_type(ThingWithActions.with_args_with_return.title, str) +assert_type(ThingWithActions.with_args_with_return.name, str) diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_properties.py similarity index 100% rename from typing_tests/thing_definitions.py rename to typing_tests/thing_properties.py