From 54737d04d9713740ed28032962a9a97beeb5fda8 Mon Sep 17 00:00:00 2001 From: Devan Agrawal Date: Mon, 15 Jul 2024 08:45:19 -0700 Subject: [PATCH 1/3] ENH: implement Client.snap, ._gather_pvs, ._build_snapshot methods --- superscore/client.py | 168 ++++++++++++++++++++++-------- superscore/control_layers/core.py | 5 +- superscore/tests/conftest.py | 19 ++++ superscore/tests/test_client.py | 46 +++++++- superscore/tests/test_model.py | 4 +- 5 files changed, 196 insertions(+), 46 deletions(-) diff --git a/superscore/client.py b/superscore/client.py index f6b1677..4745547 100644 --- a/superscore/client.py +++ b/superscore/client.py @@ -3,14 +3,17 @@ import logging import os from pathlib import Path -from typing import Any, Generator, List, Optional, Union +from typing import Any, Dict, Generator, List, Optional, Union from uuid import UUID from superscore.backends import get_backend from superscore.backends.core import _Backend from superscore.control_layers import ControlLayer from superscore.control_layers.status import TaskStatus -from superscore.model import Entry, Setpoint, Snapshot +from superscore.errors import CommunicationError +from superscore.model import (Collection, Entry, Nestable, Parameter, Readback, + Setpoint, Snapshot) +from superscore.type_hints import AnyEpicsType from superscore.utils import build_abs_path logger = logging.getLogger(__name__) @@ -163,6 +166,35 @@ def compare(self, entry_l: Entry, entry_r: Entry) -> Any: """Compare two entries. Should be of same type, and return a diff""" raise NotImplementedError + def snap(self, entry: Collection) -> Snapshot: + """ + Asyncronously read data for all PVs under ``entry``, and store in a + Snapshot. PVs that can't be read will have an exception as their value. + + Parameters + ---------- + entry : Collection + the Collection to save + + Returns + ------- + Snapshot + a Snapshot corresponding to the input Collection + """ + logger.debug(f"Saving Snapshot for Collection {entry.uuid}") + pvs, _ = self._gather_data(entry) + pvs.extend(Collection.meta_pvs) + values = self.cl.get(pvs) + data = {} + for pv, value in zip(pvs, values): + if isinstance(value, CommunicationError): + logger.debug(f"Couldn't read value for {pv}, storing \"None\"") + data[pv] = None + else: + logger.debug(f"Storing {pv} = {value}") + data[pv] = value + return self._build_snapshot(entry, data) + def apply( self, entry: Union[Setpoint, Snapshot], @@ -194,7 +226,7 @@ def apply( # Gather pv-value list and apply at once status_list = [] - pv_list, data_list = self._gather_data(entry) + pv_list, data_list = self._gather_data(entry, writable_only=True) if sequential: for pv, data in zip(pv_list, data_list): logger.debug(f'Putting {pv} = {data}') @@ -210,57 +242,109 @@ def apply( def _gather_data( self, - entry: Union[Setpoint, Snapshot, UUID], - pv_list: Optional[List[str]] = None, - data_list: Optional[List[Any]] = None - ) -> Optional[tuple[List[str], List[Any]]]: + entry: Union[Entry, UUID], + writable_only: bool = False, + ) -> tuple[List[str], Optional[List[Any]]]: """ - Gather writable pv name - data pairs recursively. - If pv_list and data_list are provided, gathered data will be added to - these lists in-place. If both lists are omitted, this function will return - the two lists after gathering. - - Queries the backend to fill any UUID values found. + Gather PV name - data pairs that are accessible from ``entry``. Queries + the backend to fill any UUIDs found. Parameters ---------- - entry : Union[Setpoint, Snapshot, UUID] - Entry to gather writable data from - pv_list : Optional[List[str]], optional - List of addresses to write data to, by default None - data_list : Optional[List[Any]], optional - List of data to write to addresses in ``pv_list``, by default None + entry : Union[Entry, UUID] + Entry to gather data from + writable_only : bool + If True, only include writable data e.g. omit Readbacks; by default False Returns ------- - Optional[tuple[List[str], List[Any]]] + tuple[List[str], Optional[List[Any]]] the filled pv_list and data_list """ - top_level = False - if (pv_list is None) and (data_list is None): - pv_list = [] - data_list = [] - top_level = True - elif (pv_list is None) or (data_list is None): - raise ValueError( - "Arguments pv_list and data_list must either both be provided " - "or both omitted." - ) + pv_list = [] + data_list = [] + + seen = set() + q = [entry] + while len(q) > 0: + entry = q.pop() + uuid = entry if isinstance(entry, UUID) else entry.uuid + if uuid in seen: + continue + elif isinstance(entry, UUID): + entry = self.backend.get_entry(entry) + seen.add(entry.uuid) + + if isinstance(entry, Nestable): + q.extend(reversed(entry.children)) # preserve execution order + elif isinstance(entry, Readback) and writable_only: + pass + else: # entry is Parameter, Setpoint, or Readback + pv_list.append(entry.pv_name) + if hasattr(entry, "data"): + data_list.append(entry.data) + if getattr(entry, "readback", None) is not None: + q.append(entry.readback) + return pv_list, data_list + + def _build_snapshot( + self, + coll: Collection, + values: Dict[str, AnyEpicsType], + ) -> Snapshot: + """ + Traverse a Collection, assembling a Snapshot using pre-fetched data + along the way - if isinstance(entry, Snapshot): - for child in entry.children: - self._gather_data(child, pv_list, data_list) - elif isinstance(entry, UUID): - child_entry = self.backend.get_entry(entry) - self._gather_data(child_entry, pv_list, data_list) - elif isinstance(entry, Setpoint): - pv_list.append(entry.pv_name) - data_list.append(entry.data) + Parameters + ---------- + coll : Collection + The collection being saved + values : Dict[str, AnyEpicsType] + A dictionary mapping PV names to pre-fetched values - # Readbacks are not writable, and are not gathered + Returns + ------- + Snapshot + A Snapshot corresponding to the input Collection + """ + snapshot = Snapshot( + title=coll.title, + tags=coll.tags.copy(), + origin_collection=coll + ) + + for child in coll.children: + if isinstance(child, UUID): + child = self.backend.get(child) + if isinstance(child, Parameter): + if child.readback is not None: + readback = Readback( + pv_name=child.readback.pv_name, + description=child.readback.description, + data=values[child.readback.pv_name] + ) + else: + readback = None + setpoint = Setpoint( + pv_name=child.pv_name, + description=child.description, + data=values[child.pv_name], + readback=readback + ) + snapshot.children.append(setpoint) + elif isinstance(child, Collection): + snapshot.append(self._build_snapshot(child, values)) + + snapshot.meta_pvs = [] + for pv in Collection.meta_pvs: + readback = Readback( + pv_name=readback.pv_name, + data=values[readback.pv_name] + ) + snapshot.meta_pvs.append(readback) - if top_level: - return pv_list, data_list + return snapshot def validate(self, entry: Entry): """ diff --git a/superscore/control_layers/core.py b/superscore/control_layers/core.py index 889b372..b4fb977 100644 --- a/superscore/control_layers/core.py +++ b/superscore/control_layers/core.py @@ -4,6 +4,7 @@ """ import asyncio import logging +from collections.abc import Iterable from functools import singledispatchmethod from typing import Any, Callable, Dict, List, Optional, Union @@ -99,13 +100,13 @@ def _get_single(self, address: str) -> Any: return asyncio.run(self._get_one(address)) @get.register - def _get_list(self, address: list) -> Any: + def _get_list(self, address: Iterable) -> Any: """Synchronously get a list of ``address``""" async def gathered_coros(): coros = [] for p in address: coros.append(self._get_one(p)) - return await asyncio.gather(*coros) + return await asyncio.gather(*coros, return_exceptions=True) return asyncio.run(gathered_coros()) diff --git a/superscore/tests/conftest.py b/superscore/tests/conftest.py index 8bb730f..6458f96 100644 --- a/superscore/tests/conftest.py +++ b/superscore/tests/conftest.py @@ -661,6 +661,25 @@ def sample_database() -> Root: return root +@pytest.fixture(scope='function') +def parameter_with_readback() -> Parameter: + """ + A simple setpoint-readback parameter pair + """ + readback = Parameter( + uuid="64772c61-c117-445b-b0c8-4c17fd1625d9", + pv_name="RBV", + description="A readback PV", + ) + setpoint = Parameter( + uuid="b508344d-1fe9-473b-8d43-9499d0e8e23f", + pv_name="SET", + description="A setpoint PV", + readback=readback, + ) + return setpoint + + @pytest.fixture(scope='function') def filestore_backend(tmp_path: Path) -> FilestoreBackend: fp = Path(__file__).parent / 'db' / 'filestore.json' diff --git a/superscore/tests/test_client.py b/superscore/tests/test_client.py index bcd293a..8d3a6d6 100644 --- a/superscore/tests/test_client.py +++ b/superscore/tests/test_client.py @@ -6,7 +6,8 @@ from superscore.backends.filestore import FilestoreBackend from superscore.client import Client -from superscore.model import Root +from superscore.errors import CommunicationError +from superscore.model import Parameter, Readback, Root, Setpoint from .conftest import MockTaskStatus @@ -39,6 +40,21 @@ def sscore_cfg(xdg_config_patch: Path): os.environ["XDG_CONFIG_HOME"] = xdg_cfg +def test_gather_data(mock_client, sample_database): + snapshot = sample_database.entries[3] + orig_pv = snapshot.children[0] + dup_pv = Setpoint( + uuid=orig_pv.uuid, + description=orig_pv.description, + pv_name=orig_pv.pv_name, + data="You shouldn't see this", + ) + snapshot.children.append(dup_pv) + pvs, data_list = mock_client._gather_data(snapshot) + assert len(pvs) == len(data_list) == 3 + assert data_list[pvs.index("MY:PREFIX:mtr1.ACCL")] == 2 + + @patch('superscore.control_layers.core.ControlLayer.put') def test_apply(put_mock, mock_client: Client, sample_database: Root): put_mock.return_value = MockTaskStatus() @@ -54,6 +70,34 @@ def test_apply(put_mock, mock_client: Client, sample_database: Root): assert put_mock.call_count == 3 +@patch('superscore.control_layers.core.ControlLayer._get_one') +def test_snap( + get_mock, + mock_client: Client, + sample_database: Root, + parameter_with_readback: Parameter +): + coll = sample_database.entries[2] + coll.children.append(parameter_with_readback) + + get_mock.side_effect = range(5) + snapshot = mock_client.snap(coll) + assert get_mock.call_count == 5 + assert all([snapshot.children[i].data == i for i in range(4)]) # children saved in order + setpoint = snapshot.children[-1] + assert isinstance(setpoint, Setpoint) + assert isinstance(setpoint.readback, Readback) + assert setpoint.readback.data == 4 # readback saved after setpoint + + +@patch('superscore.control_layers.core.ControlLayer._get_one') +def test_snap_exception(get_mock, mock_client: Client, sample_database: Root): + coll = sample_database.entries[2] + get_mock.side_effect = [0, 1, CommunicationError, 3, 4] + snapshot = mock_client.snap(coll) + assert snapshot.children[2].data is None + + def test_from_cfg(sscore_cfg: str): client = Client.from_config() assert isinstance(client.backend, FilestoreBackend) diff --git a/superscore/tests/test_model.py b/superscore/tests/test_model.py index 14cebe8..7a926e8 100644 --- a/superscore/tests/test_model.py +++ b/superscore/tests/test_model.py @@ -26,14 +26,16 @@ def test_serialize_snapshot_roundtrip(): v2 = Setpoint(pv_name="TEST:PV2", description="Second test Value", data=1.8, status=Status.UDF, severity=Severity.INVALID) v3 = Setpoint(pv_name="TEST:PV3", description="Third test Value", data="TRIM", status=Status.DISABLE, severity=Severity.NO_ALARM) v4 = Setpoint(pv_name="TEST:PV4", description="Fourth test Value", data=False, status=Status.HIGH, severity=Severity.MAJOR) + v5 = Setpoint(pv_name="TEST:PV2", description="Fifth test Value", data=None, status=Status.UDF, severity=Severity.INVALID) s1 = Snapshot(title="Snapshot 1", description="Snapshot of Inner Collection", children=[v1, v2]) - s2 = Snapshot(title="Snapshot 2", description="Snapshot of Outer Collection", children=[v3, s1, v4]) + s2 = Snapshot(title="Snapshot 2", description="Snapshot of Outer Collection", children=[v3, s1, v4, v5]) serial = apischema.serialize(Snapshot, s2) deserialized = apischema.deserialize(Snapshot, serial) assert deserialized == s2 assert deserialized.children[0] == v3 assert deserialized.children[1] == s1 assert deserialized.children[2] == v4 + assert deserialized.children[3] == v5 assert deserialized.children[1].children[0] == v1 assert deserialized.children[1].children[1] == v2 From e372952e71899489f20dd158cfceb8135dd5ce20 Mon Sep 17 00:00:00 2001 From: Devan Agrawal Date: Thu, 18 Jul 2024 09:54:11 -0700 Subject: [PATCH 2/3] TST: include Readback in test_client.py::test_apply The test didn't consider Readback handling because sample_database doesn't include any Readbacks. Rather than modify sample_database and possibly affect other tests, I've added a minimal fixture for considering a single Setpoint-Readback pair. --- superscore/tests/conftest.py | 21 +++++++++++++++++++++ superscore/tests/test_client.py | 6 +++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/superscore/tests/conftest.py b/superscore/tests/conftest.py index 6458f96..b465838 100644 --- a/superscore/tests/conftest.py +++ b/superscore/tests/conftest.py @@ -680,6 +680,27 @@ def parameter_with_readback() -> Parameter: return setpoint +@pytest.fixture(scope='function') +def setpoint_with_readback() -> Setpoint: + """ + A simple setpoint-readback value pair + """ + readback = Readback( + uuid="7b30ddba-9fae-4691-988c-07384c29fe22", + pv_name="RBV", + description="A readback PV", + data=False, + ) + setpoint = Setpoint( + uuid="418ed1ab-f1cf-4188-8f4c-ae7cbaf00e6c", + pv_name="SET", + description="A setpoint PV", + data=True, + readback=readback, + ) + return setpoint + + @pytest.fixture(scope='function') def filestore_backend(tmp_path: Path) -> FilestoreBackend: fp = Path(__file__).parent / 'db' / 'filestore.json' diff --git a/superscore/tests/test_client.py b/superscore/tests/test_client.py index 8d3a6d6..0c60f2c 100644 --- a/superscore/tests/test_client.py +++ b/superscore/tests/test_client.py @@ -56,7 +56,7 @@ def test_gather_data(mock_client, sample_database): @patch('superscore.control_layers.core.ControlLayer.put') -def test_apply(put_mock, mock_client: Client, sample_database: Root): +def test_apply(put_mock, mock_client: Client, sample_database: Root, setpoint_with_readback): put_mock.return_value = MockTaskStatus() snap = sample_database.entries[3] mock_client.apply(snap) @@ -69,6 +69,10 @@ def test_apply(put_mock, mock_client: Client, sample_database: Root): mock_client.apply(snap, sequential=True) assert put_mock.call_count == 3 + put_mock.reset_mock() + mock_client.apply(setpoint_with_readback, sequential=True) + assert put_mock.call_count == 1 + @patch('superscore.control_layers.core.ControlLayer._get_one') def test_snap( From 0fcf43b04ae07da9be96c18133f92b5acb7cceb1 Mon Sep 17 00:00:00 2001 From: Devan Agrawal Date: Fri, 19 Jul 2024 10:01:01 -0700 Subject: [PATCH 3/3] DOC: pre-release notes --- .../57-snapshot_saving.rst | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 docs/source/upcoming_release_notes/57-snapshot_saving.rst diff --git a/docs/source/upcoming_release_notes/57-snapshot_saving.rst b/docs/source/upcoming_release_notes/57-snapshot_saving.rst new file mode 100644 index 0000000..ffdb802 --- /dev/null +++ b/docs/source/upcoming_release_notes/57-snapshot_saving.rst @@ -0,0 +1,23 @@ +57 snapshot saving +################# + +API Breaks +---------- +- N/A + +Features +-------- +- ability to save Snapshots for a Collection + +Bugfixes +-------- +- N/A + +Maintenance +----------- +- generalized Client._gather_data to work on any type of Entry +- made Client._gather_data iterative rather than recursive to simplify the conditional logic + +Contributors +------------ +- shilorigins