Refactor element attachment with holder-specific dispatch

[gupichon]

Summary

Refactor element attachment to replace the type-based if/elif chains in
Simulator and ControlSystem with holder-specific dispatch.

ElementHolder.fill_device() now owns the common iteration over configured
elements. Each supported element type delegates its attachment to a dedicated
fill_* method implemented by the target holder.

Changes

• Add explicit attachment delegation to magnets, combined-function magnets,
  serialized magnets, BPMs, RF plants, tune monitors, tuning tools,
  measurement tools, and unbound elements.
• Add abstract fill_* hooks to ElementHolder.
• Move the existing simulator and control-system attachment logic into their
  corresponding holder-specific methods.
• Preserve the existing UnboundElement behaviour:
  • ControlSystem instantiates it only for matching control_modes.
  • Simulator ignores it, as before.
• Preserve existing configuration formats and runtime attachment behaviour.

Validation

• Ran the standard test suite.
• Ran the DT4ACC integration smoke test against a local twin for both
  tango-pyaml and pyaml-cs-oa.

[TeresiaOlsson]

I think it's better than before but is it possible to move the attachment process completely out of controlsystem and simulator? For example, this I think should not be the responsibility of that level but happen already at the Element level.

    def fill_magnet(self, magnet: Magnet) -> None:
        device = self.get_device_access(magnet.model.get_device_names()[0])
        current = RWHardwareScalar(magnet.model, device) if magnet.model.has_hardware() else None
        strength = RWStrengthScalar(magnet.model, device) if magnet.model.has_physics() else None
        self.magnet.add(magnet.attach(self, strength, current))

If not, every single time someone wants to add a new type of element (or change an existing one) changes are required in both controlsystem and simulator which make the code difficult to maintain and very difficult for users to contribute their own devices. It also requires the peer concept which I personally doubt the users will understand because I also don't understand why it is needed. It's strange to me that after creating the object once it has to be copied and the attributes of it has to be repeated somewhere else in the codebase. That feels error-prone to me.

I think when creating an Element it should be self-contained. You create the object and it knows everything it needs from the start. In ophyd-async this is done by specifying the signal types at the device level and giving it which backend to use as part of the initalization of the object. Wouldn't that be possible to also do for us? So when creating the Element you input the controlsystem/simulator and in that way this attachment can be handled at the Element level?

[gupichon]

I think it's better than before but is it possible to move the attachment process completely out of controlsystem and simulator? For example, this I think should not be the responsibility of that level but happen already at the Element level.

    def fill_magnet(self, magnet: Magnet) -> None:
        device = self.get_device_access(magnet.model.get_device_names()[0])
        current = RWHardwareScalar(magnet.model, device) if magnet.model.has_hardware() else None
        strength = RWStrengthScalar(magnet.model, device) if magnet.model.has_physics() else None
        self.magnet.add(magnet.attach(self, strength, current))

If not, every single time someone wants to add a new type of element (or change an existing one) changes are required in both controlsystem and simulator which make the code difficult to maintain and very difficult for users to contribute their own devices. It also requires the peer concept which I personally doubt the users will understand because I also don't understand why it is needed. It's strange to me that after creating the object once it has to be copied and the attributes of it has to be repeated somewhere else in the codebase. That feels error-prone to me.

I think when creating an Element it should be self-contained. You create the object and it knows everything it needs from the start. In ophyd-async this is done by specifying the signal types at the device level and giving it which backend to use as part of the initalization of the object. Wouldn't that be possible to also do for us? So when creating the Element you input the controlsystem/simulator and in that way this attachment can be handled at the Element level?

I completely agree, but I would like to do it step by step. In the end, the control system should not be responsible for building the PyAML object. Defining a customizable factory will be a subsequent step. I’ve never agreed with this peer concept either.

Another potential misuse of the current implementation would be overriding object creation in tango-pyaml. It is actually perfectly doable (as before), but it is not something we want to allow.

PyAML objects should only have a lightweight link with the control system or the lattice. It's not completely the case yet.

We will need to take some time to define a better architecture, especially regarding how PyAML objects are defined and linked to their holders. Unlike in Ophyd, here we have a single definition with multiple holders (multiple lattices and multiple control systems).

Anyway, I hope this current change is a step in the right direction.

