Skip to content

Commit

Permalink
Have ForwardModelRunner validate executable_file earlier
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jonathan-eq committed Sep 25, 2024
1 parent 9a5ab89 commit 535e314
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 21 deletions.
11 changes: 6 additions & 5 deletions src/_ert/forward_model_runner/io/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)]
Expand All @@ -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 ""
10 changes: 5 additions & 5 deletions src/_ert/forward_model_runner/job.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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"]

Expand Down Expand Up @@ -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):
Expand Down
20 changes: 9 additions & 11 deletions tests/ert/unit_tests/forward_model_runner/test_job.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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():
Expand Down

0 comments on commit 535e314

Please sign in to comment.