From a5d5ed33df8fe3c36caef19d08c8295dd8046c93 Mon Sep 17 00:00:00 2001 From: Yiheng Wang Date: Fri, 15 Mar 2024 14:25:56 +0800 Subject: [PATCH 1/9] change bundle workflow args Signed-off-by: Yiheng Wang --- monai/bundle/workflows.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/monai/bundle/workflows.py b/monai/bundle/workflows.py index da3aa30141..80ce7e8edd 100644 --- a/monai/bundle/workflows.py +++ b/monai/bundle/workflows.py @@ -46,6 +46,9 @@ class BundleWorkflow(ABC): or "infer", "inference", "eval", "evaluation" for a inference workflow, other unsupported string will raise a ValueError. default to `None` for common workflow. + meta_file: filepath of the metadata file, if this is a list of file paths, their contents will be merged in order. + logging_file: config file for `logging` module in the program. for more details: + https://docs.python.org/3/library/logging.config.html#logging.config.fileConfig. """ @@ -59,7 +62,13 @@ class BundleWorkflow(ABC): new_name="workflow_type", msg_suffix="please use `workflow_type` instead.", ) - def __init__(self, workflow_type: str | None = None, workflow: str | None = None): + def __init__( + self, + workflow_type: str | None = None, + workflow: str | None = None, + meta_file: str | Sequence[str] | None = None, + logging_file: str | None = None, + ): workflow_type = workflow if workflow is not None else workflow_type if workflow_type is None: self.properties = copy(MetaProperties) @@ -233,7 +242,7 @@ def __init__( **override: Any, ) -> None: workflow_type = workflow if workflow is not None else workflow_type - super().__init__(workflow_type=workflow_type) + super().__init__(workflow_type=workflow_type, meta_file=meta_file, logging_file=logging_file) if config_file is not None: _config_files = ensure_tuple(config_file) self.config_root_path = Path(_config_files[0]).parent From cf082dafa4e4fa19771d68931233f7cfc82cb3ff Mon Sep 17 00:00:00 2001 From: Yiheng Wang Date: Wed, 20 Mar 2024 13:32:45 +0800 Subject: [PATCH 2/9] finish implement and add test cases Signed-off-by: Yiheng Wang --- monai/bundle/workflows.py | 11 +++++++++++ tests/nonconfig_workflow.py | 4 ++-- tests/test_bundle_workflow.py | 10 ++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/monai/bundle/workflows.py b/monai/bundle/workflows.py index 80ce7e8edd..85b5311b48 100644 --- a/monai/bundle/workflows.py +++ b/monai/bundle/workflows.py @@ -83,6 +83,17 @@ def __init__( else: raise ValueError(f"Unsupported workflow type: '{workflow_type}'.") + if logging_file is not None: + if not os.path.exists(logging_file): + raise FileNotFoundError(f"Cannot find the logging config file: {logging_file}.") + logger.info(f"Setting logging properties based on config: {logging_file}.") + fileConfig(logging_file, disable_existing_loggers=False) + + if meta_file is not None: + if not os.path.exists(meta_file): + raise FileNotFoundError(f"Cannot find the metadata config file: {meta_file}.") + self.meta_file = meta_file + @abstractmethod def initialize(self, *args: Any, **kwargs: Any) -> Any: """ diff --git a/tests/nonconfig_workflow.py b/tests/nonconfig_workflow.py index 7b5328bf72..b2c44c12c6 100644 --- a/tests/nonconfig_workflow.py +++ b/tests/nonconfig_workflow.py @@ -36,8 +36,8 @@ class NonConfigWorkflow(BundleWorkflow): """ - def __init__(self, filename, output_dir): - super().__init__(workflow_type="inference") + def __init__(self, filename, output_dir, meta_file=None, logging_file=None): + super().__init__(workflow_type="inference", meta_file=meta_file, logging_file=logging_file) self.filename = filename self.output_dir = output_dir self._bundle_root = "will override" diff --git a/tests/test_bundle_workflow.py b/tests/test_bundle_workflow.py index f7da37acef..9182c18054 100644 --- a/tests/test_bundle_workflow.py +++ b/tests/test_bundle_workflow.py @@ -35,6 +35,10 @@ TEST_CASE_3 = [os.path.join(os.path.dirname(__file__), "testing_data", "config_fl_train.json")] +TEST_CASE_NON_CONFIG_WRONG_LOG = [None, "logging.conf", "Cannot find the logging config file: logging.conf."] + +TEST_CASE_NON_CONFIG_WRONG_META = ["meta.json", None, "Cannot find the metadata config file: meta.json."] + class TestBundleWorkflow(unittest.TestCase): @@ -144,8 +148,14 @@ def test_train_config(self, config_file): def test_non_config(self): # test user defined python style workflow inferer = NonConfigWorkflow(self.filename, self.data_dir) + self.assertEqual(inferer.meta_file, None) self._test_inferer(inferer) + @parameterized.expand([TEST_CASE_NON_CONFIG_WRONG_META, TEST_CASE_NON_CONFIG_WRONG_LOG]) + def test_non_config_wrong_cases(self, meta_file, logging_file, expected_error): + with self.assertRaisesRegex(FileNotFoundError, expected_error): + NonConfigWorkflow(self.filename, self.data_dir, meta_file, logging_file) + if __name__ == "__main__": unittest.main() From 46a8c67958808077578f176e50c74604e8dc634e Mon Sep 17 00:00:00 2001 From: Yiheng Wang Date: Wed, 20 Mar 2024 14:34:11 +0800 Subject: [PATCH 3/9] fix mypy error Signed-off-by: Yiheng Wang --- monai/bundle/workflows.py | 16 ++++++++++++++-- tests/test_bundle_workflow.py | 4 +++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/monai/bundle/workflows.py b/monai/bundle/workflows.py index 85b5311b48..653c25cdd3 100644 --- a/monai/bundle/workflows.py +++ b/monai/bundle/workflows.py @@ -90,8 +90,13 @@ def __init__( fileConfig(logging_file, disable_existing_loggers=False) if meta_file is not None: - if not os.path.exists(meta_file): + if isinstance(meta_file, str) and not os.path.exists(meta_file): raise FileNotFoundError(f"Cannot find the metadata config file: {meta_file}.") + if isinstance(meta_file, list): + for f in meta_file: + if not os.path.exists(f): + raise FileNotFoundError(f"Cannot find the metadata config file: {f}.") + self.meta_file = meta_file @abstractmethod @@ -253,7 +258,7 @@ def __init__( **override: Any, ) -> None: workflow_type = workflow if workflow is not None else workflow_type - super().__init__(workflow_type=workflow_type, meta_file=meta_file, logging_file=logging_file) + super().__init__(workflow_type=workflow_type) if config_file is not None: _config_files = ensure_tuple(config_file) self.config_root_path = Path(_config_files[0]).parent @@ -288,6 +293,13 @@ def __init__( f"Cannot find the metadata config file: {meta_file}. " "Please see: https://docs.monai.io/en/stable/mb_specification.html" ) + elif isinstance(meta_file, list): + for f in meta_file: + if not os.path.exists(f): + logger.error( + f"Cannot find the metadata config file: {f}. " + "Please see: https://docs.monai.io/en/stable/mb_specification.html" + ) else: self.parser.read_meta(f=meta_file) diff --git a/tests/test_bundle_workflow.py b/tests/test_bundle_workflow.py index 9182c18054..c3fae2651d 100644 --- a/tests/test_bundle_workflow.py +++ b/tests/test_bundle_workflow.py @@ -39,6 +39,8 @@ TEST_CASE_NON_CONFIG_WRONG_META = ["meta.json", None, "Cannot find the metadata config file: meta.json."] +TEST_CASE_NON_CONFIG_WRONG_META_LIST = [["meta.json"], None, "Cannot find the metadata config file: meta.json."] + class TestBundleWorkflow(unittest.TestCase): @@ -151,7 +153,7 @@ def test_non_config(self): self.assertEqual(inferer.meta_file, None) self._test_inferer(inferer) - @parameterized.expand([TEST_CASE_NON_CONFIG_WRONG_META, TEST_CASE_NON_CONFIG_WRONG_LOG]) + @parameterized.expand([TEST_CASE_NON_CONFIG_WRONG_META, TEST_CASE_NON_CONFIG_WRONG_META_LIST, TEST_CASE_NON_CONFIG_WRONG_LOG]) def test_non_config_wrong_cases(self, meta_file, logging_file, expected_error): with self.assertRaisesRegex(FileNotFoundError, expected_error): NonConfigWorkflow(self.filename, self.data_dir, meta_file, logging_file) From 3fbe59841f31694f1a53795d31a0d6358531d06f Mon Sep 17 00:00:00 2001 From: Yiheng Wang Date: Wed, 20 Mar 2024 14:47:24 +0800 Subject: [PATCH 4/9] autofix Signed-off-by: Yiheng Wang --- tests/test_bundle_workflow.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_bundle_workflow.py b/tests/test_bundle_workflow.py index c3fae2651d..b333ef03dd 100644 --- a/tests/test_bundle_workflow.py +++ b/tests/test_bundle_workflow.py @@ -153,7 +153,9 @@ def test_non_config(self): self.assertEqual(inferer.meta_file, None) self._test_inferer(inferer) - @parameterized.expand([TEST_CASE_NON_CONFIG_WRONG_META, TEST_CASE_NON_CONFIG_WRONG_META_LIST, TEST_CASE_NON_CONFIG_WRONG_LOG]) + @parameterized.expand( + [TEST_CASE_NON_CONFIG_WRONG_META, TEST_CASE_NON_CONFIG_WRONG_META_LIST, TEST_CASE_NON_CONFIG_WRONG_LOG] + ) def test_non_config_wrong_cases(self, meta_file, logging_file, expected_error): with self.assertRaisesRegex(FileNotFoundError, expected_error): NonConfigWorkflow(self.filename, self.data_dir, meta_file, logging_file) From ccc2364d92e143cd3f5dd6bbf23c72ebbada7bf3 Mon Sep 17 00:00:00 2001 From: Yiheng Wang Date: Mon, 25 Mar 2024 18:08:59 +0800 Subject: [PATCH 5/9] use isfile Signed-off-by: Yiheng Wang --- monai/bundle/workflows.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/monai/bundle/workflows.py b/monai/bundle/workflows.py index 653c25cdd3..e07e6145c2 100644 --- a/monai/bundle/workflows.py +++ b/monai/bundle/workflows.py @@ -84,17 +84,17 @@ def __init__( raise ValueError(f"Unsupported workflow type: '{workflow_type}'.") if logging_file is not None: - if not os.path.exists(logging_file): + if not os.path.isfile(logging_file): raise FileNotFoundError(f"Cannot find the logging config file: {logging_file}.") logger.info(f"Setting logging properties based on config: {logging_file}.") fileConfig(logging_file, disable_existing_loggers=False) if meta_file is not None: - if isinstance(meta_file, str) and not os.path.exists(meta_file): + if isinstance(meta_file, str) and not os.path.isfile(meta_file): raise FileNotFoundError(f"Cannot find the metadata config file: {meta_file}.") if isinstance(meta_file, list): for f in meta_file: - if not os.path.exists(f): + if not os.path.isfile(f): raise FileNotFoundError(f"Cannot find the metadata config file: {f}.") self.meta_file = meta_file @@ -276,7 +276,7 @@ def __init__( logging_file = str(self.config_root_path / "logging.conf") if logging_file is None else logging_file if logging_file is not None: - if not os.path.exists(logging_file): + if not os.path.isfile(logging_file): if logging_file == str(self.config_root_path / "logging.conf"): logger.warn(f"Default logging file in {logging_file} does not exist, skipping logging.") else: @@ -288,14 +288,14 @@ def __init__( self.parser = ConfigParser() self.parser.read_config(f=config_file) meta_file = str(self.config_root_path / "metadata.json") if meta_file is None else meta_file - if isinstance(meta_file, str) and not os.path.exists(meta_file): + if isinstance(meta_file, str) and not os.path.isfile(meta_file): logger.error( f"Cannot find the metadata config file: {meta_file}. " "Please see: https://docs.monai.io/en/stable/mb_specification.html" ) elif isinstance(meta_file, list): for f in meta_file: - if not os.path.exists(f): + if not os.path.isfile(f): logger.error( f"Cannot find the metadata config file: {f}. " "Please see: https://docs.monai.io/en/stable/mb_specification.html" From f793e80de6bc395f4f0e2f581c0c6200c08cf402 Mon Sep 17 00:00:00 2001 From: Yiheng Wang Date: Tue, 26 Mar 2024 14:20:20 +0800 Subject: [PATCH 6/9] resolve comment of meta_file Signed-off-by: Yiheng Wang --- monai/bundle/workflows.py | 44 ++++++++++++++++------------------- tests/test_bundle_workflow.py | 11 +++------ 2 files changed, 23 insertions(+), 32 deletions(-) diff --git a/monai/bundle/workflows.py b/monai/bundle/workflows.py index e07e6145c2..0701aa2203 100644 --- a/monai/bundle/workflows.py +++ b/monai/bundle/workflows.py @@ -91,11 +91,19 @@ def __init__( if meta_file is not None: if isinstance(meta_file, str) and not os.path.isfile(meta_file): - raise FileNotFoundError(f"Cannot find the metadata config file: {meta_file}.") + logger.error( + f"Cannot find the metadata config file: {meta_file}. " + "Please see: https://docs.monai.io/en/stable/mb_specification.html" + ) + meta_file = None if isinstance(meta_file, list): for f in meta_file: if not os.path.isfile(f): - raise FileNotFoundError(f"Cannot find the metadata config file: {f}.") + logger.error( + f"Cannot find the metadata config file: {f}. " + "Please see: https://docs.monai.io/en/stable/mb_specification.html" + ) + meta_file = None self.meta_file = meta_file @@ -258,22 +266,23 @@ def __init__( **override: Any, ) -> None: workflow_type = workflow if workflow is not None else workflow_type - super().__init__(workflow_type=workflow_type) if config_file is not None: _config_files = ensure_tuple(config_file) - self.config_root_path = Path(_config_files[0]).parent + config_root_path = Path(_config_files[0]).parent for _config_file in _config_files: _config_file = Path(_config_file) - if _config_file.parent != self.config_root_path: + if _config_file.parent != config_root_path: logger.warn( - f"Not all config files are in {self.config_root_path}. If logging_file and meta_file are" - f"not specified, {self.config_root_path} will be used as the default config root directory." + f"Not all config files are in {config_root_path}. If logging_file and meta_file are" + f"not specified, {config_root_path} will be used as the default config root directory." ) if not _config_file.is_file(): raise FileNotFoundError(f"Cannot find the config file: {_config_file}.") else: - self.config_root_path = Path("configs") - + config_root_path = Path("configs") + meta_file = str(config_root_path / "metadata.json") if meta_file is None else meta_file + super().__init__(workflow_type=workflow_type, meta_file=meta_file) + self.config_root_path = config_root_path logging_file = str(self.config_root_path / "logging.conf") if logging_file is None else logging_file if logging_file is not None: if not os.path.isfile(logging_file): @@ -287,21 +296,8 @@ def __init__( self.parser = ConfigParser() self.parser.read_config(f=config_file) - meta_file = str(self.config_root_path / "metadata.json") if meta_file is None else meta_file - if isinstance(meta_file, str) and not os.path.isfile(meta_file): - logger.error( - f"Cannot find the metadata config file: {meta_file}. " - "Please see: https://docs.monai.io/en/stable/mb_specification.html" - ) - elif isinstance(meta_file, list): - for f in meta_file: - if not os.path.isfile(f): - logger.error( - f"Cannot find the metadata config file: {f}. " - "Please see: https://docs.monai.io/en/stable/mb_specification.html" - ) - else: - self.parser.read_meta(f=meta_file) + if self.meta_file is not None: + self.parser.read_meta(f=self.meta_file) # the rest key-values in the _args are to override config content self.parser.update(pairs=override) diff --git a/tests/test_bundle_workflow.py b/tests/test_bundle_workflow.py index b333ef03dd..9f5da2edc6 100644 --- a/tests/test_bundle_workflow.py +++ b/tests/test_bundle_workflow.py @@ -11,6 +11,7 @@ from __future__ import annotations +import logging import os import shutil import tempfile @@ -37,10 +38,6 @@ TEST_CASE_NON_CONFIG_WRONG_LOG = [None, "logging.conf", "Cannot find the logging config file: logging.conf."] -TEST_CASE_NON_CONFIG_WRONG_META = ["meta.json", None, "Cannot find the metadata config file: meta.json."] - -TEST_CASE_NON_CONFIG_WRONG_META_LIST = [["meta.json"], None, "Cannot find the metadata config file: meta.json."] - class TestBundleWorkflow(unittest.TestCase): @@ -153,10 +150,8 @@ def test_non_config(self): self.assertEqual(inferer.meta_file, None) self._test_inferer(inferer) - @parameterized.expand( - [TEST_CASE_NON_CONFIG_WRONG_META, TEST_CASE_NON_CONFIG_WRONG_META_LIST, TEST_CASE_NON_CONFIG_WRONG_LOG] - ) - def test_non_config_wrong_cases(self, meta_file, logging_file, expected_error): + @parameterized.expand([TEST_CASE_NON_CONFIG_WRONG_LOG]) + def test_non_config_wrong_log_cases(self, meta_file, logging_file, expected_error): with self.assertRaisesRegex(FileNotFoundError, expected_error): NonConfigWorkflow(self.filename, self.data_dir, meta_file, logging_file) From 63717aea63a822cff3b220ef7c87f23beb479979 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 26 Mar 2024 06:21:26 +0000 Subject: [PATCH 7/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_bundle_workflow.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_bundle_workflow.py b/tests/test_bundle_workflow.py index 9f5da2edc6..0b0d51cbfb 100644 --- a/tests/test_bundle_workflow.py +++ b/tests/test_bundle_workflow.py @@ -11,7 +11,6 @@ from __future__ import annotations -import logging import os import shutil import tempfile From cf307c3aef5b84bba9913af637ab7845a9c931b1 Mon Sep 17 00:00:00 2001 From: Yiheng Wang Date: Tue, 26 Mar 2024 14:21:34 +0800 Subject: [PATCH 8/9] remove unused lib Signed-off-by: Yiheng Wang --- tests/test_bundle_workflow.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_bundle_workflow.py b/tests/test_bundle_workflow.py index 9f5da2edc6..0b0d51cbfb 100644 --- a/tests/test_bundle_workflow.py +++ b/tests/test_bundle_workflow.py @@ -11,7 +11,6 @@ from __future__ import annotations -import logging import os import shutil import tempfile From 859127017c9922bba00113fd44185ad3769bb781 Mon Sep 17 00:00:00 2001 From: Yiheng Wang Date: Wed, 27 Mar 2024 14:21:36 +0800 Subject: [PATCH 9/9] fix test error Signed-off-by: Yiheng Wang --- monai/bundle/workflows.py | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/monai/bundle/workflows.py b/monai/bundle/workflows.py index 0701aa2203..804b3b06f0 100644 --- a/monai/bundle/workflows.py +++ b/monai/bundle/workflows.py @@ -69,20 +69,6 @@ def __init__( meta_file: str | Sequence[str] | None = None, logging_file: str | None = None, ): - workflow_type = workflow if workflow is not None else workflow_type - if workflow_type is None: - self.properties = copy(MetaProperties) - self.workflow_type = None - return - if workflow_type.lower() in self.supported_train_type: - self.properties = {**TrainProperties, **MetaProperties} - self.workflow_type = "train" - elif workflow_type.lower() in self.supported_infer_type: - self.properties = {**InferProperties, **MetaProperties} - self.workflow_type = "infer" - else: - raise ValueError(f"Unsupported workflow type: '{workflow_type}'.") - if logging_file is not None: if not os.path.isfile(logging_file): raise FileNotFoundError(f"Cannot find the logging config file: {logging_file}.") @@ -105,6 +91,21 @@ def __init__( ) meta_file = None + workflow_type = workflow if workflow is not None else workflow_type + if workflow_type is None: + self.properties = copy(MetaProperties) + self.workflow_type = None + self.meta_file = meta_file + return + if workflow_type.lower() in self.supported_train_type: + self.properties = {**TrainProperties, **MetaProperties} + self.workflow_type = "train" + elif workflow_type.lower() in self.supported_infer_type: + self.properties = {**InferProperties, **MetaProperties} + self.workflow_type = "infer" + else: + raise ValueError(f"Unsupported workflow type: '{workflow_type}'.") + self.meta_file = meta_file @abstractmethod @@ -175,6 +176,13 @@ def get_workflow_type(self): """ return self.workflow_type + def get_meta_file(self): + """ + Get the meta file. + + """ + return self.meta_file + def add_property(self, name: str, required: str, desc: str | None = None) -> None: """ Besides the default predefined properties, some 3rd party applications may need the bundle