[TeresiaOlsson]

I think it's better than before but is it possible to move the attachment process completely out of controlsystem and simulator? For example, this I think should not be the responsibility of that level but happen already at the Element level.

    def fill_magnet(self, magnet: Magnet) -> None:
        device = self.get_device_access(magnet.model.get_device_names()[0])
        current = RWHardwareScalar(magnet.model, device) if magnet.model.has_hardware() else None
        strength = RWStrengthScalar(magnet.model, device) if magnet.model.has_physics() else None
        self.magnet.add(magnet.attach(self, strength, current))

If not, every single time someone wants to add a new type of element (or change an existing one) changes are required in both controlsystem and simulator which make the code difficult to maintain and very difficult for users to contribute their own devices. It also requires the peer concept which I personally doubt the users will understand because I also don't understand why it is needed. It's strange to me that after creating the object once it has to be copied and the attributes of it has to be repeated somewhere else in the codebase. That feels error-prone to me.
I think when creating an Element it should be self-contained. You create the object and it knows everything it needs from the start. In ophyd-async this is done by specifying the signal types at the device level and giving it which backend to use as part of the initalization of the object. Wouldn't that be possible to also do for us? So when creating the Element you input the controlsystem/simulator and in that way this attachment can be handled at the Element level?

I completely agree, but I would like to do it step by step. In the end, the control system should not be responsible for building the PyAML object. Defining a customizable factory will be a subsequent step. I’ve never agreed with this peer concept either.

Another potential misuse of the current implementation would be overriding object creation in tango-pyaml. It is actually perfectly doable (as before), but it is not something we want to allow.

PyAML objects should only have a lightweight link with the control system or the lattice. It's not completely the case yet.

We will need to take some time to define a better architecture, especially regarding how PyAML objects are defined and linked to their holders. Unlike in Ophyd, here we have a single definition with multiple holders (multiple lattices and multiple control systems).

Anyway, I hope this current change is a step in the right direction.

Then I like it :) I just wait with approving a bit to give @JeanLucPons a chance to take a look it too before it can be merged it since he might also have input.

[JeanLucPons]

Looks good for me.
But would it be possible to pass fill methods to private.
i.e:

def _fill_magnet(self, magnet: Magnet) -> None:
        pass

Otherwise it add unclear methods to the user and counter a bit holder simplification.

Same remark for element method such as fill_device in bpm.

[JeanLucPons]

This architecture is there to ensure symmetry between simulator and cs and attach mechanism should be for internal use.
This was done for technical reason and, for instance, to allow specific aggregator for cs and simulator which are not the same.

For "light" attach (when no symmetry is needed) UnboundElement is there.

[JeanLucPons]

@TeresiaOlsson

It also requires the peer concept which I personally doubt the users will understand

peer (or parent) concept is very common in many API when you want to adopt an object oriented syntax and deal with nested objects.

If you want:

sr.live.bpm.get("BPM4").strength.set(10) # You need a peer (or a parent)

Otherwise

you can have old procedural fashion, and you don't need any peer.

set_strength( live, "BPM4" , 10)

For instance in a Java application when you have embedded widget, get getParent() method is there. getParent() (defined on java.awt.Component) returns the container that a component is placed in — useful for navigating up the component hierarchy, e.g. to find an enclosing window, panel, or to walk up until you find a component of a specific type.

[TeresiaOlsson]

@TeresiaOlsson

It also requires the peer concept which I personally doubt the users will understand

peer (or parent) concept is very common in many API when you want to adopt an object oriented syntax and deal with nested objects.

If you want:

sr.live.bpm.get("BPM4").strength.set(10) # You need a peer (or a parent)

Otherwise

you can have old procedural fashion, and you don't need any peer.

set_strength( live, "BPM4" , 10)

For instance in a Java application when you have embedded widget, get getParent() method is there. getParent() (defined on java.awt.Component) returns the container that a component is placed in — useful for navigating up the component hierarchy, e.g. to find an enclosing window, panel, or to walk up until you find a component of a specific type.

So the peer corresponds to the simulator or controlssystem that the element belongs to? Or to a copy of the element where the copy is connected to either simulator or controlsystem? I think that's the part which is confusing for the users.

