From 8bf0615208f58f2c9db8fe106c3877e077fde4cf Mon Sep 17 00:00:00 2001 From: YooSunyoung Date: Thu, 26 Sep 2024 12:13:19 +0200 Subject: [PATCH 1/3] More strict nexus structure validation and tests. --- src/beamlime/applications/_nexus_helpers.py | 56 ++++++- tests/applications/nexus_helpers_test.py | 153 ++++++++++++++++---- tests/applications/streaming_module_test.py | 14 ++ tests/applications/ymir_detectors.json | 27 ++-- tests/helpers/tests/applications/data.py | 2 +- 5 files changed, 204 insertions(+), 48 deletions(-) diff --git a/src/beamlime/applications/_nexus_helpers.py b/src/beamlime/applications/_nexus_helpers.py index 1390eb1c..b04af080 100644 --- a/src/beamlime/applications/_nexus_helpers.py +++ b/src/beamlime/applications/_nexus_helpers.py @@ -182,6 +182,48 @@ def _validate_module_keys( ) +def _validate_f144_module_spec( + module_spec: StreamModuleValue, +) -> None: + """Validate the f144 module.""" + if len(module_spec.parent["children"]) != 1: + raise InvalidNexusStructureError( + "Group containing f144 module should have exactly one child" + ) + if module_spec.dtype is None or module_spec.value_units is None: + raise InvalidNexusStructureError( + "f144 module spec should have dtype and value_units" + ) + + +def _validate_f144_module_values( + key_value_dict: dict[StreamModuleKey, StreamModuleValue], +) -> None: + """Validate the module values for the f144 module.""" + for key, value in key_value_dict.items(): + if key.module_type == "f144": + _validate_f144_module_spec(value) + + +def _validate_ev44_module_spec( + module_spec: StreamModuleValue, +) -> None: + """Validate the ev44 module.""" + if len(module_spec.parent["children"]) != 1: + raise InvalidNexusStructureError( + "Group containing ev44 module should have exactly one child" + ) + + +def _validate_ev44_module_values( + key_value_dict: dict[StreamModuleKey, StreamModuleValue], +) -> None: + """Validate the module values for the ev44 module.""" + for key, value in key_value_dict.items(): + if key.module_type == "ev44": + _validate_ev44_module_spec(value) + + def collect_streaming_modules( structure: Mapping, ) -> dict[StreamModuleKey, StreamModuleValue]: @@ -222,7 +264,10 @@ def collect_streaming_modules( ) _validate_module_configs(key_value_pairs) _validate_module_keys(key_value_pairs) - return dict(key_value_pairs) + key_value_dict = dict(key_value_pairs) + _validate_f144_module_values(key_value_dict) + _validate_ev44_module_values(key_value_dict) + return key_value_dict def create_dataset( @@ -258,8 +303,7 @@ def _is_monitor(group: NexusGroup) -> bool: def _initialize_ev44(module_spec: StreamModuleValue) -> NexusGroup: parent = module_spec.parent - if len(parent['children']) != 1: - raise ValueError('Group containing ev44 module should have exactly one child') + _validate_ev44_module_spec(module_spec) group: NexusGroup = cast(NexusGroup, parent.copy()) group['children'] = [ create_dataset( @@ -346,11 +390,7 @@ def _merge_ev44(group: NexusGroup, data: DeserializedMessage) -> None: def _initialize_f144(module_spec: StreamModuleValue) -> NexusGroup: parent = module_spec.parent - if len(parent['children']) != 1: - raise ValueError('Group containing f144 module should have exactly one child') - if module_spec.dtype is None: - raise ValueError('f144 module spec should have dtype') - + _validate_f144_module_spec(module_spec) group: NexusGroup = cast(NexusGroup, parent.copy()) group["children"] = [ create_dataset( diff --git a/tests/applications/nexus_helpers_test.py b/tests/applications/nexus_helpers_test.py index 24f08fa9..5791265e 100644 --- a/tests/applications/nexus_helpers_test.py +++ b/tests/applications/nexus_helpers_test.py @@ -1,7 +1,6 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2024 Scipp contributors (https://github.com/scipp) import hashlib -import json import pathlib from collections.abc import Generator, Mapping @@ -9,6 +8,7 @@ import pytest from beamlime.applications._nexus_helpers import ( + InvalidNexusStructureError, StreamModuleKey, StreamModuleValue, collect_streaming_modules, @@ -112,21 +112,6 @@ def test_find_nexus_structure_not_found_raises() -> None: find_nexus_structure({}, ("b0",)) -def test_invalid_nexus_template_multiple_module_placeholders() -> None: - with open(pathlib.Path(__file__).parent / "multiple_modules_datagroup.json") as f: - modules = collect_streaming_modules(json.load(f)) - - key = StreamModuleKey("ev44", "hypothetical_detector", "ymir_00") - spec = modules[key] - with pytest.raises(ValueError, match="should have exactly one child"): - merge_message_into_nexus_store( - module_key=key, - module_spec=spec, - nexus_store={}, - data={}, # data does not matter since it reaches the error first. - ) - - def test_ymir_detector_template_checksum() -> None: """Test that the ymir template with detectors is updated. @@ -308,6 +293,46 @@ def test_nxevent_data_ev44_generator_yields_frame_by_frame() -> None: next(ev44) +def test_ev44_merge_no_children_raises() -> None: + key = StreamModuleKey("ev44", "", "") + wrong_value = StreamModuleValue( + path=("",), + parent={"children": []}, + dtype="int32", + value_units="km", + ) + with pytest.raises( + InvalidNexusStructureError, + match="Group containing ev44 module should have exactly one child", + ): + merge_message_into_nexus_store( + module_key=key, + module_spec=wrong_value, + nexus_store={}, + data={}, + ) + + +def test_ev44_merge_too_many_children_raises() -> None: + key = StreamModuleKey("ev44", "", "") + wrong_value = StreamModuleValue( + path=("",), + parent={"children": []}, + dtype="int32", + value_units="km", + ) + with pytest.raises( + InvalidNexusStructureError, + match="Group containing ev44 module should have exactly one child", + ): + merge_message_into_nexus_store( + module_key=key, + module_spec=wrong_value, + nexus_store={}, + data={}, + ) + + @pytest.fixture() def nexus_template_with_streamed_log(dtype): return { @@ -343,7 +368,7 @@ def f144_event_generator(shape, dtype): @pytest.mark.parametrize('shape', [(1,), (2,), (2, 2)]) @pytest.mark.parametrize('dtype', ['int32', 'uint32', 'float32', 'float64', 'bool']) -def test_f144(nexus_template_with_streamed_log, shape, dtype): +def test_f144_merge(nexus_template_with_streamed_log, shape, dtype): modules = collect_streaming_modules(nexus_template_with_streamed_log) f144_modules = { key: value for key, value in modules.items() if key.module_type == 'f144' @@ -370,6 +395,86 @@ def test_f144(nexus_template_with_streamed_log, shape, dtype): assert values['attributes'][0]['values'] == 'km' +def test_f144_merge_no_children_raises(): + key = StreamModuleKey(module_type='f144', topic='', source='') + wrong_value = StreamModuleValue( + path=('',), + parent={'children': []}, + dtype='int32', + value_units='km', + ) + with pytest.raises( + InvalidNexusStructureError, + match="Group containing f144 module should have exactly one child", + ): + merge_message_into_nexus_store( + module_key=key, + module_spec=wrong_value, + nexus_store={}, + data={}, + ) + + +def test_f144_merge_too_many_children_raises(): + key = StreamModuleKey(module_type='f144', topic='', source='') + wrong_value = StreamModuleValue( + path=('',), + parent={'children': [{}, {}]}, + dtype='int32', + value_units='km', + ) + with pytest.raises( + InvalidNexusStructureError, + match="Group containing f144 module should have exactly one child", + ): + merge_message_into_nexus_store( + module_key=key, + module_spec=wrong_value, + nexus_store={}, + data={}, + ) + + +def test_f144_merge_missing_dtype_raises(): + key = StreamModuleKey(module_type='f144', topic='', source='') + wrong_value = StreamModuleValue( + path=('',), + parent={'children': [{}]}, + dtype=None, + value_units='km', + ) + with pytest.raises( + InvalidNexusStructureError, + match="f144 module spec should have dtype and value_units", + ): + merge_message_into_nexus_store( + module_key=key, + module_spec=wrong_value, + nexus_store={}, + data={}, + ) + + +def test_f144_merge_missing_value_units_raises(): + key = StreamModuleKey(module_type='f144', topic='', source='') + wrong_value = StreamModuleValue( + path=('',), + parent={'children': [{}]}, + dtype='int32', + value_units=None, + ) + with pytest.raises( + InvalidNexusStructureError, + match="f144 module spec should have dtype and value_units", + ): + merge_message_into_nexus_store( + module_key=key, + module_spec=wrong_value, + nexus_store={}, + data={}, + ) + + @pytest.fixture() def nexus_template_with_streamed_tdct(): return { @@ -401,7 +506,7 @@ def tdct_event_generator(): max_last_timestamp = timestamps.max() -def test_tdct(nexus_template_with_streamed_tdct: dict): +def test_tdct_merge(nexus_template_with_streamed_tdct: dict): modules = collect_streaming_modules(nexus_template_with_streamed_tdct) tdct_modules = { key: value for key, value in modules.items() if key.module_type == 'tdct' @@ -423,15 +528,3 @@ def test_tdct(nexus_template_with_streamed_tdct: dict): assert np.issubdtype( tdct['config']['values'].dtype, np.dtype(tdct['config']['dtype']) ) - - -@pytest.fixture() -def nexus_template_with_mixed_streams( - nexus_template_with_streamed_log, nexus_template_with_streamed_tdct -): - return { - "children": [ - nexus_template_with_streamed_log, - nexus_template_with_streamed_tdct, - ], - } diff --git a/tests/applications/streaming_module_test.py b/tests/applications/streaming_module_test.py index 5a343136..6f86d0bc 100644 --- a/tests/applications/streaming_module_test.py +++ b/tests/applications/streaming_module_test.py @@ -1,6 +1,9 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2024 Scipp contributors (https://github.com/scipp) +import json +import pathlib + import pytest from beamlime.applications._nexus_helpers import ( @@ -28,6 +31,15 @@ def _make_group_with_module_place_holder( } +def test_invalid_nexus_template_multiple_module_placeholders() -> None: + with open(pathlib.Path(__file__).parent / "multiple_modules_datagroup.json") as f: + with pytest.raises( + InvalidNexusStructureError, + match="Group containing ev44 module should have exactly one child", + ): + collect_streaming_modules(json.load(f)) + + def test_collect_streaming_modules_invalid_missing_topic_raises() -> None: invalid_structure = { "children": [ @@ -122,11 +134,13 @@ def test_collect_streaming_modules_nxlogs(ymir: dict) -> None: "source": "delay_source_chopper", "topic": "ymir_motion", "dtype": "double", + "value_units": "s", }, } ], }, dtype="double", + value_units="s", ), StreamModuleKey( "f144", "ymir_motion", "YMIR-SEE:SE-LS336-004:KRDG0" diff --git a/tests/applications/ymir_detectors.json b/tests/applications/ymir_detectors.json index 8f4fdcec..895618dd 100644 --- a/tests/applications/ymir_detectors.json +++ b/tests/applications/ymir_detectors.json @@ -326,7 +326,8 @@ "config": { "source": "delay_source_chopper", "topic": "ymir_motion", - "dtype": "double" + "dtype": "double", + "value_units": "s" } } ] @@ -618,7 +619,8 @@ "config": { "source": "YMIR-SETS:SE-BADC-001:SLOWDATA", "topic": "ymir_motion", - "dtype": "double" + "dtype": "double", + "value_units": "mm" } } ] @@ -639,7 +641,8 @@ "config": { "source": "YMIR-SETS:SE-BPTP-001:PTP_STATUS", "topic": "ymir_motion", - "dtype": "double" + "dtype": "double", + "value_units": "mm" } } ] @@ -1086,7 +1089,8 @@ "config": { "source": "YMIR-SpScn:MC-Y-01:Mtr.VELO", "topic": "ymir_motion", - "dtype": "double" + "dtype": "double", + "value_units": "mm" } } ] @@ -1107,7 +1111,8 @@ "config": { "source": "YMIR-SpScn:MC-Z-01:Mtr.VELO", "topic": "ymir_motion", - "dtype": "double" + "dtype": "double", + "value_units": "mm" } } ] @@ -1128,7 +1133,8 @@ "config": { "source": "YMIR-SpScn:MC-X-01:Mtr.VELO", "topic": "ymir_motion", - "dtype": "double" + "dtype": "double", + "value_units": "mm" } } ] @@ -1184,7 +1190,8 @@ "config": { "source": "YMIR-SpScn:MC-Y-01:Mtr-RBV-TSE", "topic": "ymir_motion", - "dtype": "double" + "dtype": "double", + "value_units": "mm" } } ] @@ -1229,7 +1236,8 @@ "config": { "source": "YMIR-SpScn:MC-Z-01:Mtr-RBV-TSE", "topic": "ymir_motion", - "dtype": "double" + "dtype": "double", + "value_units": "mm" } } ] @@ -1274,7 +1282,8 @@ "config": { "source": "YMIR-SpScn:MC-X-01:Mtr-RBV-TSE", "topic": "ymir_motion", - "dtype": "double" + "dtype": "double", + "value_units": "mm" } } ] diff --git a/tests/helpers/tests/applications/data.py b/tests/helpers/tests/applications/data.py index 9701b7d4..63ac0cf9 100644 --- a/tests/helpers/tests/applications/data.py +++ b/tests/helpers/tests/applications/data.py @@ -19,7 +19,7 @@ def _make_pooch(): # A version of ymir.json where we added two fake detectors and # removed a templating string - to make it like something we might # read in a real run start message - 'ymir_detectors.json': 'md5:02bc6160081a96733c5056cfaa047fca', + 'ymir_detectors.json': 'md5:d9a25d4375ae3d414d91bfe504baa844', 'ymir.json': 'md5:5e913075094d97c5e9e9aca76fc32554', # readme of the dataset 'README.md': 'md5:778a0f290894182db5db0170b4f102fa', From 6d62c49c1abdce8524b26bb6754e0f94b0fde9a7 Mon Sep 17 00:00:00 2001 From: Sunyoung Yoo Date: Thu, 26 Sep 2024 17:07:16 +0200 Subject: [PATCH 2/3] Assume modules are validated when it's parsed. [skip ci] --- src/beamlime/applications/_nexus_helpers.py | 2 - tests/applications/nexus_helpers_test.py | 121 -------------------- 2 files changed, 123 deletions(-) diff --git a/src/beamlime/applications/_nexus_helpers.py b/src/beamlime/applications/_nexus_helpers.py index b04af080..52a42707 100644 --- a/src/beamlime/applications/_nexus_helpers.py +++ b/src/beamlime/applications/_nexus_helpers.py @@ -303,7 +303,6 @@ def _is_monitor(group: NexusGroup) -> bool: def _initialize_ev44(module_spec: StreamModuleValue) -> NexusGroup: parent = module_spec.parent - _validate_ev44_module_spec(module_spec) group: NexusGroup = cast(NexusGroup, parent.copy()) group['children'] = [ create_dataset( @@ -390,7 +389,6 @@ def _merge_ev44(group: NexusGroup, data: DeserializedMessage) -> None: def _initialize_f144(module_spec: StreamModuleValue) -> NexusGroup: parent = module_spec.parent - _validate_f144_module_spec(module_spec) group: NexusGroup = cast(NexusGroup, parent.copy()) group["children"] = [ create_dataset( diff --git a/tests/applications/nexus_helpers_test.py b/tests/applications/nexus_helpers_test.py index 5791265e..c5ea0e0c 100644 --- a/tests/applications/nexus_helpers_test.py +++ b/tests/applications/nexus_helpers_test.py @@ -8,7 +8,6 @@ import pytest from beamlime.applications._nexus_helpers import ( - InvalidNexusStructureError, StreamModuleKey, StreamModuleValue, collect_streaming_modules, @@ -293,46 +292,6 @@ def test_nxevent_data_ev44_generator_yields_frame_by_frame() -> None: next(ev44) -def test_ev44_merge_no_children_raises() -> None: - key = StreamModuleKey("ev44", "", "") - wrong_value = StreamModuleValue( - path=("",), - parent={"children": []}, - dtype="int32", - value_units="km", - ) - with pytest.raises( - InvalidNexusStructureError, - match="Group containing ev44 module should have exactly one child", - ): - merge_message_into_nexus_store( - module_key=key, - module_spec=wrong_value, - nexus_store={}, - data={}, - ) - - -def test_ev44_merge_too_many_children_raises() -> None: - key = StreamModuleKey("ev44", "", "") - wrong_value = StreamModuleValue( - path=("",), - parent={"children": []}, - dtype="int32", - value_units="km", - ) - with pytest.raises( - InvalidNexusStructureError, - match="Group containing ev44 module should have exactly one child", - ): - merge_message_into_nexus_store( - module_key=key, - module_spec=wrong_value, - nexus_store={}, - data={}, - ) - - @pytest.fixture() def nexus_template_with_streamed_log(dtype): return { @@ -395,86 +354,6 @@ def test_f144_merge(nexus_template_with_streamed_log, shape, dtype): assert values['attributes'][0]['values'] == 'km' -def test_f144_merge_no_children_raises(): - key = StreamModuleKey(module_type='f144', topic='', source='') - wrong_value = StreamModuleValue( - path=('',), - parent={'children': []}, - dtype='int32', - value_units='km', - ) - with pytest.raises( - InvalidNexusStructureError, - match="Group containing f144 module should have exactly one child", - ): - merge_message_into_nexus_store( - module_key=key, - module_spec=wrong_value, - nexus_store={}, - data={}, - ) - - -def test_f144_merge_too_many_children_raises(): - key = StreamModuleKey(module_type='f144', topic='', source='') - wrong_value = StreamModuleValue( - path=('',), - parent={'children': [{}, {}]}, - dtype='int32', - value_units='km', - ) - with pytest.raises( - InvalidNexusStructureError, - match="Group containing f144 module should have exactly one child", - ): - merge_message_into_nexus_store( - module_key=key, - module_spec=wrong_value, - nexus_store={}, - data={}, - ) - - -def test_f144_merge_missing_dtype_raises(): - key = StreamModuleKey(module_type='f144', topic='', source='') - wrong_value = StreamModuleValue( - path=('',), - parent={'children': [{}]}, - dtype=None, - value_units='km', - ) - with pytest.raises( - InvalidNexusStructureError, - match="f144 module spec should have dtype and value_units", - ): - merge_message_into_nexus_store( - module_key=key, - module_spec=wrong_value, - nexus_store={}, - data={}, - ) - - -def test_f144_merge_missing_value_units_raises(): - key = StreamModuleKey(module_type='f144', topic='', source='') - wrong_value = StreamModuleValue( - path=('',), - parent={'children': [{}]}, - dtype='int32', - value_units=None, - ) - with pytest.raises( - InvalidNexusStructureError, - match="f144 module spec should have dtype and value_units", - ): - merge_message_into_nexus_store( - module_key=key, - module_spec=wrong_value, - nexus_store={}, - data={}, - ) - - @pytest.fixture() def nexus_template_with_streamed_tdct(): return { From 2022a7024c1f4d9d10f6a5fbc23bc22dd346a1d9 Mon Sep 17 00:00:00 2001 From: Sunyoung Yoo Date: Thu, 26 Sep 2024 22:02:38 +0200 Subject: [PATCH 3/3] Remove unecessary helpers. --- src/beamlime/applications/_nexus_helpers.py | 28 ++++++--------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/src/beamlime/applications/_nexus_helpers.py b/src/beamlime/applications/_nexus_helpers.py index 52a42707..ed2c1423 100644 --- a/src/beamlime/applications/_nexus_helpers.py +++ b/src/beamlime/applications/_nexus_helpers.py @@ -196,15 +196,6 @@ def _validate_f144_module_spec( ) -def _validate_f144_module_values( - key_value_dict: dict[StreamModuleKey, StreamModuleValue], -) -> None: - """Validate the module values for the f144 module.""" - for key, value in key_value_dict.items(): - if key.module_type == "f144": - _validate_f144_module_spec(value) - - def _validate_ev44_module_spec( module_spec: StreamModuleValue, ) -> None: @@ -215,15 +206,6 @@ def _validate_ev44_module_spec( ) -def _validate_ev44_module_values( - key_value_dict: dict[StreamModuleKey, StreamModuleValue], -) -> None: - """Validate the module values for the ev44 module.""" - for key, value in key_value_dict.items(): - if key.module_type == "ev44": - _validate_ev44_module_spec(value) - - def collect_streaming_modules( structure: Mapping, ) -> dict[StreamModuleKey, StreamModuleValue]: @@ -264,9 +246,13 @@ def collect_streaming_modules( ) _validate_module_configs(key_value_pairs) _validate_module_keys(key_value_pairs) - key_value_dict = dict(key_value_pairs) - _validate_f144_module_values(key_value_dict) - _validate_ev44_module_values(key_value_dict) + # Validate each spec + for key, value in (key_value_dict := dict(key_value_pairs)).items(): + if key.module_type == 'ev44': + _validate_ev44_module_spec(value) + elif key.module_type == 'f144': + _validate_f144_module_spec(value) + return key_value_dict