From f338871247a1b61f61f8998fc6853715be829a0b Mon Sep 17 00:00:00 2001 From: Brady Planden Date: Thu, 18 Apr 2024 21:21:38 +0100 Subject: [PATCH 1/7] Bugfix optimisation.parallel, add coverage tests, initial multiprocessing import and context set --- pybop/__init__.py | 13 +++++++++++++ pybop/_optimisation.py | 4 ++-- tests/integration/test_optimisation_options.py | 6 ++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/pybop/__init__.py b/pybop/__init__.py index c5d1abf0..db6684a1 100644 --- a/pybop/__init__.py +++ b/pybop/__init__.py @@ -8,6 +8,19 @@ import sys from os import path +# +# Multiprocessing +# +try: + import multiprocessing as mp + if sys.platform == "win32": + mp.set_start_method("spawn") + else: + mp.set_start_method("fork") +except ImportError as e: + print(f"Warning: multiprocessing module not available: {e}") + pass + # # Version info # diff --git a/pybop/_optimisation.py b/pybop/_optimisation.py index 2a0b8ed9..9e8d2fea 100644 --- a/pybop/_optimisation.py +++ b/pybop/_optimisation.py @@ -234,8 +234,8 @@ def _run_pints(self): # For population based optimisers, don't use more workers than # particles! - if isinstance(self._optimiser, pints.PopulationBasedOptimiser): - n_workers = min(n_workers, self._optimiser.population_size()) + if isinstance(self.optimiser, pints.PopulationBasedOptimiser): + n_workers = min(n_workers, self.optimiser.population_size()) evaluator = pints.ParallelEvaluator(f, n_workers=n_workers) else: evaluator = pints.SequentialEvaluator(f) diff --git a/tests/integration/test_optimisation_options.py b/tests/integration/test_optimisation_options.py index e026aa40..9ff70056 100644 --- a/tests/integration/test_optimisation_options.py +++ b/tests/integration/test_optimisation_options.py @@ -1,3 +1,5 @@ +import sys + import numpy as np import pytest @@ -86,6 +88,10 @@ def test_optimisation_f_guessed(self, f_guessed, spm_costs): parameterisation.set_max_iterations(125) parameterisation.set_f_guessed_tracking(f_guessed) + # Set parallelisation if not on Windows + if sys.platform != "win32": + parameterisation.set_parallel(True) + initial_cost = parameterisation.cost(spm_costs.x0) x, final_cost = parameterisation.run() From a3500441dbed003afa3c434bb1f7f2d8070b4a42 Mon Sep 17 00:00:00 2001 From: Brady Planden Date: Fri, 19 Apr 2024 14:07:09 +0100 Subject: [PATCH 2/7] add multiprocessing import tests --- tests/unit/test_import.py | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 tests/unit/test_import.py diff --git a/tests/unit/test_import.py b/tests/unit/test_import.py new file mode 100644 index 00000000..01f25d35 --- /dev/null +++ b/tests/unit/test_import.py @@ -0,0 +1,38 @@ +import importlib +import sys +from unittest.mock import patch + +import pytest + +import pybop + + +@pytest.mark.unit +def test_multiprocessing_init_non_win32(monkeypatch): + """Test multiprocessing init on non-Windows platforms""" + monkeypatch.setattr(sys, "platform", "linux") + print(sys.platform) + with patch("multiprocessing.set_start_method") as mock_set_start_method: + importlib.reload(pybop) + mock_set_start_method.assert_called_once_with("fork") + + +@pytest.mark.unit +def test_multiprocessing_init_win32(monkeypatch): + """Test multiprocessing init on Windows""" + monkeypatch.setattr(sys, "platform", "win32") + with patch("multiprocessing.set_start_method") as mock_set_start_method: + importlib.reload(pybop) + # import pybop.__init__ as init + mock_set_start_method.assert_called_once_with("spawn") + + +def test_multiprocessing_import_error(capsys): + with patch.dict(sys.modules, {"multiprocessing": None}): + with pytest.raises(ImportError) as excinfo: + importlib.reload(pybop) + + assert "multiprocessing" in str(excinfo.value) + + captured = capsys.readouterr() + assert "Warning: multiprocessing module not available:" in captured.out From 36563a6fe2e3d34c37095a6a706670aa2b4aa000 Mon Sep 17 00:00:00 2001 From: Brady Planden Date: Fri, 19 Apr 2024 14:49:25 +0100 Subject: [PATCH 3/7] Update import warning, add changelog entry --- CHANGELOG.md | 1 + pybop/__init__.py | 9 +++++++-- tests/unit/test_import.py | 1 - 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64e150de..177caf5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ codesigned binaries and source distributions via `sigstore-python`. ## Bug Fixes +- [#299](https://github.com/pybop-team/PyBOP/pull/299) - Bugfix multiprocessing support for Linux, MacOS, Windows (WSL) and improves coverage. - [#270](https://github.com/pybop-team/PyBOP/pull/270) - Updates PR template. - [#91](https://github.com/pybop-team/PyBOP/issues/91) - Adds a check on the number of parameters for CMAES and makes XNES the default optimiser. diff --git a/pybop/__init__.py b/pybop/__init__.py index db6684a1..785a88aa 100644 --- a/pybop/__init__.py +++ b/pybop/__init__.py @@ -17,8 +17,13 @@ mp.set_start_method("spawn") else: mp.set_start_method("fork") -except ImportError as e: - print(f"Warning: multiprocessing module not available: {e}") +except Exception as e: + error_message = ( + "Multiprocessing context could not be set. " + "Continuing import without setting context.\n" + f"Error: {e}" + ) + print(error_message) pass # diff --git a/tests/unit/test_import.py b/tests/unit/test_import.py index 01f25d35..99936d1d 100644 --- a/tests/unit/test_import.py +++ b/tests/unit/test_import.py @@ -23,7 +23,6 @@ def test_multiprocessing_init_win32(monkeypatch): monkeypatch.setattr(sys, "platform", "win32") with patch("multiprocessing.set_start_method") as mock_set_start_method: importlib.reload(pybop) - # import pybop.__init__ as init mock_set_start_method.assert_called_once_with("spawn") From 20652896f821ee6e7be4f1d674a3ea7a9af648b0 Mon Sep 17 00:00:00 2001 From: Brady Planden Date: Fri, 19 Apr 2024 15:43:11 +0100 Subject: [PATCH 4/7] Remove coverage from import exception lines, remove test --- pybop/__init__.py | 8 ++++---- tests/unit/test_import.py | 11 ----------- 2 files changed, 4 insertions(+), 15 deletions(-) diff --git a/pybop/__init__.py b/pybop/__init__.py index 785a88aa..c25abdc7 100644 --- a/pybop/__init__.py +++ b/pybop/__init__.py @@ -17,14 +17,14 @@ mp.set_start_method("spawn") else: mp.set_start_method("fork") -except Exception as e: +except Exception as e: # pragma: no cover error_message = ( "Multiprocessing context could not be set. " "Continuing import without setting context.\n" f"Error: {e}" - ) - print(error_message) - pass + ) # pragma: no cover + print(error_message) # pragma: no cover + pass # pragma: no cover # # Version info diff --git a/tests/unit/test_import.py b/tests/unit/test_import.py index 99936d1d..f23ad677 100644 --- a/tests/unit/test_import.py +++ b/tests/unit/test_import.py @@ -24,14 +24,3 @@ def test_multiprocessing_init_win32(monkeypatch): with patch("multiprocessing.set_start_method") as mock_set_start_method: importlib.reload(pybop) mock_set_start_method.assert_called_once_with("spawn") - - -def test_multiprocessing_import_error(capsys): - with patch.dict(sys.modules, {"multiprocessing": None}): - with pytest.raises(ImportError) as excinfo: - importlib.reload(pybop) - - assert "multiprocessing" in str(excinfo.value) - - captured = capsys.readouterr() - assert "Warning: multiprocessing module not available:" in captured.out From 9504925079393ad3d1aff553f5c30264db5c5cd9 Mon Sep 17 00:00:00 2001 From: Brady Planden Date: Mon, 22 Apr 2024 18:23:54 +0100 Subject: [PATCH 5/7] Updt test_import with module delete and import --- tests/unit/test_import.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/unit/test_import.py b/tests/unit/test_import.py index f23ad677..a87897f2 100644 --- a/tests/unit/test_import.py +++ b/tests/unit/test_import.py @@ -4,16 +4,14 @@ import pytest -import pybop - @pytest.mark.unit def test_multiprocessing_init_non_win32(monkeypatch): """Test multiprocessing init on non-Windows platforms""" monkeypatch.setattr(sys, "platform", "linux") - print(sys.platform) with patch("multiprocessing.set_start_method") as mock_set_start_method: - importlib.reload(pybop) + del sys.modules["pybop"] + importlib.import_module("pybop") mock_set_start_method.assert_called_once_with("fork") @@ -21,6 +19,7 @@ def test_multiprocessing_init_non_win32(monkeypatch): def test_multiprocessing_init_win32(monkeypatch): """Test multiprocessing init on Windows""" monkeypatch.setattr(sys, "platform", "win32") + del sys.modules["pybop"] with patch("multiprocessing.set_start_method") as mock_set_start_method: - importlib.reload(pybop) + importlib.import_module("pybop") mock_set_start_method.assert_called_once_with("spawn") From b3872d53571e6e0f14d8d3ee5629e91bad5c83f0 Mon Sep 17 00:00:00 2001 From: Brady Planden Date: Mon, 22 Apr 2024 19:28:14 +0100 Subject: [PATCH 6/7] update unload module method --- tests/unit/test_import.py | 44 +++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/tests/unit/test_import.py b/tests/unit/test_import.py index a87897f2..e38d960a 100644 --- a/tests/unit/test_import.py +++ b/tests/unit/test_import.py @@ -5,21 +5,33 @@ import pytest -@pytest.mark.unit -def test_multiprocessing_init_non_win32(monkeypatch): - """Test multiprocessing init on non-Windows platforms""" - monkeypatch.setattr(sys, "platform", "linux") - with patch("multiprocessing.set_start_method") as mock_set_start_method: - del sys.modules["pybop"] - importlib.import_module("pybop") - mock_set_start_method.assert_called_once_with("fork") +class TestImport: + @pytest.mark.unit + def test_multiprocessing_init_non_win32(self, monkeypatch): + """Test multiprocessing init on non-Windows platforms""" + monkeypatch.setattr(sys, "platform", "linux") + # Unload pybop and its sub-modules + self.unload_pybop() + with patch("multiprocessing.set_start_method") as mock_set_start_method: + importlib.import_module("pybop") + mock_set_start_method.assert_called_once_with("fork") + @pytest.mark.unit + def test_multiprocessing_init_win32(self, monkeypatch): + """Test multiprocessing init on Windows""" + monkeypatch.setattr(sys, "platform", "win32") + self.unload_pybop() + with patch("multiprocessing.set_start_method") as mock_set_start_method: + importlib.import_module("pybop") + mock_set_start_method.assert_called_once_with("spawn") -@pytest.mark.unit -def test_multiprocessing_init_win32(monkeypatch): - """Test multiprocessing init on Windows""" - monkeypatch.setattr(sys, "platform", "win32") - del sys.modules["pybop"] - with patch("multiprocessing.set_start_method") as mock_set_start_method: - importlib.import_module("pybop") - mock_set_start_method.assert_called_once_with("spawn") + def unload_pybop(self): + """ + Unload pybop and its sub-modules. Credit PyBaMM team: + https://github.com/pybamm-team/PyBaMM/blob/develop/tests/unit/test_util.py + """ + # Unload pybop and its sub-modules + for module_name in list(sys.modules.keys()): + base_module_name = module_name.split(".")[0] + if base_module_name == "pybop": + sys.modules.pop(module_name) From fed4a125e2d8a85c96be70050caa87ccd2529f97 Mon Sep 17 00:00:00 2001 From: Brady Planden <55357039+BradyPlanden@users.noreply.github.com> Date: Mon, 22 Apr 2024 21:33:15 +0100 Subject: [PATCH 7/7] Update tests/unit/test_import.py Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com> --- tests/unit/test_import.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_import.py b/tests/unit/test_import.py index e38d960a..a24fb107 100644 --- a/tests/unit/test_import.py +++ b/tests/unit/test_import.py @@ -28,7 +28,7 @@ def test_multiprocessing_init_win32(self, monkeypatch): def unload_pybop(self): """ Unload pybop and its sub-modules. Credit PyBaMM team: - https://github.com/pybamm-team/PyBaMM/blob/develop/tests/unit/test_util.py + https://github.com/pybamm-team/PyBaMM/blob/90c1c357a97dfd5c8c6a9092a70dddf0dac978db/tests/unit/test_util.py """ # Unload pybop and its sub-modules for module_name in list(sys.modules.keys()):