For me I would think peer is another element (like when you have students in a class and they are peers) and parent is the container the element belongs to. But maybe that is not what the terms mean in software? If not, I think maybe a better term is still needed because the majority of the users won't understand the software terms and then they won't be able to write new devices themselves without needing help from a maintainer.

[JeanLucPons]

The peer is a reference to the container (holder) that contains the element.
I have nothing against changing peer to parent which I agree is more appropriate.

[gupichon]

So the peer corresponds to the simulator or controlssystem that the element belongs to?

Yes.

Or to a copy of the element where the copy is connected to either simulator or controlsystem?

Yes as well

I think that's the part which is confusing for the users.

Exactly, because the answer shouldn't be "yes" to both.

For me I would think peer is another element (like when you have students in a class and they are peers) and parent is the container the element belongs to. But maybe that is not what the terms mean in software? If not, I think maybe a better term is still needed because the majority of the users won't understand the software terms and then they won't be able to write new devices themselves without needing help from a maintainer.

You have a good point regarding terminology. Beyond the naming itself, the underlying design issue is that an element's role and behavior mutate once attached to the holder: it switches from a definition object to an implementation object.

One way or another, we do need to dispatch the elements to each holder to preserve symmetry, as @JeanLucPons mentioned. However, this should yield a distinct class rather than a copy. In my opinion, the fill_* methods on the holder should instantiate an object of a different class using a customizable factory.

[JeanLucPons]

As I already mentioned several times, the shallow copy is there to avoid duplication of large calibration data which are shared between each instance of attached objects. So objects that are fully constructed at loading time are not fully reconstructed at attachment time.

[gupichon]

As I already mentioned several times, the shallow copy is there to avoid duplication of large calibration data which are shared between each instance of attached objects. So objects that are fully constructed at loading time are not fully reconstructed at attachment time.

Yes, I see your point. I just disagree. I would prefer to have a separate project for the configuration and keep only the implementation in the pyaml core project.
In my opinion, avoiding this duplication comes at the expense of readability, which matters more to me. Anyway, it's not a priority right now. We'll see in future discussions how it evolves based on user feedback.

[TeresiaOlsson]

Changing peer to parent I think is a good start to make it much less confusing.

The other confusing part is the copy step in the attachment process but my impression is that this might have been a consequence of removing the ConfigModel and there is the possibility to simplify it now.

If I was a new user and didn't know anything about the code yet, I would expect that I can create an object for an element and add it to the controlsystem/simulator where I want it to go. And then I can create another element and add that to another controlsystem/simulator. I think the confusing part is that I need to create one object and then copies of it because then I don't understand what happens to the first object and why it was needed in the first place.

A software engineer would likely understand the difference between a definition object and an implementation object but for a physicist my feeling is that they will think of an Element as an element in a lattice file because that's what they are used to. And in pyAT there isn't the layer with definition objects so they are not familiar with it.

I think that layer in pyAML would be easier to understand if we created something like a configuration registry which is responsible for storing the configuration information in-memory after it has been loaded. Then the implementation objects are created from that information. Then there would still be definition and implementation objects but they won't both be elements so no confusing switch of the role of the object. One of them could just be an object of some generic configuration entry class.

[JeanLucPons]

If I was a new user and didn't know anything about the code yet, I would expect that I can create an object for an element and add it to the controlsystem/simulator where I want it to go. And then I can create another element and add that to another controlsystem/simulator. I think the confusing part is that I need to create one object and then copies of it because then I don't understand what happens to the first object and why it was needed in the first place.

Not a copy but a shallow copy and this is required only for peered (symmetric) objects. For UnboundObject , there is only one construction and no copy at all. You should however specify the control_modes i.e. "live". To create free object, you should follow the Ophyd device example. Peered objects are rather for advanced pyAML developer.

class MyTuneMonitor(Element, ABetatronTuneMonitor, StandardReadable):
    def __init__(self, cs: MyControlSystem, cfg: MyTuneMonitorConfigModel):

[TeresiaOlsson]

If I was a new user and didn't know anything about the code yet, I would expect that I can create an object for an element and add it to the controlsystem/simulator where I want it to go. And then I can create another element and add that to another controlsystem/simulator. I think the confusing part is that I need to create one object and then copies of it because then I don't understand what happens to the first object and why it was needed in the first place.

