46 adding catalogs section in the pyaml configuration file#192
46 adding catalogs section in the pyaml configuration file#192
Conversation
gupichon
requested review from
JeanLucPons,
TeresiaOlsson and
gubaidulinvadim
|
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. And i expect possible individual position and offset, so |
Do you mean having x_offset and y_offset, x_pos and y_pos, and so on? |
|
I've created the draft pull request, it will be easier to discuss here than under a commit. |
|
Yes but as string so we can use the catalog. |
…ction-in-the-pyaml-configuration-file
|
Thanks x_pos: str | None = None
y_pos: str | None = None |
Since the existence of available_axes: list[str] = ["x", "y"]
x_pos: str = "x_pos"
x_pos: str = "x_pos" |
|
If you configure only x_pos the you have only x, no need of an extra flag. |
|
No, I just want to make the file shorter and more readable. |
|
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] |
|
For me this is counter intuitive because i don't know what to put in the catalog. 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. |
|
As you wish then |
|
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/VOffsetand 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/Incoherencyor - 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 |
|
Actually, this can already work. We can develop a specific Tango catalog that builds the |
|
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. |
…ction-in-the-pyaml-configuration-file
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. |
|
If it's ok for you I continue with the rest of pyaml. |
|
You already rewrote and tested the examples ? |
def get_offset_devices(
self, name: str, catalog: Catalog
) -> list[DeviceAccess | None]:I don't understand why the name is needed there ? |
It add an additional complexity compare to what we have now. So it does not go in the good direction. |
… system is managed by the catalog view.
|
@JeanLucPons, the bug causing the Tango host to be tripled has been fixed. We may need to discuss how Also, can you resolve the last conflict? |
|
OK now it works. |
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 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 |
|
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_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"))] |
|
To go a bit further, and to avoid confusion, it would even change property to *_name: sr.live.get_bpms("BPM").x_pos_name = "ORBITCC:rdPos"
or
sr.live.get_bpms("BPM").x_pos_name = ["ORBITCC:rdPos"] * len(sr.live.get_bpms("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"))] |
For me it looks like that only changes the TANGO attribute for the live mode? Or it somehow will also change the TANGO attribute for all the other control modes the user potentially can have configured that also use same BPM? If it does that becomes confusing for the users, but if it doesn't it means the user need to do the same thing for all the modes despite that they in the original yaml config all were referring the same device. Personally, I think that syntax is too complicated compared to how it's done in MML and I doubt the current MML user will like it. They are already complaining about the pyAML user interface being too complicated and MML being easier to use. I think the user interface would be simpler if we don't allow changing the configuration at the element level at all but instead have something like the catalog which is responsible for being the only source of information for configuration data and manage changes to it. I would rather extend the catalog to include everything in the config and not just the control system parts. The yellow pages functionality can then just be something that prints out the information that is in the catalog. That would go in the direction of the MML AO, which I think works well and is an architecture idea worth keeping. |
|
Reading the recent discussions, I think we should address these topics more broadly. Are you available for a short Teams meeting this morning? I suggest splitting the discussion into three parts:
|
|
I am. Only thing I have planned for today is to try to make some progress on the validator decorator. If it would help I can show the MML AO and which parts of it I think are the ones that the users like and would be worth keeping in pyAML in some form. Obviously there also bad parts that should not be kept ;) |
It changes only for live, if you want to change for all lives, you may use the yellow pages later, which unfortunately returns names, at the moment :(
I propose a one line syntax. I don't understand your point, referencing is done by PyAML name and are already propagated. Why do you want one more layer ? It will complexify the config which is already too complex. We have a maintainer meeting next Friday to discuss this and try to find an agreement. |
|
Okay, then let's discuss it on Friday. @gubaidulinvadim Is there space to put it on the agenda? Maybe we need to postpone something and talk about this instead? |
|
Ok, let’s discuss this on Friday. |
|
Normally, there's no space on the agenda. But we can give priority to this discussion. Then we have to postpone the measurement class proposal of @JeanLucPons or your report/discussion on the steering committee roadmap. |
|
What about the set and wait functionality? According the roadmap, simplifying the configuration is now priority 1 and the hardware abstraction layer priority 2 so perhaps that could also be postponed for this discussion instead? |
|
The set and wait functionality is already postponed. The only two big items on the agenda are Measurement Class and discussion of the roadmap |
|
I agree, simplifying the configuration is high priority. # Load a config with BPM having no x_pos,y_pos,x_pos_index,y_pos_index
sr: Accelerator = Accelerator.load("MyACC.yaml")
sr.yellow_pages.get("BPM").x_pos = "ORBITCC:rdPos"
sr.yellow_pages.get("BPM").y_pos = "ORBITCC:rdPos"
sr.yellow_pages.get("BPM").x_pos_index = [i*2 for i in range(len(sr.yellow_pages.get("BPM"))]
sr.yellow_pages.get("BPM").y_pos_index = [i*2+1 for i in range(len(sr.yellow_pages.get("BPM"))]And usage with |
|
When deciding on the syntax, I think we need to consider the python level of the users. The MML users tell me that they are already struggling with the |
|
There is always a learning curve for all: python, pyaml, accelerator physics... Personally i do: orbit = sr.live.get_bpms("BPM").positions
# Then i use
...
orb0 = orbit.get()
...
orb1 = orbit.get() |
|
I think already to remove a lot of the |
|
Please discuss this in #199. |
|
@gupichon I have been looking at HAPPI which I would say is somewhat of a similar idea as the catalog. https://pcdshub.github.io/happi/v3.0.1/ If it helps, there might be some useful inspiration from looking at how they have implemented it. If I have understood it correctly, it's like a device database which either has a json file or a real database in the back. Sounded to me a bit similar to our approach with a yaml file or what you wanted to do with connection to the TANGO database. What I liked about it is that they are thinking about every entry as something which contains the information about how to import and instantiate a specific Python object. They say this:
Thinking about it like that at least made it easier for me to understand what the point is of the entry and how the attributes I put there is going to be used. I guess in our case |
|
They are also really thinking about it like it's a database despite that it in some cases is just a json file. Here are some examples of the user interface: https://pcdshub.github.io/happi/v3.0.1/client.html#db-choice I quite like that interface because it means that the yaml file is no longer a long confusing text file but just the backend for a database that the user isn't meant to look at directly. Instead they would browse, add and modify things in it through a python API. And that same Python API can also be used to create it from scratch for new users. Maybe that would solve our problem where every lab now need to write their own scripts to generate the yaml file? Now it's a bit like everyone needs to invent that API by themselves. |
|
Thanks a lot, @TeresiaOlsson, this will be very helpful. I’m going to restart this development from scratch. |
|
You want to restart this PR from scratch ? Using happi ? |
|
I'm also exploring to make a similar implementation as in HAPPI because it seems like the way to go. I played with it a bit last week and started to like that approach more and more. But I would not use HAPPI directly because it feels like a dependency we don't want to have. I don't think there is a big community around that package so it might not be long-term sustainable to depend on. I also think HAPPI doesn't do validation and some of the things they do could have been implemented easier if pydantic basemodels were used instead. The happyItem and containers look to me pretty much like they could have been basemodels. |
|
@gupichon I pushed what I had been playing with so far into the branch happi-style-config. So far I only tried to do a very basic yaml and json backend to understand how that might work compared to how we use the config file now. No guarantee that it actually is working because I didn't come so far. But at least my feeling was that we like this could have pretty much the same yaml file format as now. |
|
@TeresiaOlsson @JeanLucPons @gupichon I guess I'll put this discussion on the agenda for the meeting on Friday. |
|
I will be on holiday on Friday so I can't come. But if pyAML decides to do it in some other way I will keep developing it as an external package which would allow the EPICS labs to have some kind of device database that we can use to generate the config file for pyAML. At the moment it's just way too difficult for us since we don't have the TANGO database that we can extract information from. I think some EPICS labs might already have something like this but at HZB we currently only store this information inside the MML config which isn't great so we need to replace it. |
5 participants
Description
All references to the control system are moved to a catalog section.
Related Issue
Features/issues described there are:
Changes to existing functionality
Describe the changes that had to be made to an existing functionality (if they were made)
Testing
The following tests (compatible with pytest) were added:
Verify that your checklist complies with the project