DiamondLightSource / tickit

Event-based hardware simulation framework
Apache License 2.0
7 stars 0 forks source link

Make user interface for components more friendly #99

Closed abbiemery closed 1 year ago

abbiemery commented 2 years ago

Currently to set up a component you have to override the __call__ method of ComponentConfig. This is user facing so should be made to avoid a dunder method.

Maybe something like:

@component
def my_detector(thing: int, other_thing: str) -> Component:
    return DeviceSimulation(
        MyDetectorDevice(thing),
        MyDetectorAdapter(other_thing)
    )
tpoliaw commented 1 year ago

Current thoughts on approaches to this, mainly so I can pick it up again at some point without having to revisit it all.

Is it just the dunder methods you want to avoid? As it's currently set up, the type specified in the yaml file has to be a subclass of ComponentConfig which precludes using a top-level function directly.

In rough order of complexity/magic

Change the name of the method This avoids using the `__call__` method and end users have to write a class with an alternative named method. Something like ```python @dataclass class Amplifier(ComponentConfig): init_amp: float def build(self) -> Component: return DeviceSimulation( name=self.name, device=AmplifierDevice(init_amp), adapters=AmplifierAdapter(), ) ``` then, in the `cli.py` module where it's used ```diff - [config().run_forever(*get_interface(backend)) for config in configs] + [config.build().run_forever(*get_interface(backend)) for config in configs] ``` On the plus side, there is very little that needs to change and it prevents the `__call__` being needed, on the down side, it doesn't really change the amount of boilerplate.
Create a new class at runtime via a decorator I think this is closest to the example in the issue. With a decorator we'd somehow convert a function such as ```python def amplifier(name: str, inputs: Dict[PortID, ComponentPort], init_amp: float) -> Component: return DeviceSimulation( ... ) ``` into a class that would otherwise look something like ```python class amplifier(ComponentConfig): init_amp: float def __call__(self): return DeviceSimulation( ... ) ``` > As a side tangent, while writing this it seems the ComponentConfig design is split weirdly across two different purposes. One is the create the underlying device implementation, the other is to connect the device to others in the simulation. > There's also an odd mix of configuration being done in python and in yaml. Why do the adapters used have to be decided ahead of time but the parameters need to be set in configuration? Given the end result of this, the rest of the code would remain the same (and existing implementations would not have to change either). On the downside, is there a risk of too much magic hiding what's actually going on? I also doubt it would play nicely with any type checking but not sure if that's an issue as these objects only seem to exist during the deserialisation and then the returned Component is used for everything else.
Add Magic to the deserialisation The current `loading.read_configs` method takes yaml and returns a list of configs. The bit in the middle is open to ~~abuse~~ manipulation. Via custom `default_conversion` methods, we can convert the config into anything it needs to be. This approach has the potential to make the end user facing bit the simplest at the expense of being very tightly coupled to a particular deserialisation library. With the change from apischema to pydantic to pydantic2, this doesn't seem like a route we want to follow.
Refactor config loading We're currently deserialising the yaml config to a list of ComponentConfigs which are then called in turn to create the Components. We could instead change the yaml format and deserialisation target to something like `Map[name: str, (builder: callable, args: [Any])]`. The Components could then be created via `builder(args)` and the builder can then be anything, function/class/instance etc. The config list is already converted to a map of name: config when building the wiring so the change might not be as drastic as it first appears. It would mean a lot of change across the whole project though so maybe best avoided if we're otherwise happy with the design.
abbiemery commented 1 year ago

Closed as won't do for now. Can be reopened as a 'redesign ComponentConfig' ticket Is this is wanted in the future.