From e7fd053528dcbcf1b6178a6e855b191517777237 Mon Sep 17 00:00:00 2001 From: R1j1t <22280243+R1j1t@users.noreply.github.com> Date: Mon, 20 May 2024 02:45:27 -0400 Subject: [PATCH 1/7] added bugfix and corresponding tests --- parsl/executors/high_throughput/executor.py | 3 ++- .../high_throughput/mpi_prefix_composer.py | 9 ++++++++- .../test_mpi_apps/test_mpi_mode_enabled.py | 11 +++++++++- .../tests/test_mpi_apps/test_resource_spec.py | 20 +++++++++++-------- 4 files changed, 32 insertions(+), 11 deletions(-) diff --git a/parsl/executors/high_throughput/executor.py b/parsl/executors/high_throughput/executor.py index 7e4ae2ad2e..00ff35f2cf 100644 --- a/parsl/executors/high_throughput/executor.py +++ b/parsl/executors/high_throughput/executor.py @@ -645,7 +645,8 @@ def submit(self, func, resource_specification, *args, **kwargs): Returns: Future """ - validate_resource_spec(resource_specification) + + validate_resource_spec(resource_specification, is_mpi_enabled=self.enable_mpi_mode) if self.bad_state_is_set: raise self.executor_exception diff --git a/parsl/executors/high_throughput/mpi_prefix_composer.py b/parsl/executors/high_throughput/mpi_prefix_composer.py index 8b38be8549..ec12e918fb 100644 --- a/parsl/executors/high_throughput/mpi_prefix_composer.py +++ b/parsl/executors/high_throughput/mpi_prefix_composer.py @@ -18,13 +18,20 @@ def __str__(self): return f"Invalid resource specification options supplied: {self.invalid_keys}" -def validate_resource_spec(resource_spec: Dict[str, str]): +def validate_resource_spec(resource_spec: Dict[str, str], is_mpi_enabled: bool = False): """Basic validation of keys in the resource_spec Raises: InvalidResourceSpecification if the resource_spec is invalid (e.g, contains invalid keys) """ user_keys = set(resource_spec.keys()) + + # empty resource_spec when mpi_mode is set causes parsl to hang + # ref issue #3427 + if is_mpi_enabled and len(user_keys)==0: + raise InvalidResourceSpecification({'mpi_mode requires parsl_resource_specification to be configured'}) + + legal_keys = set(("ranks_per_node", "num_nodes", "num_ranks", diff --git a/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py b/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py index 44f412836a..e0ba3fb74d 100644 --- a/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py +++ b/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py @@ -6,6 +6,8 @@ from parsl import python_app, bash_app from parsl.tests.configs.htex_local import fresh_config +from parsl.executors.high_throughput.mpi_prefix_composer import InvalidResourceSpecification + import os EXECUTOR_LABEL = "MPI_TEST" @@ -162,10 +164,17 @@ def test_simulated_load(rounds: int = 100): "ranks_per_node": random.choice(ranks_per_node), } future = mock_app(sleep_dur=random.choice(sleep_choices), - parsl_resource_specification=resource_spec) + parsl_resource_specification=resource_spec) futures[future] = resource_spec for future in futures: total_ranks, nodes = future.result(timeout=10) assert len(nodes) == futures[future]["num_nodes"] assert total_ranks == futures[future]["num_nodes"] * futures[future]["ranks_per_node"] + +@pytest.mark.local +def test_missing_resource_spec(): + + with pytest.raises(InvalidResourceSpecification): + future = mock_app(sleep_dur=0.4) + future.result(timeout=10) diff --git a/parsl/tests/test_mpi_apps/test_resource_spec.py b/parsl/tests/test_mpi_apps/test_resource_spec.py index 4bef22cfff..4aef2ce7be 100644 --- a/parsl/tests/test_mpi_apps/test_resource_spec.py +++ b/parsl/tests/test_mpi_apps/test_resource_spec.py @@ -122,18 +122,22 @@ def test_top_level(): @pytest.mark.local @pytest.mark.parametrize( - "resource_spec, exception", + "resource_spec, is_mpi_enabled, exception", ( - ({"num_nodes": 2, "ranks_per_node": 1}, None), - ({"launcher_options": "--debug_foo"}, None), - ({"num_nodes": 2, "BAD_OPT": 1}, InvalidResourceSpecification), - ({}, None), + ({"num_nodes": 2, "ranks_per_node": 1}, False, None), + ({"launcher_options": "--debug_foo"}, False, None), + ({"num_nodes": 2, "BAD_OPT": 1}, False, InvalidResourceSpecification), + ({}, False, None), + ({"num_nodes": 2, "ranks_per_node": 1}, True, None), + ({"launcher_options": "--debug_foo"}, True, None), + ({"num_nodes": 2, "BAD_OPT": 1}, True, InvalidResourceSpecification), + ({}, True, InvalidResourceSpecification), ) ) -def test_resource_spec(resource_spec: Dict, exception): +def test_resource_spec(resource_spec: Dict, is_mpi_enabled: bool, exception): if exception: with pytest.raises(exception): - validate_resource_spec(resource_spec) + validate_resource_spec(resource_spec, is_mpi_enabled=is_mpi_enabled) else: - result = validate_resource_spec(resource_spec) + result = validate_resource_spec(resource_spec, is_mpi_enabled=is_mpi_enabled) assert result is None From 8789abaa1eb6f754315629f10f20bbb767ed40e5 Mon Sep 17 00:00:00 2001 From: R1j1t <22280243+R1j1t@users.noreply.github.com> Date: Mon, 20 May 2024 02:59:05 -0400 Subject: [PATCH 2/7] housekeeping tasks --- parsl/executors/high_throughput/mpi_prefix_composer.py | 3 +-- parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/parsl/executors/high_throughput/mpi_prefix_composer.py b/parsl/executors/high_throughput/mpi_prefix_composer.py index ec12e918fb..e4de34df06 100644 --- a/parsl/executors/high_throughput/mpi_prefix_composer.py +++ b/parsl/executors/high_throughput/mpi_prefix_composer.py @@ -28,9 +28,8 @@ def validate_resource_spec(resource_spec: Dict[str, str], is_mpi_enabled: bool = # empty resource_spec when mpi_mode is set causes parsl to hang # ref issue #3427 - if is_mpi_enabled and len(user_keys)==0: + if is_mpi_enabled and len(user_keys) == 0: raise InvalidResourceSpecification({'mpi_mode requires parsl_resource_specification to be configured'}) - legal_keys = set(("ranks_per_node", "num_nodes", diff --git a/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py b/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py index e0ba3fb74d..7fd4557c52 100644 --- a/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py +++ b/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py @@ -164,7 +164,7 @@ def test_simulated_load(rounds: int = 100): "ranks_per_node": random.choice(ranks_per_node), } future = mock_app(sleep_dur=random.choice(sleep_choices), - parsl_resource_specification=resource_spec) + parsl_resource_specification=resource_spec) futures[future] = resource_spec for future in futures: @@ -172,6 +172,7 @@ def test_simulated_load(rounds: int = 100): assert len(nodes) == futures[future]["num_nodes"] assert total_ranks == futures[future]["num_nodes"] * futures[future]["ranks_per_node"] + @pytest.mark.local def test_missing_resource_spec(): From 422bb85d922b3d374841ba59408470e402ca3464 Mon Sep 17 00:00:00 2001 From: R1j1t <22280243+R1j1t@users.noreply.github.com> Date: Wed, 22 May 2024 12:31:13 -0700 Subject: [PATCH 3/7] removed default & fixed `InvalidResourceSpecification` args --- .gitignore | 8 +++++++- parsl/executors/high_throughput/executor.py | 2 +- parsl/executors/high_throughput/mpi_prefix_composer.py | 10 +++++----- parsl/tests/test_mpi_apps/test_resource_spec.py | 4 ++-- 4 files changed, 15 insertions(+), 9 deletions(-) diff --git a/.gitignore b/.gitignore index acdb6254d1..654885aa23 100644 --- a/.gitignore +++ b/.gitignore @@ -62,7 +62,9 @@ htmlcov/ coverage.xml *.cover .hypothesis/ -/.pytest/ +.pytest/ +runinfo/ + # Translations *.mo @@ -120,3 +122,7 @@ ENV/ # emacs buffers \#* + +#deps +/.deps + diff --git a/parsl/executors/high_throughput/executor.py b/parsl/executors/high_throughput/executor.py index 00ff35f2cf..5407346ad8 100644 --- a/parsl/executors/high_throughput/executor.py +++ b/parsl/executors/high_throughput/executor.py @@ -646,7 +646,7 @@ def submit(self, func, resource_specification, *args, **kwargs): Future """ - validate_resource_spec(resource_specification, is_mpi_enabled=self.enable_mpi_mode) + validate_resource_spec(resource_specification, self.enable_mpi_mode) if self.bad_state_is_set: raise self.executor_exception diff --git a/parsl/executors/high_throughput/mpi_prefix_composer.py b/parsl/executors/high_throughput/mpi_prefix_composer.py index e4de34df06..c18b6cb7c1 100644 --- a/parsl/executors/high_throughput/mpi_prefix_composer.py +++ b/parsl/executors/high_throughput/mpi_prefix_composer.py @@ -1,5 +1,5 @@ import logging -from typing import Dict, List, Tuple, Set +from typing import Dict, List, Tuple, Set, Union logger = logging.getLogger(__name__) @@ -9,16 +9,16 @@ class InvalidResourceSpecification(Exception): - """Exception raised when Invalid keys are supplied via resource specification""" + """Exception raised when Invalid input is supplied via resource specification""" - def __init__(self, invalid_keys: Set[str]): + def __init__(self, invalid_keys: Union[Set[str], str]): self.invalid_keys = invalid_keys def __str__(self): return f"Invalid resource specification options supplied: {self.invalid_keys}" -def validate_resource_spec(resource_spec: Dict[str, str], is_mpi_enabled: bool = False): +def validate_resource_spec(resource_spec: Dict[str, str], is_mpi_enabled: bool): """Basic validation of keys in the resource_spec Raises: InvalidResourceSpecification if the resource_spec @@ -29,7 +29,7 @@ def validate_resource_spec(resource_spec: Dict[str, str], is_mpi_enabled: bool = # empty resource_spec when mpi_mode is set causes parsl to hang # ref issue #3427 if is_mpi_enabled and len(user_keys) == 0: - raise InvalidResourceSpecification({'mpi_mode requires parsl_resource_specification to be configured'}) + raise InvalidResourceSpecification('mpi_mode requires parsl_resource_specification to be configured') legal_keys = set(("ranks_per_node", "num_nodes", diff --git a/parsl/tests/test_mpi_apps/test_resource_spec.py b/parsl/tests/test_mpi_apps/test_resource_spec.py index 4aef2ce7be..0638c1e6ca 100644 --- a/parsl/tests/test_mpi_apps/test_resource_spec.py +++ b/parsl/tests/test_mpi_apps/test_resource_spec.py @@ -137,7 +137,7 @@ def test_top_level(): def test_resource_spec(resource_spec: Dict, is_mpi_enabled: bool, exception): if exception: with pytest.raises(exception): - validate_resource_spec(resource_spec, is_mpi_enabled=is_mpi_enabled) + validate_resource_spec(resource_spec, is_mpi_enabled) else: - result = validate_resource_spec(resource_spec, is_mpi_enabled=is_mpi_enabled) + result = validate_resource_spec(resource_spec, is_mpi_enabled) assert result is None From b9df25bdc0c19d140400e9a1f8577914e6e2f9f0 Mon Sep 17 00:00:00 2001 From: R1j1t <22280243+R1j1t@users.noreply.github.com> Date: Mon, 27 May 2024 00:33:35 -0700 Subject: [PATCH 4/7] updated missing resource exception --- .../high_throughput/mpi_prefix_composer.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/parsl/executors/high_throughput/mpi_prefix_composer.py b/parsl/executors/high_throughput/mpi_prefix_composer.py index c18b6cb7c1..07abf8f9d5 100644 --- a/parsl/executors/high_throughput/mpi_prefix_composer.py +++ b/parsl/executors/high_throughput/mpi_prefix_composer.py @@ -1,5 +1,5 @@ import logging -from typing import Dict, List, Tuple, Set, Union +from typing import Dict, List, Tuple, Set logger = logging.getLogger(__name__) @@ -8,10 +8,20 @@ 'mpiexec') +class MissingResourceSpecification(Exception): + """Exception raised when input is not supplied a resource specification""" + + def __init__(self, reason: str): + self.reason = reason + + def __str__(self): + return f"Missing resource specification: {self.reason}" + + class InvalidResourceSpecification(Exception): """Exception raised when Invalid input is supplied via resource specification""" - def __init__(self, invalid_keys: Union[Set[str], str]): + def __init__(self, invalid_keys: Set[str]): self.invalid_keys = invalid_keys def __str__(self): @@ -29,7 +39,7 @@ def validate_resource_spec(resource_spec: Dict[str, str], is_mpi_enabled: bool): # empty resource_spec when mpi_mode is set causes parsl to hang # ref issue #3427 if is_mpi_enabled and len(user_keys) == 0: - raise InvalidResourceSpecification('mpi_mode requires parsl_resource_specification to be configured') + raise MissingResourceSpecification('MPI mode requires optional parsl_resource_specification keyword argument to be configured') legal_keys = set(("ranks_per_node", "num_nodes", From e139eb54542c09bee26771bdbecdee461fb362a5 Mon Sep 17 00:00:00 2001 From: R1j1t <22280243+R1j1t@users.noreply.github.com> Date: Mon, 27 May 2024 00:35:01 -0700 Subject: [PATCH 5/7] updated tests for new exception --- parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py | 4 ++-- parsl/tests/test_mpi_apps/test_resource_spec.py | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py b/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py index 9b8387ad3a..c3d1bcfb26 100644 --- a/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py +++ b/parsl/tests/test_mpi_apps/test_mpi_mode_enabled.py @@ -6,7 +6,7 @@ from parsl import python_app, bash_app from parsl.tests.configs.htex_local import fresh_config -from parsl.executors.high_throughput.mpi_prefix_composer import InvalidResourceSpecification +from parsl.executors.high_throughput.mpi_prefix_composer import MissingResourceSpecification import os @@ -175,6 +175,6 @@ def test_simulated_load(rounds: int = 100): @pytest.mark.local def test_missing_resource_spec(): - with pytest.raises(InvalidResourceSpecification): + with pytest.raises(MissingResourceSpecification): future = mock_app(sleep_dur=0.4) future.result(timeout=10) diff --git a/parsl/tests/test_mpi_apps/test_resource_spec.py b/parsl/tests/test_mpi_apps/test_resource_spec.py index 0638c1e6ca..7e533c128f 100644 --- a/parsl/tests/test_mpi_apps/test_resource_spec.py +++ b/parsl/tests/test_mpi_apps/test_resource_spec.py @@ -19,7 +19,8 @@ ) from parsl.executors.high_throughput.mpi_prefix_composer import ( validate_resource_spec, - InvalidResourceSpecification + InvalidResourceSpecification, + MissingResourceSpecification ) EXECUTOR_LABEL = "MPI_TEST" @@ -131,7 +132,7 @@ def test_top_level(): ({"num_nodes": 2, "ranks_per_node": 1}, True, None), ({"launcher_options": "--debug_foo"}, True, None), ({"num_nodes": 2, "BAD_OPT": 1}, True, InvalidResourceSpecification), - ({}, True, InvalidResourceSpecification), + ({}, True, MissingResourceSpecification), ) ) def test_resource_spec(resource_spec: Dict, is_mpi_enabled: bool, exception): From 41922436132afb5bbffe951b210305cd92274fd5 Mon Sep 17 00:00:00 2001 From: R1j1t <22280243+R1j1t@users.noreply.github.com> Date: Mon, 27 May 2024 00:58:19 -0700 Subject: [PATCH 6/7] removed unrelated gitignore changes from this PR --- .gitignore | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index 654885aa23..57d2989425 100644 --- a/.gitignore +++ b/.gitignore @@ -62,9 +62,7 @@ htmlcov/ coverage.xml *.cover .hypothesis/ -.pytest/ -runinfo/ - +/.pytest/ # Translations *.mo @@ -123,6 +121,3 @@ ENV/ # emacs buffers \#* -#deps -/.deps - From 18e75f9d52ba9970a6435478cacfb4d833957e32 Mon Sep 17 00:00:00 2001 From: R1j1t <22280243+R1j1t@users.noreply.github.com> Date: Mon, 27 May 2024 00:59:46 -0700 Subject: [PATCH 7/7] housekeeping --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index 57d2989425..acdb6254d1 100644 --- a/.gitignore +++ b/.gitignore @@ -120,4 +120,3 @@ ENV/ # emacs buffers \#* -