46 adding catalogs section in the pyaml configuration file

[gupichon]

Description

All references to the control system are moved to a catalog section.

Related Issue

• Closes Adding catalogs section in the pyaml configuration file #187

Features/issues described there are:

• new feature: was implemented in the following way... because...
• bugfix: was implemented in the following way... because...
• ...

Changes to existing functionality

Describe the changes that had to be made to an existing functionality (if they were made)

• First change: reimplemented in the following way... because
• Second change: reimplemented in the following way... because
• ...

Testing

The following tests (compatible with pytest) were added:

• first test
• second test
• ...

Verify that your checklist complies with the project

• New and existing unit tests pass locally
• Tests were added to prove that all features/changes are effective
• The code is commented where appropriate
• Any existing features are not broken (unless there is an explicit change to an existing functionality)

[JeanLucPons]

As previoulsy discussed, it would be nice, in order to minimize impact, and allow evolution of BPM model that you restore the DeviceAccess ref in the model and change:

        if catalog.has_reference(name + "/tilt"):
            return catalog.get_one(name + "/tilt")

to

        if catalog.has_reference(name + "/" + self._cfg.tilt):
            return catalog.get_one(name + "/" + self._cfg.tilt)

and same for positions and offsets.
otherwise the re-update of unit tests and examples will be a pain.

And i expect possible individual position and offset, so catalog.get_one() at every place.

[gupichon]

As previoulsy discussed, it would be nice, in order to minimize impact, and allow evolution of BPM model that you restore the DeviceAccess ref in the model and change:

        if catalog.has_reference(name + "/tilt"):
            return catalog.get_one(name + "/tilt")

to

        if catalog.has_reference(name + "/" + self._cfg.tilt):
            return catalog.get_one(name + "/" + self._cfg.tilt)

and same for positions and offsets. otherwise the re-update of unit tests and examples will be a pain.

And i expect possible individual position and offset, so catalog.get_one() at every place.

Do you mean having x_offset and y_offset, x_pos and y_pos, and so on?

[gupichon]

I've created the draft pull request, it will be easier to discuss here than under a commit.

[JeanLucPons]

Yes but as string so we can use the catalog.
The BPM model will evolve and we will definitely add some model for position calculation from BPM button for instance, using a more accurate model than the classical DoS.
That means that instead of get_pos_devices() in the model you can have a get_button_devices() for instance.

[JeanLucPons]

Thanks
It would be nice to make them optional. Otherwise i cannot comfigure a SimpleBPM with only one position.

x_pos: str | None = None
y_pos: str | None = None

[gupichon]

Thanks It would be nice to make them optional. Otherwise i cannot comfigure a SimpleBPM with only one position.

x_pos: str | None = None
y_pos: str | None = None

Since the existence of x_pos and y_pos will be the general case, and a single position the exception, wouldn’t it be better to have a field specifying which axes are available, with both as the default? This would avoid having to specify x_pos and y_pos everywhere, except for a few BPMs. Same for offsets.

available_axes: list[str] = ["x", "y"]
x_pos: str = "x_pos"
x_pos: str = "x_pos"

[JeanLucPons]

If you configure only x_pos the you have only x, no need of an extra flag.
If you want to optimize access to the catalog with get_many() then you can easily reconstruct this flag internally.

[gupichon]

No, I just want to make the file shorter and more readable.

[gupichon]

It would look like:

devices:
- type: pyaml.bpm.bpm
  name: BPM_C01-01
  model:
    type: pyaml.bpm.bpm_tiltoffset_model
- type: pyaml.bpm.bpm
  name: BPM_C01-02
  model:
    type: pyaml.bpm.bpm_simple_model
    available_axes: [x]

[JeanLucPons]

For me this is counter intuitive because i don't know what to put in the catalog.
You will have to create hidden keyword such as pos/off and tilt.
Last but not least BPMSimpleModel will be replaced by BPMTiltOffsetModel with optional tilt and offset.

Honestly I really would prefer to have a simple string in the the x_pos attribute that directly refers to the catalog even independently of the name of the PyAML top level object.

[gupichon]

As you wish then

[JeanLucPons]

At this end i would like to be able to do:

