From 4e0b3768e0095e465ae79ea5345cafc62bb578c5 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 16 Jul 2026 14:48:19 +0300 Subject: [PATCH 01/10] Promote the control address to a typed sum at the registry seam The ControlPort docstring carried a watch item: at the second-substrate trigger, promote `address: str` to a typed-sum `ControlAddress`. Tango fired that trigger. This lands the address space itself, ahead of the adapter that motivated it. The promotion deliberately does NOT reach the caller-facing surface. `ControlPort` stays `str`-addressed because the Conductor, acquisitions and the beam-availability lookup receive raw address strings from recipe steps and cannot know a substrate: whether `2bm:rot` is Channel Access, `2bm:cam1:image` is pvAccess, or `id19/bsh/1/state` is a Tango attribute is a property of the deployment ROUTE table, never of the string. So the string surface is where callers belong, and substrate identity lives in the one component that already knows it, the ControlPortRegistry. `ControlAddress` is therefore the registry-to-adapter seam's vocabulary: EpicsPvAddress | TangoAttributeAddress | InMemoryAddress, plus the single `parse_control_address(substrate, raw)` dispatch the registry calls once it has matched a route. The sum discriminates substrate FAMILY, not transport within a family: both EPICS transports take EpicsPvAddress because a PV name is the same shape on either, and CA-vs-PVA stays a route decision. OPC UA stays deferred; no variant until its adapter lands. `SubstrateControlPort` is the typed sibling Protocol the adapters implement, generic and contravariant in the address variant because an adapter consumes addresses and legitimately accepts only its own. The registry holds adapters as SubstrateControlPort[Any]: the substrate-to- variant correlation it guarantees is a runtime invariant the type system cannot track. It takes a port-suffix allowlist slot for the same reason ControlPort holds one, with the qualifier naming which seam it faces. Malformed addresses become MalformedControlAddressError at the routing boundary, alongside NoAdapterForAddressError, rather than surfacing as a split failure deep inside an adapter's wire call. Every variant round-trips through `str(...)` so log breadcrumbs stay stable. The Substrate literal gains `tango` here rather than with the adapter because `parse_control_address` needs the value to dispatch on; the factory arm that builds a TangoControlPort follows with the adapter. Co-Authored-By: Claude Opus 4.8 --- .../cora/infrastructure/control_port_route.py | 7 +- .../cora/operation/ports/control_address.py | 155 ++++++++++++++++++ .../src/cora/operation/ports/control_port.py | 86 +++++++++- .../test_port_naming_conventions.py | 6 + .../unit/operation/test_control_address.py | 84 ++++++++++ 5 files changed, 327 insertions(+), 11 deletions(-) create mode 100644 apps/api/src/cora/operation/ports/control_address.py create mode 100644 apps/api/tests/unit/operation/test_control_address.py diff --git a/apps/api/src/cora/infrastructure/control_port_route.py b/apps/api/src/cora/infrastructure/control_port_route.py index 4d28b960592..4b0a8051e6e 100644 --- a/apps/api/src/cora/infrastructure/control_port_route.py +++ b/apps/api/src/cora/infrastructure/control_port_route.py @@ -32,12 +32,13 @@ from pydantic import BaseModel, Field -Substrate = Literal["in_memory", "epics_ca", "epics_pva"] +Substrate = Literal["in_memory", "epics_ca", "epics_pva", "tango"] """The closed set of ControlPort substrates a deployment can select. `in_memory` is the test + opt-out default; `epics_ca` + `epics_pva` -are the production EPICS adapters. New substrates land here as -additive literal values plus a matching arm in the BC-side +are the production EPICS adapters; `tango` is the PyTango adapter for +Tango-floor deployments (ESRF BLISS, MAX IV). New substrates land here +as additive literal values plus a matching arm in the BC-side `build_control_port` factory's `_build_substrate` switch. """ diff --git a/apps/api/src/cora/operation/ports/control_address.py b/apps/api/src/cora/operation/ports/control_address.py new file mode 100644 index 00000000000..8412c8b3990 --- /dev/null +++ b/apps/api/src/cora/operation/ports/control_address.py @@ -0,0 +1,155 @@ +"""ControlAddress: the typed-sum control-system address space. + +`ControlPort` (the caller-facing Protocol) stays `str`-surfaced because the +Conductor receives raw address strings from recipe steps and cannot know a +substrate: whether `2bma:rot` is EPICS Channel Access, `2bma:cam1:image` is +EPICS pvAccess, or `id19/bsh/1/state` is a Tango attribute is a property of the +deployment ROUTE table (`ControlPortRoute.substrate` + prefix), never of the +string itself. Substrate identity therefore lives in the +`ControlPortRegistry`, and the registry is the one place that can turn a raw +string into a substrate-typed `ControlAddress` (at route-match time). + +This module is that typed address space: the sum +`EpicsPvAddress | TangoAttributeAddress | InMemoryAddress` plus the single +`parse_control_address(substrate, raw)` dispatch the registry calls. The +substrate adapters (`SubstrateControlPort` implementations) consume the typed +variants so they never re-parse a raw string per call: the Tango adapter reads +`address.device` / `address.attribute` instead of splitting a TRL on every read. + +Per [[project_control_port_generalization_research]] the sum discriminates +substrate FAMILY (EPICS vs Tango vs in-memory), not transport within a family: +both EPICS Channel Access and pvAccess use `EpicsPvAddress` because a PV name is +the same shape on either transport; CA-vs-PVA stays a route decision. OPC UA is +deferred (no `OpcUaNodeAddress` variant until a real OPC UA adapter lands); the +sum extends by adding a variant plus a `parse_control_address` arm. + +Every variant round-trips to its original string via `str(...)` so structured +logging + `Measurement.name` breadcrumbs stay stable across the typed boundary. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from cora.infrastructure.control_port_route import Substrate + + +class MalformedControlAddressError(Exception): + """A raw address string does not satisfy its substrate's address syntax. + + Raised by a variant's `parse` (e.g. a Tango TRL with no attribute + segment). The registry lets it propagate the same way it propagates + `NoAdapterForAddressError`: a configuration / recipe gap surfaced at the + routing boundary rather than deep in an adapter's wire call. Carries the + raw address so operators can spot the offending value from logs alone. + """ + + def __init__(self, raw: str, substrate: str, reason: str) -> None: + super().__init__( + f"Control address {raw!r} is malformed for substrate {substrate!r}: {reason}" + ) + self.raw = raw + self.substrate = substrate + self.reason = reason + + +@dataclass(frozen=True) +class EpicsPvAddress: + """An EPICS process-variable name, used by both Channel Access and pvAccess. + + A PV name is atomic (no CORA-side sub-structure); the adapter hands it to + aioca / p4p verbatim. The CA-vs-PVA choice is a route decision, not an + address-syntax one, so both EPICS adapters accept this same variant. + """ + + pv: str + + def __str__(self) -> str: + return self.pv + + @staticmethod + def parse(raw: str, substrate: str) -> EpicsPvAddress: + if not raw: + raise MalformedControlAddressError(raw, substrate, "empty PV name") + return EpicsPvAddress(raw) + + +@dataclass(frozen=True) +class TangoAttributeAddress: + """A Tango attribute TRL split into its device name and attribute. + + `id19/bsh/1/state` -> `device="id19/bsh/1"`, `attribute="state"`. Splitting + happens once here (at the registry boundary) instead of on every adapter + call, so a malformed TRL fails as a `MalformedControlAddressError` before + any proxy work. + """ + + device: str + attribute: str + + def __str__(self) -> str: + return f"{self.device}/{self.attribute}" + + @staticmethod + def parse(raw: str, substrate: str) -> TangoAttributeAddress: + device, sep, attribute = raw.rpartition("/") + if not sep or not device or not attribute: + raise MalformedControlAddressError( + raw, substrate, "expected 'device/family/member/attribute'" + ) + return TangoAttributeAddress(device, attribute) + + +@dataclass(frozen=True) +class InMemoryAddress: + """An opaque address for the in-memory adapter (tests + opt-out routes). + + No substrate syntax to validate beyond non-emptiness; the in-memory + adapter keys its dict store by the raw string. + """ + + address: str + + def __str__(self) -> str: + return self.address + + @staticmethod + def parse(raw: str, substrate: str) -> InMemoryAddress: + if not raw: + raise MalformedControlAddressError(raw, substrate, "empty address") + return InMemoryAddress(raw) + + +ControlAddress = EpicsPvAddress | TangoAttributeAddress | InMemoryAddress +"""The typed control-system address space consumed by `SubstrateControlPort`. + +Extends by adding a variant (e.g. a future `OpcUaNodeAddress`) plus a matching +arm in `parse_control_address`. +""" + + +def parse_control_address(substrate: Substrate, raw: str) -> ControlAddress: + """Parse a raw address string into the `ControlAddress` for `substrate`. + + The single str->typed dispatch the `ControlPortRegistry` calls once it has + matched a route (whose `substrate` names the family). Raises + `MalformedControlAddressError` when `raw` does not satisfy the substrate's + syntax. + """ + if substrate in ("epics_ca", "epics_pva"): + return EpicsPvAddress.parse(raw, substrate) + if substrate == "tango": + return TangoAttributeAddress.parse(raw, substrate) + return InMemoryAddress.parse(raw, substrate) + + +__all__ = [ + "ControlAddress", + "EpicsPvAddress", + "InMemoryAddress", + "MalformedControlAddressError", + "TangoAttributeAddress", + "parse_control_address", +] diff --git a/apps/api/src/cora/operation/ports/control_port.py b/apps/api/src/cora/operation/ports/control_port.py index e94ce650373..56d189feafc 100644 --- a/apps/api/src/cora/operation/ports/control_port.py +++ b/apps/api/src/cora/operation/ports/control_port.py @@ -24,13 +24,24 @@ ## Address space -`address: str` at v1. Adapters parse substrate-specific syntax: EPICS -PV name (`"2bm:rot:rbv"`), Tango TRL (`"sys/tg_test/1/double_scalar"`), -OPC UA NodeId string (`"ns=2;s=Demo.Static.Scalar.Double"`). At second- -substrate trigger, promote to a typed-sum `ControlAddress` -(`EpicsPvAddress | TangoAttributeAddress | OpcUaNodeAddress`) per -watch item 4; the change is BC-internal and non-breaking outside the -Operation BC. +The caller-facing `ControlPort` stays `address: str`. Callers (the +Conductor, `acquisitions`, the beam-availability lookup) receive raw +address strings from recipe steps and cannot know a substrate: +whether `"2bm:rot:rbv"` is EPICS Channel Access, `"2bm:cam1:image"` +is EPICS pvAccess, or `"sys/tg_test/1/double_scalar"` is a Tango +attribute is a property of the deployment ROUTE table, never of the +string. So the string surface is where callers belong. + +The second-substrate trigger (Tango) fired the typed-sum promotion, +but at the layer where substrate identity actually lives: the +registry-to-adapter seam. `ControlPortRegistry` matches a route by +string prefix, then parses the string into a `ControlAddress` +(`EpicsPvAddress | TangoAttributeAddress | InMemoryAddress`, see +`cora.operation.ports.control_address`) per the route's declared +substrate, and hands the typed variant to a `SubstrateControlPort` +adapter. The substrate adapters consume typed addresses; the change +is BC-internal and non-breaking outside the Operation BC. OPC UA +stays deferred (no `OpcUaNodeAddress` variant until its adapter lands). ## Out of scope (deferred sibling ports) @@ -66,10 +77,20 @@ from collections.abc import AsyncIterator from enum import StrEnum -from typing import Any, Protocol, runtime_checkable +from typing import Any, Protocol, TypeVar, runtime_checkable +from cora.operation.ports.control_address import ControlAddress from cora.operation.ports.measurement import Measurement, MeasurementKind, Quality +_AddressT_contra = TypeVar("_AddressT_contra", bound=ControlAddress, contravariant=True) +"""Contravariant address type for `SubstrateControlPort`. + +Contravariant because an adapter is a consumer of addresses: an adapter that +accepts a narrow variant (`EpicsPvAddress`) satisfies +`SubstrateControlPort[EpicsPvAddress]`. The registry stores adapters as +`SubstrateControlPort[Any]` because only it knows (at route-match time, not +statically) which variant a given route's adapter expects.""" + class ActuationKind(StrEnum): """How a conducted episode drove its addresses: real hardware, a @@ -290,9 +311,57 @@ def subscribe(self, address: str) -> AsyncIterator[Measurement]: ... +class SubstrateControlPort(Protocol[_AddressT_contra]): + """Typed-address value-IO port implemented by concrete substrate adapters. + + The registry-facing sibling of `ControlPort`. Where `ControlPort` is the + caller-facing `str` surface, `SubstrateControlPort` is what + `EpicsCaControlPort`, `EpicsPvaControlPort`, `CaprotoControlPort`, and + `TangoControlPort` implement: it takes a `ControlAddress` (the typed sum + parsed by `ControlPortRegistry` from the matched route's substrate). The + adapter never re-parses a raw string; it reads the typed variant's fields + directly (`EpicsPvAddress.pv`, `TangoAttributeAddress.device` / + `.attribute`). + + Generic in the address variant (contravariant): the EPICS adapters are + `SubstrateControlPort[EpicsPvAddress]`, the Tango adapter is + `SubstrateControlPort[TangoAttributeAddress]`. Each adapter legitimately + accepts only its own variant. `ControlPortRegistry` holds routes as + `SubstrateControlPort[Any]` because the substrate->variant correlation it + guarantees (it parses the route's substrate into the matching variant) is + a runtime invariant the type system cannot track statically. + + `ControlPortRegistry` is the seam between the two surfaces: it implements + `ControlPort` (`str`), and for each call parses the address into a + `ControlAddress` and dispatches to the matched `SubstrateControlPort`. + The exception families, `Measurement` shape, and subscribe semantics are + identical to `ControlPort`; only the address type differs. + """ + + async def read(self, address: _AddressT_contra) -> Measurement: + """Read the current `Measurement` at `address`. See `ControlPort.read`.""" + ... + + async def write( + self, + address: _AddressT_contra, + value: int | float | bool | str | tuple[Any, ...], + *, + wait: bool = True, + timeout_s: float = 30.0, + ) -> None: + """Write `value` to `address`. See `ControlPort.write`.""" + ... + + def subscribe(self, address: _AddressT_contra) -> AsyncIterator[Measurement]: + """Subscribe to value changes on `address`. See `ControlPort.subscribe`.""" + ... + + __all__ = [ "ActuationKind", "ControlAccessDeniedError", + "ControlAddress", "ControlNotConnectedError", "ControlPort", "ControlTimeoutError", @@ -302,4 +371,5 @@ def subscribe(self, address: str) -> AsyncIterator[Measurement]: "MeasurementKind", "NoAdapterForAddressError", "Quality", + "SubstrateControlPort", ] diff --git a/apps/api/tests/architecture/test_port_naming_conventions.py b/apps/api/tests/architecture/test_port_naming_conventions.py index 2032b5f7640..b5b179d559e 100644 --- a/apps/api/tests/architecture/test_port_naming_conventions.py +++ b/apps/api/tests/architecture/test_port_naming_conventions.py @@ -52,6 +52,12 @@ # names why no bare role noun fits. _PORT_SUFFIX_ALLOWLIST: dict[str, str] = { "ControlPort": "value-IO seam; 'Control' alone is a bare verb, no role noun fits", + "SubstrateControlPort": ( + "the typed-address sibling of ControlPort that substrate adapters " + "implement; carries the same bare-verb 'Control' plus the 'Substrate' " + "qualifier that distinguishes the registry-facing seam from the " + "caller-facing ControlPort, so the Port suffix stays for the same reason" + ), "ComputePort": ( "compute-job submission seam; 'Compute' is a bare verb and the " "graceful role noun 'JobRunner' is reserved for the multi-substrate " diff --git a/apps/api/tests/unit/operation/test_control_address.py b/apps/api/tests/unit/operation/test_control_address.py new file mode 100644 index 00000000000..4bac2cda981 --- /dev/null +++ b/apps/api/tests/unit/operation/test_control_address.py @@ -0,0 +1,84 @@ +"""Unit tests for the typed `ControlAddress` sum + `parse_control_address`. + +Covers the str->typed dispatch the `ControlPortRegistry` performs at +route-match time: each substrate parses into its variant, per-variant +syntax validation raises `MalformedControlAddressError`, and every variant +round-trips back to its original string via `str(...)` (so logging + +`Measurement.name` breadcrumbs stay stable across the typed boundary). +""" + +from __future__ import annotations + +import pytest + +from cora.operation.ports.control_address import ( + EpicsPvAddress, + InMemoryAddress, + MalformedControlAddressError, + TangoAttributeAddress, + parse_control_address, +) + + +@pytest.mark.unit +def test_parse_epics_ca_returns_epics_pv_address() -> None: + addr = parse_control_address("epics_ca", "2bma:rot") + assert addr == EpicsPvAddress("2bma:rot") + assert str(addr) == "2bma:rot" + + +@pytest.mark.unit +def test_parse_epics_pva_returns_epics_pv_address() -> None: + """CA and PVA share the `EpicsPvAddress` variant (transport is a route decision).""" + addr = parse_control_address("epics_pva", "2bma:cam1:image") + assert addr == EpicsPvAddress("2bma:cam1:image") + + +@pytest.mark.unit +def test_parse_tango_splits_trl_into_device_and_attribute() -> None: + addr = parse_control_address("tango", "id19/bsh/1/state") + assert addr == TangoAttributeAddress(device="id19/bsh/1", attribute="state") + assert str(addr) == "id19/bsh/1/state" + + +@pytest.mark.unit +def test_parse_in_memory_returns_in_memory_address() -> None: + addr = parse_control_address("in_memory", "sim:rot") + assert addr == InMemoryAddress("sim:rot") + assert str(addr) == "sim:rot" + + +@pytest.mark.unit +def test_epics_pv_address_rejects_empty_pv() -> None: + with pytest.raises(MalformedControlAddressError) as exc_info: + parse_control_address("epics_ca", "") + assert exc_info.value.substrate == "epics_ca" + + +@pytest.mark.unit +def test_tango_address_without_attribute_segment_is_malformed() -> None: + """A TRL with no `/` has no attribute segment and cannot route (was the + adapter's `_split_trl` guard; now enforced once at the registry boundary).""" + with pytest.raises(MalformedControlAddressError) as exc_info: + parse_control_address("tango", "nodelimiter") + assert exc_info.value.raw == "nodelimiter" + assert exc_info.value.substrate == "tango" + + +@pytest.mark.unit +def test_tango_address_trailing_slash_is_malformed() -> None: + with pytest.raises(MalformedControlAddressError): + parse_control_address("tango", "id19/bsh/1/") + + +@pytest.mark.unit +def test_in_memory_address_rejects_empty() -> None: + with pytest.raises(MalformedControlAddressError): + parse_control_address("in_memory", "") + + +@pytest.mark.unit +def test_control_address_variants_are_frozen() -> None: + addr = EpicsPvAddress("2bma:rot") + with pytest.raises(AttributeError): + addr.pv = "other" # pyright: ignore[reportAttributeAccessIssue] From 2a754e8f7799a37f09f62ce15ad0b8fbbd6627ea Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 16 Jul 2026 14:51:19 +0300 Subject: [PATCH 02/10] Make the registry the str-to-typed seam and move the EPICS adapters onto it Puts the ControlAddress sum to work. The registry is the only component that can turn a raw address into a typed one, because the route table it owns is where substrate identity is declared, so it becomes the seam between the two surfaces: it implements the caller-facing `str` ControlPort, and per call it matches a route by longest prefix, parses the address per that route's declared substrate, and dispatches the typed variant to a SubstrateControlPort. The consequence for adapters is that they stop re-parsing. Each EPICS adapter now takes an EpicsPvAddress instead of splitting or validating a string on every read, write and subscribe. Parsing happens once, at the boundary, and a malformed address fails there as MalformedControlAddressError rather than surfacing mid-call. `register` therefore grows the route's substrate as a parameter: it is what selects the parse arm, and it cannot be inferred from the adapter instance. `register_str_port` is the second door, for adapters that are legitimately `str`-surfaced: InMemoryControlPort keys an opaque dict and has no substrate syntax to parse, so the registry wraps it rather than forcing a typed variant on a test double. build_control_port routes in_memory through that door and the real substrates through the typed one. The `tango` factory arm raises for now; it arrives with the adapter in the next commit, which is also where the config test's tango case lands. The Substrate literal already carries the value from the previous commit because parse_control_address dispatches on it. Co-Authored-By: Claude Opus 4.8 --- .../adapters/caproto_control_port.py | 32 ++-- .../operation/adapters/control_port_config.py | 43 +++-- .../adapters/control_port_registry.py | 144 ++++++++++++++--- .../adapters/epics_ca_control_port.py | 34 ++-- .../adapters/epics_pva_control_port.py | 40 ++--- .../integration/test_caproto_control_port.py | 45 +++--- .../integration/test_epics_ca_control_port.py | 47 +++--- .../test_epics_pva_control_port.py | 51 +++--- .../test_conduct_from_procedure_handler.py | 3 +- .../tests/unit/operation/test_conductor.py | 22 ++- .../operation/test_control_port_config.py | 19 ++- .../operation/test_control_port_registry.py | 153 ++++++++++++++---- .../test_epics_ca_control_port_acl.py | 15 +- .../test_epics_pva_control_port_acl.py | 15 +- 14 files changed, 444 insertions(+), 219 deletions(-) diff --git a/apps/api/src/cora/operation/adapters/caproto_control_port.py b/apps/api/src/cora/operation/adapters/caproto_control_port.py index e3703a9e7cc..396a839dbc0 100644 --- a/apps/api/src/cora/operation/adapters/caproto_control_port.py +++ b/apps/api/src/cora/operation/adapters/caproto_control_port.py @@ -109,6 +109,8 @@ if TYPE_CHECKING: from collections.abc import AsyncGenerator + from cora.operation.ports.control_address import EpicsPvAddress + _log = get_logger(__name__) _DISPATCH_EVENT = "controlport.dispatch" @@ -239,57 +241,59 @@ async def _connected_pv(self, address: str) -> Any: raise ControlNotConnectedError(address) from exc return pv - async def read(self, address: str) -> Measurement: - pv = await self._connected_pv(address) + async def read(self, address: EpicsPvAddress) -> Measurement: + name = address.pv + pv = await self._connected_pv(name) try: response = await pv.read(data_type="time", timeout=self._default_timeout_s) except CaprotoTimeoutError as exc: - raise ControlTimeoutError(address, self._default_timeout_s) from exc + raise ControlTimeoutError(name, self._default_timeout_s) from exc return _to_reading(response) async def write( self, - address: str, + address: EpicsPvAddress, value: int | float | bool | str | tuple[Any, ...], *, wait: bool = True, timeout_s: float = 30.0, ) -> None: + name = address.pv correlation_id = get_dispatch_correlation_id() _log.info( _DISPATCH_EVENT, - address=address, + address=name, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="started", ) try: - pv = await self._connected_pv(address) + pv = await self._connected_pv(name) await pv.write(value, wait=wait, timeout=timeout_s) except CaprotoTimeoutError as exc: _log.info( _DISPATCH_FAILED_EVENT, - address=address, + address=name, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="failed", error_class=ControlTimeoutError.__name__, ) - raise ControlTimeoutError(address, timeout_s) from exc + raise ControlTimeoutError(name, timeout_s) from exc except ErrorResponseReceived as exc: _log.info( _DISPATCH_FAILED_EVENT, - address=address, + address=name, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="failed", error_class=ControlWriteRejectedError.__name__, ) - raise ControlWriteRejectedError(address, str(exc)) from exc + raise ControlWriteRejectedError(name, str(exc)) from exc except ControlNotConnectedError: _log.info( _DISPATCH_FAILED_EVENT, - address=address, + address=name, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="failed", @@ -298,13 +302,13 @@ async def write( raise _log.info( _DISPATCH_COMPLETED_EVENT, - address=address, + address=name, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="completed", ) - def subscribe(self, address: str) -> AsyncGenerator[Measurement]: + def subscribe(self, address: EpicsPvAddress) -> AsyncGenerator[Measurement]: """Return type narrows the Protocol's `AsyncIterator` to `AsyncGenerator`. Covariant return lets tests close subscriptions via the @@ -314,7 +318,7 @@ def subscribe(self, address: str) -> AsyncGenerator[Measurement]: + connect + `pv.subscribe`) runs on the generator's first `__anext__`. """ - return self._drain(address) + return self._drain(address.pv) async def _drain(self, address: str) -> AsyncGenerator[Measurement]: pv = await self._connected_pv(address) diff --git a/apps/api/src/cora/operation/adapters/control_port_config.py b/apps/api/src/cora/operation/adapters/control_port_config.py index 2bb297edf0b..67af2c446cd 100644 --- a/apps/api/src/cora/operation/adapters/control_port_config.py +++ b/apps/api/src/cora/operation/adapters/control_port_config.py @@ -23,9 +23,9 @@ - `epics_ca`: production CA via aioca (`EpicsCaControlPort`). - `epics_pva`: production PVA via p4p (`EpicsPvaControlPort`). -Future substrates (`tango`, `opc_ua`) land as additive code edits -in `cora.infrastructure.control_port_route` (literal) plus a new -arm in `_build_substrate` here. +Future substrates (`tango`, `opc_ua`) land as additive code edits in +`cora.infrastructure.control_port_route` (literal) plus a new arm in +`_build_substrate` here. ## Lifecycle @@ -36,13 +36,14 @@ """ from collections.abc import Sequence +from typing import Any from cora.infrastructure.control_port_route import ControlPortRoute, Substrate from cora.operation.adapters.control_port_registry import ControlPortRegistry from cora.operation.adapters.epics_ca_control_port import EpicsCaControlPort from cora.operation.adapters.epics_pva_control_port import EpicsPvaControlPort from cora.operation.adapters.in_memory_control_port import InMemoryControlPort -from cora.operation.ports.control_port import ControlPort +from cora.operation.ports.control_port import ControlPort, SubstrateControlPort def build_control_port(routes: Sequence[ControlPortRoute]) -> ControlPort: @@ -51,33 +52,45 @@ def build_control_port(routes: Sequence[ControlPortRoute]) -> ControlPort: Empty routes returns a single `InMemoryControlPort` (legacy default + test convenience). Non-empty routes returns a `ControlPortRegistry` populated with the configured substrate - adapters per prefix. + adapters per prefix. `in_memory` routes go through + `register_str_port` (the registry wraps the `str`-surfaced adapter); + the real substrate adapters register directly on the typed surface. """ if not routes: return InMemoryControlPort() registry = ControlPortRegistry() for route in routes: - registry.register( - route.prefix, - _build_substrate(route.substrate), - is_simulated=route.is_simulated, - ) + if route.substrate == "in_memory": + registry.register_str_port( + route.prefix, InMemoryControlPort(), is_simulated=route.is_simulated + ) + else: + registry.register( + route.prefix, + _build_substrate(route.substrate), + route.substrate, + is_simulated=route.is_simulated, + ) return registry -def _build_substrate(substrate: Substrate) -> ControlPort: - """Construct the per-substrate adapter with deployment defaults. +def _build_substrate(substrate: Substrate) -> SubstrateControlPort[Any]: + """Construct the per-substrate typed adapter with deployment defaults. + + Handles the typed-address substrate adapters only; `in_memory` is + registered through `ControlPortRegistry.register_str_port` in + `build_control_port` because it is `str`-surfaced. Per-adapter constructor kwargs (timeouts, etc.) ride on the adapter defaults today; a future iteration may widen `ControlPortRoute` with optional per-route overrides (`timeout_s`, etc.) when a real deployment surfaces the need. """ - if substrate == "in_memory": - return InMemoryControlPort() if substrate == "epics_ca": return EpicsCaControlPort() - return EpicsPvaControlPort() + if substrate == "epics_pva": + return EpicsPvaControlPort() + raise ValueError(f"substrate {substrate!r} has no adapter wired yet") __all__ = ["build_control_port"] diff --git a/apps/api/src/cora/operation/adapters/control_port_registry.py b/apps/api/src/cora/operation/adapters/control_port_registry.py index 21e763e6ab5..b4a85a50cb9 100644 --- a/apps/api/src/cora/operation/adapters/control_port_registry.py +++ b/apps/api/src/cora/operation/adapters/control_port_registry.py @@ -1,12 +1,23 @@ """Prefix-routed `ControlPort` composite for multi-substrate deployments. -Per [[project_control_port_design]] ยง Address space and +Per [[project_control_port_design]] and [[project_control_port_generalization_research]], production -deployments will run more than one substrate adapter side by side: -2-BM's CA-only IOCs alongside detector PVs served via PVA, plus -eventually Tango device servers and OPC UA endpoints in other -facilities. The executor needs ONE `ControlPort` to talk to; the -registry is that one. +deployments run more than one substrate adapter side by side: 2-BM's +CA-only IOCs alongside detector PVs served via PVA, plus Tango device +servers (ESRF BLISS, MAX IV) and eventually OPC UA endpoints. The +executor needs ONE `ControlPort` to talk to; the registry is that one. + +## Two surfaces meet here + +The registry is the seam between the caller-facing `ControlPort` (`str` +addresses, because the Conductor cannot know a substrate from a recipe +step) and the substrate adapters' `SubstrateControlPort` (typed +`ControlAddress`). For each call it matches a route by string prefix, +parses the string into the `ControlAddress` variant the matched route's +substrate dictates (`parse_control_address`), and dispatches to the +matched `SubstrateControlPort`. A malformed address surfaces as +`MalformedControlAddressError` at this boundary, a routing miss as +`NoAdapterForAddressError`. ## Routing rule @@ -24,6 +35,15 @@ `ns=N;`). When a real deployment needs a smarter router, this class is the seam to widen, not the executor. +## In-memory routes + +`InMemoryControlPort` is `str`-surfaced (it is also the caller-facing +double used by 148 test sites), so it is not a `SubstrateControlPort`. +The registry wraps it in `_StrAddressSubstrate`, a thin shim that +unwraps a `ControlAddress` back to its string and delegates. That keeps +the in-memory adapter untouched while letting the registry's typed +dispatch treat every route uniformly. + ## Lifecycle `aclose()` calls `aclose()` on every registered adapter in @@ -45,31 +65,80 @@ import contextlib from typing import TYPE_CHECKING, Any +from cora.operation.ports.control_address import parse_control_address from cora.operation.ports.control_port import ( - ControlPort, Measurement, NoAdapterForAddressError, + SubstrateControlPort, ) if TYPE_CHECKING: from collections.abc import AsyncIterator + from cora.infrastructure.control_port_route import Substrate + from cora.operation.ports.control_address import ControlAddress + from cora.operation.ports.control_port import ControlPort + + +class _StrAddressSubstrate: + """Adapt a `str`-surfaced `ControlPort` to the typed `SubstrateControlPort`. + + Wraps `InMemoryControlPort` (and any other `str`-address port) so the + registry's typed dispatch can hold it in the same route table as the + real substrate adapters. Unwraps each `ControlAddress` to its string + via `str(...)` and delegates verbatim. + """ + + def __init__(self, inner: ControlPort) -> None: + self._inner = inner + + async def read(self, address: ControlAddress) -> Measurement: + return await self._inner.read(str(address)) + + async def write( + self, + address: ControlAddress, + value: int | float | bool | str | tuple[Any, ...], + *, + wait: bool = True, + timeout_s: float = 30.0, + ) -> None: + await self._inner.write(str(address), value, wait=wait, timeout_s=timeout_s) + + def subscribe(self, address: ControlAddress) -> AsyncIterator[Measurement]: + return self._inner.subscribe(str(address)) + + async def aclose(self) -> None: + close = getattr(self._inner, "aclose", None) + if close is not None: + await close() + class ControlPortRegistry: """Prefix-routed composite implementing `ControlPort`. - Construct empty, call `register(prefix, port)` for each substrate - route at app startup, then hand the registry to the executor as a - single `ControlPort`. The executor never sees the routing table. + Construct empty, call `register(prefix, port, substrate)` for each + substrate route at app startup, then hand the registry to the + executor as a single `ControlPort`. The executor never sees the + routing table. """ def __init__(self) -> None: - self._routes: list[tuple[str, ControlPort, bool]] = [] + self._routes: list[tuple[str, SubstrateControlPort[Any], Substrate, bool]] = [] self._closed = False - def register(self, prefix: str, port: ControlPort, *, is_simulated: bool = False) -> None: + def register( + self, + prefix: str, + port: SubstrateControlPort[Any], + substrate: Substrate, + *, + is_simulated: bool = False, + ) -> None: """Add a route. Calling with a prefix already registered replaces it. + `substrate` names the address family the registry parses `str` + addresses into for this route (`parse_control_address`). Replacement is intentional: hot-swapping a substrate adapter during integration tests should not require dropping and reconstructing the registry. @@ -78,19 +147,43 @@ def register(self, prefix: str, port: ControlPort, *, is_simulated: bool = False per deployment, not inferred from the adapter class); it feeds `route_is_simulated` and, downstream, the Dataset provenance gate. """ - self._routes = [(p, a, s) for (p, a, s) in self._routes if p != prefix] - self._routes.append((prefix, port, is_simulated)) + self._routes = [(p, a, s, sim) for (p, a, s, sim) in self._routes if p != prefix] + self._routes.append((prefix, port, substrate, is_simulated)) - def route(self, address: str) -> ControlPort: + def register_str_port( + self, + prefix: str, + port: ControlPort, + substrate: Substrate = "in_memory", + *, + is_simulated: bool = False, + ) -> None: + """Register a `str`-surfaced `ControlPort` (e.g. `InMemoryControlPort`). + + Wraps `port` in the module-private `_StrAddressSubstrate` shim so the + typed route table can hold it uniformly with the real substrate + adapters, keeping the shim an implementation detail callers never name. + `substrate` defaults to `in_memory` because that is the only + `str`-surfaced adapter today; pass another value only if a future + `str`-address adapter routes under a different parsing family. + """ + self.register(prefix, _StrAddressSubstrate(port), substrate, is_simulated=is_simulated) + + def route(self, address: str) -> SubstrateControlPort[Any]: """Return the adapter for `address` via longest-prefix-match. Raises `NoAdapterForAddressError` when no registered prefix is a prefix of `address`. The error carries the address so operators can spot the missing route from logs alone. """ - for prefix, port, _is_simulated in sorted(self._routes, key=lambda r: -len(r[0])): + port, _substrate = self._resolve(address) + return port + + def _resolve(self, address: str) -> tuple[SubstrateControlPort[Any], Substrate]: + """Longest-prefix-match returning the adapter and its substrate.""" + for prefix, port, substrate, _sim in sorted(self._routes, key=lambda r: -len(r[0])): if address.startswith(prefix): - return port + return port, substrate raise NoAdapterForAddressError(address) def route_is_simulated(self, address: str) -> bool: @@ -101,13 +194,16 @@ def route_is_simulated(self, address: str) -> bool: when no registered prefix matches, so the caller cannot mistake an unrouted address for a physical one. """ - for prefix, _port, is_simulated in sorted(self._routes, key=lambda r: -len(r[0])): + for prefix, _port, _substrate, is_simulated in sorted( + self._routes, key=lambda r: -len(r[0]) + ): if address.startswith(prefix): return is_simulated raise NoAdapterForAddressError(address) async def read(self, address: str) -> Measurement: - return await self.route(address).read(address) + port, substrate = self._resolve(address) + return await port.read(parse_control_address(substrate, address)) async def write( self, @@ -117,10 +213,14 @@ async def write( wait: bool = True, timeout_s: float = 30.0, ) -> None: - await self.route(address).write(address, value, wait=wait, timeout_s=timeout_s) + port, substrate = self._resolve(address) + await port.write( + parse_control_address(substrate, address), value, wait=wait, timeout_s=timeout_s + ) def subscribe(self, address: str) -> AsyncIterator[Measurement]: - return self.route(address).subscribe(address) + port, substrate = self._resolve(address) + return port.subscribe(parse_control_address(substrate, address)) async def aclose(self) -> None: """Close every registered adapter; idempotent. @@ -132,7 +232,7 @@ async def aclose(self) -> None: if self._closed: return self._closed = True - for _, port, _is_simulated in self._routes: + for _, port, _substrate, _is_simulated in self._routes: close = getattr(port, "aclose", None) if close is None: continue diff --git a/apps/api/src/cora/operation/adapters/epics_ca_control_port.py b/apps/api/src/cora/operation/adapters/epics_ca_control_port.py index 0765e238307..845ee0faade 100644 --- a/apps/api/src/cora/operation/adapters/epics_ca_control_port.py +++ b/apps/api/src/cora/operation/adapters/epics_ca_control_port.py @@ -135,6 +135,8 @@ if TYPE_CHECKING: from collections.abc import AsyncGenerator + from cora.operation.ports.control_address import EpicsPvAddress + _log = get_logger(__name__) _DISPATCH_EVENT = "controlport.dispatch" @@ -291,45 +293,47 @@ async def _assert_connected(self, address: str) -> None: if int(info.state) != cadef.cs_conn: raise ControlNotConnectedError(address) - async def read(self, address: str) -> Measurement: - await self._assert_connected(address) + async def read(self, address: EpicsPvAddress) -> Measurement: + pv = address.pv + await self._assert_connected(pv) try: augmented = await caget( - address, + pv, format=FORMAT_TIME, timeout=self._default_timeout_s, ) except CANothing as exc: - raise _map_ca_error(address, exc, timeout_s=self._default_timeout_s) from exc + raise _map_ca_error(pv, exc, timeout_s=self._default_timeout_s) from exc labels: tuple[str, ...] | None = None if _kind_for(augmented.datatype, augmented.element_count) == "Categorical": - labels = await self._resolve_enum_labels(address) + labels = await self._resolve_enum_labels(pv) return _to_reading(augmented, labels) async def write( self, - address: str, + address: EpicsPvAddress, value: int | float | bool | str | tuple[Any, ...], *, wait: bool = True, timeout_s: float = 30.0, ) -> None: + pv = address.pv correlation_id = get_dispatch_correlation_id() _log.info( _DISPATCH_EVENT, - address=address, + address=pv, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="started", ) try: - await self._assert_connected(address) - await caput(address, value, wait=wait, timeout=timeout_s) + await self._assert_connected(pv) + await caput(pv, value, wait=wait, timeout=timeout_s) except CANothing as exc: - mapped = _map_ca_error(address, exc, timeout_s=timeout_s) + mapped = _map_ca_error(pv, exc, timeout_s=timeout_s) _log.info( _DISPATCH_FAILED_EVENT, - address=address, + address=pv, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="failed", @@ -339,7 +343,7 @@ async def write( except ControlNotConnectedError: _log.info( _DISPATCH_FAILED_EVENT, - address=address, + address=pv, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="failed", @@ -348,13 +352,13 @@ async def write( raise _log.info( _DISPATCH_COMPLETED_EVENT, - address=address, + address=pv, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="completed", ) - def subscribe(self, address: str) -> AsyncGenerator[Measurement]: + def subscribe(self, address: EpicsPvAddress) -> AsyncGenerator[Measurement]: """Return type narrows the Protocol's `AsyncIterator` to `AsyncGenerator`. Covariant return lets tests close subscriptions via the @@ -362,7 +366,7 @@ def subscribe(self, address: str) -> AsyncGenerator[Measurement]: ports. Setup (`_assert_connected` + `camonitor`) runs on the generator's first `__anext__`. """ - return self._drain(address) + return self._drain(address.pv) async def _drain(self, address: str) -> AsyncGenerator[Measurement]: await self._assert_connected(address) diff --git a/apps/api/src/cora/operation/adapters/epics_pva_control_port.py b/apps/api/src/cora/operation/adapters/epics_pva_control_port.py index 4cd5863969d..7d30d6fab11 100644 --- a/apps/api/src/cora/operation/adapters/epics_pva_control_port.py +++ b/apps/api/src/cora/operation/adapters/epics_pva_control_port.py @@ -124,6 +124,8 @@ if TYPE_CHECKING: from collections.abc import AsyncGenerator + from cora.operation.ports.control_address import EpicsPvAddress + _log = get_logger(__name__) _DISPATCH_EVENT = "controlport.dispatch" @@ -272,31 +274,33 @@ def _ensure_context(self) -> Context: self._context = Context(provider="pva") return self._context - async def read(self, address: str) -> Measurement: + async def read(self, address: EpicsPvAddress) -> Measurement: + pv = address.pv ctx = self._ensure_context() try: value = await asyncio.wait_for( - ctx.get(address, request=_FIELD_REQUEST), + ctx.get(pv, request=_FIELD_REQUEST), timeout=self._default_timeout_s, ) except (TimeoutError, Disconnected) as exc: - raise ControlNotConnectedError(address) from exc + raise ControlNotConnectedError(pv) from exc except RemoteError as exc: - raise ControlWriteRejectedError(address, str(exc)) from exc + raise ControlWriteRejectedError(pv, str(exc)) from exc return _to_reading(value) async def write( self, - address: str, + address: EpicsPvAddress, value: int | float | bool | str | tuple[Any, ...], *, wait: bool = True, timeout_s: float = 30.0, ) -> None: + pv = address.pv correlation_id = get_dispatch_correlation_id() _log.info( _DISPATCH_EVENT, - address=address, + address=pv, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="started", @@ -304,60 +308,60 @@ async def write( ctx = self._ensure_context() try: await asyncio.wait_for( - ctx.put(address, value, wait=wait), + ctx.put(pv, value, wait=wait), timeout=timeout_s, ) except TimeoutError as exc: _log.info( _DISPATCH_FAILED_EVENT, - address=address, + address=pv, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="failed", error_class=ControlTimeoutError.__name__, ) - raise ControlTimeoutError(address, timeout_s) from exc + raise ControlTimeoutError(pv, timeout_s) from exc except Disconnected as exc: _log.info( _DISPATCH_FAILED_EVENT, - address=address, + address=pv, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="failed", error_class=ControlNotConnectedError.__name__, ) - raise ControlNotConnectedError(address) from exc + raise ControlNotConnectedError(pv) from exc except RemoteError as exc: _log.info( _DISPATCH_FAILED_EVENT, - address=address, + address=pv, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="failed", error_class=ControlWriteRejectedError.__name__, ) - raise ControlWriteRejectedError(address, str(exc)) from exc + raise ControlWriteRejectedError(pv, str(exc)) from exc except ValueError as exc: _log.info( _DISPATCH_FAILED_EVENT, - address=address, + address=pv, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="failed", error_class=ControlValueCoercionError.__name__, ) raise ControlValueCoercionError( - address, raw_type=type(value).__name__, target_kind="pva put" + pv, raw_type=type(value).__name__, target_kind="pva put" ) from exc _log.info( _DISPATCH_COMPLETED_EVENT, - address=address, + address=pv, operation="write", correlation_id=str(correlation_id) if correlation_id is not None else None, status="completed", ) - def subscribe(self, address: str) -> AsyncGenerator[Measurement]: + def subscribe(self, address: EpicsPvAddress) -> AsyncGenerator[Measurement]: """Return type narrows the Protocol's `AsyncIterator` to `AsyncGenerator`. Covariant return lets tests close subscriptions via the @@ -365,7 +369,7 @@ def subscribe(self, address: str) -> AsyncGenerator[Measurement]: Setup (`_ensure_context` + `ctx.monitor`) runs on the generator's first `__anext__`. """ - return self._drain(address) + return self._drain(address.pv) async def _drain(self, address: str) -> AsyncGenerator[Measurement]: ctx = self._ensure_context() diff --git a/apps/api/tests/integration/test_caproto_control_port.py b/apps/api/tests/integration/test_caproto_control_port.py index a0691572286..f3488c808f6 100644 --- a/apps/api/tests/integration/test_caproto_control_port.py +++ b/apps/api/tests/integration/test_caproto_control_port.py @@ -51,6 +51,7 @@ import pytest from cora.operation.adapters.caproto_control_port import CaprotoControlPort +from cora.operation.ports.control_address import EpicsPvAddress from cora.operation.ports.control_port import ( ControlNotConnectedError, ControlPort, @@ -71,8 +72,8 @@ async def test_read_double_scalar_returns_reading_with_good_quality( """DBR_DOUBLE scalar lands as Measurement(kind='Scalar', quality='Good', value=float).""" port = CaprotoControlPort() try: - await port.write(f"{softioc}double_value", 0.0, wait=True) - reading = await port.read(f"{softioc}double_value") + await port.write(EpicsPvAddress(f"{softioc}double_value"), 0.0, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}double_value")) assert isinstance(reading, Measurement) assert reading.kind == "Scalar" assert reading.quality == "Good" @@ -87,8 +88,8 @@ async def test_read_long_scalar_returns_int_value(softioc: str) -> None: """DBR_LONG scalar lands as Measurement(kind='Scalar', value=int).""" port = CaprotoControlPort() try: - await port.write(f"{softioc}long_value", 0, wait=True) - reading = await port.read(f"{softioc}long_value") + await port.write(EpicsPvAddress(f"{softioc}long_value"), 0, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}long_value")) assert reading.kind == "Scalar" assert reading.value == 0 finally: @@ -100,8 +101,8 @@ async def test_read_string_scalar_returns_decoded_utf8(softioc: str) -> None: """DBR_STRING scalar lands decoded as Python `str`, not raw `bytes`.""" port = CaprotoControlPort() try: - await port.write(f"{softioc}string_value", "initial", wait=True) - reading = await port.read(f"{softioc}string_value") + await port.write(EpicsPvAddress(f"{softioc}string_value"), "initial", wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}string_value")) assert reading.kind == "Scalar" assert reading.value == "initial" assert isinstance(reading.value, str) @@ -114,8 +115,8 @@ async def test_read_waveform_returns_array_as_tuple(softioc: str) -> None: """DBR_DOUBLE count > 1 lands as Measurement(kind='Array', value=tuple).""" port = CaprotoControlPort() try: - await port.write(f"{softioc}waveform", (1.0, 2.0, 3.0, 4.0), wait=True) - reading = await port.read(f"{softioc}waveform") + await port.write(EpicsPvAddress(f"{softioc}waveform"), (1.0, 2.0, 3.0, 4.0), wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}waveform")) assert reading.kind == "Array" assert isinstance(reading.value, tuple) assert reading.value == (1.0, 2.0, 3.0, 4.0) @@ -135,7 +136,7 @@ async def test_read_enum_returns_categorical_kind(softioc: str) -> None: """ port = CaprotoControlPort() try: - reading = await port.read(f"{softioc}enum_value") + reading = await port.read(EpicsPvAddress(f"{softioc}enum_value")) assert reading.kind == "Categorical" finally: await port.aclose() @@ -153,7 +154,7 @@ async def test_read_major_alarm_pv_returns_bad_quality(softioc: str) -> None: """ port = CaprotoControlPort() try: - reading = await port.read(f"{softioc}bad_quality_value") + reading = await port.read(EpicsPvAddress(f"{softioc}bad_quality_value")) assert reading.kind == "Scalar" assert reading.value == 99.9 assert reading.quality == "Bad" @@ -168,8 +169,8 @@ async def test_write_scalar_then_read_observes_new_value(softioc: str) -> None: """caput-callback semantics: after `wait=True` write returns, read sees new value.""" port = CaprotoControlPort() try: - await port.write(f"{softioc}double_value", 4.2, wait=True) - reading = await port.read(f"{softioc}double_value") + await port.write(EpicsPvAddress(f"{softioc}double_value"), 4.2, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}double_value")) assert reading.value == 4.2 finally: await port.aclose() @@ -180,8 +181,8 @@ async def test_write_long_then_read_observes_new_value(softioc: str) -> None: """DBR_LONG write round-trip pin: integer survives caput-callback + read.""" port = CaprotoControlPort() try: - await port.write(f"{softioc}long_value", 99, wait=True) - reading = await port.read(f"{softioc}long_value") + await port.write(EpicsPvAddress(f"{softioc}long_value"), 99, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}long_value")) assert reading.value == 99 finally: await port.aclose() @@ -192,12 +193,12 @@ async def test_subscribe_yields_initial_value_then_writes(softioc: str) -> None: """Subscribe gets the current value first, then each write fans out as a Measurement.""" port = CaprotoControlPort() try: - await port.write(f"{softioc}double_value", 0.0, wait=True) - iterator = port.subscribe(f"{softioc}double_value") + await port.write(EpicsPvAddress(f"{softioc}double_value"), 0.0, wait=True) + iterator = port.subscribe(EpicsPvAddress(f"{softioc}double_value")) first = await asyncio.wait_for(anext(iterator), timeout=2.0) assert first.value == 0.0 - await port.write(f"{softioc}double_value", 7.7, wait=True) + await port.write(EpicsPvAddress(f"{softioc}double_value"), 7.7, wait=True) second = await asyncio.wait_for(anext(iterator), timeout=2.0) assert second.value == 7.7 @@ -211,7 +212,7 @@ async def test_consumer_cancellation_runs_generator_finally(softioc: str) -> Non """Cancellation mid-`anext` runs the generator's `finally` and unregisters.""" port = CaprotoControlPort() try: - iterator = port.subscribe(f"{softioc}double_value") + iterator = port.subscribe(EpicsPvAddress(f"{softioc}double_value")) await asyncio.wait_for(anext(iterator), timeout=2.0) with pytest.raises(asyncio.TimeoutError): await asyncio.wait_for(anext(iterator), timeout=0.05) @@ -226,7 +227,7 @@ async def test_read_on_nonexistent_pv_raises_not_connected(softioc: str) -> None port = CaprotoControlPort(default_timeout_s=0.3) try: with pytest.raises(ControlNotConnectedError) as exc_info: - await port.read(f"{softioc}nonexistent") + await port.read(EpicsPvAddress(f"{softioc}nonexistent")) assert exc_info.value.address == f"{softioc}nonexistent" finally: await port.aclose() @@ -238,7 +239,7 @@ async def test_write_on_nonexistent_pv_raises_not_connected(softioc: str) -> Non port = CaprotoControlPort(default_timeout_s=0.3) try: with pytest.raises(ControlNotConnectedError): - await port.write(f"{softioc}nonexistent", 1.0) + await port.write(EpicsPvAddress(f"{softioc}nonexistent"), 1.0) finally: await port.aclose() @@ -254,7 +255,7 @@ async def test_subscribe_on_nonexistent_pv_raises_not_connected(softioc: str) -> """ port = CaprotoControlPort(default_timeout_s=0.3) try: - iterator = port.subscribe(f"{softioc}nonexistent") + iterator = port.subscribe(EpicsPvAddress(f"{softioc}nonexistent")) with pytest.raises(ControlNotConnectedError) as exc_info: await anext(iterator) assert exc_info.value.address == f"{softioc}nonexistent" @@ -267,7 +268,7 @@ async def test_subscribe_on_nonexistent_pv_raises_not_connected(softioc: str) -> async def test_aclose_is_idempotent(softioc: str) -> None: """Second aclose() call is a no-op.""" port = CaprotoControlPort() - await port.read(f"{softioc}double_value") + await port.read(EpicsPvAddress(f"{softioc}double_value")) await port.aclose() await port.aclose() assert port._context is None # pyright: ignore[reportPrivateUsage] diff --git a/apps/api/tests/integration/test_epics_ca_control_port.py b/apps/api/tests/integration/test_epics_ca_control_port.py index 0f4f4d00d08..ca776559479 100644 --- a/apps/api/tests/integration/test_epics_ca_control_port.py +++ b/apps/api/tests/integration/test_epics_ca_control_port.py @@ -47,6 +47,7 @@ import pytest from cora.operation.adapters.epics_ca_control_port import EpicsCaControlPort +from cora.operation.ports.control_address import EpicsPvAddress from cora.operation.ports.control_port import ( ControlNotConnectedError, ControlPort, @@ -67,8 +68,8 @@ async def test_read_double_scalar_returns_reading_with_good_quality( """DBR_DOUBLE scalar lands as Measurement(kind='Scalar', quality='Good', value=float).""" port = EpicsCaControlPort() try: - await port.write(f"{softioc}double_value", 0.0, wait=True) - reading = await port.read(f"{softioc}double_value") + await port.write(EpicsPvAddress(f"{softioc}double_value"), 0.0, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}double_value")) assert isinstance(reading, Measurement) assert reading.kind == "Scalar" assert reading.quality == "Good" @@ -83,8 +84,8 @@ async def test_read_long_scalar_returns_int_value(softioc: str) -> None: """DBR_LONG scalar lands as Measurement(kind='Scalar', value=int).""" port = EpicsCaControlPort() try: - await port.write(f"{softioc}long_value", 0, wait=True) - reading = await port.read(f"{softioc}long_value") + await port.write(EpicsPvAddress(f"{softioc}long_value"), 0, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}long_value")) assert reading.kind == "Scalar" assert reading.value == 0 finally: @@ -96,8 +97,8 @@ async def test_read_string_scalar_returns_decoded_utf8(softioc: str) -> None: """DBR_STRING scalar lands decoded as Python `str`, not raw `bytes`.""" port = EpicsCaControlPort() try: - await port.write(f"{softioc}string_value", "initial", wait=True) - reading = await port.read(f"{softioc}string_value") + await port.write(EpicsPvAddress(f"{softioc}string_value"), "initial", wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}string_value")) assert reading.kind == "Scalar" assert reading.value == "initial" assert isinstance(reading.value, str) @@ -110,8 +111,8 @@ async def test_read_waveform_returns_array_as_tuple(softioc: str) -> None: """DBR_DOUBLE count > 1 lands as Measurement(kind='Array', value=tuple).""" port = EpicsCaControlPort() try: - await port.write(f"{softioc}waveform", (1.0, 2.0, 3.0, 4.0), wait=True) - reading = await port.read(f"{softioc}waveform") + await port.write(EpicsPvAddress(f"{softioc}waveform"), (1.0, 2.0, 3.0, 4.0), wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}waveform")) assert reading.kind == "Array" assert isinstance(reading.value, tuple) assert reading.value == (1.0, 2.0, 3.0, 4.0) @@ -129,8 +130,8 @@ async def test_read_enum_returns_categorical_with_label(softioc: str) -> None: """ port = EpicsCaControlPort() try: - await port.write(f"{softioc}enum_value", "off", wait=True) - reading = await port.read(f"{softioc}enum_value") + await port.write(EpicsPvAddress(f"{softioc}enum_value"), "off", wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}enum_value")) assert reading.kind == "Categorical" assert reading.value == "off" finally: @@ -147,7 +148,7 @@ async def test_read_major_alarm_pv_returns_bad_quality(softioc: str) -> None: """ port = EpicsCaControlPort() try: - reading = await port.read(f"{softioc}bad_quality_value") + reading = await port.read(EpicsPvAddress(f"{softioc}bad_quality_value")) assert reading.kind == "Scalar" assert reading.value == 99.9 assert reading.quality == "Bad" @@ -162,8 +163,8 @@ async def test_write_scalar_then_read_observes_new_value(softioc: str) -> None: """caput-callback semantics: after `wait=True` write returns, read sees new value.""" port = EpicsCaControlPort() try: - await port.write(f"{softioc}double_value", 4.2, wait=True) - reading = await port.read(f"{softioc}double_value") + await port.write(EpicsPvAddress(f"{softioc}double_value"), 4.2, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}double_value")) assert reading.value == 4.2 finally: await port.aclose() @@ -174,8 +175,8 @@ async def test_write_long_then_read_observes_new_value(softioc: str) -> None: """DBR_LONG write round-trip pin: integer survives caput-callback + read.""" port = EpicsCaControlPort() try: - await port.write(f"{softioc}long_value", 99, wait=True) - reading = await port.read(f"{softioc}long_value") + await port.write(EpicsPvAddress(f"{softioc}long_value"), 99, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}long_value")) assert reading.value == 99 finally: await port.aclose() @@ -186,12 +187,12 @@ async def test_subscribe_yields_initial_value_then_writes(softioc: str) -> None: """Subscribe gets the current value first (camonitor convention), then writes fan out.""" port = EpicsCaControlPort() try: - await port.write(f"{softioc}double_value", 0.0, wait=True) - iterator = port.subscribe(f"{softioc}double_value") + await port.write(EpicsPvAddress(f"{softioc}double_value"), 0.0, wait=True) + iterator = port.subscribe(EpicsPvAddress(f"{softioc}double_value")) first = await asyncio.wait_for(anext(iterator), timeout=2.0) assert first.value == 0.0 - await port.write(f"{softioc}double_value", 7.7, wait=True) + await port.write(EpicsPvAddress(f"{softioc}double_value"), 7.7, wait=True) second = await asyncio.wait_for(anext(iterator), timeout=2.0) assert second.value == 7.7 @@ -205,7 +206,7 @@ async def test_consumer_cancellation_runs_generator_finally(softioc: str) -> Non """Cancellation mid-`anext` runs the drain generator's finally + sub.close.""" port = EpicsCaControlPort() try: - iterator = port.subscribe(f"{softioc}double_value") + iterator = port.subscribe(EpicsPvAddress(f"{softioc}double_value")) await asyncio.wait_for(anext(iterator), timeout=2.0) with pytest.raises(asyncio.TimeoutError): await asyncio.wait_for(anext(iterator), timeout=0.05) @@ -220,7 +221,7 @@ async def test_read_on_nonexistent_pv_raises_not_connected(softioc: str) -> None port = EpicsCaControlPort(default_timeout_s=0.3) try: with pytest.raises(ControlNotConnectedError) as exc_info: - await port.read(f"{softioc}nonexistent") + await port.read(EpicsPvAddress(f"{softioc}nonexistent")) assert exc_info.value.address == f"{softioc}nonexistent" finally: await port.aclose() @@ -232,7 +233,7 @@ async def test_write_on_nonexistent_pv_raises_not_connected(softioc: str) -> Non port = EpicsCaControlPort(default_timeout_s=0.3) try: with pytest.raises(ControlNotConnectedError): - await port.write(f"{softioc}nonexistent", 1.0) + await port.write(EpicsPvAddress(f"{softioc}nonexistent"), 1.0) finally: await port.aclose() @@ -248,7 +249,7 @@ async def test_subscribe_on_nonexistent_pv_raises_not_connected(softioc: str) -> """ port = EpicsCaControlPort(default_timeout_s=0.3) try: - iterator = port.subscribe(f"{softioc}nonexistent") + iterator = port.subscribe(EpicsPvAddress(f"{softioc}nonexistent")) with pytest.raises(ControlNotConnectedError) as exc_info: await anext(iterator) assert exc_info.value.address == f"{softioc}nonexistent" @@ -261,7 +262,7 @@ async def test_subscribe_on_nonexistent_pv_raises_not_connected(softioc: str) -> async def test_aclose_is_idempotent(softioc: str) -> None: """Second aclose() call is a no-op (matches Caproto + InMemory lifecycle).""" port = EpicsCaControlPort() - await port.read(f"{softioc}double_value") + await port.read(EpicsPvAddress(f"{softioc}double_value")) await port.aclose() await port.aclose() assert port._closed is True # pyright: ignore[reportPrivateUsage] diff --git a/apps/api/tests/integration/test_epics_pva_control_port.py b/apps/api/tests/integration/test_epics_pva_control_port.py index 6524a415a2a..0a3efffde8b 100644 --- a/apps/api/tests/integration/test_epics_pva_control_port.py +++ b/apps/api/tests/integration/test_epics_pva_control_port.py @@ -47,6 +47,7 @@ import pytest from cora.operation.adapters.epics_pva_control_port import EpicsPvaControlPort +from cora.operation.ports.control_address import EpicsPvAddress from cora.operation.ports.control_port import ( ControlNotConnectedError, ControlPort, @@ -67,8 +68,8 @@ async def test_read_double_scalar_returns_reading_with_good_quality( """DBR_DOUBLE via NTScalar lands as Measurement(kind='Scalar', quality='Good', value=float).""" port = EpicsPvaControlPort() try: - await port.write(f"{softioc}double_value", 0.0, wait=True) - reading = await port.read(f"{softioc}double_value") + await port.write(EpicsPvAddress(f"{softioc}double_value"), 0.0, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}double_value")) assert isinstance(reading, Measurement) assert reading.kind == "Scalar" assert reading.quality == "Good" @@ -83,8 +84,8 @@ async def test_read_long_scalar_returns_int_value(softioc: str) -> None: """DBR_LONG via NTScalar lands as Measurement(kind='Scalar', value=int).""" port = EpicsPvaControlPort() try: - await port.write(f"{softioc}long_value", 0, wait=True) - reading = await port.read(f"{softioc}long_value") + await port.write(EpicsPvAddress(f"{softioc}long_value"), 0, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}long_value")) assert reading.kind == "Scalar" assert int(reading.value) == 0 finally: @@ -96,8 +97,8 @@ async def test_read_string_scalar_returns_decoded_utf8(softioc: str) -> None: """DBR_STRING via NTScalar lands as Python `str`.""" port = EpicsPvaControlPort() try: - await port.write(f"{softioc}string_value", "initial", wait=True) - reading = await port.read(f"{softioc}string_value") + await port.write(EpicsPvAddress(f"{softioc}string_value"), "initial", wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}string_value")) assert reading.kind == "Scalar" assert reading.value == "initial" finally: @@ -109,8 +110,8 @@ async def test_read_waveform_returns_array_as_tuple(softioc: str) -> None: """DBR_DOUBLE count > 1 via NTScalarArray lands as Measurement(kind='Array', value=tuple).""" port = EpicsPvaControlPort() try: - await port.write(f"{softioc}waveform", (1.0, 2.0, 3.0, 4.0), wait=True) - reading = await port.read(f"{softioc}waveform") + await port.write(EpicsPvAddress(f"{softioc}waveform"), (1.0, 2.0, 3.0, 4.0), wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}waveform")) assert reading.kind == "Array" assert isinstance(reading.value, tuple) assert reading.value == (1.0, 2.0, 3.0, 4.0) @@ -128,8 +129,8 @@ async def test_read_enum_returns_categorical_with_label(softioc: str) -> None: """ port = EpicsPvaControlPort() try: - await port.write(f"{softioc}enum_value", 0, wait=True) - reading = await port.read(f"{softioc}enum_value") + await port.write(EpicsPvAddress(f"{softioc}enum_value"), 0, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}enum_value")) assert reading.kind == "Categorical" assert reading.value == "off" finally: @@ -156,8 +157,8 @@ async def test_read_image_returns_image_kind_with_3x2_shape(softioc: str) -> Non """ port = EpicsPvaControlPort() try: - await port.write(f"{softioc}image:data", (1, 2, 3, 4, 5, 6), wait=True) - reading = await port.read(f"{softioc}image") + await port.write(EpicsPvAddress(f"{softioc}image:data"), (1, 2, 3, 4, 5, 6), wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}image")) assert reading.kind == "Image" assert reading.quality == "Good" assert reading.produced_at.tzinfo is not None @@ -176,7 +177,7 @@ async def test_read_major_alarm_pv_returns_bad_quality(softioc: str) -> None: """ port = EpicsPvaControlPort() try: - reading = await port.read(f"{softioc}bad_quality_value") + reading = await port.read(EpicsPvAddress(f"{softioc}bad_quality_value")) assert reading.kind == "Scalar" assert float(reading.value) == 99.9 assert reading.quality == "Bad" @@ -191,8 +192,8 @@ async def test_write_scalar_then_read_observes_new_value(softioc: str) -> None: """PVA put-with-wait semantics: after `wait=True` write returns, read sees new value.""" port = EpicsPvaControlPort() try: - await port.write(f"{softioc}double_value", 4.2, wait=True) - reading = await port.read(f"{softioc}double_value") + await port.write(EpicsPvAddress(f"{softioc}double_value"), 4.2, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}double_value")) assert reading.value == 4.2 finally: await port.aclose() @@ -203,8 +204,8 @@ async def test_write_long_then_read_observes_new_value(softioc: str) -> None: """DBR_LONG via PVA put-with-wait round-trip.""" port = EpicsPvaControlPort() try: - await port.write(f"{softioc}long_value", 99, wait=True) - reading = await port.read(f"{softioc}long_value") + await port.write(EpicsPvAddress(f"{softioc}long_value"), 99, wait=True) + reading = await port.read(EpicsPvAddress(f"{softioc}long_value")) assert int(reading.value) == 99 finally: await port.aclose() @@ -215,12 +216,12 @@ async def test_subscribe_yields_initial_value_then_writes(softioc: str) -> None: """Subscribe gets the current value first, then each write fans out as a Measurement.""" port = EpicsPvaControlPort() try: - await port.write(f"{softioc}double_value", 0.0, wait=True) - iterator = port.subscribe(f"{softioc}double_value") + await port.write(EpicsPvAddress(f"{softioc}double_value"), 0.0, wait=True) + iterator = port.subscribe(EpicsPvAddress(f"{softioc}double_value")) first = await asyncio.wait_for(anext(iterator), timeout=2.0) assert first.value == 0.0 - await port.write(f"{softioc}double_value", 7.7, wait=True) + await port.write(EpicsPvAddress(f"{softioc}double_value"), 7.7, wait=True) second = await asyncio.wait_for(anext(iterator), timeout=2.0) assert second.value == 7.7 @@ -234,7 +235,7 @@ async def test_consumer_cancellation_runs_generator_finally(softioc: str) -> Non """Cancellation mid-`anext` runs the drain generator's finally + sub.close().""" port = EpicsPvaControlPort() try: - iterator = port.subscribe(f"{softioc}double_value") + iterator = port.subscribe(EpicsPvAddress(f"{softioc}double_value")) await asyncio.wait_for(anext(iterator), timeout=2.0) with pytest.raises(asyncio.TimeoutError): await asyncio.wait_for(anext(iterator), timeout=0.05) @@ -249,7 +250,7 @@ async def test_read_on_nonexistent_pv_raises_not_connected(softioc: str) -> None port = EpicsPvaControlPort(default_timeout_s=0.3) try: with pytest.raises(ControlNotConnectedError) as exc_info: - await port.read(f"{softioc}nonexistent") + await port.read(EpicsPvAddress(f"{softioc}nonexistent")) assert exc_info.value.address == f"{softioc}nonexistent" finally: await port.aclose() @@ -276,7 +277,7 @@ async def test_write_on_nonexistent_pv_raises_not_connected(softioc: str) -> Non from cora.operation.ports.control_port import ControlTimeoutError with pytest.raises((ControlTimeoutError, ControlNotConnectedError)) as exc_info: - await port.write(f"{softioc}nonexistent", 1.0) + await port.write(EpicsPvAddress(f"{softioc}nonexistent"), 1.0) assert exc_info.value.address == f"{softioc}nonexistent" finally: await port.aclose() @@ -292,7 +293,7 @@ async def test_subscribe_on_nonexistent_pv_raises_not_connected(softioc: str) -> """ port = EpicsPvaControlPort(default_timeout_s=0.3) try: - iterator = port.subscribe(f"{softioc}nonexistent") + iterator = port.subscribe(EpicsPvAddress(f"{softioc}nonexistent")) with pytest.raises(asyncio.TimeoutError): await asyncio.wait_for(anext(iterator), timeout=0.3) await iterator.aclose() @@ -304,7 +305,7 @@ async def test_subscribe_on_nonexistent_pv_raises_not_connected(softioc: str) -> async def test_aclose_is_idempotent(softioc: str) -> None: """Second aclose() call is a no-op.""" port = EpicsPvaControlPort() - await port.read(f"{softioc}double_value") + await port.read(EpicsPvAddress(f"{softioc}double_value")) await port.aclose() await port.aclose() assert port._closed is True # pyright: ignore[reportPrivateUsage] diff --git a/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py b/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py index ff7889f2dce..4f44a8da6bb 100644 --- a/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py +++ b/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py @@ -420,7 +420,8 @@ async def test_conduct_from_folds_pre_hold_actuation_kind_into_completion() -> N inner = InMemoryControlPort() inner.simulate_connect("real:a") registry = ControlPortRegistry() - registry.register("real:", inner, is_simulated=False) # the replay tail is physical + # the replay tail is physical + registry.register_str_port("real:", inner, is_simulated=False) await _seed_held_with_steps( store, steps=(SetpointStep(address="real:a", value=1.0),), diff --git a/apps/api/tests/unit/operation/test_conductor.py b/apps/api/tests/unit/operation/test_conductor.py index 9b8a39b5eef..1168703af03 100644 --- a/apps/api/tests/unit/operation/test_conductor.py +++ b/apps/api/tests/unit/operation/test_conductor.py @@ -1561,10 +1561,8 @@ async def test_conduct_reraises_concurrency_error_from_complete_procedure() -> N @pytest.mark.unit async def test_execute_setpoint_via_registry_with_unrouted_address_records_failure() -> None: """NoAdapterForAddressError now lives in _CONTROL_ERRORS; records + halts cleanly.""" - from cora.operation.adapters.control_port_registry import ControlPortRegistry - registry = ControlPortRegistry() - registry.register("known:", InMemoryControlPort()) + registry.register_str_port("known:", InMemoryControlPort()) appender = _FakeAppendStep() conductor = Conductor( control_port=registry, @@ -1597,7 +1595,7 @@ async def test_actuation_kind_is_physical_when_route_not_simulated() -> None: inner = InMemoryControlPort() inner.simulate_connect("2bma:rot:val") registry = ControlPortRegistry() - registry.register("2bma:", inner, is_simulated=False) + registry.register_str_port("2bma:", inner, is_simulated=False) appender = _FakeAppendStep() conductor = _conductor(registry, appender, ids=[uuid4()]) result = await conductor.execute( @@ -1621,7 +1619,7 @@ async def test_actuation_kind_is_simulated_when_route_is_simulated() -> None: inner = InMemoryControlPort() inner.simulate_connect("sim:rot:val") registry = ControlPortRegistry() - registry.register("sim:", inner, is_simulated=True) + registry.register_str_port("sim:", inner, is_simulated=True) appender = _FakeAppendStep() conductor = _conductor(registry, appender, ids=[uuid4()]) result = await conductor.execute( @@ -1642,8 +1640,8 @@ async def test_actuation_kind_is_hybrid_when_conduct_touches_both() -> None: real = InMemoryControlPort() real.simulate_connect("real:m1") registry = ControlPortRegistry() - registry.register("sim:", sim, is_simulated=True) - registry.register("real:", real, is_simulated=False) + registry.register_str_port("sim:", sim, is_simulated=True) + registry.register_str_port("real:", real, is_simulated=False) appender = _FakeAppendStep() conductor = _conductor(registry, appender, ids=[uuid4(), uuid4()]) result = await conductor.execute( @@ -1684,7 +1682,7 @@ async def test_actuation_kind_is_none_for_bare_port_without_routing_table() -> N async def test_actuation_kind_is_none_when_no_step_touches_control_port() -> None: """A conduct that drives nothing (empty steps) records no kind.""" registry = ControlPortRegistry() - registry.register("2bma:", InMemoryControlPort(), is_simulated=True) + registry.register_str_port("2bma:", InMemoryControlPort(), is_simulated=True) appender = _FakeAppendStep() conductor = _conductor(registry, appender) result = await conductor.execute( @@ -1703,7 +1701,7 @@ async def test_actuation_kind_is_simulated_when_action_body_drives_simulated_rou inner = InMemoryControlPort() inner.simulate_connect("sim:m1") control = ControlPortRegistry() - control.register("sim:", inner, is_simulated=True) + control.register_str_port("sim:", inner, is_simulated=True) async def drive(ctx: ActionContext) -> Mapping[str, Any]: await ctx.control_port.write("sim:m1", 1.0) @@ -1732,7 +1730,7 @@ async def test_actuation_kind_is_simulated_when_setpoint_write_fails_on_simulate conduct: the kind reflects routes attempted, not only succeeded.""" inner = InMemoryControlPort() # never connected -> write raises control = ControlPortRegistry() - control.register("sim:", inner, is_simulated=True) + control.register_str_port("sim:", inner, is_simulated=True) appender = _FakeAppendStep() conductor = _conductor(control, appender, ids=[uuid4()]) result = await conductor.execute( @@ -1756,7 +1754,7 @@ async def test_conduct_threads_simulated_kind_into_complete_command() -> None: inner = InMemoryControlPort() inner.simulate_connect("sim:rot:val") registry = ControlPortRegistry() - registry.register("sim:", inner, is_simulated=True) + registry.register_str_port("sim:", inner, is_simulated=True) appender = _FakeAppendStep() start = _FakeLifecycleHandler() complete = _FakeLifecycleHandler() @@ -1783,7 +1781,7 @@ async def test_conduct_threads_kind_into_abort_command_on_execute_failure() -> N data; routes attempted before the failing step taint it).""" inner = InMemoryControlPort() # never connected -> the write fails registry = ControlPortRegistry() - registry.register("sim:", inner, is_simulated=True) + registry.register_str_port("sim:", inner, is_simulated=True) appender = _FakeAppendStep() start = _FakeLifecycleHandler() complete = _FakeLifecycleHandler() diff --git a/apps/api/tests/unit/operation/test_control_port_config.py b/apps/api/tests/unit/operation/test_control_port_config.py index 33674f946b7..e9fc38d8b1f 100644 --- a/apps/api/tests/unit/operation/test_control_port_config.py +++ b/apps/api/tests/unit/operation/test_control_port_config.py @@ -23,6 +23,10 @@ from cora.operation.adapters.epics_ca_control_port import EpicsCaControlPort from cora.operation.adapters.epics_pva_control_port import EpicsPvaControlPort from cora.operation.adapters.in_memory_control_port import InMemoryControlPort +from cora.operation.ports.control_port import ( + ControlNotConnectedError, + NoAdapterForAddressError, +) @pytest.mark.unit @@ -32,11 +36,18 @@ def test_build_control_port_with_empty_routes_returns_in_memory_port() -> None: @pytest.mark.unit -def test_build_control_port_with_single_in_memory_route_returns_registry() -> None: +async def test_build_control_port_with_single_in_memory_route_returns_registry() -> None: port = build_control_port([ControlPortRoute(prefix="2bma:", substrate="in_memory")]) assert isinstance(port, ControlPortRegistry) - routed = port.route("2bma:rot:val") - assert isinstance(routed, InMemoryControlPort) + # in_memory routes ride the registry's str-port wrapper (an internal + # detail), so assert behaviour rather than the wrapper type: a matched + # address dispatches to the in-memory adapter, which raises + # ControlNotConnectedError (its response for an unseeded address), NOT the + # NoAdapterForAddressError an unrouted prefix would raise. + with pytest.raises(ControlNotConnectedError): + await port.read("2bma:rot:val") + with pytest.raises(NoAdapterForAddressError): + await port.read("7bma:rot:val") @pytest.mark.unit @@ -80,7 +91,7 @@ def test_control_port_route_rejects_empty_prefix() -> None: @pytest.mark.unit def test_control_port_route_rejects_unknown_substrate() -> None: with pytest.raises(ValidationError): - ControlPortRoute.model_validate({"prefix": "x:", "substrate": "tango"}) + ControlPortRoute.model_validate({"prefix": "x:", "substrate": "opc_ua"}) @pytest.mark.unit diff --git a/apps/api/tests/unit/operation/test_control_port_registry.py b/apps/api/tests/unit/operation/test_control_port_registry.py index d3914da22e0..c8a97c3949a 100644 --- a/apps/api/tests/unit/operation/test_control_port_registry.py +++ b/apps/api/tests/unit/operation/test_control_port_registry.py @@ -2,14 +2,34 @@ Coverage pins the longest-prefix-match invariant + the NoAdapterForAddressError surface + read/write/subscribe pass-through -+ aclose fan-out idempotency. The unit tier uses InMemoryControlPort -on both routes so the test stays on the unit-tier pyramid. ++ aclose fan-out idempotency, plus the str->typed parse the registry now +performs at the seam between the caller-facing `ControlPort` (`str`) and +the substrate adapters' `SubstrateControlPort` (typed `ControlAddress`). + +Two registration paths are exercised: + + - `register_str_port` for `str`-surfaced adapters (`InMemoryControlPort`): + the registry wraps them internally, so these tests assert behaviour + (round-trip reads, aclose fan-out) rather than the private wrapper type. + - `register` for typed `SubstrateControlPort` adapters: a `_FakeSubstrate` + stands in for the routing / simulated-flag / typed-parse assertions and + records the `ControlAddress` a route actually receives. """ +from __future__ import annotations + +from datetime import UTC, datetime +from typing import TYPE_CHECKING, Any + import pytest from cora.operation.adapters.control_port_registry import ControlPortRegistry from cora.operation.adapters.in_memory_control_port import InMemoryControlPort +from cora.operation.ports.control_address import ( + ControlAddress, + EpicsPvAddress, + TangoAttributeAddress, +) from cora.operation.ports.control_port import ( ActuationKind, ControlPort, @@ -17,10 +37,11 @@ NoAdapterForAddressError, ) +if TYPE_CHECKING: + from collections.abc import AsyncIterator -def _reading(value: float, kind: str = "Scalar") -> Measurement: - from datetime import UTC, datetime +def _reading(value: float, kind: str = "Scalar") -> Measurement: return Measurement( value=value, kind=kind, # type: ignore[arg-type] @@ -29,6 +50,41 @@ def _reading(value: float, kind: str = "Scalar") -> Measurement: ) +class _FakeSubstrate: + """A minimal `SubstrateControlPort` that records the typed address it receives. + + Stands in for a real substrate adapter in routing / simulated-flag / + typed-parse tests, so they assert on the registry alone (no dependency on + the private in-memory shim or a live substrate). + """ + + def __init__(self) -> None: + self.last_address: ControlAddress | None = None + self.closed = False + + async def read(self, address: ControlAddress) -> Measurement: + self.last_address = address + return _reading(1.0) + + async def write( + self, + address: ControlAddress, + value: int | float | bool | str | tuple[Any, ...], + *, + wait: bool = True, + timeout_s: float = 30.0, + ) -> None: + _ = value + self.last_address = address + + def subscribe(self, address: ControlAddress) -> AsyncIterator[Measurement]: + self.last_address = address + raise NotImplementedError + + async def aclose(self) -> None: + self.closed = True + + @pytest.mark.unit def test_registry_satisfies_control_port_protocol() -> None: """An empty registry is still a `ControlPort` per `@runtime_checkable`.""" @@ -38,7 +94,7 @@ def test_registry_satisfies_control_port_protocol() -> None: @pytest.mark.unit def test_route_raises_no_adapter_when_no_prefix_matches() -> None: registry = ControlPortRegistry() - registry.register("7bma:", InMemoryControlPort()) + registry.register("7bma:", _FakeSubstrate(), "epics_ca") with pytest.raises(NoAdapterForAddressError) as exc_info: registry.route("2bma:rot:rbv") assert exc_info.value.address == "2bma:rot:rbv" @@ -47,8 +103,8 @@ def test_route_raises_no_adapter_when_no_prefix_matches() -> None: @pytest.mark.unit def test_route_returns_matching_adapter() -> None: registry = ControlPortRegistry() - aps = InMemoryControlPort() - registry.register("2bma:", aps) + aps = _FakeSubstrate() + registry.register("2bma:", aps, "epics_ca") assert registry.route("2bma:rot:rbv") is aps @@ -61,10 +117,10 @@ def test_route_picks_longest_matching_prefix_not_registration_order() -> None: Longest-match makes the routing decision deterministic. """ registry = ControlPortRegistry() - general = InMemoryControlPort() - specific = InMemoryControlPort() - registry.register("2bma:", general) - registry.register("2bma:cam:", specific) + general = _FakeSubstrate() + specific = _FakeSubstrate() + registry.register("2bma:", general, "epics_ca") + registry.register("2bma:cam:", specific, "epics_ca") assert registry.route("2bma:cam:image") is specific assert registry.route("2bma:rot:rbv") is general @@ -73,10 +129,10 @@ def test_route_picks_longest_matching_prefix_not_registration_order() -> None: def test_register_replaces_prior_route_for_same_prefix() -> None: """Re-registering a prefix replaces the prior adapter (hot-swap).""" registry = ControlPortRegistry() - old = InMemoryControlPort() - new = InMemoryControlPort() - registry.register("2bma:", old) - registry.register("2bma:", new) + old = _FakeSubstrate() + new = _FakeSubstrate() + registry.register("2bma:", old, "epics_ca") + registry.register("2bma:", new, "epics_ca") assert registry.route("2bma:rot:rbv") is new @@ -85,7 +141,7 @@ async def test_read_dispatches_to_routed_adapter() -> None: registry = ControlPortRegistry() port = InMemoryControlPort() port.set_reading("2bma:rot:rbv", _reading(1.5)) - registry.register("2bma:", port) + registry.register_str_port("2bma:", port) got = await registry.read("2bma:rot:rbv") assert got.value == 1.5 @@ -95,7 +151,7 @@ async def test_write_dispatches_to_routed_adapter() -> None: registry = ControlPortRegistry() port = InMemoryControlPort() port.simulate_connect("2bma:rot:val") - registry.register("2bma:", port) + registry.register_str_port("2bma:", port) await registry.write("2bma:rot:val", 3.14) assert (await port.read("2bma:rot:val")).value == 3.14 @@ -105,7 +161,7 @@ async def test_subscribe_dispatches_to_routed_adapter() -> None: registry = ControlPortRegistry() port = InMemoryControlPort() port.set_reading("2bma:rot:rbv", _reading(0.0)) - registry.register("2bma:", port) + registry.register_str_port("2bma:", port) iterator = registry.subscribe("2bma:rot:rbv") port.set_reading("2bma:rot:rbv", _reading(2.0)) got = await anext(iterator) @@ -113,12 +169,41 @@ async def test_subscribe_dispatches_to_routed_adapter() -> None: await iterator.aclose() # type: ignore[attr-defined] # InMemoryControlPort returns AsyncGenerator +@pytest.mark.unit +async def test_read_parses_epics_address_from_route_substrate() -> None: + """An `epics_ca` route receives an `EpicsPvAddress` parsed from the string.""" + registry = ControlPortRegistry() + recorder = _FakeSubstrate() + registry.register("2bma:", recorder, "epics_ca") + await registry.read("2bma:rot:rbv") + assert recorder.last_address == EpicsPvAddress("2bma:rot:rbv") + + +@pytest.mark.unit +async def test_read_parses_tango_address_from_route_substrate() -> None: + """A `tango` route receives a `TangoAttributeAddress` split from the TRL.""" + registry = ControlPortRegistry() + recorder = _FakeSubstrate() + registry.register("id19/", recorder, "tango") + await registry.read("id19/bsh/1/state") + assert recorder.last_address == TangoAttributeAddress(device="id19/bsh/1", attribute="state") + + +@pytest.mark.unit +async def test_write_parses_typed_address_from_route_substrate() -> None: + registry = ControlPortRegistry() + recorder = _FakeSubstrate() + registry.register("2bma:", recorder, "epics_pva") + await registry.write("2bma:cam:image", (1, 2, 3)) + assert recorder.last_address == EpicsPvAddress("2bma:cam:image") + + @pytest.mark.unit async def test_aclose_closes_every_registered_adapter() -> None: registry = ControlPortRegistry() a, b = InMemoryControlPort(), InMemoryControlPort() - registry.register("2bma:", a) - registry.register("7bma:", b) + registry.register_str_port("2bma:", a) + registry.register_str_port("7bma:", b) await registry.aclose() assert a._closed is True # pyright: ignore[reportPrivateUsage] assert b._closed is True # pyright: ignore[reportPrivateUsage] @@ -127,7 +212,7 @@ async def test_aclose_closes_every_registered_adapter() -> None: @pytest.mark.unit async def test_aclose_is_idempotent() -> None: registry = ControlPortRegistry() - registry.register("2bma:", InMemoryControlPort()) + registry.register_str_port("2bma:", InMemoryControlPort()) await registry.aclose() await registry.aclose() # no-op @@ -137,20 +222,20 @@ async def test_aclose_continues_when_one_adapter_raises() -> None: """A flaky adapter cannot strand its siblings: registry suppresses errors.""" class _Boom: - async def read(self, _address: str) -> Measurement: # pragma: no cover + async def read(self, _address: ControlAddress) -> Measurement: # pragma: no cover raise NotImplementedError async def write( self, - _address: str, - _value: object, + _address: ControlAddress, + value: object, *, wait: bool = True, timeout_s: float = 30.0, ) -> None: # pragma: no cover raise NotImplementedError - def subscribe(self, _address: str) -> object: # pragma: no cover + def subscribe(self, _address: ControlAddress) -> object: # pragma: no cover raise NotImplementedError async def aclose(self) -> None: @@ -158,8 +243,8 @@ async def aclose(self) -> None: registry = ControlPortRegistry() survivor = InMemoryControlPort() - registry.register("flaky:", _Boom()) # type: ignore[arg-type] - registry.register("ok:", survivor) + registry.register("flaky:", _Boom(), "epics_ca") # type: ignore[arg-type] + registry.register_str_port("ok:", survivor) await registry.aclose() assert survivor._closed is True # pyright: ignore[reportPrivateUsage] @@ -168,14 +253,14 @@ async def aclose(self) -> None: def test_route_is_simulated_defaults_false() -> None: """A route registered without the flag is physical by default.""" registry = ControlPortRegistry() - registry.register("2bma:", InMemoryControlPort()) + registry.register_str_port("2bma:", InMemoryControlPort()) assert registry.route_is_simulated("2bma:rot:rbv") is False @pytest.mark.unit def test_route_is_simulated_returns_declared_flag() -> None: registry = ControlPortRegistry() - registry.register("sim:", InMemoryControlPort(), is_simulated=True) + registry.register_str_port("sim:", InMemoryControlPort(), is_simulated=True) assert registry.route_is_simulated("sim:rot:rbv") is True @@ -188,8 +273,8 @@ def test_route_is_simulated_uses_longest_prefix_match() -> None: simulated sub-band out of an otherwise live crate. """ registry = ControlPortRegistry() - registry.register("2bma:", InMemoryControlPort(), is_simulated=False) - registry.register("2bma:sim:", InMemoryControlPort(), is_simulated=True) + registry.register_str_port("2bma:", InMemoryControlPort(), is_simulated=False) + registry.register_str_port("2bma:sim:", InMemoryControlPort(), is_simulated=True) assert registry.route_is_simulated("2bma:sim:rot") is True assert registry.route_is_simulated("2bma:rot:rbv") is False @@ -198,7 +283,7 @@ def test_route_is_simulated_uses_longest_prefix_match() -> None: def test_route_is_simulated_raises_when_no_prefix_matches() -> None: """An unrouted address is an error, never silently treated as physical.""" registry = ControlPortRegistry() - registry.register("7bma:", InMemoryControlPort(), is_simulated=True) + registry.register_str_port("7bma:", InMemoryControlPort(), is_simulated=True) with pytest.raises(NoAdapterForAddressError) as exc_info: registry.route_is_simulated("2bma:rot:rbv") assert exc_info.value.address == "2bma:rot:rbv" @@ -208,8 +293,8 @@ def test_route_is_simulated_raises_when_no_prefix_matches() -> None: def test_register_replacement_preserves_new_simulated_flag() -> None: """Re-registering a prefix replaces both the adapter and its flag.""" registry = ControlPortRegistry() - registry.register("2bma:", InMemoryControlPort(), is_simulated=False) - registry.register("2bma:", InMemoryControlPort(), is_simulated=True) + registry.register_str_port("2bma:", InMemoryControlPort(), is_simulated=False) + registry.register_str_port("2bma:", InMemoryControlPort(), is_simulated=True) assert registry.route_is_simulated("2bma:rot:rbv") is True diff --git a/apps/api/tests/unit/operation/test_epics_ca_control_port_acl.py b/apps/api/tests/unit/operation/test_epics_ca_control_port_acl.py index 7e81b694c3e..3b8958fa5fe 100644 --- a/apps/api/tests/unit/operation/test_epics_ca_control_port_acl.py +++ b/apps/api/tests/unit/operation/test_epics_ca_control_port_acl.py @@ -31,6 +31,7 @@ from epicscorelibs.ca import cadef from cora.operation.adapters.epics_ca_control_port import EpicsCaControlPort +from cora.operation.ports.control_address import EpicsPvAddress from cora.operation.ports.control_port import ( ControlAccessDeniedError, ControlNotConnectedError, @@ -84,7 +85,7 @@ async def test_read_translates_eca_timeout_to_control_timeout_error( monkeypatch.setattr("cora.operation.adapters.epics_ca_control_port.caget", _raise_timeout) port = EpicsCaControlPort(default_timeout_s=0.5) with pytest.raises(ControlTimeoutError) as exc_info: - await port.read("test_pv") + await port.read(EpicsPvAddress("test_pv")) assert exc_info.value.address == "test_pv" assert exc_info.value.timeout_s == 0.5 @@ -98,7 +99,7 @@ async def test_write_translates_eca_timeout_to_control_timeout_error( monkeypatch.setattr("cora.operation.adapters.epics_ca_control_port.caput", _raise_timeout) port = EpicsCaControlPort() with pytest.raises(ControlTimeoutError) as exc_info: - await port.write("test_pv", 1.0, timeout_s=2.5) + await port.write(EpicsPvAddress("test_pv"), 1.0, timeout_s=2.5) assert exc_info.value.address == "test_pv" assert exc_info.value.timeout_s == 2.5 @@ -111,7 +112,7 @@ async def test_cainfo_eca_disconn_translates_to_not_connected( monkeypatch.setattr("cora.operation.adapters.epics_ca_control_port.cainfo", _raise_disconn) port = EpicsCaControlPort() with pytest.raises(ControlNotConnectedError) as exc_info: - await port.read("test_pv") + await port.read(EpicsPvAddress("test_pv")) assert exc_info.value.address == "test_pv" @@ -126,7 +127,7 @@ async def test_write_translates_other_errorcode_to_write_rejected( ) port = EpicsCaControlPort() with pytest.raises(ControlWriteRejectedError) as exc_info: - await port.write("test_pv", 1.0) + await port.write(EpicsPvAddress("test_pv"), 1.0) assert exc_info.value.address == "test_pv" assert "999" in exc_info.value.reason @@ -145,7 +146,7 @@ async def test_read_translates_eca_nordaccess_to_access_denied( monkeypatch.setattr("cora.operation.adapters.epics_ca_control_port.caget", _raise_nordaccess) port = EpicsCaControlPort() with pytest.raises(ControlAccessDeniedError) as exc_info: - await port.read("test_pv") + await port.read(EpicsPvAddress("test_pv")) assert exc_info.value.address == "test_pv" @@ -158,7 +159,7 @@ async def test_write_translates_eca_nowtaccess_to_access_denied( monkeypatch.setattr("cora.operation.adapters.epics_ca_control_port.caput", _raise_nowtaccess) port = EpicsCaControlPort() with pytest.raises(ControlAccessDeniedError) as exc_info: - await port.write("test_pv", 1.0) + await port.write(EpicsPvAddress("test_pv"), 1.0) assert exc_info.value.address == "test_pv" @@ -193,7 +194,7 @@ def _fake_camonitor(_address: str, callback: Any, **_kwargs: Any) -> _FakeSubscr monkeypatch.setattr("cora.operation.adapters.epics_ca_control_port.cainfo", _ok_cainfo) monkeypatch.setattr("cora.operation.adapters.epics_ca_control_port.camonitor", _fake_camonitor) port = EpicsCaControlPort() - iterator = port.subscribe("test_pv") + iterator = port.subscribe(EpicsPvAddress("test_pv")) with pytest.raises(ControlNotConnectedError) as exc_info: await anext(iterator) assert exc_info.value.address == "test_pv" diff --git a/apps/api/tests/unit/operation/test_epics_pva_control_port_acl.py b/apps/api/tests/unit/operation/test_epics_pva_control_port_acl.py index 38719c930dd..04888070be4 100644 --- a/apps/api/tests/unit/operation/test_epics_pva_control_port_acl.py +++ b/apps/api/tests/unit/operation/test_epics_pva_control_port_acl.py @@ -33,6 +33,7 @@ from p4p.client.asyncio import Disconnected, RemoteError from cora.operation.adapters.epics_pva_control_port import EpicsPvaControlPort +from cora.operation.ports.control_address import EpicsPvAddress from cora.operation.ports.control_port import ( ControlNotConnectedError, ControlTimeoutError, @@ -95,7 +96,7 @@ async def test_read_translates_asyncio_timeout_to_not_connected() -> None: port = EpicsPvaControlPort(default_timeout_s=0.05) _hijack_context(port, _FakeContext(get_raises=asyncio.TimeoutError)) with pytest.raises(ControlNotConnectedError) as exc_info: - await port.read("test_pv") + await port.read(EpicsPvAddress("test_pv")) assert exc_info.value.address == "test_pv" @@ -105,7 +106,7 @@ async def test_read_translates_disconnected_to_not_connected() -> None: port = EpicsPvaControlPort() _hijack_context(port, _FakeContext(get_raises=Disconnected)) with pytest.raises(ControlNotConnectedError) as exc_info: - await port.read("test_pv") + await port.read(EpicsPvAddress("test_pv")) assert exc_info.value.address == "test_pv" @@ -119,7 +120,7 @@ async def test_write_translates_asyncio_timeout_to_control_timeout_error() -> No port = EpicsPvaControlPort() _hijack_context(port, _FakeContext(put_raises=asyncio.TimeoutError)) with pytest.raises(ControlTimeoutError) as exc_info: - await port.write("test_pv", 1.0, timeout_s=0.05) + await port.write(EpicsPvAddress("test_pv"), 1.0, timeout_s=0.05) assert exc_info.value.address == "test_pv" assert exc_info.value.timeout_s == 0.05 @@ -130,7 +131,7 @@ async def test_write_translates_remote_error_to_write_rejected() -> None: port = EpicsPvaControlPort() _hijack_context(port, _FakeContext(put_raises=RemoteError, raise_value="IOC says no")) with pytest.raises(ControlWriteRejectedError) as exc_info: - await port.write("test_pv", 1.0) + await port.write(EpicsPvAddress("test_pv"), 1.0) assert exc_info.value.address == "test_pv" assert "IOC says no" in exc_info.value.reason @@ -144,7 +145,7 @@ async def test_write_translates_value_error_to_value_coercion() -> None: _FakeContext(put_raises=ValueError, raise_value="cannot coerce 'foo' to int"), ) with pytest.raises(ControlValueCoercionError) as exc_info: - await port.write("test_pv", "foo") + await port.write(EpicsPvAddress("test_pv"), "foo") assert exc_info.value.address == "test_pv" @@ -214,7 +215,7 @@ async def test_subscribe_mid_stream_disconnect_raises_through_iterator() -> None """ port = EpicsPvaControlPort() _hijack_context(port, _MonitorContext(_FakeValue(1.0), Disconnected())) - iterator = port.subscribe("test_pv") + iterator = port.subscribe(EpicsPvAddress("test_pv")) first = await anext(iterator) assert first.value == 1.0 with pytest.raises(ControlNotConnectedError) as exc_info: @@ -234,7 +235,7 @@ async def test_subscribe_initial_disconnect_is_ignored_then_value_yields() -> No """ port = EpicsPvaControlPort() _hijack_context(port, _MonitorContext(Disconnected(), _FakeValue(2.5))) - iterator = port.subscribe("test_pv") + iterator = port.subscribe(EpicsPvAddress("test_pv")) got = await anext(iterator) assert got.value == 2.5 await iterator.aclose() From d563fbab9fe443f22bf4aca0d863b5dcd4191dd9 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 16 Jul 2026 14:52:57 +0300 Subject: [PATCH 03/10] Add the Tango arm: TangoControlPort behind an optional extra The second substrate, and the one that fired the typed-address trigger the previous two commits answered. Tango is the control floor at ESRF (BLISS) and MAX IV (Tango/Sardana), and the p10 and p21 deployment descriptors already declare it, so the ControlPort's EPICS-only adapter set was a real gap rather than a speculative one. TangoControlPort consumes TangoAttributeAddress, so it reads `address.device` and `address.attribute` instead of splitting a TRL on every call; the registry did that once at route-match time. The ACL work is the same shape as the EPICS adapters': attribute data format maps to MeasurementKind (SCALAR/SPECTRUM/IMAGE, plus DevEnum to Categorical), Tango's attribute quality collapses onto the port's three-value Quality (VALID to Good, WARNING and CHANGING to Uncertain, ALARM and INVALID to Bad), and DevFailed reasons map onto the port's exception families. PyTango stays out of the base install. It binds the C++ Tango core plus omniORB, a heavy native stack that only Tango-floor deployments need, so it ships as the optional `tango` extra and the adapter imports lazily. Construction probes importability via `_optional_tango.require_tango`, which raises ValueError rather than ImportError so a missing extra surfaces where the port is materialised, at wiring time and mapped to 422, instead of as an uncaught ImportError deep in the conduct loop after a procedure has already started. This mirrors the `bo` group's `_optional_torch.py` posture. Pinned <11 to flag a future major shift. The tests inject a fake `tango` module rather than driving a device: CI has no TangoTest the way it runs a live EPICS softIOC, so every ACL branch is covered against a double. That leaves this adapter without the live-substrate integration test each EPICS adapter has, which is a known and deliberate gap, not an oversight; closing it needs a TangoTest device in CI and is the next thing to settle. Co-Authored-By: Claude Opus 4.8 --- apps/api/pyproject.toml | 14 + .../src/cora/operation/adapters/__init__.py | 9 +- .../operation/adapters/_optional_tango.py | 30 ++ .../operation/adapters/control_port_config.py | 9 +- .../operation/adapters/tango_control_port.py | 412 ++++++++++++++++++ .../operation/test_control_port_config.py | 26 ++ .../operation/test_tango_control_port_acl.py | 364 ++++++++++++++++ apps/api/uv.lock | 61 ++- 8 files changed, 918 insertions(+), 7 deletions(-) create mode 100644 apps/api/src/cora/operation/adapters/_optional_tango.py create mode 100644 apps/api/src/cora/operation/adapters/tango_control_port.py create mode 100644 apps/api/tests/unit/operation/test_tango_control_port_acl.py diff --git a/apps/api/pyproject.toml b/apps/api/pyproject.toml index 2ca14057ca0..671bc607056 100644 --- a/apps/api/pyproject.toml +++ b/apps/api/pyproject.toml @@ -104,6 +104,20 @@ dependencies = [ bo = [ "botorch>=0.16,<1", ] +# tango (Actuate-axis Tango arm): the optional dependency group for +# `TangoControlPort` at `cora/operation/adapters/`, the ControlPort adapter +# for Tango-floor deployments (ESRF BLISS / Beacon, MAX IV Tango / Sardana). +# PyTango binds the C++ Tango core plus omniORB: a heavy native stack that +# only Tango deployments need, so it is kept OUT of the base install (EPICS +# deployments go through aioca / p4p). The adapter imports `tango.asyncio` +# lazily and probes importability at construction (`_optional_tango.py`), so +# selecting the `tango` substrate without this group installed fails as a +# `ValueError` at wiring / handler time (mapped to 422), never as an uncaught +# `ImportError` mid-loop. LGPL-3.0 (PyTango binding) over the C++ Tango core. +# Pin <11 to flag a future major SDK shift. +tango = [ + "pytango>=10,<11", +] [dependency-groups] dev = [ diff --git a/apps/api/src/cora/operation/adapters/__init__.py b/apps/api/src/cora/operation/adapters/__init__.py index fb29f8e8a66..58f5373f724 100644 --- a/apps/api/src/cora/operation/adapters/__init__.py +++ b/apps/api/src/cora/operation/adapters/__init__.py @@ -3,8 +3,9 @@ `InMemoryControlPort` is the unit-tier `ControlPort` adapter per [[project_control_port_generalization_research]]. Substrate adapters (`CaprotoControlPort` test-tier; `EpicsCaControlPort`, -`EpicsPvaControlPort` production-tier) cover the EPICS family. -`ControlPortRegistry` routes addresses to the right substrate by -longest-prefix match so the executor sees a single `ControlPort` -regardless of how many substrates a deployment runs. +`EpicsPvaControlPort` production-tier) cover the EPICS family, and +`TangoControlPort` (PyTango, optional `tango` extra) covers Tango-floor +deployments. `ControlPortRegistry` routes addresses to the right +substrate by longest-prefix match so the executor sees a single +`ControlPort` regardless of how many substrates a deployment runs. """ diff --git a/apps/api/src/cora/operation/adapters/_optional_tango.py b/apps/api/src/cora/operation/adapters/_optional_tango.py new file mode 100644 index 00000000000..4b75ace29fa --- /dev/null +++ b/apps/api/src/cora/operation/adapters/_optional_tango.py @@ -0,0 +1,30 @@ +"""Construction-time probe for the optional `tango` dependency group. + +The `TangoControlPort` adapter needs PyTango (`tango`), which binds the C++ +Tango core plus omniORB: a heavy native stack that only Tango-floor +deployments (ESRF BLISS, MAX IV Tango/Sardana) need. To keep the base install +lean and the `cora` import cheap on EPICS-only deployments, the adapter does +not import `tango` at module load; it probes importability in its `__init__` +via the helper here. + +The probe raises `ValueError` (not `ImportError`) on a missing dependency so +the gap surfaces where the ControlPort is materialised (`build_control_port` +at wiring / handler time) rather than as an uncaught `ImportError` deep in the +conduct loop, after a procedure has already started. Mirrors the `bo` group's +`_optional_torch.py` posture. +""" + +from __future__ import annotations + +import importlib.util + +_INSTALL_HINT = "install the optional 'tango' dependency group (e.g. `uv sync --extra tango`)" + + +def require_tango(substrate: str) -> None: + """Raise ValueError if `tango` (PyTango) is not importable, naming the substrate.""" + if importlib.util.find_spec("tango") is None: + raise ValueError(f"the {substrate!r} substrate needs PyTango; {_INSTALL_HINT}") + + +__all__ = ["require_tango"] diff --git a/apps/api/src/cora/operation/adapters/control_port_config.py b/apps/api/src/cora/operation/adapters/control_port_config.py index 67af2c446cd..8633ed662d9 100644 --- a/apps/api/src/cora/operation/adapters/control_port_config.py +++ b/apps/api/src/cora/operation/adapters/control_port_config.py @@ -22,8 +22,12 @@ reachable but no real substrate is exercised). - `epics_ca`: production CA via aioca (`EpicsCaControlPort`). - `epics_pva`: production PVA via p4p (`EpicsPvaControlPort`). + - `tango`: Tango device attributes via PyTango (`TangoControlPort`). + Ships under the optional `tango` dependency group; selecting it + without the extra installed raises `ValueError` at construction + (probed via `_optional_tango.require_tango`). -Future substrates (`tango`, `opc_ua`) land as additive code edits in +Future substrates (`opc_ua`) land as additive code edits in `cora.infrastructure.control_port_route` (literal) plus a new arm in `_build_substrate` here. @@ -43,6 +47,7 @@ from cora.operation.adapters.epics_ca_control_port import EpicsCaControlPort from cora.operation.adapters.epics_pva_control_port import EpicsPvaControlPort from cora.operation.adapters.in_memory_control_port import InMemoryControlPort +from cora.operation.adapters.tango_control_port import TangoControlPort from cora.operation.ports.control_port import ControlPort, SubstrateControlPort @@ -90,7 +95,7 @@ def _build_substrate(substrate: Substrate) -> SubstrateControlPort[Any]: return EpicsCaControlPort() if substrate == "epics_pva": return EpicsPvaControlPort() - raise ValueError(f"substrate {substrate!r} has no adapter wired yet") + return TangoControlPort() __all__ = ["build_control_port"] diff --git a/apps/api/src/cora/operation/adapters/tango_control_port.py b/apps/api/src/cora/operation/adapters/tango_control_port.py new file mode 100644 index 00000000000..74e2c604328 --- /dev/null +++ b/apps/api/src/cora/operation/adapters/tango_control_port.py @@ -0,0 +1,412 @@ +"""PyTango-backed `ControlPort` adapter for Tango device attributes. + +Sits in the control-port arc per [[project_control_port_design]] + +[[project_control_port_generalization_research]] alongside the EPICS family +(`EpicsCaControlPort`, `EpicsPvaControlPort`). This adapter covers the Tango +substrate that the ESRF BLISS / Beacon deployments (ID19, ID16B, ID28, ID32) +and MAX IV (Tango / Sardana) run on. Value-IO only: Tango Commands (RPC) and +Tango typed events (`DATA_READY` / `INTERFACE_CHANGE`) are out of scope per +the port's anti-hooks and land in future sibling ports. + +PyTango (`tango`) binds the C++ Tango core plus omniORB, a heavy native stack +that only Tango-floor deployments need. It ships under the optional `tango` +dependency group and is NOT imported at module load; the adapter probes +importability in `__init__` via `_optional_tango.require_tango` (raising +`ValueError`, mapped to 422, rather than an uncaught `ImportError` mid-loop) +and imports `tango.asyncio` lazily on first use. This keeps `import cora` +cheap on EPICS-only installs. + +## Address space + +The adapter takes a `TangoAttributeAddress` (already split into `.device` and +`.attribute` by `ControlPortRegistry` via `parse_control_address`). One +`DeviceProxy` is cached per `.device`; many attributes share a device proxy. +The registry does the TRL parsing once at route-match time, so a malformed TRL +fails as a `MalformedControlAddressError` at the boundary rather than here. + +## Connection model + +`tango.asyncio.DeviceProxy` runs in asyncio green mode: `read_attribute` / +`write_attribute` / `subscribe_event` are awaitable. The proxy constructor is +also awaitable (it pings the device), so it is built lazily inside the async +methods and cached. PyTango has no per-call `timeout=` kwarg on the asyncio +methods, so each call is wrapped in `asyncio.wait_for(..., timeout=...)`; a +`TimeoutError` becomes `ControlTimeoutError` (write) or +`ControlNotConnectedError` (read, where a never-exported device and a slow +device are indistinguishable at this tier, mirroring the p4p adapter). + +## ACL translation (Tango DeviceAttribute -> Measurement) + +`read_attribute` returns a `DeviceAttribute` carrying `.value`, `.quality` +(`AttrQuality`), `.data_format` (`AttrDataFormat`), `.type` (`CmdArgType`), +and `.time` (`TimeVal`; `.totime()` -> float epoch seconds). The adapter +unpacks: + + - `kind`: `IMAGE` -> "Image"; `SPECTRUM` -> "Array"; + `type == DevEnum` or a `DevState` value -> "Categorical"; else "Scalar". + - `value`: numpy scalars via `.item()`; spectra via `tuple(...)`; images via + tuple-of-tuples; `DevEnum` resolved to its label string via the attribute + config's enum labels; `DevState` via its name. + - `quality` (`AttrQuality` -> `Quality`): `ATTR_VALID` -> Good; + `ATTR_WARNING` / `ATTR_CHANGING` -> Uncertain; + `ATTR_ALARM` / `ATTR_INVALID` -> Bad. + - `quality_detail`: the raw `AttrQuality` name as a forensic breadcrumb when + quality is not VALID; empty string when VALID (matches the EPICS adapters). + - `produced_at`: `datetime.fromtimestamp(attr.time.totime(), tz=UTC)`. + +## Error mapping + +PyTango raises `tango.DevFailed`, carrying a stack of `DevError` each with a +`.reason` string. The adapter maps the first reason: + + - `API_DeviceTimedOut` / `API_CommandTimedOut` -> `ControlTimeoutError` + - `API_CantConnectToDevice` / `API_CorbaException` / + `API_DeviceNotExported` / `API_DeviceNotFound` -> `ControlNotConnectedError` + - `API_AttrNotAllowed` / `API_WAttrOutsideLimit` -> `ControlWriteRejectedError` + - `API_AttrNotWritable` and authorisation refusals -> `ControlAccessDeniedError` + - any other reason -> `ControlWriteRejectedError` with the reason folded into + the detail so the decider's event payload captures it per + [[project_non_determinism_principle]] + +Client-side value-encode failures (`ValueError` / `TypeError` raised by +PyTango before the wire call) map to `ControlValueCoercionError`. + +## Subscribe lifecycle + +`subscribe` is a plain `def` returning an async generator directly; proxy +construction + `subscribe_event(attr, EventType.CHANGE_EVENT, cb)` run on the +generator's first `__anext__`. Tango pushes an initial synchronised event on +subscribe, then one per change. The callback receives an `EventData` with +`.attr_value` (a `DeviceAttribute`) and `.err` / `.errors`; it is pushed onto +an unbounded `asyncio.Queue`. An error event raises `ControlNotConnectedError` +through the iterator so a silent stream pause is impossible. The generator's +`finally` calls `unsubscribe_event(event_id)` (matching the EPICS adapters' +cleanup discipline). +""" + +# pyright: reportUnknownMemberType=false, reportUnknownVariableType=false, reportUnknownArgumentType=false, reportMissingTypeStubs=false, reportMissingImports=false + +from __future__ import annotations + +import asyncio +import contextlib +from datetime import UTC, datetime +from typing import TYPE_CHECKING, Any + +from cora.infrastructure.logging import get_logger +from cora.operation._control_dispatch_context import get_dispatch_correlation_id +from cora.operation.adapters._optional_tango import require_tango +from cora.operation.ports.control_port import ( + ControlAccessDeniedError, + ControlNotConnectedError, + ControlTimeoutError, + ControlValueCoercionError, + ControlWriteRejectedError, + Measurement, + MeasurementKind, + Quality, +) + +if TYPE_CHECKING: + from collections.abc import AsyncGenerator + + from cora.operation.ports.control_address import TangoAttributeAddress + + +_log = get_logger(__name__) +_DISPATCH_EVENT = "controlport.dispatch" +_DISPATCH_COMPLETED_EVENT = "controlport.dispatch.completed" +_DISPATCH_FAILED_EVENT = "controlport.dispatch.failed" + + +_DEFAULT_TIMEOUT_S = 5.0 +"""Default per-operation timeout. Mirrors the EPICS adapters. Tests override +on negative-path tests (`default_timeout_s=0.3`) so a missing device fails +fast instead of stalling the suite.""" + + +_NOT_CONNECTED_REASONS = frozenset( + { + "API_CantConnectToDevice", + "API_CorbaException", + "API_DeviceNotExported", + "API_DeviceNotFound", + } +) +_TIMEOUT_REASONS = frozenset({"API_DeviceTimedOut", "API_CommandTimedOut"}) +_WRITE_REJECTED_REASONS = frozenset({"API_AttrNotAllowed", "API_WAttrOutsideLimit"}) +_ACCESS_DENIED_REASONS = frozenset({"API_AttrNotWritable", "API_UnauthorizedAccess"}) + + +def _quality_for(quality: Any) -> Quality: + """Collapse a Tango `AttrQuality` to the domain `Quality` trichotomy.""" + name = getattr(quality, "name", str(quality)) + if name == "ATTR_VALID": + return "Good" + if name in ("ATTR_WARNING", "ATTR_CHANGING"): + return "Uncertain" + return "Bad" + + +def _quality_detail_for(quality: Any) -> str: + """Forensic-breadcrumb string for `Measurement.quality_detail`. + + Surfaces the raw `AttrQuality` name when quality is not VALID; empty + string otherwise (matches the EPICS adapters). + """ + name = getattr(quality, "name", str(quality)) + if name == "ATTR_VALID": + return "" + return f"attr_quality={name}" + + +def _kind_for(attr: Any) -> MeasurementKind: + """Map a `DeviceAttribute`'s data format + type to `MeasurementKind`.""" + data_format = getattr(attr, "data_format", None) + format_name = getattr(data_format, "name", str(data_format)) + if format_name == "IMAGE": + return "Image" + if format_name == "SPECTRUM": + return "Array" + type_name = getattr(getattr(attr, "type", None), "name", "") + if type_name in ("DevEnum", "DevState"): + return "Categorical" + return "Scalar" + + +def _unpack_value(attr: Any, kind: MeasurementKind) -> Any: + """Extract a Python-native value from a Tango `DeviceAttribute`.""" + value: Any = getattr(attr, "value", None) + if kind == "Image": + if hasattr(value, "tolist"): + return tuple(tuple(row) for row in value.tolist()) + return tuple(tuple(row) for row in value) + if kind == "Array": + if hasattr(value, "tolist"): + return tuple(value.tolist()) + return tuple(value) + if kind == "Categorical": + # DevState values stringify to their state name; DevEnum values + # carry a `.name` when PyTango resolves the label, else fall back + # to the raw string form. + if hasattr(value, "name"): + return value.name + return str(value) + scalar: Any = value + if hasattr(scalar, "item"): + scalar = scalar.item() + if isinstance(scalar, bytes): + return scalar.decode("utf-8", errors="replace") + return scalar + + +def _to_reading(attr: Any) -> Measurement: + """Translate a Tango `DeviceAttribute` to `Measurement`.""" + kind = _kind_for(attr) + value = _unpack_value(attr, kind) + quality = getattr(attr, "quality", None) + time_val = getattr(attr, "time", None) + timestamp = time_val.totime() if time_val is not None else 0.0 + produced_at = datetime.fromtimestamp(float(timestamp), tz=UTC) + return Measurement( + value=value, + kind=kind, + quality=_quality_for(quality), + produced_at=produced_at, + quality_detail=_quality_detail_for(quality), + name=str(getattr(attr, "name", "")), + ) + + +def _first_reason(exc: Any) -> str: + """Return the first `DevError.reason` from a `DevFailed`, or empty string.""" + args = getattr(exc, "args", ()) + if args and hasattr(args[0], "reason"): + return str(args[0].reason) + return "" + + +def _map_dev_failed(address: str, exc: Any, *, timeout_s: float) -> Exception: + """Translate a `tango.DevFailed` to the appropriate Control*Error.""" + reason = _first_reason(exc) + if reason in _TIMEOUT_REASONS: + return ControlTimeoutError(address, timeout_s) + if reason in _NOT_CONNECTED_REASONS: + return ControlNotConnectedError(address) + if reason in _ACCESS_DENIED_REASONS: + return ControlAccessDeniedError(address) + if reason in _WRITE_REJECTED_REASONS: + return ControlWriteRejectedError(address, reason) + return ControlWriteRejectedError(address, reason or "DevFailed") + + +class TangoControlPort: + """PyTango-backed `ControlPort` implementation for Tango device attributes. + + See module docstring for the connection model, ACL table, error + mapping, and subscribe lifecycle. Probes PyTango importability at + construction so a missing `tango` extra fails as a `ValueError` at + wiring time, not as an `ImportError` mid-conduct. + """ + + def __init__(self, *, default_timeout_s: float = _DEFAULT_TIMEOUT_S) -> None: + require_tango("tango") + self._default_timeout_s = default_timeout_s + self._proxies: dict[str, Any] = {} + self._closed = False + + async def _proxy_for(self, device: str) -> Any: + """Return a cached asyncio `DeviceProxy` for `device`, building it lazily. + + The asyncio proxy constructor is awaitable (it pings the device on + build), so a construction failure surfaces here as `DevFailed` and + is mapped by the caller. + """ + proxy = self._proxies.get(device) + if proxy is None: + from tango.asyncio import DeviceProxy + + proxy = await asyncio.wait_for(DeviceProxy(device), timeout=self._default_timeout_s) + self._proxies[device] = proxy + return proxy + + async def read(self, address: TangoAttributeAddress) -> Measurement: + from tango import DevFailed + + raw = str(address) + try: + proxy = await self._proxy_for(address.device) + attr = await asyncio.wait_for( + proxy.read_attribute(address.attribute), timeout=self._default_timeout_s + ) + except TimeoutError as exc: + raise ControlNotConnectedError(raw) from exc + except DevFailed as exc: + raise _map_dev_failed(raw, exc, timeout_s=self._default_timeout_s) from exc + return _to_reading(attr) + + async def write( + self, + address: TangoAttributeAddress, + value: int | float | bool | str | tuple[Any, ...], + *, + wait: bool = True, + timeout_s: float = 30.0, + ) -> None: + from tango import DevFailed + + _ = wait # Tango writes are synchronous-confirmed; no fire-and-forget arm. + raw = str(address) + correlation_id = get_dispatch_correlation_id() + _log.info( + _DISPATCH_EVENT, + address=raw, + operation="write", + correlation_id=str(correlation_id) if correlation_id is not None else None, + status="started", + ) + try: + proxy = await self._proxy_for(address.device) + await asyncio.wait_for( + proxy.write_attribute(address.attribute, value), timeout=timeout_s + ) + except TimeoutError as exc: + _log.info( + _DISPATCH_FAILED_EVENT, + address=raw, + operation="write", + correlation_id=str(correlation_id) if correlation_id is not None else None, + status="failed", + error_class=ControlTimeoutError.__name__, + ) + raise ControlTimeoutError(raw, timeout_s) from exc + except DevFailed as exc: + mapped = _map_dev_failed(raw, exc, timeout_s=timeout_s) + _log.info( + _DISPATCH_FAILED_EVENT, + address=raw, + operation="write", + correlation_id=str(correlation_id) if correlation_id is not None else None, + status="failed", + error_class=type(mapped).__name__, + ) + raise mapped from exc + except (ValueError, TypeError) as exc: + _log.info( + _DISPATCH_FAILED_EVENT, + address=raw, + operation="write", + correlation_id=str(correlation_id) if correlation_id is not None else None, + status="failed", + error_class=ControlValueCoercionError.__name__, + ) + raise ControlValueCoercionError( + raw, raw_type=type(value).__name__, target_kind="tango write" + ) from exc + _log.info( + _DISPATCH_COMPLETED_EVENT, + address=raw, + operation="write", + correlation_id=str(correlation_id) if correlation_id is not None else None, + status="completed", + ) + + def subscribe(self, address: TangoAttributeAddress) -> AsyncGenerator[Measurement]: + """Return type narrows the Protocol's `AsyncIterator` to `AsyncGenerator`. + + Covariant return lets tests close subscriptions via the iterator's + `aclose()`; same pattern as the EPICS adapters. Setup (proxy build + + `subscribe_event`) runs on the generator's first `__anext__`. + """ + return self._drain(address) + + async def _drain(self, address: TangoAttributeAddress) -> AsyncGenerator[Measurement]: + from tango import DevFailed, EventType + + raw = str(address) + queue: asyncio.Queue[Any] = asyncio.Queue() + + def _callback(event: Any) -> None: + # Synchronous put_nowait keeps FIFO ordering across rapid events; + # the queue is unbounded so it never raises QueueFull. + queue.put_nowait(event) + + try: + proxy = await self._proxy_for(address.device) + event_id = await asyncio.wait_for( + proxy.subscribe_event(address.attribute, EventType.CHANGE_EVENT, _callback), + timeout=self._default_timeout_s, + ) + except TimeoutError as exc: + raise ControlNotConnectedError(raw) from exc + except DevFailed as exc: + raise _map_dev_failed(raw, exc, timeout_s=self._default_timeout_s) from exc + + try: + while True: + event = await queue.get() + if getattr(event, "err", False): + raise ControlNotConnectedError(raw) + attr = getattr(event, "attr_value", None) + if attr is None: + continue + yield _to_reading(attr) + finally: + with contextlib.suppress(Exception): + await proxy.unsubscribe_event(event_id) + + async def aclose(self) -> None: + """Drop cached device proxies; idempotent. + + Tango `DeviceProxy` objects release their CORBA connection on + garbage collection; dropping the cache is sufficient. Provided so + production code paths can call `aclose()` polymorphically against any + `ControlPort`. + """ + if self._closed: + return + self._closed = True + self._proxies.clear() + + +__all__ = ["TangoControlPort"] diff --git a/apps/api/tests/unit/operation/test_control_port_config.py b/apps/api/tests/unit/operation/test_control_port_config.py index e9fc38d8b1f..ffaa0f1bc41 100644 --- a/apps/api/tests/unit/operation/test_control_port_config.py +++ b/apps/api/tests/unit/operation/test_control_port_config.py @@ -23,12 +23,17 @@ from cora.operation.adapters.epics_ca_control_port import EpicsCaControlPort from cora.operation.adapters.epics_pva_control_port import EpicsPvaControlPort from cora.operation.adapters.in_memory_control_port import InMemoryControlPort +from cora.operation.adapters.tango_control_port import TangoControlPort from cora.operation.ports.control_port import ( ControlNotConnectedError, NoAdapterForAddressError, ) +def _no_tango_probe(_substrate: str) -> None: + """Neutralised `require_tango`: PyTango is absent in the base test env.""" + + @pytest.mark.unit def test_build_control_port_with_empty_routes_returns_in_memory_port() -> None: port = build_control_port([]) @@ -82,6 +87,27 @@ def test_build_control_port_with_mixed_routes_picks_right_adapter_per_prefix() - assert isinstance(port.route("2bma:rot:val"), EpicsCaControlPort) +@pytest.mark.unit +def test_build_control_port_with_tango_route_constructs_tango_adapter( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A `tango` route builds a `TangoControlPort`. + + The adapter probes PyTango importability at construction; PyTango is not + installed in the base test environment, so the probe is neutralised here + to exercise the factory arm rather than the missing-extra path (that path + is covered by `test_require_tango_raises_value_error_when_pytango_absent`). + """ + monkeypatch.setattr( + "cora.operation.adapters.tango_control_port.require_tango", + _no_tango_probe, + ) + port = build_control_port([ControlPortRoute(prefix="id19/", substrate="tango")]) + assert isinstance(port, ControlPortRegistry) + routed = port.route("id19/bsh/1/state") + assert isinstance(routed, TangoControlPort) + + @pytest.mark.unit def test_control_port_route_rejects_empty_prefix() -> None: with pytest.raises(ValidationError): diff --git a/apps/api/tests/unit/operation/test_tango_control_port_acl.py b/apps/api/tests/unit/operation/test_tango_control_port_acl.py new file mode 100644 index 00000000000..ac42f4a488e --- /dev/null +++ b/apps/api/tests/unit/operation/test_tango_control_port_acl.py @@ -0,0 +1,364 @@ +"""Unit-tier ACL tests for `TangoControlPort` translation + error mapping. + +Mirrors `test_epics_pva_control_port_acl.py`. PyTango is a heavy native +dependency that is not installed in the base test environment and CI has no +TangoTest device the way it runs a live EPICS softIOC, so these tests inject a +fake `tango` / `tango.asyncio` module into `sys.modules`, neutralise the +import probe, and pre-seed the adapter's per-device proxy cache. That exercises +every ACL branch (kind classification, quality collapse, DevFailed reason +mapping, subscribe lifecycle) without a live Tango control plane. + +Coverage: + + - kind classification: SCALAR / SPECTRUM / IMAGE / DevEnum -> + Scalar / Array / Image / Categorical + - quality collapse: VALID -> Good; WARNING / CHANGING -> Uncertain; + ALARM / INVALID -> Bad + - read timeout -> ControlNotConnectedError; write timeout -> ControlTimeoutError + - DevFailed reason mapping: not-connected, timeout, write-rejected, + access-denied + - write value-coercion (ValueError / TypeError) -> ControlValueCoercionError + - subscribe: value then err-event -> NotConnected through the iterator; + unsubscribe called on close; aclose idempotent + - probe: require_tango raises ValueError when PyTango is absent + +The adapter now takes a typed `TangoAttributeAddress`; TRL parsing and its +malformed-address guard moved to the registry boundary and are covered in +`test_control_address.py`. +""" + +from __future__ import annotations + +import importlib.util +import sys +import types +from typing import TYPE_CHECKING, Any + +import pytest + +from cora.operation.ports.control_address import TangoAttributeAddress +from cora.operation.ports.control_port import ( + ControlAccessDeniedError, + ControlNotConnectedError, + ControlTimeoutError, + ControlValueCoercionError, + ControlWriteRejectedError, +) + +if TYPE_CHECKING: + from collections.abc import Iterator + + +_ADDR = TangoAttributeAddress(device="dev", attribute="attr") +"""The typed address every test uses; `str(_ADDR)` == "dev/attr", which the +fake proxy is seeded under (`_proxies["dev"]`) and which error breadcrumbs +carry. TRL-parsing / malformed-address coverage lives in test_control_address.""" + + +def _no_probe(_substrate: str) -> None: + """Neutralised `require_tango`: PyTango is absent in the base test env.""" + + +def _no_spec(_name: str) -> None: + """Stand-in for `importlib.util.find_spec` returning None (module absent).""" + return None + + +class _DevError: + """Stand-in for a `tango.DevError`, carrying the `.reason` the ACL reads.""" + + def __init__(self, reason: str) -> None: + self.reason = reason + + +class _DevFailed(Exception): # noqa: N818 + """Stand-in for `tango.DevFailed`: args[0] is a `_DevError` with `.reason`. + + Named to mirror PyTango's own `DevFailed` (no Error suffix) so the test + reads against the real symbol name the adapter catches. + """ + + +class _Enum: + """Minimal `.name`-carrying stand-in for a Tango enum member (quality / format / type).""" + + def __init__(self, name: str) -> None: + self.name = name + + +class _TimeVal: + def __init__(self, seconds: float) -> None: + self._seconds = seconds + + def totime(self) -> float: + return self._seconds + + +class _DeviceAttribute: + """Stand-in for `tango.DeviceAttribute` with the fields the ACL unpacks.""" + + def __init__( + self, + *, + value: Any, + quality: str = "ATTR_VALID", + data_format: str = "SCALAR", + attr_type: str = "DevDouble", + seconds: float = 0.0, + name: str = "attr", + ) -> None: + self.value = value + self.quality = _Enum(quality) + self.data_format = _Enum(data_format) + self.type = _Enum(attr_type) + self.time = _TimeVal(seconds) + self.name = name + + +class _EventData: + """Stand-in for a Tango CHANGE_EVENT `EventData`.""" + + def __init__(self, *, attr_value: Any = None, err: bool = False) -> None: + self.attr_value = attr_value + self.err = err + self.errors = () + + +class _FakeProxy: + """Programmable stand-in for `tango.asyncio.DeviceProxy`. + + `read_attribute` / `write_attribute` either return / accept, or raise a + configured exception. `subscribe_event` invokes its callback synchronously + with the configured event sequence (mirroring the p4p `_MonitorContext`). + """ + + def __init__( + self, + *, + read_result: Any = None, + read_raises: BaseException | None = None, + write_raises: BaseException | None = None, + events: tuple[Any, ...] = (), + ) -> None: + self._read_result = read_result + self._read_raises = read_raises + self._write_raises = write_raises + self._events = events + self.unsubscribed: list[int] = [] + + async def read_attribute(self, _attribute: str) -> Any: + if self._read_raises is not None: + raise self._read_raises + return self._read_result + + async def write_attribute(self, _attribute: str, _value: Any) -> None: + if self._write_raises is not None: + raise self._write_raises + + async def subscribe_event(self, _attribute: str, _event_type: Any, callback: Any) -> int: + for event in self._events: + callback(event) + return 7 + + async def unsubscribe_event(self, event_id: int) -> None: + self.unsubscribed.append(event_id) + + +@pytest.fixture +def fake_tango(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: + """Inject a fake `tango` / `tango.asyncio` module and neutralise the probe. + + The adapter imports `DevFailed` / `EventType` from `tango` and + `DeviceProxy` from `tango.asyncio` lazily inside its methods; seeding + `sys.modules` makes those imports resolve to the stand-ins here. The + import probe is patched to a no-op so `TangoControlPort()` constructs. + """ + tango_mod = types.ModuleType("tango") + tango_mod.DevFailed = _DevFailed # pyright: ignore[reportAttributeAccessIssue] + tango_mod.EventType = types.SimpleNamespace( # pyright: ignore[reportAttributeAccessIssue] + CHANGE_EVENT="change" + ) + asyncio_mod = types.ModuleType("tango.asyncio") + asyncio_mod.DeviceProxy = _FakeProxy # pyright: ignore[reportAttributeAccessIssue] + tango_mod.asyncio = asyncio_mod # pyright: ignore[reportAttributeAccessIssue] + monkeypatch.setitem(sys.modules, "tango", tango_mod) + monkeypatch.setitem(sys.modules, "tango.asyncio", asyncio_mod) + monkeypatch.setattr("cora.operation.adapters.tango_control_port.require_tango", _no_probe) + yield + + +def _make_port(**proxy_kwargs: Any) -> Any: + """Build a `TangoControlPort` with a pre-seeded fake proxy for device `dev`.""" + from cora.operation.adapters.tango_control_port import TangoControlPort + + port = TangoControlPort(default_timeout_s=0.05) + port._proxies["dev"] = _FakeProxy(**proxy_kwargs) # pyright: ignore[reportPrivateUsage] + return port + + +@pytest.mark.unit +async def test_read_scalar_translates_to_good_scalar_measurement(fake_tango: None) -> None: + port = _make_port(read_result=_DeviceAttribute(value=2.5)) + reading = await port.read(_ADDR) + assert reading.kind == "Scalar" + assert reading.value == 2.5 + assert reading.quality == "Good" + assert reading.quality_detail == "" + + +@pytest.mark.unit +async def test_read_spectrum_translates_to_array_kind(fake_tango: None) -> None: + port = _make_port(read_result=_DeviceAttribute(value=[1.0, 2.0, 3.0], data_format="SPECTRUM")) + reading = await port.read(_ADDR) + assert reading.kind == "Array" + assert reading.value == (1.0, 2.0, 3.0) + + +@pytest.mark.unit +async def test_read_image_translates_to_image_kind(fake_tango: None) -> None: + port = _make_port(read_result=_DeviceAttribute(value=[[1, 2], [3, 4]], data_format="IMAGE")) + reading = await port.read(_ADDR) + assert reading.kind == "Image" + assert reading.value == ((1, 2), (3, 4)) + + +@pytest.mark.unit +async def test_read_devenum_translates_to_categorical_label(fake_tango: None) -> None: + port = _make_port(read_result=_DeviceAttribute(value=_Enum("OPEN"), attr_type="DevEnum")) + reading = await port.read(_ADDR) + assert reading.kind == "Categorical" + assert reading.value == "OPEN" + + +@pytest.mark.unit +@pytest.mark.parametrize( + ("attr_quality", "expected"), + [ + ("ATTR_VALID", "Good"), + ("ATTR_WARNING", "Uncertain"), + ("ATTR_CHANGING", "Uncertain"), + ("ATTR_ALARM", "Bad"), + ("ATTR_INVALID", "Bad"), + ], +) +async def test_read_collapses_attr_quality( + fake_tango: None, attr_quality: str, expected: str +) -> None: + port = _make_port(read_result=_DeviceAttribute(value=1.0, quality=attr_quality)) + reading = await port.read(_ADDR) + assert reading.quality == expected + if expected != "Good": + assert attr_quality in reading.quality_detail + + +@pytest.mark.unit +async def test_read_timeout_translates_to_not_connected(fake_tango: None) -> None: + """A `wait_for` timeout on read maps to NotConnected (slow == absent at this tier).""" + port = _make_port(read_raises=TimeoutError()) + with pytest.raises(ControlNotConnectedError) as exc_info: + await port.read(_ADDR) + assert exc_info.value.address == "dev/attr" + + +@pytest.mark.unit +async def test_read_dev_failed_cant_connect_translates_to_not_connected( + fake_tango: None, +) -> None: + port = _make_port(read_raises=_DevFailed(_DevError("API_CantConnectToDevice"))) + with pytest.raises(ControlNotConnectedError) as exc_info: + await port.read(_ADDR) + assert exc_info.value.address == "dev/attr" + + +@pytest.mark.unit +async def test_write_timeout_translates_to_control_timeout_error(fake_tango: None) -> None: + port = _make_port(write_raises=TimeoutError()) + with pytest.raises(ControlTimeoutError) as exc_info: + await port.write(_ADDR, 1.0, timeout_s=0.05) + assert exc_info.value.address == "dev/attr" + assert exc_info.value.timeout_s == 0.05 + + +@pytest.mark.unit +async def test_write_dev_failed_attr_not_allowed_translates_to_write_rejected( + fake_tango: None, +) -> None: + port = _make_port(write_raises=_DevFailed(_DevError("API_AttrNotAllowed"))) + with pytest.raises(ControlWriteRejectedError) as exc_info: + await port.write(_ADDR, 1.0) + assert exc_info.value.address == "dev/attr" + assert "API_AttrNotAllowed" in exc_info.value.reason + + +@pytest.mark.unit +async def test_write_dev_failed_not_writable_translates_to_access_denied( + fake_tango: None, +) -> None: + port = _make_port(write_raises=_DevFailed(_DevError("API_AttrNotWritable"))) + with pytest.raises(ControlAccessDeniedError) as exc_info: + await port.write(_ADDR, 1.0) + assert exc_info.value.address == "dev/attr" + + +@pytest.mark.unit +async def test_write_value_error_translates_to_value_coercion(fake_tango: None) -> None: + port = _make_port(write_raises=ValueError("cannot encode")) + with pytest.raises(ControlValueCoercionError) as exc_info: + await port.write(_ADDR, "foo") + assert exc_info.value.address == "dev/attr" + + +@pytest.mark.unit +async def test_subscribe_yields_value_then_err_event_raises_through_iterator( + fake_tango: None, +) -> None: + """After at least one value, an err-event raises NotConnected through the iterator.""" + port = _make_port( + events=( + _EventData(attr_value=_DeviceAttribute(value=1.0)), + _EventData(err=True), + ) + ) + iterator = port.subscribe(_ADDR) + first = await anext(iterator) + assert first.value == 1.0 + with pytest.raises(ControlNotConnectedError) as exc_info: + await anext(iterator) + assert exc_info.value.address == "dev/attr" + await iterator.aclose() + + +@pytest.mark.unit +async def test_subscribe_unsubscribes_on_close(fake_tango: None) -> None: + """Closing the subscribe iterator calls `unsubscribe_event` with the id.""" + proxy = _FakeProxy(events=(_EventData(attr_value=_DeviceAttribute(value=1.0)),)) + from cora.operation.adapters.tango_control_port import TangoControlPort + + port = TangoControlPort(default_timeout_s=0.05) + port._proxies["dev"] = proxy # pyright: ignore[reportPrivateUsage] + iterator = port.subscribe(_ADDR) + got = await anext(iterator) + assert got.value == 1.0 + await iterator.aclose() + assert proxy.unsubscribed == [7] + + +@pytest.mark.unit +async def test_aclose_clears_proxies_idempotently(fake_tango: None) -> None: + port = _make_port(read_result=_DeviceAttribute(value=1.0)) + await port.aclose() + assert port._proxies == {} # pyright: ignore[reportPrivateUsage] + await port.aclose() # idempotent + assert port._closed is True # pyright: ignore[reportPrivateUsage] + + +@pytest.mark.unit +def test_require_tango_raises_value_error_when_pytango_absent( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The probe raises a ValueError (mapped to 422), never an ImportError.""" + from cora.operation.adapters._optional_tango import require_tango + + monkeypatch.setattr(importlib.util, "find_spec", _no_spec) + with pytest.raises(ValueError, match="PyTango"): + require_tango("tango") diff --git a/apps/api/uv.lock b/apps/api/uv.lock index 7c6cdc28dc6..8af00661516 100644 --- a/apps/api/uv.lock +++ b/apps/api/uv.lock @@ -359,6 +359,9 @@ dependencies = [ bo = [ { name = "botorch" }, ] +tango = [ + { name = "pytango" }, +] [package.dev-dependencies] dev = [ @@ -412,11 +415,12 @@ requires-dist = [ { name = "pydantic", specifier = ">=2.13.4,<3" }, { name = "pydantic-settings", specifier = ">=2.14.2,<3" }, { name = "pyjwt", extras = ["crypto"], specifier = ">=2.13.0,<3" }, + { name = "pytango", marker = "extra == 'tango'", specifier = ">=10,<11" }, { name = "structlog", specifier = ">=26.1.0" }, { name = "uuid-utils", specifier = ">=0.16.2" }, { name = "uvicorn", extras = ["standard"], specifier = ">=0.49.0" }, ] -provides-extras = ["bo"] +provides-extras = ["bo", "tango"] [package.metadata.requires-dev] dev = [ @@ -1868,6 +1872,34 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/c4/72/02445137af02769918a93807b2b7890047c32bfb9f90371cbc12688819eb/protobuf-6.33.6-py3-none-any.whl", hash = "sha256:77179e006c476e69bf8e8ce866640091ec42e1beb80b213c3900006ecfba6901", size = 170656, upload-time = "2026-03-18T19:04:59.826Z" }, ] +[[package]] +name = "psutil" +version = "7.2.2" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/aa/c6/d1ddf4abb55e93cebc4f2ed8b5d6dbad109ecb8d63748dd2b20ab5e57ebe/psutil-7.2.2.tar.gz", hash = "sha256:0746f5f8d406af344fd547f1c8daa5f5c33dbc293bb8d6a16d80b4bb88f59372", size = 493740, upload-time = "2026-01-28T18:14:54.428Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/51/08/510cbdb69c25a96f4ae523f733cdc963ae654904e8db864c07585ef99875/psutil-7.2.2-cp313-cp313t-macosx_10_13_x86_64.whl", hash = "sha256:2edccc433cbfa046b980b0df0171cd25bcaeb3a68fe9022db0979e7aa74a826b", size = 130595, upload-time = "2026-01-28T18:14:57.293Z" }, + { url = "https://files.pythonhosted.org/packages/d6/f5/97baea3fe7a5a9af7436301f85490905379b1c6f2dd51fe3ecf24b4c5fbf/psutil-7.2.2-cp313-cp313t-macosx_11_0_arm64.whl", hash = "sha256:e78c8603dcd9a04c7364f1a3e670cea95d51ee865e4efb3556a3a63adef958ea", size = 131082, upload-time = "2026-01-28T18:14:59.732Z" }, + { url = "https://files.pythonhosted.org/packages/37/d6/246513fbf9fa174af531f28412297dd05241d97a75911ac8febefa1a53c6/psutil-7.2.2-cp313-cp313t-manylinux2010_x86_64.manylinux_2_12_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:1a571f2330c966c62aeda00dd24620425d4b0cc86881c89861fbc04549e5dc63", size = 181476, upload-time = "2026-01-28T18:15:01.884Z" }, + { url = "https://files.pythonhosted.org/packages/b8/b5/9182c9af3836cca61696dabe4fd1304e17bc56cb62f17439e1154f225dd3/psutil-7.2.2-cp313-cp313t-manylinux2014_aarch64.manylinux_2_17_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:917e891983ca3c1887b4ef36447b1e0873e70c933afc831c6b6da078ba474312", size = 184062, upload-time = "2026-01-28T18:15:04.436Z" }, + { url = "https://files.pythonhosted.org/packages/16/ba/0756dca669f5a9300d0cbcbfae9a4c30e446dfc7440ffe43ded5724bfd93/psutil-7.2.2-cp313-cp313t-win_amd64.whl", hash = "sha256:ab486563df44c17f5173621c7b198955bd6b613fb87c71c161f827d3fb149a9b", size = 139893, upload-time = "2026-01-28T18:15:06.378Z" }, + { url = "https://files.pythonhosted.org/packages/1c/61/8fa0e26f33623b49949346de05ec1ddaad02ed8ba64af45f40a147dbfa97/psutil-7.2.2-cp313-cp313t-win_arm64.whl", hash = "sha256:ae0aefdd8796a7737eccea863f80f81e468a1e4cf14d926bd9b6f5f2d5f90ca9", size = 135589, upload-time = "2026-01-28T18:15:08.03Z" }, + { url = "https://files.pythonhosted.org/packages/81/69/ef179ab5ca24f32acc1dac0c247fd6a13b501fd5534dbae0e05a1c48b66d/psutil-7.2.2-cp314-cp314t-macosx_10_15_x86_64.whl", hash = "sha256:eed63d3b4d62449571547b60578c5b2c4bcccc5387148db46e0c2313dad0ee00", size = 130664, upload-time = "2026-01-28T18:15:09.469Z" }, + { url = "https://files.pythonhosted.org/packages/7b/64/665248b557a236d3fa9efc378d60d95ef56dd0a490c2cd37dafc7660d4a9/psutil-7.2.2-cp314-cp314t-macosx_11_0_arm64.whl", hash = "sha256:7b6d09433a10592ce39b13d7be5a54fbac1d1228ed29abc880fb23df7cb694c9", size = 131087, upload-time = "2026-01-28T18:15:11.724Z" }, + { url = "https://files.pythonhosted.org/packages/d5/2e/e6782744700d6759ebce3043dcfa661fb61e2fb752b91cdeae9af12c2178/psutil-7.2.2-cp314-cp314t-manylinux2010_x86_64.manylinux_2_12_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:1fa4ecf83bcdf6e6c8f4449aff98eefb5d0604bf88cb883d7da3d8d2d909546a", size = 182383, upload-time = "2026-01-28T18:15:13.445Z" }, + { url = "https://files.pythonhosted.org/packages/57/49/0a41cefd10cb7505cdc04dab3eacf24c0c2cb158a998b8c7b1d27ee2c1f5/psutil-7.2.2-cp314-cp314t-manylinux2014_aarch64.manylinux_2_17_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:e452c464a02e7dc7822a05d25db4cde564444a67e58539a00f929c51eddda0cf", size = 185210, upload-time = "2026-01-28T18:15:16.002Z" }, + { url = "https://files.pythonhosted.org/packages/dd/2c/ff9bfb544f283ba5f83ba725a3c5fec6d6b10b8f27ac1dc641c473dc390d/psutil-7.2.2-cp314-cp314t-win_amd64.whl", hash = "sha256:c7663d4e37f13e884d13994247449e9f8f574bc4655d509c3b95e9ec9e2b9dc1", size = 141228, upload-time = "2026-01-28T18:15:18.385Z" }, + { url = "https://files.pythonhosted.org/packages/f2/fc/f8d9c31db14fcec13748d373e668bc3bed94d9077dbc17fb0eebc073233c/psutil-7.2.2-cp314-cp314t-win_arm64.whl", hash = "sha256:11fe5a4f613759764e79c65cf11ebdf26e33d6dd34336f8a337aa2996d71c841", size = 136284, upload-time = "2026-01-28T18:15:19.912Z" }, + { url = "https://files.pythonhosted.org/packages/e7/36/5ee6e05c9bd427237b11b3937ad82bb8ad2752d72c6969314590dd0c2f6e/psutil-7.2.2-cp36-abi3-macosx_10_9_x86_64.whl", hash = "sha256:ed0cace939114f62738d808fdcecd4c869222507e266e574799e9c0faa17d486", size = 129090, upload-time = "2026-01-28T18:15:22.168Z" }, + { url = "https://files.pythonhosted.org/packages/80/c4/f5af4c1ca8c1eeb2e92ccca14ce8effdeec651d5ab6053c589b074eda6e1/psutil-7.2.2-cp36-abi3-macosx_11_0_arm64.whl", hash = "sha256:1a7b04c10f32cc88ab39cbf606e117fd74721c831c98a27dc04578deb0c16979", size = 129859, upload-time = "2026-01-28T18:15:23.795Z" }, + { url = "https://files.pythonhosted.org/packages/b5/70/5d8df3b09e25bce090399cf48e452d25c935ab72dad19406c77f4e828045/psutil-7.2.2-cp36-abi3-manylinux2010_x86_64.manylinux_2_12_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:076a2d2f923fd4821644f5ba89f059523da90dc9014e85f8e45a5774ca5bc6f9", size = 155560, upload-time = "2026-01-28T18:15:25.976Z" }, + { url = "https://files.pythonhosted.org/packages/63/65/37648c0c158dc222aba51c089eb3bdfa238e621674dc42d48706e639204f/psutil-7.2.2-cp36-abi3-manylinux2014_aarch64.manylinux_2_17_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:b0726cecd84f9474419d67252add4ac0cd9811b04d61123054b9fb6f57df6e9e", size = 156997, upload-time = "2026-01-28T18:15:27.794Z" }, + { url = "https://files.pythonhosted.org/packages/8e/13/125093eadae863ce03c6ffdbae9929430d116a246ef69866dad94da3bfbc/psutil-7.2.2-cp36-abi3-musllinux_1_2_aarch64.whl", hash = "sha256:fd04ef36b4a6d599bbdb225dd1d3f51e00105f6d48a28f006da7f9822f2606d8", size = 148972, upload-time = "2026-01-28T18:15:29.342Z" }, + { url = "https://files.pythonhosted.org/packages/04/78/0acd37ca84ce3ddffaa92ef0f571e073faa6d8ff1f0559ab1272188ea2be/psutil-7.2.2-cp36-abi3-musllinux_1_2_x86_64.whl", hash = "sha256:b58fabe35e80b264a4e3bb23e6b96f9e45a3df7fb7eed419ac0e5947c61e47cc", size = 148266, upload-time = "2026-01-28T18:15:31.597Z" }, + { url = "https://files.pythonhosted.org/packages/b4/90/e2159492b5426be0c1fef7acba807a03511f97c5f86b3caeda6ad92351a7/psutil-7.2.2-cp37-abi3-win_amd64.whl", hash = "sha256:eb7e81434c8d223ec4a219b5fc1c47d0417b12be7ea866e24fb5ad6e84b3d988", size = 137737, upload-time = "2026-01-28T18:15:33.849Z" }, + { url = "https://files.pythonhosted.org/packages/8c/c7/7bb2e321574b10df20cbde462a94e2b71d05f9bbda251ef27d104668306a/psutil-7.2.2-cp37-abi3-win_arm64.whl", hash = "sha256:8c233660f575a5a89e6d4cb65d9f938126312bca76d8fe087b947b3a1aaac9ee", size = 134617, upload-time = "2026-01-28T18:15:36.514Z" }, +] + [[package]] name = "pvxslibs" version = "1.5.1" @@ -2052,6 +2084,33 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/0a/49/385be530a6a5b78d1cbcd5c2e38debc8959a2fc6bdb716f4e581002979fc/pyright-1.1.411-py3-none-any.whl", hash = "sha256:dc7c72a8e2700c55baa127554040e067041ea53ccfd50bf96308cc4291c7d5d9", size = 6181526, upload-time = "2026-06-25T02:14:04.691Z" }, ] +[[package]] +name = "pytango" +version = "10.3.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "docstring-parser" }, + { name = "numpy" }, + { name = "packaging" }, + { name = "psutil" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/5d/8e/0648a8528e587ad93643a60a688ba86a7bb5de44baa134622c33a4b131e0/pytango-10.3.0.tar.gz", hash = "sha256:27e03c9feac6227c2534e1fb7dbcd33fa33616cbc1969c3fd86d303d4402f2f6", size = 2823824, upload-time = "2026-06-03T16:15:38.756Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/bc/b8/dabd8f399ce7edb24bdf5250225aafd4022e7e96034fdc63039ffe20d76c/pytango-10.3.0-cp313-cp313-macosx_12_0_arm64.whl", hash = "sha256:943e2b67b85e8a3146c636960285c8002cb0b2f59cb684bc90efb3b29b737771", size = 17483439, upload-time = "2026-06-03T16:15:06.495Z" }, + { url = "https://files.pythonhosted.org/packages/4b/d1/b9161817239ff5c35de5b64d3b03533a240bb379e0538ceaa9204beedac0/pytango-10.3.0-cp313-cp313-macosx_12_0_x86_64.whl", hash = "sha256:92e0581352034715959d82faaecc8af52000cfe44a161940ebdaaa8eb1dac112", size = 18604079, upload-time = "2026-06-03T16:15:09.279Z" }, + { url = "https://files.pythonhosted.org/packages/12/28/e042c5a10a4e37ffe3bafa343c1b2d374ddb3786efe1bd430659e5e7dd40/pytango-10.3.0-cp313-cp313-manylinux2014_aarch64.manylinux_2_17_aarch64.whl", hash = "sha256:24c4d8fabd4984af087d0752b8f9a4a5b8c92c58f1256b10e2be5a843b1317e3", size = 18231784, upload-time = "2026-06-03T16:15:12.105Z" }, + { url = "https://files.pythonhosted.org/packages/d5/c8/194e4e20d5bafdb7063d3cad97c9b145410fbab2660df643634b8ae447a4/pytango-10.3.0-cp313-cp313-manylinux2014_i686.manylinux_2_17_i686.whl", hash = "sha256:276eb6a6b4b7aa38b070651e7af348d0c70133a1c1d9c6ef47103b10e74e8ac3", size = 20155329, upload-time = "2026-06-03T16:15:14.974Z" }, + { url = "https://files.pythonhosted.org/packages/f6/2d/913cde31f908db77d8d586beea7bcc7457be5e4b9c92586db162b90c2ec2/pytango-10.3.0-cp313-cp313-manylinux2014_x86_64.manylinux_2_17_x86_64.whl", hash = "sha256:c66f312494b3bcb880dfd55d9d63e153c7b63b60bc92d31dd9b08d4b7546e417", size = 19216039, upload-time = "2026-06-03T16:15:17.704Z" }, + { url = "https://files.pythonhosted.org/packages/94/00/6d7c0568289616777d342aababe6dc08b6b05a4b31e396b6c07027c91e98/pytango-10.3.0-cp313-cp313-win32.whl", hash = "sha256:999fd8cda6854055c8c98ed1672a0d51d1cba9f5e2c30f3574817b19aaf04e28", size = 4651036, upload-time = "2026-06-03T16:15:20.238Z" }, + { url = "https://files.pythonhosted.org/packages/36/82/1408869bea586a2ae2c806396c7e9de0540325144674b20be19e5332e8cb/pytango-10.3.0-cp313-cp313-win_amd64.whl", hash = "sha256:835c12a2b750216e51070f2d5db734f2cd8755cd3cf7fcad1ef7e2990483d9a5", size = 5093616, upload-time = "2026-06-03T16:15:22.093Z" }, + { url = "https://files.pythonhosted.org/packages/bd/07/b68cd0e30f87677f63d70992b946a070b8c96dd1ce5f29979114fbee552f/pytango-10.3.0-cp314-cp314-macosx_12_0_arm64.whl", hash = "sha256:f86db7cb045c9585f92fdd7af07b99185ff492e032b026261cc471effcdc3816", size = 17486350, upload-time = "2026-06-03T16:15:24.241Z" }, + { url = "https://files.pythonhosted.org/packages/a8/21/ebb09c1108fba37e39e23f6d771328a40a78f44f77fbbc88977f5921ecee/pytango-10.3.0-cp314-cp314-macosx_12_0_x86_64.whl", hash = "sha256:debea3d7e10945afd604d394a615b6aaf90a05fa793ddacb58dba1e3b4a7bc6d", size = 18603297, upload-time = "2026-06-03T16:15:26.999Z" }, + { url = "https://files.pythonhosted.org/packages/e4/ac/b699c0c2978ee1478d2e18f4bd0160809851a6301090d5a7f34c5ea1e6d6/pytango-10.3.0-cp314-cp314-manylinux2014_aarch64.manylinux_2_17_aarch64.whl", hash = "sha256:5f8a96a3c24c0c9299c4cdb8575667629eb3fad8e466c68ede6fb09f2d0e8a7b", size = 18239620, upload-time = "2026-06-03T16:15:30.093Z" }, + { url = "https://files.pythonhosted.org/packages/48/16/bd73cd3b4be06ce498036fff28eb9ccae65eda57ec200dc429fb321d3867/pytango-10.3.0-cp314-cp314-manylinux2014_x86_64.manylinux_2_17_x86_64.whl", hash = "sha256:166326a27290705971b138be2daff5d47ed107341ec3b39080c5dc22ee35d668", size = 19219472, upload-time = "2026-06-03T16:15:32.779Z" }, + { url = "https://files.pythonhosted.org/packages/d2/81/aef910f68ddf8503ea2f32b96b4b17a05ef345df4c573c500110aa0d2c0e/pytango-10.3.0-cp314-cp314-win32.whl", hash = "sha256:ea326d5cdb16d0e9cd069f8a9aa77f98290f6f05d1abf718fc5754cb13e4196a", size = 4766319, upload-time = "2026-06-03T16:15:35.046Z" }, + { url = "https://files.pythonhosted.org/packages/93/bc/46ec02784b3fbfee42b51caffdaff613d9d07dae91baeb699491b438aa07/pytango-10.3.0-cp314-cp314-win_amd64.whl", hash = "sha256:00fd3434341550ad73bf94d70c6adc3cbacc47345d65a80fcb1a99b9662bdc95", size = 5275649, upload-time = "2026-06-03T16:15:36.837Z" }, +] + [[package]] name = "pytest" version = "9.1.1" From a1cfe1bbcd0dfc8667d7cd265971134af1fb1869 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 16 Jul 2026 16:19:13 +0300 Subject: [PATCH 04/10] Hold the Tango adapter to real PyTango, not to its own fake The unit ACL suite fakes the `tango` module, and it passes 19 out of 19. That number meant less than it looked. A fake written alongside the adapter encodes the same assumptions as the adapter, so the two agree precisely where both are wrong about the library, and the suite confirms its author rather than PyTango. Driving the real adapter against a real device found two defects the fake had certified as correct: - A DevEnum read returns a bare int from `read_attribute`, so the adapter's `hasattr(value, "name")` probe misses and it stringifies the ordinal: a shutter reads as '1', never 'OPEN'. It fails silently, because `kind='Categorical'` promises a label, so a comparison to 'OPEN' just never matches. The `.name` branch is not careless, it is correct for DevState; the mistake was generalising a true DevState fact one step too far, and the fake agreed. - Tango discards the value on ATTR_INVALID but keeps `data_format`, so an INVALID spectrum or image reaches `tuple(None)` and raises a bare TypeError. It is not a port error family, so the Conductor's deliberately closed `_CONTROL_ERRORS` tuple does not catch it and it crashes the conduct task. The trigger is an ordinary unarmed detector. The fake paired a live value with ATTR_INVALID, a combination the Tango core cannot produce, so that cell was never crossed. Both are pinned here as strict xfail asserting the behaviour we want, so their fix flips them green and `strict` refuses to let a stale marker survive. The rest of the module is the coverage the fake could not vouch for: every MeasurementKind branch, quality collapse, the write round-trip and its coercion failure, access-denied, subscribe fan-out, aclose. The old rationale for faking was false on both clauses, so it is gone rather than reworded. `DeviceTestContext` ships inside PyTango and runs a real device server in-process with no Tango database and no subprocess, which makes this cheaper than the EPICS softIOC fixture it mirrors, and CI has had PyTango all along because setup runs `uv sync --all-extras`. The fake keeps its tier: it is still the right tool for injecting a failure by reason, which is awkward against a live device. Two Tango constraints the EPICS pattern does not carry are documented in the module: PyTango binds one process-global executor to the first loop it sees, so the loop scope is pinned to session locally rather than changing the global default, and omniORB allows one DeviceTestContext per process, which the module-scoped fixture satisfies. Timeout, event-error, read-failure-class and authorisation paths are left out of scope with reasons, each landing with the fix it belongs to. Co-Authored-By: Claude Opus 4.8 --- .../integration/test_tango_control_port.py | 336 ++++++++++++++++++ .../operation/test_tango_control_port_acl.py | 21 +- 2 files changed, 350 insertions(+), 7 deletions(-) create mode 100644 apps/api/tests/integration/test_tango_control_port.py diff --git a/apps/api/tests/integration/test_tango_control_port.py b/apps/api/tests/integration/test_tango_control_port.py new file mode 100644 index 00000000000..7b89e1d3c5d --- /dev/null +++ b/apps/api/tests/integration/test_tango_control_port.py @@ -0,0 +1,336 @@ +"""Integration tests: `TangoControlPort` (PyTango) against a real in-process device. + +Production Tango adapter of the control-port arc per +[[project_control_port_design]] + +[[project_control_port_generalization_research]]. Real PyTango client +(pytango / cppTango over omniORB) talking to a real Tango device server +that `tango.test_context.DeviceTestContext` runs inside the test process. + +The Tango twin of the EPICS `softioc` fixture, and a cheaper one: there is +no Tango database and no subprocess. The context mints a no-database TRL +(`tango://127.0.0.1:/test/nodb/controlportprobe#dbase=no`) on a +loopback port and serves it from a thread, so nothing is faked between the +adapter and the Tango core. + +The module-scoped `tango_device` fixture stays local rather than moving to +`tests/integration/conftest.py`: `softioc` is shared because two modules +need it, and this is the only Tango module. Move it when a second arrives. + +Test pattern: write-then-read for any value assertion, since device state +persists across tests within the module. Quality and failure paths do not +mutate state and stay order-independent. + +## Two Tango-specific constraints the EPICS pattern does not carry + + - Session-scoped event loop is mandatory. PyTango's asyncio green mode + caches one process-global executor bound to the first running loop it + ever sees. Under the repo's default function-scoped loop, only the + first async Tango test per worker passes and the rest die with + `RuntimeError: Event loop is closed`. Module scope is not enough + either once a second Tango module exists, so this pins `session` + locally rather than moving the global default. + - One `DeviceTestContext` per process (omniORB runs a single ORB, and a + second context's `stop()` raises `BAD_INV_ORDER_ORBHasShutdown`). The + module-scoped fixture satisfies this. xdist workers are separate + processes, so `-n 4` is unaffected. + +## Coverage + + - Protocol conformance via `isinstance` (no device) + - Every `MeasurementKind` branch (Scalar / Array / Image / Categorical) + - `Quality=Uncertain` via a device-declared ATTR_WARNING attribute + - Write round-trip on a scalar, and the value-coercion failure path + - `ControlAccessDeniedError` via API_AttrNotWritable on a read-only + attribute + - subscribe initial synchronised event + post-write fan-out + - aclose idempotency + - Two known defects pinned as strict xfail, so their fix flips them + green and `strict` forbids leaving the marker behind + +Out of scope: + + - Timeout paths (the adapter never calls `set_timeout_millis`, so + PyTango's 3000 ms client default caps every call): lands with that + fix, since asserting it here would pin a wall-clock defect. + - Read failures surfacing as `ControlWriteRejectedError`: real, but the + correct class is an open port-taxonomy question (`ControlPort` has no + read-failure family), so this waits on that decision rather than + asserting a class we have not chosen. + - Event-error handling (`API_EventTimeout` is treated as terminal + though Tango self-heals): needs ~10 s of real heartbeat per + assertion; lands with that fix. + - `set_access_control` authorisation refusals (`API_ReadOnlyMode`): the + adapter matches an `API_UnauthorizedAccess` literal cppTango never + emits; lands with that fix. +""" + +import asyncio +import time +from collections.abc import Iterator + +import pytest + +pytest.importorskip("tango", reason="TangoControlPort needs the optional 'tango' extra") + +from tango import AttrQuality, AttrWriteType, DevState +from tango.server import Device, attribute +from tango.test_context import DeviceTestContext + +from cora.operation.adapters.tango_control_port import TangoControlPort +from cora.operation.ports.control_address import TangoAttributeAddress +from cora.operation.ports.control_port import ( + ControlAccessDeniedError, + ControlPort, + ControlValueCoercionError, + Measurement, +) + +pytestmark = pytest.mark.asyncio(loop_scope="session") + + +class ControlPortProbe(Device): + """Device exposing one attribute per adapter branch under test.""" + + _scalar = 2.5 + _shutter = 1 + _pushed = 0.0 + + def init_device(self) -> None: + super().init_device() + self.set_change_event("pushed", True, False) + + @attribute(dtype=float, access=AttrWriteType.READ_WRITE) + def double_scalar(self) -> float: + return self._scalar + + @double_scalar.write + def double_scalar(self, value: float) -> None: + self._scalar = value + + @attribute(dtype=(float,), max_dim_x=3) + def spectrum(self) -> list[float]: + return [1.0, 2.0, 3.0] + + @attribute(dtype=((int,),), max_dim_x=2, max_dim_y=2) + def image(self) -> list[list[int]]: + return [[1, 2], [3, 4]] + + @attribute(dtype=DevState) + def state_attr(self) -> DevState: + return DevState.RUNNING + + @attribute( + dtype="DevEnum", + enum_labels=["CLOSED", "OPEN", "FAULT"], + access=AttrWriteType.READ_WRITE, + ) + def shutter(self) -> int: + return self._shutter + + @shutter.write + def shutter(self, value: int) -> None: + self._shutter = value + + @attribute(dtype=float) + def warned(self) -> tuple[float, float, AttrQuality]: + return 1.0, time.time(), AttrQuality.ATTR_WARNING + + @attribute(dtype=(float,), max_dim_x=3) + def invalid_spectrum(self) -> tuple[list[float], float, AttrQuality]: + return [1.0, 2.0, 3.0], time.time(), AttrQuality.ATTR_INVALID + + @attribute(dtype=float, access=AttrWriteType.READ) + def readonly_attr(self) -> float: + return 1.0 + + @attribute(dtype=float, access=AttrWriteType.READ_WRITE) + def pushed(self) -> float: + return self._pushed + + @pushed.write + def pushed(self, value: float) -> None: + self._pushed = value + self.push_change_event("pushed", value) + + +@pytest.fixture(scope="module") +def tango_device() -> Iterator[str]: + """Real in-process Tango device server; yields its no-database TRL.""" + context = DeviceTestContext(ControlPortProbe, process=False) + context.start() + try: + yield context.get_device_access() + finally: + context.stop() + + +def _addr(device: str, attribute_name: str) -> TangoAttributeAddress: + """The typed address `ControlPortRegistry` parses out of a route match.""" + return TangoAttributeAddress(device=device, attribute=attribute_name) + + +@pytest.mark.integration +async def test_tango_control_port_satisfies_control_port_protocol() -> None: + assert isinstance(TangoControlPort(), ControlPort) + + +@pytest.mark.integration +async def test_read_double_scalar_returns_good_scalar_measurement( + tango_device: str, +) -> None: + port = TangoControlPort() + try: + await port.write(_addr(tango_device, "double_scalar"), 2.5) + reading = await port.read(_addr(tango_device, "double_scalar")) + assert isinstance(reading, Measurement) + assert reading.kind == "Scalar" + assert reading.quality == "Good" + assert reading.value == 2.5 + assert reading.quality_detail == "" + assert reading.name == "double_scalar" + assert reading.produced_at.tzinfo is not None + finally: + await port.aclose() + + +@pytest.mark.integration +async def test_write_scalar_then_read_observes_written_value(tango_device: str) -> None: + port = TangoControlPort() + try: + await port.write(_addr(tango_device, "double_scalar"), 4.2) + reading = await port.read(_addr(tango_device, "double_scalar")) + assert reading.value == 4.2 + finally: + await port.aclose() + + +@pytest.mark.integration +async def test_read_spectrum_returns_array_of_python_floats(tango_device: str) -> None: + port = TangoControlPort() + try: + reading = await port.read(_addr(tango_device, "spectrum")) + assert reading.kind == "Array" + assert reading.value == (1.0, 2.0, 3.0) + assert all(type(item) is float for item in reading.value) + finally: + await port.aclose() + + +@pytest.mark.integration +async def test_read_image_returns_tuple_of_row_tuples(tango_device: str) -> None: + port = TangoControlPort() + try: + reading = await port.read(_addr(tango_device, "image")) + assert reading.kind == "Image" + assert reading.value == ((1, 2), (3, 4)) + assert all(type(cell) is int for row in reading.value for cell in row) + finally: + await port.aclose() + + +@pytest.mark.integration +async def test_read_devstate_returns_categorical_state_name(tango_device: str) -> None: + """DevState really does arrive as an IntEnum, so the adapter's `.name` branch fits.""" + port = TangoControlPort() + try: + reading = await port.read(_addr(tango_device, "state_attr")) + assert reading.kind == "Categorical" + assert reading.value == "RUNNING" + finally: + await port.aclose() + + +@pytest.mark.integration +@pytest.mark.xfail( + strict=True, + reason="DevEnum labels are not resolved: read_attribute returns a bare int, so " + "the adapter stringifies the ordinal. Needs get_attribute_config().enum_labels.", +) +async def test_read_devenum_returns_categorical_label(tango_device: str) -> None: + port = TangoControlPort() + try: + reading = await port.read(_addr(tango_device, "shutter")) + assert reading.kind == "Categorical" + assert reading.value == "OPEN" + finally: + await port.aclose() + + +@pytest.mark.integration +async def test_read_warning_quality_collapses_to_uncertain(tango_device: str) -> None: + port = TangoControlPort() + try: + reading = await port.read(_addr(tango_device, "warned")) + assert reading.quality == "Uncertain" + assert "ATTR_WARNING" in reading.quality_detail + finally: + await port.aclose() + + +@pytest.mark.integration +@pytest.mark.xfail( + strict=True, + reason="An ATTR_INVALID array raises a bare TypeError from _unpack_value: Tango " + "discards the value but keeps data_format, so tuple(None) escapes the port's " + "error families and the Conductor's closed catch tuple.", +) +async def test_read_invalid_quality_array_raises_value_coercion( + tango_device: str, +) -> None: + port = TangoControlPort() + try: + with pytest.raises(ControlValueCoercionError): + await port.read(_addr(tango_device, "invalid_spectrum")) + finally: + await port.aclose() + + +@pytest.mark.integration +async def test_write_to_readonly_attribute_raises_access_denied( + tango_device: str, +) -> None: + port = TangoControlPort() + try: + with pytest.raises(ControlAccessDeniedError): + await port.write(_addr(tango_device, "readonly_attr"), 1.0) + finally: + await port.aclose() + + +@pytest.mark.integration +async def test_write_wrong_value_type_raises_value_coercion(tango_device: str) -> None: + port = TangoControlPort() + try: + with pytest.raises(ControlValueCoercionError): + await port.write(_addr(tango_device, "double_scalar"), "not-a-number") + finally: + await port.aclose() + + +@pytest.mark.integration +async def test_subscribe_yields_initial_event_then_one_per_write( + tango_device: str, +) -> None: + port = TangoControlPort() + address = _addr(tango_device, "pushed") + try: + await port.write(address, 0.0) + iterator = port.subscribe(address) + first = await asyncio.wait_for(anext(iterator), timeout=8.0) + assert first.kind == "Scalar" + assert first.quality == "Good" + + await port.write(address, 7.7) + second = await asyncio.wait_for(anext(iterator), timeout=8.0) + assert second.value == 7.7 + await iterator.aclose() + finally: + await port.aclose() + + +@pytest.mark.integration +async def test_aclose_after_read_is_idempotent(tango_device: str) -> None: + port = TangoControlPort() + await port.read(_addr(tango_device, "double_scalar")) + await port.aclose() + await port.aclose() diff --git a/apps/api/tests/unit/operation/test_tango_control_port_acl.py b/apps/api/tests/unit/operation/test_tango_control_port_acl.py index ac42f4a488e..664433fc100 100644 --- a/apps/api/tests/unit/operation/test_tango_control_port_acl.py +++ b/apps/api/tests/unit/operation/test_tango_control_port_acl.py @@ -1,12 +1,19 @@ """Unit-tier ACL tests for `TangoControlPort` translation + error mapping. -Mirrors `test_epics_pva_control_port_acl.py`. PyTango is a heavy native -dependency that is not installed in the base test environment and CI has no -TangoTest device the way it runs a live EPICS softIOC, so these tests inject a -fake `tango` / `tango.asyncio` module into `sys.modules`, neutralise the -import probe, and pre-seed the adapter's per-device proxy cache. That exercises -every ACL branch (kind classification, quality collapse, DevFailed reason -mapping, subscribe lifecycle) without a live Tango control plane. +Mirrors `test_epics_pva_control_port_acl.py`. These tests inject a fake +`tango` / `tango.asyncio` module into `sys.modules`, neutralise the import +probe, and pre-seed the adapter's per-device proxy cache, so every ACL branch +(kind classification, quality collapse, DevFailed reason mapping, subscribe +lifecycle) runs with no substrate and no optional extra installed. Injecting a +failure by reason is also far easier against a double than against a device. + +The fake earns its place at this tier only. It is not evidence that the +adapter matches PyTango: a fake written alongside the adapter encodes the same +assumptions, so both agree even where the real library disagrees with both. +`tests/integration/test_tango_control_port.py` drives the real adapter against +a real device server and is what holds the adapter to PyTango's actual +behaviour. Keep the two in step: when a branch here asserts a shape, the +integration module is where that shape is confirmed to be real. Coverage: From be04e68034a71be0a1b186e1b2d0edef36bfe9ea Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 16 Jul 2026 16:30:18 +0300 Subject: [PATCH 05/10] Read an invalid Tango attribute as no value, not as a crash On ATTR_INVALID the Tango core discards the value but keeps data_format, so an invalid spectrum still classified as Array while attr.value was None, and _unpack_value reached tuple(None). The resulting TypeError is not a ControlPort error family, so the Conductor's deliberately closed _CONTROL_ERRORS tuple did not catch it and it escaped the step loop and crashed the conduct task. The trigger is nothing exotic: an unarmed detector, an offline camera, an attribute on a device in FAULT. _unpack_value now returns None whenever the attribute carries no value, before any kind branch. That also fixes a quieter case in the same branch: a Categorical invalid read used to stringify None into the literal label 'None', a truthy string a consumer cannot tell from a real one. This does NOT raise ControlValueCoercionError, which was the obvious candidate. Three reasons. An invalid scalar already reads back as None/Bad today, so raising only for arrays would make one physical condition, an invalid attribute, produce either a Measurement or an exception depending on nothing but data format, which a caller cannot reason about. Raising also discards the forensic pairing of quality=Bad with quality_detail=attr_quality=ATTR_INVALID that the Conductor records against the failed step, which is the evidence an operator needs. And the error itself is the wrong shape: it takes (address, raw_type, target_kind) and means a value whose TYPE will not coerce, which is not what an absent value is. Whether an absent value is usable is the consumer's judgment, and the consumers already make it correctly: _require_finite_number(None) raises ControlValueCoercionError, naming the kind IT needs, and a check criterion treats None as a clean mismatch. Both degrade to a structured step failure. _to_reading stays outside read()'s try. Making _unpack_value total is what closes this; catching translation failures around it would launder a genuine CORA bug into a step failure, and the Conductor's no-catch-all posture deliberately lets those surface. The fake could not have caught this and now can. _DeviceAttribute took value and quality as independent kwargs and paired a live value with ATTR_INVALID, which the Tango core cannot produce, so the invalid-array cell was unreachable and the suite stayed green over the crash. It now couples them the way Tango does. Removing the guard makes the repaired fake fail 3 of the 4 new cases (Array, Image, Categorical; Scalar never crashed), so the unit tier now falsifies this on its own. The integration test asserts the reading rather than the raise, and its strict xfail is gone: the real-device case passes. Co-Authored-By: Claude Opus 4.8 --- .../operation/adapters/tango_control_port.py | 19 +++++++- .../integration/test_tango_control_port.py | 26 ++++++----- .../operation/test_tango_control_port_acl.py | 43 ++++++++++++++++++- 3 files changed, 75 insertions(+), 13 deletions(-) diff --git a/apps/api/src/cora/operation/adapters/tango_control_port.py b/apps/api/src/cora/operation/adapters/tango_control_port.py index 74e2c604328..68e3abe82b6 100644 --- a/apps/api/src/cora/operation/adapters/tango_control_port.py +++ b/apps/api/src/cora/operation/adapters/tango_control_port.py @@ -175,8 +175,25 @@ def _kind_for(attr: Any) -> MeasurementKind: def _unpack_value(attr: Any, kind: MeasurementKind) -> Any: - """Extract a Python-native value from a Tango `DeviceAttribute`.""" + """Extract a Python-native value from a Tango `DeviceAttribute`. + + Returns `None` when the attribute carries no value, whatever its kind. + On ATTR_INVALID the Tango core discards the value but keeps + `data_format`, so an invalid spectrum still classifies as `Array` while + `attr.value` is `None`: unpacking it as a sequence would raise a bare + `TypeError` that is not a `ControlPort` error family and so escapes the + Conductor's closed catch tuple. `None` instead keeps an invalid read + shaped like every other invalid read (an invalid scalar already reads + back as `None`), and preserves the forensic pairing of `quality='Bad'` + with `quality_detail=attr_quality=ATTR_INVALID` that the Conductor + records against the failed step. Deciding whether a value is usable is + the consumer's call, not this adapter's: `_require_finite_number` + already turns a `None` capture into a structured step failure, and a + check criterion already treats it as a clean mismatch. + """ value: Any = getattr(attr, "value", None) + if value is None: + return None if kind == "Image": if hasattr(value, "tolist"): return tuple(tuple(row) for row in value.tolist()) diff --git a/apps/api/tests/integration/test_tango_control_port.py b/apps/api/tests/integration/test_tango_control_port.py index 7b89e1d3c5d..c2b2ad2afb2 100644 --- a/apps/api/tests/integration/test_tango_control_port.py +++ b/apps/api/tests/integration/test_tango_control_port.py @@ -39,12 +39,14 @@ - Protocol conformance via `isinstance` (no device) - Every `MeasurementKind` branch (Scalar / Array / Image / Categorical) - `Quality=Uncertain` via a device-declared ATTR_WARNING attribute + - `Quality=Bad` with no value via a device-declared ATTR_INVALID + spectrum (the shape the Tango core forces on an invalid read) - Write round-trip on a scalar, and the value-coercion failure path - `ControlAccessDeniedError` via API_AttrNotWritable on a read-only attribute - subscribe initial synchronised event + post-write fan-out - aclose idempotency - - Two known defects pinned as strict xfail, so their fix flips them + - The DevEnum label defect pinned as strict xfail, so its fix flips it green and `strict` forbids leaving the marker behind Out of scope: @@ -268,19 +270,23 @@ async def test_read_warning_quality_collapses_to_uncertain(tango_device: str) -> @pytest.mark.integration -@pytest.mark.xfail( - strict=True, - reason="An ATTR_INVALID array raises a bare TypeError from _unpack_value: Tango " - "discards the value but keeps data_format, so tuple(None) escapes the port's " - "error families and the Conductor's closed catch tuple.", -) -async def test_read_invalid_quality_array_raises_value_coercion( +async def test_read_invalid_quality_array_returns_bad_reading_with_no_value( tango_device: str, ) -> None: + """Tango discards the value on ATTR_INVALID but keeps data_format. + + So the kind stays `Array` with nothing to unpack. The reading carries + that as a None value against Bad quality rather than raising, which is + what an invalid scalar already does, and keeps the ATTR_INVALID + breadcrumb the Conductor records against the failed step. + """ port = TangoControlPort() try: - with pytest.raises(ControlValueCoercionError): - await port.read(_addr(tango_device, "invalid_spectrum")) + reading = await port.read(_addr(tango_device, "invalid_spectrum")) + assert reading.kind == "Array" + assert reading.value is None + assert reading.quality == "Bad" + assert "ATTR_INVALID" in reading.quality_detail finally: await port.aclose() diff --git a/apps/api/tests/unit/operation/test_tango_control_port_acl.py b/apps/api/tests/unit/operation/test_tango_control_port_acl.py index 664433fc100..26d9216b807 100644 --- a/apps/api/tests/unit/operation/test_tango_control_port_acl.py +++ b/apps/api/tests/unit/operation/test_tango_control_port_acl.py @@ -102,7 +102,16 @@ def totime(self) -> float: class _DeviceAttribute: - """Stand-in for `tango.DeviceAttribute` with the fields the ACL unpacks.""" + """Stand-in for `tango.DeviceAttribute` with the fields the ACL unpacks. + + ATTR_INVALID forces `value` to None whatever the caller passes, because + the Tango core discards the value on an invalid read while keeping + `data_format`. Coupling the two here is what stops this double from + expressing a reading the real substrate cannot produce: an earlier + version took value and quality as independent kwargs, so a live value + paired with ATTR_INVALID looked reasonable, and the invalid-array cell + (where the adapter unpacked None as a sequence) was never reachable. + """ def __init__( self, @@ -114,7 +123,7 @@ def __init__( seconds: float = 0.0, name: str = "attr", ) -> None: - self.value = value + self.value = None if quality == "ATTR_INVALID" else value self.quality = _Enum(quality) self.data_format = _Enum(data_format) self.type = _Enum(attr_type) @@ -258,6 +267,36 @@ async def test_read_collapses_attr_quality( assert attr_quality in reading.quality_detail +@pytest.mark.unit +@pytest.mark.parametrize( + ("data_format", "attr_type"), + [ + ("SCALAR", "DevDouble"), + ("SPECTRUM", "DevDouble"), + ("IMAGE", "DevLong"), + ("SCALAR", "DevEnum"), + ], +) +async def test_read_invalid_quality_returns_none_value_for_every_data_format( + fake_tango: None, data_format: str, attr_type: str +) -> None: + """An invalid read carries no value, and says so the same way at every kind. + + The kind still comes from `data_format` (Tango keeps it on an invalid + read), so this pins that an invalid SPECTRUM stays `Array` while its + value is None, rather than being unpacked as a sequence. + """ + port = _make_port( + read_result=_DeviceAttribute( + value=1.0, quality="ATTR_INVALID", data_format=data_format, attr_type=attr_type + ) + ) + reading = await port.read(_ADDR) + assert reading.value is None + assert reading.quality == "Bad" + assert "ATTR_INVALID" in reading.quality_detail + + @pytest.mark.unit async def test_read_timeout_translates_to_not_connected(fake_tango: None) -> None: """A `wait_for` timeout on read maps to NotConnected (slow == absent at this tier).""" From 8e399003ef8d7bea3dbac10b0adadaed1457c0fb Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 16 Jul 2026 16:49:02 +0300 Subject: [PATCH 06/10] Resolve DevEnum labels from the attribute config, not from the value A DevEnum read returns a bare int. The adapter probed the value for a `.name` and, missing it, stringified the ordinal: a shutter read back as '1', never 'OPEN'. It failed silently, because kind='Categorical' promises a label, so a decider comparing a reading to 'OPEN' never matched and never raised. Labels are not on the value at all; they live in the attribute config, which the adapter never asked for. The `.name` branch stays and is checked first. It is not the bug: a DevState value really is an IntEnum carrying its own label, and DevState reports NO enum labels, so it cannot be served by the config path. The original mistake was generalising a true DevState fact to DevEnum, and since the fake supplied a `.name`-carrying object for both, nothing could contradict it. _resolve_enum_labels mirrors EpicsCaControlPort's: cached per address, cleared on aclose so a reopened port cannot trust labels from before a device restart. It caches the NEGATIVE too, which the EPICS twin does not need to: there, Categorical always means an enum, whereas DevState is a common Tango Categorical with no labels, so without caching the empty result every DevState read would re-fetch config forever. A config fetch is 0.07 ms on loopback but a real CORBA round trip on a beamline network, which is why it stays off the conduct loop's per-read path. A failed config fetch falls back to the ordinal rather than raising: the read itself succeeded, and turning a readable attribute into a failed step because its config was momentarily unreachable would be a regression, not a fix. An ordinal outside the label range falls back the same way. A DevEnum SPECTRUM stays an Array of ordinals, documented rather than fixed. `_kind_for` sorts on data format before type, and a per-element label array has no consumer today; inventing one here would be scope, not correctness. The fake now models what PyTango returns: a bare ordinal on the value, a configurable enum_labels on get_attribute_config, and empty labels by default because that is what every non-enum attribute reports. Reverting the adapter to the old `.name`-only branch now fails the unit tier on its own, so the fake finally falsifies this instead of certifying it. Two integration tests cover what the fake structurally cannot: the label against a real device (its strict xfail is gone, and strict is what caught the XPASS), and a write-then-read pair proving the per-address cache stores labels rather than a resolved label. Both write before reading, per the module's own rule, so neither depends on running first. Co-Authored-By: Claude Opus 4.8 --- .../operation/adapters/tango_control_port.py | 85 ++++++++++++++--- .../integration/test_tango_control_port.py | 24 +++-- .../operation/test_tango_control_port_acl.py | 92 ++++++++++++++++++- 3 files changed, 179 insertions(+), 22 deletions(-) diff --git a/apps/api/src/cora/operation/adapters/tango_control_port.py b/apps/api/src/cora/operation/adapters/tango_control_port.py index 68e3abe82b6..e8e099be906 100644 --- a/apps/api/src/cora/operation/adapters/tango_control_port.py +++ b/apps/api/src/cora/operation/adapters/tango_control_port.py @@ -46,7 +46,13 @@ `type == DevEnum` or a `DevState` value -> "Categorical"; else "Scalar". - `value`: numpy scalars via `.item()`; spectra via `tuple(...)`; images via tuple-of-tuples; `DevEnum` resolved to its label string via the attribute - config's enum labels; `DevState` via its name. + config's enum labels, cached per address and falling back to the + stringified ordinal when the config is unreachable; `DevState` via its + name (it arrives as a real IntEnum and reports no enum labels). `None` + whenever the attribute carries no value, which is what an ATTR_INVALID + read is. A `DevEnum` SPECTRUM stays an `Array` of ordinals: the label + lookup covers the scalar case only, since a per-element label array has + no consumer yet. - `quality` (`AttrQuality` -> `Quality`): `ATTR_VALID` -> Good; `ATTR_WARNING` / `ATTR_CHANGING` -> Uncertain; `ATTR_ALARM` / `ATTR_INVALID` -> Bad. @@ -174,7 +180,7 @@ def _kind_for(attr: Any) -> MeasurementKind: return "Scalar" -def _unpack_value(attr: Any, kind: MeasurementKind) -> Any: +def _unpack_value(attr: Any, kind: MeasurementKind, enum_labels: tuple[str, ...] | None) -> Any: """Extract a Python-native value from a Tango `DeviceAttribute`. Returns `None` when the attribute carries no value, whatever its kind. @@ -203,12 +209,15 @@ def _unpack_value(attr: Any, kind: MeasurementKind) -> Any: return tuple(value.tolist()) return tuple(value) if kind == "Categorical": - # DevState values stringify to their state name; DevEnum values - # carry a `.name` when PyTango resolves the label, else fall back - # to the raw string form. + # DevState arrives as a real IntEnum carrying its own label, so it + # needs no config lookup (and reports no enum_labels). DevEnum + # arrives as a bare int and is resolved against the cached labels. if hasattr(value, "name"): return value.name - return str(value) + index = int(value) + if enum_labels is not None and 0 <= index < len(enum_labels): + return enum_labels[index] + return str(index) scalar: Any = value if hasattr(scalar, "item"): scalar = scalar.item() @@ -217,10 +226,15 @@ def _unpack_value(attr: Any, kind: MeasurementKind) -> Any: return scalar -def _to_reading(attr: Any) -> Measurement: - """Translate a Tango `DeviceAttribute` to `Measurement`.""" +def _to_reading(attr: Any, enum_labels: tuple[str, ...] | None = None) -> Measurement: + """Translate a Tango `DeviceAttribute` to `Measurement`. + + `enum_labels` resolves a DevEnum ordinal to its label; None leaves the + ordinal stringified, which is the right fallback for a non-enum or an + unreachable attribute config. + """ kind = _kind_for(attr) - value = _unpack_value(attr, kind) + value = _unpack_value(attr, kind, enum_labels) quality = getattr(attr, "quality", None) time_val = getattr(attr, "time", None) timestamp = time_val.totime() if time_val is not None else 0.0 @@ -270,8 +284,44 @@ def __init__(self, *, default_timeout_s: float = _DEFAULT_TIMEOUT_S) -> None: require_tango("tango") self._default_timeout_s = default_timeout_s self._proxies: dict[str, Any] = {} + self._enum_labels: dict[str, tuple[str, ...]] = {} self._closed = False + async def _resolve_enum_labels(self, address: TangoAttributeAddress) -> tuple[str, ...] | None: + """Cache a DevEnum attribute's labels per address via its attribute config. + + Mirrors `EpicsCaControlPort._resolve_enum_labels`. Labels live in the + attribute CONFIG, never on the read value, so this costs one extra + round trip at first encounter; caching keeps it off the conduct + loop's per-read path, where on a real Tango network (unlike + loopback) it would be a real CORBA call per read. + + An empty tuple is cached and returned as None, meaning "not an enum". + Caching that negative matters here in a way it does not for EPICS: + `DevState` is a common Categorical that reports NO labels, so + without it every DevState read would re-fetch the config forever. + A config fetch that fails is not an error worth raising: the read + itself already succeeded, so fall back to the ordinal rather than + turn a readable attribute into a failed step. + """ + from tango import DevFailed + + raw = str(address) + cached = self._enum_labels.get(raw) + if cached is not None: + return cached or None + try: + proxy = await self._proxy_for(address.device) + config = await asyncio.wait_for( + proxy.get_attribute_config(address.attribute), + timeout=self._default_timeout_s, + ) + except (TimeoutError, DevFailed): + return None + labels = tuple(str(label) for label in getattr(config, "enum_labels", ())) + self._enum_labels[raw] = labels + return labels or None + async def _proxy_for(self, device: str) -> Any: """Return a cached asyncio `DeviceProxy` for `device`, building it lazily. @@ -300,7 +350,10 @@ async def read(self, address: TangoAttributeAddress) -> Measurement: raise ControlNotConnectedError(raw) from exc except DevFailed as exc: raise _map_dev_failed(raw, exc, timeout_s=self._default_timeout_s) from exc - return _to_reading(attr) + enum_labels: tuple[str, ...] | None = None + if _kind_for(attr) == "Categorical": + enum_labels = await self._resolve_enum_labels(address) + return _to_reading(attr, enum_labels) async def write( self, @@ -407,16 +460,21 @@ def _callback(event: Any) -> None: attr = getattr(event, "attr_value", None) if attr is None: continue - yield _to_reading(attr) + enum_labels: tuple[str, ...] | None = None + if _kind_for(attr) == "Categorical": + enum_labels = await self._resolve_enum_labels(address) + yield _to_reading(attr, enum_labels) finally: with contextlib.suppress(Exception): await proxy.unsubscribe_event(event_id) async def aclose(self) -> None: - """Drop cached device proxies; idempotent. + """Drop cached device proxies + enum labels; idempotent. Tango `DeviceProxy` objects release their CORBA connection on - garbage collection; dropping the cache is sufficient. Provided so + garbage collection; dropping the cache is sufficient. The enum-label + cache goes with them so a reopened port re-reads attribute config + rather than trusting labels from before a device restart. Provided so production code paths can call `aclose()` polymorphically against any `ControlPort`. """ @@ -424,6 +482,7 @@ async def aclose(self) -> None: return self._closed = True self._proxies.clear() + self._enum_labels.clear() __all__ = ["TangoControlPort"] diff --git a/apps/api/tests/integration/test_tango_control_port.py b/apps/api/tests/integration/test_tango_control_port.py index c2b2ad2afb2..124c695817c 100644 --- a/apps/api/tests/integration/test_tango_control_port.py +++ b/apps/api/tests/integration/test_tango_control_port.py @@ -44,10 +44,10 @@ - Write round-trip on a scalar, and the value-coercion failure path - `ControlAccessDeniedError` via API_AttrNotWritable on a read-only attribute + - DevEnum label resolution off the attribute config, and DevState off + the value's own name - subscribe initial synchronised event + post-write fan-out - aclose idempotency - - The DevEnum label defect pinned as strict xfail, so its fix flips it - green and `strict` forbids leaving the marker behind Out of scope: @@ -243,14 +243,10 @@ async def test_read_devstate_returns_categorical_state_name(tango_device: str) - @pytest.mark.integration -@pytest.mark.xfail( - strict=True, - reason="DevEnum labels are not resolved: read_attribute returns a bare int, so " - "the adapter stringifies the ordinal. Needs get_attribute_config().enum_labels.", -) async def test_read_devenum_returns_categorical_label(tango_device: str) -> None: port = TangoControlPort() try: + await port.write(_addr(tango_device, "shutter"), 1) reading = await port.read(_addr(tango_device, "shutter")) assert reading.kind == "Categorical" assert reading.value == "OPEN" @@ -258,6 +254,20 @@ async def test_read_devenum_returns_categorical_label(tango_device: str) -> None await port.aclose() +@pytest.mark.integration +async def test_read_devenum_after_write_reflects_the_new_label(tango_device: str) -> None: + """Labels are cached per address, so a later read must still track the value.""" + port = TangoControlPort() + try: + await port.write(_addr(tango_device, "shutter"), 2) + reading = await port.read(_addr(tango_device, "shutter")) + assert reading.value == "FAULT" + await port.write(_addr(tango_device, "shutter"), 0) + assert (await port.read(_addr(tango_device, "shutter"))).value == "CLOSED" + finally: + await port.aclose() + + @pytest.mark.integration async def test_read_warning_quality_collapses_to_uncertain(tango_device: str) -> None: port = TangoControlPort() diff --git a/apps/api/tests/unit/operation/test_tango_control_port_acl.py b/apps/api/tests/unit/operation/test_tango_control_port_acl.py index 26d9216b807..9b46421eb83 100644 --- a/apps/api/tests/unit/operation/test_tango_control_port_acl.py +++ b/apps/api/tests/unit/operation/test_tango_control_port_acl.py @@ -155,18 +155,34 @@ def __init__( read_raises: BaseException | None = None, write_raises: BaseException | None = None, events: tuple[Any, ...] = (), + enum_labels: tuple[str, ...] = (), + config_raises: BaseException | None = None, ) -> None: self._read_result = read_result self._read_raises = read_raises self._write_raises = write_raises self._events = events + self._enum_labels = enum_labels + self._config_raises = config_raises self.unsubscribed: list[int] = [] + self.config_reads: list[str] = [] async def read_attribute(self, _attribute: str) -> Any: if self._read_raises is not None: raise self._read_raises return self._read_result + async def get_attribute_config(self, attribute: str) -> Any: + """Enum labels live in the CONFIG, never on the read value. + + Empty labels is what a real non-enum attribute reports, including + DevState, so that is the default here. + """ + self.config_reads.append(attribute) + if self._config_raises is not None: + raise self._config_raises + return types.SimpleNamespace(enum_labels=self._enum_labels) + async def write_attribute(self, _attribute: str, _value: Any) -> None: if self._write_raises is not None: raise self._write_raises @@ -239,13 +255,85 @@ async def test_read_image_translates_to_image_kind(fake_tango: None) -> None: @pytest.mark.unit -async def test_read_devenum_translates_to_categorical_label(fake_tango: None) -> None: - port = _make_port(read_result=_DeviceAttribute(value=_Enum("OPEN"), attr_type="DevEnum")) +async def test_read_devenum_resolves_ordinal_to_label_from_config(fake_tango: None) -> None: + """A DevEnum read carries a bare ordinal; the label comes from the config.""" + port = _make_port( + read_result=_DeviceAttribute(value=1, attr_type="DevEnum"), + enum_labels=("CLOSED", "OPEN", "FAULT"), + ) reading = await port.read(_ADDR) assert reading.kind == "Categorical" assert reading.value == "OPEN" +@pytest.mark.unit +async def test_read_devstate_uses_value_name_without_reading_config(fake_tango: None) -> None: + """DevState arrives as a real IntEnum, so it needs no config round trip.""" + port = _make_port(read_result=_DeviceAttribute(value=_Enum("RUNNING"), attr_type="DevState")) + reading = await port.read(_ADDR) + assert reading.kind == "Categorical" + assert reading.value == "RUNNING" + + +@pytest.mark.unit +async def test_read_devenum_falls_back_to_ordinal_when_config_unreadable( + fake_tango: None, +) -> None: + """A readable attribute must not fail a step because its config is not.""" + port = _make_port( + read_result=_DeviceAttribute(value=2, attr_type="DevEnum"), + config_raises=_DevFailed(_DevError("API_AttrNotFound")), + ) + reading = await port.read(_ADDR) + assert reading.value == "2" + + +@pytest.mark.unit +async def test_read_devenum_ordinal_outside_labels_falls_back_to_ordinal( + fake_tango: None, +) -> None: + port = _make_port( + read_result=_DeviceAttribute(value=9, attr_type="DevEnum"), + enum_labels=("CLOSED", "OPEN"), + ) + reading = await port.read(_ADDR) + assert reading.value == "9" + + +@pytest.mark.unit +async def test_read_devenum_twice_reads_config_once(fake_tango: None) -> None: + """The label cache keeps the config round trip off the per-read path.""" + port = _make_port( + read_result=_DeviceAttribute(value=1, attr_type="DevEnum"), + enum_labels=("CLOSED", "OPEN"), + ) + await port.read(_ADDR) + await port.read(_ADDR) + proxy = port._proxies[_ADDR.device] # pyright: ignore[reportPrivateUsage] + assert proxy.config_reads == [_ADDR.attribute] + + +@pytest.mark.unit +async def test_read_devstate_twice_reads_config_once(fake_tango: None) -> None: + """The negative is cached too, or every DevState read pays a round trip.""" + port = _make_port(read_result=_DeviceAttribute(value=_Enum("FAULT"), attr_type="DevState")) + await port.read(_ADDR) + await port.read(_ADDR) + proxy = port._proxies[_ADDR.device] # pyright: ignore[reportPrivateUsage] + assert len(proxy.config_reads) <= 1 + + +@pytest.mark.unit +async def test_aclose_drops_the_enum_label_cache(fake_tango: None) -> None: + port = _make_port( + read_result=_DeviceAttribute(value=1, attr_type="DevEnum"), + enum_labels=("CLOSED", "OPEN"), + ) + await port.read(_ADDR) + await port.aclose() + assert port._enum_labels == {} # pyright: ignore[reportPrivateUsage] + + @pytest.mark.unit @pytest.mark.parametrize( ("attr_quality", "expected"), From 22aea4473eedb7d1fb78cee1ddcd7e8f102985ec Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 16 Jul 2026 17:10:48 +0300 Subject: [PATCH 07/10] Correct what free_localhost_port actually guarantees The docstring claimed "the kernel never reissues an in-use port to a concurrent bind, so each xdist worker gets a distinct port". That is true and it is what xdist needs, but it was carrying more weight than it can bear: the function closes the socket before returning, so the port is not reserved, and it comes from the same ephemeral range the kernel draws bind(0) from. _pin_epics_env calls it at session start while the softIOC does not bind until module setup, so the gap can be minutes wide. Investigated because a single run of the four control-port integration modules gave 43 passed and 14 ERROR-at-setup in the EPICS CA module, and this race was the suspect: a Tango integration module had just landed, and DeviceTestContext binds a dynamic loopback port in the same process. The suspicion was wrong, and the measurement says so. Forcing the collision rather than waiting for it: EPICS uses this one number for both its TCP server and its UDP name-search channel, which are separate port namespaces. A TCP thief is SURVIVABLE, 6 of 6 trials, because CAS falls back to another TCP port and UDP search still resolves the PV and redirects the client. Only a UDP thief is fatal, 2 of 2 trials, and it fails exactly as the observed run did, with the readiness poll timing out and every test in the module erroring at setup. DeviceTestContext binds TCP through omniORB, so it is the survivable case and cannot have caused those 14 errors. So no code change. The window is real but currently unreachable: nothing in the test tier binds an ephemeral UDP port. And the obvious fix would have missed anyway, which is the detail worth recording: socket.socket() is SOCK_STREAM, so holding this socket would reserve the TCP number, the harmless one, and leave the UDP number unreserved. Restructuring shared EPICS fixtures to close a hole that is both unreachable and on the other protocol is not a trade worth making, so the docstring now carries the measurements and the trigger for when it does become worth it. The 14 errors remain unexplained. The better-supported hypothesis is the 5.0s readiness deadline missed under transient load: that run took 57.3s against a 46.6-47.1s baseline over 12 later runs, all green. Left alone rather than tuned on one data point. Verified unchanged: 60 passed serially and under -n 4. Co-Authored-By: Claude Opus 4.8 --- apps/api/tests/integration/_softioc.py | 40 ++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/apps/api/tests/integration/_softioc.py b/apps/api/tests/integration/_softioc.py index 80926e4758f..d51effa82f9 100644 --- a/apps/api/tests/integration/_softioc.py +++ b/apps/api/tests/integration/_softioc.py @@ -248,8 +248,44 @@ def free_localhost_port() -> int: """Allocate a free loopback port via bind-and-close. - The kernel never reissues an in-use port to a concurrent bind, - so each xdist worker gets a distinct port even under `-n 4+`. + What this DOES guarantee, and what xdist needs: the kernel will not + hand the same port to two binds that are live at the same moment, so + two workers calling this concurrently always get distinct ports, even + under `-n 4+`. + + What it does NOT guarantee: a reservation. The socket is closed before + the number is returned, so the port is free again on return, and it + came from the ephemeral range the kernel draws `bind(0)` from + (49152-65535 on darwin, typically 32768-60999 on linux). Anything that + binds an ephemeral port afterwards can be handed it back. + `_pin_epics_env` calls this at SESSION start while the softIOC does not + bind until MODULE setup, so that gap can be minutes wide. + + Measured consequence of losing the race (darwin, PyTango 10.3.0 era), + which is narrower than it looks. EPICS uses this one number for BOTH + its TCP server and its UDP name-search channel, and those are separate + port namespaces: + + - A TCP thief is SURVIVABLE (6 of 6 forced trials): CAS falls back to + another TCP port and UDP name search still resolves the PV, so the + client is redirected and the fixture comes up green. + - A UDP thief is FATAL (2 of 2 forced trials): name search goes + unanswered, `wait_for_softioc_ready` times out, and every test in + the module ERRORs at setup. + + So the exposure today is nil, and note the asymmetry before "fixing" + this: `socket.socket()` is SOCK_STREAM, so holding this socket open + would reserve the TCP number, which is the harmless case, and would + still leave the UDP number (the fatal one) unreserved. Nothing in the + test tier binds an ephemeral UDP port; `DeviceTestContext` in + `test_tango_control_port.py` binds TCP via omniORB, the survivable + case. + + Revisit when something here starts binding ephemeral UDP. The fix then + is to reserve BOTH protocols on the number and hold them until the + consumer binds, which for the softIOC means releasing immediately + before `start_softioc` spawns the child that binds by number, and + re-reserving once it exits. """ with socket.socket() as s: s.bind(("127.0.0.1", 0)) From 7d999a376e6b1a6b6b922cd289bb7495721d1cfe Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:46:09 +0300 Subject: [PATCH 08/10] Finish the typed-address migration across the integration tier The registry-to-adapter seam moved the substrate adapters onto typed addresses, but seven integration modules kept calling the old surface. They all pass, because none of this is wrong at runtime: a test holding a bare EpicsCaControlPort and handing it a PV string still reads the right PV. It is wrong to the type checker, which is why the migration missed them and why only a full-tree pyright surfaced them. Two shapes. Tests that built a bare adapter and drove it with a str now route through a ControlPortRegistry, which is the component that owns the str surface and is what production wires via build_control_port; the adapter itself speaks EpicsPvAddress. Tests already using the registry just gained the substrate argument that selects the parse arm. `control_port_reuse` yields the registry now rather than the adapter, which is why its prefix parameter stops being ignored. The registry closes every route it holds, so the adapter still shuts down with the block. One case is not epics_ca: the 8-ID XPCS scenario registers an InMemoryControlPort, which is str-surfaced and has no address syntax to parse, so it takes register_str_port. Registering it as a substrate route made the registry parse an EpicsPvAddress and hand the typed variant to an adapter expecting a string, and that test was the one that caught it. The adapter's own error is a PyTango typing limitation, not ours: tango.asyncio.DeviceProxy is a partial over get_device_proxy whose declared return does not vary with green mode, so it types as a bare DeviceProxy while under Asyncio it returns an awaitable. The integration module proves the runtime shape, since wait_for on a non-awaitable would raise rather than pass, so this is a narrow ignore with the reason recorded. Full tree now green where it had never been run: pre-commit passes end to end, including pyright, tach and the architecture fitness functions. Tiers verified: 1080 integration, 12614 unit, 29560 architecture. Co-Authored-By: Claude Opus 4.8 --- .../operation/adapters/tango_control_port.py | 10 ++++++- .../scenarios/test_2bm_align_resolution.py | 4 +-- .../scenarios/test_2bm_align_rotation.py | 4 +-- .../scenarios/test_2bm_align_rotation_auto.py | 4 +-- .../scenarios/test_2bm_flat_field.py | 7 +++-- .../scenarios/test_8id_xpcs_stream.py | 2 +- ...t_acquisitions_against_softioc_postgres.py | 28 ++++++++++++----- ...tion_kind_gate_against_softioc_postgres.py | 8 ++--- ...test_conductor_against_softioc_postgres.py | 30 +++++++++++++++++-- .../integration/test_tango_control_port.py | 6 ++++ 10 files changed, 77 insertions(+), 26 deletions(-) diff --git a/apps/api/src/cora/operation/adapters/tango_control_port.py b/apps/api/src/cora/operation/adapters/tango_control_port.py index e8e099be906..99df7e9eeb4 100644 --- a/apps/api/src/cora/operation/adapters/tango_control_port.py +++ b/apps/api/src/cora/operation/adapters/tango_control_port.py @@ -333,7 +333,15 @@ async def _proxy_for(self, device: str) -> Any: if proxy is None: from tango.asyncio import DeviceProxy - proxy = await asyncio.wait_for(DeviceProxy(device), timeout=self._default_timeout_s) + # `tango.asyncio.DeviceProxy` is a partial over `get_device_proxy` + # whose declared return does not vary with green mode, so it types + # as a bare DeviceProxy. Under Asyncio it really returns an + # awaitable, which the integration module proves: `wait_for` on a + # non-awaitable would raise TypeError instead of passing. + proxy = await asyncio.wait_for( + DeviceProxy(device), # pyright: ignore[reportArgumentType] + timeout=self._default_timeout_s, + ) self._proxies[device] = proxy return proxy diff --git a/apps/api/tests/integration/scenarios/test_2bm_align_resolution.py b/apps/api/tests/integration/scenarios/test_2bm_align_resolution.py index f9bffa3aab4..8cdcfeb5cc4 100644 --- a/apps/api/tests/integration/scenarios/test_2bm_align_resolution.py +++ b/apps/api/tests/integration/scenarios/test_2bm_align_resolution.py @@ -248,7 +248,7 @@ async def test_align_resolution_recipe_conducts_compute_and_writes_calibration( # The pixel-size Measurement is seeded so fetch_measurements surfaces it. port = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, port, is_simulated=True) + registry.register(softioc, port, "epics_ca", is_simulated=True) compute_port = InMemoryComputePort() compute_port.set_next_measurements((_pixel_size_measurement(_MEASURED_PIXEL_SIZE_UM),)) step_store = PostgresActivityStore(db_pool) @@ -267,7 +267,7 @@ async def test_align_resolution_recipe_conducts_compute_and_writes_calibration( try: # Park the sample at the in-beam home the first setpoint expects. - await port.write(axis, _SAMPLE_IN_MM, wait=True) + await registry.write(axis, _SAMPLE_IN_MM, wait=True) # Recipe-driven conduct: empty caller steps re-expand the pinned template. result = await conduct( ConductProcedure(procedure_id=procedure_id, steps=()), diff --git a/apps/api/tests/integration/scenarios/test_2bm_align_rotation.py b/apps/api/tests/integration/scenarios/test_2bm_align_rotation.py index dc4c7af4eb3..b0fbd31d61c 100644 --- a/apps/api/tests/integration/scenarios/test_2bm_align_rotation.py +++ b/apps/api/tests/integration/scenarios/test_2bm_align_rotation.py @@ -266,7 +266,7 @@ async def test_align_rotation_recipe_conducts_compute_and_writes_calibration( # tuple under exercise). port = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, port, is_simulated=True) + registry.register(softioc, port, "epics_ca", is_simulated=True) compute_port = InMemoryComputePort() compute_port.set_next_measurements( ( @@ -291,7 +291,7 @@ async def test_align_rotation_recipe_conducts_compute_and_writes_calibration( try: # Park theta at the home the first rotation setpoint expects. - await port.write(theta, _THETA_ZERO_DEG, wait=True) + await registry.write(theta, _THETA_ZERO_DEG, wait=True) # Recipe-driven conduct: empty caller steps re-expand the pinned template. result = await conduct( ConductProcedure(procedure_id=procedure_id, steps=()), diff --git a/apps/api/tests/integration/scenarios/test_2bm_align_rotation_auto.py b/apps/api/tests/integration/scenarios/test_2bm_align_rotation_auto.py index b1e2e79c722..f04aa87fc0b 100644 --- a/apps/api/tests/integration/scenarios/test_2bm_align_rotation_auto.py +++ b/apps/api/tests/integration/scenarios/test_2bm_align_rotation_auto.py @@ -265,7 +265,7 @@ async def _build_align_routine( port = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, port, is_simulated=True) + registry.register(softioc, port, "epics_ca", is_simulated=True) compute_port = InMemoryComputePort() compute_port.set_measurement_sequence( tuple((_rotation_center_measurement(c),) for c in centers_sequence) @@ -286,7 +286,7 @@ async def _build_align_routine( ) conduct = bind_conduct_until_converged(deps, conductor=conductor, expansion_port=expander) # Park theta at the home the first rotation setpoint expects. - await port.write(theta, _THETA_ZERO_DEG, wait=True) + await registry.write(theta, _THETA_ZERO_DEG, wait=True) return deps, conduct, procedure_id, rotary_asset_id, registry, compute_port diff --git a/apps/api/tests/integration/scenarios/test_2bm_flat_field.py b/apps/api/tests/integration/scenarios/test_2bm_flat_field.py index 5801361efec..89dfde11156 100644 --- a/apps/api/tests/integration/scenarios/test_2bm_flat_field.py +++ b/apps/api/tests/integration/scenarios/test_2bm_flat_field.py @@ -325,7 +325,7 @@ async def test_flat_field_recipe_conducts_flats_against_softioc( # conduct observe Simulated. port = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, port, is_simulated=True) + registry.register(softioc, port, "epics_ca", is_simulated=True) step_store = PostgresActivityStore(db_pool) conductor = Conductor( control_port=registry, @@ -341,7 +341,7 @@ async def test_flat_field_recipe_conducts_flats_against_softioc( try: # The aligned home the CaptureStep observes + the restore returns to. - await port.write(axis, _SAMPLE_HOME_MM, wait=True) + await registry.write(axis, _SAMPLE_HOME_MM, wait=True) # The conduct->complete-Run glue conducts the phase Procedure (recipe- # driven: empty caller steps re-expand the pinned template) THEN completes # the parent Run carrying the conduct's observed kind. This is the @@ -424,7 +424,8 @@ async def test_flat_field_recipe_conducts_flats_against_softioc( assert restore_entry["payload"]["value"] == pytest.approx(_SAMPLE_HOME_MM) # The axis is back at the captured aligned home (the restore landed). - readback_port = EpicsCaControlPort() + readback_port = ControlPortRegistry() + readback_port.register(softioc, EpicsCaControlPort(), "epics_ca") try: axis_final = await readback_port.read(axis) assert axis_final.value == pytest.approx(_SAMPLE_HOME_MM) diff --git a/apps/api/tests/integration/scenarios/test_8id_xpcs_stream.py b/apps/api/tests/integration/scenarios/test_8id_xpcs_stream.py index 9f9495a36a5..e0dfbe37277 100644 --- a/apps/api/tests/integration/scenarios/test_8id_xpcs_stream.py +++ b/apps/api/tests/integration/scenarios/test_8id_xpcs_stream.py @@ -231,7 +231,7 @@ async def test_xpcs_stream_recipe_conducts_event_stream_and_leaves_no_observatio Measurement(value=_URI, kind="Categorical", quality="Good", produced_at=_NOW), ) registry = ControlPortRegistry() - registry.register(_DETECTOR, port, is_simulated=True) + registry.register_str_port(_DETECTOR, port, is_simulated=True) step_store = PostgresActivityStore(db_pool) conductor = Conductor( control_port=registry, diff --git a/apps/api/tests/integration/test_acquisitions_against_softioc_postgres.py b/apps/api/tests/integration/test_acquisitions_against_softioc_postgres.py index 524455f51dd..eb96108944f 100644 --- a/apps/api/tests/integration/test_acquisitions_against_softioc_postgres.py +++ b/apps/api/tests/integration/test_acquisitions_against_softioc_postgres.py @@ -31,6 +31,7 @@ from cora.infrastructure.event_envelope import to_new_event from cora.operation.acquisitions import collect, continuous, discrete +from cora.operation.adapters.control_port_registry import ControlPortRegistry from cora.operation.adapters.epics_ca_control_port import EpicsCaControlPort from cora.operation.aggregates.procedure import ( PostgresActivityStore, @@ -88,7 +89,7 @@ async def _seed_defined_procedure(deps_event_store: object, procedure_id: UUID) def _build_conductor( deps_event_store: object, db_pool: asyncpg.Pool, - control_port: EpicsCaControlPort, + control_port: ControlPortRegistry, *, clock: object, id_generator: object, @@ -138,7 +139,8 @@ async def test_conductor_runs_collect_action_against_real_softioc_and_postgres( ) await _seed_defined_procedure(deps.event_store, procedure_id) step_store = PostgresActivityStore(db_pool) - control_port = EpicsCaControlPort() + control_port = ControlPortRegistry() + control_port.register(softioc, EpicsCaControlPort(), "epics_ca") conductor = _build_conductor( deps.event_store, db_pool, @@ -230,7 +232,8 @@ async def test_conductor_runs_discrete_action_walks_axis_with_per_point_collects ) await _seed_defined_procedure(deps.event_store, procedure_id) step_store = PostgresActivityStore(db_pool) - control_port = EpicsCaControlPort() + control_port = ControlPortRegistry() + control_port.register(softioc, EpicsCaControlPort(), "epics_ca") conductor = _build_conductor( deps.event_store, db_pool, @@ -317,7 +320,8 @@ async def test_conductor_runs_continuous_action_with_axis_sweep_against_softioc( ) await _seed_defined_procedure(deps.event_store, procedure_id) step_store = PostgresActivityStore(db_pool) - control_port = EpicsCaControlPort() + control_port = ControlPortRegistry() + control_port.register(softioc, EpicsCaControlPort(), "epics_ca") conductor = _build_conductor( deps.event_store, db_pool, @@ -377,18 +381,26 @@ async def test_conductor_runs_continuous_action_with_axis_sweep_against_softioc( class control_port_reuse: # noqa: N801 - """Async-context manager that opens a fresh `EpicsCaControlPort` for assertions. + """Async-context manager that opens a fresh `ControlPort` for assertions. The Conductor's own port is `aclose()`d after `conduct()` returns; readback assertions against PVs the body just wrote need a NEW port. Wrapping with `async with` so the assertion block closes the port deterministically. + + Yields the registry, not the bare adapter, which is why the prefix is + no longer ignored: the registry carries the `str` address surface these + assertions read with, and it is what production wires via + `build_control_port`. `EpicsCaControlPort` itself speaks + `EpicsPvAddress`. The registry closes every route it holds, so the + adapter still shuts down with the block. """ - def __init__(self, _softioc_prefix: str) -> None: - self._port = EpicsCaControlPort() + def __init__(self, softioc_prefix: str) -> None: + self._port = ControlPortRegistry() + self._port.register(softioc_prefix, EpicsCaControlPort(), "epics_ca") - async def __aenter__(self) -> EpicsCaControlPort: + async def __aenter__(self) -> ControlPortRegistry: return self._port async def __aexit__(self, *_: object) -> None: diff --git a/apps/api/tests/integration/test_actuation_kind_gate_against_softioc_postgres.py b/apps/api/tests/integration/test_actuation_kind_gate_against_softioc_postgres.py index 9b726476167..8c1c53c9aac 100644 --- a/apps/api/tests/integration/test_actuation_kind_gate_against_softioc_postgres.py +++ b/apps/api/tests/integration/test_actuation_kind_gate_against_softioc_postgres.py @@ -122,7 +122,7 @@ async def test_simulated_route_conduct_yields_non_promotable_dataset( inner = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, inner, is_simulated=True) + registry.register(softioc, inner, "epics_ca", is_simulated=True) conductor = _conductor(deps, db_pool, registry) try: result = await conductor.conduct( @@ -184,7 +184,7 @@ async def test_physical_route_conduct_yields_promotable_dataset( inner = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, inner, is_simulated=False) + registry.register(softioc, inner, "epics_ca", is_simulated=False) conductor = _conductor(deps, db_pool, registry) try: result = await conductor.conduct( @@ -295,8 +295,8 @@ async def test_hybrid_route_conduct_yields_non_promotable_dataset( registry = ControlPortRegistry() # Longest-prefix match: the double_value PV is declared simulated, the # long_value PV physical. Both speak CA to the same soft IOC. - registry.register(f"{softioc}double_value", inner, is_simulated=True) - registry.register(f"{softioc}long_value", inner, is_simulated=False) + registry.register(f"{softioc}double_value", inner, "epics_ca", is_simulated=True) + registry.register(f"{softioc}long_value", inner, "epics_ca", is_simulated=False) conductor = _conductor(deps, db_pool, registry) try: result = await conductor.conduct( diff --git a/apps/api/tests/integration/test_conductor_against_softioc_postgres.py b/apps/api/tests/integration/test_conductor_against_softioc_postgres.py index 88c24c0439f..c0ab1b904dc 100644 --- a/apps/api/tests/integration/test_conductor_against_softioc_postgres.py +++ b/apps/api/tests/integration/test_conductor_against_softioc_postgres.py @@ -30,6 +30,7 @@ import pytest from cora.infrastructure.event_envelope import to_new_event +from cora.operation.adapters.control_port_registry import ControlPortRegistry from cora.operation.adapters.epics_ca_control_port import EpicsCaControlPort from cora.operation.aggregates.procedure import ( PostgresActivityStore, @@ -55,6 +56,29 @@ _CORRELATION_ID = UUID("01900000-0000-7000-8000-0000020c00aa") +def _control_port(softioc: str, *, default_timeout_s: float | None = None) -> ControlPortRegistry: + """Wire the CA adapter the way `build_control_port` does in production. + + The Conductor takes a `str`-addressed `ControlPort`, which is the + registry, not a substrate adapter: `EpicsCaControlPort` speaks + `EpicsPvAddress` and satisfies `SubstrateControlPort` instead. Handing + the bare adapter to the Conductor typechecks as an error even though it + runs, because `isinstance` against a runtime-checkable Protocol compares + method names and not signatures. + + `default_timeout_s` stays None unless a test needs a short one, so the + adapter keeps its own default rather than having it restated here. + """ + adapter = ( + EpicsCaControlPort() + if default_timeout_s is None + else EpicsCaControlPort(default_timeout_s=default_timeout_s) + ) + registry = ControlPortRegistry() + registry.register(softioc, adapter, "epics_ca") + return registry + + async def _seed_defined_procedure(deps_event_store: object, procedure_id: UUID) -> None: """Seed a single ProcedureRegistered event so the Procedure exists in `Defined`. @@ -128,7 +152,7 @@ async def test_conductor_runs_setpoint_check_against_real_softioc_and_postgres( complete_handler = bind_complete(deps) abort_handler = bind_abort(deps) append_step_handler = bind_append(deps, step_store=step_store) - control_port = EpicsCaControlPort() + control_port = _control_port(softioc) conductor = Conductor( control_port=control_port, append_step=append_step_handler, @@ -236,7 +260,7 @@ async def test_conductor_aborts_procedure_when_setpoint_fails_against_softioc( ) await _seed_defined_procedure(deps.event_store, procedure_id) step_store = PostgresActivityStore(db_pool) - control_port = EpicsCaControlPort(default_timeout_s=0.3) + control_port = _control_port(softioc, default_timeout_s=0.3) conductor = Conductor( control_port=control_port, append_step=bind_append(deps, step_store=step_store), @@ -307,7 +331,7 @@ async def test_conductor_completes_procedure_with_equals_check_against_softioc( ) await _seed_defined_procedure(deps.event_store, procedure_id) step_store = PostgresActivityStore(db_pool) - control_port = EpicsCaControlPort() + control_port = _control_port(softioc) conductor = Conductor( control_port=control_port, append_step=bind_append(deps, step_store=step_store), diff --git a/apps/api/tests/integration/test_tango_control_port.py b/apps/api/tests/integration/test_tango_control_port.py index 124c695817c..427934780a5 100644 --- a/apps/api/tests/integration/test_tango_control_port.py +++ b/apps/api/tests/integration/test_tango_control_port.py @@ -66,6 +66,12 @@ emits; lands with that fix. """ +# PyTango's server decorators are untyped: `@attribute` plus its `.write` +# sibling reads as a redeclaration of the same name, and the device methods +# it wraps come back partially unknown. Same posture as the EPICS +# integration modules, which suppress the same rules for aioca / p4p. +# pyright: reportUnknownMemberType=false, reportUnknownVariableType=false, reportRedeclaration=false + import asyncio import time from collections.abc import Iterator From 084754e8dca63f325db9e749be334068117974ed Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 16 Jul 2026 23:22:21 +0300 Subject: [PATCH 09/10] Name the registry's two doors after the protocols they accept Naming review of the series (R1-R6) found three real breaks, all in names this branch coined, so they are fixed here while they are still cheap. `register_str_port` reached for a Python type token where every other name in the series uses domain words, and it stalls read aloud. It is also a name that can go stale: if `ControlPort` ever stopped being `str`-surfaced, `register_str_port` becomes a lie while `register_control_port` stays true. The two doors were skeleton-mismatched too, a bare verb against `__`, when each door has an obvious name available: the Protocol it accepts. So `register` becomes `register_substrate_port` (takes a `SubstrateControlPort`) and `register_str_port` becomes `register_control_port` (takes a `ControlPort`). The mild `ControlPortRegistry.register_control_port` stutter is the cheaper cost. `_StrAddressSubstrate` broke R3 outright: its head noun said Substrate but the class is a `SubstrateControlPort` implementation, so the family noun was missing from last position and the name named the wrong family. Worse, `Substrate` is a live imported type ten lines above it, so a reader meets the word twice with two meanings. Now `_StrAddressSubstratePort`, and the test double follows as `_FakeSubstratePort`. `parse_control_address` took (substrate, raw) while the three variant `parse` staticmethods it dispatches to take (raw, substrate). One family, two argument orders. Aligned on subject-first, matching `strptime`. Renaming a method called `register` is a trap, because the projection registry, the tool registry, the signing registry and several lookups all have one and none of them should move. So the callers came from pyright rather than from a pattern match: rename the definition, let the type checker enumerate what broke. That found 51 sites across 12 files and no false ones. It has one blind spot worth recording. Pyright treats `# type: ignore` as a blanket line suppression rather than a mypy-style code-specific one, so a `registry.register(...) # type: ignore[arg-type]` in the registry's own aclose test stayed invisible to it and failed at runtime instead. The unit tier caught it. Static enumeration is the right instrument here, but it cannot see through an existing ignore. Green: pre-commit end to end, 12614 unit, 1080 integration. Co-Authored-By: Claude Opus 4.8 --- .../operation/adapters/control_port_config.py | 8 +-- .../adapters/control_port_registry.py | 22 +++--- .../cora/operation/ports/control_address.py | 4 +- .../scenarios/test_2bm_align_resolution.py | 2 +- .../scenarios/test_2bm_align_rotation.py | 2 +- .../scenarios/test_2bm_align_rotation_auto.py | 2 +- .../scenarios/test_2bm_flat_field.py | 4 +- .../scenarios/test_8id_xpcs_stream.py | 2 +- ...t_acquisitions_against_softioc_postgres.py | 8 +-- ...tion_kind_gate_against_softioc_postgres.py | 8 +-- ...test_conductor_against_softioc_postgres.py | 2 +- .../test_conduct_from_procedure_handler.py | 2 +- .../tests/unit/operation/test_conductor.py | 20 +++--- .../unit/operation/test_control_address.py | 16 ++--- .../operation/test_control_port_registry.py | 70 +++++++++---------- 15 files changed, 87 insertions(+), 85 deletions(-) diff --git a/apps/api/src/cora/operation/adapters/control_port_config.py b/apps/api/src/cora/operation/adapters/control_port_config.py index 8633ed662d9..b7fd310e833 100644 --- a/apps/api/src/cora/operation/adapters/control_port_config.py +++ b/apps/api/src/cora/operation/adapters/control_port_config.py @@ -58,7 +58,7 @@ def build_control_port(routes: Sequence[ControlPortRoute]) -> ControlPort: default + test convenience). Non-empty routes returns a `ControlPortRegistry` populated with the configured substrate adapters per prefix. `in_memory` routes go through - `register_str_port` (the registry wraps the `str`-surfaced adapter); + `register_control_port` (the registry wraps the `str`-surfaced adapter); the real substrate adapters register directly on the typed surface. """ if not routes: @@ -66,11 +66,11 @@ def build_control_port(routes: Sequence[ControlPortRoute]) -> ControlPort: registry = ControlPortRegistry() for route in routes: if route.substrate == "in_memory": - registry.register_str_port( + registry.register_control_port( route.prefix, InMemoryControlPort(), is_simulated=route.is_simulated ) else: - registry.register( + registry.register_substrate_port( route.prefix, _build_substrate(route.substrate), route.substrate, @@ -83,7 +83,7 @@ def _build_substrate(substrate: Substrate) -> SubstrateControlPort[Any]: """Construct the per-substrate typed adapter with deployment defaults. Handles the typed-address substrate adapters only; `in_memory` is - registered through `ControlPortRegistry.register_str_port` in + registered through `ControlPortRegistry.register_control_port` in `build_control_port` because it is `str`-surfaced. Per-adapter constructor kwargs (timeouts, etc.) ride on the diff --git a/apps/api/src/cora/operation/adapters/control_port_registry.py b/apps/api/src/cora/operation/adapters/control_port_registry.py index b4a85a50cb9..82c385d12e6 100644 --- a/apps/api/src/cora/operation/adapters/control_port_registry.py +++ b/apps/api/src/cora/operation/adapters/control_port_registry.py @@ -39,7 +39,7 @@ `InMemoryControlPort` is `str`-surfaced (it is also the caller-facing double used by 148 test sites), so it is not a `SubstrateControlPort`. -The registry wraps it in `_StrAddressSubstrate`, a thin shim that +The registry wraps it in `_StrAddressSubstratePort`, a thin shim that unwraps a `ControlAddress` back to its string and delegates. That keeps the in-memory adapter untouched while letting the registry's typed dispatch treat every route uniformly. @@ -80,7 +80,7 @@ from cora.operation.ports.control_port import ControlPort -class _StrAddressSubstrate: +class _StrAddressSubstratePort: """Adapt a `str`-surfaced `ControlPort` to the typed `SubstrateControlPort`. Wraps `InMemoryControlPort` (and any other `str`-address port) so the @@ -117,7 +117,7 @@ async def aclose(self) -> None: class ControlPortRegistry: """Prefix-routed composite implementing `ControlPort`. - Construct empty, call `register(prefix, port, substrate)` for each + Construct empty, call `register_substrate_port(prefix, port, substrate)` for each substrate route at app startup, then hand the registry to the executor as a single `ControlPort`. The executor never sees the routing table. @@ -127,7 +127,7 @@ def __init__(self) -> None: self._routes: list[tuple[str, SubstrateControlPort[Any], Substrate, bool]] = [] self._closed = False - def register( + def register_substrate_port( self, prefix: str, port: SubstrateControlPort[Any], @@ -150,7 +150,7 @@ def register( self._routes = [(p, a, s, sim) for (p, a, s, sim) in self._routes if p != prefix] self._routes.append((prefix, port, substrate, is_simulated)) - def register_str_port( + def register_control_port( self, prefix: str, port: ControlPort, @@ -160,14 +160,16 @@ def register_str_port( ) -> None: """Register a `str`-surfaced `ControlPort` (e.g. `InMemoryControlPort`). - Wraps `port` in the module-private `_StrAddressSubstrate` shim so the + Wraps `port` in the module-private `_StrAddressSubstratePort` shim so the typed route table can hold it uniformly with the real substrate adapters, keeping the shim an implementation detail callers never name. `substrate` defaults to `in_memory` because that is the only `str`-surfaced adapter today; pass another value only if a future `str`-address adapter routes under a different parsing family. """ - self.register(prefix, _StrAddressSubstrate(port), substrate, is_simulated=is_simulated) + self.register_substrate_port( + prefix, _StrAddressSubstratePort(port), substrate, is_simulated=is_simulated + ) def route(self, address: str) -> SubstrateControlPort[Any]: """Return the adapter for `address` via longest-prefix-match. @@ -203,7 +205,7 @@ def route_is_simulated(self, address: str) -> bool: async def read(self, address: str) -> Measurement: port, substrate = self._resolve(address) - return await port.read(parse_control_address(substrate, address)) + return await port.read(parse_control_address(address, substrate)) async def write( self, @@ -215,12 +217,12 @@ async def write( ) -> None: port, substrate = self._resolve(address) await port.write( - parse_control_address(substrate, address), value, wait=wait, timeout_s=timeout_s + parse_control_address(address, substrate), value, wait=wait, timeout_s=timeout_s ) def subscribe(self, address: str) -> AsyncIterator[Measurement]: port, substrate = self._resolve(address) - return port.subscribe(parse_control_address(substrate, address)) + return port.subscribe(parse_control_address(address, substrate)) async def aclose(self) -> None: """Close every registered adapter; idempotent. diff --git a/apps/api/src/cora/operation/ports/control_address.py b/apps/api/src/cora/operation/ports/control_address.py index 8412c8b3990..925a1823015 100644 --- a/apps/api/src/cora/operation/ports/control_address.py +++ b/apps/api/src/cora/operation/ports/control_address.py @@ -11,7 +11,7 @@ This module is that typed address space: the sum `EpicsPvAddress | TangoAttributeAddress | InMemoryAddress` plus the single -`parse_control_address(substrate, raw)` dispatch the registry calls. The +`parse_control_address(raw, substrate)` dispatch the registry calls. The substrate adapters (`SubstrateControlPort` implementations) consume the typed variants so they never re-parse a raw string per call: the Tango adapter reads `address.device` / `address.attribute` instead of splitting a TRL on every read. @@ -130,7 +130,7 @@ def parse(raw: str, substrate: str) -> InMemoryAddress: """ -def parse_control_address(substrate: Substrate, raw: str) -> ControlAddress: +def parse_control_address(raw: str, substrate: Substrate) -> ControlAddress: """Parse a raw address string into the `ControlAddress` for `substrate`. The single str->typed dispatch the `ControlPortRegistry` calls once it has diff --git a/apps/api/tests/integration/scenarios/test_2bm_align_resolution.py b/apps/api/tests/integration/scenarios/test_2bm_align_resolution.py index 8cdcfeb5cc4..01ed626a68d 100644 --- a/apps/api/tests/integration/scenarios/test_2bm_align_resolution.py +++ b/apps/api/tests/integration/scenarios/test_2bm_align_resolution.py @@ -248,7 +248,7 @@ async def test_align_resolution_recipe_conducts_compute_and_writes_calibration( # The pixel-size Measurement is seeded so fetch_measurements surfaces it. port = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, port, "epics_ca", is_simulated=True) + registry.register_substrate_port(softioc, port, "epics_ca", is_simulated=True) compute_port = InMemoryComputePort() compute_port.set_next_measurements((_pixel_size_measurement(_MEASURED_PIXEL_SIZE_UM),)) step_store = PostgresActivityStore(db_pool) diff --git a/apps/api/tests/integration/scenarios/test_2bm_align_rotation.py b/apps/api/tests/integration/scenarios/test_2bm_align_rotation.py index b0fbd31d61c..91d95a10760 100644 --- a/apps/api/tests/integration/scenarios/test_2bm_align_rotation.py +++ b/apps/api/tests/integration/scenarios/test_2bm_align_rotation.py @@ -266,7 +266,7 @@ async def test_align_rotation_recipe_conducts_compute_and_writes_calibration( # tuple under exercise). port = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, port, "epics_ca", is_simulated=True) + registry.register_substrate_port(softioc, port, "epics_ca", is_simulated=True) compute_port = InMemoryComputePort() compute_port.set_next_measurements( ( diff --git a/apps/api/tests/integration/scenarios/test_2bm_align_rotation_auto.py b/apps/api/tests/integration/scenarios/test_2bm_align_rotation_auto.py index f04aa87fc0b..ef3072510c9 100644 --- a/apps/api/tests/integration/scenarios/test_2bm_align_rotation_auto.py +++ b/apps/api/tests/integration/scenarios/test_2bm_align_rotation_auto.py @@ -265,7 +265,7 @@ async def _build_align_routine( port = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, port, "epics_ca", is_simulated=True) + registry.register_substrate_port(softioc, port, "epics_ca", is_simulated=True) compute_port = InMemoryComputePort() compute_port.set_measurement_sequence( tuple((_rotation_center_measurement(c),) for c in centers_sequence) diff --git a/apps/api/tests/integration/scenarios/test_2bm_flat_field.py b/apps/api/tests/integration/scenarios/test_2bm_flat_field.py index 89dfde11156..28a193ff923 100644 --- a/apps/api/tests/integration/scenarios/test_2bm_flat_field.py +++ b/apps/api/tests/integration/scenarios/test_2bm_flat_field.py @@ -325,7 +325,7 @@ async def test_flat_field_recipe_conducts_flats_against_softioc( # conduct observe Simulated. port = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, port, "epics_ca", is_simulated=True) + registry.register_substrate_port(softioc, port, "epics_ca", is_simulated=True) step_store = PostgresActivityStore(db_pool) conductor = Conductor( control_port=registry, @@ -425,7 +425,7 @@ async def test_flat_field_recipe_conducts_flats_against_softioc( # The axis is back at the captured aligned home (the restore landed). readback_port = ControlPortRegistry() - readback_port.register(softioc, EpicsCaControlPort(), "epics_ca") + readback_port.register_substrate_port(softioc, EpicsCaControlPort(), "epics_ca") try: axis_final = await readback_port.read(axis) assert axis_final.value == pytest.approx(_SAMPLE_HOME_MM) diff --git a/apps/api/tests/integration/scenarios/test_8id_xpcs_stream.py b/apps/api/tests/integration/scenarios/test_8id_xpcs_stream.py index e0dfbe37277..8c24e51196b 100644 --- a/apps/api/tests/integration/scenarios/test_8id_xpcs_stream.py +++ b/apps/api/tests/integration/scenarios/test_8id_xpcs_stream.py @@ -231,7 +231,7 @@ async def test_xpcs_stream_recipe_conducts_event_stream_and_leaves_no_observatio Measurement(value=_URI, kind="Categorical", quality="Good", produced_at=_NOW), ) registry = ControlPortRegistry() - registry.register_str_port(_DETECTOR, port, is_simulated=True) + registry.register_control_port(_DETECTOR, port, is_simulated=True) step_store = PostgresActivityStore(db_pool) conductor = Conductor( control_port=registry, diff --git a/apps/api/tests/integration/test_acquisitions_against_softioc_postgres.py b/apps/api/tests/integration/test_acquisitions_against_softioc_postgres.py index eb96108944f..1b35ab813a9 100644 --- a/apps/api/tests/integration/test_acquisitions_against_softioc_postgres.py +++ b/apps/api/tests/integration/test_acquisitions_against_softioc_postgres.py @@ -140,7 +140,7 @@ async def test_conductor_runs_collect_action_against_real_softioc_and_postgres( await _seed_defined_procedure(deps.event_store, procedure_id) step_store = PostgresActivityStore(db_pool) control_port = ControlPortRegistry() - control_port.register(softioc, EpicsCaControlPort(), "epics_ca") + control_port.register_substrate_port(softioc, EpicsCaControlPort(), "epics_ca") conductor = _build_conductor( deps.event_store, db_pool, @@ -233,7 +233,7 @@ async def test_conductor_runs_discrete_action_walks_axis_with_per_point_collects await _seed_defined_procedure(deps.event_store, procedure_id) step_store = PostgresActivityStore(db_pool) control_port = ControlPortRegistry() - control_port.register(softioc, EpicsCaControlPort(), "epics_ca") + control_port.register_substrate_port(softioc, EpicsCaControlPort(), "epics_ca") conductor = _build_conductor( deps.event_store, db_pool, @@ -321,7 +321,7 @@ async def test_conductor_runs_continuous_action_with_axis_sweep_against_softioc( await _seed_defined_procedure(deps.event_store, procedure_id) step_store = PostgresActivityStore(db_pool) control_port = ControlPortRegistry() - control_port.register(softioc, EpicsCaControlPort(), "epics_ca") + control_port.register_substrate_port(softioc, EpicsCaControlPort(), "epics_ca") conductor = _build_conductor( deps.event_store, db_pool, @@ -398,7 +398,7 @@ class control_port_reuse: # noqa: N801 def __init__(self, softioc_prefix: str) -> None: self._port = ControlPortRegistry() - self._port.register(softioc_prefix, EpicsCaControlPort(), "epics_ca") + self._port.register_substrate_port(softioc_prefix, EpicsCaControlPort(), "epics_ca") async def __aenter__(self) -> ControlPortRegistry: return self._port diff --git a/apps/api/tests/integration/test_actuation_kind_gate_against_softioc_postgres.py b/apps/api/tests/integration/test_actuation_kind_gate_against_softioc_postgres.py index 8c1c53c9aac..6268908efff 100644 --- a/apps/api/tests/integration/test_actuation_kind_gate_against_softioc_postgres.py +++ b/apps/api/tests/integration/test_actuation_kind_gate_against_softioc_postgres.py @@ -122,7 +122,7 @@ async def test_simulated_route_conduct_yields_non_promotable_dataset( inner = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, inner, "epics_ca", is_simulated=True) + registry.register_substrate_port(softioc, inner, "epics_ca", is_simulated=True) conductor = _conductor(deps, db_pool, registry) try: result = await conductor.conduct( @@ -184,7 +184,7 @@ async def test_physical_route_conduct_yields_promotable_dataset( inner = EpicsCaControlPort() registry = ControlPortRegistry() - registry.register(softioc, inner, "epics_ca", is_simulated=False) + registry.register_substrate_port(softioc, inner, "epics_ca", is_simulated=False) conductor = _conductor(deps, db_pool, registry) try: result = await conductor.conduct( @@ -295,8 +295,8 @@ async def test_hybrid_route_conduct_yields_non_promotable_dataset( registry = ControlPortRegistry() # Longest-prefix match: the double_value PV is declared simulated, the # long_value PV physical. Both speak CA to the same soft IOC. - registry.register(f"{softioc}double_value", inner, "epics_ca", is_simulated=True) - registry.register(f"{softioc}long_value", inner, "epics_ca", is_simulated=False) + registry.register_substrate_port(f"{softioc}double_value", inner, "epics_ca", is_simulated=True) + registry.register_substrate_port(f"{softioc}long_value", inner, "epics_ca", is_simulated=False) conductor = _conductor(deps, db_pool, registry) try: result = await conductor.conduct( diff --git a/apps/api/tests/integration/test_conductor_against_softioc_postgres.py b/apps/api/tests/integration/test_conductor_against_softioc_postgres.py index c0ab1b904dc..42db2d546c7 100644 --- a/apps/api/tests/integration/test_conductor_against_softioc_postgres.py +++ b/apps/api/tests/integration/test_conductor_against_softioc_postgres.py @@ -75,7 +75,7 @@ def _control_port(softioc: str, *, default_timeout_s: float | None = None) -> Co else EpicsCaControlPort(default_timeout_s=default_timeout_s) ) registry = ControlPortRegistry() - registry.register(softioc, adapter, "epics_ca") + registry.register_substrate_port(softioc, adapter, "epics_ca") return registry diff --git a/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py b/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py index 4f44a8da6bb..54b5aada37e 100644 --- a/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py +++ b/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py @@ -421,7 +421,7 @@ async def test_conduct_from_folds_pre_hold_actuation_kind_into_completion() -> N inner.simulate_connect("real:a") registry = ControlPortRegistry() # the replay tail is physical - registry.register_str_port("real:", inner, is_simulated=False) + registry.register_control_port("real:", inner, is_simulated=False) await _seed_held_with_steps( store, steps=(SetpointStep(address="real:a", value=1.0),), diff --git a/apps/api/tests/unit/operation/test_conductor.py b/apps/api/tests/unit/operation/test_conductor.py index 1168703af03..d6ce8e88d64 100644 --- a/apps/api/tests/unit/operation/test_conductor.py +++ b/apps/api/tests/unit/operation/test_conductor.py @@ -1562,7 +1562,7 @@ async def test_conduct_reraises_concurrency_error_from_complete_procedure() -> N async def test_execute_setpoint_via_registry_with_unrouted_address_records_failure() -> None: """NoAdapterForAddressError now lives in _CONTROL_ERRORS; records + halts cleanly.""" registry = ControlPortRegistry() - registry.register_str_port("known:", InMemoryControlPort()) + registry.register_control_port("known:", InMemoryControlPort()) appender = _FakeAppendStep() conductor = Conductor( control_port=registry, @@ -1595,7 +1595,7 @@ async def test_actuation_kind_is_physical_when_route_not_simulated() -> None: inner = InMemoryControlPort() inner.simulate_connect("2bma:rot:val") registry = ControlPortRegistry() - registry.register_str_port("2bma:", inner, is_simulated=False) + registry.register_control_port("2bma:", inner, is_simulated=False) appender = _FakeAppendStep() conductor = _conductor(registry, appender, ids=[uuid4()]) result = await conductor.execute( @@ -1619,7 +1619,7 @@ async def test_actuation_kind_is_simulated_when_route_is_simulated() -> None: inner = InMemoryControlPort() inner.simulate_connect("sim:rot:val") registry = ControlPortRegistry() - registry.register_str_port("sim:", inner, is_simulated=True) + registry.register_control_port("sim:", inner, is_simulated=True) appender = _FakeAppendStep() conductor = _conductor(registry, appender, ids=[uuid4()]) result = await conductor.execute( @@ -1640,8 +1640,8 @@ async def test_actuation_kind_is_hybrid_when_conduct_touches_both() -> None: real = InMemoryControlPort() real.simulate_connect("real:m1") registry = ControlPortRegistry() - registry.register_str_port("sim:", sim, is_simulated=True) - registry.register_str_port("real:", real, is_simulated=False) + registry.register_control_port("sim:", sim, is_simulated=True) + registry.register_control_port("real:", real, is_simulated=False) appender = _FakeAppendStep() conductor = _conductor(registry, appender, ids=[uuid4(), uuid4()]) result = await conductor.execute( @@ -1682,7 +1682,7 @@ async def test_actuation_kind_is_none_for_bare_port_without_routing_table() -> N async def test_actuation_kind_is_none_when_no_step_touches_control_port() -> None: """A conduct that drives nothing (empty steps) records no kind.""" registry = ControlPortRegistry() - registry.register_str_port("2bma:", InMemoryControlPort(), is_simulated=True) + registry.register_control_port("2bma:", InMemoryControlPort(), is_simulated=True) appender = _FakeAppendStep() conductor = _conductor(registry, appender) result = await conductor.execute( @@ -1701,7 +1701,7 @@ async def test_actuation_kind_is_simulated_when_action_body_drives_simulated_rou inner = InMemoryControlPort() inner.simulate_connect("sim:m1") control = ControlPortRegistry() - control.register_str_port("sim:", inner, is_simulated=True) + control.register_control_port("sim:", inner, is_simulated=True) async def drive(ctx: ActionContext) -> Mapping[str, Any]: await ctx.control_port.write("sim:m1", 1.0) @@ -1730,7 +1730,7 @@ async def test_actuation_kind_is_simulated_when_setpoint_write_fails_on_simulate conduct: the kind reflects routes attempted, not only succeeded.""" inner = InMemoryControlPort() # never connected -> write raises control = ControlPortRegistry() - control.register_str_port("sim:", inner, is_simulated=True) + control.register_control_port("sim:", inner, is_simulated=True) appender = _FakeAppendStep() conductor = _conductor(control, appender, ids=[uuid4()]) result = await conductor.execute( @@ -1754,7 +1754,7 @@ async def test_conduct_threads_simulated_kind_into_complete_command() -> None: inner = InMemoryControlPort() inner.simulate_connect("sim:rot:val") registry = ControlPortRegistry() - registry.register_str_port("sim:", inner, is_simulated=True) + registry.register_control_port("sim:", inner, is_simulated=True) appender = _FakeAppendStep() start = _FakeLifecycleHandler() complete = _FakeLifecycleHandler() @@ -1781,7 +1781,7 @@ async def test_conduct_threads_kind_into_abort_command_on_execute_failure() -> N data; routes attempted before the failing step taint it).""" inner = InMemoryControlPort() # never connected -> the write fails registry = ControlPortRegistry() - registry.register_str_port("sim:", inner, is_simulated=True) + registry.register_control_port("sim:", inner, is_simulated=True) appender = _FakeAppendStep() start = _FakeLifecycleHandler() complete = _FakeLifecycleHandler() diff --git a/apps/api/tests/unit/operation/test_control_address.py b/apps/api/tests/unit/operation/test_control_address.py index 4bac2cda981..7674fb55417 100644 --- a/apps/api/tests/unit/operation/test_control_address.py +++ b/apps/api/tests/unit/operation/test_control_address.py @@ -22,7 +22,7 @@ @pytest.mark.unit def test_parse_epics_ca_returns_epics_pv_address() -> None: - addr = parse_control_address("epics_ca", "2bma:rot") + addr = parse_control_address("2bma:rot", "epics_ca") assert addr == EpicsPvAddress("2bma:rot") assert str(addr) == "2bma:rot" @@ -30,20 +30,20 @@ def test_parse_epics_ca_returns_epics_pv_address() -> None: @pytest.mark.unit def test_parse_epics_pva_returns_epics_pv_address() -> None: """CA and PVA share the `EpicsPvAddress` variant (transport is a route decision).""" - addr = parse_control_address("epics_pva", "2bma:cam1:image") + addr = parse_control_address("2bma:cam1:image", "epics_pva") assert addr == EpicsPvAddress("2bma:cam1:image") @pytest.mark.unit def test_parse_tango_splits_trl_into_device_and_attribute() -> None: - addr = parse_control_address("tango", "id19/bsh/1/state") + addr = parse_control_address("id19/bsh/1/state", "tango") assert addr == TangoAttributeAddress(device="id19/bsh/1", attribute="state") assert str(addr) == "id19/bsh/1/state" @pytest.mark.unit def test_parse_in_memory_returns_in_memory_address() -> None: - addr = parse_control_address("in_memory", "sim:rot") + addr = parse_control_address("sim:rot", "in_memory") assert addr == InMemoryAddress("sim:rot") assert str(addr) == "sim:rot" @@ -51,7 +51,7 @@ def test_parse_in_memory_returns_in_memory_address() -> None: @pytest.mark.unit def test_epics_pv_address_rejects_empty_pv() -> None: with pytest.raises(MalformedControlAddressError) as exc_info: - parse_control_address("epics_ca", "") + parse_control_address("", "epics_ca") assert exc_info.value.substrate == "epics_ca" @@ -60,7 +60,7 @@ def test_tango_address_without_attribute_segment_is_malformed() -> None: """A TRL with no `/` has no attribute segment and cannot route (was the adapter's `_split_trl` guard; now enforced once at the registry boundary).""" with pytest.raises(MalformedControlAddressError) as exc_info: - parse_control_address("tango", "nodelimiter") + parse_control_address("nodelimiter", "tango") assert exc_info.value.raw == "nodelimiter" assert exc_info.value.substrate == "tango" @@ -68,13 +68,13 @@ def test_tango_address_without_attribute_segment_is_malformed() -> None: @pytest.mark.unit def test_tango_address_trailing_slash_is_malformed() -> None: with pytest.raises(MalformedControlAddressError): - parse_control_address("tango", "id19/bsh/1/") + parse_control_address("id19/bsh/1/", "tango") @pytest.mark.unit def test_in_memory_address_rejects_empty() -> None: with pytest.raises(MalformedControlAddressError): - parse_control_address("in_memory", "") + parse_control_address("", "in_memory") @pytest.mark.unit diff --git a/apps/api/tests/unit/operation/test_control_port_registry.py b/apps/api/tests/unit/operation/test_control_port_registry.py index c8a97c3949a..d3604b5f4fe 100644 --- a/apps/api/tests/unit/operation/test_control_port_registry.py +++ b/apps/api/tests/unit/operation/test_control_port_registry.py @@ -8,10 +8,10 @@ Two registration paths are exercised: - - `register_str_port` for `str`-surfaced adapters (`InMemoryControlPort`): + - `register_control_port` for `str`-surfaced adapters (`InMemoryControlPort`): the registry wraps them internally, so these tests assert behaviour (round-trip reads, aclose fan-out) rather than the private wrapper type. - - `register` for typed `SubstrateControlPort` adapters: a `_FakeSubstrate` + - `register_substrate_port` for typed `SubstrateControlPort` adapters: a `_FakeSubstratePort` stands in for the routing / simulated-flag / typed-parse assertions and records the `ControlAddress` a route actually receives. """ @@ -50,7 +50,7 @@ def _reading(value: float, kind: str = "Scalar") -> Measurement: ) -class _FakeSubstrate: +class _FakeSubstratePort: """A minimal `SubstrateControlPort` that records the typed address it receives. Stands in for a real substrate adapter in routing / simulated-flag / @@ -94,7 +94,7 @@ def test_registry_satisfies_control_port_protocol() -> None: @pytest.mark.unit def test_route_raises_no_adapter_when_no_prefix_matches() -> None: registry = ControlPortRegistry() - registry.register("7bma:", _FakeSubstrate(), "epics_ca") + registry.register_substrate_port("7bma:", _FakeSubstratePort(), "epics_ca") with pytest.raises(NoAdapterForAddressError) as exc_info: registry.route("2bma:rot:rbv") assert exc_info.value.address == "2bma:rot:rbv" @@ -103,8 +103,8 @@ def test_route_raises_no_adapter_when_no_prefix_matches() -> None: @pytest.mark.unit def test_route_returns_matching_adapter() -> None: registry = ControlPortRegistry() - aps = _FakeSubstrate() - registry.register("2bma:", aps, "epics_ca") + aps = _FakeSubstratePort() + registry.register_substrate_port("2bma:", aps, "epics_ca") assert registry.route("2bma:rot:rbv") is aps @@ -117,10 +117,10 @@ def test_route_picks_longest_matching_prefix_not_registration_order() -> None: Longest-match makes the routing decision deterministic. """ registry = ControlPortRegistry() - general = _FakeSubstrate() - specific = _FakeSubstrate() - registry.register("2bma:", general, "epics_ca") - registry.register("2bma:cam:", specific, "epics_ca") + general = _FakeSubstratePort() + specific = _FakeSubstratePort() + registry.register_substrate_port("2bma:", general, "epics_ca") + registry.register_substrate_port("2bma:cam:", specific, "epics_ca") assert registry.route("2bma:cam:image") is specific assert registry.route("2bma:rot:rbv") is general @@ -129,10 +129,10 @@ def test_route_picks_longest_matching_prefix_not_registration_order() -> None: def test_register_replaces_prior_route_for_same_prefix() -> None: """Re-registering a prefix replaces the prior adapter (hot-swap).""" registry = ControlPortRegistry() - old = _FakeSubstrate() - new = _FakeSubstrate() - registry.register("2bma:", old, "epics_ca") - registry.register("2bma:", new, "epics_ca") + old = _FakeSubstratePort() + new = _FakeSubstratePort() + registry.register_substrate_port("2bma:", old, "epics_ca") + registry.register_substrate_port("2bma:", new, "epics_ca") assert registry.route("2bma:rot:rbv") is new @@ -141,7 +141,7 @@ async def test_read_dispatches_to_routed_adapter() -> None: registry = ControlPortRegistry() port = InMemoryControlPort() port.set_reading("2bma:rot:rbv", _reading(1.5)) - registry.register_str_port("2bma:", port) + registry.register_control_port("2bma:", port) got = await registry.read("2bma:rot:rbv") assert got.value == 1.5 @@ -151,7 +151,7 @@ async def test_write_dispatches_to_routed_adapter() -> None: registry = ControlPortRegistry() port = InMemoryControlPort() port.simulate_connect("2bma:rot:val") - registry.register_str_port("2bma:", port) + registry.register_control_port("2bma:", port) await registry.write("2bma:rot:val", 3.14) assert (await port.read("2bma:rot:val")).value == 3.14 @@ -161,7 +161,7 @@ async def test_subscribe_dispatches_to_routed_adapter() -> None: registry = ControlPortRegistry() port = InMemoryControlPort() port.set_reading("2bma:rot:rbv", _reading(0.0)) - registry.register_str_port("2bma:", port) + registry.register_control_port("2bma:", port) iterator = registry.subscribe("2bma:rot:rbv") port.set_reading("2bma:rot:rbv", _reading(2.0)) got = await anext(iterator) @@ -173,8 +173,8 @@ async def test_subscribe_dispatches_to_routed_adapter() -> None: async def test_read_parses_epics_address_from_route_substrate() -> None: """An `epics_ca` route receives an `EpicsPvAddress` parsed from the string.""" registry = ControlPortRegistry() - recorder = _FakeSubstrate() - registry.register("2bma:", recorder, "epics_ca") + recorder = _FakeSubstratePort() + registry.register_substrate_port("2bma:", recorder, "epics_ca") await registry.read("2bma:rot:rbv") assert recorder.last_address == EpicsPvAddress("2bma:rot:rbv") @@ -183,8 +183,8 @@ async def test_read_parses_epics_address_from_route_substrate() -> None: async def test_read_parses_tango_address_from_route_substrate() -> None: """A `tango` route receives a `TangoAttributeAddress` split from the TRL.""" registry = ControlPortRegistry() - recorder = _FakeSubstrate() - registry.register("id19/", recorder, "tango") + recorder = _FakeSubstratePort() + registry.register_substrate_port("id19/", recorder, "tango") await registry.read("id19/bsh/1/state") assert recorder.last_address == TangoAttributeAddress(device="id19/bsh/1", attribute="state") @@ -192,8 +192,8 @@ async def test_read_parses_tango_address_from_route_substrate() -> None: @pytest.mark.unit async def test_write_parses_typed_address_from_route_substrate() -> None: registry = ControlPortRegistry() - recorder = _FakeSubstrate() - registry.register("2bma:", recorder, "epics_pva") + recorder = _FakeSubstratePort() + registry.register_substrate_port("2bma:", recorder, "epics_pva") await registry.write("2bma:cam:image", (1, 2, 3)) assert recorder.last_address == EpicsPvAddress("2bma:cam:image") @@ -202,8 +202,8 @@ async def test_write_parses_typed_address_from_route_substrate() -> None: async def test_aclose_closes_every_registered_adapter() -> None: registry = ControlPortRegistry() a, b = InMemoryControlPort(), InMemoryControlPort() - registry.register_str_port("2bma:", a) - registry.register_str_port("7bma:", b) + registry.register_control_port("2bma:", a) + registry.register_control_port("7bma:", b) await registry.aclose() assert a._closed is True # pyright: ignore[reportPrivateUsage] assert b._closed is True # pyright: ignore[reportPrivateUsage] @@ -212,7 +212,7 @@ async def test_aclose_closes_every_registered_adapter() -> None: @pytest.mark.unit async def test_aclose_is_idempotent() -> None: registry = ControlPortRegistry() - registry.register_str_port("2bma:", InMemoryControlPort()) + registry.register_control_port("2bma:", InMemoryControlPort()) await registry.aclose() await registry.aclose() # no-op @@ -243,8 +243,8 @@ async def aclose(self) -> None: registry = ControlPortRegistry() survivor = InMemoryControlPort() - registry.register("flaky:", _Boom(), "epics_ca") # type: ignore[arg-type] - registry.register_str_port("ok:", survivor) + registry.register_substrate_port("flaky:", _Boom(), "epics_ca") # type: ignore[arg-type] + registry.register_control_port("ok:", survivor) await registry.aclose() assert survivor._closed is True # pyright: ignore[reportPrivateUsage] @@ -253,14 +253,14 @@ async def aclose(self) -> None: def test_route_is_simulated_defaults_false() -> None: """A route registered without the flag is physical by default.""" registry = ControlPortRegistry() - registry.register_str_port("2bma:", InMemoryControlPort()) + registry.register_control_port("2bma:", InMemoryControlPort()) assert registry.route_is_simulated("2bma:rot:rbv") is False @pytest.mark.unit def test_route_is_simulated_returns_declared_flag() -> None: registry = ControlPortRegistry() - registry.register_str_port("sim:", InMemoryControlPort(), is_simulated=True) + registry.register_control_port("sim:", InMemoryControlPort(), is_simulated=True) assert registry.route_is_simulated("sim:rot:rbv") is True @@ -273,8 +273,8 @@ def test_route_is_simulated_uses_longest_prefix_match() -> None: simulated sub-band out of an otherwise live crate. """ registry = ControlPortRegistry() - registry.register_str_port("2bma:", InMemoryControlPort(), is_simulated=False) - registry.register_str_port("2bma:sim:", InMemoryControlPort(), is_simulated=True) + registry.register_control_port("2bma:", InMemoryControlPort(), is_simulated=False) + registry.register_control_port("2bma:sim:", InMemoryControlPort(), is_simulated=True) assert registry.route_is_simulated("2bma:sim:rot") is True assert registry.route_is_simulated("2bma:rot:rbv") is False @@ -283,7 +283,7 @@ def test_route_is_simulated_uses_longest_prefix_match() -> None: def test_route_is_simulated_raises_when_no_prefix_matches() -> None: """An unrouted address is an error, never silently treated as physical.""" registry = ControlPortRegistry() - registry.register_str_port("7bma:", InMemoryControlPort(), is_simulated=True) + registry.register_control_port("7bma:", InMemoryControlPort(), is_simulated=True) with pytest.raises(NoAdapterForAddressError) as exc_info: registry.route_is_simulated("2bma:rot:rbv") assert exc_info.value.address == "2bma:rot:rbv" @@ -293,8 +293,8 @@ def test_route_is_simulated_raises_when_no_prefix_matches() -> None: def test_register_replacement_preserves_new_simulated_flag() -> None: """Re-registering a prefix replaces both the adapter and its flag.""" registry = ControlPortRegistry() - registry.register_str_port("2bma:", InMemoryControlPort(), is_simulated=False) - registry.register_str_port("2bma:", InMemoryControlPort(), is_simulated=True) + registry.register_control_port("2bma:", InMemoryControlPort(), is_simulated=False) + registry.register_control_port("2bma:", InMemoryControlPort(), is_simulated=True) assert registry.route_is_simulated("2bma:rot:rbv") is True From 13405341218086790171c599c42327fd6cee8b74 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Fri, 17 Jul 2026 00:22:54 +0300 Subject: [PATCH 10/10] Catch a malformed control address instead of stranding the Procedure Gate review P1, reproduced by execution. A recipe step carrying a Tango address with no attribute segment ("id19/bsh/1/", a trailing-slash typo) prefix-matches its route, so the registry accepts it and then raises MalformedControlAddressError while parsing. That class was not in the Conductor's _CONTROL_ERRORS tuple, which is closed by design with no Exception catch-all, so it escaped the step loop and out through the route handler: start called once, complete never, abort never, the step record left at in_flight. The Procedure sits in Running with a dangling marker and no outcome, which is the exact failure the tuple exists to prevent. Its sibling NoAdapterForAddressError, raised at the same routing boundary for an address no prefix matches, IS a member. So the error's own docstring claiming it propagates "the same way" was describing an intent the code never carried out. The interesting part is why it slipped. The tuple's rule was prose, and it named exactly one module: "new exception classes in cora.operation.ports.control_port must be added here explicitly". Promoting the address to a typed sum put the new error family in the SIBLING module control_address.py, so it fell outside the stated rule by construction rather than by oversight. Re-homing it next to the other six is not available, since control_port already imports control_address and that would close an import cycle. So the rule stops being prose. test_control_errors_closed_over_port_exceptions enumerates every Exception subclass defined in either port module and asserts each is caught or explicitly allowlisted with a reason, plus the reverse direction so a retired class cannot leave a dead arm. Removing the fix fails it with the offending name. Prose asking to be remembered is what failed here; a test does not need remembering. Also corrects two comments of the same species, promising a 422 that cannot happen. build_control_port has two call sites, wire.py and the FastAPI lifespan in main.py, neither of them a request handler, so a missing tango extra fails startup rather than a request. The claim was copied forward from _optional_torch.py, where it is true. The bo group's own wording is left alone. Green: pre-commit end to end, 12614 unit, 1080 integration, 29563 architecture. Co-Authored-By: Claude Opus 4.8 --- apps/api/pyproject.toml | 5 +- .../operation/adapters/_optional_tango.py | 8 +- .../operation/adapters/tango_control_port.py | 2 +- apps/api/src/cora/operation/conductor.py | 27 +++-- ...trol_errors_closed_over_port_exceptions.py | 105 ++++++++++++++++++ 5 files changed, 132 insertions(+), 15 deletions(-) create mode 100644 apps/api/tests/architecture/test_control_errors_closed_over_port_exceptions.py diff --git a/apps/api/pyproject.toml b/apps/api/pyproject.toml index 671bc607056..53efbc5eee4 100644 --- a/apps/api/pyproject.toml +++ b/apps/api/pyproject.toml @@ -112,8 +112,9 @@ bo = [ # deployments go through aioca / p4p). The adapter imports `tango.asyncio` # lazily and probes importability at construction (`_optional_tango.py`), so # selecting the `tango` substrate without this group installed fails as a -# `ValueError` at wiring / handler time (mapped to 422), never as an uncaught -# `ImportError` mid-loop. LGPL-3.0 (PyTango binding) over the C++ Tango core. +# `ValueError` at wiring time, never as an uncaught `ImportError` mid-loop. +# The port is built once in the FastAPI lifespan, so a missing group fails +# startup rather than a request. LGPL-3.0 (PyTango binding) over the C++ Tango core. # Pin <11 to flag a future major SDK shift. tango = [ "pytango>=10,<11", diff --git a/apps/api/src/cora/operation/adapters/_optional_tango.py b/apps/api/src/cora/operation/adapters/_optional_tango.py index 4b75ace29fa..d0f82266d5f 100644 --- a/apps/api/src/cora/operation/adapters/_optional_tango.py +++ b/apps/api/src/cora/operation/adapters/_optional_tango.py @@ -8,9 +8,11 @@ via the helper here. The probe raises `ValueError` (not `ImportError`) on a missing dependency so -the gap surfaces where the ControlPort is materialised (`build_control_port` -at wiring / handler time) rather than as an uncaught `ImportError` deep in the -conduct loop, after a procedure has already started. Mirrors the `bo` group's +the gap surfaces where the ControlPort is materialised (`build_control_port`, +called once at wiring) rather than as an uncaught `ImportError` deep in the +conduct loop, after a procedure has already started. Selecting the substrate +without the group installed therefore fails startup, loudly and before any +run begins. Mirrors the `bo` group's `_optional_torch.py` posture. """ diff --git a/apps/api/src/cora/operation/adapters/tango_control_port.py b/apps/api/src/cora/operation/adapters/tango_control_port.py index 99df7e9eeb4..84a5fc02f9c 100644 --- a/apps/api/src/cora/operation/adapters/tango_control_port.py +++ b/apps/api/src/cora/operation/adapters/tango_control_port.py @@ -12,7 +12,7 @@ that only Tango-floor deployments need. It ships under the optional `tango` dependency group and is NOT imported at module load; the adapter probes importability in `__init__` via `_optional_tango.require_tango` (raising -`ValueError`, mapped to 422, rather than an uncaught `ImportError` mid-loop) +`ValueError` at wiring, rather than an uncaught `ImportError` mid-loop) and imports `tango.asyncio` lazily on first use. This keeps `import cora` cheap on EPICS-only installs. diff --git a/apps/api/src/cora/operation/conductor.py b/apps/api/src/cora/operation/conductor.py index 07082e9278e..2a5ca6ae42a 100644 --- a/apps/api/src/cora/operation/conductor.py +++ b/apps/api/src/cora/operation/conductor.py @@ -181,6 +181,7 @@ JobSpec, MeasurementNotFoundError, ) +from cora.operation.ports.control_address import MalformedControlAddressError from cora.operation.ports.control_port import ( ActuationKind, ControlAccessDeniedError, @@ -219,17 +220,25 @@ ControlValueCoercionError, ControlAccessDeniedError, NoAdapterForAddressError, + MalformedControlAddressError, ) -"""The closed set of `Control*Error` classes the Conductor maps to +"""The closed set of port error classes the Conductor maps to `ConductorFailure`. Tuple shape lets the `except` clause stay -declarative; new exception classes in `cora.operation.ports.control_port` -must be added here explicitly (no `Exception` catch-all so non-port -exceptions still propagate to the caller's task). - -`NoAdapterForAddressError` is included so a misconfigured -`ControlPortRegistry` (an address with no matching prefix) records -a structured step-failure rather than letting an opaque exception -propagate to the route layer and strand the Procedure in `Running`.""" +declarative; a new exception class in EITHER `control_port` or +`control_address` must be added here explicitly (no `Exception` +catch-all so non-port exceptions still propagate to the caller's task). +`test_control_errors_cover_port_exceptions` enforces that, because +naming only one of the two modules is exactly how +`MalformedControlAddressError` was missed. + +The two routing-boundary errors are included so a misconfigured +`ControlPortRegistry` records a structured step-failure rather than +letting an opaque exception propagate to the route layer and strand the +Procedure in `Running`: `NoAdapterForAddressError` for an address no +prefix matches, and `MalformedControlAddressError` for one that matches a +route but does not satisfy that substrate's address syntax (a Tango TRL +with no attribute segment, say). Both are recipe or configuration gaps an +operator must see as a failed step, not as a dead conduct task.""" _LIFECYCLE_RERAISE: tuple[type[Exception], ...] = ( diff --git a/apps/api/tests/architecture/test_control_errors_closed_over_port_exceptions.py b/apps/api/tests/architecture/test_control_errors_closed_over_port_exceptions.py new file mode 100644 index 00000000000..29dc4fe3d01 --- /dev/null +++ b/apps/api/tests/architecture/test_control_errors_closed_over_port_exceptions.py @@ -0,0 +1,105 @@ +"""Pin: the Conductor's `_CONTROL_ERRORS` covers every ControlPort error class. + +`Conductor._CONTROL_ERRORS` is a CLOSED tuple, deliberately: there is no +`Exception` catch-all, so an exception the tuple does not name propagates out +of the step loop, past `conduct()`, and out through the route handler, leaving +the Procedure in `Running` with a dangling in-flight step marker and no +outcome. That is the failure the tuple exists to prevent, and every +`except _CONTROL_ERRORS` site in the Conductor depends on it being complete. + +Completeness used to be a promise in prose. It did not hold. The tuple's +docstring asked that "new exception classes in `cora.operation.ports.control_port` +be added here explicitly", and naming exactly one module is what let the next +one through: when the control address was promoted to a typed sum, its +`MalformedControlAddressError` landed in the SIBLING module +`cora.operation.ports.control_address` and so fell outside the stated rule by +construction. It escaped the tuple while its own docstring claimed it +propagated "the same way" as `NoAdapterForAddressError`, which is a member. +Both are raised by `ControlPortRegistry` at the same routing boundary, one for +an address no prefix matches and one for an address that matches a route but +violates that substrate's syntax, so a Tango TRL typo in a recipe step killed +the conduct task instead of failing the step. + +Hence this test rather than a longer docstring: the rule now fails a build +instead of asking to be remembered. A new error class in either port module is +caught here, and the fix is to decide whether the Conductor should record it as +a structured step failure (add it to the tuple) or genuinely let it propagate +(add it to the allowlist below, with the reason). +""" + +# pyright: reportPrivateUsage=false + +import inspect + +import pytest + +from cora.operation.conductor import _CONTROL_ERRORS +from cora.operation.ports import control_address, control_port + +_PORT_ERROR_MODULES = (control_port, control_address) + +# Error classes the Conductor deliberately does NOT catch. Empty today: every +# error either port module raises is a substrate or configuration fault an +# operator must see as a failed step. An entry here must say why propagating +# is the correct behaviour, not merely that it is the current behaviour. +_DELIBERATELY_UNCAUGHT: dict[str, str] = {} + + +def _port_error_classes() -> dict[str, type[Exception]]: + """Every Exception subclass DEFINED in the ControlPort port modules. + + Keyed by name. Filters on `__module__` so re-exports (the modules import + from each other) are attributed to the module that defines them, not + counted twice. + """ + found: dict[str, type[Exception]] = {} + for module in _PORT_ERROR_MODULES: + for name, obj in vars(module).items(): + if ( + inspect.isclass(obj) + and issubclass(obj, Exception) + and obj.__module__ == module.__name__ + ): + found[name] = obj + return found + + +@pytest.mark.architecture +def test_control_errors_cover_port_exceptions() -> None: + caught = {error.__name__ for error in _CONTROL_ERRORS} + uncovered = sorted( + name + for name in _port_error_classes() + if name not in caught and name not in _DELIBERATELY_UNCAUGHT + ) + assert uncovered == [], ( + f"{uncovered} can be raised through ControlPort but are not in the " + "Conductor's _CONTROL_ERRORS tuple, so they would escape the step loop " + "and strand the Procedure in Running. Add each to _CONTROL_ERRORS in " + "cora/operation/conductor.py, or to _DELIBERATELY_UNCAUGHT here with " + "the reason propagating is correct." + ) + + +@pytest.mark.architecture +def test_control_errors_names_only_real_port_exceptions() -> None: + """The reverse direction: the tuple must not name a retired class.""" + defined = _port_error_classes() + stale = sorted(error.__name__ for error in _CONTROL_ERRORS if error.__name__ not in defined) + assert stale == [], ( + f"_CONTROL_ERRORS names {stale}, which no ControlPort port module defines. " + "A renamed or retired error class leaves a dead arm in the tuple." + ) + + +@pytest.mark.architecture +def test_routing_boundary_errors_are_caught() -> None: + """The two `ControlPortRegistry` raises, pinned by name. + + Both are reachable from an operator typo in a recipe step address, which is + the cheapest way to strand a Procedure, so they are named rather than left + to the sweep above. + """ + caught = {error.__name__ for error in _CONTROL_ERRORS} + assert "NoAdapterForAddressError" in caught + assert "MalformedControlAddressError" in caught