From 2a03b7d78c6222a49f46bfacdbaeb3536b640a61 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 17 Sep 2025 17:55:03 +0100 Subject: [PATCH 01/74] Proof of concept device manager --- src/dodal/beamlines/dm_demo.py | 206 +++++++++++++++++++++++++++++++++ 1 file changed, 206 insertions(+) create mode 100644 src/dodal/beamlines/dm_demo.py diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py new file mode 100644 index 00000000000..2ad50a2f2c9 --- /dev/null +++ b/src/dodal/beamlines/dm_demo.py @@ -0,0 +1,206 @@ +import functools +import inspect +from collections.abc import Callable +from functools import cache, wraps +from types import NoneType +from typing import Annotated, Any, Generic, ParamSpec, TypeVar +import typing + +from ophyd_async.core import PathProvider +from ophyd_async.epics.adsimdetector import SimDetector + +from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline +from dodal.common.beamlines.device_helpers import DET_SUFFIX, HDF5_SUFFIX +from dodal.devices import motors +from dodal.devices.motors import XThetaStage +from dodal.log import set_beamline as set_log_beamline +from dodal.utils import BeamlinePrefix, SkipType +from ophyd_async.epics.motor import Motor + +BL = "adsim" +DEFAULT_TIMEOUT = 30 +PREFIX = BeamlinePrefix("t01") +set_log_beamline(BL) +set_utils_beamline(BL) + +T = TypeVar("T") +Args = ParamSpec("Args") + + +class DeviceFactory(Generic[Args, T]): + factory: Callable[Args, T] + use_factory_name: bool + timeout: float + mock: bool + _skip: SkipType + + def __init__(self, factory, use_factory_name, timeout, mock, skip): + self.factory = factory + self.use_factory_name = use_factory_name + self.timeout = timeout + self.mock = mock + self._skip = skip + + @property + def dependencies(self) -> dict[str, type]: + sig = inspect.signature(self.factory) + return { + para.name: para.annotation + for para in sig.parameters.values() + if para.default is inspect.Parameter.empty + } + + @property + def skip(self) -> bool: + return self._skip() if callable(self._skip) else self._skip + + def reset(self): + self.factory.cache_clear() # type: ignore + + def __call__(self, **kwargs) -> T: + return self.factory(**kwargs) + + def __repr__(self) -> str: + target = self.factory.__annotations__.get("return") + target = target.__name__ if target else "???" + return f"<{self.factory.__name__}: DeviceFactory -> {target}>" + + +class DeviceManager: + def __init__(self): + self._factories = {} + + # Overload for using as plain decorator, ie: @devices.factory + @typing.overload + def factory(self, func: Callable[Args, T], /) -> Callable[Args, T]: ... + + # Overload for using as configurable decorator, eg: @devices.factory(skip=True) + @typing.overload + def factory( + self, + func: NoneType = None, + /, + use_factory_name: bool = True, + timeout: float = DEFAULT_TIMEOUT, + mock: bool = False, + skip: SkipType = False, + ) -> Callable[[Callable[Args, T]], Callable[Args, T]]: ... + + def factory( + self, + func: Callable[Args, T] | None = None, + /, + use_factory_name: Annotated[bool, "Use factory name as name of device"] = True, + timeout: Annotated[ + float, "Timeout for connecting to the device" + ] = DEFAULT_TIMEOUT, + mock: Annotated[bool, "Use Signals with mock backends for device"] = False, + skip: Annotated[ + SkipType, + "mark the factory to be (conditionally) skipped when beamline is imported by external program", + ] = False, + ) -> Callable[Args, T] | Callable[[Callable[Args, T]], Callable[Args, T]]: + def decorator(func: Callable[Args, T]) -> Callable[Args, T]: + factory = cache(func) + self._factories[func.__name__] = DeviceFactory( + factory, use_factory_name, timeout, mock, skip + ) + return factory # type: ignore + + if func is None: + return decorator + return decorator(func) + + def build_all(self, **kwargs): + order = self._build_order(kwargs) + built = {} + errors = {} + for device in order: + deps = self[device].dependencies + if deps.keys() & errors.keys(): + errors[device] = ValueError("Errors building dependencies") + else: + params = {dep: built.get(dep) or kwargs[dep] for dep in deps} + try: + built[device] = self[device](**params) + except Exception as e: + errors[device] = e + return (built, errors) + + def __getitem__(self, name): + return self._factories[name] + + def _build_order(self, fixtures=None) -> list[str]: + order = [] + available = set(fixtures or {}) + pending = list(self._factories.items()) + while pending: + buffer = [] + for device in pending: + if device[1].skip: + continue + if all(dep in available for dep in device[1].dependencies): + order.append(device[0]) + available.add(device[0]) + else: + buffer.append(device) + if len(pending) == len(buffer): + raise ValueError(f"Cannot fulfil requirements for {pending}") + buffer, pending = [], buffer + + return order + + +devices = DeviceManager() + + +@devices.factory(skip=True, timeout=13, mock=True, use_factory_name=False) +def stage() -> XThetaStage: + """Build the stage""" + return XThetaStage( + f"{PREFIX.beamline_prefix}-MO-SIMC-01:", x_infix="M1", theta_infix="M2" + ) + + +@devices.factory(skip=True) +def det(path_provider: PathProvider) -> SimDetector: + return SimDetector( + f"{PREFIX.beamline_prefix}-DI-CAM-01:", + path_provider=path_provider, + drv_suffix=DET_SUFFIX, + fileio_suffix=HDF5_SUFFIX, + ) + + +@devices.factory +def base(base_x, base_y: motors.Motor) -> Motor: + print(base_x) + print(base_y) + return base_x + base_y + + +@devices.factory +def base_x() -> motors.Motor: + # raise ValueError("Not a base_x") + return "base_x motor" + + +@devices.factory +def base_y(path_provider) -> motors.Motor: + print(f"Using {path_provider=}") + return "base_y motor" + + +@devices.factory +def unknown(): + return "unknown device" + + +if __name__ == "__main__": + for name, factory in devices._factories.items(): + print(name, factory, factory.dependencies) + # print(devices._build_order({"path_provider": ["42"]})) + + print(devices["stage"]) + print(devices["unknown"]) + print(devices.build_all(path_provider="numtracker")) From ee86c743aa0d1fe4788a7c3bf7840245a81dd421 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 18 Sep 2025 14:12:01 +0100 Subject: [PATCH 02/74] Use fixtures parameter instead of **kwargs --- src/dodal/beamlines/dm_demo.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 2ad50a2f2c9..57218461ebc 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -111,8 +111,10 @@ def decorator(func: Callable[Args, T]) -> Callable[Args, T]: return decorator return decorator(func) - def build_all(self, **kwargs): - order = self._build_order(kwargs) + def build_all( + self, fixtures: dict[str, Any] + ) -> tuple[dict[str, Any], dict[str, Exception]]: + order = self._build_order(fixtures) built = {} errors = {} for device in order: @@ -120,7 +122,7 @@ def build_all(self, **kwargs): if deps.keys() & errors.keys(): errors[device] = ValueError("Errors building dependencies") else: - params = {dep: built.get(dep) or kwargs[dep] for dep in deps} + params = {dep: built.get(dep) or fixtures[dep] for dep in deps} try: built[device] = self[device](**params) except Exception as e: From 658a45fc1e25dde2afdb615761b7d30d04c99a8f Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 23 Sep 2025 15:24:26 +0100 Subject: [PATCH 03/74] Expand dependencies --- src/dodal/beamlines/dm_demo.py | 111 +++++++++++++++++++++++++-------- 1 file changed, 85 insertions(+), 26 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 57218461ebc..defb96df0a5 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -1,13 +1,13 @@ -import functools import inspect +import typing from collections.abc import Callable from functools import cache, wraps -from types import NoneType +from types import FunctionType, NoneType from typing import Annotated, Any, Generic, ParamSpec, TypeVar -import typing from ophyd_async.core import PathProvider from ophyd_async.epics.adsimdetector import SimDetector +from ophyd_async.epics.motor import Motor from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.common.beamlines.device_helpers import DET_SUFFIX, HDF5_SUFFIX @@ -15,7 +15,6 @@ from dodal.devices.motors import XThetaStage from dodal.log import set_beamline as set_log_beamline from dodal.utils import BeamlinePrefix, SkipType -from ophyd_async.epics.motor import Motor BL = "adsim" DEFAULT_TIMEOUT = 30 @@ -40,6 +39,11 @@ def __init__(self, factory, use_factory_name, timeout, mock, skip): self.timeout = timeout self.mock = mock self._skip = skip + wraps(factory)(self) + + @property + def name(self) -> str: + return self.factory.__name__ @property def dependencies(self) -> dict[str, type]: @@ -54,16 +58,13 @@ def dependencies(self) -> dict[str, type]: def skip(self) -> bool: return self._skip() if callable(self._skip) else self._skip - def reset(self): - self.factory.cache_clear() # type: ignore - - def __call__(self, **kwargs) -> T: - return self.factory(**kwargs) + def __call__(self, *args, **kwargs) -> T: + return self.factory(*args, **kwargs) # type: ignore def __repr__(self) -> str: target = self.factory.__annotations__.get("return") target = target.__name__ if target else "???" - return f"<{self.factory.__name__}: DeviceFactory -> {target}>" + return f"<{self.name}: DeviceFactory -> {target}>" class DeviceManager: @@ -112,15 +113,29 @@ def decorator(func: Callable[Args, T]) -> Callable[Args, T]: return decorator(func) def build_all( - self, fixtures: dict[str, Any] + self, + *functions: FunctionType, + fixtures: dict[str, Any] | None = None, + connect_immediately: bool = True, ) -> tuple[dict[str, Any], dict[str, Exception]]: - order = self._build_order(fixtures) + """ + Build the devices from the given factories, ensuring that any + dependencies are built first and passed to later factories as required. + """ + factories = [self[f.__name__] for f in functions] + build_list, required_fixtures = self._expand_dependencies(factories) + fixtures = fixtures or {} + if missing := required_fixtures - fixtures.keys(): + raise ValueError(f"Missing requirements: {missing}") + order = self._build_order( + {dep: self._factories[dep] for dep in build_list}, fixtures=fixtures + ) built = {} errors = {} for device in order: deps = self[device].dependencies - if deps.keys() & errors.keys(): - errors[device] = ValueError("Errors building dependencies") + if dep_errs := deps.keys() & errors.keys(): + errors[device] = ValueError(f"Errors building dependencies: {dep_errs}") else: params = {dep: built.get(dep) or fixtures[dep] for dep in deps} try: @@ -132,20 +147,53 @@ def build_all( def __getitem__(self, name): return self._factories[name] - def _build_order(self, fixtures=None) -> list[str]: + def _expand_dependencies( + self, + factories: list[DeviceFactory[Any, Any]], + ) -> tuple[set[str], set[str]]: # dependencies, fixtures + """ + Determine full list of transitive dependencies for the given list and + any external fixtures that should be provided (aren't available as + device factories) + """ + dependencies = set() + fixtures = set() + while factories: + fact = factories.pop() + dependencies.add(fact.name) + for dep in fact.dependencies: + if dep not in dependencies: + if dep in self._factories: + dependencies.add(dep) + factories.append(self[dep]) + else: + fixtures.add(dep) + + return dependencies, fixtures + + def _build_order( + self, factories: dict[str, DeviceFactory], fixtures=None + ) -> list[str]: + """ + Determine the order devices in which devices should be build to ensure + that dependencies are built before the things that depend on them + + Assumes that all required devices and fixtures are included in the + given factory list. + """ order = [] available = set(fixtures or {}) - pending = list(self._factories.items()) + pending = factories while pending: - buffer = [] - for device in pending: - if device[1].skip: + buffer = {} + for name, factory in pending.items(): + if factory.skip: continue - if all(dep in available for dep in device[1].dependencies): - order.append(device[0]) - available.add(device[0]) + if all(dep in available for dep in factory.dependencies): + order.append(name) + available.add(name) else: - buffer.append(device) + buffer[name] = factory if len(pending) == len(buffer): raise ValueError(f"Cannot fulfil requirements for {pending}") buffer, pending = [], buffer @@ -195,6 +243,7 @@ def base_y(path_provider) -> motors.Motor: @devices.factory def unknown(): + # raise ValueError("Unknown error") return "unknown device" @@ -203,6 +252,16 @@ def unknown(): print(name, factory, factory.dependencies) # print(devices._build_order({"path_provider": ["42"]})) - print(devices["stage"]) - print(devices["unknown"]) - print(devices.build_all(path_provider="numtracker")) + # print(devices["stage"]) + # print(devices["unknown"]) + # print(devices.build_all(fixtures={"path_provider": "numtracker"})) + # print(devices._required_fixtures((devices["base"], devices["base_y"]))) + print(devices._expand_dependencies([devices["base"]])) + print(devices._expand_dependencies(list(devices._factories.values()))) + # print(devices._build_order({"base": devices["base"]})) + + print( + devices.build_all( + base, det, stage, unknown, fixtures={"path_provider": "numtracker"} + ) + ) From 318593bfa89e5fde2775c4226db50a5b753d8c9b Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 25 Sep 2025 13:52:22 +0100 Subject: [PATCH 04/74] Pre-optional handling --- src/dodal/beamlines/dm_demo.py | 142 ++++++++++++++++++++++----------- 1 file changed, 97 insertions(+), 45 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index defb96df0a5..aec71b6e876 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -1,8 +1,8 @@ import inspect import typing -from collections.abc import Callable +from collections.abc import Callable, Iterable from functools import cache, wraps -from types import FunctionType, NoneType +from types import NoneType from typing import Annotated, Any, Generic, ParamSpec, TypeVar from ophyd_async.core import PathProvider @@ -32,13 +32,15 @@ class DeviceFactory(Generic[Args, T]): timeout: float mock: bool _skip: SkipType + _manager: "DeviceManager" - def __init__(self, factory, use_factory_name, timeout, mock, skip): - self.factory = factory + def __init__(self, factory, use_factory_name, timeout, mock, skip, manager): + self.factory = cache(factory) # type: ignore self.use_factory_name = use_factory_name self.timeout = timeout self.mock = mock self._skip = skip + self._manager = manager wraps(factory)(self) @property @@ -46,25 +48,41 @@ def name(self) -> str: return self.factory.__name__ @property - def dependencies(self) -> dict[str, type]: + def dependencies(self) -> dict[str, type | None]: sig = inspect.signature(self.factory) return { para.name: para.annotation + if para.annotation is not inspect.Parameter.empty + else None for para in sig.parameters.values() - if para.default is inspect.Parameter.empty + # if para.default is inspect.Parameter.empty } @property def skip(self) -> bool: return self._skip() if callable(self._skip) else self._skip + def build( + self, + mock: bool = False, + connect_immediately: bool = False, + name: str | None = None, + timeout: float | None = None, + **fixtures, + ) -> T: + devices, errors = self._manager.build_devices(self, fixtures=fixtures) + if errors: + raise errors[self.name] + else: + return devices[self.name] + def __call__(self, *args, **kwargs) -> T: return self.factory(*args, **kwargs) # type: ignore def __repr__(self) -> str: target = self.factory.__annotations__.get("return") target = target.__name__ if target else "???" - return f"<{self.name}: DeviceFactory -> {target}>" + return f"<{self.name}: DeviceFactory ({', '.join(self.dependencies.keys())}) -> {target}>" class DeviceManager: @@ -73,7 +91,7 @@ def __init__(self): # Overload for using as plain decorator, ie: @devices.factory @typing.overload - def factory(self, func: Callable[Args, T], /) -> Callable[Args, T]: ... + def factory(self, func: Callable[Args, T], /) -> DeviceFactory[Args, T]: ... # Overload for using as configurable decorator, eg: @devices.factory(skip=True) @typing.overload @@ -85,7 +103,7 @@ def factory( timeout: float = DEFAULT_TIMEOUT, mock: bool = False, skip: SkipType = False, - ) -> Callable[[Callable[Args, T]], Callable[Args, T]]: ... + ) -> Callable[[Callable[Args, T]], DeviceFactory[Args, T]]: ... def factory( self, @@ -100,33 +118,34 @@ def factory( SkipType, "mark the factory to be (conditionally) skipped when beamline is imported by external program", ] = False, - ) -> Callable[Args, T] | Callable[[Callable[Args, T]], Callable[Args, T]]: - def decorator(func: Callable[Args, T]) -> Callable[Args, T]: - factory = cache(func) - self._factories[func.__name__] = DeviceFactory( - factory, use_factory_name, timeout, mock, skip - ) - return factory # type: ignore + ) -> DeviceFactory[Args, T] | Callable[[Callable[Args, T]], DeviceFactory[Args, T]]: + def decorator(func: Callable[Args, T]) -> DeviceFactory[Args, T]: + factory = DeviceFactory(func, use_factory_name, timeout, mock, skip, self) + self._factories[func.__name__] = factory + return factory if func is None: return decorator return decorator(func) - def build_all( + def build_all(self, fixtures: dict[str, Any] | None = None): + return self.build_devices( + *(f for f in self._factories.values() if not f.skip), fixtures=fixtures + ) + + def build_devices( self, - *functions: FunctionType, + *factories: DeviceFactory, fixtures: dict[str, Any] | None = None, - connect_immediately: bool = True, ) -> tuple[dict[str, Any], dict[str, Exception]]: """ Build the devices from the given factories, ensuring that any dependencies are built first and passed to later factories as required. """ - factories = [self[f.__name__] for f in functions] - build_list, required_fixtures = self._expand_dependencies(factories) + # TODO: Should we check all the factories are our factories? + print("building: ", factories) fixtures = fixtures or {} - if missing := required_fixtures - fixtures.keys(): - raise ValueError(f"Missing requirements: {missing}") + build_list = self._expand_dependencies(factories, fixtures) order = self._build_order( {dep: self._factories[dep] for dep in build_list}, fixtures=fixtures ) @@ -149,27 +168,35 @@ def __getitem__(self, name): def _expand_dependencies( self, - factories: list[DeviceFactory[Any, Any]], - ) -> tuple[set[str], set[str]]: # dependencies, fixtures + factories: Iterable[DeviceFactory[Any, Any]], + available_fixtures: dict[str, Any], + ) -> set[str]: """ - Determine full list of transitive dependencies for the given list and - any external fixtures that should be provided (aren't available as - device factories) + Determine full list of devices that are required to build the given devices. + If a dependency is available via the fixtures, a matching device factory + will not be included unless explicitly requested allowing for devices to + be overridden. + + Errors: + If a required dependencies is not available as either a device + factory or a fixture, a ValueError is raised """ dependencies = set() - fixtures = set() + factories = list(factories) while factories: fact = factories.pop() dependencies.add(fact.name) for dep in fact.dependencies: - if dep not in dependencies: + if dep not in dependencies and dep not in available_fixtures: if dep in self._factories: dependencies.add(dep) factories.append(self[dep]) else: - fixtures.add(dep) + raise ValueError( + f"Missing fixture or factory for {dep}", + ) - return dependencies, fixtures + return dependencies def _build_order( self, factories: dict[str, DeviceFactory], fixtures=None @@ -187,8 +214,8 @@ def _build_order( while pending: buffer = {} for name, factory in pending.items(): - if factory.skip: - continue + # if factory.skip: + # continue if all(dep in available for dep in factory.dependencies): order.append(name) available.add(name) @@ -212,7 +239,7 @@ def stage() -> XThetaStage: ) -@devices.factory(skip=True) +@devices.factory(skip=stage.skip) def det(path_provider: PathProvider) -> SimDetector: return SimDetector( f"{PREFIX.beamline_prefix}-DI-CAM-01:", @@ -224,8 +251,8 @@ def det(path_provider: PathProvider) -> SimDetector: @devices.factory def base(base_x, base_y: motors.Motor) -> Motor: - print(base_x) - print(base_y) + # print(base_x) + # print(base_y) return base_x + base_y @@ -235,33 +262,58 @@ def base_x() -> motors.Motor: return "base_x motor" -@devices.factory +@devices.factory(skip=True) def base_y(path_provider) -> motors.Motor: - print(f"Using {path_provider=}") + # print(f"Using {path_provider=}") return "base_y motor" +@devices.factory +def optional(base_z=42): + return base_z + + @devices.factory def unknown(): # raise ValueError("Unknown error") return "unknown device" +others = DeviceManager() + if __name__ == "__main__": - for name, factory in devices._factories.items(): - print(name, factory, factory.dependencies) + # for name, factory in devices._factories.items(): + # print(name, factory, factory.dependencies) # print(devices._build_order({"path_provider": ["42"]})) # print(devices["stage"]) # print(devices["unknown"]) # print(devices.build_all(fixtures={"path_provider": "numtracker"})) # print(devices._required_fixtures((devices["base"], devices["base_y"]))) - print(devices._expand_dependencies([devices["base"]])) - print(devices._expand_dependencies(list(devices._factories.values()))) + # print(devices._expand_dependencies([devices["base"]])) + # print(devices._expand_dependencies(list(devices._factories.values()))) # print(devices._build_order({"base": devices["base"]})) print( - devices.build_all( + "build_all", + devices.build_all({"path_provider": "all_nt", "base_y": "other base_y"}), + ) + print( + "build_some", + devices.build_devices( base, det, stage, unknown, fixtures={"path_provider": "numtracker"} - ) + ), ) + + print("base", base) + print("base_y", base_y) + print("b1", b1 := base.build(path_provider="num_track")) + print("b2", b2 := base(base_x="base_x motor", base_y="base_y motor")) + print("b1 is b2", b1 is b2) + + print("unknown()", unknown()) + print("unknown.build()", unknown.build()) + + print("optional build", optional.build()) + print("optional with override", optional.build(base_z=14)) + print("optional without override", optional()) From d81502b649423c5ac85d224a3244cc1e8e4bdc98 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 25 Sep 2025 16:29:25 +0100 Subject: [PATCH 05/74] 'Working' optional dependencies --- src/dodal/beamlines/dm_demo.py | 133 ++++++++++++++++++++++----------- 1 file changed, 91 insertions(+), 42 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index aec71b6e876..f84d57b51d8 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -1,7 +1,7 @@ import inspect import typing from collections.abc import Callable, Iterable -from functools import cache, wraps +from functools import wraps from types import NoneType from typing import Annotated, Any, Generic, ParamSpec, TypeVar @@ -35,7 +35,7 @@ class DeviceFactory(Generic[Args, T]): _manager: "DeviceManager" def __init__(self, factory, use_factory_name, timeout, mock, skip, manager): - self.factory = cache(factory) # type: ignore + self.factory = factory self.use_factory_name = use_factory_name self.timeout = timeout self.mock = mock @@ -48,16 +48,23 @@ def name(self) -> str: return self.factory.__name__ @property - def dependencies(self) -> dict[str, type | None]: + def dependencies(self) -> set[str]: sig = inspect.signature(self.factory) return { - para.name: para.annotation - if para.annotation is not inspect.Parameter.empty - else None + para.name for para in sig.parameters.values() # if para.default is inspect.Parameter.empty } + @property + def optional_dependencies(self) -> set[str]: + sig = inspect.signature(self.factory) + return { + para.name + for para in sig.parameters.values() + if para.default is not inspect.Parameter.empty + } + @property def skip(self) -> bool: return self._skip() if callable(self._skip) else self._skip @@ -82,7 +89,8 @@ def __call__(self, *args, **kwargs) -> T: def __repr__(self) -> str: target = self.factory.__annotations__.get("return") target = target.__name__ if target else "???" - return f"<{self.name}: DeviceFactory ({', '.join(self.dependencies.keys())}) -> {target}>" + params = inspect.signature(self.factory) + return f"<{self.name}: DeviceFactory ({params}) -> {target}>" class DeviceManager: @@ -129,8 +137,14 @@ def decorator(func: Callable[Args, T]) -> DeviceFactory[Args, T]: return decorator(func) def build_all(self, fixtures: dict[str, Any] | None = None): + # exclude all skipped devices and those that have been overridden by fixtures return self.build_devices( - *(f for f in self._factories.values() if not f.skip), fixtures=fixtures + *( + f + for f in self._factories.values() + if not f.skip and f.name not in fixtures + ), + fixtures=fixtures, ) def build_devices( @@ -143,7 +157,7 @@ def build_devices( dependencies are built first and passed to later factories as required. """ # TODO: Should we check all the factories are our factories? - print("building: ", factories) + # print("building: ", factories) fixtures = fixtures or {} build_list = self._expand_dependencies(factories, fixtures) order = self._build_order( @@ -151,12 +165,19 @@ def build_devices( ) built = {} errors = {} + # print(order) for device in order: deps = self[device].dependencies - if dep_errs := deps.keys() & errors.keys(): + if dep_errs := deps & errors.keys(): errors[device] = ValueError(f"Errors building dependencies: {dep_errs}") else: - params = {dep: built.get(dep) or fixtures[dep] for dep in deps} + # If we've made it this far, any devices that aren't available must have default + # values so ignore anything that's missing + params = { + dep: built.get(dep) or fixtures[dep] + for dep in deps + if dep in built.keys() | fixtures.keys() + } try: built[device] = self[device](**params) except Exception as e: @@ -186,12 +207,13 @@ def _expand_dependencies( while factories: fact = factories.pop() dependencies.add(fact.name) + options = fact.optional_dependencies for dep in fact.dependencies: if dep not in dependencies and dep not in available_fixtures: if dep in self._factories: dependencies.add(dep) factories.append(self[dep]) - else: + elif dep not in options: raise ValueError( f"Missing fixture or factory for {dep}", ) @@ -214,15 +236,23 @@ def _build_order( while pending: buffer = {} for name, factory in pending.items(): - # if factory.skip: - # continue - if all(dep in available for dep in factory.dependencies): + buildable_deps = factory.dependencies & factories.keys() + # print("buildable", name, buildable_deps, available) + # We should only have been called with a resolvable set of things to build + # but just to double check + assert buildable_deps.issubset( + factory.dependencies - factory.optional_dependencies + ) + if all(dep in available for dep in buildable_deps): order.append(name) available.add(name) else: buffer[name] = factory if len(pending) == len(buffer): - raise ValueError(f"Cannot fulfil requirements for {pending}") + # This should only be reachable if we have circular dependencies + raise ValueError( + f"Cannot determine build order - possibly circular dependencies ({', '.join(pending.keys())})" + ) buffer, pending = [], buffer return order @@ -249,11 +279,11 @@ def det(path_provider: PathProvider) -> SimDetector: ) -@devices.factory +@devices.factory(mock=True) def base(base_x, base_y: motors.Motor) -> Motor: # print(base_x) # print(base_y) - return base_x + base_y + return f"{base_x} - {base_y}" @devices.factory @@ -269,8 +299,8 @@ def base_y(path_provider) -> motors.Motor: @devices.factory -def optional(base_z=42): - return base_z +def optional(base_x, base_z=42): + return (base_x, base_z) @devices.factory @@ -279,6 +309,14 @@ def unknown(): return "unknown device" +# @devices.factory +# def circ_1(circ_2): ... + + +# @devices.factory +# def circ_2(circ_1): ... + + others = DeviceManager() if __name__ == "__main__": @@ -294,26 +332,37 @@ def unknown(): # print(devices._expand_dependencies(list(devices._factories.values()))) # print(devices._build_order({"base": devices["base"]})) - print( - "build_all", - devices.build_all({"path_provider": "all_nt", "base_y": "other base_y"}), - ) - print( - "build_some", - devices.build_devices( - base, det, stage, unknown, fixtures={"path_provider": "numtracker"} - ), + # print( + # "build_all", + # devices.build_all({"path_provider": "all_nt", "base_y": "other base_y"}), + # ) + # print( + # "build_some", + # devices.build_devices( + # base, det, stage, unknown, fixtures={"path_provider": "numtracker"} + # ), + # ) + + # print("base", optional) + # print("base_y", base_y) + # print("b1", b1 := base.build(path_provider="num_track")) + # print("b2", b2 := base(base_x="base_x motor", base_y="base_y motor")) + # print("b1 is b2", b1 is b2) + + # print("unknown()", unknown()) + # print("unknown.build()", unknown.build()) + + # print("circular", circ_1.build()) + + # print("optional deps", optional.dependencies) + # print("optional optional deps", optional.optional_dependencies) + # print("optional build", optional.build()) + # print("optional with override", optional.build(base_z=14)) + # print("optional without override", optional(17)) + # print("optional override required", optional.build(base_x=19)) + + valid, errs = devices.build_all( + fixtures={"base_x": 19, "path_provider": "numtrack"} ) - - print("base", base) - print("base_y", base_y) - print("b1", b1 := base.build(path_provider="num_track")) - print("b2", b2 := base(base_x="base_x motor", base_y="base_y motor")) - print("b1 is b2", b1 is b2) - - print("unknown()", unknown()) - print("unknown.build()", unknown.build()) - - print("optional build", optional.build()) - print("optional with override", optional.build(base_z=14)) - print("optional without override", optional()) + print(valid) + print(errs) From 123e96f234f842d3d8ea8b38c4036e5f77da89b8 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 25 Sep 2025 17:11:23 +0100 Subject: [PATCH 06/74] Docs, comments and warnings --- src/dodal/beamlines/dm_demo.py | 53 +++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index f84d57b51d8..f335a9f23a6 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -1,5 +1,6 @@ import inspect import typing +import warnings from collections.abc import Callable, Iterable from functools import wraps from types import NoneType @@ -27,6 +28,13 @@ class DeviceFactory(Generic[Args, T]): + """ + Wrapper around a device factory (any function returning a device) that holds + a reference to a device manager that can provide dependencies, along with + default connection information for how the created device should be + connected. + """ + factory: Callable[Args, T] use_factory_name: bool timeout: float @@ -45,19 +53,18 @@ def __init__(self, factory, use_factory_name, timeout, mock, skip, manager): @property def name(self) -> str: + """Name of the underlying factory function""" return self.factory.__name__ @property def dependencies(self) -> set[str]: + """Names of all parameters""" sig = inspect.signature(self.factory) - return { - para.name - for para in sig.parameters.values() - # if para.default is inspect.Parameter.empty - } + return {para.name for para in sig.parameters.values()} @property def optional_dependencies(self) -> set[str]: + """Names of optional dependencies""" sig = inspect.signature(self.factory) return { para.name @@ -67,6 +74,10 @@ def optional_dependencies(self) -> set[str]: @property def skip(self) -> bool: + """ + Whether this device should be skipped as part of build_all - it will + still be built if a required device depends on it + """ return self._skip() if callable(self._skip) else self._skip def build( @@ -77,6 +88,7 @@ def build( timeout: float | None = None, **fixtures, ) -> T: + """Build this device, building any dependencies first""" devices, errors = self._manager.build_devices(self, fixtures=fixtures) if errors: raise errors[self.name] @@ -84,7 +96,7 @@ def build( return devices[self.name] def __call__(self, *args, **kwargs) -> T: - return self.factory(*args, **kwargs) # type: ignore + return self.factory(*args, **kwargs) def __repr__(self) -> str: target = self.factory.__annotations__.get("return") @@ -157,15 +169,20 @@ def build_devices( dependencies are built first and passed to later factories as required. """ # TODO: Should we check all the factories are our factories? - # print("building: ", factories) fixtures = fixtures or {} + if common := fixtures.keys() & {f.name for f in factories}: + warnings.warn( + f"Factories ({common}) will be overridden by fixtures", stacklevel=1 + ) + factories = tuple(f for f in factories if f.name not in common) + if unknown := {f for f in factories if f not in self._factories.values()}: + raise ValueError(f"Factories ({unknown}) are unknown to this manager") build_list = self._expand_dependencies(factories, fixtures) order = self._build_order( {dep: self._factories[dep] for dep in build_list}, fixtures=fixtures ) built = {} errors = {} - # print(order) for device in order: deps = self[device].dependencies if dep_errs := deps & errors.keys(): @@ -237,7 +254,6 @@ def _build_order( buffer = {} for name, factory in pending.items(): buildable_deps = factory.dependencies & factories.keys() - # print("buildable", name, buildable_deps, available) # We should only have been called with a resolvable set of things to build # but just to double check assert buildable_deps.issubset( @@ -309,15 +325,16 @@ def unknown(): return "unknown device" -# @devices.factory -# def circ_1(circ_2): ... +others = DeviceManager() -# @devices.factory -# def circ_2(circ_1): ... +@others.factory +def circ_1(circ_2): ... -others = DeviceManager() +@others.factory +def circ_2(circ_1): ... + if __name__ == "__main__": # for name, factory in devices._factories.items(): @@ -366,3 +383,11 @@ def unknown(): ) print(valid) print(errs) + + valid, errs = devices.build_devices( + base_x, base, optional, fixtures={"base_x": "19", "path_provider": "nt_pp"} + ) + print(valid) + print(errs) + + # devices.build_devices(circ_1, circ_2) From 15d4dc4a53ae4ec3b976d290120daf6f216ed285 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 26 Sep 2025 10:52:26 +0100 Subject: [PATCH 07/74] Check for positional only arguments --- src/dodal/beamlines/dm_demo.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index f335a9f23a6..270308fed57 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -43,6 +43,11 @@ class DeviceFactory(Generic[Args, T]): _manager: "DeviceManager" def __init__(self, factory, use_factory_name, timeout, mock, skip, manager): + if any( + p.kind == inspect.Parameter.POSITIONAL_ONLY + for p in inspect.signature(factory).parameters.values() + ): + raise ValueError(f"{factory.__name__} has positional only arguments") self.factory = factory self.use_factory_name = use_factory_name self.timeout = timeout From f3832d6d59d9b25b9a32826bcb0248662e01710c Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 26 Sep 2025 17:26:26 +0100 Subject: [PATCH 08/74] Handle connections and support new loader in cli --- src/dodal/beamlines/dm_demo.py | 84 +++++++++++++++++++++++++++++----- src/dodal/cli.py | 40 ++++++++++++---- 2 files changed, 103 insertions(+), 21 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 270308fed57..4f0a37cc18e 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -2,10 +2,12 @@ import typing import warnings from collections.abc import Callable, Iterable -from functools import wraps +from functools import cached_property, wraps from types import NoneType -from typing import Annotated, Any, Generic, ParamSpec, TypeVar +from typing import Annotated, Any, Generic, Mapping, ParamSpec, TypeVar +from bluesky.run_engine import call_in_bluesky_event_loop +from ophyd import Device from ophyd_async.core import PathProvider from ophyd_async.epics.adsimdetector import SimDetector from ophyd_async.epics.motor import Motor @@ -15,7 +17,13 @@ from dodal.devices import motors from dodal.devices.motors import XThetaStage from dodal.log import set_beamline as set_log_beamline -from dodal.utils import BeamlinePrefix, SkipType +from dodal.utils import ( + AnyDevice, + BeamlinePrefix, + OphydV1Device, + OphydV2Device, + SkipType, +) BL = "adsim" DEFAULT_TIMEOUT = 30 @@ -62,12 +70,16 @@ def name(self) -> str: return self.factory.__name__ @property + def device_type(self) -> type[T]: + return inspect.signature(self.factory).return_annotation + + @cached_property def dependencies(self) -> set[str]: """Names of all parameters""" sig = inspect.signature(self.factory) return {para.name for para in sig.parameters.values()} - @property + @cached_property def optional_dependencies(self) -> set[str]: """Names of optional dependencies""" sig = inspect.signature(self.factory) @@ -94,11 +106,20 @@ def build( **fixtures, ) -> T: """Build this device, building any dependencies first""" - devices, errors = self._manager.build_devices(self, fixtures=fixtures) + devices, errors = self._manager.build_devices( + self, + fixtures=fixtures, + mock=mock, + connect_immediately=connect_immediately, + timeout=timeout or self.timeout, + ) if errors: raise errors[self.name] else: - return devices[self.name] + device = devices[self.name] + if name: + device.set_name(device) + return device def __call__(self, *args, **kwargs) -> T: return self.factory(*args, **kwargs) @@ -153,21 +174,36 @@ def decorator(func: Callable[Args, T]) -> DeviceFactory[Args, T]: return decorator return decorator(func) - def build_all(self, fixtures: dict[str, Any] | None = None): + def build_all( + self, + include_skipped=False, + fixtures: dict[str, Any] | None = None, + mock: bool = False, + connect_immediately: bool = False, + timeout: float | None = None, + ): + fixtures = fixtures or {} # exclude all skipped devices and those that have been overridden by fixtures return self.build_devices( *( f for f in self._factories.values() - if not f.skip and f.name not in fixtures + # allow overriding skip but still allow fixtures to override devices + if (include_skipped or not f.skip) and f.name not in fixtures ), fixtures=fixtures, + mock=mock, + connect_immediately=connect_immediately, + timeout=timeout, ) def build_devices( self, *factories: DeviceFactory, fixtures: dict[str, Any] | None = None, + mock: bool = False, + connect_immediately: bool = False, + timeout: float | None = None, ) -> tuple[dict[str, Any], dict[str, Exception]]: """ Build the devices from the given factories, ensuring that any @@ -189,7 +225,8 @@ def build_devices( built = {} errors = {} for device in order: - deps = self[device].dependencies + factory = self[device] + deps = factory.dependencies if dep_errs := deps & errors.keys(): errors[device] = ValueError(f"Errors building dependencies: {dep_errs}") else: @@ -201,10 +238,32 @@ def build_devices( if dep in built.keys() | fixtures.keys() } try: - built[device] = self[device](**params) + mock = mock or factory.mock + if issubclass(factory.device_type, OphydV1Device): + print("building v1") + built_device = factory(mock=mock, **params) + else: + print("building v2") + built_device = factory(**params) + if factory.use_factory_name: + built_device.set_name(device) + if connect_immediately: + if issubclass(factory.device_type, OphydV1Device): + print("connecting v1") + built_device.wait_for_connection() + else: + print("connecting v2") + call_in_bluesky_event_loop( + built_device.connect( + mock=mock, + timeout=timeout or factory.timeout, + ) + ) + built[device] = built_device except Exception as e: errors[device] = e - return (built, errors) + + return built, errors def __getitem__(self, name): return self._factories[name] @@ -278,6 +337,9 @@ def _build_order( return order + def __repr__(self) -> str: + return f"" + devices = DeviceManager() diff --git a/src/dodal/cli.py b/src/dodal/cli.py index 12d21f54db7..35b6862efb2 100644 --- a/src/dodal/cli.py +++ b/src/dodal/cli.py @@ -1,4 +1,6 @@ +import importlib import os +import warnings from collections.abc import Mapping from pathlib import Path @@ -8,6 +10,7 @@ from ophyd_async.plan_stubs import ensure_connected from dodal.beamlines import all_beamline_names, module_name_for_beamline +from dodal.beamlines.dm_demo import DeviceManager from dodal.common.beamlines.beamline_utils import set_path_provider from dodal.utils import AnyDevice, filter_ophyd_devices, make_all_devices @@ -51,7 +54,6 @@ def connect(beamline: str, all: bool, sim_backend: bool) -> None: # We need to make a fake path provider for any detectors that need one, # it is not used in dodal connect - _spoof_path_provider() module_name = module_name_for_beamline(beamline) full_module_path = f"dodal.beamlines.{module_name}" @@ -62,15 +64,33 @@ def connect(beamline: str, all: bool, sim_backend: bool) -> None: print(f"Attempting connection to {beamline} (using {full_module_path})") - # Force all devices to be lazy (don't connect to PVs on instantiation) and do - # connection as an extra step, because the alternatives is handling the fact - # that only some devices may be lazy. - devices, instance_exceptions = make_all_devices( - full_module_path, - include_skipped=all, - fake_with_ophyd_sim=sim_backend, - wait_for_connection=False, - ) + mod = importlib.import_module(full_module_path) + + # Don't connect devices as they're built and do connection as an extra step, + # because the alternatives is handling the fact that only some devices may + # be lazy. + if (manager := getattr(mod, "devices", None)) and isinstance( + manager, DeviceManager + ): + path_provider = StaticPathProvider(UUIDFilenameProvider(), Path("/tmp")) + devices, instance_exceptions = manager.build_all( + mock=sim_backend, # only used by v1 devices + fixtures={"path_provider": path_provider}, + ) + else: + # Fall back to the previous approach + warnings.warn( + "Using deprecated @device_factory approach - consider using DeviceManager", + DeprecationWarning, + stacklevel=1, + ) + _spoof_path_provider() + devices, instance_exceptions = make_all_devices( + full_module_path, + include_skipped=all, + fake_with_ophyd_sim=sim_backend, + wait_for_connection=False, + ) devices, connect_exceptions = _connect_devices(run_engine, devices, sim_backend) # Inform user of successful connections From 5fe05c7346768283684b64c2cb4635b926d89831 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 26 Sep 2025 17:27:18 +0100 Subject: [PATCH 09/74] Convert adsim to new loading --- src/dodal/beamlines/adsim.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/dodal/beamlines/adsim.py b/src/dodal/beamlines/adsim.py index 5ec56286583..17421cc60ef 100644 --- a/src/dodal/beamlines/adsim.py +++ b/src/dodal/beamlines/adsim.py @@ -10,6 +10,8 @@ from dodal.log import set_beamline as set_log_beamline from dodal.utils import BeamlinePrefix +from dodal.beamlines.dm_demo import DeviceManager + BL = "adsim" PREFIX = BeamlinePrefix("t01") set_log_beamline(BL) @@ -63,19 +65,21 @@ """ +devices = DeviceManager() + -@device_factory() +@devices.factory(timeout=2) def stage() -> XThetaStage: return XThetaStage( f"{PREFIX.beamline_prefix}-MO-SIMC-01:", x_infix="M1", theta_infix="M2" ) -@device_factory() -def det() -> SimDetector: +@devices.factory(timeout=2) +def det(path_provider) -> SimDetector: return SimDetector( f"{PREFIX.beamline_prefix}-DI-CAM-01:", - path_provider=get_path_provider(), + path_provider=path_provider, drv_suffix=DET_SUFFIX, fileio_suffix=HDF5_SUFFIX, ) From cbfa2f6608036fe4e8ea0310495a56ec00575787 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Mon, 29 Sep 2025 18:17:39 +0100 Subject: [PATCH 10/74] Add auto_connect to device manager --- src/dodal/beamlines/dm_demo.py | 70 +++++++++++++++++++++++----------- src/dodal/cli.py | 10 ++--- 2 files changed, 52 insertions(+), 28 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 4f0a37cc18e..f937e0ed38e 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -4,7 +4,7 @@ from collections.abc import Callable, Iterable from functools import cached_property, wraps from types import NoneType -from typing import Annotated, Any, Generic, Mapping, ParamSpec, TypeVar +from typing import Annotated, Any, Generic, Mapping, NamedTuple, ParamSpec, TypeVar from bluesky.run_engine import call_in_bluesky_event_loop from ophyd import Device @@ -122,7 +122,13 @@ def build( return device def __call__(self, *args, **kwargs) -> T: - return self.factory(*args, **kwargs) + device = self.factory(*args, **kwargs) + if isinstance(device, OphydV2Device): + if self.use_factory_name: + device.set_name(self.name) + if (conn := self._manager._connect): + call_in_bluesky_event_loop(device.connect(conn.mock or self.mock), timeout=conn.timeout) + return device def __repr__(self) -> str: target = self.factory.__annotations__.get("return") @@ -131,9 +137,17 @@ def __repr__(self) -> str: return f"<{self.name}: DeviceFactory ({params}) -> {target}>" +class ConnectionOptions(NamedTuple): + mock: bool + timeout: float + class DeviceManager: + _factories: dict[str, DeviceFactory] + _connect: ConnectionOptions | None + def __init__(self): self._factories = {} + self._connect = None # Overload for using as plain decorator, ie: @devices.factory @typing.overload @@ -174,6 +188,12 @@ def decorator(func: Callable[Args, T]) -> DeviceFactory[Args, T]: return decorator return decorator(func) + def auto_connect(self, connect=True, mock=False, timeout: float =10): + if connect: + self._connect = ConnectionOptions(mock, timeout) + else: + self._connect = None + def build_all( self, include_skipped=False, @@ -224,6 +244,8 @@ def build_devices( ) built = {} errors = {} + previous_connect = self._connect + self.auto_connect(connect_immediately, mock, timeout or 10) for device in order: factory = self[device] deps = factory.dependencies @@ -238,31 +260,30 @@ def build_devices( if dep in built.keys() | fixtures.keys() } try: - mock = mock or factory.mock - if issubclass(factory.device_type, OphydV1Device): - print("building v1") - built_device = factory(mock=mock, **params) - else: - print("building v2") - built_device = factory(**params) - if factory.use_factory_name: - built_device.set_name(device) - if connect_immediately: - if issubclass(factory.device_type, OphydV1Device): - print("connecting v1") - built_device.wait_for_connection() - else: - print("connecting v2") - call_in_bluesky_event_loop( - built_device.connect( - mock=mock, - timeout=timeout or factory.timeout, - ) - ) + # mock = mock or factory.mock + # if issubclass(factory.device_type, OphydV1Device): + # print("building v1") + # built_device = factory(mock=mock, **params) + # else: + # print("building v2") + built_device = factory(**params) + # if connect_immediately: + # if issubclass(factory.device_type, OphydV1Device): + # print("connecting v1") + # built_device.wait_for_connection() + # else: + # print("connecting v2") + # call_in_bluesky_event_loop( + # built_device.connect( + # mock=mock, + # timeout=timeout or factory.timeout, + # ) + # ) built[device] = built_device except Exception as e: errors[device] = e + self._connect = previous_connect return built, errors def __getitem__(self, name): @@ -457,4 +478,7 @@ def circ_2(circ_1): ... print(valid) print(errs) + devices.auto_connect(mock=True, timeout=17) + print(base_x()) + # devices.build_devices(circ_1, circ_2) diff --git a/src/dodal/cli.py b/src/dodal/cli.py index 35b6862efb2..992c69d498e 100644 --- a/src/dodal/cli.py +++ b/src/dodal/cli.py @@ -79,11 +79,11 @@ def connect(beamline: str, all: bool, sim_backend: bool) -> None: ) else: # Fall back to the previous approach - warnings.warn( - "Using deprecated @device_factory approach - consider using DeviceManager", - DeprecationWarning, - stacklevel=1, - ) + # warnings.warn( + # "Using deprecated @device_factory approach - consider using DeviceManager", + # DeprecationWarning, + # stacklevel=1, + # ) _spoof_path_provider() devices, instance_exceptions = make_all_devices( full_module_path, From 784856b75919bd7e5fe5ebc3bab91dda59c474cb Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 30 Sep 2025 13:55:04 +0100 Subject: [PATCH 11/74] Remove references to connecting from device manager --- src/dodal/beamlines/dm_demo.py | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index f937e0ed38e..3ae70521e04 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -126,8 +126,6 @@ def __call__(self, *args, **kwargs) -> T: if isinstance(device, OphydV2Device): if self.use_factory_name: device.set_name(self.name) - if (conn := self._manager._connect): - call_in_bluesky_event_loop(device.connect(conn.mock or self.mock), timeout=conn.timeout) return device def __repr__(self) -> str: @@ -141,13 +139,12 @@ class ConnectionOptions(NamedTuple): mock: bool timeout: float + class DeviceManager: _factories: dict[str, DeviceFactory] - _connect: ConnectionOptions | None def __init__(self): self._factories = {} - self._connect = None # Overload for using as plain decorator, ie: @devices.factory @typing.overload @@ -188,12 +185,6 @@ def decorator(func: Callable[Args, T]) -> DeviceFactory[Args, T]: return decorator return decorator(func) - def auto_connect(self, connect=True, mock=False, timeout: float =10): - if connect: - self._connect = ConnectionOptions(mock, timeout) - else: - self._connect = None - def build_all( self, include_skipped=False, @@ -244,8 +235,6 @@ def build_devices( ) built = {} errors = {} - previous_connect = self._connect - self.auto_connect(connect_immediately, mock, timeout or 10) for device in order: factory = self[device] deps = factory.dependencies @@ -260,30 +249,11 @@ def build_devices( if dep in built.keys() | fixtures.keys() } try: - # mock = mock or factory.mock - # if issubclass(factory.device_type, OphydV1Device): - # print("building v1") - # built_device = factory(mock=mock, **params) - # else: - # print("building v2") built_device = factory(**params) - # if connect_immediately: - # if issubclass(factory.device_type, OphydV1Device): - # print("connecting v1") - # built_device.wait_for_connection() - # else: - # print("connecting v2") - # call_in_bluesky_event_loop( - # built_device.connect( - # mock=mock, - # timeout=timeout or factory.timeout, - # ) - # ) built[device] = built_device except Exception as e: errors[device] = e - self._connect = previous_connect return built, errors def __getitem__(self, name): @@ -478,7 +448,6 @@ def circ_2(circ_1): ... print(valid) print(errs) - devices.auto_connect(mock=True, timeout=17) print(base_x()) # devices.build_devices(circ_1, circ_2) From 5d8bfa58fd0ac1ea7844818e703d5ada4ccd28f1 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 30 Sep 2025 14:52:51 +0100 Subject: [PATCH 12/74] DeviceResult wrappers --- src/dodal/beamlines/dm_demo.py | 79 ++++++++++++++++++++++++++-------- src/dodal/cli.py | 7 ++- 2 files changed, 66 insertions(+), 20 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 3ae70521e04..0382dc873de 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -1,3 +1,6 @@ +import asyncio +from concurrent import futures +import concurrent import inspect import typing import warnings @@ -6,7 +9,11 @@ from types import NoneType from typing import Annotated, Any, Generic, Mapping, NamedTuple, ParamSpec, TypeVar -from bluesky.run_engine import call_in_bluesky_event_loop +from bluesky.run_engine import ( + RunEngine, + call_in_bluesky_event_loop, + get_bluesky_event_loop, +) from ophyd import Device from ophyd_async.core import PathProvider from ophyd_async.epics.adsimdetector import SimDetector @@ -135,9 +142,40 @@ def __repr__(self) -> str: return f"<{self.name}: DeviceFactory ({params}) -> {target}>" -class ConnectionOptions(NamedTuple): - mock: bool - timeout: float +class ConnectionResult(NamedTuple): + devices: dict[str, AnyDevice] + build_errors: dict[str, Exception] + connection_errors: dict[str, Exception] + + +class DeviceBuildResult(NamedTuple): + devices: dict[str, Any] + errors: dict[str, Exception] + + def connect( + self, mock: bool = False, run_engine: RunEngine | None = None + ) -> ConnectionResult: + connections = {} + loop: asyncio.EventLoop = ( # type: ignore + run_engine.loop if run_engine else get_bluesky_event_loop() + ) + for name, device in self.devices.items(): + fut: futures.Future = asyncio.run_coroutine_threadsafe( + device.connect(mock=mock), # type: ignore + loop=loop, + ) + connections[name] = fut + + connected = {} + connection_errors = {} + for name, connection_future in connections.items(): + try: + connection_future.result(timeout=12) + connected[name] = self.devices[name] + except Exception as e: + connection_errors[name] = e + + return ConnectionResult(connected, self.errors, connection_errors) class DeviceManager: @@ -192,7 +230,7 @@ def build_all( mock: bool = False, connect_immediately: bool = False, timeout: float | None = None, - ): + ) -> DeviceBuildResult: fixtures = fixtures or {} # exclude all skipped devices and those that have been overridden by fixtures return self.build_devices( @@ -215,7 +253,7 @@ def build_devices( mock: bool = False, connect_immediately: bool = False, timeout: float | None = None, - ) -> tuple[dict[str, Any], dict[str, Exception]]: + ) -> DeviceBuildResult: """ Build the devices from the given factories, ensuring that any dependencies are built first and passed to later factories as required. @@ -254,7 +292,7 @@ def build_devices( except Exception as e: errors[device] = e - return built, errors + return DeviceBuildResult(built, errors) def __getitem__(self, name): return self._factories[name] @@ -436,18 +474,23 @@ def circ_2(circ_1): ... # print("optional without override", optional(17)) # print("optional override required", optional.build(base_x=19)) - valid, errs = devices.build_all( - fixtures={"base_x": 19, "path_provider": "numtrack"} - ) - print(valid) - print(errs) + # valid, errs = devices.build_all( + # fixtures={"base_x": 19, "path_provider": "numtrack"} + # ) + # print(valid) + # print(errs) - valid, errs = devices.build_devices( - base_x, base, optional, fixtures={"base_x": "19", "path_provider": "nt_pp"} - ) - print(valid) - print(errs) + # valid, errs = devices.build_devices( + # base_x, base, optional, fixtures={"base_x": "19", "path_provider": "nt_pp"} + # ) + # print(valid) + # print(errs) + + # print(base_x()) - print(base_x()) + res = devices.build_all(fixtures={"path_provider": "nt_path_provider"}) + print(res) + conn = res.connect() + print(conn) # devices.build_devices(circ_1, circ_2) diff --git a/src/dodal/cli.py b/src/dodal/cli.py index 992c69d498e..fd839539d11 100644 --- a/src/dodal/cli.py +++ b/src/dodal/cli.py @@ -73,10 +73,13 @@ def connect(beamline: str, all: bool, sim_backend: bool) -> None: manager, DeviceManager ): path_provider = StaticPathProvider(UUIDFilenameProvider(), Path("/tmp")) - devices, instance_exceptions = manager.build_all( + device_result = manager.build_all( mock=sim_backend, # only used by v1 devices fixtures={"path_provider": path_provider}, ) + devices, instance_exceptions, connect_exceptions = device_result.connect( + mock=sim_backend + ) else: # Fall back to the previous approach # warnings.warn( @@ -91,7 +94,7 @@ def connect(beamline: str, all: bool, sim_backend: bool) -> None: fake_with_ophyd_sim=sim_backend, wait_for_connection=False, ) - devices, connect_exceptions = _connect_devices(run_engine, devices, sim_backend) + devices, connect_exceptions = _connect_devices(run_engine, devices, sim_backend) # Inform user of successful connections _report_successful_devices(devices, sim_backend) From 05ed69bc67b3838c7e61c5a7a1a22942c6f5fa54 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 30 Sep 2025 16:02:09 +0100 Subject: [PATCH 13/74] Demo adsim --- src/dodal/beamlines/adsim.py | 15 ++++---- src/dodal/beamlines/dm_demo.py | 70 ++++++++++++++++++++++++---------- 2 files changed, 58 insertions(+), 27 deletions(-) diff --git a/src/dodal/beamlines/adsim.py b/src/dodal/beamlines/adsim.py index 17421cc60ef..98603b7a27e 100644 --- a/src/dodal/beamlines/adsim.py +++ b/src/dodal/beamlines/adsim.py @@ -1,17 +1,13 @@ +from ophyd.epics_motor import EpicsMotor from ophyd_async.epics.adsimdetector import SimDetector -from dodal.common.beamlines.beamline_utils import ( - device_factory, - get_path_provider, -) +from dodal.beamlines.dm_demo import DeviceManager from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.common.beamlines.device_helpers import DET_SUFFIX, HDF5_SUFFIX from dodal.devices.motors import XThetaStage from dodal.log import set_beamline as set_log_beamline from dodal.utils import BeamlinePrefix -from dodal.beamlines.dm_demo import DeviceManager - BL = "adsim" PREFIX = BeamlinePrefix("t01") set_log_beamline(BL) @@ -68,7 +64,7 @@ devices = DeviceManager() -@devices.factory(timeout=2) +@devices.factory(timeout=2, mock=True) def stage() -> XThetaStage: return XThetaStage( f"{PREFIX.beamline_prefix}-MO-SIMC-01:", x_infix="M1", theta_infix="M2" @@ -83,3 +79,8 @@ def det(path_provider) -> SimDetector: drv_suffix=DET_SUFFIX, fileio_suffix=HDF5_SUFFIX, ) + + +# @devices.factory +# def old_motor() -> EpicsMotor: +# return EpicsMotor(name="old_motor", prefix=f"{PREFIX.beamline_prefix}-MO-SIMC-01:") diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 0382dc873de..644aae63359 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -7,7 +7,16 @@ from collections.abc import Callable, Iterable from functools import cached_property, wraps from types import NoneType -from typing import Annotated, Any, Generic, Mapping, NamedTuple, ParamSpec, TypeVar +from typing import ( + Annotated, + Any, + Generic, + Literal, + Mapping, + NamedTuple, + ParamSpec, + TypeVar, +) from bluesky.run_engine import ( RunEngine, @@ -113,19 +122,26 @@ def build( **fixtures, ) -> T: """Build this device, building any dependencies first""" - devices, errors = self._manager.build_devices( + devices = self._manager.build_devices( self, fixtures=fixtures, mock=mock, - connect_immediately=connect_immediately, - timeout=timeout or self.timeout, + # connect_immediately=connect_immediately, + # timeout=timeout or self.timeout, ) - if errors: - raise errors[self.name] + if devices.errors: + raise Exception("??? build") else: - device = devices[self.name] + # device = devices[self.name][0] + if connect_immediately: + conn = devices.connect(timeout=timeout) + if conn.connection_errors: + raise Exception("??? conn") + device = devices.devices[self.name][0] if name: device.set_name(device) + elif self.use_factory_name: + device.set_name(self.name) return device def __call__(self, *args, **kwargs) -> T: @@ -149,19 +165,19 @@ class ConnectionResult(NamedTuple): class DeviceBuildResult(NamedTuple): - devices: dict[str, Any] + devices: dict[str, tuple[Any, bool]] errors: dict[str, Exception] def connect( - self, mock: bool = False, run_engine: RunEngine | None = None + self, timeout: float | None = None, run_engine: RunEngine | None = None ) -> ConnectionResult: connections = {} loop: asyncio.EventLoop = ( # type: ignore run_engine.loop if run_engine else get_bluesky_event_loop() ) - for name, device in self.devices.items(): + for name, (device, mock) in self.devices.items(): fut: futures.Future = asyncio.run_coroutine_threadsafe( - device.connect(mock=mock), # type: ignore + device.connect(mock=mock, timeout=timeout or 10), # type: ignore loop=loop, ) connections[name] = fut @@ -171,12 +187,26 @@ def connect( for name, connection_future in connections.items(): try: connection_future.result(timeout=12) - connected[name] = self.devices[name] + connected[name] = self.devices[name][0] except Exception as e: connection_errors[name] = e return ConnectionResult(connected, self.errors, connection_errors) + # @typing.overload + # def __getitem__(self, idx: Literal[0]) -> dict[str, Any]: ... + # @typing.overload + # def __getitem__(self, idx: Literal[1]) -> dict[str, Exception]: ... + + # def __getitem__(self, idx): + # match idx: + # case 0: + # return {n: d[0] for n, d in self.devices.items()} + # case 1: + # return self.errors + # case _: + # raise IndexError() + class DeviceManager: _factories: dict[str, DeviceFactory] @@ -228,8 +258,8 @@ def build_all( include_skipped=False, fixtures: dict[str, Any] | None = None, mock: bool = False, - connect_immediately: bool = False, - timeout: float | None = None, + # connect_immediately: bool = False, + # timeout: float | None = None, ) -> DeviceBuildResult: fixtures = fixtures or {} # exclude all skipped devices and those that have been overridden by fixtures @@ -242,8 +272,8 @@ def build_all( ), fixtures=fixtures, mock=mock, - connect_immediately=connect_immediately, - timeout=timeout, + # connect_immediately=connect_immediately, + # timeout=timeout, ) def build_devices( @@ -251,8 +281,8 @@ def build_devices( *factories: DeviceFactory, fixtures: dict[str, Any] | None = None, mock: bool = False, - connect_immediately: bool = False, - timeout: float | None = None, + # connect_immediately: bool = False, + # timeout: float | None = None, ) -> DeviceBuildResult: """ Build the devices from the given factories, ensuring that any @@ -271,7 +301,7 @@ def build_devices( order = self._build_order( {dep: self._factories[dep] for dep in build_list}, fixtures=fixtures ) - built = {} + built: dict[str, tuple[Any, bool]] = {} errors = {} for device in order: factory = self[device] @@ -288,7 +318,7 @@ def build_devices( } try: built_device = factory(**params) - built[device] = built_device + built[device] = (built_device, mock or factory.mock) except Exception as e: errors[device] = e From 62f41f7c484f4535277a22862a813a08779179e6 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 30 Sep 2025 16:56:14 +0100 Subject: [PATCH 14/74] build_and_connect method --- src/dodal/beamlines/dm_demo.py | 62 ++++++++++++++-------------------- src/dodal/cli.py | 13 ++----- 2 files changed, 27 insertions(+), 48 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 644aae63359..1c257521efd 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -23,7 +23,6 @@ call_in_bluesky_event_loop, get_bluesky_event_loop, ) -from ophyd import Device from ophyd_async.core import PathProvider from ophyd_async.epics.adsimdetector import SimDetector from ophyd_async.epics.motor import Motor @@ -36,7 +35,6 @@ from dodal.utils import ( AnyDevice, BeamlinePrefix, - OphydV1Device, OphydV2Device, SkipType, ) @@ -126,18 +124,15 @@ def build( self, fixtures=fixtures, mock=mock, - # connect_immediately=connect_immediately, - # timeout=timeout or self.timeout, ) if devices.errors: raise Exception("??? build") else: - # device = devices[self.name][0] if connect_immediately: conn = devices.connect(timeout=timeout) if conn.connection_errors: raise Exception("??? conn") - device = devices.devices[self.name][0] + device = devices.devices[self.name].device if name: device.set_name(device) elif self.use_factory_name: @@ -158,26 +153,36 @@ def __repr__(self) -> str: return f"<{self.name}: DeviceFactory ({params}) -> {target}>" +class ConnectionSpec(NamedTuple): + device: Any + mock: bool + + class ConnectionResult(NamedTuple): devices: dict[str, AnyDevice] build_errors: dict[str, Exception] connection_errors: dict[str, Exception] + def or_raise(self) -> dict[str, Any]: + if self.build_errors or self.connection_errors: + all_exc = [] + for name, exc in (self.build_errors | self.connection_errors).items(): + exc.add_note(name) + all_exc.append(exc) + raise ExceptionGroup("Some devices failed", tuple(all_exc)) + return self.devices + class DeviceBuildResult(NamedTuple): - devices: dict[str, tuple[Any, bool]] + devices: dict[str, ConnectionSpec] errors: dict[str, Exception] - def connect( - self, timeout: float | None = None, run_engine: RunEngine | None = None - ) -> ConnectionResult: + def connect(self, timeout: float | None = None) -> ConnectionResult: connections = {} - loop: asyncio.EventLoop = ( # type: ignore - run_engine.loop if run_engine else get_bluesky_event_loop() - ) + loop: asyncio.EventLoop = get_bluesky_event_loop() # type: ignore for name, (device, mock) in self.devices.items(): fut: futures.Future = asyncio.run_coroutine_threadsafe( - device.connect(mock=mock, timeout=timeout or 10), # type: ignore + device.connect(mock=mock, timeout=timeout or 5), # type: ignore loop=loop, ) connections[name] = fut @@ -187,26 +192,12 @@ def connect( for name, connection_future in connections.items(): try: connection_future.result(timeout=12) - connected[name] = self.devices[name][0] + connected[name] = self.devices[name].device except Exception as e: connection_errors[name] = e return ConnectionResult(connected, self.errors, connection_errors) - # @typing.overload - # def __getitem__(self, idx: Literal[0]) -> dict[str, Any]: ... - # @typing.overload - # def __getitem__(self, idx: Literal[1]) -> dict[str, Exception]: ... - - # def __getitem__(self, idx): - # match idx: - # case 0: - # return {n: d[0] for n, d in self.devices.items()} - # case 1: - # return self.errors - # case _: - # raise IndexError() - class DeviceManager: _factories: dict[str, DeviceFactory] @@ -253,13 +244,14 @@ def decorator(func: Callable[Args, T]) -> DeviceFactory[Args, T]: return decorator return decorator(func) + def build_and_connect(self, fixtures, mock) -> ConnectionResult: + return self.build_all(fixtures=fixtures, mock=mock).connect() + def build_all( self, include_skipped=False, fixtures: dict[str, Any] | None = None, mock: bool = False, - # connect_immediately: bool = False, - # timeout: float | None = None, ) -> DeviceBuildResult: fixtures = fixtures or {} # exclude all skipped devices and those that have been overridden by fixtures @@ -272,8 +264,6 @@ def build_all( ), fixtures=fixtures, mock=mock, - # connect_immediately=connect_immediately, - # timeout=timeout, ) def build_devices( @@ -281,8 +271,6 @@ def build_devices( *factories: DeviceFactory, fixtures: dict[str, Any] | None = None, mock: bool = False, - # connect_immediately: bool = False, - # timeout: float | None = None, ) -> DeviceBuildResult: """ Build the devices from the given factories, ensuring that any @@ -301,7 +289,7 @@ def build_devices( order = self._build_order( {dep: self._factories[dep] for dep in build_list}, fixtures=fixtures ) - built: dict[str, tuple[Any, bool]] = {} + built: dict[str, ConnectionSpec] = {} errors = {} for device in order: factory = self[device] @@ -318,7 +306,7 @@ def build_devices( } try: built_device = factory(**params) - built[device] = (built_device, mock or factory.mock) + built[device] = ConnectionSpec(built_device, mock or factory.mock) except Exception as e: errors[device] = e diff --git a/src/dodal/cli.py b/src/dodal/cli.py index fd839539d11..8c54bd11718 100644 --- a/src/dodal/cli.py +++ b/src/dodal/cli.py @@ -73,20 +73,11 @@ def connect(beamline: str, all: bool, sim_backend: bool) -> None: manager, DeviceManager ): path_provider = StaticPathProvider(UUIDFilenameProvider(), Path("/tmp")) - device_result = manager.build_all( - mock=sim_backend, # only used by v1 devices + devices, instance_exceptions, connect_exceptions = manager.build_and_connect( + mock=sim_backend, fixtures={"path_provider": path_provider}, ) - devices, instance_exceptions, connect_exceptions = device_result.connect( - mock=sim_backend - ) else: - # Fall back to the previous approach - # warnings.warn( - # "Using deprecated @device_factory approach - consider using DeviceManager", - # DeprecationWarning, - # stacklevel=1, - # ) _spoof_path_provider() devices, instance_exceptions = make_all_devices( full_module_path, From 37cdb9ec83493eca119f7f7a3a1678b0ada59f86 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 30 Sep 2025 17:23:23 +0100 Subject: [PATCH 15/74] Multiple manager CLI --- src/dodal/cli.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/dodal/cli.py b/src/dodal/cli.py index 8c54bd11718..b844e9df58c 100644 --- a/src/dodal/cli.py +++ b/src/dodal/cli.py @@ -46,7 +46,8 @@ def main(ctx: click.Context) -> None: "attempt any I/O. Useful as a a dry-run.", default=False, ) -def connect(beamline: str, all: bool, sim_backend: bool) -> None: +@click.option("-n", "--name", multiple=True) +def connect(beamline: str, all: bool, sim_backend: bool, name: tuple[str, ...]) -> None: """Initialises a beamline module, connects to all devices, reports any connection issues.""" @@ -69,14 +70,19 @@ def connect(beamline: str, all: bool, sim_backend: bool) -> None: # Don't connect devices as they're built and do connection as an extra step, # because the alternatives is handling the fact that only some devices may # be lazy. - if (manager := getattr(mod, "devices", None)) and isinstance( - manager, DeviceManager - ): - path_provider = StaticPathProvider(UUIDFilenameProvider(), Path("/tmp")) - devices, instance_exceptions, connect_exceptions = manager.build_and_connect( - mock=sim_backend, - fixtures={"path_provider": path_provider}, - ) + + if name: + for manager_name in name: + if (manager := getattr(mod, manager_name, None)) and isinstance( + manager, DeviceManager + ): + path_provider = StaticPathProvider(UUIDFilenameProvider(), Path("/tmp")) + devices, instance_exceptions, connect_exceptions = ( + manager.build_and_connect( + mock=sim_backend, + fixtures={"path_provider": path_provider}, + ) + ) else: _spoof_path_provider() devices, instance_exceptions = make_all_devices( From 7f15f341ede27d109c8042e625d14c260c1873c5 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 30 Sep 2025 17:35:38 +0100 Subject: [PATCH 16/74] FIXTUEES --- src/dodal/beamlines/adsim.py | 14 ++++++++++++-- src/dodal/beamlines/dm_demo.py | 13 +++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/dodal/beamlines/adsim.py b/src/dodal/beamlines/adsim.py index 98603b7a27e..fd2acb5fd08 100644 --- a/src/dodal/beamlines/adsim.py +++ b/src/dodal/beamlines/adsim.py @@ -1,4 +1,6 @@ +from pathlib import PurePath from ophyd.epics_motor import EpicsMotor +from ophyd_async.core import PathProvider, StaticPathProvider, UUIDFilenameProvider from ophyd_async.epics.adsimdetector import SimDetector from dodal.beamlines.dm_demo import DeviceManager @@ -64,7 +66,12 @@ devices = DeviceManager() -@devices.factory(timeout=2, mock=True) +@devices.fixture +def path_provider() -> PathProvider: + return StaticPathProvider(UUIDFilenameProvider(), PurePath("/")) + + +@devices.factory(timeout=2, mock=False) def stage() -> XThetaStage: return XThetaStage( f"{PREFIX.beamline_prefix}-MO-SIMC-01:", x_infix="M1", theta_infix="M2" @@ -81,6 +88,9 @@ def det(path_provider) -> SimDetector: ) -# @devices.factory +# v1_devices = OphydV1DeviceManager() + + +# @v1_devices.factory # def old_motor() -> EpicsMotor: # return EpicsMotor(name="old_motor", prefix=f"{PREFIX.beamline_prefix}-MO-SIMC-01:") diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 1c257521efd..e6fb01efb4d 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -201,9 +201,14 @@ def connect(self, timeout: float | None = None) -> ConnectionResult: class DeviceManager: _factories: dict[str, DeviceFactory] + _fixtures: dict[str, Any] def __init__(self): self._factories = {} + self._fixtures = {} + + def fixture(self, func: Callable[[], Any]): + self._fixtures[func.__name__] = func # Overload for using as plain decorator, ie: @devices.factory @typing.overload @@ -277,7 +282,15 @@ def build_devices( dependencies are built first and passed to later factories as required. """ # TODO: Should we check all the factories are our factories? + + # TODO: build fixtures lazily fixtures = fixtures or {} + builtin_fixtures = { + name: func() + for name, func in self._fixtures.items() + if name not in fixtures + } + fixtures = builtin_fixtures | fixtures if common := fixtures.keys() & {f.name for f in factories}: warnings.warn( f"Factories ({common}) will be overridden by fixtures", stacklevel=1 From db69729f3137db9930e4404ae6cc819085a0d6d0 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 1 Oct 2025 12:09:18 +0100 Subject: [PATCH 17/74] Use device decorator for timeouts --- src/dodal/beamlines/dm_demo.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index e6fb01efb4d..922287b4945 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -129,7 +129,7 @@ def build( raise Exception("??? build") else: if connect_immediately: - conn = devices.connect(timeout=timeout) + conn = devices.connect(timeout=timeout or self.timeout) if conn.connection_errors: raise Exception("??? conn") device = devices.devices[self.name].device @@ -153,9 +153,14 @@ def __repr__(self) -> str: return f"<{self.name}: DeviceFactory ({params}) -> {target}>" +class ConnectionParameters(NamedTuple): + mock: bool + timeout: float + + class ConnectionSpec(NamedTuple): device: Any - mock: bool + params: ConnectionParameters class ConnectionResult(NamedTuple): @@ -180,9 +185,10 @@ class DeviceBuildResult(NamedTuple): def connect(self, timeout: float | None = None) -> ConnectionResult: connections = {} loop: asyncio.EventLoop = get_bluesky_event_loop() # type: ignore - for name, (device, mock) in self.devices.items(): + for name, (device, (mock, dev_timeout)) in self.devices.items(): + timeout = timeout or dev_timeout or DEFAULT_TIMEOUT fut: futures.Future = asyncio.run_coroutine_threadsafe( - device.connect(mock=mock, timeout=timeout or 5), # type: ignore + device.connect(mock=mock, timeout=timeout), # type: ignore loop=loop, ) connections[name] = fut @@ -191,7 +197,7 @@ def connect(self, timeout: float | None = None) -> ConnectionResult: connection_errors = {} for name, connection_future in connections.items(): try: - connection_future.result(timeout=12) + connection_future.result() connected[name] = self.devices[name].device except Exception as e: connection_errors[name] = e @@ -319,7 +325,13 @@ def build_devices( } try: built_device = factory(**params) - built[device] = ConnectionSpec(built_device, mock or factory.mock) + built[device] = ConnectionSpec( + built_device, + ConnectionParameters( + mock=mock or factory.mock, + timeout=factory.timeout, + ), + ) except Exception as e: errors[device] = e From 636ba067a470fa05f990d301db835636e00e3f1a Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 1 Oct 2025 12:09:57 +0100 Subject: [PATCH 18/74] Type build_and_connect and rely on fixtures for path provider --- src/dodal/beamlines/adsim.py | 2 +- src/dodal/beamlines/dm_demo.py | 5 ++++- src/dodal/cli.py | 2 -- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/dodal/beamlines/adsim.py b/src/dodal/beamlines/adsim.py index fd2acb5fd08..02ab9947e17 100644 --- a/src/dodal/beamlines/adsim.py +++ b/src/dodal/beamlines/adsim.py @@ -79,7 +79,7 @@ def stage() -> XThetaStage: @devices.factory(timeout=2) -def det(path_provider) -> SimDetector: +def det(path_provider: PathProvider) -> SimDetector: return SimDetector( f"{PREFIX.beamline_prefix}-DI-CAM-01:", path_provider=path_provider, diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 922287b4945..3484324dd64 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -214,6 +214,7 @@ def __init__(self): self._fixtures = {} def fixture(self, func: Callable[[], Any]): + """Add a function that can provide fixtures required by the factories""" self._fixtures[func.__name__] = func # Overload for using as plain decorator, ie: @devices.factory @@ -255,7 +256,9 @@ def decorator(func: Callable[Args, T]) -> DeviceFactory[Args, T]: return decorator return decorator(func) - def build_and_connect(self, fixtures, mock) -> ConnectionResult: + def build_and_connect( + self, *, fixtures: dict[str, Any] | None = None, mock: bool = False + ) -> ConnectionResult: return self.build_all(fixtures=fixtures, mock=mock).connect() def build_all( diff --git a/src/dodal/cli.py b/src/dodal/cli.py index b844e9df58c..6a0ae809139 100644 --- a/src/dodal/cli.py +++ b/src/dodal/cli.py @@ -76,11 +76,9 @@ def connect(beamline: str, all: bool, sim_backend: bool, name: tuple[str, ...]) if (manager := getattr(mod, manager_name, None)) and isinstance( manager, DeviceManager ): - path_provider = StaticPathProvider(UUIDFilenameProvider(), Path("/tmp")) devices, instance_exceptions, connect_exceptions = ( manager.build_and_connect( mock=sim_backend, - fixtures={"path_provider": path_provider}, ) ) else: From 23d5c1e76e6d0aed38f3f755e10947399640b412 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 1 Oct 2025 13:15:57 +0100 Subject: [PATCH 19/74] mostly reformatting --- src/dodal/beamlines/dm_demo.py | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 3484324dd64..4d9608231a6 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -1,26 +1,21 @@ import asyncio -from concurrent import futures -import concurrent import inspect import typing import warnings from collections.abc import Callable, Iterable +from concurrent import futures from functools import cached_property, wraps from types import NoneType from typing import ( Annotated, Any, Generic, - Literal, - Mapping, NamedTuple, ParamSpec, TypeVar, ) from bluesky.run_engine import ( - RunEngine, - call_in_bluesky_event_loop, get_bluesky_event_loop, ) from ophyd_async.core import PathProvider @@ -126,24 +121,23 @@ def build( mock=mock, ) if devices.errors: + # TODO: NotBuilt? raise Exception("??? build") else: if connect_immediately: conn = devices.connect(timeout=timeout or self.timeout) if conn.connection_errors: + # TODO: NotConnected? raise Exception("??? conn") device = devices.devices[self.name].device if name: device.set_name(device) - elif self.use_factory_name: - device.set_name(self.name) return device def __call__(self, *args, **kwargs) -> T: device = self.factory(*args, **kwargs) - if isinstance(device, OphydV2Device): - if self.use_factory_name: - device.set_name(self.name) + if isinstance(device, OphydV2Device) and self.use_factory_name: + device.set_name(self.name) return device def __repr__(self) -> str: From ab2346b6389891e40705eaa84cb404f77b5970ae Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 1 Oct 2025 14:29:27 +0100 Subject: [PATCH 20/74] Make fixture generators lazy --- src/dodal/beamlines/dm_demo.py | 80 ++++++++++++++++++++++++++-------- 1 file changed, 62 insertions(+), 18 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 4d9608231a6..5d79e347148 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -1,15 +1,18 @@ import asyncio import inspect +import itertools import typing import warnings -from collections.abc import Callable, Iterable +from collections.abc import Callable, Iterable, Mapping from concurrent import futures +from dataclasses import InitVar, dataclass, field from functools import cached_property, wraps from types import NoneType from typing import ( Annotated, Any, Generic, + MutableMapping, NamedTuple, ParamSpec, TypeVar, @@ -44,6 +47,50 @@ Args = ParamSpec("Args") +_EMPTY = object() +"""Sentinel value to distinguish between missing values and present but null values""" + + +class LazyFixtures(Mapping[str, Any]): + """ + Wrapper around fixtures and fixture generators + + If a fixture is provided at runtime, the generator function does not have to be called. + """ + + ready: MutableMapping[str, Any] = field(init=False) + lazy: MutableMapping[str, Callable[[], Any]] = field(init=False) + + def __init__( + self, + provided: Mapping[str, Any] | None, + factories: Mapping[str, Callable[[], Any]], + ): + self.ready = dict(provided or {}) + # wrap to prevent modification escaping + # drop duplicate keys so the len and iter methods are easier + self.lazy = {k: v for k, v in factories.items() if k not in self.ready} + + def __contains__(self, key: Any) -> bool: + return key in self.ready or key in self.lazy + + def __len__(self) -> int: + # Can just add the lengths as the keys are distinct by construction + return len(self.ready.keys()) + len(self.lazy.keys()) + + def __getitem__(self, key: str) -> Any: + if (value := self.ready.get(key, _EMPTY)) is not _EMPTY: + return value + if factory := self.lazy.pop(key, None): + value = factory() + self.ready[key] = value + return value + raise KeyError(key) + + def __iter__(self): + return itertools.chain(self.lazy.keys(), self.ready.keys()) + + class DeviceFactory(Generic[Args, T]): """ Wrapper around a device factory (any function returning a device) that holds @@ -201,7 +248,7 @@ def connect(self, timeout: float | None = None) -> ConnectionResult: class DeviceManager: _factories: dict[str, DeviceFactory] - _fixtures: dict[str, Any] + _fixtures: dict[str, Callable[[], Any]] def __init__(self): self._factories = {} @@ -261,14 +308,15 @@ def build_all( fixtures: dict[str, Any] | None = None, mock: bool = False, ) -> DeviceBuildResult: - fixtures = fixtures or {} # exclude all skipped devices and those that have been overridden by fixtures return self.build_devices( *( f for f in self._factories.values() # allow overriding skip but still allow fixtures to override devices - if (include_skipped or not f.skip) and f.name not in fixtures + if (include_skipped or not f.skip) + # don't build anything that has been overridden by a fixture + and (not fixtures or f.name not in fixtures) ), fixtures=fixtures, mock=mock, @@ -277,7 +325,7 @@ def build_all( def build_devices( self, *factories: DeviceFactory, - fixtures: dict[str, Any] | None = None, + fixtures: Mapping[str, Any] | None = None, mock: bool = False, ) -> DeviceBuildResult: """ @@ -286,14 +334,7 @@ def build_devices( """ # TODO: Should we check all the factories are our factories? - # TODO: build fixtures lazily - fixtures = fixtures or {} - builtin_fixtures = { - name: func() - for name, func in self._fixtures.items() - if name not in fixtures - } - fixtures = builtin_fixtures | fixtures + fixtures = LazyFixtures(provided=fixtures, factories=self._fixtures) if common := fixtures.keys() & {f.name for f in factories}: warnings.warn( f"Factories ({common}) will be overridden by fixtures", stacklevel=1 @@ -316,9 +357,12 @@ def build_devices( # If we've made it this far, any devices that aren't available must have default # values so ignore anything that's missing params = { - dep: built.get(dep) or fixtures[dep] + dep: value for dep in deps - if dep in built.keys() | fixtures.keys() + # get from built if it's there, from fixtures otherwise... + if (value := (built.get(dep, fixtures.get(dep, _EMPTY)))) + # ...and skip if in neither + is not _EMPTY } try: built_device = factory(**params) @@ -340,7 +384,7 @@ def __getitem__(self, name): def _expand_dependencies( self, factories: Iterable[DeviceFactory[Any, Any]], - available_fixtures: dict[str, Any], + available_fixtures: Mapping[str, Any], ) -> set[str]: """ Determine full list of devices that are required to build the given devices. @@ -371,7 +415,7 @@ def _expand_dependencies( return dependencies def _build_order( - self, factories: dict[str, DeviceFactory], fixtures=None + self, factories: dict[str, DeviceFactory], fixtures: Mapping[str, Any] ) -> list[str]: """ Determine the order devices in which devices should be build to ensure @@ -381,7 +425,7 @@ def _build_order( given factory list. """ order = [] - available = set(fixtures or {}) + available = set(fixtures.keys()) pending = factories while pending: buffer = {} From 7635976b38cfd7fb5f1e917acab8b790115f1d54 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 1 Oct 2025 14:29:49 +0100 Subject: [PATCH 21/74] Apologise for build_order method --- src/dodal/beamlines/dm_demo.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 5d79e347148..bd8ba6f0f40 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -424,6 +424,9 @@ def _build_order( Assumes that all required devices and fixtures are included in the given factory list. """ + + # TODO: This is not an efficient way of doing this + # However, for realistic use cases, it is fast enough for now order = [] available = set(fixtures.keys()) pending = factories From c7fcad13f0027294d6aa50b0605e78741e522f7e Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 1 Oct 2025 14:34:11 +0100 Subject: [PATCH 22/74] Return function from fixture decorator --- src/dodal/beamlines/dm_demo.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index bd8ba6f0f40..33e228bbb7d 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -254,9 +254,10 @@ def __init__(self): self._factories = {} self._fixtures = {} - def fixture(self, func: Callable[[], Any]): + def fixture(self, func: Callable[[], T]) -> Callable[[], T]: """Add a function that can provide fixtures required by the factories""" self._fixtures[func.__name__] = func + return func # Overload for using as plain decorator, ie: @devices.factory @typing.overload From 5e886984fe63debc59439da813199988ff6d4f21 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 1 Oct 2025 14:34:32 +0100 Subject: [PATCH 23/74] Add timeout parameter to build_and_connect --- src/dodal/beamlines/dm_demo.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 33e228bbb7d..db74953ef0a 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -299,9 +299,13 @@ def decorator(func: Callable[Args, T]) -> DeviceFactory[Args, T]: return decorator(func) def build_and_connect( - self, *, fixtures: dict[str, Any] | None = None, mock: bool = False + self, + *, + fixtures: dict[str, Any] | None = None, + mock: bool = False, + timeout: float | None = None, ) -> ConnectionResult: - return self.build_all(fixtures=fixtures, mock=mock).connect() + return self.build_all(fixtures=fixtures, mock=mock).connect(timeout=timeout) def build_all( self, From 7f0b9271b419fdb2b7d3c0abe78ea2c76d5faca6 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 1 Oct 2025 14:34:44 +0100 Subject: [PATCH 24/74] Remove dead comment --- src/dodal/beamlines/dm_demo.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index db74953ef0a..9a8a1edbf48 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -337,7 +337,6 @@ def build_devices( Build the devices from the given factories, ensuring that any dependencies are built first and passed to later factories as required. """ - # TODO: Should we check all the factories are our factories? fixtures = LazyFixtures(provided=fixtures, factories=self._fixtures) if common := fixtures.keys() & {f.name for f in factories}: From 922f0f548530202e760b986e565cfe7b2ce9fec3 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 1 Oct 2025 15:16:27 +0100 Subject: [PATCH 25/74] Use set in expand_dependencies to prevent repetition --- src/dodal/beamlines/dm_demo.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 9a8a1edbf48..71d330ec1ff 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -397,11 +397,11 @@ def _expand_dependencies( be overridden. Errors: - If a required dependencies is not available as either a device + If a required dependency is not available as either a device factory or a fixture, a ValueError is raised """ dependencies = set() - factories = list(factories) + factories = set(factories) while factories: fact = factories.pop() dependencies.add(fact.name) @@ -409,8 +409,7 @@ def _expand_dependencies( for dep in fact.dependencies: if dep not in dependencies and dep not in available_fixtures: if dep in self._factories: - dependencies.add(dep) - factories.append(self[dep]) + factories.add(self[dep]) elif dep not in options: raise ValueError( f"Missing fixture or factory for {dep}", From 71d72cdba668d5b612b0df0138cd98970c3473ff Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 2 Oct 2025 13:25:15 +0100 Subject: [PATCH 26/74] Check for duplicate factory names --- src/dodal/beamlines/dm_demo.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index 71d330ec1ff..e1a63cd8027 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -291,6 +291,8 @@ def factory( ) -> DeviceFactory[Args, T] | Callable[[Callable[Args, T]], DeviceFactory[Args, T]]: def decorator(func: Callable[Args, T]) -> DeviceFactory[Args, T]: factory = DeviceFactory(func, use_factory_name, timeout, mock, skip, self) + if func.__name__ in self._factories: + raise ValueError(f"Duplicate factory name: {func.__name__}") self._factories[func.__name__] = factory return factory From 13f0380c413cf9cc6f0df741906dbefe21e174ab Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 21 Oct 2025 17:04:28 +0100 Subject: [PATCH 27/74] Add Ophyd v1 support --- src/dodal/beamlines/adsim.py | 9 +- src/dodal/beamlines/dm_demo.py | 219 ++++++++++++++++++++++++++++----- 2 files changed, 188 insertions(+), 40 deletions(-) diff --git a/src/dodal/beamlines/adsim.py b/src/dodal/beamlines/adsim.py index 02ab9947e17..8acf0fbe41a 100644 --- a/src/dodal/beamlines/adsim.py +++ b/src/dodal/beamlines/adsim.py @@ -88,9 +88,6 @@ def det(path_provider: PathProvider) -> SimDetector: ) -# v1_devices = OphydV1DeviceManager() - - -# @v1_devices.factory -# def old_motor() -> EpicsMotor: -# return EpicsMotor(name="old_motor", prefix=f"{PREFIX.beamline_prefix}-MO-SIMC-01:") +@devices.v1_init(factory=EpicsMotor, prefix=f"{PREFIX.beamline_prefix}-MO-SIMC-01:M1") +def old_motor(motor: EpicsMotor): + print(f"Built {motor}") diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index e1a63cd8027..c7fcdf56c02 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -3,29 +3,37 @@ import itertools import typing import warnings -from collections.abc import Callable, Iterable, Mapping +from collections.abc import Callable, Iterable, Mapping, MutableMapping from concurrent import futures -from dataclasses import InitVar, dataclass, field from functools import cached_property, wraps +from inspect import Parameter from types import NoneType from typing import ( Annotated, Any, + Concatenate, Generic, - MutableMapping, NamedTuple, ParamSpec, TypeVar, + cast, ) from bluesky.run_engine import ( get_bluesky_event_loop, ) +from ophyd import EpicsMotor +from ophyd.sim import make_fake_device from ophyd_async.core import PathProvider from ophyd_async.epics.adsimdetector import SimDetector from ophyd_async.epics.motor import Motor -from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline +from dodal.common.beamlines.beamline_utils import ( + set_beamline as set_utils_beamline, +) +from dodal.common.beamlines.beamline_utils import ( + wait_for_connection, +) from dodal.common.beamlines.device_helpers import DET_SUFFIX, HDF5_SUFFIX from dodal.devices import motors from dodal.devices.motors import XThetaStage @@ -33,6 +41,7 @@ from dodal.utils import ( AnyDevice, BeamlinePrefix, + OphydV1Device, OphydV2Device, SkipType, ) @@ -46,6 +55,11 @@ T = TypeVar("T") Args = ParamSpec("Args") +V1 = TypeVar("V1", bound=OphydV1Device) +V2 = TypeVar("V2", bound=OphydV2Device) + +DeviceFactoryDecorator = Callable[[Callable[Args, V2]], "DeviceFactory[Args, V2]"] +OphydInitialiser = Callable[Concatenate[V1, ...], V1 | None] _EMPTY = object() """Sentinel value to distinguish between missing values and present but null values""" @@ -58,8 +72,8 @@ class LazyFixtures(Mapping[str, Any]): If a fixture is provided at runtime, the generator function does not have to be called. """ - ready: MutableMapping[str, Any] = field(init=False) - lazy: MutableMapping[str, Callable[[], Any]] = field(init=False) + ready: MutableMapping[str, Any] + lazy: MutableMapping[str, Callable[[], Any]] def __init__( self, @@ -91,7 +105,7 @@ def __iter__(self): return itertools.chain(self.lazy.keys(), self.ready.keys()) -class DeviceFactory(Generic[Args, T]): +class DeviceFactory(Generic[Args, V2]): """ Wrapper around a device factory (any function returning a device) that holds a reference to a device manager that can provide dependencies, along with @@ -99,7 +113,7 @@ class DeviceFactory(Generic[Args, T]): connected. """ - factory: Callable[Args, T] + factory: Callable[Args, V2] use_factory_name: bool timeout: float mock: bool @@ -108,7 +122,7 @@ class DeviceFactory(Generic[Args, T]): def __init__(self, factory, use_factory_name, timeout, mock, skip, manager): if any( - p.kind == inspect.Parameter.POSITIONAL_ONLY + p.kind == Parameter.POSITIONAL_ONLY for p in inspect.signature(factory).parameters.values() ): raise ValueError(f"{factory.__name__} has positional only arguments") @@ -126,7 +140,7 @@ def name(self) -> str: return self.factory.__name__ @property - def device_type(self) -> type[T]: + def device_type(self) -> type[V2]: return inspect.signature(self.factory).return_annotation @cached_property @@ -142,7 +156,7 @@ def optional_dependencies(self) -> set[str]: return { para.name for para in sig.parameters.values() - if para.default is not inspect.Parameter.empty + if para.default is not Parameter.empty } @property @@ -160,7 +174,7 @@ def build( name: str | None = None, timeout: float | None = None, **fixtures, - ) -> T: + ) -> V2: """Build this device, building any dependencies first""" devices = self._manager.build_devices( self, @@ -178,13 +192,15 @@ def build( raise Exception("??? conn") device = devices.devices[self.name].device if name: - device.set_name(device) + device.set_name(name) return device - def __call__(self, *args, **kwargs) -> T: + def _create(self, *args, **kwargs) -> V2: + return self(*args, **kwargs) + + def __call__(self, *args, **kwargs) -> V2: device = self.factory(*args, **kwargs) - if isinstance(device, OphydV2Device) and self.use_factory_name: - device.set_name(self.name) + device.set_name(self.name) return device def __repr__(self) -> str: @@ -194,13 +210,103 @@ def __repr__(self) -> str: return f"<{self.name}: DeviceFactory ({params}) -> {target}>" +class V1DeviceFactory(Generic[V1]): + def __init__( + self, + *, + factory: type[V1], + prefix: str, + mock: bool, + skip: SkipType, + wait: bool, + timeout: int, + init: OphydInitialiser[V1] | None, + manager: "DeviceManager", + ): + self.factory = factory + self.prefix = prefix + self.mock = mock + self._skip = skip + self.wait = wait + self.timeout = timeout + self.post_create = init or (lambda x: x) + self._manager = manager + if init: + wraps(init)(self) + + @property + def name(self) -> str: + """Name of the underlying factory function""" + return self.post_create.__name__ + + @property + def device_type(self) -> type[V1]: + return self.factory + + @cached_property + def dependencies(self) -> set[str]: + """Names of all parameters""" + sig = inspect.signature(self.post_create) + # first parameter should be the device we've just built + _, *params = sig.parameters.values() + return {para.name for para in params} + + @cached_property + def optional_dependencies(self) -> set[str]: + """Names of optional dependencies""" + sig = inspect.signature(self.post_create) + _, *params = sig.parameters.values() + return {para.name for para in params if para.default is not Parameter.empty} + + @property + def skip(self) -> bool: + """ + Whether this device should be skipped as part of build_all - it will + still be built if a required device depends on it + """ + return self._skip() if callable(self._skip) else self._skip + + def __call__(self, *args, **kwargs): + """Call the wrapped function to make decorator transparent""" + return self.post_create(*args, **kwargs) + + def _create(self, *args, **kwargs) -> V1: + device = self.factory(name=self.name, prefix=self.prefix) + if self.wait: + wait_for_connection(device, timeout=self.timeout) + self.post_create(device, *args, **kwargs) + return device + + def build(self, mock: bool = False, fixtures: dict[str, Any] | None = None) -> V1: + """Build this device, building any dependencies first""" + devices = self._manager.build_devices( + self, + fixtures=fixtures, + mock=mock, + ) + if devices.errors: + # TODO: NotBuilt? + print(devices.errors) + raise Exception("??? build") + else: + # if connect_immediately: + # conn = devices.connect(timeout=timeout or self.timeout) + # if conn.connection_errors: + # # TODO: NotConnected? + # raise Exception("??? conn") + device = devices.devices[self.name].device + # if name: + # device.set_name(name) + return device + + class ConnectionParameters(NamedTuple): mock: bool timeout: float class ConnectionSpec(NamedTuple): - device: Any + device: OphydV1Device | OphydV2Device params: ConnectionParameters @@ -229,7 +335,7 @@ def connect(self, timeout: float | None = None) -> ConnectionResult: for name, (device, (mock, dev_timeout)) in self.devices.items(): timeout = timeout or dev_timeout or DEFAULT_TIMEOUT fut: futures.Future = asyncio.run_coroutine_threadsafe( - device.connect(mock=mock, timeout=timeout), # type: ignore + device.connect(mock=mock, timeout=timeout), loop=loop, ) connections[name] = fut @@ -249,9 +355,11 @@ def connect(self, timeout: float | None = None) -> ConnectionResult: class DeviceManager: _factories: dict[str, DeviceFactory] _fixtures: dict[str, Callable[[], Any]] + _v1_factories: dict[str, V1DeviceFactory] def __init__(self): self._factories = {} + self._v1_factories = {} self._fixtures = {} def fixture(self, func: Callable[[], T]) -> Callable[[], T]: @@ -259,9 +367,37 @@ def fixture(self, func: Callable[[], T]) -> Callable[[], T]: self._fixtures[func.__name__] = func return func + def v1_init( + self, + factory: type[V1], + # name: str, + prefix: str, + mock: bool = False, + skip: SkipType = False, + wait: bool = False, + ): + def decorator(init: OphydInitialiser[V1]): + name = init.__name__ + if name in self: + raise ValueError(f"Duplicate factory name: {name}") + device_factory = V1DeviceFactory( + factory=factory, + prefix=prefix, + mock=mock, + skip=skip, + wait=wait, + timeout=DEFAULT_TIMEOUT, + init=init, + manager=self, + ) + self._v1_factories[name] = device_factory + return device_factory + + return decorator + # Overload for using as plain decorator, ie: @devices.factory @typing.overload - def factory(self, func: Callable[Args, T], /) -> DeviceFactory[Args, T]: ... + def factory(self, func: Callable[Args, V2], /) -> DeviceFactory[Args, V2]: ... # Overload for using as configurable decorator, eg: @devices.factory(skip=True) @typing.overload @@ -273,11 +409,11 @@ def factory( timeout: float = DEFAULT_TIMEOUT, mock: bool = False, skip: SkipType = False, - ) -> Callable[[Callable[Args, T]], DeviceFactory[Args, T]]: ... + ) -> Callable[[Callable[Args, V2]], DeviceFactory[Args, V2]]: ... def factory( self, - func: Callable[Args, T] | None = None, + func: Callable[Args, V2] | None = None, /, use_factory_name: Annotated[bool, "Use factory name as name of device"] = True, timeout: Annotated[ @@ -288,11 +424,11 @@ def factory( SkipType, "mark the factory to be (conditionally) skipped when beamline is imported by external program", ] = False, - ) -> DeviceFactory[Args, T] | Callable[[Callable[Args, T]], DeviceFactory[Args, T]]: - def decorator(func: Callable[Args, T]) -> DeviceFactory[Args, T]: - factory = DeviceFactory(func, use_factory_name, timeout, mock, skip, self) - if func.__name__ in self._factories: + ) -> DeviceFactory[Args, V2] | DeviceFactoryDecorator[Args, V2]: + def decorator(func: Callable[Args, V2]) -> DeviceFactory[Args, V2]: + if func.__name__ in self: raise ValueError(f"Duplicate factory name: {func.__name__}") + factory = DeviceFactory(func, use_factory_name, timeout, mock, skip, self) self._factories[func.__name__] = factory return factory @@ -316,10 +452,11 @@ def build_all( mock: bool = False, ) -> DeviceBuildResult: # exclude all skipped devices and those that have been overridden by fixtures + return self.build_devices( *( f - for f in self._factories.values() + for f in (self._factories | self._v1_factories).values() # allow overriding skip but still allow fixtures to override devices if (include_skipped or not f.skip) # don't build anything that has been overridden by a fixture @@ -331,7 +468,7 @@ def build_all( def build_devices( self, - *factories: DeviceFactory, + *factories: DeviceFactory | V1DeviceFactory, fixtures: Mapping[str, Any] | None = None, mock: bool = False, ) -> DeviceBuildResult: @@ -346,11 +483,12 @@ def build_devices( f"Factories ({common}) will be overridden by fixtures", stacklevel=1 ) factories = tuple(f for f in factories if f.name not in common) - if unknown := {f for f in factories if f not in self._factories.values()}: - raise ValueError(f"Factories ({unknown}) are unknown to this manager") + # if unknown := {f for f in factories if f not in self._factories.values()}: + # raise ValueError(f"Factories ({unknown}) are unknown to this manager") build_list = self._expand_dependencies(factories, fixtures) + # print(build_list) order = self._build_order( - {dep: self._factories[dep] for dep in build_list}, fixtures=fixtures + {dep: self[dep] for dep in build_list}, fixtures=fixtures ) built: dict[str, ConnectionSpec] = {} errors = {} @@ -371,7 +509,7 @@ def build_devices( is not _EMPTY } try: - built_device = factory(**params) + built_device = factory._create(**params) built[device] = ConnectionSpec( built_device, ConnectionParameters( @@ -384,12 +522,15 @@ def build_devices( return DeviceBuildResult(built, errors) + def __contains__(self, name): + return name in self._factories or name in self._v1_factories + def __getitem__(self, name): - return self._factories[name] + return self._factories.get(name) or self._v1_factories[name] def _expand_dependencies( self, - factories: Iterable[DeviceFactory[Any, Any]], + factories: Iterable[DeviceFactory[..., V2] | V1DeviceFactory[V1]], available_fixtures: Mapping[str, Any], ) -> set[str]: """ @@ -405,10 +546,13 @@ def _expand_dependencies( dependencies = set() factories = set(factories) while factories: + # print(dependencies) fact = factories.pop() dependencies.add(fact.name) options = fact.optional_dependencies + # print(f" {fact.name}") for dep in fact.dependencies: + # print(f" {dep}") if dep not in dependencies and dep not in available_fixtures: if dep in self._factories: factories.add(self[dep]) @@ -420,7 +564,9 @@ def _expand_dependencies( return dependencies def _build_order( - self, factories: dict[str, DeviceFactory], fixtures: Mapping[str, Any] + self, + factories: dict[str, DeviceFactory[..., V2] | V1DeviceFactory[V1]], + fixtures: Mapping[str, Any], ) -> list[str]: """ Determine the order devices in which devices should be build to ensure @@ -524,6 +670,11 @@ def circ_1(circ_2): ... def circ_2(circ_1): ... +@devices.v1_init(factory=EpicsMotor, prefix=f"{PREFIX.beamline_prefix}-MO-SIMC-01:M1") +def old_motor(motor: EpicsMotor, foo: int = 42): + print(f"Built {motor.name} with {foo=}") + + if __name__ == "__main__": # for name, factory in devices._factories.items(): # print(name, factory, factory.dependencies) From cbe601215f4d8e717442cd9b0038998d1703d600 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 11:53:27 +0100 Subject: [PATCH 28/74] Connect Ophyd v1 devices --- src/dodal/beamlines/dm_demo.py | 39 +++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/beamlines/dm_demo.py index c7fcdf56c02..986e3347491 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/beamlines/dm_demo.py @@ -15,6 +15,7 @@ Generic, NamedTuple, ParamSpec, + Self, TypeVar, cast, ) @@ -193,7 +194,7 @@ def build( device = devices.devices[self.name].device if name: device.set_name(name) - return device + return device # type: ignore - it's us, honest def _create(self, *args, **kwargs) -> V2: return self(*args, **kwargs) @@ -266,6 +267,22 @@ def skip(self) -> bool: """ return self._skip() if callable(self._skip) else self._skip + def mock_if_needed(self, mock=False) -> Self: + # TODO: Remove when Ophyd V1 support is no longer required + factory = ( + make_fake_device(self.factory) if (self.mock or mock) else self.factory + ) + return self.__class__( + factory=factory, + prefix=self.prefix, + mock=mock or self.mock, + skip=self._skip, + wait=self.wait, + timeout=self.timeout, + init=self.post_create, + manager=self._manager, + ) + def __call__(self, *args, **kwargs): """Call the wrapped function to make decorator transparent""" return self.post_create(*args, **kwargs) @@ -289,15 +306,8 @@ def build(self, mock: bool = False, fixtures: dict[str, Any] | None = None) -> V print(devices.errors) raise Exception("??? build") else: - # if connect_immediately: - # conn = devices.connect(timeout=timeout or self.timeout) - # if conn.connection_errors: - # # TODO: NotConnected? - # raise Exception("??? conn") device = devices.devices[self.name].device - # if name: - # device.set_name(name) - return device + return device # type: ignore - it's us really, promise class ConnectionParameters(NamedTuple): @@ -306,7 +316,7 @@ class ConnectionParameters(NamedTuple): class ConnectionSpec(NamedTuple): - device: OphydV1Device | OphydV2Device + device: OphydV2Device params: ConnectionParameters @@ -331,16 +341,19 @@ class DeviceBuildResult(NamedTuple): def connect(self, timeout: float | None = None) -> ConnectionResult: connections = {} + connected = {} loop: asyncio.EventLoop = get_bluesky_event_loop() # type: ignore for name, (device, (mock, dev_timeout)) in self.devices.items(): + if not isinstance(device, OphydV2Device): + connected[name] = device + continue timeout = timeout or dev_timeout or DEFAULT_TIMEOUT - fut: futures.Future = asyncio.run_coroutine_threadsafe( + fut = asyncio.run_coroutine_threadsafe( device.connect(mock=mock, timeout=timeout), loop=loop, ) connections[name] = fut - connected = {} connection_errors = {} for name, connection_future in connections.items(): try: @@ -509,6 +522,8 @@ def build_devices( is not _EMPTY } try: + if isinstance(factory, V1DeviceFactory): + factory = factory.mock_if_needed(mock) built_device = factory._create(**params) built[device] = ConnectionSpec( built_device, From f7679ebe074d6eb5063e207e7d466f52d9e76be4 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 11:57:52 +0100 Subject: [PATCH 29/74] Move device_manager to new module Instead of burying it amongst the beamlines. --- src/dodal/beamlines/adsim.py | 2 +- src/dodal/cli.py | 2 +- .../dm_demo.py => device_manager.py} | 34 ++++++++++++++++--- 3 files changed, 32 insertions(+), 6 deletions(-) rename src/dodal/{beamlines/dm_demo.py => device_manager.py} (98%) diff --git a/src/dodal/beamlines/adsim.py b/src/dodal/beamlines/adsim.py index 8acf0fbe41a..c5dd2557386 100644 --- a/src/dodal/beamlines/adsim.py +++ b/src/dodal/beamlines/adsim.py @@ -3,7 +3,7 @@ from ophyd_async.core import PathProvider, StaticPathProvider, UUIDFilenameProvider from ophyd_async.epics.adsimdetector import SimDetector -from dodal.beamlines.dm_demo import DeviceManager +from dodal.device_manager import DeviceManager from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.common.beamlines.device_helpers import DET_SUFFIX, HDF5_SUFFIX from dodal.devices.motors import XThetaStage diff --git a/src/dodal/cli.py b/src/dodal/cli.py index 6a0ae809139..9f78ff40a5e 100644 --- a/src/dodal/cli.py +++ b/src/dodal/cli.py @@ -10,7 +10,7 @@ from ophyd_async.plan_stubs import ensure_connected from dodal.beamlines import all_beamline_names, module_name_for_beamline -from dodal.beamlines.dm_demo import DeviceManager +from dodal.device_manager import DeviceManager from dodal.common.beamlines.beamline_utils import set_path_provider from dodal.utils import AnyDevice, filter_ophyd_devices, make_all_devices diff --git a/src/dodal/beamlines/dm_demo.py b/src/dodal/device_manager.py similarity index 98% rename from src/dodal/beamlines/dm_demo.py rename to src/dodal/device_manager.py index 986e3347491..d707655938c 100644 --- a/src/dodal/beamlines/dm_demo.py +++ b/src/dodal/device_manager.py @@ -184,6 +184,7 @@ def build( ) if devices.errors: # TODO: NotBuilt? + print(devices.errors) raise Exception("??? build") else: if connect_immediately: @@ -674,6 +675,23 @@ def unknown(): return "unknown device" +@devices.factory +def one(): ... + + +@devices.factory +def two(one): ... + + +@devices.factory +def three(one, two): ... + + +@devices.factory +def four(one, two, three): + return 4 + + others = DeviceManager() @@ -746,9 +764,17 @@ def old_motor(motor: EpicsMotor, foo: int = 42): # print(base_x()) - res = devices.build_all(fixtures={"path_provider": "nt_path_provider"}) - print(res) - conn = res.connect() - print(conn) + # res = devices.build_all(fixtures={"path_provider": "nt_path_provider"}) + # print(res) + # conn = res.connect() + # print(conn) + + # res = four.build() + # print(res) # devices.build_devices(circ_1, circ_2) + + # print(old_motor.dependencies) + print(old_motor.build(mock=False)) + + # res = others.build_all() From 794067b780b16f3b64dc487821d63a7edd005ee4 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 11:59:26 +0100 Subject: [PATCH 30/74] Remove test code from device_manager module --- src/dodal/device_manager.py | 174 ------------------------------------ 1 file changed, 174 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index d707655938c..8b287fa12e9 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -4,7 +4,6 @@ import typing import warnings from collections.abc import Callable, Iterable, Mapping, MutableMapping -from concurrent import futures from functools import cached_property, wraps from inspect import Parameter from types import NoneType @@ -17,41 +16,24 @@ ParamSpec, Self, TypeVar, - cast, ) from bluesky.run_engine import ( get_bluesky_event_loop, ) -from ophyd import EpicsMotor from ophyd.sim import make_fake_device -from ophyd_async.core import PathProvider -from ophyd_async.epics.adsimdetector import SimDetector -from ophyd_async.epics.motor import Motor -from dodal.common.beamlines.beamline_utils import ( - set_beamline as set_utils_beamline, -) from dodal.common.beamlines.beamline_utils import ( wait_for_connection, ) -from dodal.common.beamlines.device_helpers import DET_SUFFIX, HDF5_SUFFIX -from dodal.devices import motors -from dodal.devices.motors import XThetaStage -from dodal.log import set_beamline as set_log_beamline from dodal.utils import ( AnyDevice, - BeamlinePrefix, OphydV1Device, OphydV2Device, SkipType, ) -BL = "adsim" DEFAULT_TIMEOUT = 30 -PREFIX = BeamlinePrefix("t01") -set_log_beamline(BL) -set_utils_beamline(BL) T = TypeVar("T") Args = ParamSpec("Args") @@ -622,159 +604,3 @@ def _build_order( def __repr__(self) -> str: return f"" - - -devices = DeviceManager() - - -@devices.factory(skip=True, timeout=13, mock=True, use_factory_name=False) -def stage() -> XThetaStage: - """Build the stage""" - return XThetaStage( - f"{PREFIX.beamline_prefix}-MO-SIMC-01:", x_infix="M1", theta_infix="M2" - ) - - -@devices.factory(skip=stage.skip) -def det(path_provider: PathProvider) -> SimDetector: - return SimDetector( - f"{PREFIX.beamline_prefix}-DI-CAM-01:", - path_provider=path_provider, - drv_suffix=DET_SUFFIX, - fileio_suffix=HDF5_SUFFIX, - ) - - -@devices.factory(mock=True) -def base(base_x, base_y: motors.Motor) -> Motor: - # print(base_x) - # print(base_y) - return f"{base_x} - {base_y}" - - -@devices.factory -def base_x() -> motors.Motor: - # raise ValueError("Not a base_x") - return "base_x motor" - - -@devices.factory(skip=True) -def base_y(path_provider) -> motors.Motor: - # print(f"Using {path_provider=}") - return "base_y motor" - - -@devices.factory -def optional(base_x, base_z=42): - return (base_x, base_z) - - -@devices.factory -def unknown(): - # raise ValueError("Unknown error") - return "unknown device" - - -@devices.factory -def one(): ... - - -@devices.factory -def two(one): ... - - -@devices.factory -def three(one, two): ... - - -@devices.factory -def four(one, two, three): - return 4 - - -others = DeviceManager() - - -@others.factory -def circ_1(circ_2): ... - - -@others.factory -def circ_2(circ_1): ... - - -@devices.v1_init(factory=EpicsMotor, prefix=f"{PREFIX.beamline_prefix}-MO-SIMC-01:M1") -def old_motor(motor: EpicsMotor, foo: int = 42): - print(f"Built {motor.name} with {foo=}") - - -if __name__ == "__main__": - # for name, factory in devices._factories.items(): - # print(name, factory, factory.dependencies) - # print(devices._build_order({"path_provider": ["42"]})) - - # print(devices["stage"]) - # print(devices["unknown"]) - # print(devices.build_all(fixtures={"path_provider": "numtracker"})) - # print(devices._required_fixtures((devices["base"], devices["base_y"]))) - # print(devices._expand_dependencies([devices["base"]])) - # print(devices._expand_dependencies(list(devices._factories.values()))) - # print(devices._build_order({"base": devices["base"]})) - - # print( - # "build_all", - # devices.build_all({"path_provider": "all_nt", "base_y": "other base_y"}), - # ) - # print( - # "build_some", - # devices.build_devices( - # base, det, stage, unknown, fixtures={"path_provider": "numtracker"} - # ), - # ) - - # print("base", optional) - # print("base_y", base_y) - # print("b1", b1 := base.build(path_provider="num_track")) - # print("b2", b2 := base(base_x="base_x motor", base_y="base_y motor")) - # print("b1 is b2", b1 is b2) - - # print("unknown()", unknown()) - # print("unknown.build()", unknown.build()) - - # print("circular", circ_1.build()) - - # print("optional deps", optional.dependencies) - # print("optional optional deps", optional.optional_dependencies) - # print("optional build", optional.build()) - # print("optional with override", optional.build(base_z=14)) - # print("optional without override", optional(17)) - # print("optional override required", optional.build(base_x=19)) - - # valid, errs = devices.build_all( - # fixtures={"base_x": 19, "path_provider": "numtrack"} - # ) - # print(valid) - # print(errs) - - # valid, errs = devices.build_devices( - # base_x, base, optional, fixtures={"base_x": "19", "path_provider": "nt_pp"} - # ) - # print(valid) - # print(errs) - - # print(base_x()) - - # res = devices.build_all(fixtures={"path_provider": "nt_path_provider"}) - # print(res) - # conn = res.connect() - # print(conn) - - # res = four.build() - # print(res) - - # devices.build_devices(circ_1, circ_2) - - # print(old_motor.dependencies) - print(old_motor.build(mock=False)) - - # res = others.build_all() From 7a8afda8d6ac523ffbcdc8faa13efca11b56bc07 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 12:14:01 +0100 Subject: [PATCH 31/74] Remove debugging and commented code --- src/dodal/beamlines/adsim.py | 3 ++- src/dodal/device_manager.py | 11 +---------- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/dodal/beamlines/adsim.py b/src/dodal/beamlines/adsim.py index c5dd2557386..7e11854a6ad 100644 --- a/src/dodal/beamlines/adsim.py +++ b/src/dodal/beamlines/adsim.py @@ -90,4 +90,5 @@ def det(path_provider: PathProvider) -> SimDetector: @devices.v1_init(factory=EpicsMotor, prefix=f"{PREFIX.beamline_prefix}-MO-SIMC-01:M1") def old_motor(motor: EpicsMotor): - print(f"Built {motor}") + # arbitrary post-init configuration + motor.settle_time = 12 diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 8b287fa12e9..f100efcfe96 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -63,8 +63,8 @@ def __init__( provided: Mapping[str, Any] | None, factories: Mapping[str, Callable[[], Any]], ): - self.ready = dict(provided or {}) # wrap to prevent modification escaping + self.ready = dict(provided or {}) # drop duplicate keys so the len and iter methods are easier self.lazy = {k: v for k, v in factories.items() if k not in self.ready} @@ -166,7 +166,6 @@ def build( ) if devices.errors: # TODO: NotBuilt? - print(devices.errors) raise Exception("??? build") else: if connect_immediately: @@ -286,7 +285,6 @@ def build(self, mock: bool = False, fixtures: dict[str, Any] | None = None) -> V ) if devices.errors: # TODO: NotBuilt? - print(devices.errors) raise Exception("??? build") else: device = devices.devices[self.name].device @@ -366,7 +364,6 @@ def fixture(self, func: Callable[[], T]) -> Callable[[], T]: def v1_init( self, factory: type[V1], - # name: str, prefix: str, mock: bool = False, skip: SkipType = False, @@ -479,10 +476,7 @@ def build_devices( f"Factories ({common}) will be overridden by fixtures", stacklevel=1 ) factories = tuple(f for f in factories if f.name not in common) - # if unknown := {f for f in factories if f not in self._factories.values()}: - # raise ValueError(f"Factories ({unknown}) are unknown to this manager") build_list = self._expand_dependencies(factories, fixtures) - # print(build_list) order = self._build_order( {dep: self[dep] for dep in build_list}, fixtures=fixtures ) @@ -544,13 +538,10 @@ def _expand_dependencies( dependencies = set() factories = set(factories) while factories: - # print(dependencies) fact = factories.pop() dependencies.add(fact.name) options = fact.optional_dependencies - # print(f" {fact.name}") for dep in fact.dependencies: - # print(f" {dep}") if dep not in dependencies and dep not in available_fixtures: if dep in self._factories: factories.add(self[dep]) From 10ee7369362f2d74554ebc096a8a09c4ac14d565 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 13:31:11 +0100 Subject: [PATCH 32/74] Merge connectionspec and connectionparameters --- src/dodal/device_manager.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index f100efcfe96..9e8e4071164 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -291,14 +291,11 @@ def build(self, mock: bool = False, fixtures: dict[str, Any] | None = None) -> V return device # type: ignore - it's us really, promise -class ConnectionParameters(NamedTuple): - mock: bool - timeout: float - class ConnectionSpec(NamedTuple): device: OphydV2Device - params: ConnectionParameters + mock: bool + timeout: float class ConnectionResult(NamedTuple): @@ -324,7 +321,7 @@ def connect(self, timeout: float | None = None) -> ConnectionResult: connections = {} connected = {} loop: asyncio.EventLoop = get_bluesky_event_loop() # type: ignore - for name, (device, (mock, dev_timeout)) in self.devices.items(): + for name, (device, mock, dev_timeout) in self.devices.items(): if not isinstance(device, OphydV2Device): connected[name] = device continue @@ -504,10 +501,8 @@ def build_devices( built_device = factory._create(**params) built[device] = ConnectionSpec( built_device, - ConnectionParameters( - mock=mock or factory.mock, - timeout=factory.timeout, - ), + mock=mock or factory.mock, + timeout=factory.timeout, ) except Exception as e: errors[device] = e From 1e71b35092788785db1bb2135e7449bf5dc34d6e Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 13:32:28 +0100 Subject: [PATCH 33/74] Add or_raise method to DeviceBuildResult --- src/dodal/device_manager.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 9e8e4071164..0667d3e217e 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -282,14 +282,10 @@ def build(self, mock: bool = False, fixtures: dict[str, Any] | None = None) -> V self, fixtures=fixtures, mock=mock, - ) - if devices.errors: - # TODO: NotBuilt? - raise Exception("??? build") - else: - device = devices.devices[self.name].device - return device # type: ignore - it's us really, promise + ).or_raise() + device = devices.devices[self.name].device + return device # type: ignore - it's us really, promise class ConnectionSpec(NamedTuple): @@ -342,6 +338,13 @@ def connect(self, timeout: float | None = None) -> ConnectionResult: return ConnectionResult(connected, self.errors, connection_errors) + def or_raise(self) -> Self: + if self.errors: + for name, exc in self.errors.items(): + exc.add_note(name) + raise ExceptionGroup("Some devices failed", tuple(self.errors.values())) + return self + class DeviceManager: _factories: dict[str, DeviceFactory] From 5cd9eef953a85a6f709ce46024372ea185d1a7ef Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 13:40:58 +0100 Subject: [PATCH 34/74] Add docstrings --- src/dodal/device_manager.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 0667d3e217e..4b33c8818b2 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -194,6 +194,12 @@ def __repr__(self) -> str: class V1DeviceFactory(Generic[V1]): + """ + Wrapper around an ophyd v1 device that holds a reference to a device + manager that can provide dependencies, along with default connection + information for how the created device should be connected. + """ + def __init__( self, *, @@ -289,17 +295,22 @@ def build(self, mock: bool = False, fixtures: dict[str, Any] | None = None) -> V class ConnectionSpec(NamedTuple): + """A device paired with the options used to configure it""" + device: OphydV2Device mock: bool timeout: float class ConnectionResult(NamedTuple): + """Wrapper around results of building and connecting devices""" + devices: dict[str, AnyDevice] build_errors: dict[str, Exception] connection_errors: dict[str, Exception] def or_raise(self) -> dict[str, Any]: + """Re-raise any errors from build or connect stage or return devices""" if self.build_errors or self.connection_errors: all_exc = [] for name, exc in (self.build_errors | self.connection_errors).items(): @@ -310,10 +321,13 @@ def or_raise(self) -> dict[str, Any]: class DeviceBuildResult(NamedTuple): + """Wrapper around the results of building devices""" + devices: dict[str, ConnectionSpec] errors: dict[str, Exception] def connect(self, timeout: float | None = None) -> ConnectionResult: + """Connect all devices that didn't fail to build""" connections = {} connected = {} loop: asyncio.EventLoop = get_bluesky_event_loop() # type: ignore @@ -339,6 +353,7 @@ def connect(self, timeout: float | None = None) -> ConnectionResult: return ConnectionResult(connected, self.errors, connection_errors) def or_raise(self) -> Self: + """Re-raise any build errors""" if self.errors: for name, exc in self.errors.items(): exc.add_note(name) @@ -347,6 +362,8 @@ def or_raise(self) -> Self: class DeviceManager: + """Manager to handle building and connecting interdependent devices""" + _factories: dict[str, DeviceFactory] _fixtures: dict[str, Callable[[], Any]] _v1_factories: dict[str, V1DeviceFactory] From 22d40fb09a744ebef98ddd020181a4ddac538694 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 13:44:38 +0100 Subject: [PATCH 35/74] Use or_raise to handle build errors --- src/dodal/device_manager.py | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 4b33c8818b2..fb8bb75d9af 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -163,20 +163,13 @@ def build( self, fixtures=fixtures, mock=mock, - ) - if devices.errors: - # TODO: NotBuilt? - raise Exception("??? build") - else: - if connect_immediately: - conn = devices.connect(timeout=timeout or self.timeout) - if conn.connection_errors: - # TODO: NotConnected? - raise Exception("??? conn") - device = devices.devices[self.name].device - if name: - device.set_name(name) - return device # type: ignore - it's us, honest + ).or_raise() + if connect_immediately: + devices.connect(timeout=timeout or self.timeout).or_raise() + device = devices.devices[self.name].device + if name: + device.set_name(name) + return device # type: ignore - it's us, honest def _create(self, *args, **kwargs) -> V2: return self(*args, **kwargs) From 04ff9cf62ec103e0f459a0edde2e64a2d9ffbe28 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 13:51:01 +0100 Subject: [PATCH 36/74] Only set device name if required --- src/dodal/device_manager.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index fb8bb75d9af..a089f930694 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -176,7 +176,8 @@ def _create(self, *args, **kwargs) -> V2: def __call__(self, *args, **kwargs) -> V2: device = self.factory(*args, **kwargs) - device.set_name(self.name) + if self.use_factory_name: + device.set_name(self.name) return device def __repr__(self) -> str: From 622cca5010e9964226abf6081b7a5df64ea5a0b0 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 13:51:19 +0100 Subject: [PATCH 37/74] Add TODOs to remove v1 support --- src/dodal/device_manager.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index a089f930694..3184eefdddc 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -172,6 +172,7 @@ def build( return device # type: ignore - it's us, honest def _create(self, *args, **kwargs) -> V2: + # TODO: Remove when v1 support is no longer required return self(*args, **kwargs) def __call__(self, *args, **kwargs) -> V2: @@ -187,6 +188,7 @@ def __repr__(self) -> str: return f"<{self.name}: DeviceFactory ({params}) -> {target}>" +# TODO: Remove when ophyd v1 support is no longer required class V1DeviceFactory(Generic[V1]): """ Wrapper around an ophyd v1 device that holds a reference to a device @@ -327,6 +329,8 @@ def connect(self, timeout: float | None = None) -> ConnectionResult: loop: asyncio.EventLoop = get_bluesky_event_loop() # type: ignore for name, (device, mock, dev_timeout) in self.devices.items(): if not isinstance(device, OphydV2Device): + # TODO: Remove when ophyd v1 support is no longer required + # V1 devices are connected at creation time assuming wait is not set to False connected[name] = device continue timeout = timeout or dev_timeout or DEFAULT_TIMEOUT @@ -380,6 +384,13 @@ def v1_init( skip: SkipType = False, wait: bool = False, ): + """ + Register an ophyd v1 device + + The function this decorates is an initialiser that takes a built device + and is not used to create the device. + """ + def decorator(init: OphydInitialiser[V1]): name = init.__name__ if name in self: @@ -511,6 +522,7 @@ def build_devices( } try: if isinstance(factory, V1DeviceFactory): + # TODO: Remove when ophyd v1 support is no longer required factory = factory.mock_if_needed(mock) built_device = factory._create(**params) built[device] = ConnectionSpec( From 75263da9ddefc85bef13f3bba40aeaa354bd2ec6 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 14:08:44 +0100 Subject: [PATCH 38/74] Make v1 device timeout configurable --- src/dodal/device_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 3184eefdddc..1801ea0596a 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -401,7 +401,7 @@ def decorator(init: OphydInitialiser[V1]): mock=mock, skip=skip, wait=wait, - timeout=DEFAULT_TIMEOUT, + timeout=timeout, init=init, manager=self, ) From a033594a86ec4d61f8815764d40d794c06f904f7 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 14:09:43 +0100 Subject: [PATCH 39/74] Default to waiting for v1 device connection --- src/dodal/device_manager.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 1801ea0596a..0b447938995 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -382,7 +382,8 @@ def v1_init( prefix: str, mock: bool = False, skip: SkipType = False, - wait: bool = False, + wait: bool = True, + timeout: int = DEFAULT_TIMEOUT, ): """ Register an ophyd v1 device From 3837ca6fcc144ad81c095e1a36400e76358f498f Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 23 Oct 2025 14:17:29 +0100 Subject: [PATCH 40/74] Add repr to v1 device factory --- src/dodal/device_manager.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 0b447938995..93f2ea24b21 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -289,6 +289,9 @@ def build(self, mock: bool = False, fixtures: dict[str, Any] | None = None) -> V device = devices.devices[self.name].device return device # type: ignore - it's us really, promise + def __repr__(self) -> str: + return f"<{self.name}: V1DeviceFactory({self.factory.__name__})>" + class ConnectionSpec(NamedTuple): """A device paired with the options used to configure it""" From cd4b87659a5aaba6d6b1c4dfac14173ef74407d5 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 24 Oct 2025 14:53:13 +0100 Subject: [PATCH 41/74] Split DeviceBuildResult devices and connection specs Makes access to the devices easier and makes it easier to build parameters --- src/dodal/device_manager.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 93f2ea24b21..1cff58d0a6b 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -166,7 +166,7 @@ def build( ).or_raise() if connect_immediately: devices.connect(timeout=timeout or self.timeout).or_raise() - device = devices.devices[self.name].device + device = devices.devices[self.name] if name: device.set_name(name) return device # type: ignore - it's us, honest @@ -286,7 +286,7 @@ def build(self, mock: bool = False, fixtures: dict[str, Any] | None = None) -> V mock=mock, ).or_raise() - device = devices.devices[self.name].device + device = devices.devices[self.name] return device # type: ignore - it's us really, promise def __repr__(self) -> str: @@ -294,9 +294,8 @@ def __repr__(self) -> str: class ConnectionSpec(NamedTuple): - """A device paired with the options used to configure it""" + """The options used to configure a device""" - device: OphydV2Device mock: bool timeout: float @@ -322,20 +321,22 @@ def or_raise(self) -> dict[str, Any]: class DeviceBuildResult(NamedTuple): """Wrapper around the results of building devices""" - devices: dict[str, ConnectionSpec] + devices: dict[str, AnyDevice] errors: dict[str, Exception] + connection_specs: dict[str, ConnectionSpec] def connect(self, timeout: float | None = None) -> ConnectionResult: """Connect all devices that didn't fail to build""" connections = {} connected = {} loop: asyncio.EventLoop = get_bluesky_event_loop() # type: ignore - for name, (device, mock, dev_timeout) in self.devices.items(): + for name, device in self.devices.items(): if not isinstance(device, OphydV2Device): # TODO: Remove when ophyd v1 support is no longer required # V1 devices are connected at creation time assuming wait is not set to False connected[name] = device continue + mock, dev_timeout = self.connection_specs[name] timeout = timeout or dev_timeout or DEFAULT_TIMEOUT fut = asyncio.run_coroutine_threadsafe( device.connect(mock=mock, timeout=timeout), @@ -347,7 +348,7 @@ def connect(self, timeout: float | None = None) -> ConnectionResult: for name, connection_future in connections.items(): try: connection_future.result() - connected[name] = self.devices[name].device + connected[name] = self.devices[name] except Exception as e: connection_errors[name] = e @@ -506,7 +507,8 @@ def build_devices( order = self._build_order( {dep: self[dep] for dep in build_list}, fixtures=fixtures ) - built: dict[str, ConnectionSpec] = {} + built: dict[str, AnyDevice] = {} + connection_specs: dict[str, ConnectionSpec] = {} errors = {} for device in order: factory = self[device] @@ -529,15 +531,15 @@ def build_devices( # TODO: Remove when ophyd v1 support is no longer required factory = factory.mock_if_needed(mock) built_device = factory._create(**params) - built[device] = ConnectionSpec( - built_device, + built[device] = built_device + connection_specs[device] = ConnectionSpec( mock=mock or factory.mock, timeout=factory.timeout, ) except Exception as e: errors[device] = e - return DeviceBuildResult(built, errors) + return DeviceBuildResult(built, errors, connection_specs) def __contains__(self, name): return name in self._factories or name in self._v1_factories From ba075e168b6eed6818321eae4cfeb11dc03de752 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 24 Oct 2025 16:52:39 +0100 Subject: [PATCH 42/74] Remove device_type property from factories --- src/dodal/device_manager.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 1cff58d0a6b..e78df6d11c8 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -122,10 +122,6 @@ def name(self) -> str: """Name of the underlying factory function""" return self.factory.__name__ - @property - def device_type(self) -> type[V2]: - return inspect.signature(self.factory).return_annotation - @cached_property def dependencies(self) -> set[str]: """Names of all parameters""" @@ -224,10 +220,6 @@ def name(self) -> str: """Name of the underlying factory function""" return self.post_create.__name__ - @property - def device_type(self) -> type[V1]: - return self.factory - @cached_property def dependencies(self) -> set[str]: """Names of all parameters""" From 161285320c72b8d8a62fc7e1fea126c743fd1f66 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 24 Oct 2025 16:53:09 +0100 Subject: [PATCH 43/74] Include fixture overrides in built devices --- src/dodal/device_manager.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index e78df6d11c8..18dfe7636f1 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -471,8 +471,6 @@ def build_all( for f in (self._factories | self._v1_factories).values() # allow overriding skip but still allow fixtures to override devices if (include_skipped or not f.skip) - # don't build anything that has been overridden by a fixture - and (not fixtures or f.name not in fixtures) ), fixtures=fixtures, mock=mock, @@ -491,15 +489,14 @@ def build_devices( fixtures = LazyFixtures(provided=fixtures, factories=self._fixtures) if common := fixtures.keys() & {f.name for f in factories}: - warnings.warn( - f"Factories ({common}) will be overridden by fixtures", stacklevel=1 - ) factories = tuple(f for f in factories if f.name not in common) build_list = self._expand_dependencies(factories, fixtures) order = self._build_order( {dep: self[dep] for dep in build_list}, fixtures=fixtures ) - built: dict[str, AnyDevice] = {} + built: dict[str, AnyDevice] = { + override: fixtures[override] for override in common + } connection_specs: dict[str, ConnectionSpec] = {} errors = {} for device in order: From 79f8bdfa69d1fe114bc504f770767e13c2836560 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 24 Oct 2025 16:53:39 +0100 Subject: [PATCH 44/74] Fix duplication in factory repr --- src/dodal/device_manager.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 18dfe7636f1..3c1bc1757d3 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -178,10 +178,8 @@ def __call__(self, *args, **kwargs) -> V2: return device def __repr__(self) -> str: - target = self.factory.__annotations__.get("return") - target = target.__name__ if target else "???" params = inspect.signature(self.factory) - return f"<{self.name}: DeviceFactory ({params}) -> {target}>" + return f"<{self.name}: DeviceFactory{params}>" # TODO: Remove when ophyd v1 support is no longer required From 6a81076a7f2536f6badb2de4831ecbdb9022df5d Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 24 Oct 2025 16:54:03 +0100 Subject: [PATCH 45/74] Add initial device_manager tests --- tests/test_device_manager.py | 452 +++++++++++++++++++++++++++++++++++ 1 file changed, 452 insertions(+) create mode 100644 tests/test_device_manager.py diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py new file mode 100644 index 00000000000..665337abe6e --- /dev/null +++ b/tests/test_device_manager.py @@ -0,0 +1,452 @@ +from ophyd_async.sim import SimMotor +import pytest +from pytest import RaisesExc, RaisesGroup +from unittest.mock import Mock + +from ophyd_async.core import Device as OphydV2Device + +from dodal.device_manager import DeviceManager + + +def test_single_factory(): + dm = DeviceManager() + + s1 = Mock() + + @dm.factory + def sim() -> OphydV2Device: + return s1() + + devices = dm.build_all() + s1.assert_called_once() + assert devices.devices == {"sim": s1()} + + +def test_two_devices(): + dm = DeviceManager() + + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(): + return s1() + + @dm.factory + def bar(): + return s2() + + devices = dm.build_all() + s1.assert_called_once() + s2.assert_called_once() + assert devices.devices == {"foo": s1(), "bar": s2()} + + +def test_build_one_of_two(): + dm = DeviceManager() + + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(): + return s1() + + @dm.factory + def bar(): + return s2() + + devices = dm.build_devices(foo) + s1.assert_called_once() + s2.assert_not_called() + assert devices.devices == {"foo": s1()} + + +def test_build_directly(): + dm = DeviceManager() + + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(): + return s1() + + @dm.factory + def bar(): + return s2() + + f = foo() + s1.assert_called_once() + s2.assert_not_called() + assert f == s1() + + +def test_build_method(): + dm = DeviceManager() + + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(): + return s1() + + @dm.factory + def bar(): + return s2() + + f = foo.build() + s1.assert_called_once() + s2.assert_not_called() + assert f == s1() + + +def test_dependent_devices(): + dm = DeviceManager() + + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(bar): + return s1(bar) + + @dm.factory + def bar(): + return s2() + + devices = dm.build_all() + s2.assert_called_once() + s1.assert_called_once_with(s2()) + + assert devices.devices == {"foo": s1(s2()), "bar": s2()} + + +def test_dependent_device_build_method(): + dm = DeviceManager() + + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(bar): + return s1(bar) + + @dm.factory + def bar(): + return s2() + + f = foo.build() + s2.assert_called_once() + s1.assert_called_once_with(s2()) + + assert f == s1(s2()) + + +def test_fixture_argument(): + dm = DeviceManager() + + s1 = Mock() + + @dm.factory + def foo(bar): + return s1(bar) + + dm.build_all(fixtures={"bar": 123}) + s1.assert_called_once_with(123) + + +def test_none_fixture(): + dm = DeviceManager() + + s1 = Mock() + + @dm.factory + def foo(bar): + return s1(bar) + + devices = dm.build_all(fixtures={"bar": None}) + s1.assert_called_once_with(None) + assert devices.devices == {"foo": s1(None)} + + +def test_default_argument(): + dm = DeviceManager() + + s1 = Mock() + + @dm.factory + def foo(bar=42): + return s1(bar) + + dm.build_all() + s1.assert_called_once_with(42) + + +def test_fixture_overrides_default_argument(): + dm = DeviceManager() + + s1 = Mock() + + @dm.factory + def foo(bar=42): + return s1(bar) + + dm.build_all(fixtures={"bar": 17}) + s1.assert_called_once_with(17) + + +def test_fixture_overrides_factory(): + dm = DeviceManager() + + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(bar): + return s1(bar) + + @dm.factory + def bar(): + return s2() + + devices = dm.build_all(fixtures={"bar": 17}) + s1.assert_called_once_with(17) + s2.assert_not_called() + + assert devices.devices == {"foo": s1(17), "bar": 17} + + +def test_fixture_function(): + dm = DeviceManager() + + s1 = Mock() + + @dm.factory + def foo(bar=42): + return s1(bar) + + @dm.fixture + def bar(): + return 42 + + devices = dm.build_all() + s1.assert_called_once_with(42) + assert devices.devices == {"foo": s1(42)} + + +def test_fixture_functions_are_lazy(): + dm = DeviceManager() + + s1 = Mock() + fix = Mock() + fix.__name__ = "fix" + dm.fixture(fix) + + @dm.factory + def foo(): + return s1() + + devices = dm.build_all() + s1.assert_called_once() + fix.assert_not_called() + assert devices.devices == {"foo": s1()} + + +def test_duplicate_factories_error(): + dm = DeviceManager() + + s1 = Mock() + + @dm.factory + def foo(): + return s1() + + with pytest.raises(ValueError): + + @dm.factory + def foo(): + return s1() + + +def test_missing_dependency_errors(): + dm = DeviceManager() + s1 = Mock() + + @dm.factory + def foo(bar): + return s1() + + with pytest.raises(ValueError, match="Missing fixture or factory for bar"): + dm.build_all() + + +def test_missing_fixture_ok_if_not_required(): + dm = DeviceManager() + + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(unknown): + return s1(bar) + + @dm.factory + def bar(): + return s2() + + # missing unknown but not needed for bar so ok + devices = dm.build_devices(bar) + + s1.assert_not_called() + assert devices.devices == {"bar": s2()} + + +def test_circular_dependencies_error(): + dm = DeviceManager() + + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(bar): + return s1(bar) + + @dm.factory + def bar(foo): + return s2(foo) + + with pytest.raises(ValueError, match="circular dependencies"): + dm.build_all() + + +# chasing coverage numbers +def test_repr(): + dm = DeviceManager() + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(bar: int) -> SimMotor: + return s1(bar) + + @dm.factory + def bar(foo): + return s2(foo) + + assert repr(dm) == "" + assert ( + repr(foo) == " ophyd_async.sim._motor.SimMotor>" + ) + + +def test_build_errors_are_caught(): + dm = DeviceManager() + s1 = Mock() + err = RuntimeError("Build failed") + s1.side_effect = err + + @dm.factory + def foo(): + return s1() + + devices = dm.build_all() + + s1.assert_called_once() + assert devices.devices == {} + assert devices.errors == {"foo": err} + + +def test_dependency_errors_propagate(): + dm = DeviceManager() + + s1 = Mock() + err = RuntimeError("Build failed") + s2 = Mock() + s2.side_effect = err + + @dm.factory + def foo(bar): + return s1(bar) + + @dm.factory + def bar(): + return s2() + + devices = dm.build_all() + s2.assert_called_once() + s1.assert_not_called() + + assert devices.devices == {} + assert devices.errors["bar"] == err + foo_err = devices.errors["foo"] + assert isinstance(foo_err, ValueError) + assert foo_err.args == ("Errors building dependencies: {'bar'}",) + + +def test_devices_are_named(): + dm = DeviceManager() + s1 = Mock() + + @dm.factory + def foo(): + return s1() + + f = foo.build() + + s1.assert_called_once() + f.set_name.assert_called_once_with("foo") + + +def test_skip_naming(): + dm = DeviceManager() + s1 = Mock() + + @dm.factory(use_factory_name=False) + def foo(): + return s1() + + f = foo.build() + + s1.assert_called_once() + f.set_name.assert_not_called() + + +def test_override_name(): + dm = DeviceManager() + s1 = Mock() + + @dm.factory + def foo(): + return s1() + + f = foo.build(name="not_foo") + + s1.assert_called_once() + f.set_name.assert_called_with("not_foo") + + +def test_positional_only_args_error(): + dm = DeviceManager() + s1 = Mock() + + with pytest.raises(ValueError, match="positional only arguments"): + + @dm.factory + def foo(foo, /): + return s1() + + +def test_devices_or_raise(): + dm = DeviceManager() + s1 = Mock() + s1.side_effect = RuntimeError("Build failed") + + @dm.factory + def foo(): + return s1() + + devices = dm.build_all() + with RaisesGroup(RaisesExc(RuntimeError, match="Build failed")): + devices.or_raise() From 3175300046696b300421bad618601f18d15e0333 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Mon, 27 Oct 2025 09:13:17 +0000 Subject: [PATCH 46/74] Revert adsim changes for now --- src/dodal/beamlines/adsim.py | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/src/dodal/beamlines/adsim.py b/src/dodal/beamlines/adsim.py index 7e11854a6ad..5ec56286583 100644 --- a/src/dodal/beamlines/adsim.py +++ b/src/dodal/beamlines/adsim.py @@ -1,9 +1,9 @@ -from pathlib import PurePath -from ophyd.epics_motor import EpicsMotor -from ophyd_async.core import PathProvider, StaticPathProvider, UUIDFilenameProvider from ophyd_async.epics.adsimdetector import SimDetector -from dodal.device_manager import DeviceManager +from dodal.common.beamlines.beamline_utils import ( + device_factory, + get_path_provider, +) from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.common.beamlines.device_helpers import DET_SUFFIX, HDF5_SUFFIX from dodal.devices.motors import XThetaStage @@ -63,32 +63,19 @@ """ -devices = DeviceManager() - - -@devices.fixture -def path_provider() -> PathProvider: - return StaticPathProvider(UUIDFilenameProvider(), PurePath("/")) - -@devices.factory(timeout=2, mock=False) +@device_factory() def stage() -> XThetaStage: return XThetaStage( f"{PREFIX.beamline_prefix}-MO-SIMC-01:", x_infix="M1", theta_infix="M2" ) -@devices.factory(timeout=2) -def det(path_provider: PathProvider) -> SimDetector: +@device_factory() +def det() -> SimDetector: return SimDetector( f"{PREFIX.beamline_prefix}-DI-CAM-01:", - path_provider=path_provider, + path_provider=get_path_provider(), drv_suffix=DET_SUFFIX, fileio_suffix=HDF5_SUFFIX, ) - - -@devices.v1_init(factory=EpicsMotor, prefix=f"{PREFIX.beamline_prefix}-MO-SIMC-01:M1") -def old_motor(motor: EpicsMotor): - # arbitrary post-init configuration - motor.settle_time = 12 From 15d7dc1a33a92d8cd1fd715201f39ba5df73427a Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Mon, 27 Oct 2025 18:09:17 +0000 Subject: [PATCH 47/74] Enough tests to get full coverage Maybe enough tests to cover basic use --- src/dodal/device_manager.py | 12 +- tests/test_device_manager.py | 423 ++++++++++++++++++++++++++++------- 2 files changed, 348 insertions(+), 87 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 3c1bc1757d3..e47001c4417 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -199,7 +199,7 @@ def __init__( skip: SkipType, wait: bool, timeout: int, - init: OphydInitialiser[V1] | None, + init: OphydInitialiser[V1], manager: "DeviceManager", ): self.factory = factory @@ -210,8 +210,7 @@ def __init__( self.timeout = timeout self.post_create = init or (lambda x: x) self._manager = manager - if init: - wraps(init)(self) + wraps(init)(self) @property def name(self) -> str: @@ -280,7 +279,7 @@ def build(self, mock: bool = False, fixtures: dict[str, Any] | None = None) -> V return device # type: ignore - it's us really, promise def __repr__(self) -> str: - return f"<{self.name}: V1DeviceFactory({self.factory.__name__})>" + return f"<{self.name}: V1DeviceFactory[{self.factory.__name__}]>" class ConnectionSpec(NamedTuple): @@ -607,5 +606,8 @@ def _build_order( return order + def __len__(self) -> int: + return len(self._factories) + len(self._v1_factories) + def __repr__(self) -> str: - return f"" + return f"" diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py index 665337abe6e..6f841a80275 100644 --- a/tests/test_device_manager.py +++ b/tests/test_device_manager.py @@ -1,11 +1,13 @@ +from dodal import device_manager from ophyd_async.sim import SimMotor import pytest from pytest import RaisesExc, RaisesGroup -from unittest.mock import Mock +from unittest.mock import MagicMock, Mock, patch +from ophyd.device import Device as OphydV1Device from ophyd_async.core import Device as OphydV2Device -from dodal.device_manager import DeviceManager +from dodal.device_manager import DEFAULT_TIMEOUT, DeviceBuildResult, DeviceManager, LazyFixtures def test_single_factory(): @@ -14,8 +16,7 @@ def test_single_factory(): s1 = Mock() @dm.factory - def sim() -> OphydV2Device: - return s1() + def sim() -> OphydV2Device: return s1() devices = dm.build_all() s1.assert_called_once() @@ -29,12 +30,10 @@ def test_two_devices(): s2 = Mock() @dm.factory - def foo(): - return s1() + def foo(): return s1() @dm.factory - def bar(): - return s2() + def bar(): return s2() devices = dm.build_all() s1.assert_called_once() @@ -49,12 +48,10 @@ def test_build_one_of_two(): s2 = Mock() @dm.factory - def foo(): - return s1() + def foo(): return s1() @dm.factory - def bar(): - return s2() + def bar(): return s2() devices = dm.build_devices(foo) s1.assert_called_once() @@ -69,12 +66,10 @@ def test_build_directly(): s2 = Mock() @dm.factory - def foo(): - return s1() + def foo(): return s1() @dm.factory - def bar(): - return s2() + def bar(): return s2() f = foo() s1.assert_called_once() @@ -89,12 +84,10 @@ def test_build_method(): s2 = Mock() @dm.factory - def foo(): - return s1() + def foo(): return s1() @dm.factory - def bar(): - return s2() + def bar(): return s2() f = foo.build() s1.assert_called_once() @@ -109,12 +102,10 @@ def test_dependent_devices(): s2 = Mock() @dm.factory - def foo(bar): - return s1(bar) + def foo(bar): return s1(bar) @dm.factory - def bar(): - return s2() + def bar(): return s2() devices = dm.build_all() s2.assert_called_once() @@ -130,12 +121,10 @@ def test_dependent_device_build_method(): s2 = Mock() @dm.factory - def foo(bar): - return s1(bar) + def foo(bar): return s1(bar) @dm.factory - def bar(): - return s2() + def bar(): return s2() f = foo.build() s2.assert_called_once() @@ -150,8 +139,7 @@ def test_fixture_argument(): s1 = Mock() @dm.factory - def foo(bar): - return s1(bar) + def foo(bar): return s1(bar) dm.build_all(fixtures={"bar": 123}) s1.assert_called_once_with(123) @@ -163,8 +151,7 @@ def test_none_fixture(): s1 = Mock() @dm.factory - def foo(bar): - return s1(bar) + def foo(bar): return s1(bar) devices = dm.build_all(fixtures={"bar": None}) s1.assert_called_once_with(None) @@ -177,8 +164,7 @@ def test_default_argument(): s1 = Mock() @dm.factory - def foo(bar=42): - return s1(bar) + def foo(bar=42): return s1(bar) dm.build_all() s1.assert_called_once_with(42) @@ -190,8 +176,7 @@ def test_fixture_overrides_default_argument(): s1 = Mock() @dm.factory - def foo(bar=42): - return s1(bar) + def foo(bar=42): return s1(bar) dm.build_all(fixtures={"bar": 17}) s1.assert_called_once_with(17) @@ -204,12 +189,10 @@ def test_fixture_overrides_factory(): s2 = Mock() @dm.factory - def foo(bar): - return s1(bar) + def foo(bar): return s1(bar) @dm.factory - def bar(): - return s2() + def bar(): return s2() devices = dm.build_all(fixtures={"bar": 17}) s1.assert_called_once_with(17) @@ -224,12 +207,10 @@ def test_fixture_function(): s1 = Mock() @dm.factory - def foo(bar=42): - return s1(bar) + def foo(bar=42): return s1(bar) @dm.fixture - def bar(): - return 42 + def bar(): return 42 devices = dm.build_all() s1.assert_called_once_with(42) @@ -245,8 +226,7 @@ def test_fixture_functions_are_lazy(): dm.fixture(fix) @dm.factory - def foo(): - return s1() + def foo(): return s1() devices = dm.build_all() s1.assert_called_once() @@ -260,14 +240,12 @@ def test_duplicate_factories_error(): s1 = Mock() @dm.factory - def foo(): - return s1() + def foo(): return s1() with pytest.raises(ValueError): @dm.factory - def foo(): - return s1() + def foo(): return s1() def test_missing_dependency_errors(): @@ -275,8 +253,7 @@ def test_missing_dependency_errors(): s1 = Mock() @dm.factory - def foo(bar): - return s1() + def foo(bar): return s1() with pytest.raises(ValueError, match="Missing fixture or factory for bar"): dm.build_all() @@ -289,12 +266,10 @@ def test_missing_fixture_ok_if_not_required(): s2 = Mock() @dm.factory - def foo(unknown): - return s1(bar) + def foo(unknown): return s1(bar) @dm.factory - def bar(): - return s2() + def bar(): return s2() # missing unknown but not needed for bar so ok devices = dm.build_devices(bar) @@ -310,12 +285,10 @@ def test_circular_dependencies_error(): s2 = Mock() @dm.factory - def foo(bar): - return s1(bar) + def foo(bar): return s1(bar) @dm.factory - def bar(foo): - return s2(foo) + def bar(foo): return s2(foo) with pytest.raises(ValueError, match="circular dependencies"): dm.build_all() @@ -326,19 +299,19 @@ def test_repr(): dm = DeviceManager() s1 = Mock() s2 = Mock() + s2.__name__ = "S2" @dm.factory - def foo(bar: int) -> SimMotor: - return s1(bar) + def foo(one: int) -> SimMotor: return s1(one) - @dm.factory - def bar(foo): - return s2(foo) + @dm.v1_init(s2, prefix="S2_PREFIX") + def bar(_): pass assert repr(dm) == "" assert ( - repr(foo) == " ophyd_async.sim._motor.SimMotor>" + repr(foo) == " ophyd_async.sim._motor.SimMotor>" ) + assert repr(bar) == "" def test_build_errors_are_caught(): @@ -348,8 +321,7 @@ def test_build_errors_are_caught(): s1.side_effect = err @dm.factory - def foo(): - return s1() + def foo(): return s1() devices = dm.build_all() @@ -367,12 +339,10 @@ def test_dependency_errors_propagate(): s2.side_effect = err @dm.factory - def foo(bar): - return s1(bar) + def foo(bar): return s1(bar) @dm.factory - def bar(): - return s2() + def bar(): return s2() devices = dm.build_all() s2.assert_called_once() @@ -390,8 +360,7 @@ def test_devices_are_named(): s1 = Mock() @dm.factory - def foo(): - return s1() + def foo(): return s1() f = foo.build() @@ -404,8 +373,7 @@ def test_skip_naming(): s1 = Mock() @dm.factory(use_factory_name=False) - def foo(): - return s1() + def foo(): return s1() f = foo.build() @@ -418,8 +386,7 @@ def test_override_name(): s1 = Mock() @dm.factory - def foo(): - return s1() + def foo(): return s1() f = foo.build(name="not_foo") @@ -434,8 +401,7 @@ def test_positional_only_args_error(): with pytest.raises(ValueError, match="positional only arguments"): @dm.factory - def foo(foo, /): - return s1() + def foo(foo, /): return s1() def test_devices_or_raise(): @@ -444,9 +410,302 @@ def test_devices_or_raise(): s1.side_effect = RuntimeError("Build failed") @dm.factory - def foo(): - return s1() + def foo(): return s1() devices = dm.build_all() with RaisesGroup(RaisesExc(RuntimeError, match="Build failed")): devices.or_raise() + +def test_connect(): + dm = DeviceManager() + + s1 = Mock() + s1.return_value = Mock(spec=OphydV2Device) + + @dm.factory + def foo(): return s1() + + con = dm.build_devices(foo).connect() + s1.assert_called_once() + s1().connect.assert_called_once_with(timeout=DEFAULT_TIMEOUT, mock=False) + + assert con.devices == {"foo": s1()} + assert con.build_errors == {} + assert con.connection_errors == {} + +def test_build_and_connect(): + dm = DeviceManager() + + s1 = Mock() + s1.return_value = Mock(spec=OphydV2Device) + + @dm.factory + def foo(): return s1() + + con = dm.build_and_connect() + s1.assert_called_once() + s1().connect.assert_called_once_with(timeout=DEFAULT_TIMEOUT, mock=False) + + assert con.devices == {"foo": s1()} + assert con.build_errors == {} + assert con.connection_errors == {} + + +def test_factory_options(): + dm = DeviceManager() + s1 = Mock() + s1.return_value = Mock(spec=OphydV2Device) + @dm.factory(mock=True, timeout=12) + def foo(): return s1() + con = dm.build_devices(foo).connect() + s1().connect.assert_called_once_with(mock=True, timeout=12) + assert con.connection_errors == {} + assert con.build_errors == {} + +def test_connect_failures(): + dm = DeviceManager() + s1 = Mock() + s1.return_value = Mock(spec=OphydV2Device) + err = ValueError("Not connected") + s1.return_value.connect.side_effect = err + @dm.factory + def foo(): return s1() + con = dm.build_devices(foo).connect() + s1().connect.assert_called_once_with(mock=False, timeout=DEFAULT_TIMEOUT) + assert con.connection_errors == {"foo": err} + assert con.build_errors == {} + +def test_connect_or_raise_without_errors(): + dm = DeviceManager() + + s1 = Mock() + s1.return_value = Mock(spec=OphydV2Device) + + @dm.factory + def foo(): return s1() + + con = dm.build_devices(foo).connect().or_raise() + s1.assert_called_once() + s1().connect.assert_called_once_with(timeout=DEFAULT_TIMEOUT, mock=False) + + assert con == {"foo": s1()} + +def test_connect_or_raise_with_build_errors(): + dm = DeviceManager() + + s1 = Mock() + s1.return_value = Mock(spec=OphydV2Device) + + @dm.factory + def foo(): raise ValueError("foo error") + + with RaisesGroup(RaisesExc(ValueError, match="foo error")): + dm.build_devices(foo).connect().or_raise() + + +def test_connect_or_raise_with_connect_errors(): + dm = DeviceManager() + + s1 = Mock() + s1.return_value = Mock(spec=OphydV2Device) + s1.return_value.connect.side_effect = ValueError("foo connection") + + @dm.factory + def foo(): return s1() + + with RaisesGroup(RaisesExc(ValueError, match="foo connection")): + dm.build_devices(foo).connect().or_raise() + +def test_build_and_connect_immediately(): + dm = DeviceManager() + s1 = Mock() + s1.return_value = Mock(spec=OphydV2Device) + @dm.factory + def foo(): return s1() + f = foo.build(connect_immediately=True) + s1.assert_called_once() + s1().connect.assert_called_with(mock=False, timeout=DEFAULT_TIMEOUT) + assert f is s1() + +def test_skip_factory(): + dm = DeviceManager() + + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(): return s1() + + @dm.factory(skip=True) + def bar(): return s2() + + devices = dm.build_all() + s1.assert_called_once() + s2.assert_not_called() + assert devices.devices == {"foo": s1()} + +def test_skip_ignored_if_required(): + dm = DeviceManager() + + s1 = Mock() + s2 = Mock() + + @dm.factory + def foo(bar): return s1(bar) + + @dm.factory(skip=True) + def bar(): return s2() + + devices = dm.build_all() + s2.assert_called_once() + s1.assert_called_once_with(s2()) + assert devices.devices == {"foo": s1(), "bar": s2()} + +def test_mock_all(): + dm = DeviceManager() + s1 = Mock() + s1.return_value = Mock(spec=OphydV2Device) + @dm.factory + def foo(): return s1() + + dm.build_all(mock=True).connect(timeout=11) + s1().connect.assert_called_once_with(mock=True, timeout=11) + + +def test_v1_device_factory(): + dm = DeviceManager() + s1 = MagicMock(spec=OphydV1Device) # type: ignore + s1.__name__ = "S1" + + @dm.v1_init(s1, prefix="S1_PREFIX") + def foo(_): pass + + with patch("dodal.device_manager.wait_for_connection") as wfc: + devices = dm.build_all() + + s1.assert_called_once_with(name="foo", prefix="S1_PREFIX") + device = s1(name="foo", prefix="S1_PREFIX") + assert devices.devices["foo"] is device + wfc.assert_called_once_with(device, timeout=DEFAULT_TIMEOUT) + +def test_v1_v2_name_clash(): + dm = DeviceManager() + s1 = Mock() + s2 = MagicMock() + + @dm.factory + def foo(): return s1() + + with pytest.raises(ValueError, match="name"): + @dm.v1_init(s2, prefix="S2_PREFIX") + def foo(_): + pass + +def test_v1_decorator_is_transparent(): + dm = DeviceManager() + s1 = MagicMock() + + @dm.v1_init(s1, prefix="S1_PREFIX") + def foo(s): + # arbitrary setup method + s.special_init_method() + + dev = Mock() + foo(dev) + + dev.special_init_method.assert_called_once() + s1.assert_not_called() + +def test_v1_no_wait(): + dm = DeviceManager() + s1 = Mock() + @dm.v1_init(s1, prefix="S1_PREFIX", wait=False) + def foo(_): + pass + with patch("dodal.device_manager.wait_for_connection") as wfc: + foo.build() + wfc.assert_not_called() + + +def test_connect_ignores_v1(): + v1 = Mock(spec=OphydV1Device) + dbr = DeviceBuildResult({"foo": v1}, {}, {}) + con = dbr.connect() + # mock raises exception if connect is called + assert con.devices == {"foo": v1} + +def test_v1_mocking(): + s1 = Mock() + s1.return_value = Mock(spec=OphydV1Device) + dm = DeviceManager() + @dm.v1_init(s1, prefix="S1_PREFIX", mock=True) + def foo(_): pass + with patch("dodal.device_manager.make_fake_device") as mfd: + dm.build_all() + mfd.assert_called_once_with(s1) + +def test_v1_init_params(): + # values are passed from fixtures + dm = DeviceManager() + s1 = Mock() + s1.return_value = Mock(spec=OphydV1Device) + s1.return_value.mock_add_spec(["set_up_with"]) + @dm.fixture + def one(): return "one" + @dm.v1_init(s1, prefix="S1_PREFIX", wait=False) + def foo(s, one, two): + s.set_up_with(one, two) + dev = dm.build_devices(foo, fixtures={"two": 2}) + s1.assert_called_once_with(name="foo", prefix="S1_PREFIX") + s1().set_up_with.assert_called_once_with("one", 2) + + +def test_lazy_fixtures_non_lazy(): + lf = LazyFixtures(provided={"foo": "bar"}, factories={}) + assert lf["foo"] == "bar" + +def test_lazy_fixtures_lazy(): + buzz = Mock() + buzz.return_value = "buzz" + lf = LazyFixtures(provided={}, factories={"fizz": buzz}) + + assert lf.get("foo") is None + buzz.assert_not_called() + + assert lf.get("fizz") == "buzz" + buzz.assert_called_once() + +def test_lazy_fixtures_provided_overrides_factory(): + buzz = Mock() + buzz.return_value = "buzz" + + lf = LazyFixtures(provided={"fizz": "foo"}, factories={"fizz": buzz}) + + assert lf["fizz"] == "foo" + buzz.assert_not_called() + +def test_lazy_fixtures_deduplicated(): + buzz = Mock() + buzz.return_value = "buzz" + + lf = LazyFixtures(provided={"fizz": "foo"}, factories={"fizz": buzz}) + + assert len(lf) == 1 + +def test_lazy_fixtures_iter(): + buzz = Mock() + buzz.return_value = "buzz" + + lf = LazyFixtures(provided={"foo": "bar", "one": "two"}, factories={"fizz": buzz, "one": "two"}) + + assert sorted(lf) == ["fizz", "foo", "one"] + +def test_lazy_fixtures_contains(): + buzz = Mock() + buzz.return_value = "buzz" + + lf = LazyFixtures(provided={"foo": "bar", "one": "two"}, factories={"fizz": buzz, "one": "two"}) + + assert "foo" in lf + assert "fizz" in lf + assert "two" not in lf From 28724b42050928ebbb8a30ae1079dd38d8f58020 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Mon, 27 Oct 2025 18:14:37 +0000 Subject: [PATCH 48/74] Create DeviceManager in fixture --- tests/test_device_manager.py | 157 ++++++++++------------------------- 1 file changed, 46 insertions(+), 111 deletions(-) diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py index 6f841a80275..ab740cbaef9 100644 --- a/tests/test_device_manager.py +++ b/tests/test_device_manager.py @@ -9,10 +9,11 @@ from dodal.device_manager import DEFAULT_TIMEOUT, DeviceBuildResult, DeviceManager, LazyFixtures +@pytest.fixture +def dm() -> DeviceManager: + return DeviceManager() -def test_single_factory(): - dm = DeviceManager() - +def test_single_factory(dm: DeviceManager): s1 = Mock() @dm.factory @@ -23,9 +24,7 @@ def sim() -> OphydV2Device: return s1() assert devices.devices == {"sim": s1()} -def test_two_devices(): - dm = DeviceManager() - +def test_two_devices(dm: DeviceManager): s1 = Mock() s2 = Mock() @@ -41,9 +40,7 @@ def bar(): return s2() assert devices.devices == {"foo": s1(), "bar": s2()} -def test_build_one_of_two(): - dm = DeviceManager() - +def test_build_one_of_two(dm: DeviceManager): s1 = Mock() s2 = Mock() @@ -59,9 +56,7 @@ def bar(): return s2() assert devices.devices == {"foo": s1()} -def test_build_directly(): - dm = DeviceManager() - +def test_build_directly(dm: DeviceManager): s1 = Mock() s2 = Mock() @@ -77,9 +72,7 @@ def bar(): return s2() assert f == s1() -def test_build_method(): - dm = DeviceManager() - +def test_build_method(dm: DeviceManager): s1 = Mock() s2 = Mock() @@ -95,9 +88,7 @@ def bar(): return s2() assert f == s1() -def test_dependent_devices(): - dm = DeviceManager() - +def test_dependent_devices(dm: DeviceManager): s1 = Mock() s2 = Mock() @@ -114,9 +105,7 @@ def bar(): return s2() assert devices.devices == {"foo": s1(s2()), "bar": s2()} -def test_dependent_device_build_method(): - dm = DeviceManager() - +def test_dependent_device_build_method(dm: DeviceManager): s1 = Mock() s2 = Mock() @@ -133,9 +122,7 @@ def bar(): return s2() assert f == s1(s2()) -def test_fixture_argument(): - dm = DeviceManager() - +def test_fixture_argument(dm: DeviceManager): s1 = Mock() @dm.factory @@ -145,9 +132,7 @@ def foo(bar): return s1(bar) s1.assert_called_once_with(123) -def test_none_fixture(): - dm = DeviceManager() - +def test_none_fixture(dm: DeviceManager): s1 = Mock() @dm.factory @@ -158,9 +143,7 @@ def foo(bar): return s1(bar) assert devices.devices == {"foo": s1(None)} -def test_default_argument(): - dm = DeviceManager() - +def test_default_argument(dm: DeviceManager): s1 = Mock() @dm.factory @@ -170,9 +153,7 @@ def foo(bar=42): return s1(bar) s1.assert_called_once_with(42) -def test_fixture_overrides_default_argument(): - dm = DeviceManager() - +def test_fixture_overrides_default_argument(dm: DeviceManager): s1 = Mock() @dm.factory @@ -182,9 +163,7 @@ def foo(bar=42): return s1(bar) s1.assert_called_once_with(17) -def test_fixture_overrides_factory(): - dm = DeviceManager() - +def test_fixture_overrides_factory(dm: DeviceManager): s1 = Mock() s2 = Mock() @@ -201,9 +180,7 @@ def bar(): return s2() assert devices.devices == {"foo": s1(17), "bar": 17} -def test_fixture_function(): - dm = DeviceManager() - +def test_fixture_function(dm: DeviceManager): s1 = Mock() @dm.factory @@ -217,9 +194,7 @@ def bar(): return 42 assert devices.devices == {"foo": s1(42)} -def test_fixture_functions_are_lazy(): - dm = DeviceManager() - +def test_fixture_functions_are_lazy(dm: DeviceManager): s1 = Mock() fix = Mock() fix.__name__ = "fix" @@ -234,9 +209,7 @@ def foo(): return s1() assert devices.devices == {"foo": s1()} -def test_duplicate_factories_error(): - dm = DeviceManager() - +def test_duplicate_factories_error(dm: DeviceManager): s1 = Mock() @dm.factory @@ -248,8 +221,7 @@ def foo(): return s1() def foo(): return s1() -def test_missing_dependency_errors(): - dm = DeviceManager() +def test_missing_dependency_errors(dm: DeviceManager): s1 = Mock() @dm.factory @@ -259,9 +231,7 @@ def foo(bar): return s1() dm.build_all() -def test_missing_fixture_ok_if_not_required(): - dm = DeviceManager() - +def test_missing_fixture_ok_if_not_required(dm: DeviceManager): s1 = Mock() s2 = Mock() @@ -278,9 +248,7 @@ def bar(): return s2() assert devices.devices == {"bar": s2()} -def test_circular_dependencies_error(): - dm = DeviceManager() - +def test_circular_dependencies_error(dm: DeviceManager): s1 = Mock() s2 = Mock() @@ -295,8 +263,7 @@ def bar(foo): return s2(foo) # chasing coverage numbers -def test_repr(): - dm = DeviceManager() +def test_repr(dm: DeviceManager): s1 = Mock() s2 = Mock() s2.__name__ = "S2" @@ -314,8 +281,7 @@ def bar(_): pass assert repr(bar) == "" -def test_build_errors_are_caught(): - dm = DeviceManager() +def test_build_errors_are_caught(dm: DeviceManager): s1 = Mock() err = RuntimeError("Build failed") s1.side_effect = err @@ -330,9 +296,7 @@ def foo(): return s1() assert devices.errors == {"foo": err} -def test_dependency_errors_propagate(): - dm = DeviceManager() - +def test_dependency_errors_propagate(dm: DeviceManager): s1 = Mock() err = RuntimeError("Build failed") s2 = Mock() @@ -355,8 +319,7 @@ def bar(): return s2() assert foo_err.args == ("Errors building dependencies: {'bar'}",) -def test_devices_are_named(): - dm = DeviceManager() +def test_devices_are_named(dm: DeviceManager): s1 = Mock() @dm.factory @@ -368,8 +331,7 @@ def foo(): return s1() f.set_name.assert_called_once_with("foo") -def test_skip_naming(): - dm = DeviceManager() +def test_skip_naming(dm: DeviceManager): s1 = Mock() @dm.factory(use_factory_name=False) @@ -381,8 +343,7 @@ def foo(): return s1() f.set_name.assert_not_called() -def test_override_name(): - dm = DeviceManager() +def test_override_name(dm: DeviceManager): s1 = Mock() @dm.factory @@ -394,8 +355,7 @@ def foo(): return s1() f.set_name.assert_called_with("not_foo") -def test_positional_only_args_error(): - dm = DeviceManager() +def test_positional_only_args_error(dm: DeviceManager): s1 = Mock() with pytest.raises(ValueError, match="positional only arguments"): @@ -404,8 +364,7 @@ def test_positional_only_args_error(): def foo(foo, /): return s1() -def test_devices_or_raise(): - dm = DeviceManager() +def test_devices_or_raise(dm: DeviceManager): s1 = Mock() s1.side_effect = RuntimeError("Build failed") @@ -416,9 +375,7 @@ def foo(): return s1() with RaisesGroup(RaisesExc(RuntimeError, match="Build failed")): devices.or_raise() -def test_connect(): - dm = DeviceManager() - +def test_connect(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) @@ -433,9 +390,7 @@ def foo(): return s1() assert con.build_errors == {} assert con.connection_errors == {} -def test_build_and_connect(): - dm = DeviceManager() - +def test_build_and_connect(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) @@ -451,8 +406,7 @@ def foo(): return s1() assert con.connection_errors == {} -def test_factory_options(): - dm = DeviceManager() +def test_factory_options(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) @dm.factory(mock=True, timeout=12) @@ -462,8 +416,7 @@ def foo(): return s1() assert con.connection_errors == {} assert con.build_errors == {} -def test_connect_failures(): - dm = DeviceManager() +def test_connect_failures(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) err = ValueError("Not connected") @@ -475,9 +428,7 @@ def foo(): return s1() assert con.connection_errors == {"foo": err} assert con.build_errors == {} -def test_connect_or_raise_without_errors(): - dm = DeviceManager() - +def test_connect_or_raise_without_errors(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) @@ -490,9 +441,7 @@ def foo(): return s1() assert con == {"foo": s1()} -def test_connect_or_raise_with_build_errors(): - dm = DeviceManager() - +def test_connect_or_raise_with_build_errors(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) @@ -503,9 +452,7 @@ def foo(): raise ValueError("foo error") dm.build_devices(foo).connect().or_raise() -def test_connect_or_raise_with_connect_errors(): - dm = DeviceManager() - +def test_connect_or_raise_with_connect_errors(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) s1.return_value.connect.side_effect = ValueError("foo connection") @@ -516,8 +463,7 @@ def foo(): return s1() with RaisesGroup(RaisesExc(ValueError, match="foo connection")): dm.build_devices(foo).connect().or_raise() -def test_build_and_connect_immediately(): - dm = DeviceManager() +def test_build_and_connect_immediately(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) @dm.factory @@ -527,9 +473,7 @@ def foo(): return s1() s1().connect.assert_called_with(mock=False, timeout=DEFAULT_TIMEOUT) assert f is s1() -def test_skip_factory(): - dm = DeviceManager() - +def test_skip_factory(dm: DeviceManager): s1 = Mock() s2 = Mock() @@ -544,9 +488,7 @@ def bar(): return s2() s2.assert_not_called() assert devices.devices == {"foo": s1()} -def test_skip_ignored_if_required(): - dm = DeviceManager() - +def test_skip_ignored_if_required(dm: DeviceManager): s1 = Mock() s2 = Mock() @@ -561,8 +503,7 @@ def bar(): return s2() s1.assert_called_once_with(s2()) assert devices.devices == {"foo": s1(), "bar": s2()} -def test_mock_all(): - dm = DeviceManager() +def test_mock_all(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) @dm.factory @@ -572,8 +513,7 @@ def foo(): return s1() s1().connect.assert_called_once_with(mock=True, timeout=11) -def test_v1_device_factory(): - dm = DeviceManager() +def test_v1_device_factory(dm: DeviceManager): s1 = MagicMock(spec=OphydV1Device) # type: ignore s1.__name__ = "S1" @@ -588,8 +528,7 @@ def foo(_): pass assert devices.devices["foo"] is device wfc.assert_called_once_with(device, timeout=DEFAULT_TIMEOUT) -def test_v1_v2_name_clash(): - dm = DeviceManager() +def test_v1_v2_name_clash(dm: DeviceManager): s1 = Mock() s2 = MagicMock() @@ -601,8 +540,7 @@ def foo(): return s1() def foo(_): pass -def test_v1_decorator_is_transparent(): - dm = DeviceManager() +def test_v1_decorator_is_transparent(dm: DeviceManager): s1 = MagicMock() @dm.v1_init(s1, prefix="S1_PREFIX") @@ -616,8 +554,7 @@ def foo(s): dev.special_init_method.assert_called_once() s1.assert_not_called() -def test_v1_no_wait(): - dm = DeviceManager() +def test_v1_no_wait(dm: DeviceManager): s1 = Mock() @dm.v1_init(s1, prefix="S1_PREFIX", wait=False) def foo(_): @@ -634,19 +571,17 @@ def test_connect_ignores_v1(): # mock raises exception if connect is called assert con.devices == {"foo": v1} -def test_v1_mocking(): +def test_v1_mocking(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV1Device) - dm = DeviceManager() @dm.v1_init(s1, prefix="S1_PREFIX", mock=True) def foo(_): pass with patch("dodal.device_manager.make_fake_device") as mfd: dm.build_all() mfd.assert_called_once_with(s1) -def test_v1_init_params(): +def test_v1_init_params(dm: DeviceManager): # values are passed from fixtures - dm = DeviceManager() s1 = Mock() s1.return_value = Mock(spec=OphydV1Device) s1.return_value.mock_add_spec(["set_up_with"]) From 9434201b1af9face0326e5591cd9d4ba3ce26038 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 28 Oct 2025 11:27:25 +0000 Subject: [PATCH 49/74] Add test for docstrings --- tests/test_device_manager.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py index ab740cbaef9..4efff143111 100644 --- a/tests/test_device_manager.py +++ b/tests/test_device_manager.py @@ -644,3 +644,18 @@ def test_lazy_fixtures_contains(): assert "foo" in lf assert "fizz" in lf assert "two" not in lf + + +def test_docstrings_are_kept(dm: DeviceManager): + @dm.factory + def foo(): + """This is the docstring for foo""" + return Mock() + + @dm.v1_init(Mock(), prefix="MOCK_PREFIX") + def bar(_): + """This is the docstring for bar""" + pass + + assert foo.__doc__ == "This is the docstring for foo" + assert bar.__doc__ == "This is the docstring for bar" From fef69593dc73377802f95a320c5bdcd6e62c1a09 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 28 Oct 2025 11:27:41 +0000 Subject: [PATCH 50/74] Reformat tests --- tests/test_device_manager.py | 221 +++++++++++++++++++++++++---------- 1 file changed, 161 insertions(+), 60 deletions(-) diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py index 4efff143111..0853458f018 100644 --- a/tests/test_device_manager.py +++ b/tests/test_device_manager.py @@ -7,17 +7,25 @@ from ophyd.device import Device as OphydV1Device from ophyd_async.core import Device as OphydV2Device -from dodal.device_manager import DEFAULT_TIMEOUT, DeviceBuildResult, DeviceManager, LazyFixtures +from dodal.device_manager import ( + DEFAULT_TIMEOUT, + DeviceBuildResult, + DeviceManager, + LazyFixtures, +) + @pytest.fixture def dm() -> DeviceManager: return DeviceManager() + def test_single_factory(dm: DeviceManager): s1 = Mock() @dm.factory - def sim() -> OphydV2Device: return s1() + def sim() -> OphydV2Device: + return s1() devices = dm.build_all() s1.assert_called_once() @@ -29,10 +37,12 @@ def test_two_devices(dm: DeviceManager): s2 = Mock() @dm.factory - def foo(): return s1() + def foo(): + return s1() @dm.factory - def bar(): return s2() + def bar(): + return s2() devices = dm.build_all() s1.assert_called_once() @@ -45,10 +55,12 @@ def test_build_one_of_two(dm: DeviceManager): s2 = Mock() @dm.factory - def foo(): return s1() + def foo(): + return s1() @dm.factory - def bar(): return s2() + def bar(): + return s2() devices = dm.build_devices(foo) s1.assert_called_once() @@ -61,10 +73,12 @@ def test_build_directly(dm: DeviceManager): s2 = Mock() @dm.factory - def foo(): return s1() + def foo(): + return s1() @dm.factory - def bar(): return s2() + def bar(): + return s2() f = foo() s1.assert_called_once() @@ -77,10 +91,12 @@ def test_build_method(dm: DeviceManager): s2 = Mock() @dm.factory - def foo(): return s1() + def foo(): + return s1() @dm.factory - def bar(): return s2() + def bar(): + return s2() f = foo.build() s1.assert_called_once() @@ -93,10 +109,12 @@ def test_dependent_devices(dm: DeviceManager): s2 = Mock() @dm.factory - def foo(bar): return s1(bar) + def foo(bar): + return s1(bar) @dm.factory - def bar(): return s2() + def bar(): + return s2() devices = dm.build_all() s2.assert_called_once() @@ -110,10 +128,12 @@ def test_dependent_device_build_method(dm: DeviceManager): s2 = Mock() @dm.factory - def foo(bar): return s1(bar) + def foo(bar): + return s1(bar) @dm.factory - def bar(): return s2() + def bar(): + return s2() f = foo.build() s2.assert_called_once() @@ -126,7 +146,8 @@ def test_fixture_argument(dm: DeviceManager): s1 = Mock() @dm.factory - def foo(bar): return s1(bar) + def foo(bar): + return s1(bar) dm.build_all(fixtures={"bar": 123}) s1.assert_called_once_with(123) @@ -136,7 +157,8 @@ def test_none_fixture(dm: DeviceManager): s1 = Mock() @dm.factory - def foo(bar): return s1(bar) + def foo(bar): + return s1(bar) devices = dm.build_all(fixtures={"bar": None}) s1.assert_called_once_with(None) @@ -147,7 +169,8 @@ def test_default_argument(dm: DeviceManager): s1 = Mock() @dm.factory - def foo(bar=42): return s1(bar) + def foo(bar=42): + return s1(bar) dm.build_all() s1.assert_called_once_with(42) @@ -157,7 +180,8 @@ def test_fixture_overrides_default_argument(dm: DeviceManager): s1 = Mock() @dm.factory - def foo(bar=42): return s1(bar) + def foo(bar=42): + return s1(bar) dm.build_all(fixtures={"bar": 17}) s1.assert_called_once_with(17) @@ -168,10 +192,12 @@ def test_fixture_overrides_factory(dm: DeviceManager): s2 = Mock() @dm.factory - def foo(bar): return s1(bar) + def foo(bar): + return s1(bar) @dm.factory - def bar(): return s2() + def bar(): + return s2() devices = dm.build_all(fixtures={"bar": 17}) s1.assert_called_once_with(17) @@ -184,10 +210,12 @@ def test_fixture_function(dm: DeviceManager): s1 = Mock() @dm.factory - def foo(bar=42): return s1(bar) + def foo(bar=42): + return s1(bar) @dm.fixture - def bar(): return 42 + def bar(): + return 42 devices = dm.build_all() s1.assert_called_once_with(42) @@ -201,7 +229,8 @@ def test_fixture_functions_are_lazy(dm: DeviceManager): dm.fixture(fix) @dm.factory - def foo(): return s1() + def foo(): + return s1() devices = dm.build_all() s1.assert_called_once() @@ -213,19 +242,22 @@ def test_duplicate_factories_error(dm: DeviceManager): s1 = Mock() @dm.factory - def foo(): return s1() + def foo(): + return s1() with pytest.raises(ValueError): @dm.factory - def foo(): return s1() + def foo(): + return s1() def test_missing_dependency_errors(dm: DeviceManager): s1 = Mock() @dm.factory - def foo(bar): return s1() + def foo(bar): + return s1() with pytest.raises(ValueError, match="Missing fixture or factory for bar"): dm.build_all() @@ -236,10 +268,12 @@ def test_missing_fixture_ok_if_not_required(dm: DeviceManager): s2 = Mock() @dm.factory - def foo(unknown): return s1(bar) + def foo(unknown): + return s1(bar) @dm.factory - def bar(): return s2() + def bar(): + return s2() # missing unknown but not needed for bar so ok devices = dm.build_devices(bar) @@ -253,10 +287,12 @@ def test_circular_dependencies_error(dm: DeviceManager): s2 = Mock() @dm.factory - def foo(bar): return s1(bar) + def foo(bar): + return s1(bar) @dm.factory - def bar(foo): return s2(foo) + def bar(foo): + return s2(foo) with pytest.raises(ValueError, match="circular dependencies"): dm.build_all() @@ -269,10 +305,12 @@ def test_repr(dm: DeviceManager): s2.__name__ = "S2" @dm.factory - def foo(one: int) -> SimMotor: return s1(one) + def foo(one: int) -> SimMotor: + return s1(one) @dm.v1_init(s2, prefix="S2_PREFIX") - def bar(_): pass + def bar(_): + pass assert repr(dm) == "" assert ( @@ -287,7 +325,8 @@ def test_build_errors_are_caught(dm: DeviceManager): s1.side_effect = err @dm.factory - def foo(): return s1() + def foo(): + return s1() devices = dm.build_all() @@ -303,10 +342,12 @@ def test_dependency_errors_propagate(dm: DeviceManager): s2.side_effect = err @dm.factory - def foo(bar): return s1(bar) + def foo(bar): + return s1(bar) @dm.factory - def bar(): return s2() + def bar(): + return s2() devices = dm.build_all() s2.assert_called_once() @@ -323,7 +364,8 @@ def test_devices_are_named(dm: DeviceManager): s1 = Mock() @dm.factory - def foo(): return s1() + def foo(): + return s1() f = foo.build() @@ -335,7 +377,8 @@ def test_skip_naming(dm: DeviceManager): s1 = Mock() @dm.factory(use_factory_name=False) - def foo(): return s1() + def foo(): + return s1() f = foo.build() @@ -347,7 +390,8 @@ def test_override_name(dm: DeviceManager): s1 = Mock() @dm.factory - def foo(): return s1() + def foo(): + return s1() f = foo.build(name="not_foo") @@ -361,7 +405,8 @@ def test_positional_only_args_error(dm: DeviceManager): with pytest.raises(ValueError, match="positional only arguments"): @dm.factory - def foo(foo, /): return s1() + def foo(foo, /): + return s1() def test_devices_or_raise(dm: DeviceManager): @@ -369,18 +414,21 @@ def test_devices_or_raise(dm: DeviceManager): s1.side_effect = RuntimeError("Build failed") @dm.factory - def foo(): return s1() + def foo(): + return s1() devices = dm.build_all() with RaisesGroup(RaisesExc(RuntimeError, match="Build failed")): devices.or_raise() + def test_connect(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) @dm.factory - def foo(): return s1() + def foo(): + return s1() con = dm.build_devices(foo).connect() s1.assert_called_once() @@ -390,12 +438,14 @@ def foo(): return s1() assert con.build_errors == {} assert con.connection_errors == {} + def test_build_and_connect(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) @dm.factory - def foo(): return s1() + def foo(): + return s1() con = dm.build_and_connect() s1.assert_called_once() @@ -409,31 +459,40 @@ def foo(): return s1() def test_factory_options(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) + @dm.factory(mock=True, timeout=12) - def foo(): return s1() + def foo(): + return s1() + con = dm.build_devices(foo).connect() s1().connect.assert_called_once_with(mock=True, timeout=12) assert con.connection_errors == {} assert con.build_errors == {} + def test_connect_failures(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) err = ValueError("Not connected") s1.return_value.connect.side_effect = err + @dm.factory - def foo(): return s1() + def foo(): + return s1() + con = dm.build_devices(foo).connect() s1().connect.assert_called_once_with(mock=False, timeout=DEFAULT_TIMEOUT) assert con.connection_errors == {"foo": err} assert con.build_errors == {} + def test_connect_or_raise_without_errors(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) @dm.factory - def foo(): return s1() + def foo(): + return s1() con = dm.build_devices(foo).connect().or_raise() s1.assert_called_once() @@ -441,12 +500,14 @@ def foo(): return s1() assert con == {"foo": s1()} + def test_connect_or_raise_with_build_errors(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) @dm.factory - def foo(): raise ValueError("foo error") + def foo(): + raise ValueError("foo error") with RaisesGroup(RaisesExc(ValueError, match="foo error")): dm.build_devices(foo).connect().or_raise() @@ -458,67 +519,82 @@ def test_connect_or_raise_with_connect_errors(dm: DeviceManager): s1.return_value.connect.side_effect = ValueError("foo connection") @dm.factory - def foo(): return s1() + def foo(): + return s1() with RaisesGroup(RaisesExc(ValueError, match="foo connection")): dm.build_devices(foo).connect().or_raise() + def test_build_and_connect_immediately(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) + @dm.factory - def foo(): return s1() + def foo(): + return s1() + f = foo.build(connect_immediately=True) s1.assert_called_once() s1().connect.assert_called_with(mock=False, timeout=DEFAULT_TIMEOUT) assert f is s1() + def test_skip_factory(dm: DeviceManager): s1 = Mock() s2 = Mock() @dm.factory - def foo(): return s1() + def foo(): + return s1() @dm.factory(skip=True) - def bar(): return s2() + def bar(): + return s2() devices = dm.build_all() s1.assert_called_once() s2.assert_not_called() assert devices.devices == {"foo": s1()} + def test_skip_ignored_if_required(dm: DeviceManager): s1 = Mock() s2 = Mock() @dm.factory - def foo(bar): return s1(bar) + def foo(bar): + return s1(bar) @dm.factory(skip=True) - def bar(): return s2() + def bar(): + return s2() devices = dm.build_all() s2.assert_called_once() s1.assert_called_once_with(s2()) assert devices.devices == {"foo": s1(), "bar": s2()} + def test_mock_all(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV2Device) + @dm.factory - def foo(): return s1() + def foo(): + return s1() dm.build_all(mock=True).connect(timeout=11) s1().connect.assert_called_once_with(mock=True, timeout=11) def test_v1_device_factory(dm: DeviceManager): - s1 = MagicMock(spec=OphydV1Device) # type: ignore + s1 = MagicMock(spec=OphydV1Device) # type: ignore s1.__name__ = "S1" @dm.v1_init(s1, prefix="S1_PREFIX") - def foo(_): pass + def foo(_): + pass with patch("dodal.device_manager.wait_for_connection") as wfc: devices = dm.build_all() @@ -528,18 +604,22 @@ def foo(_): pass assert devices.devices["foo"] is device wfc.assert_called_once_with(device, timeout=DEFAULT_TIMEOUT) + def test_v1_v2_name_clash(dm: DeviceManager): s1 = Mock() s2 = MagicMock() @dm.factory - def foo(): return s1() + def foo(): + return s1() with pytest.raises(ValueError, match="name"): + @dm.v1_init(s2, prefix="S2_PREFIX") def foo(_): pass + def test_v1_decorator_is_transparent(dm: DeviceManager): s1 = MagicMock() @@ -554,11 +634,14 @@ def foo(s): dev.special_init_method.assert_called_once() s1.assert_not_called() + def test_v1_no_wait(dm: DeviceManager): s1 = Mock() + @dm.v1_init(s1, prefix="S1_PREFIX", wait=False) def foo(_): pass + with patch("dodal.device_manager.wait_for_connection") as wfc: foo.build() wfc.assert_not_called() @@ -571,25 +654,34 @@ def test_connect_ignores_v1(): # mock raises exception if connect is called assert con.devices == {"foo": v1} + def test_v1_mocking(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV1Device) + @dm.v1_init(s1, prefix="S1_PREFIX", mock=True) - def foo(_): pass + def foo(_): + pass + with patch("dodal.device_manager.make_fake_device") as mfd: dm.build_all() mfd.assert_called_once_with(s1) + def test_v1_init_params(dm: DeviceManager): # values are passed from fixtures s1 = Mock() s1.return_value = Mock(spec=OphydV1Device) s1.return_value.mock_add_spec(["set_up_with"]) + @dm.fixture - def one(): return "one" + def one(): + return "one" + @dm.v1_init(s1, prefix="S1_PREFIX", wait=False) def foo(s, one, two): s.set_up_with(one, two) + dev = dm.build_devices(foo, fixtures={"two": 2}) s1.assert_called_once_with(name="foo", prefix="S1_PREFIX") s1().set_up_with.assert_called_once_with("one", 2) @@ -599,6 +691,7 @@ def test_lazy_fixtures_non_lazy(): lf = LazyFixtures(provided={"foo": "bar"}, factories={}) assert lf["foo"] == "bar" + def test_lazy_fixtures_lazy(): buzz = Mock() buzz.return_value = "buzz" @@ -610,6 +703,7 @@ def test_lazy_fixtures_lazy(): assert lf.get("fizz") == "buzz" buzz.assert_called_once() + def test_lazy_fixtures_provided_overrides_factory(): buzz = Mock() buzz.return_value = "buzz" @@ -619,6 +713,7 @@ def test_lazy_fixtures_provided_overrides_factory(): assert lf["fizz"] == "foo" buzz.assert_not_called() + def test_lazy_fixtures_deduplicated(): buzz = Mock() buzz.return_value = "buzz" @@ -627,19 +722,25 @@ def test_lazy_fixtures_deduplicated(): assert len(lf) == 1 + def test_lazy_fixtures_iter(): buzz = Mock() buzz.return_value = "buzz" - lf = LazyFixtures(provided={"foo": "bar", "one": "two"}, factories={"fizz": buzz, "one": "two"}) + lf = LazyFixtures( + provided={"foo": "bar", "one": "two"}, factories={"fizz": buzz, "one": "two"} + ) assert sorted(lf) == ["fizz", "foo", "one"] + def test_lazy_fixtures_contains(): buzz = Mock() buzz.return_value = "buzz" - lf = LazyFixtures(provided={"foo": "bar", "one": "two"}, factories={"fizz": buzz, "one": "two"}) + lf = LazyFixtures( + provided={"foo": "bar", "one": "two"}, factories={"fizz": buzz, "one": "two"} + ) assert "foo" in lf assert "fizz" in lf From 3609a8fb81c309e0654e4c11fc69d2c9829d10d9 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 12 Nov 2025 15:32:24 +0000 Subject: [PATCH 51/74] Linting et al --- src/dodal/cli.py | 3 +-- src/dodal/device_manager.py | 1 - tests/test_device_manager.py | 7 +++---- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/dodal/cli.py b/src/dodal/cli.py index 9f78ff40a5e..5afbdfbbf9b 100644 --- a/src/dodal/cli.py +++ b/src/dodal/cli.py @@ -1,6 +1,5 @@ import importlib import os -import warnings from collections.abc import Mapping from pathlib import Path @@ -10,8 +9,8 @@ from ophyd_async.plan_stubs import ensure_connected from dodal.beamlines import all_beamline_names, module_name_for_beamline -from dodal.device_manager import DeviceManager from dodal.common.beamlines.beamline_utils import set_path_provider +from dodal.device_manager import DeviceManager from dodal.utils import AnyDevice, filter_ophyd_devices, make_all_devices from . import __version__ diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index e47001c4417..82b098b44d2 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -2,7 +2,6 @@ import inspect import itertools import typing -import warnings from collections.abc import Callable, Iterable, Mapping, MutableMapping from functools import cached_property, wraps from inspect import Parameter diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py index 0853458f018..ab1ad460db6 100644 --- a/tests/test_device_manager.py +++ b/tests/test_device_manager.py @@ -1,11 +1,10 @@ -from dodal import device_manager -from ophyd_async.sim import SimMotor -import pytest -from pytest import RaisesExc, RaisesGroup from unittest.mock import MagicMock, Mock, patch +import pytest from ophyd.device import Device as OphydV1Device from ophyd_async.core import Device as OphydV2Device +from ophyd_async.sim import SimMotor +from pytest import RaisesExc, RaisesGroup from dodal.device_manager import ( DEFAULT_TIMEOUT, From 14f96a3bb64e222f7f1ad625851b05f2a3cb03f7 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 12 Nov 2025 15:51:35 +0000 Subject: [PATCH 52/74] Support single device manager name in CLI --- src/dodal/cli.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/dodal/cli.py b/src/dodal/cli.py index 5afbdfbbf9b..cd90258cdf2 100644 --- a/src/dodal/cli.py +++ b/src/dodal/cli.py @@ -45,8 +45,8 @@ def main(ctx: click.Context) -> None: "attempt any I/O. Useful as a a dry-run.", default=False, ) -@click.option("-n", "--name", multiple=True) -def connect(beamline: str, all: bool, sim_backend: bool, name: tuple[str, ...]) -> None: +@click.option("-n", "--name") +def connect(beamline: str, all: bool, sim_backend: bool, name: str | None) -> None: """Initialises a beamline module, connects to all devices, reports any connection issues.""" @@ -71,15 +71,16 @@ def connect(beamline: str, all: bool, sim_backend: bool, name: tuple[str, ...]) # be lazy. if name: - for manager_name in name: - if (manager := getattr(mod, manager_name, None)) and isinstance( - manager, DeviceManager - ): - devices, instance_exceptions, connect_exceptions = ( - manager.build_and_connect( - mock=sim_backend, - ) + if (manager := getattr(mod, name, None)) and isinstance(manager, DeviceManager): + devices, instance_exceptions, connect_exceptions = ( + manager.build_and_connect( + mock=sim_backend, ) + ) + else: + raise ValueError( + f"Name '{name}' could not be found or is not a DeviceManager" + ) else: _spoof_path_provider() devices, instance_exceptions = make_all_devices( From e3ee34071d558908a196c84f379ff6347c569e63 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 12 Nov 2025 15:51:46 +0000 Subject: [PATCH 53/74] Ignore mock type warnings --- tests/test_device_manager.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py index ab1ad460db6..01a25affcea 100644 --- a/tests/test_device_manager.py +++ b/tests/test_device_manager.py @@ -300,7 +300,7 @@ def bar(foo): # chasing coverage numbers def test_repr(dm: DeviceManager): s1 = Mock() - s2 = Mock() + s2: type[OphydV1Device] = Mock() # type: ignore s2.__name__ = "S2" @dm.factory @@ -588,10 +588,10 @@ def foo(): def test_v1_device_factory(dm: DeviceManager): - s1 = MagicMock(spec=OphydV1Device) # type: ignore + s1 = Mock(spec=OphydV1Device) s1.__name__ = "S1" - @dm.v1_init(s1, prefix="S1_PREFIX") + @dm.v1_init(s1, prefix="S1_PREFIX") # type: ignore def foo(_): pass @@ -609,12 +609,12 @@ def test_v1_v2_name_clash(dm: DeviceManager): s2 = MagicMock() @dm.factory - def foo(): + def foo(): # type: ignore foo is overridden below return s1() with pytest.raises(ValueError, match="name"): - @dm.v1_init(s2, prefix="S2_PREFIX") + @dm.v1_init(s2, prefix="S2_PREFIX") # type:ignore def foo(_): pass @@ -622,7 +622,7 @@ def foo(_): def test_v1_decorator_is_transparent(dm: DeviceManager): s1 = MagicMock() - @dm.v1_init(s1, prefix="S1_PREFIX") + @dm.v1_init(s1, prefix="S1_PREFIX") # type: ignore def foo(s): # arbitrary setup method s.special_init_method() @@ -637,7 +637,7 @@ def foo(s): def test_v1_no_wait(dm: DeviceManager): s1 = Mock() - @dm.v1_init(s1, prefix="S1_PREFIX", wait=False) + @dm.v1_init(s1, prefix="S1_PREFIX", wait=False) # type: ignore def foo(_): pass @@ -658,7 +658,7 @@ def test_v1_mocking(dm: DeviceManager): s1 = Mock() s1.return_value = Mock(spec=OphydV1Device) - @dm.v1_init(s1, prefix="S1_PREFIX", mock=True) + @dm.v1_init(s1, prefix="S1_PREFIX", mock=True) # type: ignore def foo(_): pass @@ -677,7 +677,7 @@ def test_v1_init_params(dm: DeviceManager): def one(): return "one" - @dm.v1_init(s1, prefix="S1_PREFIX", wait=False) + @dm.v1_init(s1, prefix="S1_PREFIX", wait=False) # type: ignore def foo(s, one, two): s.set_up_with(one, two) @@ -752,7 +752,7 @@ def foo(): """This is the docstring for foo""" return Mock() - @dm.v1_init(Mock(), prefix="MOCK_PREFIX") + @dm.v1_init(Mock(), prefix="MOCK_PREFIX") # type: ignore def bar(_): """This is the docstring for bar""" pass From 45716f2e499041c3c79f8a7f774e36b2de28e6b8 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 12 Nov 2025 16:54:16 +0000 Subject: [PATCH 54/74] Appease pre-commit lints --- src/dodal/device_manager.py | 6 +++--- tests/test_device_manager.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 82b098b44d2..2fa954a74df 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -166,7 +166,7 @@ def build( device.set_name(name) return device # type: ignore - it's us, honest - def _create(self, *args, **kwargs) -> V2: + def create(self, *args, **kwargs) -> V2: # TODO: Remove when v1 support is no longer required return self(*args, **kwargs) @@ -259,7 +259,7 @@ def __call__(self, *args, **kwargs): """Call the wrapped function to make decorator transparent""" return self.post_create(*args, **kwargs) - def _create(self, *args, **kwargs) -> V1: + def create(self, *args, **kwargs) -> V1: device = self.factory(name=self.name, prefix=self.prefix) if self.wait: wait_for_connection(device, timeout=self.timeout) @@ -515,7 +515,7 @@ def build_devices( if isinstance(factory, V1DeviceFactory): # TODO: Remove when ophyd v1 support is no longer required factory = factory.mock_if_needed(mock) - built_device = factory._create(**params) + built_device = factory.create(**params) built[device] = built_device connection_specs[device] = ConnectionSpec( mock=mock or factory.mock, diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py index 01a25affcea..bb8ce0dd4a7 100644 --- a/tests/test_device_manager.py +++ b/tests/test_device_manager.py @@ -681,7 +681,7 @@ def one(): def foo(s, one, two): s.set_up_with(one, two) - dev = dm.build_devices(foo, fixtures={"two": 2}) + dm.build_devices(foo, fixtures={"two": 2}) s1.assert_called_once_with(name="foo", prefix="S1_PREFIX") s1().set_up_with.assert_called_once_with("one", 2) From 897d129d5e65d9e24ff0dbcd00acaee8071b7112 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 13 Nov 2025 16:15:49 +0000 Subject: [PATCH 55/74] Add tests for device manager use in CLI --- tests/test_cli.py | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 7132a64b069..509fd72cbe7 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,5 +1,5 @@ import os -from unittest.mock import patch +from unittest.mock import Mock, patch import pytest from bluesky import RunEngine @@ -13,6 +13,7 @@ from dodal import __version__ from dodal.cli import main +from dodal.device_manager import DeviceManager from dodal.utils import AnyDevice, OphydV1Device, OphydV2Device # Test with an example beamline, device instantiation is already tested @@ -297,6 +298,34 @@ def test_cli_connect_when_devices_error( ) +@patch.dict(os.environ, clear=True) +def test_missing_device_manager(runner: CliRunner): + with patch("dodal.cli.importlib"): + with pytest.raises( + ValueError, + match="Name 'devices' could not be found or is not a DeviceManager", + ): + print("Invoking connect") + runner.invoke( + main, ["connect", "-n", "devices", "i22"], catch_exceptions=False + ) + print("Ran connect") + + +@patch.dict(os.environ, clear=True) +@pytest.mark.parametrize("mock", [True, False], ids=["live", "sim"].__getitem__) +def test_device_manager_init(runner: CliRunner, mock: bool): + with patch("dodal.cli.importlib") as mock_import: + dm = mock_import.import_module("dodal.beamlines.i22") + dm.devices = Mock(spec=DeviceManager) + mock_import.reset_mock() + runner.invoke( + main, ["connect", "-n", "devices", "i22"] + (["-s"] if mock else []) + ) + mock_import.import_module.assert_called_once_with("dodal.beamlines.i22") + dm.devices.build_and_connect.assert_called_once_with(mock=mock) + + def _mock_connect( *args, runner: CliRunner, From f1366014d5306bb1a00d7bbf0477389d72ba50e7 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 19 Nov 2025 10:35:26 +0000 Subject: [PATCH 56/74] Make 'devices' default name for device manager in CLI --- src/dodal/cli.py | 22 +++++++++------------- tests/test_cli.py | 21 ++++++++++----------- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/src/dodal/cli.py b/src/dodal/cli.py index cd90258cdf2..919c7b46fa8 100644 --- a/src/dodal/cli.py +++ b/src/dodal/cli.py @@ -45,8 +45,8 @@ def main(ctx: click.Context) -> None: "attempt any I/O. Useful as a a dry-run.", default=False, ) -@click.option("-n", "--name") -def connect(beamline: str, all: bool, sim_backend: bool, name: str | None) -> None: +@click.option("-n", "--name", "device_manager", default="devices") +def connect(beamline: str, all: bool, sim_backend: bool, device_manager: str) -> None: """Initialises a beamline module, connects to all devices, reports any connection issues.""" @@ -70,18 +70,14 @@ def connect(beamline: str, all: bool, sim_backend: bool, name: str | None) -> No # because the alternatives is handling the fact that only some devices may # be lazy. - if name: - if (manager := getattr(mod, name, None)) and isinstance(manager, DeviceManager): - devices, instance_exceptions, connect_exceptions = ( - manager.build_and_connect( - mock=sim_backend, - ) - ) - else: - raise ValueError( - f"Name '{name}' could not be found or is not a DeviceManager" - ) + if (manager := getattr(mod, device_manager, None)) and isinstance( + manager, DeviceManager + ): + devices, instance_exceptions, connect_exceptions = manager.build_and_connect( + mock=sim_backend, + ) else: + print(f"No device manager named '{device_manager}' found in {mod}") _spoof_path_provider() devices, instance_exceptions = make_all_devices( full_module_path, diff --git a/tests/test_cli.py b/tests/test_cli.py index 509fd72cbe7..4128cb3b6f2 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -298,18 +298,17 @@ def test_cli_connect_when_devices_error( ) +@patch("dodal.cli.importlib") +@patch("dodal.cli.make_all_devices") +@patch("dodal.cli._connect_devices") @patch.dict(os.environ, clear=True) -def test_missing_device_manager(runner: CliRunner): - with patch("dodal.cli.importlib"): - with pytest.raises( - ValueError, - match="Name 'devices' could not be found or is not a DeviceManager", - ): - print("Invoking connect") - runner.invoke( - main, ["connect", "-n", "devices", "i22"], catch_exceptions=False - ) - print("Ran connect") +def test_missing_device_manager(connect, make, imp, runner: CliRunner): + # If the device manager cannot be found, it should fall back to the + # make_all_devices + _connect_devices approach. + make.return_value = ({}, {}) + runner.invoke(main, ["connect", "-n", "devices", "i22"]) + make.assert_called_once() + connect.assert_called_once() @patch.dict(os.environ, clear=True) From f33a7b5e5cd8f2899583c6026d02224fd4eb86a2 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 19 Nov 2025 10:35:48 +0000 Subject: [PATCH 57/74] Clean up TODO comments --- src/dodal/device_manager.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 2fa954a74df..624367e38eb 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -167,7 +167,7 @@ def build( return device # type: ignore - it's us, honest def create(self, *args, **kwargs) -> V2: - # TODO: Remove when v1 support is no longer required + # TODO: Remove when v1 support is no longer required - see #1718 return self(*args, **kwargs) def __call__(self, *args, **kwargs) -> V2: @@ -181,7 +181,7 @@ def __repr__(self) -> str: return f"<{self.name}: DeviceFactory{params}>" -# TODO: Remove when ophyd v1 support is no longer required +# TODO: Remove when ophyd v1 support is no longer required - see #1718 class V1DeviceFactory(Generic[V1]): """ Wrapper around an ophyd v1 device that holds a reference to a device @@ -240,7 +240,7 @@ def skip(self) -> bool: return self._skip() if callable(self._skip) else self._skip def mock_if_needed(self, mock=False) -> Self: - # TODO: Remove when Ophyd V1 support is no longer required + # TODO: Remove when Ophyd V1 support is no longer required - see #1718 factory = ( make_fake_device(self.factory) if (self.mock or mock) else self.factory ) @@ -320,7 +320,7 @@ def connect(self, timeout: float | None = None) -> ConnectionResult: loop: asyncio.EventLoop = get_bluesky_event_loop() # type: ignore for name, device in self.devices.items(): if not isinstance(device, OphydV2Device): - # TODO: Remove when ophyd v1 support is no longer required + # TODO: Remove when ophyd v1 support is no longer required - see #1718 # V1 devices are connected at creation time assuming wait is not set to False connected[name] = device continue @@ -513,7 +513,7 @@ def build_devices( } try: if isinstance(factory, V1DeviceFactory): - # TODO: Remove when ophyd v1 support is no longer required + # TODO: Remove when ophyd v1 support is no longer required - see #1718 factory = factory.mock_if_needed(mock) built_device = factory.create(**params) built[device] = built_device @@ -577,8 +577,8 @@ def _build_order( given factory list. """ - # TODO: This is not an efficient way of doing this - # However, for realistic use cases, it is fast enough for now + # This is not an efficient way of doing this, however, for realistic use + # cases, it is fast enough for now order = [] available = set(fixtures.keys()) pending = factories From caa6319868593c1524021fbfa9dbe6eb2a522d64 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 19 Nov 2025 11:03:11 +0000 Subject: [PATCH 58/74] Set return_value when creating mocks --- tests/test_device_manager.py | 50 ++++++++++++------------------------ 1 file changed, 17 insertions(+), 33 deletions(-) diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py index bb8ce0dd4a7..d4b3d2ce683 100644 --- a/tests/test_device_manager.py +++ b/tests/test_device_manager.py @@ -422,8 +422,7 @@ def foo(): def test_connect(dm: DeviceManager): - s1 = Mock() - s1.return_value = Mock(spec=OphydV2Device) + s1 = Mock(return_value=Mock(spec=OphydV2Device)) @dm.factory def foo(): @@ -439,8 +438,7 @@ def foo(): def test_build_and_connect(dm: DeviceManager): - s1 = Mock() - s1.return_value = Mock(spec=OphydV2Device) + s1 = Mock(return_value=Mock(spec=OphydV2Device)) @dm.factory def foo(): @@ -456,8 +454,7 @@ def foo(): def test_factory_options(dm: DeviceManager): - s1 = Mock() - s1.return_value = Mock(spec=OphydV2Device) + s1 = Mock(return_value=Mock(spec=OphydV2Device)) @dm.factory(mock=True, timeout=12) def foo(): @@ -470,8 +467,7 @@ def foo(): def test_connect_failures(dm: DeviceManager): - s1 = Mock() - s1.return_value = Mock(spec=OphydV2Device) + s1 = Mock(return_value=Mock(spec=OphydV2Device)) err = ValueError("Not connected") s1.return_value.connect.side_effect = err @@ -486,8 +482,7 @@ def foo(): def test_connect_or_raise_without_errors(dm: DeviceManager): - s1 = Mock() - s1.return_value = Mock(spec=OphydV2Device) + s1 = Mock(return_value=Mock(spec=OphydV2Device)) @dm.factory def foo(): @@ -501,8 +496,7 @@ def foo(): def test_connect_or_raise_with_build_errors(dm: DeviceManager): - s1 = Mock() - s1.return_value = Mock(spec=OphydV2Device) + Mock(return_value=Mock(spec=OphydV2Device)) @dm.factory def foo(): @@ -513,8 +507,7 @@ def foo(): def test_connect_or_raise_with_connect_errors(dm: DeviceManager): - s1 = Mock() - s1.return_value = Mock(spec=OphydV2Device) + s1 = Mock(return_value=Mock(spec=OphydV2Device)) s1.return_value.connect.side_effect = ValueError("foo connection") @dm.factory @@ -526,14 +519,13 @@ def foo(): def test_build_and_connect_immediately(dm: DeviceManager): - s1 = Mock() - s1.return_value = Mock(spec=OphydV2Device) + s1 = Mock(return_value=Mock(spec=OphydV2Device)) @dm.factory def foo(): return s1() - f = foo.build(connect_immediately=True) + f = foo.build(connect_immediately=True) # type: ignore s1.assert_called_once() s1().connect.assert_called_with(mock=False, timeout=DEFAULT_TIMEOUT) assert f is s1() @@ -576,8 +568,7 @@ def bar(): def test_mock_all(dm: DeviceManager): - s1 = Mock() - s1.return_value = Mock(spec=OphydV2Device) + s1 = Mock(return_value=Mock(spec=OphydV2Device)) @dm.factory def foo(): @@ -655,8 +646,7 @@ def test_connect_ignores_v1(): def test_v1_mocking(dm: DeviceManager): - s1 = Mock() - s1.return_value = Mock(spec=OphydV1Device) + s1 = Mock(return_value=Mock(spec=OphydV1Device)) @dm.v1_init(s1, prefix="S1_PREFIX", mock=True) # type: ignore def foo(_): @@ -669,8 +659,7 @@ def foo(_): def test_v1_init_params(dm: DeviceManager): # values are passed from fixtures - s1 = Mock() - s1.return_value = Mock(spec=OphydV1Device) + s1 = Mock(return_value=Mock(spec=OphydV1Device)) s1.return_value.mock_add_spec(["set_up_with"]) @dm.fixture @@ -692,8 +681,7 @@ def test_lazy_fixtures_non_lazy(): def test_lazy_fixtures_lazy(): - buzz = Mock() - buzz.return_value = "buzz" + buzz = Mock(return_value="buzz") lf = LazyFixtures(provided={}, factories={"fizz": buzz}) assert lf.get("foo") is None @@ -704,8 +692,7 @@ def test_lazy_fixtures_lazy(): def test_lazy_fixtures_provided_overrides_factory(): - buzz = Mock() - buzz.return_value = "buzz" + buzz = Mock(return_value="buzz") lf = LazyFixtures(provided={"fizz": "foo"}, factories={"fizz": buzz}) @@ -714,8 +701,7 @@ def test_lazy_fixtures_provided_overrides_factory(): def test_lazy_fixtures_deduplicated(): - buzz = Mock() - buzz.return_value = "buzz" + buzz = Mock(return_value="buzz") lf = LazyFixtures(provided={"fizz": "foo"}, factories={"fizz": buzz}) @@ -723,8 +709,7 @@ def test_lazy_fixtures_deduplicated(): def test_lazy_fixtures_iter(): - buzz = Mock() - buzz.return_value = "buzz" + buzz = Mock(return_value="buzz") lf = LazyFixtures( provided={"foo": "bar", "one": "two"}, factories={"fizz": buzz, "one": "two"} @@ -734,8 +719,7 @@ def test_lazy_fixtures_iter(): def test_lazy_fixtures_contains(): - buzz = Mock() - buzz.return_value = "buzz" + buzz = Mock(return_value="buzz") lf = LazyFixtures( provided={"foo": "bar", "one": "two"}, factories={"fizz": buzz, "one": "two"} From 815bd47c8487d4bacd7f5638cbbe09fbed1e7d03 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 21 Nov 2025 14:26:10 +0000 Subject: [PATCH 59/74] Fix typing in v1_init decorator --- src/dodal/device_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 624367e38eb..1f9ea590eee 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -384,7 +384,7 @@ def v1_init( and is not used to create the device. """ - def decorator(init: OphydInitialiser[V1]): + def decorator(init: OphydInitialiser[V1]) -> V1DeviceFactory[V1]: name = init.__name__ if name in self: raise ValueError(f"Duplicate factory name: {name}") From 9c22d22b1c460b2ea579f6444529801695031e81 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 21 Nov 2025 15:45:40 +0000 Subject: [PATCH 60/74] Use ParamSpec when passing through __call__ --- src/dodal/device_manager.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 1f9ea590eee..153654ed426 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -41,7 +41,7 @@ V2 = TypeVar("V2", bound=OphydV2Device) DeviceFactoryDecorator = Callable[[Callable[Args, V2]], "DeviceFactory[Args, V2]"] -OphydInitialiser = Callable[Concatenate[V1, ...], V1 | None] +OphydInitialiser = Callable[Concatenate[V1, Args], V1 | None] _EMPTY = object() """Sentinel value to distinguish between missing values and present but null values""" @@ -166,11 +166,11 @@ def build( device.set_name(name) return device # type: ignore - it's us, honest - def create(self, *args, **kwargs) -> V2: + def create(self, *args: Args.args, **kwargs: Args.kwargs) -> V2: # TODO: Remove when v1 support is no longer required - see #1718 return self(*args, **kwargs) - def __call__(self, *args, **kwargs) -> V2: + def __call__(self, *args: Args.args, **kwargs: Args.kwargs) -> V2: device = self.factory(*args, **kwargs) if self.use_factory_name: device.set_name(self.name) @@ -182,7 +182,7 @@ def __repr__(self) -> str: # TODO: Remove when ophyd v1 support is no longer required - see #1718 -class V1DeviceFactory(Generic[V1]): +class V1DeviceFactory(Generic[Args, V1]): """ Wrapper around an ophyd v1 device that holds a reference to a device manager that can provide dependencies, along with default connection @@ -198,7 +198,7 @@ def __init__( skip: SkipType, wait: bool, timeout: int, - init: OphydInitialiser[V1], + init: OphydInitialiser[V1, Args], manager: "DeviceManager", ): self.factory = factory @@ -255,11 +255,11 @@ def mock_if_needed(self, mock=False) -> Self: manager=self._manager, ) - def __call__(self, *args, **kwargs): + def __call__(self, dev: V1, *args: Args.args, **kwargs: Args.kwargs): """Call the wrapped function to make decorator transparent""" - return self.post_create(*args, **kwargs) + return self.post_create(dev, *args, **kwargs) - def create(self, *args, **kwargs) -> V1: + def create(self, *args: Args.args, **kwargs: Args.kwargs) -> V1: device = self.factory(name=self.name, prefix=self.prefix) if self.wait: wait_for_connection(device, timeout=self.timeout) @@ -384,7 +384,7 @@ def v1_init( and is not used to create the device. """ - def decorator(init: OphydInitialiser[V1]) -> V1DeviceFactory[V1]: + def decorator(init: OphydInitialiser[V1, Args]) -> V1DeviceFactory[Args, V1]: name = init.__name__ if name in self: raise ValueError(f"Duplicate factory name: {name}") @@ -534,7 +534,7 @@ def __getitem__(self, name): def _expand_dependencies( self, - factories: Iterable[DeviceFactory[..., V2] | V1DeviceFactory[V1]], + factories: Iterable[DeviceFactory[..., V2] | V1DeviceFactory[..., V1]], available_fixtures: Mapping[str, Any], ) -> set[str]: """ @@ -566,7 +566,7 @@ def _expand_dependencies( def _build_order( self, - factories: dict[str, DeviceFactory[..., V2] | V1DeviceFactory[V1]], + factories: dict[str, DeviceFactory[..., V2] | V1DeviceFactory[..., V1]], fixtures: Mapping[str, Any], ) -> list[str]: """ From 820049dc19a244bb5a6b25df94b19d0b47573654 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 21 Nov 2025 15:47:41 +0000 Subject: [PATCH 61/74] Handle or ignore var args --- src/dodal/device_manager.py | 39 +++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 153654ed426..25ec91061cf 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -95,19 +95,23 @@ class DeviceFactory(Generic[Args, V2]): connected. """ - factory: Callable[Args, V2] - use_factory_name: bool - timeout: float - mock: bool - _skip: SkipType - _manager: "DeviceManager" - - def __init__(self, factory, use_factory_name, timeout, mock, skip, manager): - if any( - p.kind == Parameter.POSITIONAL_ONLY - for p in inspect.signature(factory).parameters.values() - ): - raise ValueError(f"{factory.__name__} has positional only arguments") + def __init__( + self, + factory: Callable[Args, V2], + use_factory_name: bool, + timeout: float, + mock: bool, + skip: SkipType, + manager: "DeviceManager", + ): + for name, param in inspect.signature(factory).parameters.items(): + if param.kind == Parameter.POSITIONAL_ONLY: + raise ValueError( + f"{factory.__name__} has positional only argument '{name}'" + ) + elif param.kind == Parameter.VAR_POSITIONAL: + raise ValueError(f"{factory.__name__} has variadic argument '{name}'") + self.factory = factory self.use_factory_name = use_factory_name self.timeout = timeout @@ -125,7 +129,11 @@ def name(self) -> str: def dependencies(self) -> set[str]: """Names of all parameters""" sig = inspect.signature(self.factory) - return {para.name for para in sig.parameters.values()} + return { + para.name + for para in sig.parameters.values() + if para.kind is not Parameter.VAR_KEYWORD + } @cached_property def optional_dependencies(self) -> set[str]: @@ -135,6 +143,7 @@ def optional_dependencies(self) -> set[str]: para.name for para in sig.parameters.values() if para.default is not Parameter.empty + and para.kind is not Parameter.VAR_KEYWORD } @property @@ -222,7 +231,7 @@ def dependencies(self) -> set[str]: sig = inspect.signature(self.post_create) # first parameter should be the device we've just built _, *params = sig.parameters.values() - return {para.name for para in params} + return {para.name for para in params if para.kind is not Parameter.VAR_KEYWORD} @cached_property def optional_dependencies(self) -> set[str]: From f626761860af9ab6311c2c2a0857c2478bbc49c6 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 21 Nov 2025 15:47:52 +0000 Subject: [PATCH 62/74] Update tests --- tests/test_device_manager.py | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py index d4b3d2ce683..f2acf9ebdb9 100644 --- a/tests/test_device_manager.py +++ b/tests/test_device_manager.py @@ -268,7 +268,7 @@ def test_missing_fixture_ok_if_not_required(dm: DeviceManager): @dm.factory def foo(unknown): - return s1(bar) + return s1(unknown) @dm.factory def bar(): @@ -319,9 +319,8 @@ def bar(_): def test_build_errors_are_caught(dm: DeviceManager): - s1 = Mock() err = RuntimeError("Build failed") - s1.side_effect = err + s1 = Mock(side_effect=err) @dm.factory def foo(): @@ -337,8 +336,7 @@ def foo(): def test_dependency_errors_propagate(dm: DeviceManager): s1 = Mock() err = RuntimeError("Build failed") - s2 = Mock() - s2.side_effect = err + s2 = Mock(side_effect=err) @dm.factory def foo(bar): @@ -401,16 +399,36 @@ def foo(): def test_positional_only_args_error(dm: DeviceManager): s1 = Mock() - with pytest.raises(ValueError, match="positional only arguments"): + with pytest.raises(ValueError, match="positional only argument 'bar'"): @dm.factory - def foo(foo, /): + def foo(bar, /): return s1() -def test_devices_or_raise(dm: DeviceManager): +def test_variadic_args_error(dm: DeviceManager): s1 = Mock() - s1.side_effect = RuntimeError("Build failed") + with pytest.raises(ValueError, match="variadic argument 'args'"): + + @dm.factory + def foo(*args): + return s1(*args) + + +def test_kwargs_factory(dm: DeviceManager): + s1 = Mock() + + # Factories can have kwargs but they're ignored by the device manager + @dm.factory + def foo(**kwargs): + s1(**kwargs) + + dm.build_all(s1) + s1.assert_called_once_with() + + +def test_devices_or_raise(dm: DeviceManager): + s1 = Mock(side_effect=RuntimeError("Build failed")) @dm.factory def foo(): From 6afc663f779c07c85cb17436fc8f14a454067864 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 21 Nov 2025 16:23:32 +0000 Subject: [PATCH 63/74] Rename ignore skip test --- tests/test_device_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py index f2acf9ebdb9..f4f51488ed3 100644 --- a/tests/test_device_manager.py +++ b/tests/test_device_manager.py @@ -567,7 +567,7 @@ def bar(): assert devices.devices == {"foo": s1()} -def test_skip_ignored_if_required(dm: DeviceManager): +def test_skip_is_ignored_if_device_is_required(dm: DeviceManager): s1 = Mock() s2 = Mock() From 4b195851b857ddccf4454a153b41fb2ea9ebcee9 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 21 Nov 2025 16:27:59 +0000 Subject: [PATCH 64/74] Simplify LazyFixture __getitem__ --- src/dodal/device_manager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index 25ec91061cf..dac25409c9c 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -75,8 +75,8 @@ def __len__(self) -> int: return len(self.ready.keys()) + len(self.lazy.keys()) def __getitem__(self, key: str) -> Any: - if (value := self.ready.get(key, _EMPTY)) is not _EMPTY: - return value + if key in self.ready: + return self.ready[key] if factory := self.lazy.pop(key, None): value = factory() self.ready[key] = value From 0487c9b5c14ddeaea51163d90da129061e8aebf4 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 21 Nov 2025 16:51:49 +0000 Subject: [PATCH 65/74] Used UserDict as base class for LazyFixtures --- src/dodal/device_manager.py | 31 +++++++------------------------ 1 file changed, 7 insertions(+), 24 deletions(-) diff --git a/src/dodal/device_manager.py b/src/dodal/device_manager.py index dac25409c9c..2eb490dfd27 100644 --- a/src/dodal/device_manager.py +++ b/src/dodal/device_manager.py @@ -1,7 +1,7 @@ import asyncio import inspect -import itertools import typing +from collections import UserDict from collections.abc import Callable, Iterable, Mapping, MutableMapping from functools import cached_property, wraps from inspect import Parameter @@ -47,14 +47,13 @@ """Sentinel value to distinguish between missing values and present but null values""" -class LazyFixtures(Mapping[str, Any]): +class LazyFixtures(UserDict[str, Any]): """ Wrapper around fixtures and fixture generators If a fixture is provided at runtime, the generator function does not have to be called. """ - ready: MutableMapping[str, Any] lazy: MutableMapping[str, Callable[[], Any]] def __init__( @@ -62,29 +61,13 @@ def __init__( provided: Mapping[str, Any] | None, factories: Mapping[str, Callable[[], Any]], ): - # wrap to prevent modification escaping - self.ready = dict(provided or {}) - # drop duplicate keys so the len and iter methods are easier - self.lazy = {k: v for k, v in factories.items() if k not in self.ready} - - def __contains__(self, key: Any) -> bool: - return key in self.ready or key in self.lazy - - def __len__(self) -> int: - # Can just add the lengths as the keys are distinct by construction - return len(self.ready.keys()) + len(self.lazy.keys()) + super().__init__(dict.fromkeys(factories, _EMPTY) | dict(provided or {})) + self.lazy = dict(factories) def __getitem__(self, key: str) -> Any: - if key in self.ready: - return self.ready[key] - if factory := self.lazy.pop(key, None): - value = factory() - self.ready[key] = value - return value - raise KeyError(key) - - def __iter__(self): - return itertools.chain(self.lazy.keys(), self.ready.keys()) + if self.data[key] is _EMPTY: + self.data[key] = self.lazy[key]() + return self.data[key] class DeviceFactory(Generic[Args, V2]): From 9af24281e37608fe67569728d47d22f23589d70f Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Mon, 24 Nov 2025 17:22:49 +0000 Subject: [PATCH 66/74] Make pyright happy --- tests/test_device_manager.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_device_manager.py b/tests/test_device_manager.py index f4f51488ed3..69d228197f4 100644 --- a/tests/test_device_manager.py +++ b/tests/test_device_manager.py @@ -2,6 +2,7 @@ import pytest from ophyd.device import Device as OphydV1Device +from ophyd_async.core import Device from ophyd_async.core import Device as OphydV2Device from ophyd_async.sim import SimMotor from pytest import RaisesExc, RaisesGroup @@ -420,7 +421,7 @@ def test_kwargs_factory(dm: DeviceManager): # Factories can have kwargs but they're ignored by the device manager @dm.factory - def foo(**kwargs): + def foo(**kwargs) -> Device: # type: ignore s1(**kwargs) dm.build_all(s1) From e4740e6a2bf1c4f6b57ad34b629778c78522f28c Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 30 Oct 2025 16:00:59 +0000 Subject: [PATCH 67/74] Example conversion to the new device manager --- src/dodal/beamlines/i03.py | 153 ++++++++++++++++++------------------- 1 file changed, 73 insertions(+), 80 deletions(-) diff --git a/src/dodal/beamlines/i03.py b/src/dodal/beamlines/i03.py index 30133e4b162..9f2d1c19e9a 100644 --- a/src/dodal/beamlines/i03.py +++ b/src/dodal/beamlines/i03.py @@ -1,17 +1,14 @@ -from ophyd_async.core import Reference +from functools import cache + +from ophyd_async.core import PathProvider, Reference from ophyd_async.fastcs.eiger import EigerDetector as FastEiger from ophyd_async.fastcs.panda import HDFPanda from yarl import URL from dodal.common.beamlines.beamline_parameters import get_beamline_parameters -from dodal.common.beamlines.beamline_utils import ( - device_factory, - device_instantiation, - get_path_provider, - set_path_provider, -) from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.common.udc_directory_provider import PandASubpathProvider +from dodal.device_manager import DeviceManager from dodal.devices.aperturescatterguard import ( AperturePosition, ApertureScatterguard, @@ -71,8 +68,6 @@ set_log_beamline(BL) set_utils_beamline(BL) -set_path_provider(PandASubpathProvider()) - I03_ZEBRA_MAPPING = ZebraMapping( outputs=ZebraTTLOutputs(TTL_DETECTOR=1, TTL_SHUTTER=2, TTL_XSPRESS3=3, TTL_PANDA=4), sources=ZebraSources(), @@ -80,8 +75,21 @@ PREFIX = BeamlinePrefix(BL) +devices = DeviceManager() -@device_factory() + +@devices.fixture +@cache +def path_provider() -> PathProvider: + return PandASubpathProvider() + + +@devices.fixture +def daq_configuration_path() -> str: + return DAQ_CONFIGURATION_PATH + + +@devices.factory() def aperture_scatterguard() -> ApertureScatterguard: """Get the i03 aperture and scatterguard device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing @@ -96,7 +104,7 @@ def aperture_scatterguard() -> ApertureScatterguard: ) -@device_factory() +@devices.factory() def attenuator() -> BinaryFilterAttenuator: """Get the i03 attenuator device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -107,7 +115,7 @@ def attenuator() -> BinaryFilterAttenuator: ) -@device_factory() +@devices.factory() def beamstop() -> Beamstop: """Get the i03 beamstop device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -118,7 +126,7 @@ def beamstop() -> Beamstop: ) -@device_factory() +@devices.factory() def dcm() -> DCM: """Get the i03 DCM device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -126,7 +134,7 @@ def dcm() -> DCM: return DCM(prefix=f"{PREFIX.beamline_prefix}-MO-DCM-01:") -@device_factory() +@devices.factory() def vfm() -> FocusingMirrorWithStripes: return FocusingMirrorWithStripes( prefix=f"{PREFIX.beamline_prefix}-OP-VFM-01:", @@ -137,7 +145,7 @@ def vfm() -> FocusingMirrorWithStripes: ) -@device_factory() +@devices.factory() def mirror_voltages() -> MirrorVoltages: return MirrorVoltages( prefix=f"{PREFIX.beamline_prefix}-MO-PSU-01:", @@ -145,7 +153,7 @@ def mirror_voltages() -> MirrorVoltages: ) -@device_factory() +@devices.factory() def backlight() -> Backlight: """Get the i03 backlight device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -153,7 +161,7 @@ def backlight() -> Backlight: return Backlight(prefix=PREFIX.beamline_prefix) -@device_factory() +@devices.factory() def detector_motion() -> DetectorMotion: """Get the i03 detector motion device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -164,36 +172,26 @@ def detector_motion() -> DetectorMotion: ) -@device_factory() -def eiger(mock: bool = False) -> EigerDetector: - """Get the i03 Eiger device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ +@devices.v1_init(EigerDetector, prefix="BL03I-EA-EIGER-01:", wait=False) +def eiger(eiger: EigerDetector) -> EigerDetector: + return eiger - return device_instantiation( - device_factory=EigerDetector, - name="eiger", - prefix="-EA-EIGER-01:", - wait=False, - fake=mock, - ) - -@device_factory() -def fastcs_eiger() -> FastEiger: +@devices.factory() +def fastcs_eiger(path_provider: PathProvider) -> FastEiger: """Get the i03 FastCS Eiger device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. """ return FastEiger( prefix=PREFIX.beamline_prefix, - path_provider=get_path_provider(), + path_provider=path_provider, drv_suffix="-EA-EIGER-02:", hdf_suffix="-EA-EIGER-01:OD:", ) -@device_factory() +@devices.factory() def zebra_fast_grid_scan() -> ZebraFastGridScanThreeD: """Get the i03 zebra_fast_grid_scan device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -203,7 +201,7 @@ def zebra_fast_grid_scan() -> ZebraFastGridScanThreeD: ) -@device_factory() +@devices.factory() def panda_fast_grid_scan() -> PandAFastGridScan: """Get the i03 panda_fast_grid_scan device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -212,7 +210,7 @@ def panda_fast_grid_scan() -> PandAFastGridScan: return PandAFastGridScan(prefix=f"{PREFIX.beamline_prefix}-MO-SGON-01:") -@device_factory() +@devices.factory() def oav( params: OAVConfigBeamCentre | None = None, ) -> OAVBeamCentreFile: @@ -225,7 +223,7 @@ def oav( ) -@device_factory() +@devices.factory() def pin_tip_detection() -> PinTipDetection: """Get the i03 pin tip detection device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -233,7 +231,7 @@ def pin_tip_detection() -> PinTipDetection: return PinTipDetection(f"{PREFIX.beamline_prefix}-DI-OAV-01:") -@device_factory() +@devices.factory() def smargon() -> Smargon: """Get the i03 Smargon device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -241,7 +239,7 @@ def smargon() -> Smargon: return Smargon(f"{PREFIX.beamline_prefix}-MO-SGON-01:") -@device_factory() +@devices.factory() def s4_slit_gaps() -> S4SlitGaps: """Get the i03 s4_slit_gaps device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -249,7 +247,7 @@ def s4_slit_gaps() -> S4SlitGaps: return S4SlitGaps(f"{PREFIX.beamline_prefix}-AL-SLITS-04:") -@device_factory() +@devices.factory() def synchrotron() -> Synchrotron: """Get the i03 synchrotron device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -257,38 +255,33 @@ def synchrotron() -> Synchrotron: return Synchrotron() -@device_factory() -def undulator(daq_configuration_path: str | None = None) -> UndulatorInKeV: +@devices.factory() +def undulator(daq_configuration_path: str) -> UndulatorInKeV: """Get the i03 undulator device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. """ return UndulatorInKeV( f"{BeamlinePrefix(BL).insertion_prefix}-MO-SERVC-01:", - # evaluate here not as parameter default to enable post-import mocking - id_gap_lookup_table_path=f"{daq_configuration_path or DAQ_CONFIGURATION_PATH}/lookup/BeamLine_Undulator_toGap.txt", + id_gap_lookup_table_path=f"{daq_configuration_path}/lookup/BeamLine_Undulator_toGap.txt", baton=baton(), ) -@device_factory() -def undulator_dcm(daq_configuration_path: str | None = None) -> UndulatorDCM: +@devices.factory() +def undulator_dcm( + undulator: UndulatorInKeV, daq_configuration_path: str +) -> UndulatorDCM: """Get the i03 undulator DCM device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. """ - # evaluate here not as parameter default to enable post-import mocking - undulator_singleton = ( - undulator(daq_configuration_path=daq_configuration_path) - if daq_configuration_path and daq_configuration_path != DAQ_CONFIGURATION_PATH - else undulator() - ) return UndulatorDCM( - undulator=undulator_singleton, + undulator=undulator, dcm=dcm(), - daq_configuration_path=daq_configuration_path or DAQ_CONFIGURATION_PATH, + daq_configuration_path=daq_configuration_path, ) -@device_factory() +@devices.factory() def zebra() -> Zebra: """Get the i03 zebra device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -299,7 +292,7 @@ def zebra() -> Zebra: ) -@device_factory() +@devices.factory() def xspress3mini() -> Xspress3: """Get the i03 Xspress3Mini device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -307,18 +300,18 @@ def xspress3mini() -> Xspress3: return Xspress3(f"{PREFIX.beamline_prefix}-EA-XSP3-01:") -@device_factory() -def panda() -> HDFPanda: +@devices.factory() +def panda(path_provider: PathProvider) -> HDFPanda: """Get the i03 panda device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. """ return HDFPanda( f"{PREFIX.beamline_prefix}-EA-PANDA-01:", - path_provider=get_path_provider(), + path_provider=path_provider, ) -@device_factory() +@devices.factory() def sample_shutter() -> ZebraShutter: """Get the i03 sample shutter device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -326,7 +319,7 @@ def sample_shutter() -> ZebraShutter: return ZebraShutter(f"{PREFIX.beamline_prefix}-EA-SHTR-01:") -@device_factory() +@devices.factory() def hutch_shutter() -> HutchShutter: """Get the i03 hutch shutter device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -334,7 +327,7 @@ def hutch_shutter() -> HutchShutter: return HutchShutter(f"{PREFIX.beamline_prefix}-PS-SHTR-01:") -@device_factory() +@devices.factory() def flux() -> Flux: """Get the i03 flux device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -342,15 +335,15 @@ def flux() -> Flux: return Flux(f"{PREFIX.beamline_prefix}-MO-FLUX-01:") -@device_factory() -def xbpm_feedback() -> XBPMFeedback: +@devices.factory() +def xbpm_feedback(baton: Baton) -> XBPMFeedback: """Get the i03 XBPM feeback device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. """ - return XBPMFeedback(f"{PREFIX.beamline_prefix}-EA-FDBK-01:", baton=baton()) + return XBPMFeedback(f"{PREFIX.beamline_prefix}-EA-FDBK-01:", baton=baton) -@device_factory() +@devices.factory() def zocalo() -> ZocaloResults: """Get the i03 ZocaloResults device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -358,7 +351,7 @@ def zocalo() -> ZocaloResults: return ZocaloResults(results_source=ZocaloSource.GPU) -@device_factory() +@devices.factory() def robot() -> BartRobot: """Get the i03 robot device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -366,7 +359,7 @@ def robot() -> BartRobot: return BartRobot(f"{PREFIX.beamline_prefix}-MO-ROBOT-01:") -@device_factory() +@devices.factory() def webcam() -> Webcam: """Get the i03 webcam, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -374,7 +367,7 @@ def webcam() -> Webcam: return Webcam(url=URL("http://i03-webcam1/axis-cgi/jpg/image.cgi")) -@device_factory() +@devices.factory() def thawer() -> Thawer: """Get the i03 thawer, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -382,7 +375,7 @@ def thawer() -> Thawer: return Thawer(f"{PREFIX.beamline_prefix}-EA-THAW-01") -@device_factory() +@devices.factory() def lower_gonio() -> XYZStage: """Get the i03 lower gonio device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -390,7 +383,7 @@ def lower_gonio() -> XYZStage: return XYZStage(f"{PREFIX.beamline_prefix}-MO-GONP-01:") -@device_factory() +@devices.factory() def cryostream() -> CryoStream: """Get the i03 cryostream device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -398,7 +391,7 @@ def cryostream() -> CryoStream: return CryoStream(PREFIX.beamline_prefix) -@device_factory() +@devices.factory() def cryostream_gantry() -> CryoStreamGantry: """Get the i03 cryostream gantry device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -406,7 +399,7 @@ def cryostream_gantry() -> CryoStreamGantry: return CryoStreamGantry(PREFIX.beamline_prefix) -@device_factory() +@devices.factory() def diamond_filter() -> DiamondFilter[I03Filters]: """Get the i03 diamond filter device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -416,7 +409,7 @@ def diamond_filter() -> DiamondFilter[I03Filters]: ) -@device_factory() +@devices.factory() def qbpm() -> QBPM: """Get the i03 qbpm device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -424,7 +417,7 @@ def qbpm() -> QBPM: return QBPM(f"{PREFIX.beamline_prefix}-DI-QBPM-01:") -@device_factory() +@devices.factory() def baton() -> Baton: """Get the i03 baton device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -432,7 +425,7 @@ def baton() -> Baton: return Baton(f"{PREFIX.beamline_prefix}-CS-BATON-01:") -@device_factory() +@devices.factory() def fluorescence_det_motion() -> FluorescenceDetector: """Get the i03 device for moving the fluorescence detector, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. @@ -440,19 +433,19 @@ def fluorescence_det_motion() -> FluorescenceDetector: return FluorescenceDetector(f"{PREFIX.beamline_prefix}-EA-FLU-01:") -@device_factory() -def scintillator() -> Scintillator: +@devices.factory() +def scintillator(aperture_scatterguard: ApertureScatterguard) -> Scintillator: """Get the i03 scintillator device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. """ return Scintillator( f"{PREFIX.beamline_prefix}-MO-SCIN-01:", - Reference(aperture_scatterguard()), + Reference(aperture_scatterguard), get_beamline_parameters(), ) -@device_factory() +@devices.factory() def collimation_table() -> CollimationTable: """Get the i03 device for moving the collimation table, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. From 62ed2da81c959442428de1b900f01bfe11345427 Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Tue, 18 Nov 2025 12:29:53 +0000 Subject: [PATCH 68/74] Fix device heirachy --- src/dodal/beamlines/i03.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dodal/beamlines/i03.py b/src/dodal/beamlines/i03.py index 9f2d1c19e9a..fff24753f7a 100644 --- a/src/dodal/beamlines/i03.py +++ b/src/dodal/beamlines/i03.py @@ -256,27 +256,27 @@ def synchrotron() -> Synchrotron: @devices.factory() -def undulator(daq_configuration_path: str) -> UndulatorInKeV: +def undulator(baton: Baton, daq_configuration_path: str) -> UndulatorInKeV: """Get the i03 undulator device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. """ return UndulatorInKeV( f"{BeamlinePrefix(BL).insertion_prefix}-MO-SERVC-01:", id_gap_lookup_table_path=f"{daq_configuration_path}/lookup/BeamLine_Undulator_toGap.txt", - baton=baton(), + baton=baton, ) @devices.factory() def undulator_dcm( - undulator: UndulatorInKeV, daq_configuration_path: str + undulator: UndulatorInKeV, dcm: DCM, daq_configuration_path: str ) -> UndulatorDCM: """Get the i03 undulator DCM device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. """ return UndulatorDCM( undulator=undulator, - dcm=dcm(), + dcm=dcm, daq_configuration_path=daq_configuration_path, ) From 613bb090bb56d49b2b855452e9bc967693254d77 Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Tue, 18 Nov 2025 13:57:22 +0000 Subject: [PATCH 69/74] Fix some tests --- ...nfigure_arm_trigger_and_disarm_detector.py | 6 +-- tests/test_utils.py | 44 ++++++------------- 2 files changed, 16 insertions(+), 34 deletions(-) diff --git a/src/dodal/plans/configure_arm_trigger_and_disarm_detector.py b/src/dodal/plans/configure_arm_trigger_and_disarm_detector.py index 91dc4d89cfe..808a42c9c70 100644 --- a/src/dodal/plans/configure_arm_trigger_and_disarm_detector.py +++ b/src/dodal/plans/configure_arm_trigger_and_disarm_detector.py @@ -12,7 +12,7 @@ ) from ophyd_async.fastcs.eiger import EigerDetector -from dodal.beamlines.i03 import fastcs_eiger, set_path_provider +from dodal.beamlines.i03 import fastcs_eiger from dodal.devices.detector import DetectorParams from dodal.log import LOGGER, do_default_logging_setup @@ -163,9 +163,7 @@ def set_mx_settings_pvs( PurePath("/dls/i03/data/2025/cm40607-2/test_new_eiger/"), ) - set_path_provider(path_provider) - - eiger = fastcs_eiger(connect_immediately=True) + eiger = fastcs_eiger.build(connect_immediately=True, path_provider=path_provider) run_engine( configure_arm_trigger_and_disarm_detector( eiger=eiger, diff --git a/tests/test_utils.py b/tests/test_utils.py index bedefc5cdca..89aeb9d561b 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -8,7 +8,7 @@ from bluesky.protocols import Readable from ophyd_async.epics.motor import Motor -from dodal.beamlines import i03, i23 +from dodal.beamlines import i04, i23 from dodal.devices.diamond_filter import DiamondFilter, I03Filters from dodal.utils import ( AnyDevice, @@ -313,7 +313,7 @@ def test_invalid_beamline_variable_causes_get_device_module_to_raise(bl): get_beamline_based_on_environment_variable() -@pytest.mark.parametrize("bl,module", [("i03", i03), ("i23", i23)]) +@pytest.mark.parametrize("bl,module", [("i04", i04), ("i23", i23)]) def test_valid_beamline_variable_causes_get_device_module_to_return_module(bl, module): with patch.dict(os.environ, {"BEAMLINE": bl}): assert get_beamline_based_on_environment_variable() == module @@ -488,40 +488,24 @@ def test_is_v2_device_type(input: Any, expected_result: bool): def test_calling_factory_with_different_args_raises_an_exception(): - i03.undulator(daq_configuration_path=MOCK_DAQ_CONFIG_PATH) + i04.oav(params=MagicMock()) with pytest.raises( RuntimeError, match="Device factory method called multiple times with different parameters", ): - i03.undulator(daq_configuration_path=MOCK_DAQ_CONFIG_PATH + "x") - i03.undulator.cache_clear() + i04.oav(params=MagicMock()) + i04.oav.cache_clear() -def test_calling_factory_with_different_args_does_not_raise_an_exception_after_cache_clear( - alternate_config, -): - i03.undulator(daq_configuration_path=MOCK_DAQ_CONFIG_PATH) - i03.undulator.cache_clear() - i03.undulator(daq_configuration_path=alternate_config) - i03.undulator.cache_clear() - - -def test_factories_can_be_called_in_any_order(alternate_config): - i03.undulator_dcm(daq_configuration_path=alternate_config) - i03.undulator(daq_configuration_path=alternate_config) - - i03.undulator_dcm.cache_clear() - i03.undulator.cache_clear() - - i03.undulator(daq_configuration_path=alternate_config) - i03.undulator_dcm(daq_configuration_path=alternate_config) - - i03.undulator.cache_clear() - i03.undulator_dcm.cache_clear() +def test_calling_factory_with_different_args_does_not_raise_an_exception_after_cache_clear(): + i04.oav(params=MagicMock()) + i04.oav.cache_clear() + i04.oav(params=MagicMock()) + i04.oav.cache_clear() -def test_factory_calls_are_cached(alternate_config): - undulator1 = i03.undulator(daq_configuration_path=alternate_config) - undulator2 = i03.undulator(daq_configuration_path=alternate_config) +def test_factory_calls_are_cached(): + undulator1 = i04.undulator() + undulator2 = i04.undulator() assert undulator1 is undulator2 - i03.undulator.cache_clear() + i04.undulator.cache_clear() From 0c7d7895f1566bfa0a28dc2a18ee8d190e1839c9 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 19 Nov 2025 11:46:13 +0000 Subject: [PATCH 70/74] Ignore device_manager factories in utils --- src/dodal/utils.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/dodal/utils.py b/src/dodal/utils.py index 9be4675e23e..2677bf0bf4a 100644 --- a/src/dodal/utils.py +++ b/src/dodal/utils.py @@ -11,7 +11,7 @@ from importlib import import_module from inspect import signature from os import environ -from types import ModuleType +from types import FunctionType, ModuleType from typing import ( Any, Generic, @@ -413,7 +413,9 @@ def is_v2_device_factory(func: Callable) -> TypeGuard[V2DeviceFactory]: def is_any_device_factory(func: Callable) -> TypeGuard[AnyDeviceFactory]: - return is_v1_device_factory(func) or is_v2_device_factory(func) + return isinstance(func, (FunctionType, DeviceInitializationController)) and ( + is_v1_device_factory(func) or is_v2_device_factory(func) + ) def is_v2_device_type(obj: type[Any]) -> bool: From b9b096f9141d8f7663ed60c4ec2abc88469a952c Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Fri, 21 Nov 2025 12:21:04 +0000 Subject: [PATCH 71/74] Fix test_device_instantiation --- src/dodal/utils.py | 5 +++-- .../beamlines/test_device_instantiation.py | 9 +++++++-- tests/conftest.py | 20 ++++++++++++++----- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/dodal/utils.py b/src/dodal/utils.py index 2677bf0bf4a..8b6ebe649b8 100644 --- a/src/dodal/utils.py +++ b/src/dodal/utils.py @@ -240,7 +240,8 @@ def make_device( def make_all_devices( module: str | ModuleType | None = None, include_skipped: bool = False, **kwargs ) -> tuple[dict[str, AnyDevice], dict[str, Exception]]: - """Makes all devices in the given beamline module. + """Makes all devices in the given beamline module, for those modules using device factories + as opposed to the DeviceManager. In cases of device interdependencies it ensures a device is created before any which depend on it. @@ -413,7 +414,7 @@ def is_v2_device_factory(func: Callable) -> TypeGuard[V2DeviceFactory]: def is_any_device_factory(func: Callable) -> TypeGuard[AnyDeviceFactory]: - return isinstance(func, (FunctionType, DeviceInitializationController)) and ( + return isinstance(func, FunctionType | DeviceInitializationController) and ( is_v1_device_factory(func) or is_v2_device_factory(func) ) diff --git a/tests/common/beamlines/test_device_instantiation.py b/tests/common/beamlines/test_device_instantiation.py index 4f9bc96b1c9..7ce915f5ce4 100644 --- a/tests/common/beamlines/test_device_instantiation.py +++ b/tests/common/beamlines/test_device_instantiation.py @@ -4,6 +4,7 @@ from ophyd_async.core import NotConnectedError from dodal.beamlines import all_beamline_modules +from dodal.device_manager import DeviceManager from dodal.utils import BLUESKY_PROTOCOLS, make_all_devices @@ -44,6 +45,10 @@ def test_devices_are_identical(module_and_devices_for_beamline): Ensures that for every beamline all device functions prevent duplicate instantiation. """ bl_mod, devices_a, _ = module_and_devices_for_beamline + if isinstance(getattr(bl_mod, "devices", None), DeviceManager): + # DeviceManager beamline modules do not cache device instances + return + devices_b, _ = make_all_devices( bl_mod, include_skipped=True, @@ -55,7 +60,7 @@ def test_devices_are_identical(module_and_devices_for_beamline): if device is not devices_b[device_name] ] total_number_of_devices = len(devices_a) - non_identical_number_of_devies = len(devices_a) + non_identical_number_of_devices = len(devices_a) assert len(non_identical_names) == 0, ( - f"{non_identical_number_of_devies}/{total_number_of_devices} devices were not identical: {non_identical_names}" + f"{non_identical_number_of_devices}/{total_number_of_devices} devices were not identical: {non_identical_names}" ) diff --git a/tests/conftest.py b/tests/conftest.py index fe4b41820ef..b326effebac 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -11,6 +11,7 @@ from conftest import mock_attributes_table from dodal.common.beamlines import beamline_parameters, beamline_utils from dodal.common.beamlines.commissioning_mode import set_commissioning_signal +from dodal.device_manager import DeviceManager from dodal.devices.baton import Baton from dodal.devices.detector import DetectorParams from dodal.devices.detector.det_dim_constants import EIGER2_X_16M_SIZE @@ -32,11 +33,20 @@ def module_and_devices_for_beamline(request: pytest.FixtureRequest): with patch.dict(os.environ, {"BEAMLINE": beamline}, clear=True): bl_mod = importlib.import_module("dodal.beamlines." + beamline) mock_beamline_module_filepaths(beamline, bl_mod) - devices, exceptions = make_all_devices( - bl_mod, - include_skipped=True, - fake_with_ophyd_sim=True, - ) + if isinstance( + device_manager := getattr(bl_mod, "devices", None), DeviceManager + ): + result = device_manager.build_all(include_skipped=True, mock=True).connect() + devices, exceptions = ( + result.devices, + result.connection_errors | result.build_errors, + ) + else: + devices, exceptions = make_all_devices( + bl_mod, + include_skipped=True, + fake_with_ophyd_sim=True, + ) yield (bl_mod, devices, exceptions) beamline_utils.clear_devices() for factory in collect_factories(bl_mod).values(): From aa3882ca9b26539262d0440ee12005b5e5915a0f Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Fri, 21 Nov 2025 13:29:37 +0000 Subject: [PATCH 72/74] Update beamline howto docs --- docs/how-to/create-beamline.md | 36 ++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/docs/how-to/create-beamline.md b/docs/how-to/create-beamline.md index 919d856457b..228d811fb81 100644 --- a/docs/how-to/create-beamline.md +++ b/docs/how-to/create-beamline.md @@ -23,28 +23,31 @@ The following example creates a fictitious beamline ``w41``, with a simulated tw ```python + from functools import cache + from pathlib import Path + from ophyd_async.core import PathProvider from ophyd_async.epics.adaravis import AravisDetector - from dodal.common.beamlines.beamline_utils import ( - device_factory, - get_path_provider, - set_path_provider, - ) from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.common.beamlines.device_helpers import CAM_SUFFIX, HDF5_SUFFIX - from dodal.common.visit import LocalDirectoryServiceClient, StaticVisitPathProvider + from dodal.common.visit import RemoteDirectoryServiceClient, StaticVisitPathProvider + from dodal.device_manager import DeviceManager from dodal.devices.synchrotron import Synchrotron from dodal.log import set_beamline as set_log_beamline - from dodal.utils import BeamlinePrefix + from dodal.utils import BeamlinePrefix, get_beamline_name BL = get_beamline_name("s41") # Default used when not on a live beamline PREFIX = BeamlinePrefix(BL) set_log_beamline(BL) # Configure logging and util functions set_utils_beamline(BL) - # Currently we must hard-code the visit, determining the visit is WIP. - set_path_provider( - StaticVisitPathProvider( + devices = DeviceManager() + + @devices.fixture + @cache + def path_provider() -> PathProvider: + # Currently we must hard-code the visit, determining the visit is WIP. + return StaticVisitPathProvider( BL, # Root directory for all detectors Path("/dls/w41/data/YYYY/cm12345-1"), @@ -52,7 +55,7 @@ The following example creates a fictitious beamline ``w41``, with a simulated tw client=RemoteDirectoryServiceClient("http://s41-control:8088/api"), # Else if no GDA server use a LocalDirectoryServiceClient(), ) - ) + """ Define device factory functions below this point. @@ -63,23 +66,22 @@ The following example creates a fictitious beamline ``w41``, with a simulated tw """ This decorator gives extra desirable behaviour to this factory function: - it may be instantiated automatically, selectively on live beamline - - caching and re-using the result for subsequent calls - it automatically names the device if no name is explicitly set - it may be skipped when make_all_devices is called on this module - it must be explicitly connected (which may be automated by tools) - when connected it may connect to a simulated backend - it may be connected concurrently (when automated by tools) - """" - @device_factory(skip = BL == "s41") + """ + @devices.factory(skip = BL == "s41") def synchrotron() -> Synchrotron: return Synchrotron() - @device_factory() - def d11() -> AravisDetector: + @devices.factory() + def d11(path_provider: PathProvider) -> AravisDetector: return AravisDetector( f"{PREFIX.beamline_prefix}-DI-DCAM-01:", - path_provider=get_path_provider(), + path_provider=path_provider, drv_suffix=CAM_SUFFIX, fileio_suffix=HDF5_SUFFIX, ) From c943f7a0080888b8fa251e2271590c562f6052a0 Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Thu, 27 Nov 2025 13:31:49 +0000 Subject: [PATCH 73/74] Fix merge --- src/dodal/beamlines/i03.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/dodal/beamlines/i03.py b/src/dodal/beamlines/i03.py index fff24753f7a..676c7cc0b10 100644 --- a/src/dodal/beamlines/i03.py +++ b/src/dodal/beamlines/i03.py @@ -453,17 +453,15 @@ def collimation_table() -> CollimationTable: return CollimationTable(prefix=f"{PREFIX.beamline_prefix}-MO-TABLE-01") -@device_factory() -def beamsize() -> Beamsize: +@devices.factory() +def beamsize(aperture_scatterguard: ApertureScatterguard) -> Beamsize: """Get the i03 beamsize device, instantiate it if it hasn't already been. If this is called when already instantiated in i03, it will return the existing object. """ - return Beamsize( - aperture_scatterguard=aperture_scatterguard(), - ) + return Beamsize(aperture_scatterguard) -@device_factory() +@devices.factory() def ipin() -> IPin: """Get the i03 ipin device, instantiate it if it hasn't already been. If this is called when already instantiated in i04, it will return the existing object. From 97189cbee61c02269066954108924c4d7793d322 Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Thu, 27 Nov 2025 17:34:41 +0000 Subject: [PATCH 74/74] Remove spurious comments --- src/dodal/beamlines/i03.py | 118 +------------------------------------ 1 file changed, 1 insertion(+), 117 deletions(-) diff --git a/src/dodal/beamlines/i03.py b/src/dodal/beamlines/i03.py index 676c7cc0b10..14e35d185e7 100644 --- a/src/dodal/beamlines/i03.py +++ b/src/dodal/beamlines/i03.py @@ -91,10 +91,6 @@ def daq_configuration_path() -> str: @devices.factory() def aperture_scatterguard() -> ApertureScatterguard: - """Get the i03 aperture and scatterguard device, instantiate it if it hasn't already - been. If this is called when already instantiated in i03, it will return the existing - object. - """ params = get_beamline_parameters() return ApertureScatterguard( aperture_prefix=f"{PREFIX.beamline_prefix}-MO-MAPT-01:", @@ -106,9 +102,6 @@ def aperture_scatterguard() -> ApertureScatterguard: @devices.factory() def attenuator() -> BinaryFilterAttenuator: - """Get the i03 attenuator device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return BinaryFilterAttenuator( prefix=f"{PREFIX.beamline_prefix}-EA-ATTN-01:", num_filters=16, @@ -117,9 +110,6 @@ def attenuator() -> BinaryFilterAttenuator: @devices.factory() def beamstop() -> Beamstop: - """Get the i03 beamstop device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Beamstop( prefix=f"{PREFIX.beamline_prefix}-MO-BS-01:", beamline_parameters=get_beamline_parameters(), @@ -128,9 +118,6 @@ def beamstop() -> Beamstop: @devices.factory() def dcm() -> DCM: - """Get the i03 DCM device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return DCM(prefix=f"{PREFIX.beamline_prefix}-MO-DCM-01:") @@ -155,17 +142,11 @@ def mirror_voltages() -> MirrorVoltages: @devices.factory() def backlight() -> Backlight: - """Get the i03 backlight device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Backlight(prefix=PREFIX.beamline_prefix) @devices.factory() def detector_motion() -> DetectorMotion: - """Get the i03 detector motion device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return DetectorMotion( device_prefix=f"{PREFIX.beamline_prefix}-MO-DET-01:", pmac_prefix=f"{PREFIX.beamline_prefix}-MO-PMAC-02:", @@ -179,10 +160,6 @@ def eiger(eiger: EigerDetector) -> EigerDetector: @devices.factory() def fastcs_eiger(path_provider: PathProvider) -> FastEiger: - """Get the i03 FastCS Eiger device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ - return FastEiger( prefix=PREFIX.beamline_prefix, path_provider=path_provider, @@ -193,9 +170,6 @@ def fastcs_eiger(path_provider: PathProvider) -> FastEiger: @devices.factory() def zebra_fast_grid_scan() -> ZebraFastGridScanThreeD: - """Get the i03 zebra_fast_grid_scan device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return ZebraFastGridScanThreeD( prefix=f"{PREFIX.beamline_prefix}-MO-SGON-01:", ) @@ -203,10 +177,7 @@ def zebra_fast_grid_scan() -> ZebraFastGridScanThreeD: @devices.factory() def panda_fast_grid_scan() -> PandAFastGridScan: - """Get the i03 panda_fast_grid_scan device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - This is used instead of the zebra_fast_grid_scan device when using the PandA. - """ + """This is used instead of the zebra_fast_grid_scan device when using the PandA.""" return PandAFastGridScan(prefix=f"{PREFIX.beamline_prefix}-MO-SGON-01:") @@ -214,9 +185,6 @@ def panda_fast_grid_scan() -> PandAFastGridScan: def oav( params: OAVConfigBeamCentre | None = None, ) -> OAVBeamCentreFile: - """Get the i03 OAV device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return OAVBeamCentreFile( prefix=f"{PREFIX.beamline_prefix}-DI-OAV-01:", config=params or OAVConfigBeamCentre(ZOOM_PARAMS_FILE, DISPLAY_CONFIG), @@ -225,41 +193,26 @@ def oav( @devices.factory() def pin_tip_detection() -> PinTipDetection: - """Get the i03 pin tip detection device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return PinTipDetection(f"{PREFIX.beamline_prefix}-DI-OAV-01:") @devices.factory() def smargon() -> Smargon: - """Get the i03 Smargon device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Smargon(f"{PREFIX.beamline_prefix}-MO-SGON-01:") @devices.factory() def s4_slit_gaps() -> S4SlitGaps: - """Get the i03 s4_slit_gaps device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return S4SlitGaps(f"{PREFIX.beamline_prefix}-AL-SLITS-04:") @devices.factory() def synchrotron() -> Synchrotron: - """Get the i03 synchrotron device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Synchrotron() @devices.factory() def undulator(baton: Baton, daq_configuration_path: str) -> UndulatorInKeV: - """Get the i03 undulator device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return UndulatorInKeV( f"{BeamlinePrefix(BL).insertion_prefix}-MO-SERVC-01:", id_gap_lookup_table_path=f"{daq_configuration_path}/lookup/BeamLine_Undulator_toGap.txt", @@ -271,9 +224,6 @@ def undulator(baton: Baton, daq_configuration_path: str) -> UndulatorInKeV: def undulator_dcm( undulator: UndulatorInKeV, dcm: DCM, daq_configuration_path: str ) -> UndulatorDCM: - """Get the i03 undulator DCM device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return UndulatorDCM( undulator=undulator, dcm=dcm, @@ -283,9 +233,6 @@ def undulator_dcm( @devices.factory() def zebra() -> Zebra: - """Get the i03 zebra device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Zebra( prefix=f"{PREFIX.beamline_prefix}-EA-ZEBRA-01:", mapping=I03_ZEBRA_MAPPING, @@ -294,17 +241,11 @@ def zebra() -> Zebra: @devices.factory() def xspress3mini() -> Xspress3: - """Get the i03 Xspress3Mini device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Xspress3(f"{PREFIX.beamline_prefix}-EA-XSP3-01:") @devices.factory() def panda(path_provider: PathProvider) -> HDFPanda: - """Get the i03 panda device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return HDFPanda( f"{PREFIX.beamline_prefix}-EA-PANDA-01:", path_provider=path_provider, @@ -313,97 +254,61 @@ def panda(path_provider: PathProvider) -> HDFPanda: @devices.factory() def sample_shutter() -> ZebraShutter: - """Get the i03 sample shutter device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return ZebraShutter(f"{PREFIX.beamline_prefix}-EA-SHTR-01:") @devices.factory() def hutch_shutter() -> HutchShutter: - """Get the i03 hutch shutter device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return HutchShutter(f"{PREFIX.beamline_prefix}-PS-SHTR-01:") @devices.factory() def flux() -> Flux: - """Get the i03 flux device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Flux(f"{PREFIX.beamline_prefix}-MO-FLUX-01:") @devices.factory() def xbpm_feedback(baton: Baton) -> XBPMFeedback: - """Get the i03 XBPM feeback device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return XBPMFeedback(f"{PREFIX.beamline_prefix}-EA-FDBK-01:", baton=baton) @devices.factory() def zocalo() -> ZocaloResults: - """Get the i03 ZocaloResults device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return ZocaloResults(results_source=ZocaloSource.GPU) @devices.factory() def robot() -> BartRobot: - """Get the i03 robot device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return BartRobot(f"{PREFIX.beamline_prefix}-MO-ROBOT-01:") @devices.factory() def webcam() -> Webcam: - """Get the i03 webcam, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Webcam(url=URL("http://i03-webcam1/axis-cgi/jpg/image.cgi")) @devices.factory() def thawer() -> Thawer: - """Get the i03 thawer, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Thawer(f"{PREFIX.beamline_prefix}-EA-THAW-01") @devices.factory() def lower_gonio() -> XYZStage: - """Get the i03 lower gonio device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return XYZStage(f"{PREFIX.beamline_prefix}-MO-GONP-01:") @devices.factory() def cryostream() -> CryoStream: - """Get the i03 cryostream device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return CryoStream(PREFIX.beamline_prefix) @devices.factory() def cryostream_gantry() -> CryoStreamGantry: - """Get the i03 cryostream gantry device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return CryoStreamGantry(PREFIX.beamline_prefix) @devices.factory() def diamond_filter() -> DiamondFilter[I03Filters]: - """Get the i03 diamond filter device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return DiamondFilter[I03Filters]( f"{PREFIX.beamline_prefix}-MO-FLTR-01:Y", I03Filters ) @@ -411,33 +316,21 @@ def diamond_filter() -> DiamondFilter[I03Filters]: @devices.factory() def qbpm() -> QBPM: - """Get the i03 qbpm device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return QBPM(f"{PREFIX.beamline_prefix}-DI-QBPM-01:") @devices.factory() def baton() -> Baton: - """Get the i03 baton device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Baton(f"{PREFIX.beamline_prefix}-CS-BATON-01:") @devices.factory() def fluorescence_det_motion() -> FluorescenceDetector: - """Get the i03 device for moving the fluorescence detector, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return FluorescenceDetector(f"{PREFIX.beamline_prefix}-EA-FLU-01:") @devices.factory() def scintillator(aperture_scatterguard: ApertureScatterguard) -> Scintillator: - """Get the i03 scintillator device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Scintillator( f"{PREFIX.beamline_prefix}-MO-SCIN-01:", Reference(aperture_scatterguard), @@ -447,23 +340,14 @@ def scintillator(aperture_scatterguard: ApertureScatterguard) -> Scintillator: @devices.factory() def collimation_table() -> CollimationTable: - """Get the i03 device for moving the collimation table, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return CollimationTable(prefix=f"{PREFIX.beamline_prefix}-MO-TABLE-01") @devices.factory() def beamsize(aperture_scatterguard: ApertureScatterguard) -> Beamsize: - """Get the i03 beamsize device, instantiate it if it hasn't already been. - If this is called when already instantiated in i03, it will return the existing object. - """ return Beamsize(aperture_scatterguard) @devices.factory() def ipin() -> IPin: - """Get the i03 ipin device, instantiate it if it hasn't already been. - If this is called when already instantiated in i04, it will return the existing object. - """ return IPin(f"{PREFIX.beamline_prefix}-EA-PIN-01:")