Repository: Quantum-Accelerators/quacc
Pull request #1850 (2024-03-06 18:02:39)

Don't call a subprocess with VASP custodian
## Summary of Changes

Call Custodian directly rather than via a subprocess for VASP.

FYI: @zulissimeta, I had to revert this PR due to issues with concurrent calculations in certain workflow engines. Once we solve the upstream issue in Custodian, I can re-merge this PR.

Requires:
- https://github.com/Quantum-Accelerators/quacc/issues/1821
- Custodian > 2024.2.15

### Checklist

- [X] I have read the ["Guidelines" section](https://quantum-accelerators.github.io/quacc/dev/contributing.html#guidelines) of the contributing guide. Don't lie! 😉
- [X] My PR is on a custom branch and is _not_ named `main`.
- [X] I have added relevant, comprehensive [unit tests](https://quantum-accelerators.github.io/quacc/dev/contributing.html#unit-tests).

### Notes

- Your PR will likely not be merged without proper and thorough tests.
- If you are an external contributor, you will see a comment from [@buildbot-princeton](https://github.com/buildbot-princeton). This is solely for the maintainers.
- When your code is ready for review, ping one of the [active maintainers](https://quantum-accelerators.github.io/quacc/about/contributors.html#active-maintainers).


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1850?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators) Report
Attention: Patch coverage is `81.81818%` with `2 lines` in your changes are missing coverage. Please review.
> Project coverage is 99.33%. Comparing base [(`fe3a378`)](https://app.codecov.io/gh/Quantum-Accelerators/quacc/commit/fe3a378da6e8973726976f4d66e23000cad7a24b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators) to head [(`51a45e5`)](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1850?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators).

| [Files](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1850?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators) | Patch % | Lines |
|---|---|---|
| [src/quacc/calculators/vasp/vasp.py](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1850?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators#diff-c3JjL3F1YWNjL2NhbGN1bGF0b3JzL3Zhc3AvdmFzcC5weQ==) | 81.81% | [2 Missing :warning: ](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1850?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1850      +/-   ##
==========================================
- Coverage   99.40%   99.33%   -0.07%     
==========================================
  Files          81       81              
  Lines        3171     3176       +5     
==========================================
+ Hits         3152     3155       +3     
- Misses         19       21       +2     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/Quantum-Accelerators/quacc/pull/1850?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Quantum-Accelerators).


Patch:
diff --git a/src/quacc/calculators/qchem/qchem.py b/src/quacc/calculators/qchem/qchem.py
index 478a01af..b89daec5 100644
--- a/src/quacc/calculators/qchem/qchem.py
+++ b/src/quacc/calculators/qchem/qchem.py
@@ -2,14 +2,10 @@
 
 from __future__ import annotations
 
-import inspect
-import sys
-from pathlib import Path
 from typing import TYPE_CHECKING
 
 from ase.calculators.calculator import FileIOCalculator
 
-from quacc.calculators.qchem import qchem_custodian
 from quacc.calculators.qchem.io import read_qchem, write_qchem
 from quacc.calculators.qchem.params import cleanup_attrs, make_qc_input
 from quacc.calculators.qchem.qchem_custodian import run_custodian
@@ -242,14 +238,11 @@ class QChem(FileIOCalculator):
         # Set default params
         self._set_default_params()
 
-        # Get Q-Chem executable command
-        command = self._manage_environment()
-
         # Instantiate the calculator
         super().__init__(
             restart=None,
             label=None,
-            command=command,
+            command="",
             atoms=self.atoms,
             profile=None,
             **self.fileiocalculator_kwargs,
@@ -287,8 +280,7 @@ class QChem(FileIOCalculator):
             prev_orbital_coeffs=self.prev_orbital_coeffs,
         )
 
-    @staticmethod
-    def execute() -> int:
+    def execute(self) -> int:
         """
         Execute Q-Chem.
 
@@ -298,7 +290,7 @@ class QChem(FileIOCalculator):
             The return code.
         """
 
-        run_custodian()
+        run_custodian(directory=self.directory)
         return 0
 
     def read_results(self) -> None:
@@ -314,19 +306,6 @@ class QChem(FileIOCalculator):
         self.results = results
         self.prev_orbital_coeffs = prev_orbital_coeffs
 
-    @staticmethod
-    def _manage_environment() -> str:
-        """
-        Return the command to run the Q-Chem calculator via Custodian.
-        Returns
-        -------
-        str
-            The command flag to run Q-Chem with Custodian.
-        """
-
-        qchem_custodian_script = Path(inspect.getfile(qchem_custodian)).resolve()
-        return f"{sys.executable} {qchem_custodian_script}"
-
     def _set_default_params(self) -> None:
         """
         Store the parameters that have been passed to the Q-Chem calculator in
diff --git a/src/quacc/calculators/qchem/qchem_custodian.py b/src/quacc/calculators/qchem/qchem_custodian.py
index ad7aec39..179e3756 100644
--- a/src/quacc/calculators/qchem/qchem_custodian.py
+++ b/src/quacc/calculators/qchem/qchem_custodian.py
@@ -25,6 +25,7 @@ def run_custodian(
     qchem_use_error_handlers: bool | None = None,
     qchem_custodian_max_errors: int | None = None,
     qchem_nbo_exe: str | Path | None = None,
+    directory: str | Path = "./",
 ) -> Popen:
     """
     Function to run QChem Custodian.
@@ -45,6 +46,8 @@ def run_custodian(
         in settings.
     qchem_nbo_exe
         The full path to the NBO executable.
+    directory
+        The runtime directory.
 
     Returns
     -------
@@ -94,6 +97,7 @@ def run_custodian(
         jobs,
         max_errors=qchem_custodian_max_errors,
         terminate_on_nonzero_returncode=False,
+        directory=directory,
     )
 
     return c.run()
diff --git a/src/quacc/calculators/vasp/vasp.py b/src/quacc/calculators/vasp/vasp.py
index 29eb4bb4..bc8bf846 100644
--- a/src/quacc/calculators/vasp/vasp.py
+++ b/src/quacc/calculators/vasp/vasp.py
@@ -2,9 +2,8 @@
 
 from __future__ import annotations
 
-import inspect
 import os
-import sys
+import subprocess
 from pathlib import Path
 from typing import TYPE_CHECKING
 
@@ -13,7 +12,6 @@ from ase.calculators.vasp import Vasp as Vasp_
 from ase.calculators.vasp import setups as ase_setups
 from ase.constraints import FixAtoms
 
-from quacc.calculators.vasp import vasp_custodian
 from quacc.calculators.vasp.io import load_vasp_yaml_calc
 from quacc.calculators.vasp.params import (
     get_param_swaps,
@@ -23,6 +21,7 @@ from quacc.calculators.vasp.params import (
     set_auto_dipole,
     set_pmg_kpts,
 )
+from quacc.calculators.vasp.vasp_custodian import run_custodian
 from quacc.schemas.prep import set_magmoms
 from quacc.utils.dicts import sort_dict
 
@@ -181,10 +180,6 @@ class Vasp(Vasp_):
                 "VASP_VDW setting was not provided, yet you requested a vdW functional."
             )
 
-        if self.use_custodian:
-            run_vasp_custodian_file = Path(inspect.getfile(vasp_custodian)).resolve()
-            return f"{sys.executable} {run_vasp_custodian_file}"
-
         # Return vanilla ASE command
         vasp_cmd = (
             SETTINGS.VASP_GAMMA_CMD
@@ -290,3 +285,39 @@ class Vasp(Vasp_):
         self.user_calc_params = sort_dict(
             normalize_params(remove_unused_flags(self.user_calc_params))
         )
+
+    def _run(
+        self,
+        command: list[str] | None = None,
+        out: Path | str | None = None,
+        directory: Path | str | None = None,
+    ) -> int:
+        """
+        Override the Vasp calculator's run method to use Custodian if necessary.
+
+        Parameters
+        ----------
+        command
+            The command to run the VASP calculation. If None, will use the
+            self.command attribute.
+        out
+            The stdout file path.
+        directory
+            The directory to run the calculation in. If None, will use the
+            self.directory attribute.
+
+        Returns
+        -------
+        int
+            The return code.
+        """
+        if command is None:
+            command = self.command
+        if directory is None:
+            directory = self.directory
+
+        if self.use_custodian:
+            run_custodian(directory=directory)
+            return 0
+        else:
+            return subprocess.call(command, shell=True, stdout=out, cwd=directory)
diff --git a/src/quacc/calculators/vasp/vasp_custodian.py b/src/quacc/calculators/vasp/vasp_custodian.py
index 277ecae0..5697a4d8 100644
--- a/src/quacc/calculators/vasp/vasp_custodian.py
+++ b/src/quacc/calculators/vasp/vasp_custodian.py
@@ -68,6 +68,7 @@ def run_custodian(
     vasp_custodian_handlers: list[str] | None = None,
     vasp_custodian_validators: list[str] | None = None,
     scratch_dir: str | None = None,
+    directory: str | None = "./",
     vasp_job_kwargs: VaspJobKwargs | None = None,
     custodian_kwargs: CustodianKwargs | None = None,
 ) -> list[list[dict]]:
@@ -202,6 +203,7 @@ def run_custodian(
         validators=validators,
         max_errors=vasp_custodian_max_errors,
         scratch_dir=scratch_dir,
+        directory=directory,
         **custodian_kwargs,
     )
 


Test patch:
diff --git a/tests/core/calculators/vasp/test_vasp.py b/tests/core/calculators/vasp/test_vasp.py
index 2f63b84a..9df8487f 100644
--- a/tests/core/calculators/vasp/test_vasp.py
+++ b/tests/core/calculators/vasp/test_vasp.py
@@ -903,3 +903,8 @@ def test_run(monkeypatch, tmp_path):
     atoms = bulk("Cu")
     calc = Vasp(atoms, xc="PBE", use_custodian=False)
     assert calc._run() > 0
+
+    atoms = bulk("Cu")
+    calc = Vasp(atoms, xc="PBE", use_custodian=True)
+    with pytest.raises(FileNotFoundError):
+        calc._run()