From f3bac9d85b05ac23ce8fe4bfa3ba5e17db735352 Mon Sep 17 00:00:00 2001 From: Sophie Gerits Date: Wed, 11 Sep 2024 10:48:41 -0600 Subject: [PATCH 01/10] Added JDFTx jobs.py script needed to run JDFTx using Custodian Job. --- src/custodian/jdftx/jobs.py | 88 +++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 src/custodian/jdftx/jobs.py diff --git a/src/custodian/jdftx/jobs.py b/src/custodian/jdftx/jobs.py new file mode 100644 index 00000000..6a4c2004 --- /dev/null +++ b/src/custodian/jdftx/jobs.py @@ -0,0 +1,88 @@ +"""This module implements basic kinds of jobs for JDFTx runs.""" + +import logging +import os +import subprocess + +from custodian.custodian import Job + +logger = logging.getLogger(__name__) + + +class JDFTxJob(Job): + """A basic JDFTx job. Runs whatever is in the working directory.""" + + # If testing, use something like: + # job = JDFTxJob() + # job.run() # assumes input files already written to directory + + # Used Cp2kJob developed by Nick Winner as a template. + + def __init__( + self, + jdftx_cmd, + input_file="jdftx.in", + output_file="jdftx.out", + stderr_file="std_err.txt", + ) -> None: + """ + This constructor is necessarily complex due to the need for + flexibility. For standard kinds of runs, it's often better to use one + of the static constructors. The defaults are usually fine too. + + Args: + jdftx_cmd (str): Command to run JDFTx as a string. + input_file (str): Name of the file to use as input to JDFTx + executable. Defaults to "input.in" + output_file (str): Name of file to direct standard out to. + Defaults to "jdftx.out". + stderr_file (str): Name of file to direct standard error to. + Defaults to "std_err.txt". + """ + self.jdftx_cmd = jdftx_cmd + self.input_file = input_file + self.output_file = output_file + self.stderr_file = stderr_file + + def setup(self, directory="./") -> None: + """No setup required.""" + + def run(self, directory="./"): + """ + Perform the actual JDFTx run. + + Returns: + ------- + (subprocess.Popen) Used for monitoring. + """ + cmd = self.jdftx_cmd + " -i " + self.input_file + " -o " + self.output_file + logger.info(f"Running {cmd}") + with ( + open(os.path.join(directory, self.output_file), "w") as f_std, + open(os.path.join(directory, self.stderr_file), "w", buffering=1) as f_err, + ): + # use line buffering for stderr + return subprocess.run( + cmd.split(), + cwd=directory, + stdout=f_std, + stderr=f_err, + shell=False, + check=False, + ) + + def postprocess(self, directory="./") -> None: + """No post-processing required.""" + + def terminate(self, directory="./") -> None: + """Terminate JDFTx.""" + # This will kill any running process with "jdftx" in the name, + # this might have unintended consequences if running multiple jdftx processes + # on the same node. + for cmd in self.jdftx_cmd: + if "jdftx" in cmd: + try: + os.system(f"killall {cmd}") + except Exception as e: + print(f"Unexpected error occurred: {e}") + raise From baedd283e0e613fbdd76ae32287be966dd84e85a Mon Sep 17 00:00:00 2001 From: Sophie Gerits Date: Wed, 11 Sep 2024 11:16:14 -0600 Subject: [PATCH 02/10] Added __init__.py --- src/custodian/jdftx/__init__.py | 4 ++++ src/custodian/jdftx/jobs.py | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 src/custodian/jdftx/__init__.py diff --git a/src/custodian/jdftx/__init__.py b/src/custodian/jdftx/__init__.py new file mode 100644 index 00000000..a5f0efca --- /dev/null +++ b/src/custodian/jdftx/__init__.py @@ -0,0 +1,4 @@ +""" +This package implements various JDFTx Jobs and Error Handlers. +Used Cp2kJob developed by Nick Winner as a template. +""" \ No newline at end of file diff --git a/src/custodian/jdftx/jobs.py b/src/custodian/jdftx/jobs.py index 6a4c2004..4084ca1c 100644 --- a/src/custodian/jdftx/jobs.py +++ b/src/custodian/jdftx/jobs.py @@ -16,8 +16,6 @@ class JDFTxJob(Job): # job = JDFTxJob() # job.run() # assumes input files already written to directory - # Used Cp2kJob developed by Nick Winner as a template. - def __init__( self, jdftx_cmd, From 0e23c63110dc3d05fa420c5bb52be6238396e727 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 11 Sep 2024 17:24:45 +0000 Subject: [PATCH 03/10] pre-commit auto-fixes --- src/custodian/jdftx/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/custodian/jdftx/__init__.py b/src/custodian/jdftx/__init__.py index a5f0efca..2fe49558 100644 --- a/src/custodian/jdftx/__init__.py +++ b/src/custodian/jdftx/__init__.py @@ -1,4 +1,4 @@ """ This package implements various JDFTx Jobs and Error Handlers. Used Cp2kJob developed by Nick Winner as a template. -""" \ No newline at end of file +""" From 82d1adb6575cea185c6baeff9ed95b4d0c4863a4 Mon Sep 17 00:00:00 2001 From: Sophie Gerits Date: Thu, 26 Sep 2024 09:58:20 -0600 Subject: [PATCH 04/10] will need to make this more flexible --- src/custodian/jdftx/jobs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/custodian/jdftx/jobs.py b/src/custodian/jdftx/jobs.py index 6a4c2004..7cee99e4 100644 --- a/src/custodian/jdftx/jobs.py +++ b/src/custodian/jdftx/jobs.py @@ -21,7 +21,7 @@ class JDFTxJob(Job): def __init__( self, jdftx_cmd, - input_file="jdftx.in", + input_file="init.in", output_file="jdftx.out", stderr_file="std_err.txt", ) -> None: From a6886886c2c3e38532e2e141bdcdd1799fa607b8 Mon Sep 17 00:00:00 2001 From: Sophie Gerits Date: Sun, 29 Sep 2024 16:26:42 -0600 Subject: [PATCH 05/10] Changed input file name to init.in. --- src/custodian/jdftx/jobs.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/custodian/jdftx/jobs.py b/src/custodian/jdftx/jobs.py index 7cee99e4..4f5983ec 100644 --- a/src/custodian/jdftx/jobs.py +++ b/src/custodian/jdftx/jobs.py @@ -12,10 +12,6 @@ class JDFTxJob(Job): """A basic JDFTx job. Runs whatever is in the working directory.""" - # If testing, use something like: - # job = JDFTxJob() - # job.run() # assumes input files already written to directory - # Used Cp2kJob developed by Nick Winner as a template. def __init__( @@ -33,7 +29,7 @@ def __init__( Args: jdftx_cmd (str): Command to run JDFTx as a string. input_file (str): Name of the file to use as input to JDFTx - executable. Defaults to "input.in" + executable. Defaults to "init.in" output_file (str): Name of file to direct standard out to. Defaults to "jdftx.out". stderr_file (str): Name of file to direct standard error to. From 4a44a831969c048bc9e5634e3c15aae06c6f5bfd Mon Sep 17 00:00:00 2001 From: Sophie Gerits Date: Thu, 10 Oct 2024 19:51:57 -0600 Subject: [PATCH 06/10] Finished tests for jobs.py + changed the terminate method --- src/custodian/jdftx/jobs.py | 58 +++++++++--- src/custodian/vasp/jobs.py | 4 +- tests/jdftx/test_jobs.py | 144 +++++++++++++++++++++++++++++ tests/jdftx/test_jobs_test.py | 165 ++++++++++++++++++++++++++++++++++ 4 files changed, 355 insertions(+), 16 deletions(-) create mode 100644 tests/jdftx/test_jobs.py create mode 100644 tests/jdftx/test_jobs_test.py diff --git a/src/custodian/jdftx/jobs.py b/src/custodian/jdftx/jobs.py index 4f5983ec..af359ffd 100644 --- a/src/custodian/jdftx/jobs.py +++ b/src/custodian/jdftx/jobs.py @@ -4,6 +4,8 @@ import os import subprocess +import psutil + from custodian.custodian import Job logger = logging.getLogger(__name__) @@ -22,10 +24,6 @@ def __init__( stderr_file="std_err.txt", ) -> None: """ - This constructor is necessarily complex due to the need for - flexibility. For standard kinds of runs, it's often better to use one - of the static constructors. The defaults are usually fine too. - Args: jdftx_cmd (str): Command to run JDFTx as a string. input_file (str): Name of the file to use as input to JDFTx @@ -72,13 +70,45 @@ def postprocess(self, directory="./") -> None: def terminate(self, directory="./") -> None: """Terminate JDFTx.""" - # This will kill any running process with "jdftx" in the name, - # this might have unintended consequences if running multiple jdftx processes - # on the same node. - for cmd in self.jdftx_cmd: - if "jdftx" in cmd: - try: - os.system(f"killall {cmd}") - except Exception as e: - print(f"Unexpected error occurred: {e}") - raise + + work_dir = directory + logger.info(f"Killing JDFTx processes in {work_dir=}.") + for proc in psutil.process_iter(): + try: + if "jdftx" in proc.name(): + print("name:", proc.name()) + open_paths = [file.path for file in proc.open_files()] + run_path = os.path.join(work_dir, self.output_file) + if (run_path in open_paths) and psutil.pid_exists(proc.pid): + self.terminate_process(proc) + return + except (psutil.NoSuchProcess, psutil.AccessDenied) as e: + logger.warning(f"Exception {e} encountered while killing JDFTx.") + continue + + logger.warning( + f"Killing JDFTx processes in {work_dir=} failed with subprocess.Popen.terminate(). Resorting to 'killall'." + ) + cmd = self.jdftx_cmd + print("cmd:", cmd) + if "jdftx" in cmd: + subprocess.run(["killall", f"{cmd}"], check=False) + + + @staticmethod + def terminate_process(proc, timeout=5): + try: + proc.terminate() + try: + proc.wait(timeout=timeout) + except psutil.TimeoutExpired: + # If process is still running after the timeout, kill it + logger.warning(f"Process {proc.pid} did not terminate gracefully, killing it.") + proc.kill() + #proc.wait() + else: + logger.info(f"Process {proc.pid} terminated gracefully.") + except (psutil.NoSuchProcess, psutil.AccessDenied) as e: + logger.warning(f"Error while terminating process {proc.pid}: {e}") + + diff --git a/src/custodian/vasp/jobs.py b/src/custodian/vasp/jobs.py index ffef7ea8..b0781515 100644 --- a/src/custodian/vasp/jobs.py +++ b/src/custodian/vasp/jobs.py @@ -252,8 +252,8 @@ def run(self, directory="./"): open(os.path.join(directory, self.output_file), "w") as f_std, open(os.path.join(directory, self.stderr_file), "w", buffering=1) as f_err, ): - # use line buffering for stderr - return subprocess.Popen(cmd, cwd=directory, stdout=f_std, stderr=f_err, start_new_session=True) # pylint: disable=R1732 + # use line buffering for stderrsubprocess.Popen(cmd, cwd=directory, stdout=f_std, stderr=f_err, start_new_session=True) + return # pylint: disable=R1732 def postprocess(self, directory="./") -> None: """ diff --git a/tests/jdftx/test_jobs.py b/tests/jdftx/test_jobs.py new file mode 100644 index 00000000..b622a5cb --- /dev/null +++ b/tests/jdftx/test_jobs.py @@ -0,0 +1,144 @@ +import os +import pytest +from pathlib import Path +from unittest.mock import patch, MagicMock, ANY +import psutil +from custodian.jdftx.jobs import JDFTxJob + +TEST_DIR = Path(__file__).resolve().parent.parent +TEST_FILES = f"{TEST_DIR}/files/jdftx" + +@pytest.fixture +def jdftx_job(): + return JDFTxJob(jdftx_cmd="jdftx") + +@pytest.fixture +def mock_process(tmp_path, jdftx_job): + process = MagicMock(spec=psutil.Process) + process.name.return_value = "jdftx" + process.pid = 12345 + process.open_files.return_value = [MagicMock(path=os.path.join(str(tmp_path), jdftx_job.output_file))] + return process + + +def test_jdftx_job_init(jdftx_job): + assert jdftx_job.jdftx_cmd == "jdftx" + assert jdftx_job.input_file == "init.in" + assert jdftx_job.output_file == "jdftx.out" + assert jdftx_job.stderr_file == "std_err.txt" + +def test_jdftx_job_setup(jdftx_job, tmp_path): + jdftx_job.setup(str(tmp_path)) + # Setup method doesn't do anything, so just checking that it doesn't raise an exception + +def test_jdftx_job_run(jdftx_job, tmp_path): + with patch("subprocess.run") as mock_run: + mock_process = MagicMock() + mock_run.return_value = mock_process + + result = jdftx_job.run(str(tmp_path)) + + assert result == mock_process + mock_run.assert_called_once_with( + ["jdftx", "-i", "init.in", "-o", "jdftx.out"], + cwd=str(tmp_path), + stdout=ANY, + stderr=ANY, + shell=False, + check=False, + ) + +def test_jdftx_job_run_creates_output_files(jdftx_job, temp_dir): + with patch("subprocess.run"): + jdftx_job.run(str(temp_dir)) + + assert os.path.exists(os.path.join(str(temp_dir), "jdftx.out")) + assert os.path.exists(os.path.join(str(temp_dir), "std_err.txt")) + +def test_jdftx_job_postprocess(jdftx_job, tmp_path): + jdftx_job.postprocess(str(tmp_path)) + # Postprocess method doesn't do anything, so we just check that it doesn't raise an exception + +@patch("psutil.process_iter") +@patch("psutil.pid_exists") +@patch("subprocess.run") +@patch.object(JDFTxJob, 'terminate_process', autospec=True) +def test_jdftx_job_terminate(mock_terminate_process, mock_subprocess_run, mock_pid_exists, mock_process_iter, jdftx_job, mock_process, tmp_path): + jdftx_job.output_file = "jdftx.out" # Ensure this matches the actual output file name + run_path = os.path.join(str(tmp_path), jdftx_job.output_file) + mock_process.open_files.return_value = [MagicMock(path=run_path)] + mock_process.name.return_value = "jdftx" + mock_process_iter.return_value = [mock_process] + + # Test successful termination + mock_pid_exists.return_value = True + jdftx_job.terminate(str(tmp_path)) + mock_terminate_process.assert_called_once_with(mock_process) + mock_subprocess_run.assert_not_called() + + mock_terminate_process.reset_mock() + mock_subprocess_run.reset_mock() + + # Test when process doesn't exist + mock_process.name.return_value = "not_jdft" + jdftx_job.terminate(str(tmp_path)) + mock_terminate_process.assert_not_called() + mock_subprocess_run.assert_called_once_with(["killall", "jdftx"], check=False) + + mock_terminate_process.reset_mock() + mock_subprocess_run.reset_mock() + + # Test when psutil raises exceptions + mock_pid_exists.return_value = "jdftx" + mock_process_iter.side_effect = psutil.NoSuchProcess(pid=12345) + jdftx_job.terminate(str(tmp_path)) + mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False) + + mock_terminate_process.reset_mock() + mock_subprocess_run.reset_mock() + mock_pid_exists.side_effect = None + mock_process_iter.side_effect = None + + #mock_pid_exists.return_value = True + mock_process_iter.side_effect = psutil.AccessDenied(pid=12345) + jdftx_job.terminate(str(tmp_path)) + mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False) + +@patch("psutil.Process") +def test_terminate_process(mock_process, jdftx_job): + process = mock_process.return_value + + # Test successful termination + jdftx_job.terminate_process(process) + process.terminate.assert_called_once() + process.wait.assert_called_once_with(timeout=5) + + process.reset_mock() + + # Test when process doesn't terminate gracefully + process.wait.side_effect = [psutil.TimeoutExpired(5), None] + jdftx_job.terminate_process(process) + process.kill.assert_called_once() + + # Test when process raises NoSuchProcess + process.terminate.side_effect = psutil.NoSuchProcess(pid=12345) + jdftx_job.terminate_process(process) + + process.reset_mock() + + # Test when process raises AccessDenied + process.terminate.side_effect = psutil.AccessDenied(pid=12345) + jdftx_job.terminate_process(process) + +def test_jdftx_job_with_custom_parameters(): + custom_job = JDFTxJob( + jdftx_cmd="custom_jdftx", + input_file="custom.in", + output_file="custom.out", + stderr_file="custom_err.txt" + ) + + assert custom_job.jdftx_cmd == "custom_jdftx" + assert custom_job.input_file == "custom.in" + assert custom_job.output_file == "custom.out" + assert custom_job.stderr_file == "custom_err.txt" diff --git a/tests/jdftx/test_jobs_test.py b/tests/jdftx/test_jobs_test.py new file mode 100644 index 00000000..038cdb16 --- /dev/null +++ b/tests/jdftx/test_jobs_test.py @@ -0,0 +1,165 @@ +import os +import pytest +from pathlib import Path +import psutil +from custodian.jdftx.jobs import JDFTxJob +from unittest import mock +from unittest.mock import patch, MagicMock, ANY + +TEST_DIR = Path(__file__).resolve().parent.parent +TEST_FILES = f"{TEST_DIR}/files/jdftx" + +@pytest.fixture +def jdftx_job(): + return JDFTxJob(jdftx_cmd="jdftx", output_file="jdftx.out") + +def create_mock_process(name="jdftx", open_files=None, pid=12345, name_side_effect=None, wait_side_effect=None, terminate_side_effect=None): + if open_files is None: # Set a default value if not provided + open_files = [MagicMock(path=os.path.join("default_path", "jdftx.out"))] + + mock_process = mock.Mock(spec=psutil.Process) + mock_process.name.return_value = name + mock_process.open_files.return_value = open_files + mock_process.pid = pid + mock_process.name.side_effect = name_side_effect + mock_process.wait.side_effect = wait_side_effect + mock_process.terminate.side_effect = terminate_side_effect + return mock_process + +def test_jdftx_job_init(jdftx_job): + assert jdftx_job.jdftx_cmd == "jdftx" + assert jdftx_job.input_file == "init.in" + assert jdftx_job.output_file == "jdftx.out" + assert jdftx_job.stderr_file == "std_err.txt" + +def test_jdftx_job_setup(jdftx_job, tmp_path): + jdftx_job.setup(str(tmp_path)) + # Setup method doesn't do anything, so just checking that it doesn't raise an exception + +def test_jdftx_job_run(jdftx_job, tmp_path): + with patch("subprocess.run") as mock_run: + mock_process = MagicMock() + mock_run.return_value = mock_process + + result = jdftx_job.run(str(tmp_path)) + + assert result == mock_process + mock_run.assert_called_once_with( + ["jdftx", "-i", "init.in", "-o", "jdftx.out"], + cwd=str(tmp_path), + stdout=ANY, + stderr=ANY, + shell=False, + check=False, + ) + +def test_jdftx_job_run_creates_output_files(jdftx_job, tmp_path): + with patch("subprocess.run"): + jdftx_job.run(str(tmp_path)) + + assert os.path.exists(os.path.join(str(tmp_path), "jdftx.out")) + assert os.path.exists(os.path.join(str(tmp_path), "std_err.txt")) + +def test_jdftx_job_postprocess(jdftx_job, tmp_path): + jdftx_job.postprocess(str(tmp_path)) + # Postprocess method doesn't do anything, so we just check that it doesn't raise an exception + + +@mock.patch("psutil.pid_exists") +@mock.patch("subprocess.run") +@mock.patch.object(JDFTxJob, 'terminate_process', autospec=True) +def test_jdftx_job_terminate(mock_terminate_process, mock_subprocess_run, mock_pid_exists, jdftx_job, tmp_path, caplog): + open_files=[MagicMock(path=os.path.join(str(tmp_path), jdftx_job.output_file))] + # Test when JDFTx process exists + mock_process = create_mock_process(name="jdftx", open_files=open_files, pid=12345) + + with patch("psutil.process_iter", return_value=[mock_process]): + mock_pid_exists.return_value = True + jdftx_job.terminate(str(tmp_path)) + mock_terminate_process.assert_called_once_with(mock_process) + mock_subprocess_run.assert_not_called() + + mock_terminate_process.reset_mock() + mock_subprocess_run.reset_mock() + + # Test when no JDFTx process exists + mock_process = create_mock_process(name="vasp", open_files=open_files, pid=12345) + + with patch("psutil.process_iter", return_value=[mock_process]): + jdftx_job.terminate(str(tmp_path)) + mock_terminate_process.assert_not_called() + mock_subprocess_run.assert_called_once_with(["killall", "jdftx"], check=False) + + mock_terminate_process.reset_mock() + mock_subprocess_run.reset_mock() + + # Test when psutil.process_iter raises NoSuchProcess + mock_process = create_mock_process(name="jdftx", open_files=open_files, pid=12345, name_side_effect=psutil.NoSuchProcess(pid=12345)) + + with caplog.at_level("WARNING"): + with patch("psutil.process_iter", return_value=[mock_process]): + jdftx_job.terminate(str(tmp_path)) + mock_terminate_process.assert_not_called() + mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False) + + assert "Exception" in caplog.text + assert "encountered while killing JDFTx" in caplog.text + + mock_terminate_process.reset_mock() + mock_subprocess_run.reset_mock() + + # Test when psutil.process_iter raises AccessDenied + with caplog.at_level("WARNING"): + mock_process = create_mock_process(name="jdftx", open_files=open_files, pid=12345, name_side_effect=psutil.AccessDenied(pid=12345)) + with patch("psutil.process_iter", return_value=[mock_process]): + jdftx_job.terminate(str(tmp_path)) + mock_terminate_process.assert_not_called() + mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False) + + assert "Exception" in caplog.text + assert "encountered while killing JDFTx" in caplog.text + +#@patch("psutil.Process") +def test_terminate_process(jdftx_job, caplog): + # Test successful termination + mock_process = create_mock_process() + mock_process.terminate.return_value = None # Simulate successful termination + mock_process.wait.return_value = None # Simulate process finished immediately + with caplog.at_level("INFO"): + jdftx_job.terminate_process(mock_process) + + mock_process.terminate.assert_called_once() + mock_process.wait.assert_called_once() + + assert "Process" in caplog.text + assert "terminated gracefully" in caplog.text + + mock_process.reset_mock() + + # Test when process doesn't terminate gracefully + mock_process = create_mock_process(pid=12345, wait_side_effect= psutil.TimeoutExpired(seconds=5)) + mock_process.terminate.return_value = None + + jdftx_job.terminate_process(mock_process) + mock_process.terminate.assert_called_once() + mock_process.kill.assert_called_once() + mock_process.wait.assert_called() + + mock_process.reset_mock() + + # Test when process raises NoSuchProcess + mock_process = create_mock_process(pid=12345, terminate_side_effect=psutil.NoSuchProcess(pid=12345)) + with caplog.at_level("WARNING"): + jdftx_job.terminate_process(mock_process) + + assert "Error while terminating process" in caplog.text + + mock_process.reset_mock() + + # Test when process raises AccessDenied + mock_process = create_mock_process(pid=12345, terminate_side_effect=psutil.AccessDenied(pid=12345)) + + with caplog.at_level("WARNING"): + jdftx_job.terminate_process(mock_process) + + assert "Error while terminating process" in caplog.text From 8fc5b5072d12fd8399c19f18c78cf7c2c485fdd8 Mon Sep 17 00:00:00 2001 From: Sophie Gerits Date: Fri, 11 Oct 2024 18:38:44 -0600 Subject: [PATCH 07/10] changed cmd.split to schlex.split + made pre-commit changes + created conftest.py to store fixtures --- jdftxcov.xml | 64 ++++++++++++ src/custodian/jdftx/jobs.py | 11 +- src/custodian/vasp/jobs.py | 6 +- tests/jdftx/conftest.py | 6 ++ tests/jdftx/test_jobs.py | 185 ++++++++++++++++++++-------------- tests/jdftx/test_jobs_test.py | 165 ------------------------------ 6 files changed, 184 insertions(+), 253 deletions(-) create mode 100644 jdftxcov.xml create mode 100644 tests/jdftx/conftest.py delete mode 100644 tests/jdftx/test_jobs_test.py diff --git a/jdftxcov.xml b/jdftxcov.xml new file mode 100644 index 00000000..b9493f99 --- /dev/null +++ b/jdftxcov.xml @@ -0,0 +1,64 @@ + + + + + + src/custodian/jdftx + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/custodian/jdftx/jobs.py b/src/custodian/jdftx/jobs.py index af359ffd..9b4ad0d2 100644 --- a/src/custodian/jdftx/jobs.py +++ b/src/custodian/jdftx/jobs.py @@ -3,9 +3,9 @@ import logging import os import subprocess +import shlex import psutil - from custodian.custodian import Job logger = logging.getLogger(__name__) @@ -57,7 +57,7 @@ def run(self, directory="./"): ): # use line buffering for stderr return subprocess.run( - cmd.split(), + shlex.split(), cwd=directory, stdout=f_std, stderr=f_err, @@ -70,7 +70,6 @@ def postprocess(self, directory="./") -> None: def terminate(self, directory="./") -> None: """Terminate JDFTx.""" - work_dir = directory logger.info(f"Killing JDFTx processes in {work_dir=}.") for proc in psutil.process_iter(): @@ -94,9 +93,9 @@ def terminate(self, directory="./") -> None: if "jdftx" in cmd: subprocess.run(["killall", f"{cmd}"], check=False) - @staticmethod def terminate_process(proc, timeout=5): + """Terminate a process gracefully, then forcefully if necessary.""" try: proc.terminate() try: @@ -105,10 +104,8 @@ def terminate_process(proc, timeout=5): # If process is still running after the timeout, kill it logger.warning(f"Process {proc.pid} did not terminate gracefully, killing it.") proc.kill() - #proc.wait() + # proc.wait() else: logger.info(f"Process {proc.pid} terminated gracefully.") except (psutil.NoSuchProcess, psutil.AccessDenied) as e: logger.warning(f"Error while terminating process {proc.pid}: {e}") - - diff --git a/src/custodian/vasp/jobs.py b/src/custodian/vasp/jobs.py index b0781515..ff209d2d 100644 --- a/src/custodian/vasp/jobs.py +++ b/src/custodian/vasp/jobs.py @@ -249,11 +249,11 @@ def run(self, directory="./"): cmd[-1] += ".gamma" logger.info(f"Running {' '.join(cmd)}") with ( - open(os.path.join(directory, self.output_file), "w") as f_std, - open(os.path.join(directory, self.stderr_file), "w", buffering=1) as f_err, + open(os.path.join(directory, self.output_file), "w"), + open(os.path.join(directory, self.stderr_file), "w", buffering=1), ): # use line buffering for stderrsubprocess.Popen(cmd, cwd=directory, stdout=f_std, stderr=f_err, start_new_session=True) - return # pylint: disable=R1732 + return # pylint: disable=R1732 def postprocess(self, directory="./") -> None: """ diff --git a/tests/jdftx/conftest.py b/tests/jdftx/conftest.py new file mode 100644 index 00000000..2f382e5b --- /dev/null +++ b/tests/jdftx/conftest.py @@ -0,0 +1,6 @@ +import pytest +from custodian.jdftx.jobs import JDFTxJob + +@pytest.fixture() +def jdftx_job(): + return JDFTxJob(jdftx_cmd="jdftx", output_file="jdftx.out") \ No newline at end of file diff --git a/tests/jdftx/test_jobs.py b/tests/jdftx/test_jobs.py index b622a5cb..9fd88cba 100644 --- a/tests/jdftx/test_jobs.py +++ b/tests/jdftx/test_jobs.py @@ -1,24 +1,29 @@ import os -import pytest from pathlib import Path -from unittest.mock import patch, MagicMock, ANY +from unittest import mock +from unittest.mock import ANY, MagicMock, patch + import psutil +import pytest from custodian.jdftx.jobs import JDFTxJob TEST_DIR = Path(__file__).resolve().parent.parent TEST_FILES = f"{TEST_DIR}/files/jdftx" -@pytest.fixture -def jdftx_job(): - return JDFTxJob(jdftx_cmd="jdftx") +def create_mock_process( + name="jdftx", open_files=None, pid=12345, name_side_effect=None, wait_side_effect=None, terminate_side_effect=None +): + if open_files is None: # Set a default value if not provided + open_files = [MagicMock(path=os.path.join("default_path", "jdftx.out"))] -@pytest.fixture -def mock_process(tmp_path, jdftx_job): - process = MagicMock(spec=psutil.Process) - process.name.return_value = "jdftx" - process.pid = 12345 - process.open_files.return_value = [MagicMock(path=os.path.join(str(tmp_path), jdftx_job.output_file))] - return process + mock_process = mock.Mock(spec=psutil.Process) + mock_process.name.return_value = name + mock_process.open_files.return_value = open_files + mock_process.pid = pid + mock_process.name.side_effect = name_side_effect + mock_process.wait.side_effect = wait_side_effect + mock_process.terminate.side_effect = terminate_side_effect + return mock_process def test_jdftx_job_init(jdftx_job): @@ -27,10 +32,12 @@ def test_jdftx_job_init(jdftx_job): assert jdftx_job.output_file == "jdftx.out" assert jdftx_job.stderr_file == "std_err.txt" + def test_jdftx_job_setup(jdftx_job, tmp_path): jdftx_job.setup(str(tmp_path)) # Setup method doesn't do anything, so just checking that it doesn't raise an exception + def test_jdftx_job_run(jdftx_job, tmp_path): with patch("subprocess.run") as mock_run: mock_process = MagicMock() @@ -48,97 +55,119 @@ def test_jdftx_job_run(jdftx_job, tmp_path): check=False, ) -def test_jdftx_job_run_creates_output_files(jdftx_job, temp_dir): + +def test_jdftx_job_run_creates_output_files(jdftx_job, tmp_path): with patch("subprocess.run"): - jdftx_job.run(str(temp_dir)) + jdftx_job.run(str(tmp_path)) + + assert os.path.exists(os.path.join(str(tmp_path), "jdftx.out")) + assert os.path.exists(os.path.join(str(tmp_path), "std_err.txt")) - assert os.path.exists(os.path.join(str(temp_dir), "jdftx.out")) - assert os.path.exists(os.path.join(str(temp_dir), "std_err.txt")) def test_jdftx_job_postprocess(jdftx_job, tmp_path): jdftx_job.postprocess(str(tmp_path)) # Postprocess method doesn't do anything, so we just check that it doesn't raise an exception -@patch("psutil.process_iter") -@patch("psutil.pid_exists") -@patch("subprocess.run") -@patch.object(JDFTxJob, 'terminate_process', autospec=True) -def test_jdftx_job_terminate(mock_terminate_process, mock_subprocess_run, mock_pid_exists, mock_process_iter, jdftx_job, mock_process, tmp_path): - jdftx_job.output_file = "jdftx.out" # Ensure this matches the actual output file name - run_path = os.path.join(str(tmp_path), jdftx_job.output_file) - mock_process.open_files.return_value = [MagicMock(path=run_path)] - mock_process.name.return_value = "jdftx" - mock_process_iter.return_value = [mock_process] - # Test successful termination - mock_pid_exists.return_value = True - jdftx_job.terminate(str(tmp_path)) - mock_terminate_process.assert_called_once_with(mock_process) - mock_subprocess_run.assert_not_called() +@mock.patch("psutil.pid_exists") +@mock.patch("subprocess.run") +@mock.patch.object(JDFTxJob, "terminate_process", autospec=True) +def test_jdftx_job_terminate(mock_terminate_process, mock_subprocess_run, mock_pid_exists, jdftx_job, tmp_path, caplog): + open_files = [MagicMock(path=os.path.join(str(tmp_path), jdftx_job.output_file))] + # Test when JDFTx process exists + mock_process = create_mock_process(name="jdftx", open_files=open_files, pid=12345) + + with patch("psutil.process_iter", return_value=[mock_process]): + mock_pid_exists.return_value = True + jdftx_job.terminate(str(tmp_path)) + mock_terminate_process.assert_called_once_with(mock_process) + mock_subprocess_run.assert_not_called() mock_terminate_process.reset_mock() mock_subprocess_run.reset_mock() - # Test when process doesn't exist - mock_process.name.return_value = "not_jdft" - jdftx_job.terminate(str(tmp_path)) - mock_terminate_process.assert_not_called() - mock_subprocess_run.assert_called_once_with(["killall", "jdftx"], check=False) + # Test when no JDFTx process exists + mock_process = create_mock_process(name="vasp", open_files=open_files, pid=12345) + + with patch("psutil.process_iter", return_value=[mock_process]): + jdftx_job.terminate(str(tmp_path)) + mock_terminate_process.assert_not_called() + mock_subprocess_run.assert_called_once_with(["killall", "jdftx"], check=False) mock_terminate_process.reset_mock() mock_subprocess_run.reset_mock() - # Test when psutil raises exceptions - mock_pid_exists.return_value = "jdftx" - mock_process_iter.side_effect = psutil.NoSuchProcess(pid=12345) - jdftx_job.terminate(str(tmp_path)) - mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False) + # Test when psutil.process_iter raises NoSuchProcess + mock_process = create_mock_process( + name="jdftx", open_files=open_files, pid=12345, name_side_effect=psutil.NoSuchProcess(pid=12345) + ) + + with caplog.at_level("WARNING"): + with patch("psutil.process_iter", return_value=[mock_process]): + jdftx_job.terminate(str(tmp_path)) + mock_terminate_process.assert_not_called() + mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False) + + assert "Exception" in caplog.text + assert "encountered while killing JDFTx" in caplog.text mock_terminate_process.reset_mock() mock_subprocess_run.reset_mock() - mock_pid_exists.side_effect = None - mock_process_iter.side_effect = None - - #mock_pid_exists.return_value = True - mock_process_iter.side_effect = psutil.AccessDenied(pid=12345) - jdftx_job.terminate(str(tmp_path)) - mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False) - -@patch("psutil.Process") -def test_terminate_process(mock_process, jdftx_job): - process = mock_process.return_value - + + # Test when psutil.process_iter raises AccessDenied + with caplog.at_level("WARNING"): + mock_process = create_mock_process( + name="jdftx", open_files=open_files, pid=12345, name_side_effect=psutil.AccessDenied(pid=12345) + ) + with patch("psutil.process_iter", return_value=[mock_process]): + jdftx_job.terminate(str(tmp_path)) + mock_terminate_process.assert_not_called() + mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False) + + assert "Exception" in caplog.text + assert "encountered while killing JDFTx" in caplog.text + + +def test_terminate_process(jdftx_job, caplog): # Test successful termination - jdftx_job.terminate_process(process) - process.terminate.assert_called_once() - process.wait.assert_called_once_with(timeout=5) + mock_process = create_mock_process() + mock_process.terminate.return_value = None # Simulate successful termination + mock_process.wait.return_value = None # Simulate process finished immediately + with caplog.at_level("INFO"): + jdftx_job.terminate_process(mock_process) + + mock_process.terminate.assert_called_once() + mock_process.wait.assert_called_once() + + assert "Process" in caplog.text + assert "terminated gracefully" in caplog.text - process.reset_mock() + mock_process.reset_mock() # Test when process doesn't terminate gracefully - process.wait.side_effect = [psutil.TimeoutExpired(5), None] - jdftx_job.terminate_process(process) - process.kill.assert_called_once() + mock_process = create_mock_process(pid=12345, wait_side_effect=psutil.TimeoutExpired(seconds=5)) + mock_process.terminate.return_value = None + + jdftx_job.terminate_process(mock_process) + mock_process.terminate.assert_called_once() + mock_process.kill.assert_called_once() + mock_process.wait.assert_called() + + mock_process.reset_mock() # Test when process raises NoSuchProcess - process.terminate.side_effect = psutil.NoSuchProcess(pid=12345) - jdftx_job.terminate_process(process) + mock_process = create_mock_process(pid=12345, terminate_side_effect=psutil.NoSuchProcess(pid=12345)) + with caplog.at_level("WARNING"): + jdftx_job.terminate_process(mock_process) + + assert "Error while terminating process" in caplog.text - process.reset_mock() + mock_process.reset_mock() # Test when process raises AccessDenied - process.terminate.side_effect = psutil.AccessDenied(pid=12345) - jdftx_job.terminate_process(process) - -def test_jdftx_job_with_custom_parameters(): - custom_job = JDFTxJob( - jdftx_cmd="custom_jdftx", - input_file="custom.in", - output_file="custom.out", - stderr_file="custom_err.txt" - ) - - assert custom_job.jdftx_cmd == "custom_jdftx" - assert custom_job.input_file == "custom.in" - assert custom_job.output_file == "custom.out" - assert custom_job.stderr_file == "custom_err.txt" + mock_process = create_mock_process(pid=12345, terminate_side_effect=psutil.AccessDenied(pid=12345)) + + with caplog.at_level("WARNING"): + jdftx_job.terminate_process(mock_process) + + assert "Error while terminating process" in caplog.text diff --git a/tests/jdftx/test_jobs_test.py b/tests/jdftx/test_jobs_test.py deleted file mode 100644 index 038cdb16..00000000 --- a/tests/jdftx/test_jobs_test.py +++ /dev/null @@ -1,165 +0,0 @@ -import os -import pytest -from pathlib import Path -import psutil -from custodian.jdftx.jobs import JDFTxJob -from unittest import mock -from unittest.mock import patch, MagicMock, ANY - -TEST_DIR = Path(__file__).resolve().parent.parent -TEST_FILES = f"{TEST_DIR}/files/jdftx" - -@pytest.fixture -def jdftx_job(): - return JDFTxJob(jdftx_cmd="jdftx", output_file="jdftx.out") - -def create_mock_process(name="jdftx", open_files=None, pid=12345, name_side_effect=None, wait_side_effect=None, terminate_side_effect=None): - if open_files is None: # Set a default value if not provided - open_files = [MagicMock(path=os.path.join("default_path", "jdftx.out"))] - - mock_process = mock.Mock(spec=psutil.Process) - mock_process.name.return_value = name - mock_process.open_files.return_value = open_files - mock_process.pid = pid - mock_process.name.side_effect = name_side_effect - mock_process.wait.side_effect = wait_side_effect - mock_process.terminate.side_effect = terminate_side_effect - return mock_process - -def test_jdftx_job_init(jdftx_job): - assert jdftx_job.jdftx_cmd == "jdftx" - assert jdftx_job.input_file == "init.in" - assert jdftx_job.output_file == "jdftx.out" - assert jdftx_job.stderr_file == "std_err.txt" - -def test_jdftx_job_setup(jdftx_job, tmp_path): - jdftx_job.setup(str(tmp_path)) - # Setup method doesn't do anything, so just checking that it doesn't raise an exception - -def test_jdftx_job_run(jdftx_job, tmp_path): - with patch("subprocess.run") as mock_run: - mock_process = MagicMock() - mock_run.return_value = mock_process - - result = jdftx_job.run(str(tmp_path)) - - assert result == mock_process - mock_run.assert_called_once_with( - ["jdftx", "-i", "init.in", "-o", "jdftx.out"], - cwd=str(tmp_path), - stdout=ANY, - stderr=ANY, - shell=False, - check=False, - ) - -def test_jdftx_job_run_creates_output_files(jdftx_job, tmp_path): - with patch("subprocess.run"): - jdftx_job.run(str(tmp_path)) - - assert os.path.exists(os.path.join(str(tmp_path), "jdftx.out")) - assert os.path.exists(os.path.join(str(tmp_path), "std_err.txt")) - -def test_jdftx_job_postprocess(jdftx_job, tmp_path): - jdftx_job.postprocess(str(tmp_path)) - # Postprocess method doesn't do anything, so we just check that it doesn't raise an exception - - -@mock.patch("psutil.pid_exists") -@mock.patch("subprocess.run") -@mock.patch.object(JDFTxJob, 'terminate_process', autospec=True) -def test_jdftx_job_terminate(mock_terminate_process, mock_subprocess_run, mock_pid_exists, jdftx_job, tmp_path, caplog): - open_files=[MagicMock(path=os.path.join(str(tmp_path), jdftx_job.output_file))] - # Test when JDFTx process exists - mock_process = create_mock_process(name="jdftx", open_files=open_files, pid=12345) - - with patch("psutil.process_iter", return_value=[mock_process]): - mock_pid_exists.return_value = True - jdftx_job.terminate(str(tmp_path)) - mock_terminate_process.assert_called_once_with(mock_process) - mock_subprocess_run.assert_not_called() - - mock_terminate_process.reset_mock() - mock_subprocess_run.reset_mock() - - # Test when no JDFTx process exists - mock_process = create_mock_process(name="vasp", open_files=open_files, pid=12345) - - with patch("psutil.process_iter", return_value=[mock_process]): - jdftx_job.terminate(str(tmp_path)) - mock_terminate_process.assert_not_called() - mock_subprocess_run.assert_called_once_with(["killall", "jdftx"], check=False) - - mock_terminate_process.reset_mock() - mock_subprocess_run.reset_mock() - - # Test when psutil.process_iter raises NoSuchProcess - mock_process = create_mock_process(name="jdftx", open_files=open_files, pid=12345, name_side_effect=psutil.NoSuchProcess(pid=12345)) - - with caplog.at_level("WARNING"): - with patch("psutil.process_iter", return_value=[mock_process]): - jdftx_job.terminate(str(tmp_path)) - mock_terminate_process.assert_not_called() - mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False) - - assert "Exception" in caplog.text - assert "encountered while killing JDFTx" in caplog.text - - mock_terminate_process.reset_mock() - mock_subprocess_run.reset_mock() - - # Test when psutil.process_iter raises AccessDenied - with caplog.at_level("WARNING"): - mock_process = create_mock_process(name="jdftx", open_files=open_files, pid=12345, name_side_effect=psutil.AccessDenied(pid=12345)) - with patch("psutil.process_iter", return_value=[mock_process]): - jdftx_job.terminate(str(tmp_path)) - mock_terminate_process.assert_not_called() - mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False) - - assert "Exception" in caplog.text - assert "encountered while killing JDFTx" in caplog.text - -#@patch("psutil.Process") -def test_terminate_process(jdftx_job, caplog): - # Test successful termination - mock_process = create_mock_process() - mock_process.terminate.return_value = None # Simulate successful termination - mock_process.wait.return_value = None # Simulate process finished immediately - with caplog.at_level("INFO"): - jdftx_job.terminate_process(mock_process) - - mock_process.terminate.assert_called_once() - mock_process.wait.assert_called_once() - - assert "Process" in caplog.text - assert "terminated gracefully" in caplog.text - - mock_process.reset_mock() - - # Test when process doesn't terminate gracefully - mock_process = create_mock_process(pid=12345, wait_side_effect= psutil.TimeoutExpired(seconds=5)) - mock_process.terminate.return_value = None - - jdftx_job.terminate_process(mock_process) - mock_process.terminate.assert_called_once() - mock_process.kill.assert_called_once() - mock_process.wait.assert_called() - - mock_process.reset_mock() - - # Test when process raises NoSuchProcess - mock_process = create_mock_process(pid=12345, terminate_side_effect=psutil.NoSuchProcess(pid=12345)) - with caplog.at_level("WARNING"): - jdftx_job.terminate_process(mock_process) - - assert "Error while terminating process" in caplog.text - - mock_process.reset_mock() - - # Test when process raises AccessDenied - mock_process = create_mock_process(pid=12345, terminate_side_effect=psutil.AccessDenied(pid=12345)) - - with caplog.at_level("WARNING"): - jdftx_job.terminate_process(mock_process) - - assert "Error while terminating process" in caplog.text From 7775f1d329a2f559299663ca488db5f7635669d7 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 12 Oct 2024 00:42:54 +0000 Subject: [PATCH 08/10] pre-commit auto-fixes --- src/custodian/jdftx/jobs.py | 3 ++- tests/jdftx/conftest.py | 3 ++- tests/jdftx/test_jobs.py | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/custodian/jdftx/jobs.py b/src/custodian/jdftx/jobs.py index 2359ea3f..fe9e19bf 100644 --- a/src/custodian/jdftx/jobs.py +++ b/src/custodian/jdftx/jobs.py @@ -2,10 +2,11 @@ import logging import os -import subprocess import shlex +import subprocess import psutil + from custodian.custodian import Job logger = logging.getLogger(__name__) diff --git a/tests/jdftx/conftest.py b/tests/jdftx/conftest.py index 2f382e5b..475dcdeb 100644 --- a/tests/jdftx/conftest.py +++ b/tests/jdftx/conftest.py @@ -1,6 +1,7 @@ import pytest from custodian.jdftx.jobs import JDFTxJob + @pytest.fixture() def jdftx_job(): - return JDFTxJob(jdftx_cmd="jdftx", output_file="jdftx.out") \ No newline at end of file + return JDFTxJob(jdftx_cmd="jdftx", output_file="jdftx.out") diff --git a/tests/jdftx/test_jobs.py b/tests/jdftx/test_jobs.py index 9fd88cba..92ecc4d4 100644 --- a/tests/jdftx/test_jobs.py +++ b/tests/jdftx/test_jobs.py @@ -4,12 +4,12 @@ from unittest.mock import ANY, MagicMock, patch import psutil -import pytest from custodian.jdftx.jobs import JDFTxJob TEST_DIR = Path(__file__).resolve().parent.parent TEST_FILES = f"{TEST_DIR}/files/jdftx" + def create_mock_process( name="jdftx", open_files=None, pid=12345, name_side_effect=None, wait_side_effect=None, terminate_side_effect=None ): From b69cacb09045392e8978513a345222ae223c4ff4 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 16 Oct 2024 21:16:21 +0000 Subject: [PATCH 09/10] pre-commit auto-fixes --- tests/jdftx/conftest.py | 3 ++- tests/jdftx/test_jobs.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/jdftx/conftest.py b/tests/jdftx/conftest.py index 475dcdeb..8c415f72 100644 --- a/tests/jdftx/conftest.py +++ b/tests/jdftx/conftest.py @@ -1,7 +1,8 @@ import pytest + from custodian.jdftx.jobs import JDFTxJob -@pytest.fixture() +@pytest.fixture def jdftx_job(): return JDFTxJob(jdftx_cmd="jdftx", output_file="jdftx.out") diff --git a/tests/jdftx/test_jobs.py b/tests/jdftx/test_jobs.py index 92ecc4d4..7c4b5481 100644 --- a/tests/jdftx/test_jobs.py +++ b/tests/jdftx/test_jobs.py @@ -4,6 +4,7 @@ from unittest.mock import ANY, MagicMock, patch import psutil + from custodian.jdftx.jobs import JDFTxJob TEST_DIR = Path(__file__).resolve().parent.parent From 2628e942af09744cb59636ebd4751c2901972925 Mon Sep 17 00:00:00 2001 From: Sophie Gerits Date: Wed, 16 Oct 2024 15:18:20 -0600 Subject: [PATCH 10/10] undo accidental chage to vasp file + added string as argument into schlex.split() --- jdftxcov.xml | 64 ------------------------------------- src/custodian/jdftx/jobs.py | 2 +- src/custodian/vasp/jobs.py | 9 +++--- 3 files changed, 6 insertions(+), 69 deletions(-) delete mode 100644 jdftxcov.xml diff --git a/jdftxcov.xml b/jdftxcov.xml deleted file mode 100644 index b9493f99..00000000 --- a/jdftxcov.xml +++ /dev/null @@ -1,64 +0,0 @@ - - - - - - src/custodian/jdftx - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/src/custodian/jdftx/jobs.py b/src/custodian/jdftx/jobs.py index 9b4ad0d2..db2d432b 100644 --- a/src/custodian/jdftx/jobs.py +++ b/src/custodian/jdftx/jobs.py @@ -57,7 +57,7 @@ def run(self, directory="./"): ): # use line buffering for stderr return subprocess.run( - shlex.split(), + shlex.split(cmd), cwd=directory, stdout=f_std, stderr=f_err, diff --git a/src/custodian/vasp/jobs.py b/src/custodian/vasp/jobs.py index ff209d2d..314ea4a2 100644 --- a/src/custodian/vasp/jobs.py +++ b/src/custodian/vasp/jobs.py @@ -249,11 +249,12 @@ def run(self, directory="./"): cmd[-1] += ".gamma" logger.info(f"Running {' '.join(cmd)}") with ( - open(os.path.join(directory, self.output_file), "w"), - open(os.path.join(directory, self.stderr_file), "w", buffering=1), + open(os.path.join(directory, self.output_file), "w") as f_std, + open(os.path.join(directory, self.stderr_file), "w", buffering=1) as f_err, ): - # use line buffering for stderrsubprocess.Popen(cmd, cwd=directory, stdout=f_std, stderr=f_err, start_new_session=True) - return # pylint: disable=R1732 + # use line buffering for stderr + return subprocess.Popen(cmd, cwd=directory, stdout=f_std, stderr=f_err, start_new_session=True) + # pylint: disable=R1732 def postprocess(self, directory="./") -> None: """