Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make user interface for components more friendly #99

Closed
abbiemery opened this issue Oct 28, 2022 · 2 comments
Closed

Make user interface for components more friendly #99

abbiemery opened this issue Oct 28, 2022 · 2 comments

Comments

@abbiemery
Copy link
Collaborator

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)
    )
@abbiemery abbiemery added the needed next Issues needed to be resolved before the others should be addressed label Oct 28, 2022
@abbiemery abbiemery removed the needed next Issues needed to be resolved before the others should be addressed label Apr 12, 2023
@tpoliaw
Copy link
Collaborator

tpoliaw commented Jul 7, 2023

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

@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

- [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

def amplifier(name: str, inputs: Dict[PortID, ComponentPort], init_amp: float) -> Component:
    return DeviceSimulation(
        ...
    )

into a class that would otherwise look something like

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
Copy link
Collaborator Author

abbiemery commented Jul 21, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants