Conversation
…ection-in-the-pyaml-configuration-file-new-implementation # Conflicts: # pyaml/accelerator.py # pyaml/configuration/__init__.py
|
What is the |
|
Would it be possible to try to cut this kind of PR into several tasks and PRs ? |
I'm still hesitating about whether we should let the user provide a DeviceAccess object or a DeviceAccess key only. This approach allows for both. It also prevents regressions in existing configuration files, even if that isn't a strict requirement in the current development phase. |
It's quite heavy to split it, as I want to be sure that I cover every use case. It’s debatable... |
|
@JeanLucPons, I’m still in the middle of a major development phase. I’ve created this draft PR to show you and others the early progress. |
|
At the same time, I'm developing the dynamic catalog in tango-pyaml. |
|
Please make separate issues. |
Such as ? |
|
Written in the review |
|
Usually, I can see your reviews, but I don’t see anything on my end this time... Maybe the review is still in 'Pending' mode? |
|
Yes it was still pending, sorry |
…ection-in-the-pyaml-configuration-file-new-implementation
…ection-in-the-pyaml-configuration-file-new-implementation
|
@JeanLucPons, I received your comment via email regarding pyaml/diagnostics/tune_monitor.py. However, since that file is no longer within the scope of this PR, I cannot reply to it directly here. I am working on reducing the impact of the changes by reverting some modifications (even if I have to re-introduce them later). Please note that several of the new files are actually dedicated to tests. |
|
The difficulty will be in the backend in order to avoid duplication of |
from pyaml.common.abstract import ReadWriteFloatScalar
from pyaml.common.element import Element, ElementConfigModel
from pyaml.control.deviceaccess import DeviceAccess
from pyaml.control.controlsystem import ControlSystem
import numpy as np
PYAMLCLASS = "ID"
class ConfigModel(ElementConfigModel):
gap: DeviceAccess | None
class ID(Element):
"""
Class providing access to an insertion device
"""
def __init__(self, cs:ControlSystem, cfg: ConfigModel):
super().__init__(cfg.name)
self.device = cs.attach([cfg.gap])[0] # Retrieve device handle
@property
def gap(self) -> ReadWriteFloatScalar:
class GapRW(ReadWriteFloatScalar):
def __init__(self, parent: ID):
self.parent = parent
def get(self) -> np.array:
return self.parent.device.get()
def set(self, value: float):
"""Set the values"""
self.parent.device.set(value)
def set_and_wait(self, value: float):
...
def unit(self) -> str:
return "mm"
return GapRW(self)Please also think to this use case where the cs.attach([cfg.gap])[0] # Retrieve device handleis not very user friendly, I would like to have something like: cs.get_device(cfg.gap) # Retrieve device handleWhere the get_device call should return the appropriate |
It is supposed to be hidden from the user by the validator, which will allow the keys as you requested. This configuration will be accepted: - type: pyaml.bpm.bpm
name: BPMZ7T3R
model:
type: pyaml.bpm.bpm_simple_model
# Will work with all catalogs types
x_pos: ORBITCC:rdPos@82
y_pos: ORBITCC:rdPos@83In general, the catalog should only care about the link with the backend, not about how it is used (with a specific index, for instance). Moreover, if you switch between two catalogs (one static and another dynamic), you would have to deal with different behaviors. You use case work also if I adapt it to my proposal: from pyaml.common.abstract import ReadWriteFloatScalar
from pyaml.common.element import Element, ElementConfigModel
from pyaml.control.deviceaccess import DeviceAccess
from pyaml.control.deviceaccesskey import DeviceAccessKey
from pyaml.control.controlsystem import ControlSystem
import numpy as np
PYAMLCLASS = "ID"
class ConfigModel(ElementConfigModel):
gap: DeviceAccessKey | None
class ID(Element):
"""
Class providing access to an insertion device
"""
def __init__(self, cs:ControlSystem, cfg: ConfigModel):
super().__init__(cfg.name)
self.device:DeviceAccess | None = cs.attach(cfg.gap) # Return a ResolvedDeviceAccess which is a DeviceAccess
@property
def gap(self) -> ReadWriteFloatScalar:
class GapRW(ReadWriteFloatScalar):
def __init__(self, parent: ID):
self.parent = parent
def get(self) -> np.array:
return self.parent.device.get()
def set(self, value: float):
"""Set the values"""
self.parent.device.set(value)
def set_and_wait(self, value: float):
...
def unit(self) -> str:
return "mm"
return GapRW(self)I prefer to adapt the cs.attach signature to return a single element if it receives a single element instead of a list. It’s very easy to implement. To me, get_device implies that the device is already attached and simply returns it. If the index were embedded directly inside Sorry for the long response, I was updating it while you were posting yours. |
|
In addition, the decorator we considered developing must accept the same entries as the configuration. So this must work: my_id:ID = get_my_id()
my_id.device= "ORBITCC:rdPos@82" # Replace by a PV that is actually meaningfulIt will build the |
…ne limit change Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
@JeanLucPons, with the latest merge from main, the number of changed files has dropped to 34! |
Roughly speaking I don't want to be able to see this in the config: x_pos:
type: pyaml.control.device_access_key
key: srdiag/bpm/c01-04/Position
index: 0To me it is more intuitive to have the index the If i insist to have it in the backend it is because the advanced syntax will depend on the control system and on the need of the users. i.e. - type: pyaml.id
name: ID24
gap: R:GAP:ID24, W:GAP:ID24_SETor - type: pyaml.id
name: ID24
gap: R:GAP:ALL_ID@24, W:GAP:ID24_SET@24or - type: pyaml.id
name: ID24
gap: id/id/24/gapor We just need to have a simple catalog with simple string for key in PyAML and well define the |
|
It is good but i still can see changes that I do not expect in magnets. |
|
For this PR we will forget the indexed stuff. |
|
Thinking about your comment, I agree that we should delegate the entire DeviceAccess to the Control System. The CS side would then implement an object similar to the ResolvedDeviceAccess I suggested. Furthermore, all catalogs—including the static one—should be handled by the CS. PyAML will simply define an interface that the control system must respect. The CS will handle indices and other specifics, returning a DeviceAccess object to PyAML with full value management. Consequently, the current StaticCatalog would be moved to tango-pyaml, pyaml-cs-oa, and dummy-cs as specializations. This won't be a major change since the current implementation is actually quite lightweight. The implementation will be the same for Tango, but a more elaborate one will be needed for EPICS, as the keys can be significantly more complex. I can write a full proposal if you want ? |
|
It is fine for me. |
Yes, it’s a leftover from a bug fix for serialized magnets that I need to move to another branch. Sorry about that! |
…to another branch
…backend. Remove StaticCatalog from pyaml core, add AttributeIndexed to the dummy CS, and strip all *_pos_index / attach_indexed / CSBPMArrayMapper logic from pyaml.
… and get_devices, and update internal call sites.
There was a problem hiding this comment.
I don't understand "ORBITCC:rdPos@0" It is already a dynamic catalog ?
There was a problem hiding this comment.
Yes, it is. You can take a look at the dedicated branch and the PR on pyaml-cs-oa. There is also a PR for tango-pyaml.
I haven't done much testing yet as there are no unit tests in pyaml-cs-oa, but I've confirmed that the file loads successfully.
There was a problem hiding this comment.
-
I'm afraid that BPM aggregators will not work ex expected....
-
I expect:
devs = self.get_devices(b.model.get_pos_devices())instead of:
devs = self.attach(self.get_devices(b.model.get_pos_devices()))The ControlSystem::get_devices(name:str) should be responsible of attachement.
Later, in an other PR, for BPM refurbishment, we will rename b.model.get_pos_devices() to b.model.get_pos_device_names()
There was a problem hiding this comment.
I'm afraid that BPM aggregators will not work ex expected....
Could you clarify this point?
The ControlSystem::get_devices(name:str) should be responsible of attachement.
So, should it be handled in a single call? I’m fine with that, but I suggest doing it later as other devices (Magnets, RF, etc.) still rely on this method. Once this PR is merged, we can finish refactoring the attachment process within the control systems. This will also involve significant updates across other projects.
There was a problem hiding this comment.
- Details for aggregator are given in the pyaml-cs-oa corresponding PR
- The idea is to get, in a first step, the 2 methods (attach() for old devices and get_device() for BPM). For this PR BPM should no longer access the old DeviceAccess interface.
There was a problem hiding this comment.
The idea is to get, in a first step, the 2 methods (attach() for old devices and get_device() for BPM). For this PR BPM should no longer access the old DeviceAccess interface.
Should the get_device(s) methods simply wrap the attach method?
I've pushed a small commit following this approach.
There was a problem hiding this comment.
The idea was rather to move get_devices() in the backend and to abstract it in pyaml.
At the end, I would like to have only a single string in the config and a get_devices(name:str) method at the ControlSystem level that return a list of DeviceAccess.
BPMs now use get_devices() directly instead of attach(get_devices(...)).
…ection-in-the-pyaml-configuration-file-new-implementation # Conflicts: # examples/SOLEIL_examples/p.yaml
There was a problem hiding this comment.
The idea was rather to move get_devices() in the backend and to abstract it in pyaml.
At the end, I would like to have only a single string in the config and a get_devices(name:str) method at the ControlSystem level that return a list of DeviceAccess.
There was a problem hiding this comment.
attach_control_system() should be removed.
There was a problem hiding this comment.
Actually, it is mandatory for Tango dynamic catalogs. An optional 'connected' mode allows the catalog to perform checks to ensure a correct DeviceAccess is built. Specifically to verify the data type (scalar vs. spectrum), ranges, and units.
There was a problem hiding this comment.
I'm still working on it
There was a problem hiding this comment.
The catalog config is linked to control system only.
Could the catalogs field be moved in ControlSystem configuration ?
There was a problem hiding this comment.
If we want to share the catalog between control systems (live and virtual), it would be better to define it at the accelerator level. Otherwise, we would need to update the 'simulators' section to support a more complex structure than just a simple list.
There was a problem hiding this comment.
I don't understand your point, what simulators do here ?
Catalog are control system specific and it would be logical to have it in the Control section whatever the ControlSystem configuration structure.
There was a problem hiding this comment.
What prefix does in the catalog, it is very confusing.
Could it explains unwanted creation of unattached object ?
There was a problem hiding this comment.
OK I think I understand. The build call is wrong in the dynamic catalog. A catalog should only return a temporary config object.
3 participants
Description
Add named
Catalogobjects to the PyAML configuration so device references can be resolved by key at control-system attachment time.The implementation present on this branch is intentionally narrower than the original idea:
DeviceAccessobjects;DeviceAccessconfiguration for now.The goal of this work is still to extend the same mechanism to all PyAML devices over time, not only BPMs.
The catalog API keeps a control-system attachment hook so catalog implementations can be specialized by inheritance. Base PyAML provides
StaticCatalogas the default implementation, but the API is intentionally open to additional catalog classes built in PyAML or external projects.Related Issue
Features implemented on this branch are:
pyaml.accelerator.ConfigModelresolve(key) -> DeviceAccessCatalogResolverso one shared catalog can produce a resolver bound to each control systemStaticCatalogEntryobject so catalog entries are built through the factory like the rest of the configurationControlSystemto declarecatalog: str | Catalog | NoneConfigurationManagerThis PR does not close
#187. It only implements the BPM step of the catalog mechanism. The issue remains open until the same approach is extended to the other PyAML device-bearing object families.Scope and Non-Goals
The implementation on this branch stays within the following scope:
DeviceAccessControlSystem, immediately before BPM attachmentpyaml.bpm.bpmmodels currently implemented in base PyAMLDeviceAccessExpected Design
The implementation currently present on the branch follows this design:
CatalogandCatalogResolverlive underpyaml.configurationCatalog.nameis part of the catalog object itselfCatalog.attach_control_system()returns a resolver bound to one control system; the default implementation returnsselfStaticCatalogis the built-in catalog implementation provided by base PyAMLCatalogStaticCatalogstoresentries: list[StaticCatalogEntry]Accelerator.ConfigModelexposescatalogs: list[Catalog] | NoneAcceleratorstores named catalogs and resolvescontrolsystem.catalogbefore callingfill_device()ControlSystemstores both the configured catalog object and the bound resolverresolve_device()/resolve_devices()to turn BPM keys intoDeviceAccessDeviceAccessobjects directlyChanges to existing functionality
Describe the changes that had to be made to an existing functionality.
Acceleratornow builds and stores named catalogs before devices are attached to control systemsControlSystemnow binds one catalog resolver per control system, even when several control systems share the same catalog objectBPMSimpleModelandBPMTiltOffsetModelnow require catalog keys for BPM device fieldsDeviceAccessconfigurationImplementation Notes
Use these constraints when discussing or extending the implementation:
attach_control_system()hook so inherited catalog implementations can provide contextual resolversStaticCatalog.entriesRecommended areas to review:
pyaml/accelerator.pypyaml/control/controlsystem.pypyaml/configuration/catalog.pypyaml/configuration/static_catalog.pypyaml/configuration/static_catalog_entry.pypyaml/bpm/bpm_model.pypyaml/bpm/bpm_simple_model.pypyaml/bpm/bpm_tiltoffset_model.pyImpacted PyAML Objects
Directly impacted objects and models on this branch:
AcceleratorControlSystemCatalogCatalogResolverStaticCatalogStaticCatalogEntryBPMModelBPMSimpleModelBPMTiltOffsetModelDirectly impacted runtime paths:
ControlSystem.fill_device()ControlSystem.create_bpm_aggregators()Objects intentionally left unchanged by this branch:
RFPlantRFTransmitterBetatronTuneMonitorConfiguration Examples
Accelerator-level static catalog:
Control systems referencing the same named catalog:
BPM using catalog keys:
Inline catalog still supported on one control system:
Testing
The following tests are covered on this branch:
ConfigurationManagertracks catalogs as a named categoryConfigurationManagercan add a catalog-only fragmentVerify that your checklist complies with the project