- type: pyaml.bpm.bpm
  name: BPM_C21-09
  model:
    type: pyaml.bpm.bpm_simple_model
    x_pos: srdiag/bpm/c21-09/SA_HPosition
    y_pos: srdiag/bpm/c21-09/SA_VPosition
    x_offset: srdiag/bpm/c21-09/HOffset
    y_offset: srdiag/bpm/c21-09/VOffset

and later:

- type: pyaml.bpm.bpm
  name: BPM_C21-09
  model:
    type: pyaml.bpm.bpm_simple_model
    x_pos: srdiag/bpm/c21-09/SA_HPosition
    y_pos: srdiag/bpm/c21-09/SA_VPosition
    x_offset: srdiag/bpm/c21-09/HOffset
    y_offset: srdiag/bpm/c21-09/VOffset
    incoherency:  srdiag/bpm/c21-09/Incoherency

or

- type: pyaml.bpm.bpm
  name: BPM_C21-09
  model:
    type: pyaml.bpm.bpm_button_model
    va: srdiag/bpm/c21-09/SA_VA
    vb: srdiag/bpm/c21-09/SA_VB
    vc: srdiag/bpm/c21-09/SA_VC
    vd: srdiag/bpm/c21-09/SA_DC
    K: [1.001,0.9956,1.019287,1.0021]

Then srdiag/bpm/c21-09/SA_HPosition will look in the catalog for the CS backend.
That would be nice.

[gupichon]

Actually, this can already work. We can develop a specific Tango catalog that builds the DeviceAccess object based on the key used to query it, which would simply be the device name.

[gupichon]

You can even go further and have the control system register itself in the catalog. Such a catalog could then check the Tango database to see whether a device exists, fetch its units, and so on. In that case, you would not even need to list the devices anymore.
However, this would reintroduce the naming convention at the device level again.

[JeanLucPons]

You can even go further and have the control system register itself in the catalog. Such a catalog could then check the Tango database to see whether a device exists, fetch its units, and so on. In that case, you would not even need to list the devices anymore. However, this would reintroduce the naming convention at the device level again.

This is not really the goal, you have still in your catalog all the configuration of your underlying device which can be Epics or Tango. I recall that, for instance, the type is needed with pyaml-cs-oa. For the time being pyaml-cs-oa assumes that all control system variables are Float64 or Float64 array. We don't have this issue with native Tango backend (this is why attach_array() and attach() have the same implementation in native Tango backend). Last but not least for a RW variable, in Epics you have 2 underlying PV.

For x_pos, i expect a random string that will refer to one RO Tango attribute or Epics PV.

I like the idea of this Catalog. You have just a simple string for your device config at the Element (or Model) level.

[gupichon]

If it's ok for you I continue with the rest of pyaml.

[JeanLucPons]

You already rewrote and tested the examples ?
I'll be off Tomorow and Monday.

[JeanLucPons]

 def get_offset_devices(
        self, name: str, catalog: Catalog
    ) -> list[DeviceAccess | None]:

I don't understand why the name is needed there ?

[gupichon]

If you want to change the reference without reloading the entire configuration (at runtime), the idea is to update the prototype, and the catalog will then update the specialized instances.

[JeanLucPons]

If you want to change the reference without reloading the entire configuration (at runtime), the idea is to update the prototype, and the catalog will then update the specialized instances.

Do you need to change the reference of a control system device at runtime ? This configuration here is expected to be static. No dynamic change of CS attribute name or PV. Adding this level of complexity is lot of work for no real added value.

[gupichon]

The idea of runtime modification is mainly to allow correcting or completing a configuration interactively, for example from the command line. This avoids having to perform a large configuration step upfront when working in environments such as IPython or Jupyter notebooks, since you can update the configuration at runtime.

This is obviously not intended for production use, where the configuration is expected to remain static.

This is intended to lower the entry barrier for new users and make local testing and development easier.

[JeanLucPons]

OK
This will also introduce the way to break coherency between the runtime and the configuration.
However, it is plan to be able to update the config dynamically, @TeresiaOlsson is working on it.
Her decorator will also allow to update dynamically the config and make re-attachment when needed.
For instance:

# Trigger a new attachment and a new `DeviceAccess` instance of attribute srdiag/bpm/c16-02/SA_HPosition
sr.live.get_bpm("BPM_C16-02").x_pos = "srdiag/bpm/c16-02/SA_HPosition"

while

