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

PerSignalConfig dataclass for StandardReadable devices to be used with a common implementation of prepare. #543

Closed
wants to merge 13 commits into from

Conversation

burkeds
Copy link
Contributor

@burkeds burkeds commented Aug 28, 2024

This is one way prepare can be used in a standard fashion across StandardReadables while preserving the benefits of type-hinting.

PerSignalConfig is a dict-like dataclass which will take device signals as a key and a value. This dataclass is then passed to prepare and the signals will be set accordingly. Type checkers should recognize if a value not matching the signal type is assigned.

Example usage:

class MyMotor(StandardReadable):
    velocity: SignalRW[float]
    accl: SignalRW[float]

    def __init__(self, name: str = ""):
        super().__init__(name=name)
        with self.add_children_as_readables(ConfigSignal):
            self.velocity = SignalRW(name="velocity",
                                     backend=SoftSignalBackend(datatype=float,
                                                               initial_value=0.0))
            self.accl = SignalRW(name="accl",
                                 backend=SoftSignalBackend(datatype=float,
                                                           initial_value=0.0))


async def main():
    async with DeviceCollector(mock=True):
        device = MyMotor()
    config = PerSignalConfig()
    config[device.velocity] = 1.0 # Pyright throws a type-error if this is a str or something
    config[device.accl] = 2.0
    await device.prepare(config)
    print(await device.velocity.read())
    print(await device.accl.read())
    print("Finished")

if __name__ == "__main__":
    asyncio.run(main())


# OUTPUT
{'device-velocity': {'value': 1.0, 'timestamp': 6675122.751791038, 'alarm_severity': 0}}
{'device-accl': {'value': 2.0, 'timestamp': 6675122.752000068, 'alarm_severity': 0}}
Finished

Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks interesting and mirrors the behaviour of StandardDetector quite well. I've left some comments to stimulate discussion in the call later.

@@ -210,6 +212,36 @@ def add_readables(
if isinstance(obj, HasHints):
self._has_hints += (obj,)

@AsyncStatus.wrap
async def prepare(self, value: ReadableDeviceConfig) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we started on ophyd-async, a concern we had both with ophyd and pymalcolm was the large inheritance hierarchies that had developed over time. It was sometimes hard to reason about how a device would behave because the components of its behaviour were scattered across its parents in a non-obvious way. That's why Device is very sparse and even StandardReadble is still quite sparse, sometimes explicitness is better even at the cost of some duplication. We should be careful about introducing new default functionality at the parent level.

The alternative is to say that there is deliberately no default, generic functionality at the parent level and devices must implement their own, specific functionality.

Comment on lines 217 to 233
tasks = []
for dtype, signals in value.signals.items():
for signal_name, (expected_dtype, val) in signals.items():
if hasattr(self, signal_name):
attr = getattr(self, signal_name)
if isinstance(attr, (HintedSignal, ConfigSignal)):
attr = attr.signal
if attr._backend.datatype == expected_dtype: # noqa: SLF001
if val is not None:
tasks.append(attr.set(val))
else:
raise TypeError(
f"Expected value of type {expected_dtype} for attribute"
f" '{signal_name}',"
f" got {type(attr._backend.datatype)}" # noqa: SLF001
)
await asyncio.gather(*tasks)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this give us over a custom plan? I.e. this means we can type

yield from prepare(my_readable, ReadableDeviceConfig(foo=1, bar=2))
yield from read(my_readable)

instead of

yield from mv(my_readable.foo, 1, my_readable.bar, 2)
yield from read(my_readable)

But does it cover any other use cases?

Copy link
Contributor Author

@burkeds burkeds Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was motivated to do this because there is no obvious way to configure a device. In StandardReadable there is no method that can use the configure protocol. It looks like prepare was intended to be a way to configure a device but there is no obvious way to use it. prepare only takes a single argument, intended to be a dataclass, so this is one way a dataclass might be used to write to any of the configuration signals using prepare.

To me, it does not at all seem intuitive to "move" a config signal to some value to set it. I suspect a typical user or beamline scientist might perceive this as "hacky".

One nice application of these dataclasses is that it provides a data structure for configurations one may use to swap configurations easily within a plan or save/load configurations to and from a file or database.

src/ophyd_async/core/_readable.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_readable_config.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_readable_config.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_readable_config.py Outdated Show resolved Hide resolved
@burkeds burkeds marked this pull request as ready for review September 3, 2024 13:49
@burkeds burkeds changed the title ReadableDeviceConfig dataclass for StandardReadable devices to be used with a common implementation of prepare. PerSignalConfig dataclass for StandardReadable devices to be used with a common implementation of prepare. Sep 3, 2024
@coretl coretl self-requested a review September 9, 2024 13:13
@burkeds burkeds requested a review from coretl September 18, 2024 11:18
@burkeds
Copy link
Contributor Author

burkeds commented Sep 19, 2024

@coretl Looks like there is a new linting error regarding an incompatible override of the epics motor with prepare. How do you think we should handle this?

@coretl
Copy link
Collaborator

coretl commented Sep 19, 2024

@coretl Looks like there is a new linting error regarding an incompatible override of the epics motor with prepare. How do you think we should handle this?

Does making FlyMotorConfig inherit from PerSignalConfig fix the issue?

@burkeds
Copy link
Contributor Author

burkeds commented Oct 7, 2024

@coretl Looks like there is a new linting error regarding an incompatible override of the epics motor with prepare. How do you think we should handle this?

Does making FlyMotorConfig inherit from PerSignalConfig fix the issue?

It does not because the attributes of FlyMotorConfig are not SignalW. For this approach to work, any variables required by prepare might need to be soft signals. What do you think?

@coretl
Copy link
Collaborator

coretl commented Oct 7, 2024

After the collab call, are we still convinced we need this, or can it be replaced by load/save?

@burkeds
Copy link
Contributor Author

burkeds commented Oct 9, 2024

After the collab call, are we still convinced we need this, or can it be replaced by load/save?

I think we can shelve this now. Config via load/save and abs_set seems fine.

@coretl coretl closed this Oct 9, 2024
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

Successfully merging this pull request may close these issues.

3 participants