From 15ceebbdecbe9ec61284e05ed22313e5f5e42279 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 00:28:32 +0000 Subject: [PATCH 1/7] controllers: ControllerFiller, and one declarative mechanism for attributes ADR 0013's declarative/procedural split. A class body now holds declarations and decorated behaviour; an `Attribute` constructed there is rejected, because one object would be shared by every instance of the controller. `ControllerFiller` reads the class-body hints during `__init__`. A hint that names its datatype (`frames: AttrRW[int]`) becomes an unfilled attribute straight away, so it exists as soon as `__init__` returns and the rest of `__init__` may reference it - the rule that makes `initialise` safe to run in parallel. `fill_attribute` then provisions the IO and metadata in place, so a reference taken during construction is the object that ends up serving the device, and validates the metadata against the datatype the hint declared. A hint that cannot name its datatype - `state: AttrR`, an enum whose members only exist on the wire - is a promise instead: introspection must add it, and `check_filled(source)` reports what it did not. `HintedAttribute`, `_validate_type_hints` and the `_validate_hinted_*` family are gone; the filler subsumes them. So is the deepcopy half of `_bind_attrs`; `@attr`, `@command` and `@scan` binding is untouched. An `Annotated` hint's extras are handed back untouched through the filler's `(child, extras)` iteration, which is how a protocol layer outside core FastCS gets a declarative vocabulary of its own. Core defines none. The Eiger example now fills its declared parameters rather than adding a second attribute of the same name, and names the parameter tree as the source when a promise goes unkept. Closes #394 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PP7FfLHyvQYkzKs3nm8ARv --- docs/explanations/controllers.md | 23 +- docs/explanations/declaring-attributes.md | 121 +++++++ docs/how-to/arrange-epics-screens.md | 26 +- docs/how-to/table-waveform-data.md | 66 ++-- docs/how-to/typed-commands.md | 4 +- docs/how-to/update-attributes-from-device.md | 4 +- docs/how-to/wait-methods.md | 12 +- docs/snippets/static03.py | 2 +- docs/snippets/static04.py | 2 +- docs/snippets/static05.py | 2 +- docs/snippets/static06.py | 2 +- docs/tutorials/static-drivers.md | 8 +- src/fastcs/attributes/__init__.py | 1 - src/fastcs/attributes/attr_r.py | 38 ++ src/fastcs/attributes/attr_w.py | 21 ++ src/fastcs/attributes/hinted_attribute.py | 18 - src/fastcs/controllers/__init__.py | 2 + src/fastcs/controllers/base_controller.py | 275 ++++++-------- src/fastcs/controllers/filler.py | 335 ++++++++++++++++++ src/fastcs/demo/eiger.py | 38 +- tests/benchmarking/controller.py | 7 +- tests/conftest.py | 12 +- tests/example_p4p_ioc.py | 48 +-- tests/example_softioc.py | 6 +- tests/test_controller_filler.py | 324 +++++++++++++++++ tests/test_controllers.py | 141 ++++++-- tests/test_launch.py | 2 +- tests/test_multi_controller.py | 8 +- tests/test_typed_commands.py | 2 +- tests/transports/epics/ca/test_gui.py | 4 +- .../transports/epics/ca/test_initial_value.py | 55 +-- tests/transports/epics/ca/test_softioc.py | 25 +- tests/transports/epics/test_emission.py | 4 +- tests/transports/graphQL/test_graphql.py | 12 +- tests/transports/rest/test_rest.py | 21 +- tests/transports/tango/test_dsr.py | 21 +- 36 files changed, 1296 insertions(+), 396 deletions(-) create mode 100644 docs/explanations/declaring-attributes.md delete mode 100644 src/fastcs/attributes/hinted_attribute.py create mode 100644 src/fastcs/controllers/filler.py create mode 100644 tests/test_controller_filler.py diff --git a/docs/explanations/controllers.md b/docs/explanations/controllers.md index 30a09da0a..b697fca1a 100644 --- a/docs/explanations/controllers.md +++ b/docs/explanations/controllers.md @@ -20,11 +20,16 @@ lifecycle, if required. | Method | Purpose | |---|---| -| `initialise` | Dynamically add attributes on startup, before the API is built | +| `initialise` | Fill declared attributes, and add dynamic ones, before the API is built | | `connect` | Open connection to device | | `reconnect` | Re-open connection after scan error | | `disconnect` | Release device resources before shutdown | +Attributes are constructed in `__init__`, or declared as class-body type hints +and created for you - see [](declaring-attributes.md) for which to use when. +An `Attribute` assigned in the class body is rejected: one object would be +shared by every instance of the controller. + ### Scan task behaviour When used as the root controller, FastCS collects all `@scan` methods and readable @@ -41,8 +46,12 @@ from fastcs.methods import scan class TemperatureController(Controller): - temperature = AttrR(float, units="degC") - setpoint = AttrRW(float, units="degC") + def __init__(self, host, port): + super().__init__() + self._host, self._port = host, port + + self.temperature = AttrR(float, units="degC") + self.setpoint = AttrRW(float, units="degC") async def connect(self): self._client = await DeviceClient.connect(self._host, self._port) @@ -72,7 +81,9 @@ controller also has connection logic, the parent must invoke it explicitly: ```python class ChannelController(Controller): - value = AttrR(float) + def __init__(self): + super().__init__() + self.value = AttrR(float) async def connect(self): ... @@ -107,7 +118,9 @@ from fastcs.controllers import Controller, ControllerVector class ChannelController(Controller): - value = AttrR(float) + def __init__(self): + super().__init__() + self.value = AttrR(float) class RootController(Controller): diff --git a/docs/explanations/declaring-attributes.md b/docs/explanations/declaring-attributes.md new file mode 100644 index 000000000..f036f860d --- /dev/null +++ b/docs/explanations/declaring-attributes.md @@ -0,0 +1,121 @@ +# Declaring Attributes: Class Body vs `__init__` + +A `Controller`'s class body holds **declarations**; its `__init__` holds +**construction with data**. Which of the two an attribute belongs in follows +from one question: is everything about the attribute known when you write the +class? + +## Construct it in `__init__` when you know everything + +If you can write the attribute down in full — its datatype, its metadata, and +the IO that reads and writes it — construct it in `__init__` and assign it to +`self`: + +```python +class PowerSupplyController(Controller): + def __init__(self, protocol: PowerSupplyProtocol) -> None: + super().__init__() + + self.voltage = AttrRW( + float, + getter=Polled(protocol.get_voltage, period=0.5), + setter=protocol.set_voltage, + units="V", + precision=3, + ) +``` + +This is most drivers, most of the time. Because the attribute is built per +instance, it can close over per-instance state — which is what lets one +channel's index be baked into its own getter rather than dispatched on at IO +time. + +An `Attribute` **may not** be assigned in the class body. One built there would +be a single object shared by every instance of the controller, so two devices +of the same model would write into each other; FastCS raises at construction +rather than let that happen, and names the attribute. + +## Declare it as a hint when the data arrives later + +Some drivers cannot write the attribute down in full, because what it needs +comes from the device — a detector that reports its own parameter tree — or +from a protocol library that turns one line of metadata into a getter and a +setter. Those declare a **type hint**, and `ControllerFiller` creates the +attribute from it: + +```python +class OdinDetector(Controller): + frames: AttrRW[int] + + async def initialise(self) -> None: + for name, spec in await self._query_parameter_tree(): + self.filler.fill_attribute( + name, getter=spec.getter, setter=spec.setter, **spec.meta + ) + + self.filler.check_filled("the Odin parameter tree") +``` + +The hint is not a promise to build something later. `self.frames` **exists as +soon as `__init__` returns** — as an `AttrRW[int]` with no IO yet — so the rest +of `__init__` can reference it, hand it to a sibling, or subscribe to it. That +rule is what makes `initialise` safe to run in parallel across controllers: +only `__init__` is serial, and by the time it ends every attribute anything +refers to is there. + +`fill_attribute` provisions the attribute **in place**, so a reference taken +during `__init__` is the same object that ends up serving the device. It +validates as it goes: the metadata against the datatype the hint declared +(`precision` on a `str` raises, naming the field and the attribute), and, when +you pass `datatype=`, what the device reported against what you declared. + +### A hint that cannot name its datatype + +Occasionally the datatype itself is only knowable over the wire — an enum whose +members the device reports. Write the hint without a subscript: + +```python +class EigerDetector(Controller): + state: AttrR # enum built from the device's `allowed_values` +``` + +FastCS cannot create that one, so it is a **promise** instead: introspection +must add it with `add_attribute`, and `check_filled` fails if nothing did. The +access mode is still checked — adding an `AttrW` where an `AttrR` was promised +raises. + +### Extras: metadata a protocol layer defines + +An `Annotated` hint carries anything else you put in it, and the filler hands +it back untouched: + +```python +class Instrument(SCPIController): + power: Annotated[AttrRW[float], SCPIParam("P", precision=3, units="W")] +``` + +Core FastCS defines **no** extras vocabulary. `SCPIParam` above belongs to +whatever protocol package you build on top: it reads the extras off each +declaration, builds the getter and setter its protocol implies, and fills the +attribute. This is how a protocol library gets a declarative spelling of its +own without FastCS knowing anything about it. + +## Checking what was promised + +`check_filled(source)` raises if anything the class body declared is missing, +listing it by name and naming where the data was supposed to come from. FastCS +calls it across the whole controller tree after `initialise`, so a driver that +forgets cannot serve a half-built API; call it yourself at the end of your own +`initialise` to get the better error message. An `| None` hint is not required. + +## Summary + +| You know | Write | +|---|---| +| Everything about the attribute | `self.x = AttrRW(...)` in `__init__` | +| Its type, but not its IO or metadata | `x: AttrRW[int]` and fill it in `initialise` | +| Its access mode only | `x: AttrR` and `add_attribute` it in `initialise` | +| Nothing until the device answers | No declaration; `add_attribute` in `initialise` | + +See [ADR 0013](decisions/0013-declarative-procedural-split-and-controller-filler.md) +for why there is one declarative mechanism rather than two. diff --git a/docs/how-to/arrange-epics-screens.md b/docs/how-to/arrange-epics-screens.md index 10b9c1649..1df1a5c46 100644 --- a/docs/how-to/arrange-epics-screens.md +++ b/docs/how-to/arrange-epics-screens.md @@ -20,12 +20,15 @@ from fastcs.methods import command class PowerSupplyController(Controller): - voltage = AttrRW(float, group="Output") - current = AttrRW(float, group="Output") - power = AttrR(float, group="Output") + def __init__(self) -> None: + super().__init__() + + self.voltage = AttrRW(float, group="Output") + self.current = AttrRW(float, group="Output") + self.power = AttrR(float, group="Output") - temperature = AttrR(float, group="Status") - fault_code = AttrR(int, group="Status") + self.temperature = AttrR(float, group="Status") + self.fault_code = AttrR(int, group="Status") @command(group="Actions") async def reset_faults(self) -> None: @@ -54,9 +57,12 @@ from fastcs.methods import command class ChannelController(Controller): - voltage = AttrRW(float, group="Output") - current = AttrRW(float, group="Output") - temperature = AttrR(float, group="Status") + def __init__(self) -> None: + super().__init__() + + self.voltage = AttrRW(float, group="Output") + self.current = AttrRW(float, group="Output") + self.temperature = AttrR(float, group="Status") @command(group="Actions") async def enable(self) -> None: @@ -64,14 +70,14 @@ class ChannelController(Controller): class MultiChannelPSU(Controller): - total_power = AttrR(float) - @command() async def disable_all(self) -> None: ... def __init__(self, num_channels: int) -> None: super().__init__() + + self.total_power = AttrR(float) for i in range(1, num_channels + 1): self.add_sub_controller(f"Ch{i:02d}", ChannelController()) ``` diff --git a/docs/how-to/table-waveform-data.md b/docs/how-to/table-waveform-data.md index a9cb4667d..51bba40e4 100644 --- a/docs/how-to/table-waveform-data.md +++ b/docs/how-to/table-waveform-data.md @@ -14,11 +14,14 @@ from fastcs.controllers import Controller from fastcs.datatypes import Array1D class SpectrumController(Controller): - # 1D array of 1000 float64 values - spectrum = AttrR(Array1D[np.float64], shape=(1000,)) + def __init__(self): + super().__init__() - # Writable array - setpoints = AttrRW(Array1D[np.float64], shape=(100,)) + # 1D array of 1000 float64 values + self.spectrum = AttrR(Array1D[np.float64], shape=(1000,)) + + # Writable array + self.setpoints = AttrRW(Array1D[np.float64], shape=(100,)) ``` ### 2D Arrays (Images) @@ -29,11 +32,14 @@ ophyd-async-compatible spelling, so write it as `np.ndarray` with an explicit ```python class CameraController(Controller): - # 2D array for images (max 1024x1024 uint16) - image = AttrR(np.ndarray, array_dtype=np.uint16, shape=(1024, 1024)) + def __init__(self): + super().__init__() + + # 2D array for images (max 1024x1024 uint16) + self.image = AttrR(np.ndarray, array_dtype=np.uint16, shape=(1024, 1024)) - # Smaller region of interest - roi = AttrRW(np.ndarray, array_dtype=np.uint16, shape=(256, 256)) + # Smaller region of interest + self.roi = AttrRW(np.ndarray, array_dtype=np.uint16, shape=(256, 256)) ``` ### Array Metadata @@ -49,7 +55,9 @@ class CameraController(Controller): from fastcs.methods import scan class SpectrumController(Controller): - spectrum = AttrR(Array1D[np.float64], shape=(1000,)) + def __init__(self): + super().__init__() + self.spectrum = AttrR(Array1D[np.float64], shape=(1000,)) @scan(period=0.1) async def read_spectrum(self): @@ -88,15 +96,18 @@ from fastcs.controllers import Controller from fastcs.datatypes import Table class MeasurementController(Controller): - # Table with columns: name (string), value (float), valid (bool) - results = AttrR( - Table, - structured_dtype=[ - ("name", "S32"), # 32-character string - ("value", np.float64), - ("valid", np.bool_), - ], - ) + def __init__(self): + super().__init__() + + # Table with columns: name (string), value (float), valid (bool) + self.results = AttrR( + Table, + structured_dtype=[ + ("name", "S32"), # 32-character string + ("value", np.float64), + ("valid", np.bool_), + ], + ) ``` ### Table Metadata @@ -113,14 +124,17 @@ from fastcs.controllers import Controller from fastcs.datatypes import Table class ChannelController(Controller): - channel_data = AttrR( - Table, - structured_dtype=[ - ("channel", np.int32), - ("temperature", np.float64), - ("status", "S10"), - ], - ) + def __init__(self): + super().__init__() + + self.channel_data = AttrR( + Table, + structured_dtype=[ + ("channel", np.int32), + ("temperature", np.float64), + ("status", "S10"), + ], + ) # Create data using numpy structured array data = np.array([ diff --git a/docs/how-to/typed-commands.md b/docs/how-to/typed-commands.md index b85f34655..563fc83eb 100644 --- a/docs/how-to/typed-commands.md +++ b/docs/how-to/typed-commands.md @@ -74,8 +74,8 @@ and results on attributes: ```python class Stage(Controller): - target = AttrRW(float) - last_position = AttrR(float) + target: AttrRW[float] + last_position: AttrR[float] @command() async def move(self) -> None: diff --git a/docs/how-to/update-attributes-from-device.md b/docs/how-to/update-attributes-from-device.md index b5b710965..f67dca33a 100644 --- a/docs/how-to/update-attributes-from-device.md +++ b/docs/how-to/update-attributes-from-device.md @@ -136,7 +136,7 @@ from fastcs.methods import scan class ChannelController(Controller): - voltage = AttrR(float) # No getter — updated by parent scan method + voltage: AttrR[float] # No getter — updated by parent scan method def __init__(self, index: int, connection): super().__init__(f"Ch{index:02d}") @@ -230,7 +230,7 @@ from fastcs.controllers import Controller class SubscriptionController(Controller): - temperature = AttrR(float) + temperature: AttrR[float] def __init__(self, subscription_client): super().__init__() diff --git a/docs/how-to/wait-methods.md b/docs/how-to/wait-methods.md index ee59bc67b..b3403cf82 100644 --- a/docs/how-to/wait-methods.md +++ b/docs/how-to/wait-methods.md @@ -13,8 +13,8 @@ from fastcs.controllers import Controller from fastcs.methods import command class MotorController(Controller): - position: AttrR[int] = AttrR(int) - target: AttrR[int] = AttrR(int) + position: AttrR[int] + target: AttrR[int] @command() async def move_and_wait(self): @@ -39,7 +39,7 @@ from fastcs.controllers import Controller from fastcs.methods import command class TemperatureController(Controller): - temperature: AttrR[float] = AttrR(float) + temperature: AttrR[float] @command() async def wait_for_stable(self): @@ -90,9 +90,9 @@ from fastcs.controllers import Controller from fastcs.methods import command class MultiAxisController(Controller): - x_position = AttrR(float) - y_position = AttrR(float) - z_position = AttrR(float) + x_position: AttrR[float] + y_position: AttrR[float] + z_position: AttrR[float] @command() async def move_all_and_wait(self): diff --git a/docs/snippets/static03.py b/docs/snippets/static03.py index 82b2ee5fb..cb490c62e 100644 --- a/docs/snippets/static03.py +++ b/docs/snippets/static03.py @@ -4,7 +4,7 @@ class TemperatureController(Controller): - device_id = AttrR(str) + device_id: AttrR[str] fastcs = FastCS(TemperatureController(), []) diff --git a/docs/snippets/static04.py b/docs/snippets/static04.py index c52801fab..724c8cccb 100644 --- a/docs/snippets/static04.py +++ b/docs/snippets/static04.py @@ -5,7 +5,7 @@ class TemperatureController(Controller): - device_id = AttrR(str) + device_id: AttrR[str] epics_ca = EpicsCATransport() diff --git a/docs/snippets/static05.py b/docs/snippets/static05.py index 2851dc6d0..b7b10f1be 100644 --- a/docs/snippets/static05.py +++ b/docs/snippets/static05.py @@ -8,7 +8,7 @@ class TemperatureController(Controller): - device_id = AttrR(str) + device_id: AttrR[str] gui_options = EpicsGUIOptions(output_dir=Path("."), title="Demo Temperature Controller") diff --git a/docs/snippets/static06.py b/docs/snippets/static06.py index f7bd33d15..98af3b41b 100644 --- a/docs/snippets/static06.py +++ b/docs/snippets/static06.py @@ -9,7 +9,7 @@ class TemperatureController(Controller): - device_id = AttrR(str) + device_id: AttrR[str] def __init__(self, settings: IPConnectionSettings): super().__init__() diff --git a/docs/tutorials/static-drivers.md b/docs/tutorials/static-drivers.md index e90248e9f..78ecf9eb5 100644 --- a/docs/tutorials/static-drivers.md +++ b/docs/tutorials/static-drivers.md @@ -58,7 +58,13 @@ The simulator has an API to get its ID. To expose this in the driver, an `Attrib be added to the `Controller`. There are 3 types of `Attribute`: `AttrR`, `AttrW` and `AttrRW`, representing the access mode of the API. The ID can be read, but it cannot be written, so add an `AttrR`. An `Attribute` also needs a type. The ID from the simulator -is a string, so `String` should be used. +is a string, so `str` should be used. + +Written as a type hint in the class body, the attribute is created for you, once per +controller instance, and is there to be referenced as soon as the controller is +constructed. Once there is a device to read it from, the same attribute gets its +`getter` - see [](../explanations/declaring-attributes.md) for the two spellings and +when to reach for each. ::::{admonition} Code 3 :class: dropdown, hint diff --git a/src/fastcs/attributes/__init__.py b/src/fastcs/attributes/__init__.py index 80b848411..633fcc4b1 100644 --- a/src/fastcs/attributes/__init__.py +++ b/src/fastcs/attributes/__init__.py @@ -13,6 +13,5 @@ from .attr_w import Setter as Setter from .attribute import Attribute as Attribute from .attribute import AttributeAccessMode as AttributeAccessMode -from .hinted_attribute import HintedAttribute as HintedAttribute from .severity import Severity as Severity from .update import Update as Update diff --git a/src/fastcs/attributes/attr_r.py b/src/fastcs/attributes/attr_r.py index b36b6c3a5..cf171617c 100644 --- a/src/fastcs/attributes/attr_r.py +++ b/src/fastcs/attributes/attr_r.py @@ -231,6 +231,44 @@ def severity(self) -> Severity: def has_getter(self) -> bool: return self._getter is not None + def set_getter(self, getter: Getter[DType_T] | Schedule[DType_T]) -> None: + """Provision the IO that reads this attribute, after construction. + + The `ControllerFiller` calls this to fill an attribute a class-body + hint declared but could not wire up, so that a reference taken during + ``__init__`` stays valid. It is not for changing the IO of an attribute + that already has some - that would silently swap what a running + transport is reading. + + Args: + getter: The getter, optionally wrapped in a `Polled`/`NotPolled` + schedule; a bare getter is read once, on connect + + Raises: + ValueError: If the attribute already has a getter, or a schedule + was passed with nothing to schedule + + """ + if self._getter is not None: + raise ValueError( + f"Attribute {self.full_name or type(self).__name__} already has a " + "getter" + ) + + match getter: + case Polled() | NotPolled(): + if getter.getter is None: + raise ValueError( + f"{type(getter).__name__} was given no getter to schedule" + ) + self._getter = getter.getter + self._poll_period = ( + getter.period if isinstance(getter, Polled) else None + ) + case _: + self._getter = getter + self._poll_period = ONCE + @property def poll_period(self) -> float | None: return self._poll_period diff --git a/src/fastcs/attributes/attr_w.py b/src/fastcs/attributes/attr_w.py index ca7e06c85..82ca6213d 100644 --- a/src/fastcs/attributes/attr_w.py +++ b/src/fastcs/attributes/attr_w.py @@ -138,6 +138,27 @@ def setpoint(self) -> DType_T: def has_setter(self) -> bool: return self._setter is not None + def set_setter(self, setter: Setter[DType_T]) -> None: + """Provision the IO that writes this attribute, after construction. + + The counterpart to `AttrR.set_getter`, and used the same way: by the + `ControllerFiller`, to fill an attribute a class-body hint declared. + + Args: + setter: The setter to apply values with + + Raises: + ValueError: If the attribute already has a setter + + """ + if self._setter is not None: + raise ValueError( + f"Attribute {self.full_name or type(self).__name__} already has a " + "setter" + ) + + self._setter = setter + @property def access_mode(self) -> AttributeAccessMode: return "w" diff --git a/src/fastcs/attributes/hinted_attribute.py b/src/fastcs/attributes/hinted_attribute.py deleted file mode 100644 index d58fadd46..000000000 --- a/src/fastcs/attributes/hinted_attribute.py +++ /dev/null @@ -1,18 +0,0 @@ -from dataclasses import dataclass - -from fastcs.attributes.attribute import Attribute -from fastcs.datatypes import DType - - -@dataclass(kw_only=True) -class HintedAttribute: - """An `Attribute` type hint found on a `Controller` class - - e.g. ``attr: AttrR[int]`` - - """ - - attr_type: type[Attribute] - """The type of the `Attribute` in the type hint - e.g. `AttrR`""" - dtype: type[DType] | None - """The dtype of the `Attribute` in the type hint, if any - e.g. `int`""" diff --git a/src/fastcs/controllers/__init__.py b/src/fastcs/controllers/__init__.py index e3fe4106e..774f41fa2 100644 --- a/src/fastcs/controllers/__init__.py +++ b/src/fastcs/controllers/__init__.py @@ -2,4 +2,6 @@ from .controller import Controller as Controller from .controller_api import ControllerAPI as ControllerAPI from .controller_vector import ControllerVector as ControllerVector +from .filler import ControllerFiller as ControllerFiller +from .filler import Declaration as Declaration from .runner import ControllerRunner as ControllerRunner diff --git a/src/fastcs/controllers/base_controller.py b/src/fastcs/controllers/base_controller.py index ef3b76252..b3efc8986 100755 --- a/src/fastcs/controllers/base_controller.py +++ b/src/fastcs/controllers/base_controller.py @@ -1,24 +1,27 @@ from __future__ import annotations -from copy import deepcopy from inspect import getattr_static from typing import ( TypeVar, - _GenericAlias, # type: ignore get_args, get_origin, - get_type_hints, ) -from fastcs.attributes import Attribute, HintedAttribute, UnboundAttr +from fastcs.attributes import Attribute, UnboundAttr from fastcs.controllers.controller_api import ControllerAPI -from fastcs.logging import logger -from fastcs.methods import Command, Method, Scan, UnboundCommand, UnboundScan +from fastcs.controllers.filler import ControllerFiller +from fastcs.methods import Command, Scan, UnboundCommand, UnboundScan from fastcs.tracer import Tracer T = TypeVar("T") +def _declared_datatype(hint: object) -> type | None: + """The datatype an ``AttrR[int]``-style hint names, if it names one.""" + args = get_args(hint) + return args[0] if len(args) == 1 and isinstance(args[0], type) else None + + class BaseController(Tracer): """Base class for controllers @@ -55,42 +58,12 @@ def __init__( self.__command_methods: dict[str, Command] = {} self.__scan_methods: dict[str, Scan] = {} - self.__hinted_attributes: dict[str, HintedAttribute] = {} - self.__hinted_methods: dict[str, type[Method]] = {} - self.__hinted_sub_controllers: dict[str, type[BaseController]] = {} - self._find_type_hints() + self.filler = ControllerFiller(self) + """Creates and tracks the children this controller's class body declares""" + self.filler.read_hints() self._bind_attrs() - - def _find_type_hints(self): - """Find `Attribute` and `Controller` type hints for introspection validation""" - for name, hint in get_type_hints(type(self)).items(): - if isinstance(hint, _GenericAlias): # e.g. AttrR[int] - args = get_args(hint) - hint = get_origin(hint) - else: - args = None - - if isinstance(hint, type) and issubclass(hint, Attribute): - if args is None: - dtype = None - else: - if len(args) == 1: - dtype = args[0] - else: - raise TypeError( - f"Invalid type hint for attribute {name}: {hint}" - ) - - self.__hinted_attributes[name] = HintedAttribute( - attr_type=hint, dtype=dtype - ) - - elif isinstance(hint, type) and issubclass(hint, BaseController): - self.__hinted_sub_controllers[name] = hint - - elif isinstance(hint, type) and issubclass(hint, Method): - self.__hinted_methods[name] = hint + self.filler.create_children_from_hints() @classmethod def _walk_mro(cls): @@ -110,27 +83,22 @@ def _walk_mro(cls): return class_dir def _bind_attrs(self) -> None: - """Bind Attributes and Methods to this instance. - - This method will bind the attributes of this controller class to - this specific instance. For Attributes, this is just a case of copying and - re-assigning to ``self`` to make it unique across multiple instances of this - controller class. For Methods, this requires creating a bound method from a - class method and a controller instance, so that it can be called from any - context with the controller instance passed as the ``self`` argument. - An ``@attr``-decorated getter is an `UnboundAttr` declaration rather than - an Attribute, and is bound the same way the Methods are - into a fresh - Attribute whose getter and setter are methods of this instance. + """Bind the class body's declarations to this instance. - """ - class_dir = dict.fromkeys(self._walk_mro()) - class_type_hints = { - key: value - for key, value in get_type_hints(type(self)).items() - if not key.startswith("_") - } + A class body holds declarations and decorated behaviour, never + `Attribute` instances (ADR 0013), so there is nothing to copy: each + kind of declaration is built fresh for this instance. - for attr_name in {**class_dir, **class_type_hints}: + - an ``@attr``-decorated getter is an `UnboundAttr`, bound into an + ``AttrR``/``AttrRW`` whose getter and setter are methods of this + instance + - a ``@command``/``@scan`` method is bound the same way, so it can be + called from any context with this instance as ``self`` + + Bare type hints are the third kind, and `ControllerFiller` handles + those separately - they create children rather than binding them. + """ + for attr_name in dict.fromkeys(self._walk_mro()): if attr_name == "root_attribute": continue @@ -143,22 +111,32 @@ class method and a controller instance, so that it can be called from any attr = getattr(self, attr_name, None) if isinstance(attr, Attribute): - setattr(self, attr_name, deepcopy(attr)) - else: - if isinstance(attr, Command): - self.add_command(attr_name, attr) - elif isinstance(attr, Scan): - self.add_scan(attr_name, attr) - elif isinstance( - unbound_command := getattr(attr, "__unbound_command__", None), - UnboundCommand, - ): - self.add_command(attr_name, unbound_command.bind(self)) - elif isinstance( - unbound_scan := getattr(attr, "__unbound_scan__", None), - UnboundScan, - ): - self.add_scan(attr_name, unbound_scan.bind(self)) + if attr_name in self.filler.declarations: + # A class body that both declares and assigns is the old + # spelling of one thing; the hint is the one that survives. + continue + raise TypeError( + f"{type(self).__name__}.{attr_name} is an " + f"{type(attr).__name__} in the class body, which would be " + "shared by every instance of this controller. Construct it in " + "`__init__`, or declare it as a type hint " + f"(`{attr_name}: {type(attr).__name__}[...]`) and let the " + "filler create it." + ) + if isinstance(attr, Command): + self.add_command(attr_name, attr) + elif isinstance(attr, Scan): + self.add_scan(attr_name, attr) + elif isinstance( + unbound_command := getattr(attr, "__unbound_command__", None), + UnboundCommand, + ): + self.add_command(attr_name, unbound_command.bind(self)) + elif isinstance( + unbound_scan := getattr(attr, "__unbound_scan__", None), + UnboundScan, + ): + self.add_scan(attr_name, unbound_scan.bind(self)) def __repr__(self): name = self.__class__.__name__ @@ -185,73 +163,20 @@ async def initialise(self): def post_initialise(self): """Hook to call after all attributes added, before serving the application""" - self._validate_type_hints() - - def _validate_type_hints(self): - """Validate all type-hints were introspected""" - for name in self.__hinted_attributes: - self._validate_hinted_attribute(name) - - for name in self.__hinted_sub_controllers: - self._validate_hinted_controller(name) - - for name in self.__hinted_methods: - self._validate_hinted_method(name) - - for subcontroller in self.sub_controllers.values(): - subcontroller._validate_type_hints() # noqa: SLF001 + self.check_filled() - def _validate_hinted_member(self, name: str, expected_type: type[T]) -> T: - """Validate that a hinted member exists on the controller""" - member = getattr(self, name, None) - if member is None or not isinstance(member, expected_type): - raise RuntimeError() - return member + def check_filled(self, source: str | None = None): + """Check that every class-body declaration was provisioned, recursively. - def _validate_hinted_method(self, name: str): - """Check that a `Method` with the given name exists on the controller""" - try: - method = self._validate_hinted_member(name, Method) - except RuntimeError: - raise RuntimeError( - f"Controller `{self.__class__.__name__}` failed to introspect " - f"hinted method `{name}` during initialisation" - ) from None - - logger.debug( - "Validated hinted method", name=name, controller=self, method=method - ) - - def _validate_hinted_attribute(self, name: str): - """Check that an `Attribute` with the given name exists on the controller""" - try: - attr = self._validate_hinted_member(name, Attribute) - except RuntimeError: - raise RuntimeError( - f"Controller `{self.__class__.__name__}` failed to introspect " - f"hinted attribute `{name}` during initialisation" - ) from None - - logger.debug( - "Validated hinted attribute", name=name, controller=self, attribute=attr - ) + A driver may call ``self.filler.check_filled(source)`` itself at the + end of its own ``initialise``, naming where its data came from; the + framework calls this afterwards so that a controller which forgot to + does not serve a half-built API. + """ + self.filler.check_filled(source) - def _validate_hinted_controller(self, name: str): - """Check that a sub controller with the given name exists on the controller""" - try: - controller = self._validate_hinted_member(name, BaseController) - except RuntimeError: - raise RuntimeError( - f"Controller `{self.__class__.__name__}` failed to introspect " - f"hinted controller `{name}` during initialisation" - ) from None - - logger.debug( - "Validated hinted sub controller", - name=name, - controller=self, - sub_controller=controller, - ) + for sub_controller in self.sub_controllers.values(): + sub_controller.check_filled(source) @property def path(self) -> list[str]: @@ -286,21 +211,7 @@ def add_attribute(self, name, attr: Attribute): except ValueError as exc: raise ValueError(f"Cannot add attribute {attr}.") from exc - if name in self.__hinted_attributes: - hint = self.__hinted_attributes[name] - if not isinstance(attr, hint.attr_type): - raise RuntimeError( - f"Controller '{self.__class__.__name__}' introspection of " - f"hinted attribute '{name}' does not match defined access mode. " - f"Expected '{hint.attr_type.__name__}' got '{type(attr).__name__}'." - ) - if hint.dtype is not None and hint.dtype != attr.dtype: - raise RuntimeError( - f"Controller '{self.__class__.__name__}' introspection of " - f"hinted attribute '{name}' does not match defined datatype. " - f"Expected '{hint.dtype.__name__}', " - f"got '{attr.dtype.__name__}'." - ) + self._check_against_declaration(name, attr, "attribute", "access mode") attr.set_name(name) attr.set_path(self.path) @@ -317,15 +228,7 @@ def add_sub_controller(self, name: str, sub_controller: BaseController): except ValueError as exc: raise ValueError(f"Cannot add sub controller {sub_controller}.") from exc - if name in self.__hinted_sub_controllers: - hint = self.__hinted_sub_controllers[name] - if not isinstance(sub_controller, hint): - raise RuntimeError( - f"Controller '{self.__class__.__name__}' introspection of " - f"hinted sub controller '{name}' does not match defined type. " - f"Expected '{hint.__name__}' got " - f"'{sub_controller.__class__.__name__}'." - ) + self._check_against_declaration(name, sub_controller, "sub controller", "type") sub_controller.set_path(self.path + [name]) self.__sub_controllers[name] = sub_controller @@ -338,21 +241,43 @@ def add_sub_controller(self, name: str, sub_controller: BaseController): def sub_controllers(self) -> dict[str, BaseController]: return self.__sub_controllers - def _validate_method(self, name: str, method: Method): - if name in self.__hinted_methods: - hint = self.__hinted_methods[name] - if not isinstance(method, hint): - raise RuntimeError( - f"Controller '{self.__class__.__name__}' introspection of " - f"hinted method '{name}' does not match defined type. " - f"Expected '{hint.__name__}' got " - f"'{method.__class__.__name__}'." - ) + def _check_against_declaration( + self, name: str, member: object, kind: str, mismatch: str + ): + """Check a member being added matches what the class body declared. + + The filler creates what it can from a hint, so what reaches here is + either introspection satisfying a promise (``state: AttrR``) or a + driver adding something that clashes with a declaration. + """ + declaration = self.filler.declarations.get(name) + if declaration is None: + return + + expected = get_origin(declaration.hint) or declaration.hint + if not isinstance(member, expected): + raise RuntimeError( + f"Controller '{self.__class__.__name__}' introspection of " + f"hinted {kind} '{name}' does not match defined {mismatch}. " + f"Expected '{expected.__name__}' got '{type(member).__name__}'." + ) + + datatype = _declared_datatype(declaration.hint) + if ( + datatype is not None + and isinstance(member, Attribute) + and datatype != member.dtype + ): + raise RuntimeError( + f"Controller '{self.__class__.__name__}' introspection of " + f"hinted {kind} '{name}' does not match defined datatype. " + f"Expected '{datatype.__name__}', got '{member.dtype.__name__}'." + ) def add_command(self, name: str, command: Command): try: self._check_for_name_clash(name) - self._validate_method(name, command) + self._check_against_declaration(name, command, "command method", "type") except (ValueError, RuntimeError) as exc: raise exc.__class__(f"Cannot add command method {command}.") from exc @@ -366,7 +291,7 @@ def command_methods(self) -> dict[str, Command]: def add_scan(self, name: str, scan: Scan): try: self._check_for_name_clash(name) - self._validate_method(name, scan) + self._check_against_declaration(name, scan, "scan method", "type") except (ValueError, RuntimeError) as exc: raise exc.__class__(f"Cannot add scan method {scan}.") from exc diff --git a/src/fastcs/controllers/filler.py b/src/fastcs/controllers/filler.py new file mode 100644 index 000000000..58390eba9 --- /dev/null +++ b/src/fastcs/controllers/filler.py @@ -0,0 +1,335 @@ +"""Create a `Controller`'s children from the type hints in its class body. + +The declarative half of ADR 0013: a class body holds *declarations*, and the +data that turns a declaration into a working attribute arrives later - from a +device's own description of itself, or from a protocol library's static +metadata. `ControllerFiller` is what stands between the two. It is a structural +port of ophyd-async's ``DeviceFiller``; the names follow FastCS's vocabulary +rather than ophyd-async's. + +A hint that says what it holds is **created unfilled** while the controller is +still constructing:: + + class OdinDetector(Controller): + frames: AttrRW[int] # exists as soon as __init__ returns + + async def initialise(self) -> None: + self.filler.fill_attribute("frames", getter=..., setter=...) + +so ``self.frames`` can be referenced by the rest of ``__init__`` - the rule +ADR 0013 takes from ophyd-async, and what makes ``initialise`` safe to run in +parallel across controllers. Filling provisions the IO and metadata on the +attribute that is already there, so a reference taken during ``__init__`` stays +valid. + +A hint that cannot say what it holds - ``state: AttrR``, where the datatype is +an enum whose members only exist on the wire - is a **promise** instead: it is +not created, and `ControllerFiller.check_filled` requires that introspection +added it by the time the controller is initialised. +""" + +from __future__ import annotations + +import types +from collections.abc import Iterator +from dataclasses import dataclass, field +from typing import TYPE_CHECKING, Any, Union, get_args, get_origin, get_type_hints + +from fastcs.attributes import Attribute, AttrR, AttrW +from fastcs.attributes.attr_r import Getter, Schedule +from fastcs.attributes.attr_w import Setter +from fastcs.datatypes import Meta +from fastcs.methods import Method + +if TYPE_CHECKING: + from fastcs.controllers.base_controller import BaseController + + +@dataclass +class Declaration: + """One class-body hint the filler found, and what became of it.""" + + name: str + """The attribute name on the controller, with any trailing underscore gone""" + raw_name: str + """The name as the class body wrote it, trailing underscore and all""" + hint: Any + """The declared type, with ``Annotated``/``Optional`` unwrapped""" + extras: tuple[Any, ...] = () + """Whatever else an ``Annotated`` hint carried, for a protocol layer to read""" + optional: bool = False + """Whether the hint was ``| None``, so `ControllerFiller.check_filled` does + not require it""" + child: Attribute | None = None + """The unfilled `Attribute` this created, if it could create one""" + + +@dataclass +class _Hint: + """A class-body hint, taken apart.""" + + type_: Any + extras: tuple[Any, ...] = field(default_factory=tuple) + optional: bool = False + + +def _unwrap(hint: Any) -> _Hint: + """Strip ``Annotated`` and ``| None`` off a hint, keeping what they carried.""" + extras: tuple[Any, ...] = () + optional = False + + # Annotated[X, ...] carries its extras on __metadata__, and X on the origin. + metadata = getattr(hint, "__metadata__", None) + if metadata is not None: + extras = tuple(metadata) + hint = hint.__origin__ + + if get_origin(hint) in (Union, types.UnionType): + args = [arg for arg in get_args(hint) if arg is not type(None)] + optional = len(args) != len(get_args(hint)) + if len(args) == 1: + hint = args[0] + # Annotated may sit inside the union rather than outside it. + inner = _unwrap(hint) + hint, extras = inner.type_, extras or inner.extras + + return _Hint(hint, extras, optional) + + +def _datatype_of(hint: Any) -> Any: + """The datatype an ``AttrR[int]``-style hint declares, or ``None``. + + ``None`` means the hint named an attribute class without saying what it + holds, which is a promise rather than something the filler can build. + """ + args = get_args(hint) + return args[0] if len(args) == 1 else None + + +class ControllerFiller: + """Creates and tracks the children a `Controller`'s class body declares. + + Every controller has one, as ``controller.filler``. It reads the class + hints once, during construction, and holds what it found until + `check_filled` reports on it. + """ + + def __init__(self, controller: BaseController) -> None: + self._controller = controller + self._declarations: dict[str, Declaration] = {} + + def read_hints(self) -> None: + """Record what the class body declares, without creating anything yet. + + Called by ``BaseController.__init__`` before the ``@attr``/``@command`` + declarations are bound, so that what those bind can be checked against + a hint of the same name. + """ + from fastcs.controllers.base_controller import BaseController + + for raw_name, raw_hint in get_type_hints( + type(self._controller), include_extras=True + ).items(): + if raw_name.startswith("_") or raw_name == "root_attribute": + # `root_attribute` is what a parent shows for this controller + # rather than an attribute of it, and is declared on + # `BaseController` itself, so it is not the filler's to create. + continue + + hint = _unwrap(raw_hint) + origin = get_origin(hint.type_) or hint.type_ + if not isinstance(origin, type): + continue + + if not issubclass(origin, Attribute | Method | BaseController): + continue + + # ophyd-async's convention: a trailing underscore keeps a name that + # would otherwise shadow a builtin or a framework member off the + # class body, without renaming the attribute it declares. + name = raw_name.removesuffix("_") + + self._declarations[name] = Declaration( + name=name, + raw_name=raw_name, + hint=hint.type_, + extras=hint.extras, + optional=hint.optional, + ) + + def create_children_from_hints(self) -> None: + """Create an unfilled `Attribute` for every hint that can produce one. + + Called once by ``BaseController.__init__``, after the class body's + ``@attr`` declarations have been bound - a hint whose name one of those + already provided is a check on it rather than something to create, + which is how ADR 0018's decorated attributes and ADR 0013's hints share + one class body. + """ + for declaration in self._declarations.values(): + origin = get_origin(declaration.hint) or declaration.hint + if not (isinstance(origin, type) and issubclass(origin, Attribute)): + continue + + if declaration.name in self._controller.attributes: + continue + + self._create_attribute(declaration, origin) + + def _create_attribute( + self, declaration: Declaration, attr_type: type[Attribute] + ) -> None: + datatype = _datatype_of(declaration.hint) + if datatype is None: + # A hint that does not say what it holds cannot be built, only + # promised. `state: AttrR` on an introspecting controller is the + # motivating case - the enum's members are only known over the wire. + return + + attribute = attr_type(datatype) # pyright: ignore[reportCallIssue] + declaration.child = attribute + self._controller.add_attribute(declaration.name, attribute) + + @property + def declarations(self) -> dict[str, Declaration]: + """Every class-body declaration this filler found, by name.""" + return self._declarations + + def __iter__(self) -> Iterator[tuple[Attribute | None, tuple[Any, ...]]]: + """Yield ``(child, extras)`` for each declaration, as ADR 0013 asks. + + ``child`` is the unfilled `Attribute` where one could be created, and + ``None`` for a promise. ``extras`` is whatever an ``Annotated`` hint + carried, which is how a protocol library outside core FastCS - an SCPI + package, say - gets its own declaration vocabulary without FastCS + knowing anything about it. + """ + for declaration in self._declarations.values(): + yield declaration.child, declaration.extras + + def fill_attribute( + self, + name: str, + getter: Getter[Any] | Schedule[Any] | None = None, + setter: Setter[Any] | None = None, + datatype: Any = None, + **meta: Any, + ) -> Attribute: + """Provision a declared attribute with its IO and metadata. + + Args: + name: The name the class body declared + getter: IO to read the value with, optionally wrapped in a + `Polled`/`NotPolled` schedule + setter: IO to write the value with + datatype: What the filling data says the attribute holds, checked + against what the hint declared. Pass it when the source could + disagree - introspection of a device that has changed under + you - and leave it out when it cannot. + meta: Metadata for the attribute, validated against the datatype + the hint declared - ``precision`` on a ``str`` raises + + Returns: + The attribute, which is the same object the hint created + + Raises: + KeyError: If nothing of that name was declared + TypeError: If the attribute has no half the given IO would fill, + the datatype disagrees with the hint, or the metadata does not + suit the datatype + + """ + declaration = self._declarations.get(name) + if declaration is None or declaration.child is None: + raise KeyError( + f"{type(self._controller).__name__} has no attribute declaration " + f"named '{name}' to fill. Declare it as a class-body hint with its " + "datatype, or add the attribute with `add_attribute`." + ) + + attribute = declaration.child + + if datatype is not None and datatype != attribute.dtype: + raise TypeError( + f"Controller '{type(self._controller).__name__}' filled hinted " + f"attribute '{name}' with the wrong datatype. Expected " + f"'{attribute.dtype.__name__}', got " + f"'{getattr(datatype, '__name__', datatype)}'." + ) + + if getter is not None: + if not isinstance(attribute, AttrR): + raise TypeError( + f"Attribute '{name}' was declared " + f"{type(attribute).__name__}, which has nothing to read." + ) + attribute.set_getter(getter) + + if setter is not None: + if not isinstance(attribute, AttrW): + raise TypeError( + f"Attribute '{name}' was declared " + f"{type(attribute).__name__}, which has nothing to write." + ) + attribute.set_setter(setter) + + if meta: + # `update_meta` validates the fields against the datatype the hint + # declared, which is the runtime counterpart to the static + # `Unpack[FloatMeta]` check on the constructors. + merged: dict[str, Any] = {**attribute.meta, **meta} + attribute.update_meta(merged) # pyright: ignore[reportArgumentType] + + return attribute + + def fill_meta(self, name: str, meta: Meta) -> Attribute: + """Fill a declared attribute's metadata from an extras object. + + The shape a protocol layer wants: ``SCPIParam(...).meta`` in one go, + validated against the datatype the hint declared. + """ + return self.fill_attribute(name, **meta) + + def check_filled(self, source: str | None = None) -> None: + """Raise if anything the class body promised does not exist. + + A declaration the filler could create is satisfied by having been + created, so what this reports is the promised-but-missing: a hint whose + datatype was not knowable at author time, which introspection was + therefore expected to add and did not. An ``| None`` hint is not + required. + + Args: + source: Where the filling data came from, named in the error so a + driver author can tell which introspection fell short + + Raises: + RuntimeError: Listing, by name, what is still missing + + """ + missing: list[str] = [] + + for name, declaration in self._declarations.items(): + if declaration.optional: + continue + + origin = get_origin(declaration.hint) or declaration.hint + member = getattr(self._controller, name, None) + if isinstance(member, origin): + continue + + if member is None: + missing.append(f"{name} (declared {origin.__name__}, never added)") + else: + missing.append( + f"{name} (declared {origin.__name__}, got {type(member).__name__})" + ) + + if not missing: + return + + from_source = f" from {source}" if source is not None else "" + raise RuntimeError( + f"Controller `{type(self._controller).__name__}` did not provision" + f"{from_source}: " + ", ".join(sorted(missing)) + ) diff --git a/src/fastcs/demo/eiger.py b/src/fastcs/demo/eiger.py index ba89d1f13..b6052659b 100644 --- a/src/fastcs/demo/eiger.py +++ b/src/fastcs/demo/eiger.py @@ -111,7 +111,7 @@ class EigerDetector(Controller): # Derived (soft): built on top of the introspected ``state`` param. Declaring # ``state`` as a checked attribute is what lets us reference it in code and # publish something computed from it - here, whether the detector is idle. - idle = AttrR(bool) + idle: AttrR[bool] def __init__( self, @@ -153,22 +153,36 @@ async def initialise(self) -> None: datatype = _datatype(param, data) if data["access_mode"] == "rw": - attr = AttrRW( - datatype, - getter=self._getter(subsystem, param), - setter=self._setter(subsystem, param), - ) + getter = self._getter(subsystem, param) + setter = self._setter(subsystem, param) else: # Read-only params are status values that change on the device, # so poll them periodically rather than reading once. - attr = AttrR( - datatype, - getter=Polled( - self._getter(subsystem, param), period=UPDATE_PERIOD - ), + getter = Polled( + self._getter(subsystem, param), period=UPDATE_PERIOD + ) + setter = None + + declaration = self.filler.declarations.get(param) + if declaration is not None and declaration.child is not None: + # A parameter the class body declared already exists as an + # unfilled attribute, so provision that one rather than + # adding a second of the same name. The filler checks the + # access mode and datatype the hint promised against what + # the device turned out to report. + self.filler.fill_attribute( + param, datatype=datatype, getter=getter, setter=setter + ) + elif setter is None: + self.add_attribute(param, AttrR(datatype, getter=getter)) + else: + self.add_attribute( + param, AttrRW(datatype, getter=getter, setter=setter) ) - self.add_attribute(param, attr) + # Every hinted parameter should have turned up in the tree the device + # reported; say so with the source named if one did not. + self.filler.check_filled("the Eiger REST parameter tree") # Keep the derived ``idle`` flag in sync with the introspected ``state``. self.state.add_readback_callback(self._update_idle) diff --git a/tests/benchmarking/controller.py b/tests/benchmarking/controller.py index 19932655b..46126bcbe 100644 --- a/tests/benchmarking/controller.py +++ b/tests/benchmarking/controller.py @@ -10,8 +10,11 @@ class MyTestController(Controller): - read_int: AttrR = AttrR(int, initial_value=0) - write_bool: AttrW = AttrW(bool) + def __init__(self, *args, **kwargs) -> None: + super().__init__(*args, **kwargs) + self.read_int = AttrR(int, initial_value=0) + + write_bool: AttrW[bool] def run(): diff --git a/tests/conftest.py b/tests/conftest.py index 937526e78..569b863e3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -41,12 +41,12 @@ def clear_softioc_records(): class BackendTestController(MyTestController): - read_int: AttrR = AttrR(int) - read_write_int: AttrRW = AttrRW(int) - read_write_float: AttrRW = AttrRW(float) - read_bool: AttrR = AttrR(bool) - write_bool: AttrW = AttrW(bool) - read_string: AttrRW = AttrRW(str) + read_int: AttrR[int] + read_write_int: AttrRW[int] + read_write_float: AttrRW[float] + read_bool: AttrR[bool] + write_bool: AttrW[bool] + read_string: AttrRW[str] @pytest.fixture diff --git a/tests/example_p4p_ioc.py b/tests/example_p4p_ioc.py index f41b74d6c..1aa5816e0 100644 --- a/tests/example_p4p_ioc.py +++ b/tests/example_p4p_ioc.py @@ -22,27 +22,38 @@ class FEnum(enum.Enum): class ParentController(Controller): - description = "some controller" - a: AttrRW = AttrRW( - int, - limits=NumericLimits(control=Limits(high=400_000), alarm=Limits(high=40_000)), - ) - b: AttrW = AttrW( - float, limits=NumericLimits(control=Limits(low=-1), alarm=Limits(low=-0.5)) - ) + def __init__(self, *args, **kwargs) -> None: + super().__init__(*args, **kwargs) + self.a = AttrRW( + int, + limits=NumericLimits( + control=Limits(high=400_000), alarm=Limits(high=40_000) + ), + ) + self.b = AttrW( + float, limits=NumericLimits(control=Limits(low=-1), alarm=Limits(low=-0.5)) + ) + self.table = AttrRW( + Table, + structured_dtype=[ + ("A", np.int32), + ("B", "i"), + ("C", "?"), + ("D", np.float64), + ], + ) - table: AttrRW = AttrRW( - Table, - structured_dtype=[("A", np.int32), ("B", "i"), ("C", "?"), ("D", np.float64)], - ) + description = "some controller" class ChildController(Controller): fail_on_next_e = True - c: AttrW = AttrW(int) + c: AttrW[int] def __init__(self, description: str | None = None): super().__init__(description=description) + self.g = AttrRW(Array1D[np.int64], shape=(3,)) + self.h = AttrRW(Array1D[np.float64], shape=(3, 3)) # A getter/setter pair against an in-memory "device", doing what an # AttributeIO used to do. The setter clamps the requested value and @@ -65,15 +76,13 @@ async def d(self): print("D: FINISHED") await self.j.update(self.j.readback + 1) - e: AttrR = AttrR(bool) + e: AttrR[bool] @scan(1) async def flip_flop(self): await self.e.update(not self.e.readback) - f: AttrRW = AttrRW(FEnum) - g: AttrRW = AttrRW(Array1D[np.int64], shape=(3,)) - h: AttrRW = AttrRW(Array1D[np.float64], shape=(3, 3)) + f: AttrRW[FEnum] @command() async def i(self): @@ -87,7 +96,7 @@ async def i(self): print("I: FINISHED") await self.j.update(self.j.readback + 1) - j: AttrR = AttrR(int) + j: AttrR[int] def run(id="P4P_TEST_DEVICE"): @@ -97,10 +106,9 @@ def run(id="P4P_TEST_DEVICE"): controller.set_path([id]) class ChildVector(ControllerVector): - vector_attribute: AttrR = AttrR(int) - def __init__(self, children, description=None): super().__init__(children, description) + self.vector_attribute = AttrR(int) sub_controller = ChildVector( { diff --git a/tests/example_softioc.py b/tests/example_softioc.py index 88da09e1e..e3cb1f054 100644 --- a/tests/example_softioc.py +++ b/tests/example_softioc.py @@ -12,8 +12,8 @@ class ParentController(Controller): - a: AttrR = AttrR(int) - b: AttrRW = AttrRW(int) + a: AttrR[int] + b: AttrRW[int] def __init__(self, description: str | None = None) -> None: super().__init__(description) @@ -29,7 +29,7 @@ async def set_clamped(self, value: int) -> int: class ChildController(Controller): - c: AttrW = AttrW(int) + c: AttrW[int] @command() async def d(self): diff --git a/tests/test_controller_filler.py b/tests/test_controller_filler.py new file mode 100644 index 000000000..ba940f59a --- /dev/null +++ b/tests/test_controller_filler.py @@ -0,0 +1,324 @@ +"""Tests for `ControllerFiller` - the declarative half of ADR 0013.""" + +import enum +from typing import Annotated + +import numpy as np +import pytest + +from fastcs.attributes import AttrR, AttrRW, AttrW, NotPolled +from fastcs.controllers import Controller, ControllerVector +from fastcs.datatypes import Array1D +from fastcs.methods import Command, Scan + + +class Colour(enum.Enum): + RED = "red" + + +def test_every_attribute_datatype_can_be_declared(): + class Declared(Controller): + flag: AttrR[bool] + count: AttrRW[int] + reading: AttrR[float] + label: AttrRW[str] + colour: AttrR[Colour] + trace: AttrR[Array1D[np.int32]] + + controller = Declared() + + assert {name: attr.dtype for name, attr in controller.attributes.items()} == { + "flag": bool, + "count": int, + "reading": float, + "label": str, + "colour": Colour, + "trace": np.ndarray, + } + + +def test_access_mode_comes_from_the_declared_class(): + class Declared(Controller): + readable: AttrR[int] + writable: AttrW[int] + both: AttrRW[int] + + controller = Declared() + + assert controller.readable.access_mode == "r" + assert controller.writable.access_mode == "w" + assert controller.both.access_mode == "rw" + + +def test_a_controller_with_no_hints_at_all_is_fine(): + # `fastcs-PandABlocks`/`fastcs-secop`: the whole tree comes off the wire. + class Dynamic(Controller): + async def initialise(self) -> None: + self.add_attribute("discovered", AttrR(int)) + + controller = Dynamic() + + assert controller.attributes == {} + controller.check_filled() + + +@pytest.mark.asyncio +async def test_attributes_added_without_a_hint_are_left_alone(): + class Dynamic(Controller): + async def initialise(self) -> None: + self.add_attribute("discovered", AttrR(int)) + + controller = Dynamic() + await controller.initialise() + controller.check_filled() + + assert set(controller.attributes) == {"discovered"} + + +def test_a_trailing_underscore_is_dropped_from_the_name(): + # ophyd-async's convention for a name that would otherwise clash. + class Declared(Controller): + description_: AttrR[str] + + controller = Declared() + + assert "description" in controller.attributes + assert controller.attributes["description"].dtype is str + + +def test_extras_from_an_annotated_hint_are_handed_back(): + class SCPIParam: + def __init__(self, param: str) -> None: + self.param = param + + spec = SCPIParam("P") + + class Declared(Controller): + power: Annotated[AttrRW[float], spec] + + controller = Declared() + + assert controller.filler.declarations["power"].extras == (spec,) + assert list(controller.filler) == [(controller.power, (spec,))] + + +def test_extras_survive_an_optional_annotated_hint(): + spec = object() + + class Declared(Controller): + maybe: Annotated[AttrR[int], spec] | None + + declaration = Declared().filler.declarations["maybe"] + + assert declaration.extras == (spec,) + assert declaration.optional + + +def test_a_sub_controller_hint_is_promised_not_created(): + class Child(Controller): + pass + + class Parent(Controller): + child: Child + + controller = Parent() + + assert controller.sub_controllers == {} + + with pytest.raises(RuntimeError, match="child .declared Child, never added."): + controller.check_filled() + + controller.add_sub_controller("child", Child()) + controller.check_filled() + + +def test_a_controller_vector_hint_is_promised_not_created(): + class Child(Controller): + pass + + class Parent(Controller): + children: ControllerVector[Child] + + controller = Parent() + + with pytest.raises(RuntimeError, match="children .declared ControllerVector"): + controller.check_filled() + + controller.add_sub_controller("children", ControllerVector({1: Child()})) + controller.check_filled() + + +def test_check_filled_reports_every_missing_declaration_at_once(): + class Declared(Controller): + one: AttrR + two: AttrR + + with pytest.raises(RuntimeError, match="one .*, two .*"): + Declared().check_filled() + + +def test_check_filled_recurses_into_sub_controllers(): + class Child(Controller): + promised: AttrR + + class Parent(Controller): + def __init__(self) -> None: + super().__init__() + self.child = Child() + + with pytest.raises(RuntimeError, match="promised .declared AttrR"): + Parent().check_filled() + + +def test_check_filled_names_its_source(): + class Declared(Controller): + promised: AttrR + + with pytest.raises(RuntimeError, match="did not provision from the parameter tree"): + Declared().check_filled("the parameter tree") + + +def test_a_class_body_attribute_instance_is_rejected(): + class Shared(Controller): + attr = AttrR(int) + + with pytest.raises(TypeError, match="Shared.attr is an AttrR in the class body"): + Shared() + + +@pytest.mark.asyncio +async def test_a_decorated_attribute_satisfies_a_hint_of_the_same_name(): + from fastcs.attributes import attr + + class Declared(Controller): + voltage: AttrR[float] # pyright: ignore[reportRedeclaration] + + @attr + async def voltage(self) -> float: + return 1.5 + + controller = Declared() + + # The decorator provided it, so the filler did not create a second one. + assert await controller.voltage.poll() == 1.5 + controller.check_filled() + + +def test_a_decorated_attribute_disagreeing_with_its_hint_raises(): + from fastcs.attributes import attr + + class Declared(Controller): + voltage: AttrR[int] # pyright: ignore[reportRedeclaration] + + @attr + async def voltage(self) -> float: + return 1.5 + + with pytest.raises(RuntimeError, match="does not match defined datatype"): + Declared() + + +@pytest.mark.asyncio +async def test_filling_only_a_getter_leaves_a_read_only_attribute_readable(): + class Declared(Controller): + reading: AttrR[float] + + controller = Declared() + + async def get() -> float: + return 2.5 + + controller.filler.fill_attribute("reading", getter=NotPolled(get)) + + assert controller.reading.poll_period is None + assert await controller.reading.poll() == 2.5 + + +def test_filling_a_setter_on_a_read_only_attribute_raises(): + class Declared(Controller): + reading: AttrR[float] + + controller = Declared() + + async def put(value: float) -> None: + pass + + with pytest.raises(TypeError, match="nothing to write"): + controller.filler.fill_attribute("reading", setter=put) + + +def test_filling_a_getter_on_a_write_only_attribute_raises(): + class Declared(Controller): + demand: AttrW[float] + + controller = Declared() + + async def get() -> float: + return 0.0 + + with pytest.raises(TypeError, match="nothing to read"): + controller.filler.fill_attribute("demand", getter=get) + + +def test_filling_twice_raises(): + class Declared(Controller): + reading: AttrR[float] + + controller = Declared() + + async def get() -> float: + return 0.0 + + controller.filler.fill_attribute("reading", getter=get) + + with pytest.raises(ValueError, match="already has a getter"): + controller.filler.fill_attribute("reading", getter=get) + + +def test_fill_meta_takes_a_whole_meta_dict(): + class Declared(Controller): + reading: AttrR[float] + + controller = Declared() + controller.filler.fill_meta("reading", {"units": "mm", "precision": 2}) + + assert controller.reading.meta == {"units": "mm", "precision": 2} + + +def test_a_method_hint_is_promised(): + async def noop() -> None: + pass + + class Declared(Controller): + sweep: Scan + + controller = Declared() + + with pytest.raises(RuntimeError, match="sweep .declared Scan, never added."): + controller.check_filled() + + with pytest.raises(RuntimeError, match="Cannot add command method"): + controller.add_command("sweep", Command(noop)) + + controller.add_scan("sweep", Scan(fn=noop, period=0.1)) + controller.check_filled() + + +def test_hinted_attributes_are_not_shared_between_instances(): + class Declared(Controller): + count: AttrRW[int] + + one, two = Declared(), Declared() + + assert one.count is not two.count + + +def test_attributes_can_be_added_to_a_bare_controller_from_outside(): + # What a filler does, and what `fastcs-catio` moves to instead of building + # Controller classes at runtime with `type(...)` - ADR 0013, question 2. + controller = Controller() + controller.add_attribute("discovered", AttrR(int)) + controller.temperature = AttrRW(float) + + assert set(controller.attributes) == {"discovered", "temperature"} + controller.check_filled() diff --git a/tests/test_controllers.py b/tests/test_controllers.py index c6d2a9ee9..aa2f63ccd 100644 --- a/tests/test_controllers.py +++ b/tests/test_controllers.py @@ -3,7 +3,7 @@ import pytest -from fastcs.attributes import AttrR, AttrRW +from fastcs.attributes import AttrR, AttrRW, AttrW, Polled from fastcs.controllers import Controller, ControllerVector from fastcs.methods import Command, Scan, command, scan @@ -32,15 +32,15 @@ class SomeSubController(Controller): def __init__(self): super().__init__() - sub_attribute = AttrR(int) + sub_attribute: AttrR[int] root_attribute = AttrR(int) class SomeController(Controller): annotated_attr_not_defined_in_init: AttrR[int] - equal_attr = AttrR(int) - annotated_and_equal_attr: AttrR[int] = AttrR(int) + equal_attr: AttrR[int] + annotated_and_equal_attr: AttrR[int] def __init__(self, sub_controller: Controller): super().__init__() @@ -61,16 +61,17 @@ def test_attribute_parsing(): "_attributes_attr", "attr_on_object", "_attributes_attr_equal", + "annotated_attr_not_defined_in_init", "annotated_and_equal_attr", "equal_attr", "sub_controller", } - assert SomeController.equal_attr is not controller.equal_attr - assert ( - SomeController.annotated_and_equal_attr - is not controller.annotated_and_equal_attr - ) + # Every hinted attribute is created for this instance alone, so two + # controllers of the same class never share one. + other = SomeController(SomeSubController()) + assert other.equal_attr is not controller.equal_attr + assert other.annotated_and_equal_attr is not controller.annotated_and_equal_attr assert sub_controller.attributes == { "sub_attribute": sub_controller.sub_attribute, @@ -99,11 +100,11 @@ def test_conflicting_attributes_and_controllers_and_commands( member_name, member_value, expected_error ): class ConflictingController(Controller): - attr = AttrR(int) cmd = Command(noop) def __init__(self): super().__init__() + self.attr = AttrR(int) self.sub_controller = Controller() controller = ConflictingController() @@ -155,26 +156,103 @@ def test_controller_vector_iter(): assert sub_controllers[index] == child -def test_attribute_hint_validation(): +def test_a_hint_with_a_datatype_is_created_unfilled(): + class HintedController(Controller): + read_write_int: AttrRW[int] + + controller = HintedController() + + # The rule from ADR 0013: it exists as soon as __init__ returns, so the + # rest of __init__ may reference it. + assert isinstance(controller.read_write_int, AttrRW) + assert controller.read_write_int.dtype is int + assert not controller.read_write_int.has_getter() + assert not controller.read_write_int.has_setter() + + controller.check_filled() + + +@pytest.mark.asyncio +async def test_filling_a_hinted_attribute(): class HintedController(Controller): read_write_int: AttrRW[int] controller = HintedController() + attribute = controller.read_write_int + + async def get() -> int: + return 7 - with pytest.raises(RuntimeError, match="does not match defined datatype"): - controller.add_attribute("read_write_int", AttrRW(float)) + async def put(value: int) -> None: + pass + + controller.filler.fill_attribute( + "read_write_int", getter=Polled(get, period=0.5), setter=put, units="counts" + ) + + # Filled in place, so a reference taken during __init__ is still the one + # that ends up serving the device. + assert controller.read_write_int is attribute + assert attribute.poll_period == 0.5 + assert attribute.meta.get("units") == "counts" + assert await attribute.poll() == 7 + + +def test_filling_the_wrong_datatype_raises(): + class HintedController(Controller): + read_write_int: AttrRW[int] + + controller = HintedController() + + with pytest.raises(TypeError, match="wrong datatype"): + controller.filler.fill_attribute("read_write_int", datatype=float) + + +def test_filling_metadata_the_datatype_has_no_use_for_raises(): + class HintedController(Controller): + label: AttrR[str] + + controller = HintedController() + + with pytest.raises(TypeError, match="'precision' is not valid metadata for str"): + controller.filler.fill_attribute("label", precision=3) + + +def test_filling_something_that_was_never_declared_raises(): + class HintedController(Controller): + read_write_int: AttrRW[int] + + controller = HintedController() + + with pytest.raises(KeyError, match="no attribute declaration"): + controller.filler.fill_attribute("not_declared") + + +def test_a_hint_without_a_datatype_is_promised_not_created(): + class HintedController(Controller): + # The datatype is only knowable over the wire, so the filler cannot + # build this one - introspection must add it. + state: AttrR + + controller = HintedController() + + assert "state" not in controller.attributes + + with pytest.raises(RuntimeError, match="state .declared AttrR, never added."): + controller.check_filled("the device") with pytest.raises(RuntimeError, match="does not match defined access mode"): - controller.add_attribute("read_write_int", AttrR(int)) + controller.add_attribute("state", AttrW(int)) + + controller.add_attribute("state", AttrR(int)) + controller.check_filled() - with pytest.raises(RuntimeError, match="failed to introspect hinted attribute"): - controller.read_write_int = 5 # type: ignore - controller._validate_type_hints() - with pytest.raises(RuntimeError, match="failed to introspect hinted attribute"): - controller._validate_type_hints() +def test_an_optional_hint_is_not_required(): + class HintedController(Controller): + maybe: AttrR | None - controller.add_attribute("read_write_int", AttrRW(int)) + HintedController().check_filled() def test_enum_attribute_hint_validation(): @@ -185,14 +263,14 @@ class BadEnum(enum.IntEnum): VAL = 0 class HintedController(Controller): - enum: AttrRW[GoodEnum] + enum: AttrRW controller = HintedController() - with pytest.raises(RuntimeError, match="does not match defined datatype"): - controller.add_attribute("enum", AttrRW(BadEnum)) + with pytest.raises(RuntimeError, match="does not match defined access mode"): + controller.add_attribute("enum", AttrR(GoodEnum)) - controller.add_attribute("enum", AttrRW(GoodEnum)) + controller.add_attribute("enum", AttrRW(BadEnum)) @pytest.mark.asyncio @@ -202,14 +280,14 @@ class HintedController(Controller): controller = HintedController() - with pytest.raises(RuntimeError, match="failed to introspect hinted controller"): - controller._validate_type_hints() + with pytest.raises(RuntimeError, match="child .declared SomeSubController"): + controller.check_filled() with pytest.raises(RuntimeError, match="does not match defined type"): controller.add_sub_controller("child", Controller()) controller.add_sub_controller("child", SomeSubController()) - controller._validate_type_hints() + controller.check_filled() @pytest.mark.asyncio @@ -219,23 +297,22 @@ class HintedController(Controller): controller = HintedController() - with pytest.raises(RuntimeError, match="failed to introspect hinted method"): - controller._validate_type_hints() + with pytest.raises(RuntimeError, match="method .declared Scan, never added."): + controller.check_filled() with pytest.raises(RuntimeError, match="Cannot add command method"): controller.add_command("method", Command(noop)) controller.add_scan("method", Scan(fn=noop, period=0.1)) - controller._validate_type_hints() + controller.check_filled() def test_controller_api(): class MyTestController(Controller): - attr1: AttrRW[int] = AttrRW(int) - def __init__(self): super().__init__(description="Controller for testing") + self.attr1 = AttrRW(int) self.attr2 = AttrRW(int) diff --git a/tests/test_launch.py b/tests/test_launch.py index df7bdae58..987c6bff8 100644 --- a/tests/test_launch.py +++ b/tests/test_launch.py @@ -44,7 +44,7 @@ def __init__(self, arg): class IsHinted(Controller): - read = AttrR(int) + read: AttrR[int] def __init__(self, arg: SomeConfig) -> None: super().__init__() diff --git a/tests/test_multi_controller.py b/tests/test_multi_controller.py index 1edf30f72..7b561773f 100644 --- a/tests/test_multi_controller.py +++ b/tests/test_multi_controller.py @@ -25,11 +25,11 @@ class _IdController(Controller): class _OneAttrController(Controller): - foo = AttrR(int) + foo: AttrR[int] class _OtherAttrController(Controller): - bar = AttrR(int) + bar: AttrR[int] def test_controller_api_path_uses_id(): @@ -302,7 +302,7 @@ class names, so ``DEV-1`` and ``DEV_1`` would silently override each other in class _LifecycleController(Controller): """Records lifecycle hook calls for end-to-end assertions.""" - foo = AttrR(int) + foo: AttrR[int] def __init__(self): super().__init__() @@ -324,7 +324,7 @@ async def disconnect(self): class _OtherLifecycleController(_LifecycleController): - bar = AttrR(int) + bar: AttrR[int] @pytest.mark.asyncio diff --git a/tests/test_typed_commands.py b/tests/test_typed_commands.py index c53a78afc..e33aedd75 100644 --- a/tests/test_typed_commands.py +++ b/tests/test_typed_commands.py @@ -29,7 +29,7 @@ class TypedCommandController(Controller): calls: list[tuple] = [] # The GraphQL transport refuses an API with nothing to read - position = AttrR(float) + position: AttrR[float] @command() async def stop(self) -> None: diff --git a/tests/transports/epics/ca/test_gui.py b/tests/transports/epics/ca/test_gui.py index b8586c469..997f60dd6 100644 --- a/tests/transports/epics/ca/test_gui.py +++ b/tests/transports/epics/ca/test_gui.py @@ -193,11 +193,11 @@ def test_get_command_component(): class _A(Controller): - foo = AttrR(int) + foo: AttrR[int] class _B(Controller): - bar = AttrR(int) + bar: AttrR[int] def _api_with_id(cls, name): diff --git a/tests/transports/epics/ca/test_initial_value.py b/tests/transports/epics/ca/test_initial_value.py index 9563e7e55..bdbf0faf8 100644 --- a/tests/transports/epics/ca/test_initial_value.py +++ b/tests/transports/epics/ca/test_initial_value.py @@ -19,32 +19,35 @@ class InitialEnum(enum.Enum): class InitialValuesController(Controller): - int_rw = AttrRW(int, initial_value=4) - float_rw = AttrRW(float, initial_value=3.1) - bool_rw = AttrRW(bool, initial_value=True) - enum_rw = AttrRW(InitialEnum, initial_value=InitialEnum.B) - str_rw = AttrRW(str, initial_value="initial") - waveform_rw = AttrRW( - Array1D[np.int64], - initial_value=np.array(range(10), dtype=np.int64), - shape=(10,), - ) - int_r = AttrR(int, initial_value=5) - float_r = AttrR(float, initial_value=4.1) - bool_r = AttrR(bool, initial_value=False) - enum_r = AttrR(InitialEnum, initial_value=InitialEnum.C) - str_r = AttrR(str, initial_value="initial_r") - waveform_r = AttrR( - Array1D[np.int64], - initial_value=np.array(range(10, 20), dtype=np.int64), - shape=(10,), - ) - int_w = AttrW(int) - float_w = AttrW(float) - bool_w = AttrW(bool) - enum_w = AttrW(InitialEnum) - str_w = AttrW(str) - waveform_w = AttrW(Array1D[np.int64], shape=(10,)) + def __init__(self, *args, **kwargs) -> None: + super().__init__(*args, **kwargs) + self.int_rw = AttrRW(int, initial_value=4) + self.float_rw = AttrRW(float, initial_value=3.1) + self.bool_rw = AttrRW(bool, initial_value=True) + self.enum_rw = AttrRW(InitialEnum, initial_value=InitialEnum.B) + self.str_rw = AttrRW(str, initial_value="initial") + self.waveform_rw = AttrRW( + Array1D[np.int64], + initial_value=np.array(range(10), dtype=np.int64), + shape=(10,), + ) + self.int_r = AttrR(int, initial_value=5) + self.float_r = AttrR(float, initial_value=4.1) + self.bool_r = AttrR(bool, initial_value=False) + self.enum_r = AttrR(InitialEnum, initial_value=InitialEnum.C) + self.str_r = AttrR(str, initial_value="initial_r") + self.waveform_r = AttrR( + Array1D[np.int64], + initial_value=np.array(range(10, 20), dtype=np.int64), + shape=(10,), + ) + self.waveform_w = AttrW(Array1D[np.int64], shape=(10,)) + + int_w: AttrW[int] + float_w: AttrW[float] + bool_w: AttrW[bool] + enum_w: AttrW[InitialEnum] + str_w: AttrW[str] @pytest.mark.forked diff --git a/tests/transports/epics/ca/test_softioc.py b/tests/transports/epics/ca/test_softioc.py index a29958cdd..18a776fe1 100644 --- a/tests/transports/epics/ca/test_softioc.py +++ b/tests/transports/epics/ca/test_softioc.py @@ -355,14 +355,17 @@ def test_get_output_record_raises(mocker: MockerFixture): class EpicsController(MyTestController): - read_int = AttrR(int) - read_write_int = AttrRW(int) - read_write_float = AttrRW(float) - read_bool = AttrR(bool) - write_bool = AttrW(bool) - read_string = AttrRW(str) - enum = AttrRW(enum.IntEnum("Enum", {"RED": 0, "GREEN": 1, "BLUE": 2})) - one_d_waveform = AttrRW(Array1D[np.int32], shape=(10,)) + def __init__(self, *args, **kwargs) -> None: + super().__init__(*args, **kwargs) + self.enum = AttrRW(enum.IntEnum("Enum", {"RED": 0, "GREEN": 1, "BLUE": 2})) + self.one_d_waveform = AttrRW(Array1D[np.int32], shape=(10,)) + + read_int: AttrR[int] + read_write_int: AttrRW[int] + read_write_float: AttrRW[float] + read_bool: AttrR[bool] + write_bool: AttrW[bool] + read_string: AttrRW[str] @pytest.fixture() @@ -585,9 +588,9 @@ async def do_nothing(): ... class ControllerLongNames(Controller): - attr_r_with_reallyreallyreallyreallyreallyreallyreally_long_name = AttrR(int) - attr_rw_with_a_reallyreally_long_name_that_is_too_long_for_rbv = AttrRW(int) - attr_rw_short_name = AttrRW(int) + attr_r_with_reallyreallyreallyreallyreallyreallyreally_long_name: AttrR[int] + attr_rw_with_a_reallyreally_long_name_that_is_too_long_for_rbv: AttrRW[int] + attr_rw_short_name: AttrRW[int] command_with_reallyreallyreallyreallyreallyreallyreally_long_name = Command( do_nothing ) diff --git a/tests/transports/epics/test_emission.py b/tests/transports/epics/test_emission.py index e83d0ac3f..c5bc2e095 100644 --- a/tests/transports/epics/test_emission.py +++ b/tests/transports/epics/test_emission.py @@ -23,11 +23,11 @@ class _Alpha(Controller): - foo = AttrR(int) + foo: AttrR[int] class _Beta(Controller): - bar = AttrR(int) + bar: AttrR[int] def _api_with_id(controller_class: type[Controller], name: str): diff --git a/tests/transports/graphQL/test_graphql.py b/tests/transports/graphQL/test_graphql.py index ae618141c..af09d65b4 100644 --- a/tests/transports/graphQL/test_graphql.py +++ b/tests/transports/graphQL/test_graphql.py @@ -16,12 +16,12 @@ class GraphQLController(MyTestController): - read_int = AttrR(int) - read_write_int = AttrRW(int) - read_write_float = AttrRW(float) - read_bool = AttrR(bool) - write_bool = AttrW(bool) - read_string = AttrRW(str) + read_int: AttrR[int] + read_write_int: AttrRW[int] + read_write_float: AttrRW[float] + read_bool: AttrR[bool] + write_bool: AttrW[bool] + read_string: AttrRW[str] _GQL_ID = "device" diff --git a/tests/transports/rest/test_rest.py b/tests/transports/rest/test_rest.py index 8bb7d97cc..3b54e4100 100644 --- a/tests/transports/rest/test_rest.py +++ b/tests/transports/rest/test_rest.py @@ -14,15 +14,18 @@ class RestController(MyTestController): - read_int = AttrR(int) - read_write_int = AttrRW(int) - read_write_float = AttrRW(float) - read_bool = AttrR(bool) - write_bool = AttrW(bool) - read_string = AttrRW(str) - enum = AttrRW(enum.IntEnum("Enum", {"RED": 0, "GREEN": 1, "BLUE": 2})) - one_d_waveform = AttrRW(Array1D[np.int32], shape=(10,)) - two_d_waveform = AttrRW(Array1D[np.int32], shape=(10, 10)) + def __init__(self, *args, **kwargs) -> None: + super().__init__(*args, **kwargs) + self.enum = AttrRW(enum.IntEnum("Enum", {"RED": 0, "GREEN": 1, "BLUE": 2})) + self.one_d_waveform = AttrRW(Array1D[np.int32], shape=(10,)) + self.two_d_waveform = AttrRW(Array1D[np.int32], shape=(10, 10)) + + read_int: AttrR[int] + read_write_int: AttrRW[int] + read_write_float: AttrRW[float] + read_bool: AttrR[bool] + write_bool: AttrW[bool] + read_string: AttrRW[str] @pytest.fixture(scope="class") diff --git a/tests/transports/tango/test_dsr.py b/tests/transports/tango/test_dsr.py index abbaa439a..fe9e5a9be 100644 --- a/tests/transports/tango/test_dsr.py +++ b/tests/transports/tango/test_dsr.py @@ -30,15 +30,18 @@ def mock_run_threadsafe_blocking(module_mocker: MockerFixture): class TangoController(MyTestController): - read_int = AttrR(int) - read_write_int = AttrRW(int) - read_write_float = AttrRW(float) - read_bool = AttrR(bool) - write_bool = AttrW(bool) - read_string = AttrRW(str) - enum = AttrRW(enum.IntEnum("Enum", {"RED": 0, "GREEN": 1, "BLUE": 2})) - one_d_waveform = AttrRW(Array1D[np.int32], shape=(10,)) - two_d_waveform = AttrRW(Array1D[np.int32], shape=(10, 10)) + def __init__(self, *args, **kwargs) -> None: + super().__init__(*args, **kwargs) + self.enum = AttrRW(enum.IntEnum("Enum", {"RED": 0, "GREEN": 1, "BLUE": 2})) + self.one_d_waveform = AttrRW(Array1D[np.int32], shape=(10,)) + self.two_d_waveform = AttrRW(Array1D[np.int32], shape=(10, 10)) + + read_int: AttrR[int] + read_write_int: AttrRW[int] + read_write_float: AttrRW[float] + read_bool: AttrR[bool] + write_bool: AttrW[bool] + read_string: AttrRW[str] @pytest.fixture(scope="class") From d576551218a912af710b969bc3481f6737b6e4d2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 00:35:26 +0000 Subject: [PATCH 2/7] controllers: reject annotated class-body attributes too, and migrate the p4p tests `_bind_attrs` skipped a class-body `Attribute` whose name also carried an annotation (`attr_1: AttrRW = AttrRW(...)`), on the reasoning that the hint was the declaration. But a bare `AttrRW` hint names no datatype, so the filler could not create it either: the attribute silently disappeared, which CI caught as four parameters missing from the PVA PVI structure. Every class-body `Attribute` now raises, annotated or not. The controllers in `test_p4p.py` are declared inside their test functions, so the earlier migration pass missed them. Bare ones become hints; the ones carrying metadata move into `__init__`. `SomeController.attr_1` was declared twice, int then float; the float one it actually had is what remains. `some_table.update` needed a cast that the unparameterised `AttrRW` annotation had been hiding: a `Table` is held as a plain structured ndarray. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PP7FfLHyvQYkzKs3nm8ARv --- src/fastcs/controllers/base_controller.py | 4 -- tests/transports/epics/pva/test_p4p.py | 78 +++++++++++++---------- 2 files changed, 45 insertions(+), 37 deletions(-) diff --git a/src/fastcs/controllers/base_controller.py b/src/fastcs/controllers/base_controller.py index b3efc8986..8104188a9 100755 --- a/src/fastcs/controllers/base_controller.py +++ b/src/fastcs/controllers/base_controller.py @@ -111,10 +111,6 @@ def _bind_attrs(self) -> None: attr = getattr(self, attr_name, None) if isinstance(attr, Attribute): - if attr_name in self.filler.declarations: - # A class body that both declares and assigns is the old - # spelling of one thing; the hint is the one that survives. - continue raise TypeError( f"{type(self).__name__}.{attr_name} is an " f"{type(attr).__name__} in the class body, which would be " diff --git a/tests/transports/epics/pva/test_p4p.py b/tests/transports/epics/pva/test_p4p.py index 56fe9b897..ff63015c8 100644 --- a/tests/transports/epics/pva/test_p4p.py +++ b/tests/transports/epics/pva/test_p4p.py @@ -2,6 +2,7 @@ import enum from datetime import datetime from multiprocessing import Queue +from typing import cast from unittest.mock import ANY from uuid import uuid4 @@ -224,17 +225,20 @@ def make_fastcs(pv_prefix: str, controller: Controller) -> FastCS: def test_read_signal_set(): class SomeController(Controller): - a: AttrRW = AttrRW( - int, - limits=NumericLimits( - control=Limits(high=400_000), alarm=Limits(high=40_000) - ), - ) - b: AttrR = AttrR( - float, - limits=NumericLimits(control=Limits(low=-1), alarm=Limits(low=-0.5)), - precision=2, - ) + def __init__(self) -> None: + super().__init__() + + self.a = AttrRW( + int, + limits=NumericLimits( + control=Limits(high=400_000), alarm=Limits(high=40_000) + ), + ) + self.b = AttrR( + float, + limits=NumericLimits(control=Limits(low=-1), alarm=Limits(low=-0.5)), + precision=2, + ) controller = SomeController() pv_prefix = str(uuid4()) @@ -274,29 +278,31 @@ async def _wait_and_set_attr_r(): def test_pvi_grouping(): class ChildChildController(Controller): - attr_e: AttrRW = AttrRW(int) - attr_f: AttrR = AttrR(str) + attr_e: AttrRW[int] + attr_f: AttrR[str] class ChildController(Controller): - attr_c: AttrW = AttrW(bool, description="Some bool") - attr_d: AttrW = AttrW(str) + attr_d: AttrW[str] + + def __init__(self) -> None: + super().__init__() + self.attr_c = AttrW(bool, description="Some bool") class SomeController(Controller): description = "some controller" - attr_1: AttrRW = AttrRW( - int, - limits=NumericLimits( - control=Limits(high=400_000), alarm=Limits(high=40_000) - ), - ) - attr_1: AttrRW = AttrRW( - float, - limits=NumericLimits(control=Limits(low=-1), alarm=Limits(low=-0.5)), - precision=2, - ) - another_attr_0: AttrRW = AttrRW(int) - another_attr_1000: AttrRW = AttrRW(int) - a_third_attr: AttrW = AttrW(int) + + another_attr_0: AttrRW[int] + another_attr_1000: AttrRW[int] + a_third_attr: AttrW[int] + + def __init__(self) -> None: + super().__init__() + + self.attr_1 = AttrRW( + float, + limits=NumericLimits(control=Limits(low=-1), alarm=Limits(low=-0.5)), + precision=2, + ) controller = SomeController() @@ -433,9 +439,13 @@ class AnEnum(enum.Enum): C = 3 class SomeController(Controller): - some_waveform: AttrRW = AttrRW(Array1D[np.int64], shape=(10, 10)) - some_table: AttrRW = AttrRW(Table, structured_dtype=table_columns) - some_enum: AttrRW = AttrRW(AnEnum) + some_enum: AttrRW[AnEnum] + + def __init__(self) -> None: + super().__init__() + + self.some_waveform = AttrRW(Array1D[np.int64], shape=(10, 10)) + self.some_table = AttrRW(Table, structured_dtype=table_columns) controller = SomeController() pv_prefix = str(uuid4()) @@ -467,7 +477,9 @@ async def _wait_and_set_attrs(): # resulting in only a change in the read back. await asyncio.gather( controller.some_waveform.update(server_set_waveform_value), - controller.some_table.update(server_set_table_value), + # A `Table` is held as a plain structured ndarray; the attribute + # validates the columns at runtime. + controller.some_table.update(cast(Table, server_set_table_value)), controller.some_enum.update(server_set_enum_value), ) From 71fe509b23347575551f4006fcda72d214ca1245 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 00:03:09 +0000 Subject: [PATCH 3/7] controllers: act on the filler review - typing, errors and test structure Review of #426: - `_check_against_declaration` no longer takes `kind`/`mismatch` to phrase its own message. It raises a plain RuntimeError saying what was declared and what arrived, and each of the four call sites catches it and re-raises with the context it owns - so `add_command`/`add_scan` stop flattening the underlying reason into "Cannot add command method X." - `fill_attribute` is typed: `Getter[DType_T]`/`Setter[DType_T]` rather than `Any`, `datatype: type[DType_T] | None`, and `**meta: Unpack[Meta]`. A setter taking a `datetime` against a `float` attribute, an unsupported datatype and an unknown metadata field are now author-time errors. Both `pyright: ignore`s in the filler are gone with them. - A hint that names no datatype gets its own error from `fill_attribute`, rather than sharing the "never declared" one. - `Declaration` carries the declared type and the datatype the hint subscripts, read once in `read_hints` instead of `get_origin` at each use. - `check_filled` drops its `source` parameter, which existed only to decorate its own exception. - Comments on the two hint checks in `read_hints` saying what each rejects. - Tests split one-claim-per-test, the enum datatype case made faithful to what it was before the filler, the controller/vector promise cases parametrized, and the `@attr` import moved to the top of the module. - `claude.md` gains the rules this review drew out: typing over `Any`, ignores must be required, one behaviour per test, parametrize instead of near-identical tests, and no parameter that only names a source for an error message. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NPjrPBVXdgT5Btcsi1hpum --- claude.md | 83 +++++++++++++++++ docs/explanations/declaring-attributes.md | 12 +-- src/fastcs/controllers/base_controller.py | 104 ++++++++++++--------- src/fastcs/controllers/filler.py | 106 +++++++++++++++------- src/fastcs/demo/eiger.py | 4 +- tests/test_attr_decorator.py | 2 +- tests/test_controller_filler.py | 87 ++++++++---------- tests/test_controllers.py | 102 ++++++++++++++------- 8 files changed, 336 insertions(+), 164 deletions(-) diff --git a/claude.md b/claude.md index 65c11785b..0c2622d08 100644 --- a/claude.md +++ b/claude.md @@ -53,6 +53,51 @@ Conventions for this repository. Follow these when writing or reviewing code and Rule of thumb: a helper's parameters should describe *what it's being asked to validate/produce*, never *whether/how the caller wants it validated*. +- The same applies to a parameter that only names *where the data came from* so the + helper can put it in its message (`source`, `origin`, `context`). The call-site + already knows that; if it wants it in the error, it should catch the helper's plain + exception and say so itself. Prefer a generic exception from the helper to a + parameter that exists only to decorate one. + + ```python + # Bad: `source` is threaded in purely to phrase the message + def check_filled(self, source: str | None = None) -> None: + ... + raise RuntimeError(f"did not provision from {source}: {missing}") + + # Good: the helper says what it knows + def check_filled(self) -> None: + ... + raise RuntimeError(f"did not provision: {missing}") + ``` + +## Typing + +- Do **not** annotate a parameter `Any` when a narrower type says what the function + actually accepts. `Any` in a signature is a promise the function cannot keep: it + turns an author-time error into a runtime one, or into no error at all. + + ```python + # Bad: any object at all type checks + def fill_attribute(self, name: str, datatype: Any = None) -> Attribute: ... + + # Good: only the types an attribute can hold + def fill_attribute(self, name: str, datatype: type[DType_T] | None = None) -> Attribute: ... + ``` + +- This holds especially for callables that will be attached to something already + typed. IO handed to an existing `Attribute` must be typed with the datatype + TypeVar (`Getter[DType_T]`, `Setter[DType_T]`), not `Getter[Any]` - otherwise a + setter taking a `datetime.datetime` type checks against a `float` attribute and only + fails, if at all, at runtime. Type it so the author is warned where they wrote it. + +- A `# type: ignore` or `# pyright: ignore` must be **required**. Before adding one, + remove it and re-run the type checker: if nothing is reported, it does not belong. + Before reaching for one at all, look for a signature that makes it unnecessary - + `**meta: Unpack[Meta]` rather than `**meta: Any` plus an ignore at the call that + passes them on. Every remaining ignore should be specific (`[reportCallIssue]`, not + bare) and should have a reason a reader can check. + ## Tests: `pytest.raises` - The `with pytest.raises(...):` block should contain the **minimal code that raises @@ -115,3 +160,41 @@ Conventions for this repository. Follow these when writing or reviewing code and This keeps tests readable as documentation: every line is evidence for the claim in the test's name, not incidental noise carried over from copy-pasting another test. + +## Tests: one behaviour per test + +- A test checks **one** thing, and its name says which. Do not put two `pytest.raises` + blocks, or a failing case and a happy path, in one test: they are separate claims + about the code, and when the first fails the rest never run. +- Setup shared by the split tests belongs in a fixture, a module-level class, or a + couple of repeated lines - repeating three lines of setup is cheaper than a test + that verifies four unrelated things. + + ```python + # Bad: three claims, one name, and the last two only run if the first passes + def test_hint_validation(): + controller = HintedController() + + with pytest.raises(RuntimeError, match="never added"): + controller.check_filled() + + with pytest.raises(RuntimeError, match="expected 'AttrR'"): + controller.add_attribute("state", AttrW(int)) + + controller.add_attribute("state", AttrR(int)) + controller.check_filled() + + # Good: one claim each, each named for what it checks + def test_a_hint_without_a_datatype_is_promised(): ... + def test_adding_a_promised_attribute_with_the_wrong_access_mode_raises(): ... + def test_adding_a_promised_attribute_satisfies_the_declaration(): ... + ``` + +## Tests: parametrize instead of near-identical tests + +- If two or more tests differ only in the values they use - a different hint, a + different datatype, a different expected message - write one + `@pytest.mark.parametrize`d test rather than copies of one body. +- Keep them separate when the *shape* of the test differs, not just its data: a case + that needs different setup, different assertions, or a different name to make sense + is a different test. diff --git a/docs/explanations/declaring-attributes.md b/docs/explanations/declaring-attributes.md index f036f860d..f99535f97 100644 --- a/docs/explanations/declaring-attributes.md +++ b/docs/explanations/declaring-attributes.md @@ -53,7 +53,7 @@ class OdinDetector(Controller): name, getter=spec.getter, setter=spec.setter, **spec.meta ) - self.filler.check_filled("the Odin parameter tree") + self.filler.check_filled() ``` The hint is not a promise to build something later. `self.frames` **exists as @@ -102,11 +102,11 @@ own without FastCS knowing anything about it. ## Checking what was promised -`check_filled(source)` raises if anything the class body declared is missing, -listing it by name and naming where the data was supposed to come from. FastCS -calls it across the whole controller tree after `initialise`, so a driver that -forgets cannot serve a half-built API; call it yourself at the end of your own -`initialise` to get the better error message. An `| None` hint is not required. +`check_filled()` raises if anything the class body declared is missing, listing +it by name. FastCS calls it across the whole controller tree after +`initialise`, so a driver that forgets cannot serve a half-built API; call it +yourself at the end of your own `initialise` to fail before anything else runs. +An `| None` hint is not required. ## Summary diff --git a/src/fastcs/controllers/base_controller.py b/src/fastcs/controllers/base_controller.py index 8104188a9..07871913f 100755 --- a/src/fastcs/controllers/base_controller.py +++ b/src/fastcs/controllers/base_controller.py @@ -1,27 +1,18 @@ from __future__ import annotations from inspect import getattr_static -from typing import ( - TypeVar, - get_args, - get_origin, -) +from typing import TypeVar from fastcs.attributes import Attribute, UnboundAttr from fastcs.controllers.controller_api import ControllerAPI from fastcs.controllers.filler import ControllerFiller +from fastcs.datatypes import resolve_datatype from fastcs.methods import Command, Scan, UnboundCommand, UnboundScan from fastcs.tracer import Tracer T = TypeVar("T") -def _declared_datatype(hint: object) -> type | None: - """The datatype an ``AttrR[int]``-style hint names, if it names one.""" - args = get_args(hint) - return args[0] if len(args) == 1 and isinstance(args[0], type) else None - - class BaseController(Tracer): """Base class for controllers @@ -161,18 +152,17 @@ def post_initialise(self): """Hook to call after all attributes added, before serving the application""" self.check_filled() - def check_filled(self, source: str | None = None): + def check_filled(self): """Check that every class-body declaration was provisioned, recursively. - A driver may call ``self.filler.check_filled(source)`` itself at the - end of its own ``initialise``, naming where its data came from; the - framework calls this afterwards so that a controller which forgot to - does not serve a half-built API. + A driver may call ``self.filler.check_filled()`` itself at the end of + its own ``initialise``; the framework calls this afterwards so that a + controller which forgot to does not serve a half-built API. """ - self.filler.check_filled(source) + self.filler.check_filled() for sub_controller in self.sub_controllers.values(): - sub_controller.check_filled(source) + sub_controller.check_filled() @property def path(self) -> list[str]: @@ -207,7 +197,13 @@ def add_attribute(self, name, attr: Attribute): except ValueError as exc: raise ValueError(f"Cannot add attribute {attr}.") from exc - self._check_against_declaration(name, attr, "attribute", "access mode") + try: + self._check_against_declaration(name, attr) + except RuntimeError as exc: + raise RuntimeError( + f"Cannot add attribute {attr}: it does not match the access mode " + f"or datatype hinted for '{name}' - {exc}." + ) from exc attr.set_name(name) attr.set_path(self.path) @@ -224,7 +220,13 @@ def add_sub_controller(self, name: str, sub_controller: BaseController): except ValueError as exc: raise ValueError(f"Cannot add sub controller {sub_controller}.") from exc - self._check_against_declaration(name, sub_controller, "sub controller", "type") + try: + self._check_against_declaration(name, sub_controller) + except RuntimeError as exc: + raise RuntimeError( + f"Cannot add sub controller {sub_controller}: it does not match " + f"the type hinted for '{name}' - {exc}." + ) from exc sub_controller.set_path(self.path + [name]) self.__sub_controllers[name] = sub_controller @@ -237,45 +239,54 @@ def add_sub_controller(self, name: str, sub_controller: BaseController): def sub_controllers(self) -> dict[str, BaseController]: return self.__sub_controllers - def _check_against_declaration( - self, name: str, member: object, kind: str, mismatch: str - ): + def _check_against_declaration(self, name: str, member: object): """Check a member being added matches what the class body declared. The filler creates what it can from a hint, so what reaches here is either introspection satisfying a promise (``state: AttrR``) or a driver adding something that clashes with a declaration. + + Raises: + RuntimeError: Saying what was declared and what arrived. The caller + is what knows which kind of member this is, so it catches this + and re-raises with that context. + """ declaration = self.filler.declarations.get(name) if declaration is None: return - expected = get_origin(declaration.hint) or declaration.hint - if not isinstance(member, expected): + declared = declaration.declared_type + if not isinstance(member, declared): raise RuntimeError( - f"Controller '{self.__class__.__name__}' introspection of " - f"hinted {kind} '{name}' does not match defined {mismatch}. " - f"Expected '{expected.__name__}' got '{type(member).__name__}'." + f"expected '{declared.__name__}', got '{type(member).__name__}'" ) - datatype = _declared_datatype(declaration.hint) - if ( - datatype is not None - and isinstance(member, Attribute) - and datatype != member.dtype - ): + if declaration.datatype is None or not isinstance(member, Attribute): + return + + # The hint holds the datatype as it was written, so `AttrR[Array1D[np.int32]]` + # has to be resolved to the `np.ndarray` an attribute reports. + declared_datatype, _ = resolve_datatype(declaration.datatype) + if declared_datatype != member.dtype: raise RuntimeError( - f"Controller '{self.__class__.__name__}' introspection of " - f"hinted {kind} '{name}' does not match defined datatype. " - f"Expected '{datatype.__name__}', got '{member.dtype.__name__}'." + f"expected datatype '{declared_datatype.__name__}', " + f"got '{member.dtype.__name__}'" ) def add_command(self, name: str, command: Command): try: self._check_for_name_clash(name) - self._check_against_declaration(name, command, "command method", "type") - except (ValueError, RuntimeError) as exc: - raise exc.__class__(f"Cannot add command method {command}.") from exc + except ValueError as exc: + raise ValueError(f"Cannot add command method {command}.") from exc + + try: + self._check_against_declaration(name, command) + except RuntimeError as exc: + raise RuntimeError( + f"Cannot add command method {command}: it does not match the type " + f"hinted for '{name}' - {exc}." + ) from exc self.__command_methods[name] = command super().__setattr__(name, command) @@ -287,9 +298,16 @@ def command_methods(self) -> dict[str, Command]: def add_scan(self, name: str, scan: Scan): try: self._check_for_name_clash(name) - self._check_against_declaration(name, scan, "scan method", "type") - except (ValueError, RuntimeError) as exc: - raise exc.__class__(f"Cannot add scan method {scan}.") from exc + except ValueError as exc: + raise ValueError(f"Cannot add scan method {scan}.") from exc + + try: + self._check_against_declaration(name, scan) + except RuntimeError as exc: + raise RuntimeError( + f"Cannot add scan method {scan}: it does not match the type " + f"hinted for '{name}' - {exc}." + ) from exc self.__scan_methods[name] = scan super().__setattr__(name, scan) diff --git a/src/fastcs/controllers/filler.py b/src/fastcs/controllers/filler.py index 58390eba9..ce8ac26f5 100644 --- a/src/fastcs/controllers/filler.py +++ b/src/fastcs/controllers/filler.py @@ -33,12 +33,20 @@ async def initialise(self) -> None: import types from collections.abc import Iterator from dataclasses import dataclass, field -from typing import TYPE_CHECKING, Any, Union, get_args, get_origin, get_type_hints +from typing import ( + TYPE_CHECKING, + Any, + Union, + Unpack, + get_args, + get_origin, + get_type_hints, +) from fastcs.attributes import Attribute, AttrR, AttrW from fastcs.attributes.attr_r import Getter, Schedule from fastcs.attributes.attr_w import Setter -from fastcs.datatypes import Meta +from fastcs.datatypes import DType_T, Meta from fastcs.methods import Method if TYPE_CHECKING: @@ -55,6 +63,11 @@ class Declaration: """The name as the class body wrote it, trailing underscore and all""" hint: Any """The declared type, with ``Annotated``/``Optional`` unwrapped""" + declared_type: type + """The class the hint names - ``AttrR`` for an ``AttrR[int]`` hint""" + datatype: Any = None + """The datatype the hint subscripts its class with, or ``None`` for a hint + that does not say what it holds""" extras: tuple[Any, ...] = () """Whatever else an ``Annotated`` hint carried, for a protocol layer to read""" optional: bool = False @@ -69,6 +82,13 @@ class _Hint: """A class-body hint, taken apart.""" type_: Any + """The hint with ``Annotated``/``| None`` stripped - ``AttrR[int]``""" + declared_type: Any + """What the hint is a subscript of - ``AttrR``, and ``AttrR`` for a bare + ``AttrR`` too. Not necessarily a class: a ``list[int] | str`` hint leaves a + ``typing.Union`` here, which is why the filler checks before using it.""" + datatype: Any = None + """The subscript, where there is exactly one - ``int`` for ``AttrR[int]``""" extras: tuple[Any, ...] = field(default_factory=tuple) optional: bool = False @@ -93,7 +113,13 @@ def _unwrap(hint: Any) -> _Hint: inner = _unwrap(hint) hint, extras = inner.type_, extras or inner.extras - return _Hint(hint, extras, optional) + return _Hint( + type_=hint, + declared_type=get_origin(hint) or hint, + datatype=_datatype_of(hint), + extras=extras, + optional=optional, + ) def _datatype_of(hint: Any) -> Any: @@ -137,11 +163,17 @@ def read_hints(self) -> None: continue hint = _unwrap(raw_hint) - origin = get_origin(hint.type_) or hint.type_ - if not isinstance(origin, type): + + # What a hint declares is the class it subscripts, but not every + # hint has one: `power: Annotated[AttrRW[float], spec] | AttrRW[int]` + # unwraps to a `typing.Union`, and `power: dict[str, int]` to a + # `dict`. The `issubclass` below is what rejects the second, and it + # raises `TypeError` rather than returning False for the first, so + # anything that is not a class is dropped before it gets there. + if not isinstance(hint.declared_type, type): continue - if not issubclass(origin, Attribute | Method | BaseController): + if not issubclass(hint.declared_type, Attribute | Method | BaseController): continue # ophyd-async's convention: a trailing underscore keeps a name that @@ -153,6 +185,8 @@ def read_hints(self) -> None: name=name, raw_name=raw_name, hint=hint.type_, + declared_type=hint.declared_type, + datatype=hint.datatype, extras=hint.extras, optional=hint.optional, ) @@ -167,26 +201,23 @@ def create_children_from_hints(self) -> None: one class body. """ for declaration in self._declarations.values(): - origin = get_origin(declaration.hint) or declaration.hint - if not (isinstance(origin, type) and issubclass(origin, Attribute)): + if not issubclass(declaration.declared_type, Attribute): continue if declaration.name in self._controller.attributes: continue - self._create_attribute(declaration, origin) + self._create_attribute(declaration) - def _create_attribute( - self, declaration: Declaration, attr_type: type[Attribute] - ) -> None: - datatype = _datatype_of(declaration.hint) - if datatype is None: + def _create_attribute(self, declaration: Declaration) -> None: + if declaration.datatype is None: # A hint that does not say what it holds cannot be built, only # promised. `state: AttrR` on an introspecting controller is the # motivating case - the enum's members are only known over the wire. return - attribute = attr_type(datatype) # pyright: ignore[reportCallIssue] + attr_type: type[Attribute] = declaration.declared_type + attribute = attr_type(declaration.datatype) declaration.child = attribute self._controller.add_attribute(declaration.name, attribute) @@ -210,10 +241,10 @@ def __iter__(self) -> Iterator[tuple[Attribute | None, tuple[Any, ...]]]: def fill_attribute( self, name: str, - getter: Getter[Any] | Schedule[Any] | None = None, - setter: Setter[Any] | None = None, - datatype: Any = None, - **meta: Any, + getter: Getter[DType_T] | Schedule[DType_T] | None = None, + setter: Setter[DType_T] | None = None, + datatype: type[DType_T] | None = None, + **meta: Unpack[Meta], ) -> Attribute: """Provision a declared attribute with its IO and metadata. @@ -240,13 +271,24 @@ def fill_attribute( """ declaration = self._declarations.get(name) - if declaration is None or declaration.child is None: + if declaration is None: raise KeyError( f"{type(self._controller).__name__} has no attribute declaration " f"named '{name}' to fill. Declare it as a class-body hint with its " "datatype, or add the attribute with `add_attribute`." ) + if declaration.child is None: + # A hint that does not name its datatype - `state: AttrR` - is a + # promise rather than something the filler could build, so there is + # no attribute here to provision. + raise KeyError( + f"{type(self._controller).__name__} declared '{name}' as " + f"{declaration.hint} without a datatype, so there is no attribute " + "to fill. Subscript the hint with the datatype it holds, or add " + "the attribute with `add_attribute`." + ) + attribute = declaration.child if datatype is not None and datatype != attribute.dtype: @@ -277,8 +319,8 @@ def fill_attribute( # `update_meta` validates the fields against the datatype the hint # declared, which is the runtime counterpart to the static # `Unpack[FloatMeta]` check on the constructors. - merged: dict[str, Any] = {**attribute.meta, **meta} - attribute.update_meta(merged) # pyright: ignore[reportArgumentType] + merged: Meta = {**attribute.meta, **meta} + attribute.update_meta(merged) return attribute @@ -290,7 +332,7 @@ def fill_meta(self, name: str, meta: Meta) -> Attribute: """ return self.fill_attribute(name, **meta) - def check_filled(self, source: str | None = None) -> None: + def check_filled(self) -> None: """Raise if anything the class body promised does not exist. A declaration the filler could create is satisfied by having been @@ -299,10 +341,6 @@ def check_filled(self, source: str | None = None) -> None: therefore expected to add and did not. An ``| None`` hint is not required. - Args: - source: Where the filling data came from, named in the error so a - driver author can tell which introspection fell short - Raises: RuntimeError: Listing, by name, what is still missing @@ -313,23 +351,23 @@ def check_filled(self, source: str | None = None) -> None: if declaration.optional: continue - origin = get_origin(declaration.hint) or declaration.hint + declared = declaration.declared_type member = getattr(self._controller, name, None) - if isinstance(member, origin): + if isinstance(member, declared): continue if member is None: - missing.append(f"{name} (declared {origin.__name__}, never added)") + missing.append(f"{name} (declared {declared.__name__}, never added)") else: missing.append( - f"{name} (declared {origin.__name__}, got {type(member).__name__})" + f"{name} (declared {declared.__name__}, " + f"got {type(member).__name__})" ) if not missing: return - from_source = f" from {source}" if source is not None else "" raise RuntimeError( - f"Controller `{type(self._controller).__name__}` did not provision" - f"{from_source}: " + ", ".join(sorted(missing)) + f"Controller `{type(self._controller).__name__}` did not provision: " + + ", ".join(sorted(missing)) ) diff --git a/src/fastcs/demo/eiger.py b/src/fastcs/demo/eiger.py index b6052659b..afebc0c7c 100644 --- a/src/fastcs/demo/eiger.py +++ b/src/fastcs/demo/eiger.py @@ -181,8 +181,8 @@ async def initialise(self) -> None: ) # Every hinted parameter should have turned up in the tree the device - # reported; say so with the source named if one did not. - self.filler.check_filled("the Eiger REST parameter tree") + # reported. + self.filler.check_filled() # Keep the derived ``idle`` flag in sync with the introspected ``state``. self.state.add_readback_callback(self._update_idle) diff --git a/tests/test_attr_decorator.py b/tests/test_attr_decorator.py index 711cca1c9..811731609 100644 --- a/tests/test_attr_decorator.py +++ b/tests/test_attr_decorator.py @@ -236,7 +236,7 @@ class Device(Controller): async def label(self) -> str: return "x" - with pytest.raises(RuntimeError, match="does not match defined access mode"): + with pytest.raises(RuntimeError, match="expected 'AttrRW', got 'AttrR'"): Device() diff --git a/tests/test_controller_filler.py b/tests/test_controller_filler.py index ba940f59a..b78829461 100644 --- a/tests/test_controller_filler.py +++ b/tests/test_controller_filler.py @@ -1,13 +1,14 @@ """Tests for `ControllerFiller` - the declarative half of ADR 0013.""" import enum +from collections.abc import Callable from typing import Annotated import numpy as np import pytest -from fastcs.attributes import AttrR, AttrRW, AttrW, NotPolled -from fastcs.controllers import Controller, ControllerVector +from fastcs.attributes import AttrR, AttrRW, AttrW, NotPolled, attr +from fastcs.controllers import BaseController, Controller, ControllerVector from fastcs.datatypes import Array1D from fastcs.methods import Command, Scan @@ -50,7 +51,8 @@ class Declared(Controller): assert controller.both.access_mode == "rw" -def test_a_controller_with_no_hints_at_all_is_fine(): +@pytest.mark.asyncio +async def test_attributes_added_without_a_hint(): # `fastcs-PandABlocks`/`fastcs-secop`: the whole tree comes off the wire. class Dynamic(Controller): async def initialise(self) -> None: @@ -59,20 +61,11 @@ async def initialise(self) -> None: controller = Dynamic() assert controller.attributes == {} - controller.check_filled() - - -@pytest.mark.asyncio -async def test_attributes_added_without_a_hint_are_left_alone(): - class Dynamic(Controller): - async def initialise(self) -> None: - self.add_attribute("discovered", AttrR(int)) - controller = Dynamic() await controller.initialise() - controller.check_filled() assert set(controller.attributes) == {"discovered"} + controller.check_filled() def test_a_trailing_underscore_is_dropped_from_the_name(): @@ -83,7 +76,6 @@ class Declared(Controller): controller = Declared() assert "description" in controller.attributes - assert controller.attributes["description"].dtype is str def test_extras_from_an_annotated_hint_are_handed_back(): @@ -114,37 +106,50 @@ class Declared(Controller): assert declaration.optional -def test_a_sub_controller_hint_is_promised_not_created(): - class Child(Controller): - pass +class Child(Controller): + pass - class Parent(Controller): - child: Child - controller = Parent() +class ChildHintedParent(Controller): + child: Child - assert controller.sub_controllers == {} - with pytest.raises(RuntimeError, match="child .declared Child, never added."): - controller.check_filled() +class VectorHintedParent(Controller): + children: ControllerVector[Child] - controller.add_sub_controller("child", Child()) - controller.check_filled() +@pytest.mark.parametrize( + "parent_type, name, expected", + [ + (ChildHintedParent, "child", "child .declared Child, never added."), + (VectorHintedParent, "children", "children .declared ControllerVector"), + ], +) +def test_a_controller_hint_is_not_created( + parent_type: type[Controller], name: str, expected: str +): + controller = parent_type() -def test_a_controller_vector_hint_is_promised_not_created(): - class Child(Controller): - pass + assert controller.sub_controllers == {} - class Parent(Controller): - children: ControllerVector[Child] + with pytest.raises(RuntimeError, match=expected): + controller.check_filled() - controller = Parent() - with pytest.raises(RuntimeError, match="children .declared ControllerVector"): - controller.check_filled() +@pytest.mark.parametrize( + "parent_type, name, child", + [ + (ChildHintedParent, "child", Child), + (VectorHintedParent, "children", lambda: ControllerVector({1: Child()})), + ], +) +def test_adding_a_promised_controller_satisfies_the_declaration( + parent_type: type[Controller], name: str, child: Callable[[], BaseController] +): + controller = parent_type() + + controller.add_sub_controller(name, child()) - controller.add_sub_controller("children", ControllerVector({1: Child()})) controller.check_filled() @@ -170,14 +175,6 @@ def __init__(self) -> None: Parent().check_filled() -def test_check_filled_names_its_source(): - class Declared(Controller): - promised: AttrR - - with pytest.raises(RuntimeError, match="did not provision from the parameter tree"): - Declared().check_filled("the parameter tree") - - def test_a_class_body_attribute_instance_is_rejected(): class Shared(Controller): attr = AttrR(int) @@ -188,8 +185,6 @@ class Shared(Controller): @pytest.mark.asyncio async def test_a_decorated_attribute_satisfies_a_hint_of_the_same_name(): - from fastcs.attributes import attr - class Declared(Controller): voltage: AttrR[float] # pyright: ignore[reportRedeclaration] @@ -205,8 +200,6 @@ async def voltage(self) -> float: def test_a_decorated_attribute_disagreeing_with_its_hint_raises(): - from fastcs.attributes import attr - class Declared(Controller): voltage: AttrR[int] # pyright: ignore[reportRedeclaration] @@ -214,7 +207,7 @@ class Declared(Controller): async def voltage(self) -> float: return 1.5 - with pytest.raises(RuntimeError, match="does not match defined datatype"): + with pytest.raises(RuntimeError, match="expected datatype 'int', got 'float'"): Declared() diff --git a/tests/test_controllers.py b/tests/test_controllers.py index aa2f63ccd..fb8575c21 100644 --- a/tests/test_controllers.py +++ b/tests/test_controllers.py @@ -228,23 +228,37 @@ class HintedController(Controller): controller.filler.fill_attribute("not_declared") -def test_a_hint_without_a_datatype_is_promised_not_created(): - class HintedController(Controller): - # The datatype is only knowable over the wire, so the filler cannot - # build this one - introspection must add it. - state: AttrR +class PromisedAttrController(Controller): + # The datatype is only knowable over the wire, so the filler cannot build + # this one - introspection must add it. + state: AttrR - controller = HintedController() + +def test_a_hint_without_a_datatype_is_not_created(): + controller = PromisedAttrController() assert "state" not in controller.attributes + +def test_a_hint_without_a_datatype_is_promised(): + controller = PromisedAttrController() + with pytest.raises(RuntimeError, match="state .declared AttrR, never added."): - controller.check_filled("the device") + controller.check_filled() + + +def test_adding_a_promised_attribute_with_the_wrong_access_mode_raises(): + controller = PromisedAttrController() - with pytest.raises(RuntimeError, match="does not match defined access mode"): + with pytest.raises(RuntimeError, match="expected 'AttrR', got 'AttrW'"): controller.add_attribute("state", AttrW(int)) + +def test_adding_a_promised_attribute_satisfies_the_declaration(): + controller = PromisedAttrController() + controller.add_attribute("state", AttrR(int)) + controller.check_filled() @@ -255,54 +269,80 @@ class HintedController(Controller): HintedController().check_filled() -def test_enum_attribute_hint_validation(): - class GoodEnum(enum.IntEnum): - VAL = 0 +class GoodEnum(enum.IntEnum): + VAL = 0 - class BadEnum(enum.IntEnum): - VAL = 0 - class HintedController(Controller): - enum: AttrRW +class BadEnum(enum.IntEnum): + VAL = 0 - controller = HintedController() - with pytest.raises(RuntimeError, match="does not match defined access mode"): - controller.add_attribute("enum", AttrR(GoodEnum)) +class EnumHintedController(Controller): + colour: AttrRW[GoodEnum] - controller.add_attribute("enum", AttrRW(BadEnum)) +def test_filling_an_enum_attribute_with_another_enum_raises(): + controller = EnumHintedController() -@pytest.mark.asyncio -async def test_sub_controller_hint_validation(): - class HintedController(Controller): - child: SomeSubController + with pytest.raises(TypeError, match="wrong datatype"): + controller.filler.fill_attribute("colour", datatype=BadEnum) - controller = HintedController() + +def test_filling_an_enum_attribute_with_the_declared_enum_is_accepted(): + controller = EnumHintedController() + + controller.filler.fill_attribute("colour", datatype=GoodEnum) + + assert controller.colour.dtype is GoodEnum + + +class SubControllerHintedController(Controller): + child: SomeSubController + + +def test_a_sub_controller_hint_is_promised(): + controller = SubControllerHintedController() with pytest.raises(RuntimeError, match="child .declared SomeSubController"): controller.check_filled() - with pytest.raises(RuntimeError, match="does not match defined type"): + +def test_adding_a_sub_controller_of_the_wrong_type_raises(): + controller = SubControllerHintedController() + + with pytest.raises(RuntimeError, match="expected 'SomeSubController'"): controller.add_sub_controller("child", Controller()) + +def test_adding_the_declared_sub_controller_satisfies_the_declaration(): + controller = SubControllerHintedController() + controller.add_sub_controller("child", SomeSubController()) + controller.check_filled() -@pytest.mark.asyncio -async def test_method_hint_validation(): - class HintedController(Controller): - method: Scan +class MethodHintedController(Controller): + method: Scan - controller = HintedController() + +def test_a_method_hint_is_promised(): + controller = MethodHintedController() with pytest.raises(RuntimeError, match="method .declared Scan, never added."): controller.check_filled() - with pytest.raises(RuntimeError, match="Cannot add command method"): + +def test_adding_a_method_of_the_wrong_kind_raises(): + controller = MethodHintedController() + + with pytest.raises(RuntimeError, match="expected 'Scan', got 'Command'"): controller.add_command("method", Command(noop)) + +def test_adding_the_declared_method_satisfies_the_declaration(): + controller = MethodHintedController() + controller.add_scan("method", Scan(fn=noop, period=0.1)) controller.check_filled() From 358250c9eb79dfdbb1729589903ac0d6add8f657 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 23:54:14 +0000 Subject: [PATCH 4/7] controllers: fold the hint into the declaration, and fill atomically Acts on the second round of review on #394. `Declaration` now holds the `Hint` it came from rather than copying each of its members out, so `declared_type`, `datatype`, `extras` and `optional` are read through `declaration.hint`. `Hint.datatype` is typed `type[DType] | None` rather than `Any`, matching `fill_attribute`. `fill_attribute` checks the whole request - access modes, IO already present, and the metadata - before applying any of it, so a rejected fill leaves the attribute untouched and the corrected call is not refused by IO the failed one had installed. An `@attr` that satisfies a hint of the same name is now recorded as the declaration's child, so an `Annotated` hint's extras reach the attribute the decorator provided rather than `None`. Drops the unused `name` column from a parametrized test, with the rule behind it written into `claude.md`, and removes the method-hint tests that duplicated `tests/test_controllers.py`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JAWLNMnpQSJKbsZXa3NDZx --- claude.md | 43 ++++++++ src/fastcs/controllers/__init__.py | 1 + src/fastcs/controllers/base_controller.py | 6 +- src/fastcs/controllers/filler.py | 126 ++++++++++++---------- tests/test_controller_filler.py | 79 ++++++++++---- 5 files changed, 173 insertions(+), 82 deletions(-) diff --git a/claude.md b/claude.md index 0c2622d08..034cb5265 100644 --- a/claude.md +++ b/claude.md @@ -198,3 +198,46 @@ Conventions for this repository. Follow these when writing or reviewing code and - Keep them separate when the *shape* of the test differs, not just its data: a case that needs different setup, different assertions, or a different name to make sense is a different test. + +## Unused names + +- Declare a name only where something reads it. An unused local, argument or + `@pytest.mark.parametrize` column is a reader's question with no answer: they have to + scan the whole body to find out that nothing uses it. +- This bites hardest when a parametrized test is split off another one. The columns are + copied across whole, and a column the new body never reads survives in every case: + + ```python + # Bad: `name` is in every case and read by nothing + @pytest.mark.parametrize( + "parent_type, name, expected", + [ + (ChildHintedParent, "child", "child .declared Child, never added."), + (VectorHintedParent, "children", "children .declared ControllerVector"), + ], + ) + def test_a_controller_hint_is_not_created( + parent_type: type[Controller], name: str, expected: str + ): + controller = parent_type() + + with pytest.raises(RuntimeError, match=expected): + controller.check_filled() + + # Good + @pytest.mark.parametrize( + "parent_type, expected", + [ + (ChildHintedParent, "child .declared Child, never added."), + (VectorHintedParent, "children .declared ControllerVector"), + ], + ) + def test_a_controller_hint_is_not_created( + parent_type: type[Controller], expected: str + ): + ... + ``` + +- The exception is a name a caller outside your control decides: a signature that + implements an interface, or an unpacking that has to consume every element. Prefix + those with an underscore rather than deleting them. diff --git a/src/fastcs/controllers/__init__.py b/src/fastcs/controllers/__init__.py index 774f41fa2..13e9de6de 100644 --- a/src/fastcs/controllers/__init__.py +++ b/src/fastcs/controllers/__init__.py @@ -4,4 +4,5 @@ from .controller_vector import ControllerVector as ControllerVector from .filler import ControllerFiller as ControllerFiller from .filler import Declaration as Declaration +from .filler import Hint as Hint from .runner import ControllerRunner as ControllerRunner diff --git a/src/fastcs/controllers/base_controller.py b/src/fastcs/controllers/base_controller.py index 07871913f..19954cd64 100755 --- a/src/fastcs/controllers/base_controller.py +++ b/src/fastcs/controllers/base_controller.py @@ -256,18 +256,18 @@ def _check_against_declaration(self, name: str, member: object): if declaration is None: return - declared = declaration.declared_type + declared = declaration.hint.declared_type if not isinstance(member, declared): raise RuntimeError( f"expected '{declared.__name__}', got '{type(member).__name__}'" ) - if declaration.datatype is None or not isinstance(member, Attribute): + if declaration.hint.datatype is None or not isinstance(member, Attribute): return # The hint holds the datatype as it was written, so `AttrR[Array1D[np.int32]]` # has to be resolved to the `np.ndarray` an attribute reports. - declared_datatype, _ = resolve_datatype(declaration.datatype) + declared_datatype, _ = resolve_datatype(declaration.hint.datatype) if declared_datatype != member.dtype: raise RuntimeError( f"expected datatype '{declared_datatype.__name__}', " diff --git a/src/fastcs/controllers/filler.py b/src/fastcs/controllers/filler.py index ce8ac26f5..c0e58bed5 100644 --- a/src/fastcs/controllers/filler.py +++ b/src/fastcs/controllers/filler.py @@ -31,8 +31,9 @@ async def initialise(self) -> None: from __future__ import annotations import types -from collections.abc import Iterator +from collections.abc import Callable, Iterator from dataclasses import dataclass, field +from functools import partial from typing import ( TYPE_CHECKING, Any, @@ -46,7 +47,7 @@ async def initialise(self) -> None: from fastcs.attributes import Attribute, AttrR, AttrW from fastcs.attributes.attr_r import Getter, Schedule from fastcs.attributes.attr_w import Setter -from fastcs.datatypes import DType_T, Meta +from fastcs.datatypes import DType, DType_T, Meta, validate_meta from fastcs.methods import Method if TYPE_CHECKING: @@ -54,31 +55,7 @@ async def initialise(self) -> None: @dataclass -class Declaration: - """One class-body hint the filler found, and what became of it.""" - - name: str - """The attribute name on the controller, with any trailing underscore gone""" - raw_name: str - """The name as the class body wrote it, trailing underscore and all""" - hint: Any - """The declared type, with ``Annotated``/``Optional`` unwrapped""" - declared_type: type - """The class the hint names - ``AttrR`` for an ``AttrR[int]`` hint""" - datatype: Any = None - """The datatype the hint subscripts its class with, or ``None`` for a hint - that does not say what it holds""" - extras: tuple[Any, ...] = () - """Whatever else an ``Annotated`` hint carried, for a protocol layer to read""" - optional: bool = False - """Whether the hint was ``| None``, so `ControllerFiller.check_filled` does - not require it""" - child: Attribute | None = None - """The unfilled `Attribute` this created, if it could create one""" - - -@dataclass -class _Hint: +class Hint: """A class-body hint, taken apart.""" type_: Any @@ -87,13 +64,31 @@ class _Hint: """What the hint is a subscript of - ``AttrR``, and ``AttrR`` for a bare ``AttrR`` too. Not necessarily a class: a ``list[int] | str`` hint leaves a ``typing.Union`` here, which is why the filler checks before using it.""" - datatype: Any = None - """The subscript, where there is exactly one - ``int`` for ``AttrR[int]``""" + datatype: type[DType] | None = None + """The datatype the hint subscripts its class with - ``int`` for + ``AttrR[int]`` - or ``None`` for a hint that does not say what it holds""" extras: tuple[Any, ...] = field(default_factory=tuple) + """Whatever else an ``Annotated`` hint carried, for a protocol layer to read""" optional: bool = False + """Whether the hint was ``| None``, so `ControllerFiller.check_filled` does + not require it""" + + +@dataclass +class Declaration: + """One class-body hint the filler found, and what became of it.""" + + name: str + """The attribute name on the controller, with any trailing underscore gone""" + raw_name: str + """The name as the class body wrote it, trailing underscore and all""" + hint: Hint + """The hint itself - what it declared, what it holds, and what it carried""" + child: Attribute | None = None + """The unfilled `Attribute` this created, if it could create one""" -def _unwrap(hint: Any) -> _Hint: +def _unwrap(hint: Any) -> Hint: """Strip ``Annotated`` and ``| None`` off a hint, keeping what they carried.""" extras: tuple[Any, ...] = () optional = False @@ -113,7 +108,7 @@ def _unwrap(hint: Any) -> _Hint: inner = _unwrap(hint) hint, extras = inner.type_, extras or inner.extras - return _Hint( + return Hint( type_=hint, declared_type=get_origin(hint) or hint, datatype=_datatype_of(hint), @@ -122,7 +117,7 @@ def _unwrap(hint: Any) -> _Hint: ) -def _datatype_of(hint: Any) -> Any: +def _datatype_of(hint: Any) -> type[DType] | None: """The datatype an ``AttrR[int]``-style hint declares, or ``None``. ``None`` means the hint named an attribute class without saying what it @@ -182,13 +177,7 @@ def read_hints(self) -> None: name = raw_name.removesuffix("_") self._declarations[name] = Declaration( - name=name, - raw_name=raw_name, - hint=hint.type_, - declared_type=hint.declared_type, - datatype=hint.datatype, - extras=hint.extras, - optional=hint.optional, + name=name, raw_name=raw_name, hint=hint ) def create_children_from_hints(self) -> None: @@ -201,23 +190,30 @@ def create_children_from_hints(self) -> None: one class body. """ for declaration in self._declarations.values(): - if not issubclass(declaration.declared_type, Attribute): + if not issubclass(declaration.hint.declared_type, Attribute): continue - if declaration.name in self._controller.attributes: + if ( + existing := self._controller.attributes.get(declaration.name) + ) is not None: + # An `@attr` of the same name already provided it, so the hint + # is a check on that attribute rather than something to create - + # but it is still the declaration's child, so that an + # `Annotated` hint's extras reach it through `__iter__`. + declaration.child = existing continue self._create_attribute(declaration) def _create_attribute(self, declaration: Declaration) -> None: - if declaration.datatype is None: + if declaration.hint.datatype is None: # A hint that does not say what it holds cannot be built, only # promised. `state: AttrR` on an introspecting controller is the # motivating case - the enum's members are only known over the wire. return - attr_type: type[Attribute] = declaration.declared_type - attribute = attr_type(declaration.datatype) + attr_type: type[Attribute] = declaration.hint.declared_type + attribute = attr_type(declaration.hint.datatype) declaration.child = attribute self._controller.add_attribute(declaration.name, attribute) @@ -236,7 +232,7 @@ def __iter__(self) -> Iterator[tuple[Attribute | None, tuple[Any, ...]]]: knowing anything about it. """ for declaration in self._declarations.values(): - yield declaration.child, declaration.extras + yield declaration.child, declaration.hint.extras def fill_attribute( self, @@ -284,13 +280,17 @@ def fill_attribute( # no attribute here to provision. raise KeyError( f"{type(self._controller).__name__} declared '{name}' as " - f"{declaration.hint} without a datatype, so there is no attribute " - "to fill. Subscript the hint with the datatype it holds, or add " - "the attribute with `add_attribute`." + f"{declaration.hint.type_} without a datatype, so there is no " + "attribute to fill. Subscript the hint with the datatype it holds, " + "or add the attribute with `add_attribute`." ) attribute = declaration.child + # The whole request is checked before any of it is applied, so that a + # rejected fill leaves the attribute exactly as it was. Otherwise a bad + # setter would land its getter first, and the corrected call would be + # refused by `set_getter` for IO the failed one had installed. if datatype is not None and datatype != attribute.dtype: raise TypeError( f"Controller '{type(self._controller).__name__}' filled hinted " @@ -299,13 +299,19 @@ def fill_attribute( f"'{getattr(datatype, '__name__', datatype)}'." ) + apply: list[Callable[[], None]] = [] + if getter is not None: if not isinstance(attribute, AttrR): raise TypeError( f"Attribute '{name}' was declared " f"{type(attribute).__name__}, which has nothing to read." ) - attribute.set_getter(getter) + if attribute.has_getter(): + raise ValueError( + f"Attribute {attribute.full_name or name} already has a getter" + ) + apply.append(partial(attribute.set_getter, getter)) if setter is not None: if not isinstance(attribute, AttrW): @@ -313,14 +319,22 @@ def fill_attribute( f"Attribute '{name}' was declared " f"{type(attribute).__name__}, which has nothing to write." ) - attribute.set_setter(setter) + if attribute.has_setter(): + raise ValueError( + f"Attribute {attribute.full_name or name} already has a setter" + ) + apply.append(partial(attribute.set_setter, setter)) if meta: - # `update_meta` validates the fields against the datatype the hint - # declared, which is the runtime counterpart to the static - # `Unpack[FloatMeta]` check on the constructors. + # `validate_meta` is what `update_meta` runs before it assigns, and + # the runtime counterpart to the static `Unpack[FloatMeta]` check on + # the constructors. merged: Meta = {**attribute.meta, **meta} - attribute.update_meta(merged) + validate_meta(attribute.dtype, merged, attribute.full_name or name) + apply.append(partial(attribute.update_meta, merged)) + + for step in apply: + step() return attribute @@ -348,10 +362,10 @@ def check_filled(self) -> None: missing: list[str] = [] for name, declaration in self._declarations.items(): - if declaration.optional: + if declaration.hint.optional: continue - declared = declaration.declared_type + declared = declaration.hint.declared_type member = getattr(self._controller, name, None) if isinstance(member, declared): continue diff --git a/tests/test_controller_filler.py b/tests/test_controller_filler.py index b78829461..79b3e3045 100644 --- a/tests/test_controller_filler.py +++ b/tests/test_controller_filler.py @@ -10,7 +10,6 @@ from fastcs.attributes import AttrR, AttrRW, AttrW, NotPolled, attr from fastcs.controllers import BaseController, Controller, ControllerVector from fastcs.datatypes import Array1D -from fastcs.methods import Command, Scan class Colour(enum.Enum): @@ -90,7 +89,7 @@ class Declared(Controller): controller = Declared() - assert controller.filler.declarations["power"].extras == (spec,) + assert controller.filler.declarations["power"].hint.extras == (spec,) assert list(controller.filler) == [(controller.power, (spec,))] @@ -102,8 +101,8 @@ class Declared(Controller): declaration = Declared().filler.declarations["maybe"] - assert declaration.extras == (spec,) - assert declaration.optional + assert declaration.hint.extras == (spec,) + assert declaration.hint.optional class Child(Controller): @@ -119,15 +118,13 @@ class VectorHintedParent(Controller): @pytest.mark.parametrize( - "parent_type, name, expected", + "parent_type, expected", [ - (ChildHintedParent, "child", "child .declared Child, never added."), - (VectorHintedParent, "children", "children .declared ControllerVector"), + (ChildHintedParent, "child .declared Child, never added."), + (VectorHintedParent, "children .declared ControllerVector"), ], ) -def test_a_controller_hint_is_not_created( - parent_type: type[Controller], name: str, expected: str -): +def test_a_controller_hint_is_not_created(parent_type: type[Controller], expected: str): controller = parent_type() assert controller.sub_controllers == {} @@ -199,6 +196,23 @@ async def voltage(self) -> float: controller.check_filled() +def test_a_decorated_attribute_is_the_declarations_child(): + # So a protocol layer reading `Annotated` extras off the filler reaches the + # attribute the decorator provided, rather than `None`. + spec = object() + + class Declared(Controller): + voltage: Annotated[AttrR[float], spec] # pyright: ignore[reportRedeclaration] + + @attr + async def voltage(self) -> float: + return 1.5 + + controller = Declared() + + assert list(controller.filler) == [(controller.voltage, (spec,))] + + def test_a_decorated_attribute_disagreeing_with_its_hint_raises(): class Declared(Controller): voltage: AttrR[int] # pyright: ignore[reportRedeclaration] @@ -268,33 +282,52 @@ async def get() -> float: controller.filler.fill_attribute("reading", getter=get) -def test_fill_meta_takes_a_whole_meta_dict(): +@pytest.mark.asyncio +async def test_a_rejected_fill_leaves_the_attribute_unfilled(): + # So the corrected call is not refused by IO the failed one had installed. class Declared(Controller): reading: AttrR[float] controller = Declared() - controller.filler.fill_meta("reading", {"units": "mm", "precision": 2}) - - assert controller.reading.meta == {"units": "mm", "precision": 2} + async def get() -> float: + return 3.5 -def test_a_method_hint_is_promised(): - async def noop() -> None: + async def put(value: float) -> None: pass + with pytest.raises(TypeError, match="nothing to write"): + controller.filler.fill_attribute("reading", getter=get, setter=put) + + controller.filler.fill_attribute("reading", getter=get) + + assert await controller.reading.poll() == 3.5 + + +def test_a_rejected_fill_leaves_the_metadata_alone(): class Declared(Controller): - sweep: Scan + reading: AttrR[float] controller = Declared() - with pytest.raises(RuntimeError, match="sweep .declared Scan, never added."): - controller.check_filled() + with pytest.raises(TypeError, match="not valid metadata"): + controller.filler.fill_attribute( + "reading", + units="mm", + structured_dtype=[("index", np.int32)], # pyright: ignore[reportCallIssue] + ) - with pytest.raises(RuntimeError, match="Cannot add command method"): - controller.add_command("sweep", Command(noop)) + assert controller.reading.meta == {} - controller.add_scan("sweep", Scan(fn=noop, period=0.1)) - controller.check_filled() + +def test_fill_meta_takes_a_whole_meta_dict(): + class Declared(Controller): + reading: AttrR[float] + + controller = Declared() + controller.filler.fill_meta("reading", {"units": "mm", "precision": 2}) + + assert controller.reading.meta == {"units": "mm", "precision": 2} def test_hinted_attributes_are_not_shared_between_instances(): From 1770618d50c3dd23d3bbc7f9f74f7de40a562ccc Mon Sep 17 00:00:00 2001 From: Shihab Suliman Date: Tue, 8 Sep 2026 11:39:20 +0000 Subject: [PATCH 5/7] fix: ban filling type hints with decorator --- src/fastcs/controllers/filler.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/fastcs/controllers/filler.py b/src/fastcs/controllers/filler.py index c0e58bed5..3a53495ad 100644 --- a/src/fastcs/controllers/filler.py +++ b/src/fastcs/controllers/filler.py @@ -184,24 +184,19 @@ def create_children_from_hints(self) -> None: """Create an unfilled `Attribute` for every hint that can produce one. Called once by ``BaseController.__init__``, after the class body's - ``@attr`` declarations have been bound - a hint whose name one of those - already provided is a check on it rather than something to create, - which is how ADR 0018's decorated attributes and ADR 0013's hints share - one class body. + declarations have been bound. A name cannot be declared both by an + attribute decorator and by a type hint. """ for declaration in self._declarations.values(): if not issubclass(declaration.hint.declared_type, Attribute): continue - if ( - existing := self._controller.attributes.get(declaration.name) - ) is not None: - # An `@attr` of the same name already provided it, so the hint - # is a check on that attribute rather than something to create - - # but it is still the declaration's child, so that an - # `Annotated` hint's extras reach it through `__iter__`. - declaration.child = existing - continue + if declaration.name in self._controller.attributes: + raise TypeError( + f"Controller `{type(self._controller).__name__}` cannot " + f"declare '{declaration.name}' with both an attribute " + "decorator and a type hint" + ) self._create_attribute(declaration) From 03279de11edb6734130322ee90a048682e71ba36 Mon Sep 17 00:00:00 2001 From: Shihab Suliman Date: Tue, 8 Sep 2026 11:39:42 +0000 Subject: [PATCH 6/7] test: add test to check that decorator filling type hint is banned --- tests/test_controller_filler.py | 27 +++------------------------ 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/tests/test_controller_filler.py b/tests/test_controller_filler.py index 79b3e3045..d3ba58bd2 100644 --- a/tests/test_controller_filler.py +++ b/tests/test_controller_filler.py @@ -180,8 +180,7 @@ class Shared(Controller): Shared() -@pytest.mark.asyncio -async def test_a_decorated_attribute_satisfies_a_hint_of_the_same_name(): +def test_a_decorated_attribute_cannot_satisfy_a_hint_of_the_same_name(): class Declared(Controller): voltage: AttrR[float] # pyright: ignore[reportRedeclaration] @@ -189,28 +188,8 @@ class Declared(Controller): async def voltage(self) -> float: return 1.5 - controller = Declared() - - # The decorator provided it, so the filler did not create a second one. - assert await controller.voltage.poll() == 1.5 - controller.check_filled() - - -def test_a_decorated_attribute_is_the_declarations_child(): - # So a protocol layer reading `Annotated` extras off the filler reaches the - # attribute the decorator provided, rather than `None`. - spec = object() - - class Declared(Controller): - voltage: Annotated[AttrR[float], spec] # pyright: ignore[reportRedeclaration] - - @attr - async def voltage(self) -> float: - return 1.5 - - controller = Declared() - - assert list(controller.filler) == [(controller.voltage, (spec,))] + with pytest.raises(TypeError, match="both an attribute decorator and a type hint"): + Declared() def test_a_decorated_attribute_disagreeing_with_its_hint_raises(): From 6a346d927d6d33ef022204fdff83aceb4f4ddf8b Mon Sep 17 00:00:00 2001 From: Shihab Suliman Date: Tue, 8 Sep 2026 11:54:12 +0000 Subject: [PATCH 7/7] tests: fix tests that used old decorator spelling --- tests/test_attr_decorator.py | 15 +-------------- tests/test_controller_filler.py | 6 +++--- 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/tests/test_attr_decorator.py b/tests/test_attr_decorator.py index d36688242..f5c5c1474 100644 --- a/tests/test_attr_decorator.py +++ b/tests/test_attr_decorator.py @@ -266,20 +266,7 @@ class Device(Controller): async def label(self) -> str: return "x" - controller = Device() - - assert isinstance(controller.label, AttrR) - - -def test_type_hint_of_the_wrong_access_mode_raises(): - class Device(Controller): - label: AttrRW[str] # pyright: ignore[reportRedeclaration] - - @AttrR.declare - async def label(self) -> str: - return "x" - - with pytest.raises(RuntimeError, match="expected 'AttrRW', got 'AttrR'"): + with pytest.raises(TypeError, match="both an attribute decorator and a type hint"): Device() diff --git a/tests/test_controller_filler.py b/tests/test_controller_filler.py index d3ba58bd2..f52cd9a8e 100644 --- a/tests/test_controller_filler.py +++ b/tests/test_controller_filler.py @@ -7,7 +7,7 @@ import numpy as np import pytest -from fastcs.attributes import AttrR, AttrRW, AttrW, NotPolled, attr +from fastcs.attributes import AttrR, AttrRW, AttrW, NotPolled from fastcs.controllers import BaseController, Controller, ControllerVector from fastcs.datatypes import Array1D @@ -184,7 +184,7 @@ def test_a_decorated_attribute_cannot_satisfy_a_hint_of_the_same_name(): class Declared(Controller): voltage: AttrR[float] # pyright: ignore[reportRedeclaration] - @attr + @AttrR.declare async def voltage(self) -> float: return 1.5 @@ -196,7 +196,7 @@ def test_a_decorated_attribute_disagreeing_with_its_hint_raises(): class Declared(Controller): voltage: AttrR[int] # pyright: ignore[reportRedeclaration] - @attr + @AttrR.declare async def voltage(self) -> float: return 1.5