# Do nothing expect updating the config
sr.live.get_bpm("BPM_C16-02")._cfg.x_pos = "srdiag/bpm/c16-02/SA_HPosition"

[gupichon]

Do you have the link to where this decorator is implemented? Or the discussion about it? I completely missed this development.

[JeanLucPons]

It is a PR in progress

[gupichon]

Ok, I’ve viewed it. Please correct me if I’m mistaken, but @TeresiaOlsson’s work and the catalogs appear to be complementary. We are speaking about the branch validator-decorator?

[TeresiaOlsson]

Yes, that's the branch for the draft PR #214.

My idea was just to add a decorator which links a class to the model that should be used to validate it to make it more flexible than requiring there to be a ConfigModel inside the same module since not all external packages might be written like that.

But then after discussing with @JeanLucPons it came up that maybe this can also be used to validate when the configuration is dynamically changed. So I'm also planning to add that into the same PR.

Would it cause a conflict with the catalog that I need to consider? I was hoping you could make a change in the catalog, then the validate function that this decorator is planned to add will run and validate the input before it is actually changed in the already existing object.

[JeanLucPons]

I would say that the decorator should be able to take fields that need a rebuild process.
i.e.

class ConfigModel(ElementConfigModel):
    """
    Configuration model for BetatronTuneMonitor

    Parameters
    ----------
    tune_h : str
        Horizontal betatron tune device name
    tune_v : str
        Vertical betatron tune device name
    an_other_param: float
        For example
    """

    model_config = ConfigDict(arbitrary_types_allowed=True, extra="forbid")

    tune_h: str| None
    tune_v: str| None
    an_other_param: float

@Config(tune_h,tune_v) # Indication that fields tune_h and tune_v should be mapped with a hook that will trigger reattachment, other fields are just simple mapping from _cfg
class BetatronTuneMonitor(Element):