Not a copy but a shallow copy and this is required only for peered (symmetric) objects. For UnboundObject , there is only one construction and no copy at all. You should however specify the control_modes i.e. "live". To create free object, you should follow the Ophyd device example. Peered object are rather for advanced pyAML developer.

class MyTuneMonitor(Element, ABetatronTuneMonitor, StandardReadable):
    def __init__(self, cs: MyControlSystem, cfg: MyTuneMonitorConfigModel):

Maybe the question is then what a normal user should be able to do. I think they should be able to add new Element types, for example BBB feedback, insertion devices etc since writing their own classes for such things are already what the users at HZB do today. It's just that they make them using pyepics. And if they want to write classes like that and push them to pyAML so other labs also can use them they need to understand the peer concept. So for me peered objects can't be something only advanced developers understand or we will lose out on a lot of contributors.

But I think at least part of it can be solved by documentation so it will become better when we have had time to write it. According to the roadmap the hardware abstraction layer shouldn't be stable until the end of the year so I take that as also meaning that we have until the end of the year to improve the documentation about it ;)

[gupichon]

Looks good for me. But would it be possible to pass fill methods to private. i.e:

def _fill_magnet(self, magnet: Magnet) -> None:
        pass

Otherwise it add unclear methods to the user and counter a bit holder simplification.

Same remark for element method such as fill_device in bpm.

OK, why not, but that implies the elements would call the holder's private methods, and I'm not a big fan of that. Alternatively, the elements could call a factory and pass the peer as a context provider, so it only carries the necessary data. I understand that this is a shallow copy, but what bothers me is the behavioral discrepancy between the definition and the implementation. Anyway, we can discuss this later. If that works for you, I'll make those methods private and we can merge this PR as is for now. It seems everyone agrees that it's an improvement.

[JeanLucPons]

Maybe the question is then what a normal user should be able to do. I think they should be able to add new Element types, for example BBB feedback, insertion devices etc since writing their own classes for such things are already what the users at HZB do today. It's just that they make them using pyepics. And if they want to write classes like that and push them to pyAML so other labs also can use them they need to understand the peer concept. So for me peered objects can't be something only advanced developers understand or we will lose out on a lot of contributors.

Peered objects are both linked to internal models and control systems and symmetry should be in place (including aggregators). That means that if, for instance, you want to add a peered BBB feedback device, you should write code both for AT and CS. This is what i call advanced development.

In other hand, if you just want to add a BBB feedback that connect to your control system, it is easy to override the Element class as done in the example above. However, if we want portable code, it is important to define abstract classes (such as ABetatronTuneMonitor). Implementing ABBBFeedback class will be very difficult. At ESRF our BBB is very complex with large number of parameters, and we also used it for cleaning.

[TeresiaOlsson]

Maybe the question is then what a normal user should be able to do. I think they should be able to add new Element types, for example BBB feedback, insertion devices etc since writing their own classes for such things are already what the users at HZB do today. It's just that they make them using pyepics. And if they want to write classes like that and push them to pyAML so other labs also can use them they need to understand the peer concept. So for me peered objects can't be something only advanced developers understand or we will lose out on a lot of contributors.

Peered objects are both linked to internal models and control systems and symmetry should be in place (including aggregators). That means that if, for instance, you want to add a peered BBB feedback device, you should write code both for AT and CS. This is what i call advanced development.

In other hand, if you just want to add a BBB feedback that connect to your control system, it is easy to override the Element class as done in the example above. However, if we want portable code, it is important to define abstract classes (such as ABetatronTuneMonitor). Implementing ABBBFeedback class will be very difficult. At ESRF our BBB is very complex with large number of parameters, and we also used it for cleaning.

Okay. Then I understand what you mean with peered objects. I thought you also needed to define a peer if you just want to add an element to a single controlsystem/simulator.

For the BBB feedback it should at least be possible for everyone who has the Dimtel system. @eddybl and I are working on a common package for that with the hope that it at some point in the future can be used together with pyAML. But it's still very much work in progress.

[JeanLucPons]

Sorry again, the only fill_device() that I would like to keep public was the top level one (the holder one) otherwise the @TeresiaOlsson example will have to use a private method.

The merge-base changed after approval.

The merge-base changed after approval.