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 ?

[JeanLucPons]

Rougly speking, i expect:

• that this signature:

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

becomes as before (except that it will returns str):

    def get_pos_devices(self) -> list[str| None]:

The creation of DeviceAccess should be done in ControlSystem.

And This PR should only address BPM with working examples.

[gupichon]

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

Because the model does not know the name of the BPM.
But if the control system is responsible for retrieving the DeviceAccess, it is no longer needed.

[gupichon]

You already rewrote and tested the examples ?

Yes. It was not so much work, I scripted the modifications for EBSOrbit.yaml.

[gupichon]

I have introduced the possibility of creating a sub-catalog from the main catalog in order to work with a smaller subset and avoid conflicts when using a pattern to retrieve elements.

[gupichon]

@JeanLucPons I believe I have covered all your requirements. I’ll let you check when you’re back.

[JeanLucPons]

- type: pyaml.bpm.bpm
  name: BPM_C05-01
  model:
    type: pyaml.bpm.bpm_simple_model
    x_pos: x_pos
    y_pos: y_pos
- type: pyaml.bpm.bpm
  name: BPM_C05-02
  model:
    type: pyaml.bpm.bpm_simple_model
    x_pos: x_pos
    y_pos: y_pos

How this can work ?
All BPM refer to the same catalog entry ?
What about examples ?

[gupichon]

It expects the following entries to be present in the catalog:

• BPM_C05-01\x_pos
• BPM_C05-01\y_pos
• BPM_C05-02\x_pos
• BPM_C05-02\y_pos

Otherwise, a PyAMLException will be raised: Catalog reference '{reference}' not found.

[gupichon]

If you agree with the principle, I will work on examples and so on. This PR is still a draft.

[gupichon]

@JeanLucPons, thinking about what you wanted to do:

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

I need to modify this; otherwise, it will look for entries like
BPM_C21-09/srdiag/bpm/c21-09/SA_VA,
which is not what you want. Any ideas ?
Maybe we could introduce an optional flag in the model, such as no_bpm_prefix: True, with False as the default value? Maybe at the control system level?

Nevertheless, I have added the possibility to retrieve a sub-catalog by filtering with a pattern. This way, you can request all entries starting with srdiag/, and then bpm/, from code using PyAML. It may help.

[JeanLucPons]

I need to modify this; otherwise, it will look for entries like
BPM_C21-09/srdiag/bpm/c21-09/SA_VA,
which is not what you want. Any ideas ?

Yes this is what I want and it has been discussed like that, no ?
As I said, I would like to avoid hidden keyword in the reference such as: BPM_C05-01\ x_pos and having random string (independent of the BPM pyaml name) in the reference of the model.

- 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]`

Yes I would like to have this and that the catalog entry is srdiag/bpm/c21-09/SA_VA and not BPM_C21-09/srdiag/bpm/c21-09/SA_VA without dealing with pattern, flag of other things.
The PyAML name is not mandatory in the catalog reference (this is just a random ID for a redirection), however, you can use it if you want.

[gupichon]

Yes this is what I want and it has been discussed like that, no ?

No, it was not. Since the very first discussion, I proposed that the model should handle the key it needs. You asked to move it to the control system, which I did. Now, do you prefer using a single unique key with no key building performed by any system? I’m fine with that, but I would like this to be clearly expressed.
Is there anything else I can do?

[JeanLucPons]

Sorry for misunderstanding.
Yes I prefer that we don't use the pyaml object name in DeviceAccess references of models.

The user should be free to use what he wants for reference and, as you proposed, you can build your own syntax and implements a custom catalog as the catalog follows the PyAML configuration standard. It could be useful, for a specific control system, to overpass all the DeviceAccess configuration and do it dynamically using Tango DB. Such a custom catalog can be implemented the Tango backend.

[gupichon]

It is done.

[JeanLucPons]

It fails with BPM aggregator.

            for b in bpms:
                devs = self.attach_array(b.model.get_pos_devices())
                devH = devs[0]

I'm investigating, i observed a strange issue.

[JeanLucPons]

I added unit test for BPM aggregators

[JeanLucPons]

  positions_device = self._catalog.get_one(b.model.get_positions_device())
  devs = self.attach_array([positions_device, positions_device])

It is unclear for me, get_positions_device() should be get_position_devices() and return an array of x and y pos device that can be different (and None when not defined).
The BESSY2 example is using this and it should run.
I tested EBS example and it went fine.

[gupichon]

I’ve implemented a quick fix here, but things are still unclear to me.
Previously, there was a duplicated device entry. Now, the idea is to support two different cases:

• An array of two distinct devices: one for X, one for Y.
• A single device providing both positions as a vector.

Initially, I was thinking of letting the BPM class manage this: it would retrieve all the required devices and then expose a unified interface to the control system.
But if you want to move this logic into the control system class, it becomes less obvious to me. How would you like to handle it?

[JeanLucPons]

Rougly speaking you can do something like:

dx = self._catalog.get_one(b.model.get_x_pos_device())
dy = self._catalog.get_one(b.model.get_y_pos_device())
devs = self.attach_array([dx, dy])

dx can be equal to dy when you have an attribute that returns the 2 orbits (H&V) as in the BESSY2 example.
dx can be different from dy when you have 2 attributes for 2 orbits (as we have at ESRF and that we can use)

[gupichon]

ok.
At least, its twice the same ref instead of twice the same device....

[gupichon]

Ok, I made the changes.
What bothers me is that if x/y_pos_index is set, then x_pos and y_pos will always point to the same value. That’s an underlying hidden rule.