This should allow dynamic catalogs, (if #1228) otherwise the catalog will add one more layer and make thing more difficult than before just to dynamically change an attribute/PV name.

[gupichon]

Yes, that's the branch for the draft PR #214.

My idea was just to add a decorator which links a class to the model that should be used to validate it to make it more flexible than requiring there to be a ConfigModel inside the same module since not all external packages might be written like that.

But then after discussing with @JeanLucPons it came up that maybe this can also be used to validate when the configuration is dynamically changed. So I'm also planning to add that into the same PR.

Would it cause a conflict with the catalog that I need to consider? I was hoping you could make a change in the catalog, then the validate function that this decorator is planned to add will run and validate the input before it is actually changed in the already existing object.

Yes, that’s why I think it won’t cause any conflict. Basically, in your case, you just use catalog references and can dynamically change those references. On the other hand, the catalog allows you to change what those references point to.

I may be wrong, but as I see it, these are two different concerns.

[JeanLucPons]

I do not see the point of having dynamic references to static stuff in a config file !
Unless you implement dynamic catalog.

[TeresiaOlsson]

I'm not entirely sure I understand now. What do you mean by dynamic?

For me it means that if I discover that the name of a PV is wrong I can change it without having to reload the config to test. Then I imagine that I make a change to a value in the catalog, the change somehow gets propagated to the devices that needs it, validated and changed.

But are you also talking about changing references so if I for example discover that it's not just a typo in the PV but it should actually point to a completely different entry in the catalog, you should also be able to do that change dynamically?

[gupichon]

From my perspective, the configuration file is simply one way (static) of initializing PyAML, among others. The catalog itself can remain dynamic. What is needed is an API to populate and update it, along with, through the use of decorators, the ability to switch references within high-level objects.

The primary goal of the catalog is to abstract away direct backend references from the user. Instead, users interact only with references.

[JeanLucPons]

I'm not entirely sure I understand now. What do you mean by dynamic?

For me it means that if I discover that the name of a PV is wrong I can change it without having to reload the config to test. Then I imagine that I make a change to a value in the catalog, the change somehow gets propagated to the devices that needs it, validated and changed.

But are you also talking about changing references so if I for example discover that it's not just a typo in the PV but it should actually point to a completely different entry in the catalog, you should also be able to do that change dynamically?

Yes this can be a use case:
For instance:
You have in your config:

- type: pyaml.bpm.bpm
  name: BPM_C21-09
  model:
    type: pyaml.bpm.bpm_simple_model
    x_pos: srdiag/bpm/c21-09/SAHPosition # Wrong
    y_pos: srdiag/bpm/c21-09/SA_VPosition
    x_offset: srdiag/bpm/c21-09/HOffset
    y_offset: srdiag/bpm/c21-09/VOffset

then you can dynamically do:

# Trigger a new attachment and a new `DeviceAccess` instance of attribute srdiag/bpm/c21-09/SA_HPosition
sr.live.get_bpm("BPM_C21-09").x_pos = "srdiag/bpm/c21-09/SA_HPosition"

and it will work without taking care of the catalog.

[JeanLucPons]

Otherwise without dynamic catalog, you will just report the issue to catalog entries which is one more layer of complexity.

[gupichon]

Let’s illustrate this with two examples (the first one is yours, @JeanLucPons):

1. BPM-level reference update

For a given BPM, you have:

- type: pyaml.bpm.bpm
  name: BPM_C21-09
  model:
    type: pyaml.bpm.bpm_simple_model
    x_pos: srdiag/bpm/c21-09/SAHPosition  # Wrong
    y_pos: srdiag/bpm/c21-09/SA_VPosition
    x_offset: srdiag/bpm/c21-09/HOffset
    y_offset: srdiag/bpm/c21-09/VOffset

You correct the reference directly at the BPM level:

sr.live.get_bpm("BPM_C21-09").x_pos = "srdiag/bpm/c21-09/SA_HPosition"

---

2. Catalog-level correction

In the catalog, you have:

- type: pyaml.configuration.catalog_entry
  reference: BPM_C01-02/x_pos
  device:
    type: tango.pyaml.attribute_read_only
    attribute: srdiag/bpm/c01-01/SA_HPosition  # Wrong: c01-01 instead of c01-02
    unit: mm

You fix it dynamically at the catalog level:

sr.get_catalog("live_cat").update_proto(
    Attribute(AttrConfigModel(attribute="srdiag/bpm/c01-02/SA_HPosition"))
)

---

I understand that this introduces an additional layer of complexity, but it allows users to remain independent from the backend, which can be maintained by someone else.

[JeanLucPons]

The catalog identifier is a random string, with your proposal no hope to have a dynamic catalog.
You can have also in failure at BPM_C01-02/x_pos, how you fix it dynamically ?
An again, using BPM_C01-02/x_pos implies hidden keyword.
One more layer is not really acceptable.

Several recommendation from steering committee to simplify the config as much as possible.
Being able to change an error (from an erroneous config file) dynamically is not a priority.

[gupichon]

The catalog identifier is a random string, with your proposal no hope to have a dynamic catalog. You can have also in failure at BPM_C01-02/x_pos, how you fix it dynamically ? An again, using BPM_C01-02/x_pos implies hidden keyword. One more layer is not really acceptable.

Several recommendation from steering committee to simplify the config as much as possible. Being able to change an error (from an erroneous config) file dynamically is not a priority.

I’m not sure I understand. BPM_C01-02/x_pos is simply a string like any other. I have removed all key validation, so there are no hidden keywords anymore.

[JeanLucPons]

So if there are no hidden kewords, that means you have the ref in the BPM x_pos property.
In case of error here, how you update this property dynamically (as for a wrong attribute name) in the catalog ?
And how I can implement a dynamic catalog using DB introspection ?

[gupichon]

So if there are no hidden kewords, that means you have the ref in the BPM x_pos property. In case of error here, how you update this property dynamically (as for a wrong attribute name) in the catalog ? And how I can implement a dynamic catalog using DB introspection ?

Okay, I think I understand your point now.
By a dynamic catalog, you were referring to a catalog directly linked to a database, as I previously suggested.

In that case, you would expect the keys to be resolvable identifiers in the catalog implementation (e.g., Tango attributes or aliases). Obviously, attempting to update it (update, add, or remove entries) would raise an error with such a catalog.

To me, this is not an issue if this functionality is not available in all implementations, as it is not mandatory.

[JeanLucPons]

To me, this is not an issue if this functionality is not available in all implementations, as it is not mandatory.

Unfortunately, it may create issue as I already had few remarks from users that the config is too difficult !
You add one more layer in the config, and code complexity. Honestly, I'm not able to fix the actual bug with wrong prefix.
I really would like to simplify as much as possible (at least the config).

[gupichon]

I’m currently looking into it.

The ability to change the underlying device of a reference does not introduce any additional complexity in the configuration file compared to the initial catalog proposal. It remains strictly the same. The added complexity is confined to the underlying code layers, which are hidden from users. I do not expect users to interact with this code, even when developing tools.

[JeanLucPons]

I’m currently looking into it.

The ability to change the underlying device of a reference does not introduce any additional complexity in the configuration file compared to the initial catalog proposal. It remains strictly the same.

It add an additional complexity compare to what we have now. So it does not go in the good direction.
However, I count on dynamic catalog to make DeviceAccess disappear completly from configuration.
I have some good news (I hope) from ophyd-async concerning type. I have to test.

[gupichon]

@JeanLucPons, the bug causing the Tango host to be tripled has been fixed. We may need to discuss how attach_indexed is supposed to work, but with my latest commit, it should be fine.

Also, can you resolve the last conflict?

[JeanLucPons]

OK now it works.
attach_indexed() tells the backend that an array (a DevDouble Spectrum if you prefer) is expected. It is directly link to #1228. If this issue is solved, then attach_indexed() can be removed.
I see tomorrow for conflicts.

[TeresiaOlsson]

From my perspective, the configuration file is simply one way (static) of initializing PyAML, among others. The catalog itself can remain dynamic. What is needed is an API to populate and update it, along with, through the use of decorators, the ability to switch references within high-level objects.

The primary goal of the catalog is to abstract away direct backend references from the user. Instead, users interact only with references.

For me that makes the catalog sound very similar to the AO (AcceleratorObject) in MML. I would say the AO is somewhat of an in-memory database of all the devices and their configuration and works well for the users without adding extra complexity. So you populate it once by reading in some static source and then you can dynamically change it if you want but those changes only live during that session unless you save the changes back to the static source at some point.

However, the reason why I think it works well in MML is because the only way to change the configuration of a device is to make the change in the AO. If it's possible in pyAML to change the configuration of a BPM directly on the BPM object or in the catalog I agree with @JeanLucPons that this makes things more confusing rather than simpler.

This is one of the reasons why I think creating all the objects for the elements directly after loading the config might not be the best solution. For me that always felt like it creates a lot of long-lived object that you need to manage to keep up-to-date if you implement dynamic changes of the config. I thought the dynamic config would be easier to implement if there was some central object that stores all config data (like the MML AO or the catalog) and then the objects for the elements are created when the user needs them. So sr.live.get_bpm("BPM_C21-09") would not return an object that already exists but rather go to the catalog, take the information that is there at that point and create the object. The user can then do:

bpm1 = sr.live.get_bpm("BPM_C21-09")

# Here the user changes the catalog entry using some API

bpm2 = sr.live.get_bpm("BPM_C21-09")

to get two objects for the same BPM with different configs. I think that is easy to do as long as the API for the catalog is straight-forward. Otherwise, how do you keep the live and virtual accelerator modes up-to-date with each other?

If I do sr.live.get_bpm("BPM_C21-09").x_pos = "srdiag/bpm/c21-09/SA_HPosition" what is changing at the moment? Is it only the BPM for the live mode or does it also change the corresponding entry in the catalog and propagate that to all the other objects that reference the same entry?

[JeanLucPons]

This what I would like to simplify by simply get rid off catalog or AO and just having one string that tell how to access the hardware. All reference factoring problems should not be seen by the user. If you do sr.live.get_bpm("BPM_C21-09").x_pos = "srdiag/bpm/c21-09/SA_HPosition" it changes only what you expect, the Tango attribute for BPM_C21-09. However all arrays or tools that reference BPM_C21-09 will now access this new Tango attribute.
If you use indexed BPM, then you should be able to do:

sr.live.get_bpms("BPM").x_pos = "ORBITCC:rdPos"
or
sr.live.get_bpms("BPM").x_pos = ["ORBITCC:rdPos"] * len(sr.live.get_bpm("BPM"))

sr.live.get_bpms("BPM").x_pos_index = [i*2 for i in range(len(sr.live.get_bpms("BPM"))]
sr.live.get_bpms("BPM").y_pos_index = [i*2+1 for i in range(len(sr.live.get_bpms("BPM"))]