From c6762bbe37f751249c9067ea80e73423c17c3b7b Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Thu, 21 Mar 2024 13:34:14 -0400 Subject: [PATCH 01/13] First pass at adding create_dir_depth record --- src/pandablocks_ioc/_hdf_ioc.py | 60 ++++++++++++++++++++++++++++----- 1 file changed, 52 insertions(+), 8 deletions(-) diff --git a/src/pandablocks_ioc/_hdf_ioc.py b/src/pandablocks_ioc/_hdf_ioc.py index 034adae0..28205a60 100644 --- a/src/pandablocks_ioc/_hdf_ioc.py +++ b/src/pandablocks_ioc/_hdf_ioc.py @@ -8,14 +8,8 @@ from typing import Callable, Deque, Optional, Union from pandablocks.asyncio import AsyncioClient -from pandablocks.hdf import ( - EndData, - FrameData, - Pipeline, - StartData, - create_default_pipeline, - stop_pipeline, -) +from pandablocks.hdf import (EndData, FrameData, Pipeline, StartData, + create_default_pipeline, stop_pipeline) from pandablocks.responses import EndReason, ReadyData from softioc import alarm, builder from softioc.pythonSoftIoc import RecordWrapper @@ -311,6 +305,8 @@ class HDF5RecordController: _client: AsyncioClient _directory_record: RecordWrapper + _create_directory_record: RecordWrapper + _directory_exists_record: RecordWrapper _file_name_record: RecordWrapper _file_number_record: RecordWrapper _file_format_record: RecordWrapper @@ -352,6 +348,24 @@ def __init__(self, client: AsyncioClient, record_prefix: str): record_prefix + ":" + directory_record_name.upper() ) + create_directory_record_name = EpicsName(self._DATA_PREFIX + ":CreateDirectory") + self._create_directory_record = builder.aOut( + create_directory_record_name, + DESC="Directory creation depth", + ) + self._create_directory_record.add_alias( + record_prefix + ":" + create_directory_record_name.upper() + ) + + directory_exists_name = EpicsName(self._DATA_PREFIX + ":CreateDirectory") + self._directory_exists_record = builder.boolIn( + directory_exists_name, + DESC="Directory exists" + ) + self._directory_exists_record.add_alias( + record_prefix + ":" + directory_exists_name.upper() + ) + file_name_record_name = EpicsName(self._DATA_PREFIX + ":HDFFileName") self._file_name_record = builder.longStringOut( file_name_record_name, @@ -522,6 +536,36 @@ def _parameter_validate(self, record: RecordWrapper, new_val) -> bool: return True async def _update_full_file_path(self, new_val) -> None: + new_path_as_list = os.path.normpath(new_val).split(os.sep) + create_dir_depth = self._create_directory_record.get() + + dirs_to_create = 0 + if create_dir_depth < 0: + if len(new_path_as_list) < abs(create_dir_depth): + dirs_to_create = len(new_path_as_list) + else: + dirs_to_create = abs(create_dir_depth) + elif create_dir_depth > 0: + if len(new_path_as_list) < create_dir_depth: + dirs_to_create = 0 + else: + dirs_to_create = len(new_path_as_list) - create_dir_depth + + create_dir_path = os.path.join(new_path_as_list[:-dirs_to_create]) + for i in reversed(range(dirs_to_create)): + create_dir_path = os.path.join(create_dir_path, new_path_as_list[-i]) + if not os.path.exists(create_dir_path): + try: + os.mkdir(create_dir_path) + except Exception as e: + logging.error(f"Failed to create directory: {create_dir_path}. Error: {str(e)}") + break + + if os.path.exists(new_val): + self._directory_exists_record.set(True) + else: + self._directory_exists_record.set(False) + self._full_file_path_record.set(self._get_filepath()) async def _handle_hdf5_data(self) -> None: From 7c96925970f8b552f1cc28746e1c5c4bafd4877e Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Thu, 21 Mar 2024 14:34:54 -0400 Subject: [PATCH 02/13] Fix typo --- src/pandablocks_ioc/_hdf_ioc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pandablocks_ioc/_hdf_ioc.py b/src/pandablocks_ioc/_hdf_ioc.py index 28205a60..89a814d0 100644 --- a/src/pandablocks_ioc/_hdf_ioc.py +++ b/src/pandablocks_ioc/_hdf_ioc.py @@ -357,7 +357,7 @@ def __init__(self, client: AsyncioClient, record_prefix: str): record_prefix + ":" + create_directory_record_name.upper() ) - directory_exists_name = EpicsName(self._DATA_PREFIX + ":CreateDirectory") + directory_exists_name = EpicsName(self._DATA_PREFIX + ":DirectoryExists") self._directory_exists_record = builder.boolIn( directory_exists_name, DESC="Directory exists" From 17cc85d6152568871f1348173d413709fcf1d13f Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Thu, 21 Mar 2024 14:39:48 -0400 Subject: [PATCH 03/13] Unpack list into arguments --- src/pandablocks_ioc/_hdf_ioc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pandablocks_ioc/_hdf_ioc.py b/src/pandablocks_ioc/_hdf_ioc.py index 89a814d0..caa8fce5 100644 --- a/src/pandablocks_ioc/_hdf_ioc.py +++ b/src/pandablocks_ioc/_hdf_ioc.py @@ -551,7 +551,7 @@ async def _update_full_file_path(self, new_val) -> None: else: dirs_to_create = len(new_path_as_list) - create_dir_depth - create_dir_path = os.path.join(new_path_as_list[:-dirs_to_create]) + create_dir_path = os.path.join(*new_path_as_list[:-dirs_to_create]) for i in reversed(range(dirs_to_create)): create_dir_path = os.path.join(create_dir_path, new_path_as_list[-i]) if not os.path.exists(create_dir_path): From 9c99fe8eb8e1116ed2f0cb3fef942f3c42db5637 Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Fri, 22 Mar 2024 09:57:16 -0400 Subject: [PATCH 04/13] Redo directory creation to use os.makedirs, check if hdf directory exists before opening file --- src/pandablocks_ioc/_hdf_ioc.py | 39 ++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/src/pandablocks_ioc/_hdf_ioc.py b/src/pandablocks_ioc/_hdf_ioc.py index caa8fce5..f5ca33fd 100644 --- a/src/pandablocks_ioc/_hdf_ioc.py +++ b/src/pandablocks_ioc/_hdf_ioc.py @@ -349,8 +349,9 @@ def __init__(self, client: AsyncioClient, record_prefix: str): ) create_directory_record_name = EpicsName(self._DATA_PREFIX + ":CreateDirectory") - self._create_directory_record = builder.aOut( + self._create_directory_record = builder.longOut( create_directory_record_name, + initial_value=0, DESC="Directory creation depth", ) self._create_directory_record.add_alias( @@ -360,7 +361,10 @@ def __init__(self, client: AsyncioClient, record_prefix: str): directory_exists_name = EpicsName(self._DATA_PREFIX + ":DirectoryExists") self._directory_exists_record = builder.boolIn( directory_exists_name, - DESC="Directory exists" + ZNAM="No", + ONAM="Yes", + initial_value=0, + DESC="Directory exists", ) self._directory_exists_record.add_alias( record_prefix + ":" + directory_exists_name.upper() @@ -536,7 +540,9 @@ def _parameter_validate(self, record: RecordWrapper, new_val) -> bool: return True async def _update_full_file_path(self, new_val) -> None: - new_path_as_list = os.path.normpath(new_val).split(os.sep) + full_path_as_list = os.path.normpath(new_val).split(os.sep) + drive = full_path_as_list[0] + new_path_as_list = full_path_as_list[1:] create_dir_depth = self._create_directory_record.get() dirs_to_create = 0 @@ -551,20 +557,23 @@ async def _update_full_file_path(self, new_val) -> None: else: dirs_to_create = len(new_path_as_list) - create_dir_depth - create_dir_path = os.path.join(*new_path_as_list[:-dirs_to_create]) - for i in reversed(range(dirs_to_create)): - create_dir_path = os.path.join(create_dir_path, new_path_as_list[-i]) - if not os.path.exists(create_dir_path): + if dirs_to_create > 0: + if len(full_path_as_list[:-2]) == 1: + # Insert a blank element in index #1 if at root dir. + # Ex. on linux, [''] becomes ['', ''] which joins to '/' + # On windows ['C:'] becomes ['C:', ''] which joins to 'C:\\' + full_path_as_list.insert(1, "") + if os.path.isdir(os.sep.join(full_path_as_list[:-dirs_to_create])) and not os.path.exists(new_val): try: - os.mkdir(create_dir_path) + os.makedirs(new_val, exist_ok=True) + logging.info(f"Created directory {new_val}.") except Exception as e: - logging.error(f"Failed to create directory: {create_dir_path}. Error: {str(e)}") - break + logging.error(f"Failed to create directory {new_val}! Error: {str(e)}") - if os.path.exists(new_val): - self._directory_exists_record.set(True) + if os.path.exists(new_val) and os.access(new_val, os.W_OK): + self._directory_exists_record.set(1) else: - self._directory_exists_record.set(False) + self._directory_exists_record.set(0) self._full_file_path_record.set(self._get_filepath()) @@ -574,6 +583,10 @@ async def _handle_hdf5_data(self) -> None: This method expects to be run as an asyncio Task.""" try: # Set up the hdf buffer + + if not self._directory_exists_record.get() == 1: + raise RuntimeError("Configured HDF directory does not exist or is not writable!") + num_capture: int = self._num_capture_record.get() capture_mode: CaptureMode = CaptureMode(self._capture_mode_record.get()) filepath = self._get_filepath() From 83ee85a63e44ae084e027e0a99bc11089ce69700 Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Wed, 15 May 2024 11:33:02 -0400 Subject: [PATCH 05/13] Refactor to simplify logic, all cases working: --- src/pandablocks_ioc/_hdf_ioc.py | 83 +++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 29 deletions(-) diff --git a/src/pandablocks_ioc/_hdf_ioc.py b/src/pandablocks_ioc/_hdf_ioc.py index 58bca364..9a6ebc66 100644 --- a/src/pandablocks_ioc/_hdf_ioc.py +++ b/src/pandablocks_ioc/_hdf_ioc.py @@ -7,6 +7,8 @@ from importlib.util import find_spec from typing import Callable, Deque, Optional, Union +from pathlib import Path + from pandablocks.asyncio import AsyncioClient from pandablocks.hdf import (EndData, FrameData, Pipeline, StartData, create_default_pipeline, stop_pipeline) @@ -337,6 +339,7 @@ def __init__(self, client: AsyncioClient, record_prefix: str): DESC="File path for HDF5 files", validate=self._parameter_validate, on_update=self._update_full_file_path, + always_update=True, ) add_automatic_pvi_info( PviGroup.HDF, @@ -542,41 +545,63 @@ def _parameter_validate(self, record: RecordWrapper, new_val) -> bool: return True async def _update_full_file_path(self, new_val) -> None: - full_path_as_list = os.path.normpath(new_val).split(os.sep) - drive = full_path_as_list[0] - new_path_as_list = full_path_as_list[1:] + new_path = Path(new_val).absolute() create_dir_depth = self._create_directory_record.get() - dirs_to_create = 0 if create_dir_depth < 0: - if len(new_path_as_list) < abs(create_dir_depth): - dirs_to_create = len(new_path_as_list) - else: - dirs_to_create = abs(create_dir_depth) + dirs_to_create = abs(create_dir_depth) + elif create_dir_depth > len(new_path.parents): + dirs_to_create = 0 elif create_dir_depth > 0: - if len(new_path_as_list) < create_dir_depth: - dirs_to_create = 0 + dirs_to_create = len(new_path.parents) - create_dir_depth + + logging.debug(f"Configured to create up to {dirs_to_create} dirs.") + created_dirs = 0 + for p in reversed(new_path.parents): + if not p.exists(): + created_dirs += 1 + + # Account for target path itself not existing + if not os.path.exists(new_path): + created_dirs += 1 + + logging.debug(f"Need to create {created_dirs} directories.") + + + status_msg = "Ok" + + # Case where all dirs exist + if created_dirs == 0: + if os.access(new_path, os.W_OK): + self._directory_exists_record.set(1) else: - dirs_to_create = len(new_path_as_list) - create_dir_depth - - if dirs_to_create > 0: - if len(full_path_as_list[:-2]) == 1: - # Insert a blank element in index #1 if at root dir. - # Ex. on linux, [''] becomes ['', ''] which joins to '/' - # On windows ['C:'] becomes ['C:', ''] which joins to 'C:\\' - full_path_as_list.insert(1, "") - if os.path.isdir(os.sep.join(full_path_as_list[:-dirs_to_create])) and not os.path.exists(new_val): - try: - os.makedirs(new_val, exist_ok=True) - logging.info(f"Created directory {new_val}.") - except Exception as e: - logging.error(f"Failed to create directory {new_val}! Error: {str(e)}") - - if os.path.exists(new_val) and os.access(new_val, os.W_OK): - self._directory_exists_record.set(1) - else: + status_msg = "Dirs exist but aren't writable." + self._directory_exists_record.set(0) + # Case where we will create directories + elif created_dirs <= dirs_to_create: + + logging.debug(f"Attempting to create {created_dirs} dir(s)...") + try: + os.makedirs(new_path, exist_ok=True) + status_msg = f"Created {created_dirs} dirs." + self._directory_exists_record.set(1) + except PermissionError: + status_msg = "Permission error creating dirs!" + self._directory_exists_record.set(0) + # Case where too many directories need to be created + else: + status_msg = f"Need to create {created_dirs} > {dirs_to_create} dirs." self._directory_exists_record.set(0) + sevr = alarm.NO_ALARM + alrm = None + if self._directory_exists_record.get() == 0: + sevr = alarm.MAJOR_ALARM + alrm = alarm.STATE_ALARM + logging.error(status_msg) + + self._status_message_record.set(status_msg, severity=sevr, alarm=alrm) + self._full_file_path_record.set(self._get_filepath()) async def _handle_hdf5_data(self) -> None: @@ -613,7 +638,7 @@ async def _handle_hdf5_data(self) -> None: buffer.handle_data(data) if buffer.finish_capturing: break - + except CancelledError: logging.info("Capturing task cancelled, closing HDF5 file") self._status_message_record.set("Capturing disabled") From ce18d4d037762490f1302a25aabcd2e03de4fc0f Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Thu, 16 May 2024 10:10:47 -0400 Subject: [PATCH 06/13] All tests passing + reformatting w/ ruff --- src/pandablocks_ioc/_hdf_ioc.py | 50 ++++++++++++++-------- tests/conftest.py | 19 +++++++++ tests/test_hdf_ioc.py | 75 +++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 17 deletions(-) diff --git a/src/pandablocks_ioc/_hdf_ioc.py b/src/pandablocks_ioc/_hdf_ioc.py index 9a6ebc66..f49a8738 100644 --- a/src/pandablocks_ioc/_hdf_ioc.py +++ b/src/pandablocks_ioc/_hdf_ioc.py @@ -5,13 +5,18 @@ from collections import deque from enum import Enum from importlib.util import find_spec -from typing import Callable, Deque, Optional, Union - from pathlib import Path +from typing import Callable, Deque, Optional, Union from pandablocks.asyncio import AsyncioClient -from pandablocks.hdf import (EndData, FrameData, Pipeline, StartData, - create_default_pipeline, stop_pipeline) +from pandablocks.hdf import ( + EndData, + FrameData, + Pipeline, + StartData, + create_default_pipeline, + stop_pipeline, +) from pandablocks.responses import EndReason, ReadyData from softioc import alarm, builder from softioc.pythonSoftIoc import RecordWrapper @@ -338,7 +343,7 @@ def __init__(self, client: AsyncioClient, record_prefix: str): length=path_length, DESC="File path for HDF5 files", validate=self._parameter_validate, - on_update=self._update_full_file_path, + on_update=self._update_directory_path, always_update=True, ) add_automatic_pvi_info( @@ -544,7 +549,9 @@ def _parameter_validate(self, record: RecordWrapper, new_val) -> bool: return False return True - async def _update_full_file_path(self, new_val) -> None: + async def _update_directory_path(self, new_val) -> None: + """Handles writest to the directory path PV, creating + directories based on the setting of the CreateDirectory record""" new_path = Path(new_val).absolute() create_dir_depth = self._create_directory_record.get() dirs_to_create = 0 @@ -553,22 +560,25 @@ async def _update_full_file_path(self, new_val) -> None: elif create_dir_depth > len(new_path.parents): dirs_to_create = 0 elif create_dir_depth > 0: - dirs_to_create = len(new_path.parents) - create_dir_depth + dirs_to_create = len(new_path.parents) - create_dir_depth - logging.debug(f"Configured to create up to {dirs_to_create} dirs.") + logging.debug(f"Permitted to create up to {dirs_to_create} dirs.") created_dirs = 0 for p in reversed(new_path.parents): if not p.exists(): + logging.error(f"{str(p)} does not exist") created_dirs += 1 + else: + logging.info(f"{str(p)} exists") # Account for target path itself not existing if not os.path.exists(new_path): created_dirs += 1 logging.debug(f"Need to create {created_dirs} directories.") - - status_msg = "Ok" + # Default message is "OK" + status_msg = "OK" # Case where all dirs exist if created_dirs == 0: @@ -579,8 +589,7 @@ async def _update_full_file_path(self, new_val) -> None: self._directory_exists_record.set(0) # Case where we will create directories elif created_dirs <= dirs_to_create: - - logging.debug(f"Attempting to create {created_dirs} dir(s)...") + logging.error(f"Attempting to create {created_dirs} dir(s)...") try: os.makedirs(new_path, exist_ok=True) status_msg = f"Created {created_dirs} dirs." @@ -589,7 +598,7 @@ async def _update_full_file_path(self, new_val) -> None: status_msg = "Permission error creating dirs!" self._directory_exists_record.set(0) # Case where too many directories need to be created - else: + else: status_msg = f"Need to create {created_dirs} > {dirs_to_create} dirs." self._directory_exists_record.set(0) @@ -598,10 +607,15 @@ async def _update_full_file_path(self, new_val) -> None: if self._directory_exists_record.get() == 0: sevr = alarm.MAJOR_ALARM alrm = alarm.STATE_ALARM - logging.error(status_msg) + logging.error(status_msg) + else: + logging.debug("Directory exists or has been successfully created.") self._status_message_record.set(status_msg, severity=sevr, alarm=alrm) - + + await self._update_full_file_path(new_val) + + async def _update_full_file_path(self, new_val) -> None: self._full_file_path_record.set(self._get_filepath()) async def _handle_hdf5_data(self) -> None: @@ -612,7 +626,9 @@ async def _handle_hdf5_data(self) -> None: # Set up the hdf buffer if not self._directory_exists_record.get() == 1: - raise RuntimeError("Configured HDF directory does not exist or is not writable!") + raise RuntimeError( + "Configured HDF directory does not exist or is not writable!" + ) num_capture: int = self._num_capture_record.get() capture_mode: CaptureMode = CaptureMode(self._capture_mode_record.get()) @@ -638,7 +654,7 @@ async def _handle_hdf5_data(self) -> None: buffer.handle_data(data) if buffer.finish_capturing: break - + except CancelledError: logging.info("Capturing task cancelled, closing HDF5 file") self._status_message_record.set("Capturing disabled") diff --git a/tests/conftest.py b/tests/conftest.py index 44dd9e76..94dfed72 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,23 @@ # flake8: noqa +import os + from fixtures.mocked_panda import * from fixtures.panda_data import * + + +# Autouse fixture that will set all EPICS networking env vars to use lo interface +# to avoid false failures caused by things like firewalls blocking EPICS traffic. +@pytest.fixture(scope="session", autouse=True) +def configure_epics_environment(): + os.environ["EPICS_CAS_INTF_ADDR_LIST"] = "127.0.0.1" + os.environ["EPICS_CAS_BEACON_ADDR_LIST"] = "127.0.0.1" + os.environ["EPICS_CA_ADDR_LIST"] = "127.0.0.1" + os.environ["EPICS_CAS_AUTO_ADDR_LIST"] = "NO" + os.environ["EPICS_CA_AUTO_BEACON_ADDR_LIST"] = "NO" + + os.environ["EPICS_PVAS_INTF_ADDR_LIST"] = "127.0.0.1" + os.environ["EPICS_PVAS_BEACON_ADDR_LIST"] = "127.0.0.1" + os.environ["EPICS_PVA_ADDR_LIST"] = "127.0.0.1" + os.environ["EPICS_PVAS_AUTO_BEACON_ADDR_LIST"] = "NO" + os.environ["EPICS_PVA_AUTO_ADDR_LIST"] = "NO" diff --git a/tests/test_hdf_ioc.py b/tests/test_hdf_ioc.py index 649e7fb3..3c1b97ff 100644 --- a/tests/test_hdf_ioc.py +++ b/tests/test_hdf_ioc.py @@ -2,6 +2,7 @@ import asyncio import logging +import os from asyncio import CancelledError from collections import deque from multiprocessing.connection import Connection @@ -345,6 +346,12 @@ async def test_hdf5_ioc(hdf5_subprocess_ioc): val = await caget(hdf5_test_prefix + ":Status", datatype=DBR_CHAR_STR) assert val == "OK" + val = await caget(hdf5_test_prefix + ":CreateDirectory") + assert val == 0 + + val = await caget(hdf5_test_prefix + ":DirectoryExists") + assert val == 0 + async def test_hdf5_ioc_parameter_validate_works( hdf5_subprocess_ioc_no_logging_check, tmp_path @@ -384,6 +391,58 @@ async def test_hdf5_ioc_parameter_validate_works( assert val == str(tmp_path) + "/name.h5" # put should have been stopped +@pytest.mark.parametrize( + "create_depth, path, expect_exists", + [ + (0, "panda_test1", False), + (-2, "panda_test2", True), + (-1, "panda_test3/depth_2", False), + (1, "panda_test4/depth_2", True), + (0, "/", False), + (-1, "/panda_test5", False), + ( + 0, + ".", + True, + ), # make sure our final test passes, so final status message is "OK" + ], +) +async def test_hdf5_directory_creation( + hdf5_subprocess_ioc, + tmp_path: Path, + create_depth: int, + path: str, + expect_exists: bool, +): + """Test to see if directory creation/exists works as expected""" + + _, hdf5_test_prefix = hdf5_subprocess_ioc + + target_path = str(os.path.join(tmp_path, path)) + if path.startswith("/"): + target_path = path + + await caput( + hdf5_test_prefix + ":CreateDirectory", + create_depth, + wait=True, + ) + await caput( + hdf5_test_prefix + ":HDFDirectory", + target_path, + datatype=DBR_CHAR_STR, + wait=True, + ) + exists = await caget(hdf5_test_prefix + ":DirectoryExists") + status = await caget(hdf5_test_prefix + ":Status", datatype=DBR_CHAR_STR) + + print(f"Path: {str(path)}, IOC status: {status}, exists: {exists}") + assert (exists > 0) == expect_exists + if expect_exists: + assert os.path.exists(target_path) + assert os.access(target_path, os.W_OK) + + @pytest.mark.parametrize("num_capture", [1, 1000, 10000]) async def test_hdf5_file_writing_first_n( hdf5_subprocess_ioc, tmp_path: Path, caplog, num_capture @@ -504,6 +563,7 @@ async def test_hdf5_file_writing_last_n_endreason_not_ok( assert val == test_filename val = await caget(hdf5_test_prefix + ":HDFFullFilePath", datatype=DBR_CHAR_STR) + print(val) assert val == "/".join([str(tmp_path), test_filename]) # Only a single FrameData in the example data @@ -851,6 +911,9 @@ async def mock_data(scaled, flush_period): for item in slow_dump_expected: yield item + # Assume directory exists for testing. + hdf5_controller._directory_exists_record.set(1) + # Set up all the mocks hdf5_controller._get_filepath = MagicMock( # type: ignore return_value="Some/Filepath" @@ -889,6 +952,9 @@ async def mock_data(scaled, flush_period): for item in doubled_list: yield item + # Assume directory exists for testing. + hdf5_controller._directory_exists_record.set(1) + # Set up all the mocks hdf5_controller._get_filepath = MagicMock( # type: ignore return_value="Some/Filepath" @@ -953,6 +1019,9 @@ async def mock_data(scaled, flush_period): for item in list: yield item + # Assume directory exists for testing. + hdf5_controller._directory_exists_record.set(1) + # Set up all the mocks hdf5_controller._get_filepath = MagicMock( # type: ignore return_value="Some/Filepath" @@ -1016,6 +1085,9 @@ async def mock_data(scaled, flush_period): yield item raise CancelledError + # Assume directory exists for testing. + hdf5_controller._directory_exists_record.set(1) + # Set up all the mocks hdf5_controller._get_filepath = MagicMock( # type: ignore return_value="Some/Filepath" @@ -1074,6 +1146,9 @@ async def mock_data(scaled, flush_period): yield item raise Exception("Test exception") + # Assume directory exists for testing. + hdf5_controller._directory_exists_record.set(1) + # Set up all the mocks hdf5_controller._get_filepath = MagicMock( # type: ignore return_value="Some/Filepath" From 3525f9e283aded91b3f20388591aa64e4e4808f5 Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Thu, 16 May 2024 10:14:56 -0400 Subject: [PATCH 07/13] Re-org parametrize for new test to make formatting nicer --- tests/test_hdf_ioc.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/test_hdf_ioc.py b/tests/test_hdf_ioc.py index 3c1b97ff..02dcb37e 100644 --- a/tests/test_hdf_ioc.py +++ b/tests/test_hdf_ioc.py @@ -400,11 +400,8 @@ async def test_hdf5_ioc_parameter_validate_works( (1, "panda_test4/depth_2", True), (0, "/", False), (-1, "/panda_test5", False), - ( - 0, - ".", - True, - ), # make sure our final test passes, so final status message is "OK" + # make sure our final test passes, so final status message is "OK" + (0, ".", True), ], ) async def test_hdf5_directory_creation( From 56b72a5c3bdee74b43e4376785c65c2aa1e6c21d Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Thu, 16 May 2024 10:16:28 -0400 Subject: [PATCH 08/13] Remove prints used during debugging --- tests/test_hdf_ioc.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/test_hdf_ioc.py b/tests/test_hdf_ioc.py index 02dcb37e..f549d1bb 100644 --- a/tests/test_hdf_ioc.py +++ b/tests/test_hdf_ioc.py @@ -431,9 +431,7 @@ async def test_hdf5_directory_creation( wait=True, ) exists = await caget(hdf5_test_prefix + ":DirectoryExists") - status = await caget(hdf5_test_prefix + ":Status", datatype=DBR_CHAR_STR) - print(f"Path: {str(path)}, IOC status: {status}, exists: {exists}") assert (exists > 0) == expect_exists if expect_exists: assert os.path.exists(target_path) @@ -560,7 +558,6 @@ async def test_hdf5_file_writing_last_n_endreason_not_ok( assert val == test_filename val = await caget(hdf5_test_prefix + ":HDFFullFilePath", datatype=DBR_CHAR_STR) - print(val) assert val == "/".join([str(tmp_path), test_filename]) # Only a single FrameData in the example data From 7eb4a0e1127b7de4f9032267b78af22d923b2a2a Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Thu, 16 May 2024 10:26:11 -0400 Subject: [PATCH 09/13] Fix typo --- src/pandablocks_ioc/_hdf_ioc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pandablocks_ioc/_hdf_ioc.py b/src/pandablocks_ioc/_hdf_ioc.py index f49a8738..78e06349 100644 --- a/src/pandablocks_ioc/_hdf_ioc.py +++ b/src/pandablocks_ioc/_hdf_ioc.py @@ -550,7 +550,7 @@ def _parameter_validate(self, record: RecordWrapper, new_val) -> bool: return True async def _update_directory_path(self, new_val) -> None: - """Handles writest to the directory path PV, creating + """Handles writes to the directory path PV, creating directories based on the setting of the CreateDirectory record""" new_path = Path(new_val).absolute() create_dir_depth = self._create_directory_record.get() From 7b20d698521006e10e886706abfc46dd68b55684 Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Thu, 16 May 2024 15:54:41 -0400 Subject: [PATCH 10/13] Apply some suggestions from review --- src/pandablocks_ioc/_hdf_ioc.py | 34 +++++++------ tests/test_hdf_ioc.py | 90 +++++++++++++++++++++++---------- 2 files changed, 80 insertions(+), 44 deletions(-) diff --git a/src/pandablocks_ioc/_hdf_ioc.py b/src/pandablocks_ioc/_hdf_ioc.py index 78e06349..727f0530 100644 --- a/src/pandablocks_ioc/_hdf_ioc.py +++ b/src/pandablocks_ioc/_hdf_ioc.py @@ -554,56 +554,58 @@ async def _update_directory_path(self, new_val) -> None: directories based on the setting of the CreateDirectory record""" new_path = Path(new_val).absolute() create_dir_depth = self._create_directory_record.get() - dirs_to_create = 0 + max_dirs_to_create = 0 if create_dir_depth < 0: - dirs_to_create = abs(create_dir_depth) + max_dirs_to_create = abs(create_dir_depth) elif create_dir_depth > len(new_path.parents): - dirs_to_create = 0 + max_dirs_to_create = 0 elif create_dir_depth > 0: - dirs_to_create = len(new_path.parents) - create_dir_depth + max_dirs_to_create = len(new_path.parents) - create_dir_depth - logging.debug(f"Permitted to create up to {dirs_to_create} dirs.") - created_dirs = 0 + logging.debug(f"Permitted to create up to {max_dirs_to_create} dirs.") + dirs_to_create = 0 for p in reversed(new_path.parents): if not p.exists(): - logging.error(f"{str(p)} does not exist") - created_dirs += 1 + if dirs_to_create == 0: + # First directory level that does not exist, log it. + logging.error(f"All dir from {str(p)} and below do not exist!") + dirs_to_create += 1 else: logging.info(f"{str(p)} exists") # Account for target path itself not existing if not os.path.exists(new_path): - created_dirs += 1 + dirs_to_create += 1 - logging.debug(f"Need to create {created_dirs} directories.") + logging.debug(f"Need to create {dirs_to_create} directories.") # Default message is "OK" status_msg = "OK" # Case where all dirs exist - if created_dirs == 0: + if dirs_to_create == 0: if os.access(new_path, os.W_OK): self._directory_exists_record.set(1) else: status_msg = "Dirs exist but aren't writable." self._directory_exists_record.set(0) # Case where we will create directories - elif created_dirs <= dirs_to_create: - logging.error(f"Attempting to create {created_dirs} dir(s)...") + elif dirs_to_create <= max_dirs_to_create: + logging.info(f"Attempting to create {dirs_to_create} dir(s)...") try: os.makedirs(new_path, exist_ok=True) - status_msg = f"Created {created_dirs} dirs." + status_msg = f"Created {dirs_to_create} dirs." self._directory_exists_record.set(1) except PermissionError: status_msg = "Permission error creating dirs!" self._directory_exists_record.set(0) # Case where too many directories need to be created else: - status_msg = f"Need to create {created_dirs} > {dirs_to_create} dirs." + status_msg = f"Need to create {dirs_to_create} > {max_dirs_to_create} dirs." self._directory_exists_record.set(0) sevr = alarm.NO_ALARM - alrm = None + alrm = alarm.NO_ALARM if self._directory_exists_record.get() == 0: sevr = alarm.MAJOR_ALARM alrm = alarm.STATE_ALARM diff --git a/tests/test_hdf_ioc.py b/tests/test_hdf_ioc.py index f549d1bb..9ba43721 100644 --- a/tests/test_hdf_ioc.py +++ b/tests/test_hdf_ioc.py @@ -229,6 +229,10 @@ async def hdf5_controller( test_prefix, hdf5_test_prefix = new_random_hdf5_prefix hdf5_controller = HDF5RecordController(AsyncioClient("localhost"), test_prefix) + + # When using tests w/o CA, need to manually set _directory_exists to 1 + hdf5_controller._directory_exists_record.set(1) + yield hdf5_controller # Give time for asyncio to fully close its connections await asyncio.sleep(0) @@ -392,32 +396,35 @@ async def test_hdf5_ioc_parameter_validate_works( @pytest.mark.parametrize( - "create_depth, path, expect_exists", + "create_depth, path, expect_exists, restrict_permissions", [ - (0, "panda_test1", False), - (-2, "panda_test2", True), - (-1, "panda_test3/depth_2", False), - (1, "panda_test4/depth_2", True), - (0, "/", False), - (-1, "/panda_test5", False), - # make sure our final test passes, so final status message is "OK" - (0, ".", True), + (0, ".", True, False), + (0, "panda_test1", False, False), + (-2, "panda_test2", True, False), + (-1, "panda_test3/depth_2", False, False), + (1, "panda_test4/depth_2", True, False), + (0, ".", False, True), + (1, "panda_test5", False, True), + (-1, "panda_test6", False, True), ], ) -async def test_hdf5_directory_creation( +async def test_hdf5_dir_creation( hdf5_subprocess_ioc, tmp_path: Path, create_depth: int, path: str, expect_exists: bool, + restrict_permissions: bool, ): """Test to see if directory creation/exists works as expected""" + if restrict_permissions: + # Artificially restrict perms for temp folder to simulate perm issues. + tmp_path.chmod(0o444) + _, hdf5_test_prefix = hdf5_subprocess_ioc - target_path = str(os.path.join(tmp_path, path)) - if path.startswith("/"): - target_path = path + target_path = str(tmp_path / path) await caput( hdf5_test_prefix + ":CreateDirectory", @@ -437,6 +444,48 @@ async def test_hdf5_directory_creation( assert os.path.exists(target_path) assert os.access(target_path, os.W_OK) + if restrict_permissions: + # Put back default permissions + tmp_path.chmod(0o700) + + +async def test_hdf5_file_writing_no_dir(hdf5_subprocess_ioc, tmp_path: Path, caplog): + """Test that if dir doesn't exist, HDF file writing fails with a runtime error""" + test_prefix, hdf5_test_prefix = hdf5_subprocess_ioc + + test_dir = tmp_path + test_filename = "test.h5" + await caput( + hdf5_test_prefix + ":HDFDirectory", + str(test_dir / "panda_test1"), + wait=True, + datatype=DBR_CHAR_STR, + ) + + exists = await caget(hdf5_test_prefix + ":DirectoryExists") + assert exists == 0 + + await caput( + hdf5_test_prefix + ":HDFFileName", "name.h5", wait=True, datatype=DBR_CHAR_STR + ) + await caput( + hdf5_test_prefix + ":HDFFileName", + test_filename, + wait=True, + timeout=TIMEOUT, + datatype=DBR_CHAR_STR, + ) + + val = await caget(hdf5_test_prefix + ":HDFFullFilePath", datatype=DBR_CHAR_STR) + assert val == "/".join([str(tmp_path), "panda_test1", test_filename]) + + await caput(hdf5_test_prefix + ":NumCapture", 1, wait=True, timeout=TIMEOUT) + + await caput(hdf5_test_prefix + ":Capture", 1, wait=True, timeout=TIMEOUT) + + val = await caget(hdf5_test_prefix + ":Status", datatype=DBR_CHAR_STR) + assert val == "Capture disabled, unexpected exception" + @pytest.mark.parametrize("num_capture", [1, 1000, 10000]) async def test_hdf5_file_writing_first_n( @@ -905,9 +954,6 @@ async def mock_data(scaled, flush_period): for item in slow_dump_expected: yield item - # Assume directory exists for testing. - hdf5_controller._directory_exists_record.set(1) - # Set up all the mocks hdf5_controller._get_filepath = MagicMock( # type: ignore return_value="Some/Filepath" @@ -946,9 +992,6 @@ async def mock_data(scaled, flush_period): for item in doubled_list: yield item - # Assume directory exists for testing. - hdf5_controller._directory_exists_record.set(1) - # Set up all the mocks hdf5_controller._get_filepath = MagicMock( # type: ignore return_value="Some/Filepath" @@ -1013,9 +1056,6 @@ async def mock_data(scaled, flush_period): for item in list: yield item - # Assume directory exists for testing. - hdf5_controller._directory_exists_record.set(1) - # Set up all the mocks hdf5_controller._get_filepath = MagicMock( # type: ignore return_value="Some/Filepath" @@ -1079,9 +1119,6 @@ async def mock_data(scaled, flush_period): yield item raise CancelledError - # Assume directory exists for testing. - hdf5_controller._directory_exists_record.set(1) - # Set up all the mocks hdf5_controller._get_filepath = MagicMock( # type: ignore return_value="Some/Filepath" @@ -1140,9 +1177,6 @@ async def mock_data(scaled, flush_period): yield item raise Exception("Test exception") - # Assume directory exists for testing. - hdf5_controller._directory_exists_record.set(1) - # Set up all the mocks hdf5_controller._get_filepath = MagicMock( # type: ignore return_value="Some/Filepath" From 3b02b8feeb5dfbfbbe485ad8ad64ad7a59f91c2a Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Fri, 31 May 2024 15:16:55 -0400 Subject: [PATCH 11/13] Add doc entries for new signals, commit some improvement suggestions --- docs/how-to/capture-hdf.md | 2 ++ src/pandablocks_ioc/_hdf_ioc.py | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/docs/how-to/capture-hdf.md b/docs/how-to/capture-hdf.md index f68e644e..10c754b0 100644 --- a/docs/how-to/capture-hdf.md +++ b/docs/how-to/capture-hdf.md @@ -9,6 +9,8 @@ These can be viewed from the DATA screen. ``` - The file directory and name are chosen with `:DATA:HDFDirectory` and `:DATA:HDFFileName`. +- The number of directories that the IOC is allowed to create provided they don't exist is determined by `:DATA:CreateDirectory`. The behavior of this signal is the same the identical PV in [`areaDetector`](https://areadetector.github.io/areaDetector/ADCore/NDPluginFile.html). +- `DATA:DirectoryExists` represents whether or not the directory specified exists and is writable by the user under which the IOC is running. - `:DATA:NumCapture` is the number of frames to capture in the file. - `:DATA:NumCaptured` is the number of frames written to file. - `:DATA:NumReceived` is the number of frames received from the panda. diff --git a/src/pandablocks_ioc/_hdf_ioc.py b/src/pandablocks_ioc/_hdf_ioc.py index 727f0530..9e68c558 100644 --- a/src/pandablocks_ioc/_hdf_ioc.py +++ b/src/pandablocks_ioc/_hdf_ioc.py @@ -362,6 +362,12 @@ def __init__(self, client: AsyncioClient, record_prefix: str): initial_value=0, DESC="Directory creation depth", ) + add_automatic_pvi_info( + PviGroup.HDF, + self._create_directory_record, + create_directory_record_name, + builder.longOut, + ) self._create_directory_record.add_alias( record_prefix + ":" + create_directory_record_name.upper() ) @@ -374,6 +380,12 @@ def __init__(self, client: AsyncioClient, record_prefix: str): initial_value=0, DESC="Directory exists", ) + add_automatic_pvi_info( + PviGroup.HDF, + self._directory_exists_record, + directory_exists_name, + builder.boolIn, + ) self._directory_exists_record.add_alias( record_prefix + ":" + directory_exists_name.upper() ) @@ -585,13 +597,14 @@ async def _update_directory_path(self, new_val) -> None: # Case where all dirs exist if dirs_to_create == 0: if os.access(new_path, os.W_OK): + status_msg = "Dir exists and is writable" self._directory_exists_record.set(1) else: status_msg = "Dirs exist but aren't writable." self._directory_exists_record.set(0) # Case where we will create directories elif dirs_to_create <= max_dirs_to_create: - logging.info(f"Attempting to create {dirs_to_create} dir(s)...") + logging.debug(f"Attempting to create {dirs_to_create} dir(s)...") try: os.makedirs(new_path, exist_ok=True) status_msg = f"Created {dirs_to_create} dirs." @@ -611,7 +624,7 @@ async def _update_directory_path(self, new_val) -> None: alrm = alarm.STATE_ALARM logging.error(status_msg) else: - logging.debug("Directory exists or has been successfully created.") + logging.debug(status_msg) self._status_message_record.set(status_msg, severity=sevr, alarm=alrm) From 0a6e2a1d8bfafd3d2fbb3982c87503ac45163986 Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Tue, 4 Jun 2024 17:04:49 -0400 Subject: [PATCH 12/13] Apply remaining suggestions from review --- docs/how-to/capture-hdf.md | 2 +- src/pandablocks_ioc/_hdf_ioc.py | 3 +- tests/test-bobfiles/DATA.bob | 54 ++++++++++++++++++++++++++++----- tests/test_hdf_ioc.py | 5 +-- 4 files changed, 51 insertions(+), 13 deletions(-) diff --git a/docs/how-to/capture-hdf.md b/docs/how-to/capture-hdf.md index 10c754b0..df57ea0b 100644 --- a/docs/how-to/capture-hdf.md +++ b/docs/how-to/capture-hdf.md @@ -9,7 +9,7 @@ These can be viewed from the DATA screen. ``` - The file directory and name are chosen with `:DATA:HDFDirectory` and `:DATA:HDFFileName`. -- The number of directories that the IOC is allowed to create provided they don't exist is determined by `:DATA:CreateDirectory`. The behavior of this signal is the same the identical PV in [`areaDetector`](https://areadetector.github.io/areaDetector/ADCore/NDPluginFile.html). +- The number of directories that the IOC is allowed to create provided they don't exist is determined by `:DATA:CreateDirectory`. The behavior of this signal is the same as the identical PV in [`areaDetector`](https://areadetector.github.io/areaDetector/ADCore/NDPluginFile.html). - `DATA:DirectoryExists` represents whether or not the directory specified exists and is writable by the user under which the IOC is running. - `:DATA:NumCapture` is the number of frames to capture in the file. - `:DATA:NumCaptured` is the number of frames written to file. diff --git a/src/pandablocks_ioc/_hdf_ioc.py b/src/pandablocks_ioc/_hdf_ioc.py index 9e68c558..712ac77f 100644 --- a/src/pandablocks_ioc/_hdf_ioc.py +++ b/src/pandablocks_ioc/_hdf_ioc.py @@ -620,8 +620,7 @@ async def _update_directory_path(self, new_val) -> None: sevr = alarm.NO_ALARM alrm = alarm.NO_ALARM if self._directory_exists_record.get() == 0: - sevr = alarm.MAJOR_ALARM - alrm = alarm.STATE_ALARM + sevr = alarm.MAJOR_ALARM, alrm = alarm.STATE_ALARM logging.error(status_msg) else: logging.debug(status_msg) diff --git a/tests/test-bobfiles/DATA.bob b/tests/test-bobfiles/DATA.bob index 4a8a8fd4..e4ed651b 100644 --- a/tests/test-bobfiles/DATA.bob +++ b/tests/test-bobfiles/DATA.bob @@ -3,7 +3,7 @@ 0 0 506 - 413 + 463 4 4 @@ -30,7 +30,7 @@ 5 30 496 - 106 + 156 true Label @@ -52,7 +52,7 @@ Label - Hdf File Name + Createdirectory 0 25 250 @@ -60,19 +60,57 @@ TextEntry - TEST_PREFIX:DATA:HDF_FILE_NAME + TEST_PREFIX:DATA:CreateDirectory 255 25 205 20 1 + + + Label + Directoryexists + 0 + 50 + 250 + 20 + + + TextUpdate + TEST_PREFIX:DATA:DirectoryExists + 255 + 50 + 205 + 20 + + + + + 1 + + + Label + Hdf File Name + 0 + 75 + 250 + 20 + + + TextEntry + TEST_PREFIX:DATA:HDF_FILE_NAME + 255 + 75 + 205 + 20 + 1 6 Label Hdf Full File Path 0 - 50 + 100 250 20 @@ -80,7 +118,7 @@ TextUpdate TEST_PREFIX:DATA:HDF_FULL_FILE_PATH 255 - 50 + 100 205 20 @@ -94,7 +132,7 @@ CAPTURE 5 - 141 + 191 496 206 true @@ -268,7 +306,7 @@ OUTPUTS 5 - 352 + 402 496 56 true diff --git a/tests/test_hdf_ioc.py b/tests/test_hdf_ioc.py index 9ba43721..e5f013a4 100644 --- a/tests/test_hdf_ioc.py +++ b/tests/test_hdf_ioc.py @@ -406,6 +406,7 @@ async def test_hdf5_ioc_parameter_validate_works( (0, ".", False, True), (1, "panda_test5", False, True), (-1, "panda_test6", False, True), + (10, "panda_test7", False, False), ], ) async def test_hdf5_dir_creation( @@ -616,9 +617,9 @@ async def test_hdf5_file_writing_last_n_endreason_not_ok( ) assert await caget(hdf5_test_prefix + ":NumCapture") == num_capture - # Initially Status should be "OK" + # Initially Status should be "Dir exists and is writable" val = await caget(hdf5_test_prefix + ":Status", datatype=DBR_CHAR_STR) - assert val == "OK" + assert val == "Dir exists and is writable" await caput(hdf5_test_prefix + ":Capture", 1, wait=True, timeout=TIMEOUT) From 028917868f7ac1110e8e41298681f01f7a2b7a08 Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Tue, 4 Jun 2024 17:12:11 -0400 Subject: [PATCH 13/13] Minor cleanup --- src/pandablocks_ioc/_hdf_ioc.py | 6 +----- tests/test_hdf_ioc.py | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/pandablocks_ioc/_hdf_ioc.py b/src/pandablocks_ioc/_hdf_ioc.py index 712ac77f..46d4c6bf 100644 --- a/src/pandablocks_ioc/_hdf_ioc.py +++ b/src/pandablocks_ioc/_hdf_ioc.py @@ -591,9 +591,6 @@ async def _update_directory_path(self, new_val) -> None: logging.debug(f"Need to create {dirs_to_create} directories.") - # Default message is "OK" - status_msg = "OK" - # Case where all dirs exist if dirs_to_create == 0: if os.access(new_path, os.W_OK): @@ -617,12 +614,11 @@ async def _update_directory_path(self, new_val) -> None: status_msg = f"Need to create {dirs_to_create} > {max_dirs_to_create} dirs." self._directory_exists_record.set(0) - sevr = alarm.NO_ALARM - alrm = alarm.NO_ALARM if self._directory_exists_record.get() == 0: sevr = alarm.MAJOR_ALARM, alrm = alarm.STATE_ALARM logging.error(status_msg) else: + sevr = alarm.NO_ALARM, alrm = alarm.NO_ALARM logging.debug(status_msg) self._status_message_record.set(status_msg, severity=sevr, alarm=alrm) diff --git a/tests/test_hdf_ioc.py b/tests/test_hdf_ioc.py index e5f013a4..d9d16427 100644 --- a/tests/test_hdf_ioc.py +++ b/tests/test_hdf_ioc.py @@ -619,7 +619,7 @@ async def test_hdf5_file_writing_last_n_endreason_not_ok( # Initially Status should be "Dir exists and is writable" val = await caget(hdf5_test_prefix + ":Status", datatype=DBR_CHAR_STR) - assert val == "Dir exists and is writable" + assert val == "OK" await caput(hdf5_test_prefix + ":Capture", 1, wait=True, timeout=TIMEOUT)