diff --git a/docs/conf.py b/docs/conf.py index edfd712dd..0325064f3 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -104,6 +104,9 @@ ("docutils", "fastcs.demo.temperature_attr.TemperatureControllerSettings"), # TypeVar without docstrings still give warnings ("py:class", "strawberry.schema.schema.Schema"), + # A ParamSpec gets no target of its own, so `Command[P, T]` renders a + # reference to a bare `P` that resolves to nothing + ("py:class", "P"), ] nitpick_ignore_regex = [ ("py:class", r"fastcs.*.DType_T"), diff --git a/docs/explanations/transports.md b/docs/explanations/transports.md index 5a99c9ff5..e722f891d 100644 --- a/docs/explanations/transports.md +++ b/docs/explanations/transports.md @@ -11,6 +11,13 @@ A transport connects a `ControllerAPI` to an external protocol. The `ControllerA - Scan methods (`@scan`) - Sub-controller APIs (hierarchical structure) +A command may take arguments and return a value, and not every protocol can +carry such a call. A transport reads `command.signature` (or the +`argument_types`/`return_datatype`/`is_void` shortcuts) and decides for itself: +serve it, or set `command.enabled = False` and log a warning saying why, so the +rest of the controller is still served. See +[](../how-to/typed-commands.md) for what each transport does. + ## Implementing a Transport Subclass `Transport` and implement `connect()` and `serve()`: diff --git a/docs/how-to/typed-commands.md b/docs/how-to/typed-commands.md new file mode 100644 index 000000000..b85f34655 --- /dev/null +++ b/docs/how-to/typed-commands.md @@ -0,0 +1,101 @@ +# Give a Command Arguments and a Return Value + +A `@command` may take positional arguments and give a value back. Both are +declared the ordinary way - by annotating the method - and both are optional and +independent, so a command can take arguments and return nothing, return +something and take nothing, or do both. + +```python +from fastcs.controllers import Controller +from fastcs.methods import command + +class Stage(Controller): + @command() + async def stop(self) -> None: + """Void: no arguments, no return value.""" + await self._protocol.stop() + + @command() + async def move_to(self, position: float, wait: bool) -> None: + """Two positional arguments.""" + await self._protocol.move(position, wait) + + @command() + async def measure(self) -> float: + """A return value.""" + return await self._protocol.read_position() +``` + +## What a command may take and return + +Arguments and return values are `bool`, `int`, `float`, `str`, or an +`enum.Enum` subclass - the same python types an attribute holds, minus arrays +and tables. Everything must be annotated: a command's signature is what +transports read to decide how to expose it, so it has to be fully known. + +```python +@command() +async def move_to(self, position): # TypeError: no type annotation + ... + +@command() +async def plot(self, trace: list[float]): # TypeError: unsupported type + ... +``` + +Arguments are positional. Keyword-only arguments, `*args` and `**kwargs` are +rejected. + +An array-valued command has an attribute-shaped alternative: write the array to +an `AttrW` and trigger a void command, rather than passing it as an argument. + +## Which transports serve them + +Not every protocol can carry a typed call, so each transport declares what it +can do rather than the framework assuming they are all alike. A command a +transport cannot serve is **skipped with a warning at start-up** - the rest of +the controller is still served. + +| Transport | Void command | Arguments | Return value | +| --------- | ------------ | --------- | ------------ | +| REST | ✅ | ✅ any number, as a JSON body | ✅ as `{"value": …}` | +| GraphQL | ✅ | ✅ any number, as mutation arguments | ✅ the mutation result | +| Tango | ✅ | ⚠️ at most one, and not an enum | ⚠️ not an enum | +| EPICS CA | ✅ | ❌ | ❌ | +| EPICS PVA | ✅ | ❌ | ❌ | + +The EPICS transports serve a command as a single "do it" PV. There is no PV +representation of "call with these arguments and give me this back" that is not +already a set of attributes, so a typed command has nothing to map onto. Tango +commands carry at most one input value, which is a limit of the protocol. + +If a command must be reachable over EPICS, keep it void and put its arguments +and results on attributes: + +```python +class Stage(Controller): + target = AttrRW(float) + last_position = AttrR(float) + + @command() + async def move(self) -> None: + await self._protocol.move(self.target.setpoint) + await self.last_position.update(await self._protocol.read_position()) +``` + +## Reading a command's signature + +A transport - or anything else walking a `ControllerAPI` - gets the whole +picture from the command itself: + +```python +command = controller_api.command_methods["move_to"] + +command.signature # (position: float, wait: bool) -> None +command.argument_types # (float, bool) +command.return_datatype # None +command.is_void # False +``` + +`signature` is the bound signature, without `self`, so it is what a caller +would actually pass. diff --git a/src/fastcs/methods/__init__.py b/src/fastcs/methods/__init__.py index 0cdeb616a..2136bbc80 100644 --- a/src/fastcs/methods/__init__.py +++ b/src/fastcs/methods/__init__.py @@ -1,3 +1,4 @@ +from .command import COMMAND_DTYPES as COMMAND_DTYPES from .command import Command as Command from .command import CommandCallback as CommandCallback from .command import UnboundCommand as UnboundCommand diff --git a/src/fastcs/methods/command.py b/src/fastcs/methods/command.py index 6818d7137..9aec056c2 100644 --- a/src/fastcs/methods/command.py +++ b/src/fastcs/methods/command.py @@ -1,7 +1,10 @@ +import enum from collections.abc import Callable, Coroutine +from inspect import Parameter, Signature from types import MethodType -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any, Concatenate, Generic, ParamSpec, TypeVar +from fastcs.datatypes import DType from fastcs.logging import logger from fastcs.methods.method import Controller_T, Method @@ -9,37 +12,144 @@ from fastcs.controllers import BaseController # noqa: F401 -UnboundCommandCallback = Callable[[Controller_T], Coroutine[None, None, None]] +P = ParamSpec("P") +"""The parameters a `Command` takes""" +T = TypeVar("T") +"""The value a `Command` returns""" + +UnboundCommandCallback = Callable[ + Concatenate[Controller_T, P], Coroutine[None, None, T] +] """A Command callback that is unbound and must be called with a `Controller` instance""" -CommandCallback = Callable[[], Coroutine[None, None, None]] +CommandCallback = Callable[P, Coroutine[None, None, T]] """A Command callback that is bound and can be called without `self`""" -class Command(Method["BaseController"]): +COMMAND_DTYPES: tuple[type, ...] = (bool, int, float, str, enum.Enum) +"""The types a command argument or return value may have. + +A subset of ``DType``: arrays and tables are deliberately left out. Serving them +would mean duplicating the array serialisation each transport already has for +attributes rather than sharing it, which ADR 0015 explicitly does not want, and +an array-valued command has an attribute-shaped alternative today. +""" + + +def _validate_datatype(annotation: Any, what: str, fn: Callable) -> type[DType]: + if annotation is Signature.empty: + raise TypeError( + f"{what} of command {fn.__qualname__} has no type annotation. A " + "command's argument and return types must be fully known" + ) + + if not (isinstance(annotation, type) and issubclass(annotation, COMMAND_DTYPES)): + raise TypeError( + f"{what} of command {fn.__qualname__} has unsupported type " + f"{annotation!r}. Commands take and return " + f"{', '.join(t.__name__ for t in COMMAND_DTYPES)}" + ) + + return annotation + + +def _validate_arguments( + signature: Signature, fn: Callable, *, skip: int +) -> tuple[type[DType], ...]: + """Check a command's parameters and collect their types. + + Args: + signature: The command's signature + fn: The wrapped function, to name it in errors + skip: Leading parameters that are not command arguments - one for an + unbound method, which still declares ``self`` + + Returns: + The type of each argument, in order + + Raises: + TypeError: If a parameter is not a positional argument of a known type + + """ + parameters = list(signature.parameters.values())[skip:] + + argument_types = [] + for parameter in parameters: + if parameter.kind is Parameter.KEYWORD_ONLY: + raise TypeError( + f"Command {fn.__qualname__} has keyword-only argument " + f"'{parameter.name}'. Command arguments are positional; " + "keyword arguments are not supported yet" + ) + if parameter.kind in (Parameter.VAR_POSITIONAL, Parameter.VAR_KEYWORD): + raise TypeError( + f"Command {fn.__qualname__} takes *args or **kwargs. A " + "command's arguments must be fully known" + ) + + argument_types.append( + _validate_datatype(parameter.annotation, f"Argument '{parameter.name}'", fn) + ) + + return tuple(argument_types) + + +def _validate_return(signature: Signature, fn: Callable) -> type[DType] | None: + annotation = signature.return_annotation + if annotation in (None, Signature.empty): + return None + + return _validate_datatype(annotation, "Return value", fn) + + +class Command(Method["BaseController"], Generic[P, T]): """A `Controller` `Method` that performs a single action when called. + A command may take positional arguments and return a value, both of known + types - ``Command[[float], None]`` moves to a position, ``Command[[], None]`` + is the void case. What it takes and gives back is its ``signature``, which + is what a transport reads to decide how - or whether - to serve it. + This class contains a function that is bound to a specific `Controller` instance and is callable outside of the class context, without an explicit `self` parameter. Calling an instance of this class will call the bound `Controller` method. """ - def __init__(self, fn: CommandCallback, *, group: str | None = None): + def __init__(self, fn: CommandCallback[P, T], *, group: str | None = None): super().__init__(fn, group=group) - def _validate(self, fn: CommandCallback) -> None: + def _validate(self, fn: CommandCallback[P, T]) -> None: super()._validate(fn) - if not len(self.parameters) == 0: - raise TypeError(f"Command method cannot have arguments: {fn}") + self._argument_types = _validate_arguments(self.signature, fn, skip=0) + self._return_datatype = _validate_return(self.signature, fn) - async def __call__(self): - return await self.fn() + @property + def argument_types(self) -> tuple[type[DType], ...]: + """The type of each positional argument the command takes.""" + return self._argument_types @property - def fn(self) -> CommandCallback: - async def command(): + def return_datatype(self) -> type[DType] | None: + """The type the command returns, or ``None`` if it returns nothing.""" + return self._return_datatype + + @property + def is_void(self) -> bool: + """Whether the command takes no arguments and returns nothing. + + A void command can be served by any transport; a typed one needs a + protocol that can carry a typed call. + """ + return not self._argument_types and self._return_datatype is None + + async def __call__(self, *args: P.args, **kwargs: P.kwargs) -> T: + return await self.fn(*args, **kwargs) + + @property + def fn(self) -> CommandCallback[P, T]: + async def command(*args: P.args, **kwargs: P.kwargs) -> T: try: - return await self._fn() + return await self._fn(*args, **kwargs) except Exception: logger.exception("Command failed", fn=self._fn) raise @@ -47,7 +157,7 @@ async def command(): return command -class UnboundCommand(Method[Controller_T]): +class UnboundCommand(Method[Controller_T], Generic[Controller_T, P, T]): """A wrapper of an unbound `Controller` method to be bound into a `Command`. This generic class stores an unbound `Controller` method - effectively a function @@ -59,24 +169,33 @@ class UnboundCommand(Method[Controller_T]): """ def __init__( - self, fn: UnboundCommandCallback[Controller_T], *, group: str | None = None + self, + fn: UnboundCommandCallback[Controller_T, P, T], + *, + group: str | None = None, ) -> None: super().__init__(fn, group=group) - def _validate(self, fn: UnboundCommandCallback[Controller_T]) -> None: + def _validate(self, fn: UnboundCommandCallback[Controller_T, P, T]) -> None: super()._validate(fn) - if not len(self.parameters) == 1: - raise TypeError("Command method cannot have arguments") + if not self.parameters: + raise TypeError(f"Command {fn.__qualname__} must be a method, taking self") + + # The leading parameter is the ``Controller`` this is bound to, not an + # argument of the command. + _validate_arguments(self.signature, fn, skip=1) + _validate_return(self.signature, fn) - def bind(self, controller: Controller_T) -> Command: + def bind(self, controller: Controller_T) -> Command[P, T]: return Command(MethodType(self.fn, controller), group=self.group) def command( *, group: str | None = None ) -> Callable[ - [UnboundCommandCallback[Controller_T]], UnboundCommandCallback[Controller_T] + [UnboundCommandCallback[Controller_T, P, T]], + UnboundCommandCallback[Controller_T, P, T], ]: """Decorator to register a `Controller` method as a `Command` @@ -88,8 +207,8 @@ def command( """ def wrapper( - fn: UnboundCommandCallback[Controller_T], - ) -> UnboundCommandCallback[Controller_T]: + fn: UnboundCommandCallback[Controller_T, P, T], + ) -> UnboundCommandCallback[Controller_T, P, T]: setattr(fn, "__unbound_command__", UnboundCommand(fn, group=group)) # noqa: B010 return fn diff --git a/src/fastcs/methods/method.py b/src/fastcs/methods/method.py index f475256e3..f5ddcb570 100644 --- a/src/fastcs/methods/method.py +++ b/src/fastcs/methods/method.py @@ -1,12 +1,12 @@ from asyncio import iscoroutinefunction from collections.abc import Callable, Coroutine from inspect import Signature, getdoc, signature -from typing import Generic +from typing import Any, Generic from fastcs.tracer import Tracer from fastcs.util import Controller_T -MethodCallback = Callable[..., Coroutine[None, None, None]] +MethodCallback = Callable[..., Coroutine[None, None, Any]] """Generic protocol for all `Controller` Method callbacks""" @@ -17,10 +17,7 @@ def __init__(self, fn: MethodCallback, *, group: str | None = None) -> None: super().__init__() self._docstring = getdoc(fn) - - sig = signature(fn, eval_str=True) - self._parameters = sig.parameters - self._return_type = sig.return_annotation + self._signature = signature(fn, eval_str=True) self._validate(fn) self._fn = fn @@ -28,19 +25,45 @@ def __init__(self, fn: MethodCallback, *, group: str | None = None) -> None: self.enabled = True def _validate(self, fn: MethodCallback) -> None: - if self.return_type not in (None, Signature.empty): - raise TypeError("Method return type must be None or empty") - if not iscoroutinefunction(fn): raise TypeError("Method must be async function") + def _validate_takes_no_arguments(self, kind: str, expected: int) -> None: + """Reject a method that takes anything beyond its bound ``self``. + + Args: + kind: What the method is, to name it in the error + expected: How many parameters a no-argument method has here - one + for an unbound method, which still declares ``self`` + + Raises: + TypeError: If the method takes arguments + + """ + if len(self.parameters) != expected: + raise TypeError(f"{kind} method cannot have arguments") + + def _validate_returns_nothing(self, kind: str) -> None: + if self.return_type not in (None, Signature.empty): + raise TypeError(f"{kind} method return type must be None or empty") + + @property + def signature(self) -> Signature: + """The signature of the wrapped function. + + This is the public description of how to call the method, and what it + gives back - transports read it to decide how to expose the method, and + whether they can expose it at all. + """ + return self._signature + @property def return_type(self): - return self._return_type + return self._signature.return_annotation @property def parameters(self): - return self._parameters + return self._signature.parameters @property def docstring(self): diff --git a/src/fastcs/methods/scan.py b/src/fastcs/methods/scan.py index c995490a4..c160ca80f 100644 --- a/src/fastcs/methods/scan.py +++ b/src/fastcs/methods/scan.py @@ -41,8 +41,8 @@ def period(self): def _validate(self, fn: ScanCallback) -> None: super()._validate(fn) - if not len(self.parameters) == 0: - raise TypeError("Scan method cannot have arguments") + self._validate_takes_no_arguments("Scan", expected=0) + self._validate_returns_nothing("Scan") async def __call__(self): return await self._fn() @@ -82,8 +82,9 @@ def period(self): def _validate(self, fn: UnboundScanCallback[Controller_T]) -> None: super()._validate(fn) - if not len(self.parameters) == 1: - raise TypeError("Scan method cannot have arguments") + # The leading parameter is the ``Controller`` this is bound to. + self._validate_takes_no_arguments("Scan", expected=1) + self._validate_returns_nothing("Scan") def bind(self, controller: Controller_T) -> Scan: return Scan(MethodType(self.fn, controller), self._period) diff --git a/src/fastcs/transports/epics/ca/ioc.py b/src/fastcs/transports/epics/ca/ioc.py index 29a89d3a2..4be9245a0 100644 --- a/src/fastcs/transports/epics/ca/ioc.py +++ b/src/fastcs/transports/epics/ca/ioc.py @@ -257,6 +257,20 @@ def _create_and_link_command_pvs( pv_prefix = pv_prefix_from_path(controller_api.path) for attr_name, method in controller_api.command_methods.items(): + if not method.is_void: + # A PV is a value, not a call: there is no representation of + # "call with these arguments, get this back" that is not already + # a set of attributes. Skip rather than refuse to serve the + # controller at all (ADR 0015). + logger.warning( + "EPICS CA transport cannot serve a command that takes " + "arguments or returns a value", + command=attr_name, + signature=str(method.signature), + ) + method.enabled = False + continue + pv_name = snake_to_pascal(attr_name) alias = aliases.get(f"{pv_prefix}:{pv_name}", None) diff --git a/src/fastcs/transports/epics/gui.py b/src/fastcs/transports/epics/gui.py index 882a83a02..7b4b45896 100644 --- a/src/fastcs/transports/epics/gui.py +++ b/src/fastcs/transports/epics/gui.py @@ -163,6 +163,11 @@ def extract_api_components(self, controller_api: ControllerAPI) -> Tree: groups: dict[str, list[ComponentUnion]] = {} for attr_name, attribute in controller_api.attributes.items(): + if not attribute.enabled: + # The IOC is built before the GUI, so anything it could not + # serve has already said so - don't draw a control for it. + continue + try: signal = self._get_attribute_component( controller_api.path, @@ -189,6 +194,9 @@ def extract_api_components(self, controller_api: ControllerAPI) -> Tree: components.append(signal) for name, command in controller_api.command_methods.items(): + if not command.enabled: + continue + signal = self._get_command_component(controller_api.path, name) match command: diff --git a/src/fastcs/transports/epics/pva/ioc.py b/src/fastcs/transports/epics/pva/ioc.py index a3816fe77..4bfb370d6 100644 --- a/src/fastcs/transports/epics/pva/ioc.py +++ b/src/fastcs/transports/epics/pva/ioc.py @@ -4,6 +4,7 @@ from fastcs.attributes import AttrR, AttrRW, AttrW from fastcs.controllers import ControllerAPI +from fastcs.logging import logger from fastcs.transports.epics.util import pv_prefix_from_path from fastcs.util import snake_to_pascal @@ -40,6 +41,18 @@ def parse_attributes(root_controller_api: ControllerAPI) -> StaticProvider: provider.add(f"{full_pv_name}", attribute_pv) for attr_name, method in controller_api.command_methods.items(): + if not method.is_void: + # As for CA: PVA has no typed-call representation either, so a + # typed command is skipped with a warning (ADR 0015). + logger.warning( + "EPICS PVA transport cannot serve a command that takes " + "arguments or returns a value", + command=attr_name, + signature=str(method.signature), + ) + method.enabled = False + continue + full_pv_name = f"{pv_prefix}:{snake_to_pascal(attr_name)}" command_pv = make_command_pv(method.fn) provider.add(f"{full_pv_name}", command_pv) diff --git a/src/fastcs/transports/graphql/graphql.py b/src/fastcs/transports/graphql/graphql.py index 0c74ad27d..4ead1aa18 100644 --- a/src/fastcs/transports/graphql/graphql.py +++ b/src/fastcs/transports/graphql/graphql.py @@ -1,4 +1,5 @@ from collections.abc import Awaitable, Callable, Coroutine +from inspect import Parameter, Signature from typing import Any import strawberry @@ -12,6 +13,7 @@ from fastcs.datatypes.datatype import DType_T from fastcs.exceptions import FastCSError from fastcs.logging import intercept_std_logger +from fastcs.methods import Command from .options import GraphQLServerOptions @@ -112,7 +114,7 @@ def _process_attributes(self, api: ControllerAPI): def _process_commands(self, controller_api: ControllerAPI): """Create mutations from api commands""" for name, method in controller_api.command_methods.items(): - self.mutations.append(strawberry.mutation(_wrap_command(name, method.fn))) + self.mutations.append(strawberry.mutation(_wrap_command(name, method))) def _process_sub_apis(self, root_controller_api: ControllerAPI): """Recursively add fields from the queries and mutations of sub apis""" @@ -181,13 +183,35 @@ def _dynamic_field(): return strawberry.field(_dynamic_field) -def _wrap_command(method_name: str, method: Callable) -> Callable[..., Awaitable[bool]]: +def _wrap_command(method_name: str, command: Command) -> Callable[..., Awaitable[Any]]: """Wrap a command in a function with annotations for strawberry""" + argument_names = [ + parameter.name for parameter in command.signature.parameters.values() + ] + return_datatype = command.return_datatype + # A void command has no value to give back, so it reports that it ran. + return_annotation = bool if return_datatype is None else return_datatype - async def _dynamic_f() -> bool: - await method() - return True + async def _dynamic_f(**kwargs): + result = await command.fn(*(kwargs[name] for name in argument_names)) + return True if return_datatype is None else result _dynamic_f.__name__ = method_name + # Strawberry builds the mutation's arguments and result by introspecting the + # resolver, so the command's arguments have to show up in both the signature + # and the annotations of a function that does not literally declare them. + _dynamic_f.__signature__ = Signature( # type: ignore[attr-defined] + [ + Parameter(name, Parameter.POSITIONAL_OR_KEYWORD, annotation=argument_type) + for name, argument_type in zip( + argument_names, command.argument_types, strict=True + ) + ], + return_annotation=return_annotation, + ) + _dynamic_f.__annotations__ = dict( + zip(argument_names, command.argument_types, strict=True) + ) + _dynamic_f.__annotations__["return"] = return_annotation return _dynamic_f diff --git a/src/fastcs/transports/rest/rest.py b/src/fastcs/transports/rest/rest.py index a8ca359d8..4f54195c9 100644 --- a/src/fastcs/transports/rest/rest.py +++ b/src/fastcs/transports/rest/rest.py @@ -3,13 +3,14 @@ import uvicorn from fastapi import FastAPI -from pydantic import create_model +from pydantic import BaseModel, create_model from fastcs.attributes import AttrR, AttrRW, AttrW from fastcs.controllers import ControllerAPI from fastcs.datatypes.datatype import DType_T from fastcs.logging import intercept_std_logger -from fastcs.methods import CommandCallback +from fastcs.methods import Command +from fastcs.util import snake_to_pascal from .options import RestServerOptions from .util import ( @@ -142,13 +143,52 @@ def _add_attribute_api_routes(app: FastAPI, root_controller_api: ControllerAPI) ) +def _command_arguments_body(name: str, command: Command) -> type[BaseModel]: + """A pydantic model of a command's positional arguments, as a request body.""" + parameters = list(command.signature.parameters.values()) + # key=(type, ...) to declare a field without default value + fields: dict[str, Any] = { + parameter.name: (argument_type, ...) + for parameter, argument_type in zip( + parameters, command.argument_types, strict=True + ) + } + return create_model(f"Call{snake_to_pascal(name)}Arguments", **fields) + + +def _command_response_body(name: str, return_datatype: type) -> type[BaseModel]: + fields: dict[str, Any] = {"value": (return_datatype, ...)} + return create_model(f"Call{snake_to_pascal(name)}Result", **fields) + + def _wrap_command( - method: CommandCallback, -) -> Callable[..., Coroutine[None, None, None]]: - async def command() -> None: - await method() + name: str, command: Command +) -> Callable[..., Coroutine[None, None, dict[str, object] | None]]: + """Wrap a command in a route handler that carries its arguments and result.""" + argument_names = [ + parameter.name for parameter in command.signature.parameters.values() + ] + returns_a_value = command.return_datatype is not None + + if not argument_names: + + async def call() -> dict[str, object] | None: + result = await command.fn() + return {"value": result} if returns_a_value else None + + return call + + async def call_with_arguments(request) -> dict[str, object] | None: + arguments = [getattr(request, argument) for argument in argument_names] + result = await command.fn(*arguments) + return {"value": result} if returns_a_value else None + + # Fast api uses type annotations for validation, schema, conversions + call_with_arguments.__annotations__["request"] = _command_arguments_body( + name, command + ) - return command + return call_with_arguments def _add_command_api_routes(app: FastAPI, root_controller_api: ControllerAPI) -> None: @@ -157,10 +197,18 @@ def _add_command_api_routes(app: FastAPI, root_controller_api: ControllerAPI) -> for name, method in controller_api.command_methods.items(): cmd_name = name.replace("_", "-") - route = f"/{'/'.join(path)}/{cmd_name}" if path else cmd_name + route = f"{'/'.join(path)}/{cmd_name}" if path else cmd_name + return_datatype = method.return_datatype app.add_api_route( f"/{route}", - _wrap_command(method.fn), + _wrap_command(name, method), methods=["PUT"], - status_code=204, + # A command that gives something back has a body to return, so + # it answers 200 rather than 204 No Content. + status_code=200 if return_datatype is not None else 204, + response_model=( + _command_response_body(name, return_datatype) + if return_datatype is not None + else None + ), ) diff --git a/src/fastcs/transports/tango/dsr.py b/src/fastcs/transports/tango/dsr.py index 553fe3d92..80aae1d2f 100644 --- a/src/fastcs/transports/tango/dsr.py +++ b/src/fastcs/transports/tango/dsr.py @@ -8,7 +8,8 @@ from fastcs.attributes import AttrR, AttrRW, AttrW from fastcs.controllers import ControllerAPI -from fastcs.methods import CommandCallback +from fastcs.logging import logger +from fastcs.methods import Command from .options import TangoDSROptions from .util import ( @@ -108,19 +109,50 @@ def _collect_dev_attributes( return collection +# Tango commands carry at most one input value, so a command taking more than +# one argument has no faithful representation and is skipped (ADR 0015). +TANGO_MAX_COMMAND_ARGUMENTS = 1 + +TANGO_COMMAND_DTYPES: tuple[type, ...] = (bool, int, float, str) +"""The command argument and return types Tango can carry. + +An enum is left out: Tango has no command-level enum, and picking name-or-index +for it would be a guess a driver author cannot see or override. +""" + + +def _unservable_reason(command: Command) -> str | None: + """Why Tango cannot serve this command, or ``None`` if it can.""" + if len(command.argument_types) > TANGO_MAX_COMMAND_ARGUMENTS: + return "a Tango command takes at most one argument" + + unsupported = [ + datatype + for datatype in (*command.argument_types, command.return_datatype) + if datatype is not None and datatype not in TANGO_COMMAND_DTYPES + ] + if unsupported: + names = ", ".join(datatype.__name__ for datatype in unsupported) + return f"Tango commands do not carry {names}" + + return None + + def _wrap_command_f( method_name: str, - method: CommandCallback, + command: Command, controller_api: ControllerAPI, loop: asyncio.AbstractEventLoop, -) -> Callable[..., Awaitable[None]]: - async def _dynamic_f(tango_device: Device) -> None: +) -> Callable[..., Awaitable[Any]]: + takes_argument = bool(command.argument_types) + + async def _dynamic_f(tango_device: Device, *args) -> Any: tango_device.info_stream( f"called {'_'.join(controller_api.path)} f method: {method_name}" ) - coro = method() - await _run_threadsafe_blocking(coro, loop) + coro = command.fn(*args) if takes_argument else command.fn() + return await _run_threadsafe_blocking(coro, loop) _dynamic_f.__name__ = method_name return _dynamic_f @@ -136,10 +168,22 @@ def _collect_dev_commands( path = controller_api.path[root_depth:] for name, method in controller_api.command_methods.items(): + if (reason := _unservable_reason(method)) is not None: + logger.warning( + "Tango transport cannot serve this command", + command=name, + signature=str(method.signature), + reason=reason, + ) + method.enabled = False + continue + cmd_name = name.title().replace("_", "") d_cmd_name = f"{'_'.join(path)}_{cmd_name}" if path else cmd_name collection[d_cmd_name] = server.command( - f=_wrap_command_f(d_cmd_name, method.fn, controller_api, loop) + f=_wrap_command_f(d_cmd_name, method, controller_api, loop), + dtype_in=method.argument_types[0] if method.argument_types else None, + dtype_out=method.return_datatype, ) return collection diff --git a/tests/test_methods.py b/tests/test_methods.py index a3e990996..b0d9681a5 100644 --- a/tests/test_methods.py +++ b/tests/test_methods.py @@ -1,3 +1,5 @@ +from inspect import signature + import pytest from fastcs.controllers import Controller @@ -14,12 +16,6 @@ def sync_do_nothing(): with pytest.raises(TypeError): Method(sync_do_nothing) # type: ignore - async def do_nothing_with_return() -> int: - return 1 - - with pytest.raises(TypeError): - Method(do_nothing_with_return) # type: ignore - async def do_nothing(): """Do nothing.""" pass @@ -28,6 +24,21 @@ async def do_nothing(): assert method.docstring == "Do nothing." assert method.group == "Nothing" + assert method.signature == signature(do_nothing) + + +def test_a_scan_takes_no_arguments_and_returns_nothing(): + async def scan_with_return() -> int: + return 1 + + with pytest.raises(TypeError, match="Scan method return type must be None"): + Scan(scan_with_return, 1.0) # type: ignore + + async def scan_with_argument(arg: int): + pass + + with pytest.raises(TypeError, match="Scan method cannot have arguments"): + Scan(scan_with_argument, 1.0) # type: ignore @pytest.mark.asyncio @@ -75,3 +86,93 @@ async def update_nothing_with_arg(self, arg): assert scan.period == 1.0 await scan() + + +@pytest.mark.asyncio +async def test_a_command_can_take_arguments_and_return_a_value(): + class TestController(Controller): + async def move_to(self, position: float, wait: bool) -> str: + return f"moved to {position}, waited {wait}" + + command = UnboundCommand(TestController.move_to).bind(TestController()) + + assert command.argument_types == (float, bool) + assert command.return_datatype is str + assert not command.is_void + assert await command(1.5, True) == "moved to 1.5, waited True" + + +@pytest.mark.asyncio +async def test_a_void_command_says_so(): + class TestController(Controller): + async def stop(self): + pass + + command = UnboundCommand(TestController.stop).bind(TestController()) + + assert command.argument_types == () + assert command.return_datatype is None + assert command.is_void + + +def test_command_arguments_must_be_annotated(): + class TestController(Controller): + async def move_to(self, position): + pass + + with pytest.raises(TypeError, match="Argument 'position'.*has no type annotation"): + UnboundCommand(TestController.move_to) + + +def test_command_arguments_must_be_a_supported_type(): + class TestController(Controller): + async def move_to(self, position: list[float]): + pass + + with pytest.raises(TypeError, match="Argument 'position'.*unsupported type"): + UnboundCommand(TestController.move_to) + + +def test_command_return_must_be_a_supported_type(): + class TestController(Controller): + async def measure(self) -> list[float]: + return [] + + with pytest.raises(TypeError, match="Return value.*unsupported type"): + UnboundCommand(TestController.measure) + + +def test_command_arguments_are_positional(): + class TestController(Controller): + async def move_to(self, *, position: float): + pass + + with pytest.raises(TypeError, match="keyword-only argument 'position'"): + UnboundCommand(TestController.move_to) + + +def test_command_arguments_must_be_fully_known(): + class TestController(Controller): + async def move_to(self, *args: float): + pass + + with pytest.raises(TypeError, match=r"takes \*args or \*\*kwargs"): + UnboundCommand(TestController.move_to) + + +@pytest.mark.asyncio +async def test_command_arguments_survive_binding(): + """The signature a transport reads must be the bound one, without ``self``.""" + + class TestController(Controller): + seen: list[float] = [] + + async def move_to(self, position: float) -> None: + self.seen.append(position) + + controller = TestController() + command = UnboundCommand(TestController.move_to).bind(controller) + + assert list(command.signature.parameters) == ["position"] + await command(2.5) + assert controller.seen == [2.5] diff --git a/tests/test_typed_commands.py b/tests/test_typed_commands.py new file mode 100644 index 000000000..36e6b3b82 --- /dev/null +++ b/tests/test_typed_commands.py @@ -0,0 +1,227 @@ +"""Serving commands that take arguments and return values, per transport. + +Each transport declares what it can carry: REST and GraphQL round-trip a typed +call, Tango carries at most one argument, and the EPICS transports are void-only +and skip anything else with a warning rather than refusing to serve the +controller (ADR 0015). +""" + +import asyncio +import enum + +import pytest +from fastapi.testclient import TestClient + +from fastcs.attributes import AttrR +from fastcs.controllers import Controller, ControllerAPI +from fastcs.datatypes import Float +from fastcs.methods import command +from fastcs.transports.epics.ca.ioc import EpicsCAIOC +from fastcs.transports.epics.gui import EpicsGUI +from fastcs.transports.epics.pva.ioc import parse_attributes +from fastcs.transports.graphql.transport import GraphQLTransport +from fastcs.transports.rest.transport import RestTransport +from fastcs.transports.tango.dsr import _collect_dev_commands, _unservable_reason + + +class TypedCommandController(Controller): + """A controller with one command of each shape.""" + + calls: list[tuple] = [] + + # The GraphQL transport refuses an API with nothing to read + position = AttrR(Float()) + + @command() + async def stop(self) -> None: + self.calls.append(()) + + @command() + async def move_to(self, position: float, wait: bool) -> None: + self.calls.append((position, wait)) + + @command() + async def measure(self) -> float: + return 1.5 + + @command() + async def scale(self, factor: float) -> float: + return factor * 2 + + +@pytest.fixture +def controller_api() -> ControllerAPI: + TypedCommandController.calls = [] + return TypedCommandController()._build_api(["DEVICE"]) + + +def rest_client(controller_api: ControllerAPI) -> TestClient: + transport = RestTransport() + transport.connect([controller_api], asyncio.AbstractEventLoop()) + return TestClient(transport._server._app) + + +class TestRest: + def test_void_command_answers_no_content(self, controller_api): + with rest_client(controller_api) as client: + assert client.put("/DEVICE/stop").status_code == 204 + + def test_arguments_are_taken_from_the_request_body(self, controller_api): + with rest_client(controller_api) as client: + response = client.put( + "/DEVICE/move-to", json={"position": 2.5, "wait": True} + ) + + assert response.status_code == 204 + assert TypedCommandController.calls == [(2.5, True)] + + def test_a_missing_argument_is_rejected(self, controller_api): + with rest_client(controller_api) as client: + response = client.put("/DEVICE/move-to", json={"position": 2.5}) + + assert response.status_code == 422 + assert TypedCommandController.calls == [] + + def test_return_value_comes_back_in_the_body(self, controller_api): + with rest_client(controller_api) as client: + response = client.put("/DEVICE/measure") + + assert response.status_code == 200 + assert response.json() == {"value": 1.5} + + def test_arguments_and_a_return_value_together(self, controller_api): + with rest_client(controller_api) as client: + response = client.put("/DEVICE/scale", json={"factor": 3.0}) + + assert response.status_code == 200 + assert response.json() == {"value": 6.0} + + +class TestGraphQL: + @pytest.fixture + def client(self, controller_api) -> TestClient: + transport = GraphQLTransport() + transport.connect([controller_api], asyncio.AbstractEventLoop()) + return TestClient(transport._server._app) + + def query(self, client: TestClient, mutation: str): + response = client.post("/graphql", json={"query": f"mutation {{ {mutation} }}"}) + assert response.status_code == 200 + body = response.json() + assert "errors" not in body, body["errors"] + return body["data"] + + def test_void_command_reports_that_it_ran(self, client): + assert self.query(client, "DEVICE { stop }") == {"DEVICE": {"stop": True}} + + def test_arguments_are_mutation_arguments(self, client): + assert self.query(client, "DEVICE { moveTo(position: 2.5, wait: true) }") == { + "DEVICE": {"moveTo": True} + } + assert TypedCommandController.calls == [(2.5, True)] + + def test_return_value_is_the_mutation_result(self, client): + assert self.query(client, "DEVICE { scale(factor: 3.0) }") == { + "DEVICE": {"scale": 6.0} + } + + +class TestEpicsCA: + def test_typed_commands_are_skipped_and_void_ones_are_not(self, controller_api): + """A typed command must not stop the void ones being served.""" + EpicsCAIOC([controller_api], aliases={}) + + assert { + name: method.enabled + for name, method in controller_api.command_methods.items() + } == { + "stop": True, + "move_to": False, + "measure": False, + "scale": False, + } + + def test_skipping_says_why(self, controller_api, loguru_caplog): + EpicsCAIOC([controller_api], aliases={}) + + assert ( + "EPICS CA transport cannot serve a command that takes arguments or " + "returns a value" in loguru_caplog.text + ) + + +class TestTango: + """Tango carries one argument at most, and no enum, so it declares that.""" + + def test_serves_a_void_command(self, controller_api): + assert _unservable_reason(controller_api.command_methods["stop"]) is None + + def test_serves_one_argument_and_a_return_value(self, controller_api): + assert _unservable_reason(controller_api.command_methods["scale"]) is None + + def test_refuses_more_than_one_argument(self, controller_api): + assert ( + _unservable_reason(controller_api.command_methods["move_to"]) + == "a Tango command takes at most one argument" + ) + + def test_refuses_a_datatype_it_cannot_carry(self): + class Colour(enum.Enum): + RED = "red" + + class EnumCommandController(Controller): + @command() + async def set_colour(self, colour: Colour) -> None: + pass + + api = EnumCommandController()._build_api(["DEVICE"]) + + assert ( + _unservable_reason(api.command_methods["set_colour"]) + == "Tango commands do not carry Colour" + ) + + +class TestEpicsPva: + @pytest.mark.asyncio + async def test_typed_commands_are_skipped_and_void_ones_are_not( + self, controller_api + ): + provider = parse_attributes(controller_api) + + assert "DEVICE:Stop" in provider.keys() + assert "DEVICE:MoveTo" not in provider.keys() + assert { + name: method.enabled + for name, method in controller_api.command_methods.items() + } == { + "stop": True, + "move_to": False, + "measure": False, + "scale": False, + } + + +class TestEpicsGui: + def test_a_command_the_ioc_skipped_gets_no_widget(self, controller_api): + """The IOC is built before the GUI, so a skipped command has said so.""" + EpicsCAIOC([controller_api], aliases={}) + + components = EpicsGUI(controller_api).extract_api_components(controller_api) + + assert [component.name for component in components] == ["Position", "Stop"] + + def test_a_disabled_attribute_gets_no_widget(self, controller_api): + controller_api.attributes["position"].enabled = False + + components = EpicsGUI(controller_api).extract_api_components(controller_api) + + assert "Position" not in [component.name for component in components] + + +class TestTangoCollection: + def test_only_servable_commands_are_collected(self, controller_api, mocker): + collection = _collect_dev_commands(controller_api, mocker.MagicMock()) + + assert sorted(collection) == ["Measure", "Scale", "Stop"] + assert not controller_api.command_methods["move_to"].enabled