From 535e314d1494f1a26469e0f4671d85be91e64d3a Mon Sep 17 00:00:00 2001 From: Jonathan Karlsen Date: Wed, 18 Sep 2024 15:05:06 +0200 Subject: [PATCH] Have ForwardModelRunner validate executable_file earlier This commit fixes the issue where the ForwardModelRunner on the job_runner side would crash when validating the existence of a ForwardModelStep executable; causing the drivers to hang while waiting for a never coming return code from JobRunner. --- src/_ert/forward_model_runner/io/__init__.py | 11 +++++----- src/_ert/forward_model_runner/job.py | 10 +++++----- .../forward_model_runner/test_job.py | 20 +++++++++---------- 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/_ert/forward_model_runner/io/__init__.py b/src/_ert/forward_model_runner/io/__init__.py index 6e9aa15d117..23d09b8fc7b 100644 --- a/src/_ert/forward_model_runner/io/__init__.py +++ b/src/_ert/forward_model_runner/io/__init__.py @@ -6,8 +6,8 @@ def cond_unlink(file): os.unlink(file) -def assert_file_executable(fname): - """The function raises an IOError if the given file is either not a file or +def check_executable(fname) -> str: + """The function returns an error message if the given file is either not a file or not an executable. If the given file name is an absolute path, its functionality is straight @@ -17,7 +17,7 @@ def assert_file_executable(fname): """ if not fname: - raise IOError("No executable provided!") + return "No executable provided!" fname = os.path.expanduser(fname) potential_executables = [os.path.abspath(fname)] @@ -28,7 +28,8 @@ def assert_file_executable(fname): ] if not any(map(os.path.isfile, potential_executables)): - raise IOError(f"{fname} is not a file!") + return f"{fname} is not a file!" if not any(os.access(fn, os.X_OK) for fn in potential_executables): - raise IOError(f"{fname} is not an executable!") + return f"{fname} is not an executable!" + return "" diff --git a/src/_ert/forward_model_runner/job.py b/src/_ert/forward_model_runner/job.py index 74179c58c4e..95737f71811 100644 --- a/src/_ert/forward_model_runner/job.py +++ b/src/_ert/forward_model_runner/job.py @@ -14,7 +14,7 @@ from psutil import AccessDenied, NoSuchProcess, Process, TimeoutExpired, ZombieProcess -from .io import assert_file_executable +from .io import check_executable from .reporting.message import ( Exited, ProcessTreeStatus, @@ -97,10 +97,7 @@ def run(self): yield start_message - executable = self.job_data.get("executable") - assert_file_executable(executable) - - arg_list = [executable] + arg_list = [self.job_data.get("executable")] if self.job_data.get("argList"): arg_list += self.job_data["argList"] @@ -314,6 +311,9 @@ def _check_job_files(self): ): os.unlink(self.job_data.get("error_file")) + if executable_error := check_executable(self.job_data.get("executable")): + errors.append(str(executable_error)) + return errors def _check_target_file_is_written(self, target_file_mtime: int, timeout=5): diff --git a/tests/ert/unit_tests/forward_model_runner/test_job.py b/tests/ert/unit_tests/forward_model_runner/test_job.py index 01b65aa6752..f10b8a921c6 100644 --- a/tests/ert/unit_tests/forward_model_runner/test_job.py +++ b/tests/ert/unit_tests/forward_model_runner/test_job.py @@ -16,14 +16,13 @@ from _ert.forward_model_runner.reporting.message import Exited, Running, Start -@patch("_ert.forward_model_runner.job.assert_file_executable") +@patch("_ert.forward_model_runner.job.check_executable") @patch("_ert.forward_model_runner.job.Popen") @patch("_ert.forward_model_runner.job.Process") @pytest.mark.usefixtures("use_tmpdir") -def test_run_with_process_failing( - mock_process, mock_popen, mock_assert_file_executable -): +def test_run_with_process_failing(mock_process, mock_popen, mock_check_executable): job = Job({}, 0) + mock_check_executable.return_value = "" type(mock_process.return_value.memory_info.return_value).rss = PropertyMock( return_value=10 ) @@ -290,9 +289,9 @@ def test_run_with_defined_executable_but_missing(): 0, ) - with pytest.raises(IOError): - for _ in job.run(): - pass + start_message = next(job.run()) + assert isinstance(start_message, Start) + assert "this/is/not/a/file is not a file" in start_message.error_message @pytest.mark.usefixtures("use_tmpdir") @@ -336,10 +335,9 @@ def test_run_with_defined_executable_no_exec_bit(): }, 0, ) - - with pytest.raises(IOError): - for _ in job.run(): - pass + start_message = next(job.run()) + assert isinstance(start_message, Start) + assert "foo is not an executable" in start_message.error_message def test_init_job_no_std():