From eeb48220a8b231ed474de1f874c4e37b01a5bef0 Mon Sep 17 00:00:00 2001 From: Tomasz Bartczak Date: Tue, 28 Mar 2023 13:28:35 +0200 Subject: [PATCH 01/11] will raise ValueError on duplicated key in json config Signed-off-by: Tomasz Bartczak --- monai/bundle/config_parser.py | 14 +++++++++++++- tests/test_config_parser.py | 15 +++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/monai/bundle/config_parser.py b/monai/bundle/config_parser.py index 613ad4e44a..1cd76d0e44 100644 --- a/monai/bundle/config_parser.py +++ b/monai/bundle/config_parser.py @@ -383,6 +383,16 @@ def _do_parse(self, config: Any, id: str = "") -> None: else: self.ref_resolver.add_item(ConfigItem(config=item_conf, id=id)) + @staticmethod + def raise_error_on_key_duplicates(ordered_pairs): + keys = set() + for k, _ in ordered_pairs: + if k in keys: + raise ValueError(f"Duplicate key: `{k}`") + else: + keys.add(k) + return dict(ordered_pairs) + @classmethod def load_config_file(cls, filepath: PathLike, **kwargs: Any) -> dict: """ @@ -400,7 +410,9 @@ def load_config_file(cls, filepath: PathLike, **kwargs: Any) -> dict: raise ValueError(f'unknown file input: "{filepath}"') with open(_filepath) as f: if _filepath.lower().endswith(cls.suffixes[0]): - return json.load(f, **kwargs) # type: ignore[no-any-return] + return json.load( + f, object_pairs_hook=cls.raise_error_on_key_duplicates, **kwargs + ) # type: ignore[no-any-return] if _filepath.lower().endswith(cls.suffixes[1:]): return yaml.safe_load(f, **kwargs) # type: ignore[no-any-return] raise ValueError(f"only support JSON or YAML config file so far, got name {_filepath}.") diff --git a/tests/test_config_parser.py b/tests/test_config_parser.py index c4b50daed1..8e96049300 100644 --- a/tests/test_config_parser.py +++ b/tests/test_config_parser.py @@ -14,6 +14,7 @@ import os import tempfile import unittest +from pathlib import Path from unittest import skipUnless import numpy as np @@ -109,6 +110,8 @@ def __call__(self, a, b): TEST_CASE_5 = [{"training": {"A": 1, "A_B": 2}, "total": "$@training#A + @training#A_B + 1"}, 4] +TEST_CASE_DUPLICATED_KEY = ["""{"key": {"unique": 1, "duplicate": 0, "duplicate": 4 } }"""] + class TestConfigParser(unittest.TestCase): def test_config_content(self): @@ -303,6 +306,18 @@ def test_substring_reference(self, config, expected): parser = ConfigParser(config=config) self.assertEqual(parser.get_parsed_content("total"), expected) + @parameterized.expand([TEST_CASE_DUPLICATED_KEY]) + def test_parse_json(self, config_string): + with tempfile.TemporaryDirectory() as tempdir: + config_path = Path(tempdir) / "config.json" + config_path.write_text(config_string) + parser = ConfigParser() + + with self.assertRaises(ValueError) as context: + parser.read_config(config_path) + + self.assertTrue("Duplicate key: `duplicate`" in str(context.exception)) + if __name__ == "__main__": unittest.main() From 4fd22b9ba19880b1fd6d18b46796eab6f871c181 Mon Sep 17 00:00:00 2001 From: Tomasz Bartczak Date: Tue, 28 Mar 2023 15:19:29 +0200 Subject: [PATCH 02/11] docstring for Signed-off-by: Tomasz Bartczak --- monai/bundle/config_parser.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/monai/bundle/config_parser.py b/monai/bundle/config_parser.py index 1cd76d0e44..9ef2e8519f 100644 --- a/monai/bundle/config_parser.py +++ b/monai/bundle/config_parser.py @@ -16,7 +16,7 @@ from collections.abc import Sequence from copy import deepcopy from pathlib import Path -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, Tuple from monai.bundle.config_item import ComponentLocator, ConfigComponent, ConfigExpression, ConfigItem from monai.bundle.reference_resolver import ReferenceResolver @@ -384,7 +384,14 @@ def _do_parse(self, config: Any, id: str = "") -> None: self.ref_resolver.add_item(ConfigItem(config=item_conf, id=id)) @staticmethod - def raise_error_on_key_duplicates(ordered_pairs): + def raise_error_on_key_duplicates(ordered_pairs: Sequence[Tuple[Any, Any]]): + """ + Raises ValueError if there is a duplicated key in the sequence of `ordered_pairs`. + Otherwise, it returns the dict made from this sequence. + + Args: + ordered_pairs: sequence of (key, value) + """ keys = set() for k, _ in ordered_pairs: if k in keys: From 4128028603a6b0f3b8846ba3c4b41ffecb79edfb Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 28 Mar 2023 13:23:44 +0000 Subject: [PATCH 03/11] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- monai/bundle/config_parser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/monai/bundle/config_parser.py b/monai/bundle/config_parser.py index 9ef2e8519f..6ed43fda46 100644 --- a/monai/bundle/config_parser.py +++ b/monai/bundle/config_parser.py @@ -16,7 +16,7 @@ from collections.abc import Sequence from copy import deepcopy from pathlib import Path -from typing import TYPE_CHECKING, Any, Tuple +from typing import TYPE_CHECKING, Any from monai.bundle.config_item import ComponentLocator, ConfigComponent, ConfigExpression, ConfigItem from monai.bundle.reference_resolver import ReferenceResolver @@ -384,7 +384,7 @@ def _do_parse(self, config: Any, id: str = "") -> None: self.ref_resolver.add_item(ConfigItem(config=item_conf, id=id)) @staticmethod - def raise_error_on_key_duplicates(ordered_pairs: Sequence[Tuple[Any, Any]]): + def raise_error_on_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]): """ Raises ValueError if there is a duplicated key in the sequence of `ordered_pairs`. Otherwise, it returns the dict made from this sequence. From 6baa9eaa2a0471e20c2ad12b03a8dda5516ff681 Mon Sep 17 00:00:00 2001 From: Tomasz Bartczak Date: Tue, 28 Mar 2023 18:21:47 +0200 Subject: [PATCH 04/11] fixed type annotations Signed-off-by: Tomasz Bartczak --- monai/bundle/config_parser.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/monai/bundle/config_parser.py b/monai/bundle/config_parser.py index 6ed43fda46..0c8e9e1551 100644 --- a/monai/bundle/config_parser.py +++ b/monai/bundle/config_parser.py @@ -384,7 +384,7 @@ def _do_parse(self, config: Any, id: str = "") -> None: self.ref_resolver.add_item(ConfigItem(config=item_conf, id=id)) @staticmethod - def raise_error_on_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]): + def raise_error_on_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]) -> dict[Any, Any]: """ Raises ValueError if there is a duplicated key in the sequence of `ordered_pairs`. Otherwise, it returns the dict made from this sequence. @@ -417,9 +417,9 @@ def load_config_file(cls, filepath: PathLike, **kwargs: Any) -> dict: raise ValueError(f'unknown file input: "{filepath}"') with open(_filepath) as f: if _filepath.lower().endswith(cls.suffixes[0]): - return json.load( + return json.load( # type: ignore[no-any-return] f, object_pairs_hook=cls.raise_error_on_key_duplicates, **kwargs - ) # type: ignore[no-any-return] + ) if _filepath.lower().endswith(cls.suffixes[1:]): return yaml.safe_load(f, **kwargs) # type: ignore[no-any-return] raise ValueError(f"only support JSON or YAML config file so far, got name {_filepath}.") From adb844e471150be9329d2f81e383343b374153d8 Mon Sep 17 00:00:00 2001 From: Tomasz Bartczak Date: Wed, 29 Mar 2023 10:44:54 +0200 Subject: [PATCH 05/11] warn by default, raise exception when conigured Signed-off-by: Tomasz Bartczak --- monai/bundle/config_parser.py | 20 ++++++++++++++------ tests/test_config_parser.py | 22 +++++++++++++++++++--- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/monai/bundle/config_parser.py b/monai/bundle/config_parser.py index 0c8e9e1551..df75f16cb1 100644 --- a/monai/bundle/config_parser.py +++ b/monai/bundle/config_parser.py @@ -12,6 +12,8 @@ from __future__ import annotations import json +import logging +import os import re from collections.abc import Sequence from copy import deepcopy @@ -33,6 +35,8 @@ _default_globals = {"monai": "monai", "torch": "torch", "np": "numpy", "numpy": "numpy"} +logger = logging.getLogger(__name__) + class ConfigParser: """ @@ -384,9 +388,12 @@ def _do_parse(self, config: Any, id: str = "") -> None: self.ref_resolver.add_item(ConfigItem(config=item_conf, id=id)) @staticmethod - def raise_error_on_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]) -> dict[Any, Any]: + def check_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]) -> dict[Any, Any]: """ - Raises ValueError if there is a duplicated key in the sequence of `ordered_pairs`. + Checks if there is a duplicated key in the sequence of `ordered_pairs`. + If there is - it will log a warning or raise ValueError + (if configured by environmental var `MONAI_FAIL_ON_DUPLICATE_CONFIG==1`) + Otherwise, it returns the dict made from this sequence. Args: @@ -395,7 +402,10 @@ def raise_error_on_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]) -> d keys = set() for k, _ in ordered_pairs: if k in keys: - raise ValueError(f"Duplicate key: `{k}`") + if os.environ.get("MONAI_FAIL_ON_DUPLICATE_CONFIG", "0") == "1": + raise ValueError(f"Duplicate key: `{k}`") + else: + logger.warning(f"Duplicate key: `{k}`") else: keys.add(k) return dict(ordered_pairs) @@ -417,9 +427,7 @@ def load_config_file(cls, filepath: PathLike, **kwargs: Any) -> dict: raise ValueError(f'unknown file input: "{filepath}"') with open(_filepath) as f: if _filepath.lower().endswith(cls.suffixes[0]): - return json.load( # type: ignore[no-any-return] - f, object_pairs_hook=cls.raise_error_on_key_duplicates, **kwargs - ) + return json.load(f, object_pairs_hook=cls.check_key_duplicates, **kwargs) # type: ignore[no-any-return] if _filepath.lower().endswith(cls.suffixes[1:]): return yaml.safe_load(f, **kwargs) # type: ignore[no-any-return] raise ValueError(f"only support JSON or YAML config file so far, got name {_filepath}.") diff --git a/tests/test_config_parser.py b/tests/test_config_parser.py index 8e96049300..e728d94915 100644 --- a/tests/test_config_parser.py +++ b/tests/test_config_parser.py @@ -11,11 +11,12 @@ from __future__ import annotations +import logging import os import tempfile import unittest from pathlib import Path -from unittest import skipUnless +from unittest import mock, skipUnless import numpy as np from parameterized import parameterized @@ -110,7 +111,7 @@ def __call__(self, a, b): TEST_CASE_5 = [{"training": {"A": 1, "A_B": 2}, "total": "$@training#A + @training#A_B + 1"}, 4] -TEST_CASE_DUPLICATED_KEY = ["""{"key": {"unique": 1, "duplicate": 0, "duplicate": 4 } }"""] +TEST_CASE_DUPLICATED_KEY = ["""{"key": {"unique": 1, "duplicate": 0, "duplicate": 4 } }""", 1, [0, 4]] class TestConfigParser(unittest.TestCase): @@ -307,7 +308,8 @@ def test_substring_reference(self, config, expected): self.assertEqual(parser.get_parsed_content("total"), expected) @parameterized.expand([TEST_CASE_DUPLICATED_KEY]) - def test_parse_json(self, config_string): + @mock.patch.dict(os.environ, {"MONAI_FAIL_ON_DUPLICATE_CONFIG": "1"}) + def test_parse_json_raise(self, config_string, _, __): with tempfile.TemporaryDirectory() as tempdir: config_path = Path(tempdir) / "config.json" config_path.write_text(config_string) @@ -318,6 +320,20 @@ def test_parse_json(self, config_string): self.assertTrue("Duplicate key: `duplicate`" in str(context.exception)) + @parameterized.expand([TEST_CASE_DUPLICATED_KEY]) + def test_parse_json_warn(self, config_string, expected_unique_val, expected_duplicate_vals): + with tempfile.TemporaryDirectory() as tempdir: + config_path = Path(tempdir) / "config.json" + config_path.write_text(config_string) + parser = ConfigParser() + + with self.assertLogs(level=logging.WARNING) as log: + parser.read_config(config_path) + self.assertTrue("Duplicate key: `duplicate`" in " ".join(log.output)) + + self.assertEqual(parser.get_parsed_content("key#unique"), expected_unique_val) + self.assertIn(parser.get_parsed_content("key#duplicate"), expected_duplicate_vals) + if __name__ == "__main__": unittest.main() From 578ebc42399970c28d8108c075b81668f7acfa9a Mon Sep 17 00:00:00 2001 From: Tomasz Bartczak Date: Wed, 29 Mar 2023 15:33:23 +0200 Subject: [PATCH 06/11] logger => warnings Signed-off-by: Tomasz Bartczak --- monai/bundle/config_parser.py | 6 ++---- tests/test_config_parser.py | 10 ++++++++-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/monai/bundle/config_parser.py b/monai/bundle/config_parser.py index df75f16cb1..8d3cacbeba 100644 --- a/monai/bundle/config_parser.py +++ b/monai/bundle/config_parser.py @@ -12,9 +12,9 @@ from __future__ import annotations import json -import logging import os import re +import warnings from collections.abc import Sequence from copy import deepcopy from pathlib import Path @@ -35,8 +35,6 @@ _default_globals = {"monai": "monai", "torch": "torch", "np": "numpy", "numpy": "numpy"} -logger = logging.getLogger(__name__) - class ConfigParser: """ @@ -405,7 +403,7 @@ def check_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]) -> dict[Any, if os.environ.get("MONAI_FAIL_ON_DUPLICATE_CONFIG", "0") == "1": raise ValueError(f"Duplicate key: `{k}`") else: - logger.warning(f"Duplicate key: `{k}`") + warnings.warn(f"Duplicate key: `{k}`") else: keys.add(k) return dict(ordered_pairs) diff --git a/tests/test_config_parser.py b/tests/test_config_parser.py index e728d94915..193cb7905e 100644 --- a/tests/test_config_parser.py +++ b/tests/test_config_parser.py @@ -15,6 +15,7 @@ import os import tempfile import unittest +import warnings from pathlib import Path from unittest import mock, skipUnless @@ -327,9 +328,14 @@ def test_parse_json_warn(self, config_string, expected_unique_val, expected_dupl config_path.write_text(config_string) parser = ConfigParser() - with self.assertLogs(level=logging.WARNING) as log: + # with self.assertLogs(level=logging.WARNING) as log: + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") parser.read_config(config_path) - self.assertTrue("Duplicate key: `duplicate`" in " ".join(log.output)) + print(w) + # Verify some things + self.assertEqual(len(w), 1) + self.assertTrue("Duplicate key: `duplicate`" in str(w[-1].message)) self.assertEqual(parser.get_parsed_content("key#unique"), expected_unique_val) self.assertIn(parser.get_parsed_content("key#duplicate"), expected_duplicate_vals) From 0330b90551ce49083e5b18a703bfcfd8bd7f64c9 Mon Sep 17 00:00:00 2001 From: Tomasz Bartczak Date: Wed, 29 Mar 2023 15:38:30 +0200 Subject: [PATCH 07/11] check_key_duplicates moved to utils/misc.py Signed-off-by: Tomasz Bartczak --- monai/bundle/config_parser.py | 26 ++------------------------ monai/utils/misc.py | 25 +++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/monai/bundle/config_parser.py b/monai/bundle/config_parser.py index 8d3cacbeba..e49b77b16a 100644 --- a/monai/bundle/config_parser.py +++ b/monai/bundle/config_parser.py @@ -25,6 +25,7 @@ from monai.bundle.utils import ID_REF_KEY, ID_SEP_KEY, MACRO_KEY from monai.config import PathLike from monai.utils import ensure_tuple, look_up_option, optional_import +from monai.utils.misc import check_key_duplicates if TYPE_CHECKING: import yaml @@ -385,29 +386,6 @@ def _do_parse(self, config: Any, id: str = "") -> None: else: self.ref_resolver.add_item(ConfigItem(config=item_conf, id=id)) - @staticmethod - def check_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]) -> dict[Any, Any]: - """ - Checks if there is a duplicated key in the sequence of `ordered_pairs`. - If there is - it will log a warning or raise ValueError - (if configured by environmental var `MONAI_FAIL_ON_DUPLICATE_CONFIG==1`) - - Otherwise, it returns the dict made from this sequence. - - Args: - ordered_pairs: sequence of (key, value) - """ - keys = set() - for k, _ in ordered_pairs: - if k in keys: - if os.environ.get("MONAI_FAIL_ON_DUPLICATE_CONFIG", "0") == "1": - raise ValueError(f"Duplicate key: `{k}`") - else: - warnings.warn(f"Duplicate key: `{k}`") - else: - keys.add(k) - return dict(ordered_pairs) - @classmethod def load_config_file(cls, filepath: PathLike, **kwargs: Any) -> dict: """ @@ -425,7 +403,7 @@ def load_config_file(cls, filepath: PathLike, **kwargs: Any) -> dict: raise ValueError(f'unknown file input: "{filepath}"') with open(_filepath) as f: if _filepath.lower().endswith(cls.suffixes[0]): - return json.load(f, object_pairs_hook=cls.check_key_duplicates, **kwargs) # type: ignore[no-any-return] + return json.load(f, object_pairs_hook=check_key_duplicates, **kwargs) # type: ignore[no-any-return] if _filepath.lower().endswith(cls.suffixes[1:]): return yaml.safe_load(f, **kwargs) # type: ignore[no-any-return] raise ValueError(f"only support JSON or YAML config file so far, got name {_filepath}.") diff --git a/monai/utils/misc.py b/monai/utils/misc.py index 0b2df36a5b..7058d66aad 100644 --- a/monai/utils/misc.py +++ b/monai/utils/misc.py @@ -679,3 +679,28 @@ def pprint_edges(val: Any, n_lines: int = 20) -> str: hidden_n = len(val_str) - n_lines * 2 val_str = val_str[:n_lines] + [f"\n ... omitted {hidden_n} line(s)\n\n"] + val_str[-n_lines:] return "".join(val_str) + + +def check_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]) -> dict[Any, Any]: + """ + Checks if there is a duplicated key in the sequence of `ordered_pairs`. + If there is - it will log a warning or raise ValueError + (if configured by environmental var `MONAI_FAIL_ON_DUPLICATE_CONFIG==1`) + + Otherwise, it returns the dict made from this sequence. + + Satisfies a format for an `object_pairs_hook` in `json.load` + + Args: + ordered_pairs: sequence of (key, value) + """ + keys = set() + for k, _ in ordered_pairs: + if k in keys: + if os.environ.get("MONAI_FAIL_ON_DUPLICATE_CONFIG", "0") == "1": + raise ValueError(f"Duplicate key: `{k}`") + else: + warnings.warn(f"Duplicate key: `{k}`") + else: + keys.add(k) + return dict(ordered_pairs) From 3ed9843bb73eb39a47439d0258d158269641f026 Mon Sep 17 00:00:00 2001 From: Tomasz Bartczak Date: Wed, 29 Mar 2023 16:06:35 +0200 Subject: [PATCH 08/11] checking key duplicates in yaml as well Signed-off-by: Tomasz Bartczak --- monai/bundle/config_parser.py | 6 ++---- monai/utils/misc.py | 23 +++++++++++++++++++++-- tests/test_config_parser.py | 27 ++++++++++++++------------- 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/monai/bundle/config_parser.py b/monai/bundle/config_parser.py index e49b77b16a..bef767f005 100644 --- a/monai/bundle/config_parser.py +++ b/monai/bundle/config_parser.py @@ -12,9 +12,7 @@ from __future__ import annotations import json -import os import re -import warnings from collections.abc import Sequence from copy import deepcopy from pathlib import Path @@ -25,7 +23,7 @@ from monai.bundle.utils import ID_REF_KEY, ID_SEP_KEY, MACRO_KEY from monai.config import PathLike from monai.utils import ensure_tuple, look_up_option, optional_import -from monai.utils.misc import check_key_duplicates +from monai.utils.misc import check_key_duplicates, CheckKeyDuplicatesYamlLoader if TYPE_CHECKING: import yaml @@ -405,7 +403,7 @@ def load_config_file(cls, filepath: PathLike, **kwargs: Any) -> dict: if _filepath.lower().endswith(cls.suffixes[0]): return json.load(f, object_pairs_hook=check_key_duplicates, **kwargs) # type: ignore[no-any-return] if _filepath.lower().endswith(cls.suffixes[1:]): - return yaml.safe_load(f, **kwargs) # type: ignore[no-any-return] + return yaml.load(f, CheckKeyDuplicatesYamlLoader, **kwargs) # type: ignore[no-any-return] raise ValueError(f"only support JSON or YAML config file so far, got name {_filepath}.") @classmethod diff --git a/monai/utils/misc.py b/monai/utils/misc.py index 7058d66aad..1e8bddaf13 100644 --- a/monai/utils/misc.py +++ b/monai/utils/misc.py @@ -24,13 +24,18 @@ from collections.abc import Callable, Iterable, Sequence from distutils.util import strtobool from pathlib import Path -from typing import Any, TypeVar, cast, overload +from typing import Any, TypeVar, cast, overload, TYPE_CHECKING import numpy as np import torch from monai.config.type_definitions import NdarrayOrTensor, NdarrayTensor, PathLike -from monai.utils.module import version_leq +from monai.utils.module import version_leq, optional_import + +if TYPE_CHECKING: + import yaml +else: + yaml, _ = optional_import("yaml") __all__ = [ "zip_with", @@ -704,3 +709,17 @@ def check_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]) -> dict[Any, else: keys.add(k) return dict(ordered_pairs) + + +class CheckKeyDuplicatesYamlLoader(yaml.SafeLoader): + def construct_mapping(self, node, deep=False): + mapping = set() + for key_node, value_node in node.value: + key = self.construct_object(key_node, deep=deep) + if key in mapping: + if os.environ.get("MONAI_FAIL_ON_DUPLICATE_CONFIG", "0") == "1": + raise ValueError(f"Duplicate key: `{key}`") + else: + warnings.warn(f"Duplicate key: `{key}`") + mapping.add(key) + return super().construct_mapping(node, deep) diff --git a/tests/test_config_parser.py b/tests/test_config_parser.py index 193cb7905e..2ae6ed3f21 100644 --- a/tests/test_config_parser.py +++ b/tests/test_config_parser.py @@ -112,7 +112,12 @@ def __call__(self, a, b): TEST_CASE_5 = [{"training": {"A": 1, "A_B": 2}, "total": "$@training#A + @training#A_B + 1"}, 4] -TEST_CASE_DUPLICATED_KEY = ["""{"key": {"unique": 1, "duplicate": 0, "duplicate": 4 } }""", 1, [0, 4]] +TEST_CASE_DUPLICATED_KEY_JSON = ["""{"key": {"unique": 1, "duplicate": 0, "duplicate": 4 } }""", "json", 1, [0, 4]] + +TEST_CASE_DUPLICATED_KEY_YAML = ["""key: + unique: 1 + duplicate: 0 + duplicate: 4""", "yaml", 1, [0, 4]] class TestConfigParser(unittest.TestCase): @@ -308,11 +313,11 @@ def test_substring_reference(self, config, expected): parser = ConfigParser(config=config) self.assertEqual(parser.get_parsed_content("total"), expected) - @parameterized.expand([TEST_CASE_DUPLICATED_KEY]) + @parameterized.expand([TEST_CASE_DUPLICATED_KEY_JSON, TEST_CASE_DUPLICATED_KEY_YAML]) @mock.patch.dict(os.environ, {"MONAI_FAIL_ON_DUPLICATE_CONFIG": "1"}) - def test_parse_json_raise(self, config_string, _, __): + def test_parse_json_raise(self, config_string, extension, _, __): with tempfile.TemporaryDirectory() as tempdir: - config_path = Path(tempdir) / "config.json" + config_path = Path(tempdir) / f"config.{extension}" config_path.write_text(config_string) parser = ConfigParser() @@ -321,21 +326,17 @@ def test_parse_json_raise(self, config_string, _, __): self.assertTrue("Duplicate key: `duplicate`" in str(context.exception)) - @parameterized.expand([TEST_CASE_DUPLICATED_KEY]) - def test_parse_json_warn(self, config_string, expected_unique_val, expected_duplicate_vals): + @parameterized.expand([TEST_CASE_DUPLICATED_KEY_JSON, TEST_CASE_DUPLICATED_KEY_YAML]) + def test_parse_json_warn(self, config_string, extension, expected_unique_val, expected_duplicate_vals): with tempfile.TemporaryDirectory() as tempdir: - config_path = Path(tempdir) / "config.json" + config_path = Path(tempdir) / f"config.{extension}" config_path.write_text(config_string) parser = ConfigParser() - # with self.assertLogs(level=logging.WARNING) as log: with warnings.catch_warnings(record=True) as w: - warnings.simplefilter("always") parser.read_config(config_path) - print(w) - # Verify some things - self.assertEqual(len(w), 1) - self.assertTrue("Duplicate key: `duplicate`" in str(w[-1].message)) + self.assertEqual(len(w), 1) + self.assertTrue("Duplicate key: `duplicate`" in str(w[-1].message)) self.assertEqual(parser.get_parsed_content("key#unique"), expected_unique_val) self.assertIn(parser.get_parsed_content("key#duplicate"), expected_duplicate_vals) From 2825eefb15200be8c5bbb4d355edc87de4e9600d Mon Sep 17 00:00:00 2001 From: Tomasz Bartczak Date: Wed, 29 Mar 2023 17:57:41 +0200 Subject: [PATCH 09/11] code quality fixes Signed-off-by: Tomasz Bartczak --- monai/bundle/config_parser.py | 2 +- monai/utils/misc.py | 4 ++-- tests/test_config_parser.py | 10 +++++++--- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/monai/bundle/config_parser.py b/monai/bundle/config_parser.py index bef767f005..ad20534a1f 100644 --- a/monai/bundle/config_parser.py +++ b/monai/bundle/config_parser.py @@ -23,7 +23,7 @@ from monai.bundle.utils import ID_REF_KEY, ID_SEP_KEY, MACRO_KEY from monai.config import PathLike from monai.utils import ensure_tuple, look_up_option, optional_import -from monai.utils.misc import check_key_duplicates, CheckKeyDuplicatesYamlLoader +from monai.utils.misc import CheckKeyDuplicatesYamlLoader, check_key_duplicates if TYPE_CHECKING: import yaml diff --git a/monai/utils/misc.py b/monai/utils/misc.py index 1e8bddaf13..b04e1dd180 100644 --- a/monai/utils/misc.py +++ b/monai/utils/misc.py @@ -24,13 +24,13 @@ from collections.abc import Callable, Iterable, Sequence from distutils.util import strtobool from pathlib import Path -from typing import Any, TypeVar, cast, overload, TYPE_CHECKING +from typing import TYPE_CHECKING, Any, TypeVar, cast, overload import numpy as np import torch from monai.config.type_definitions import NdarrayOrTensor, NdarrayTensor, PathLike -from monai.utils.module import version_leq, optional_import +from monai.utils.module import optional_import, version_leq if TYPE_CHECKING: import yaml diff --git a/tests/test_config_parser.py b/tests/test_config_parser.py index 2ae6ed3f21..a716eeb647 100644 --- a/tests/test_config_parser.py +++ b/tests/test_config_parser.py @@ -11,7 +11,6 @@ from __future__ import annotations -import logging import os import tempfile import unittest @@ -114,10 +113,15 @@ def __call__(self, a, b): TEST_CASE_DUPLICATED_KEY_JSON = ["""{"key": {"unique": 1, "duplicate": 0, "duplicate": 4 } }""", "json", 1, [0, 4]] -TEST_CASE_DUPLICATED_KEY_YAML = ["""key: +TEST_CASE_DUPLICATED_KEY_YAML = [ + """key: unique: 1 duplicate: 0 - duplicate: 4""", "yaml", 1, [0, 4]] + duplicate: 4""", + "yaml", + 1, + [0, 4], +] class TestConfigParser(unittest.TestCase): From 1d1aafa79c7a406222165dc14a46446ffeaa98dd Mon Sep 17 00:00:00 2001 From: Wenqi Li Date: Wed, 29 Mar 2023 22:08:03 +0100 Subject: [PATCH 10/11] fixes optional import Signed-off-by: Wenqi Li --- monai/utils/misc.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/monai/utils/misc.py b/monai/utils/misc.py index b04e1dd180..b006f134f5 100644 --- a/monai/utils/misc.py +++ b/monai/utils/misc.py @@ -33,9 +33,9 @@ from monai.utils.module import optional_import, version_leq if TYPE_CHECKING: - import yaml + from yaml import SafeLoader else: - yaml, _ = optional_import("yaml") + SafeLoader, _ = optional_import("yaml", name="SafeLoader", as_type="base") __all__ = [ "zip_with", @@ -711,7 +711,7 @@ def check_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]) -> dict[Any, return dict(ordered_pairs) -class CheckKeyDuplicatesYamlLoader(yaml.SafeLoader): +class CheckKeyDuplicatesYamlLoader(SafeLoader): def construct_mapping(self, node, deep=False): mapping = set() for key_node, value_node in node.value: From 9662cab3541b63a334f350b4d059fb7ac1a1f8f4 Mon Sep 17 00:00:00 2001 From: Wenqi Li Date: Wed, 29 Mar 2023 22:25:43 +0100 Subject: [PATCH 11/11] skipunless has yaml Signed-off-by: Wenqi Li --- monai/utils/misc.py | 2 +- tests/test_config_parser.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/monai/utils/misc.py b/monai/utils/misc.py index b006f134f5..924c00c3ec 100644 --- a/monai/utils/misc.py +++ b/monai/utils/misc.py @@ -714,7 +714,7 @@ def check_key_duplicates(ordered_pairs: Sequence[tuple[Any, Any]]) -> dict[Any, class CheckKeyDuplicatesYamlLoader(SafeLoader): def construct_mapping(self, node, deep=False): mapping = set() - for key_node, value_node in node.value: + for key_node, _ in node.value: key = self.construct_object(key_node, deep=deep) if key in mapping: if os.environ.get("MONAI_FAIL_ON_DUPLICATE_CONFIG", "0") == "1": diff --git a/tests/test_config_parser.py b/tests/test_config_parser.py index a716eeb647..d45a251f9b 100644 --- a/tests/test_config_parser.py +++ b/tests/test_config_parser.py @@ -29,6 +29,7 @@ from tests.utils import TimedCall _, has_tv = optional_import("torchvision", "0.8.0", min_version) +_, has_yaml = optional_import("yaml") @TimedCall(seconds=100, force_quit=True) @@ -319,6 +320,7 @@ def test_substring_reference(self, config, expected): @parameterized.expand([TEST_CASE_DUPLICATED_KEY_JSON, TEST_CASE_DUPLICATED_KEY_YAML]) @mock.patch.dict(os.environ, {"MONAI_FAIL_ON_DUPLICATE_CONFIG": "1"}) + @skipUnless(has_yaml, "Requires pyyaml") def test_parse_json_raise(self, config_string, extension, _, __): with tempfile.TemporaryDirectory() as tempdir: config_path = Path(tempdir) / f"config.{extension}" @@ -331,6 +333,7 @@ def test_parse_json_raise(self, config_string, extension, _, __): self.assertTrue("Duplicate key: `duplicate`" in str(context.exception)) @parameterized.expand([TEST_CASE_DUPLICATED_KEY_JSON, TEST_CASE_DUPLICATED_KEY_YAML]) + @skipUnless(has_yaml, "Requires pyyaml") def test_parse_json_warn(self, config_string, extension, expected_unique_val, expected_duplicate_vals): with tempfile.TemporaryDirectory() as tempdir: config_path = Path(tempdir) / f"config.{extension}"