From ef1f0b73ebb8d6d7fc04a2f1cf76c3279c700bf9 Mon Sep 17 00:00:00 2001 From: "Andrew S. Rosen" Date: Wed, 25 Oct 2023 12:06:12 -0700 Subject: [PATCH] Clearer error handling (#1104) Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com> --- CHANGELOG.md | 6 ++++++ src/quacc/calculators/vasp.py | 2 +- src/quacc/recipes/dftb/core.py | 8 ++++---- src/quacc/recipes/gulp/core.py | 2 +- src/quacc/runners/calc.py | 20 ++++++++++++++++--- src/quacc/schemas/ase.py | 4 ++-- src/quacc/schemas/cclib.py | 6 +++--- src/quacc/schemas/vasp.py | 6 ++++-- src/quacc/utils/files.py | 2 +- tests/core/calculators/vasp/test_vasp.py | 4 ++-- .../recipes/dftb_recipes/test_dftb_recipes.py | 15 ++++++++++++-- .../test_failed_orca_recipes.py | 2 +- .../qchem_recipes/test_qchem_recipes.py | 2 +- tests/core/schemas/test_ase.py | 2 +- tests/core/schemas/test_cclib_schema.py | 2 +- tests/core/schemas/test_vasp_pop_analysis.py | 2 +- 16 files changed, 59 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2469eaba3b..5759c55ece 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.3.11] + +## Changed + +- Improved error handling + ## [0.3.10] ### Changed diff --git a/src/quacc/calculators/vasp.py b/src/quacc/calculators/vasp.py index 28dfd039c7..fe69c61904 100644 --- a/src/quacc/calculators/vasp.py +++ b/src/quacc/calculators/vasp.py @@ -251,7 +251,7 @@ def _manage_environment(self) -> str: if SETTINGS.VASP_VDW: os.environ["ASE_VASP_VDW"] = str(SETTINGS.VASP_VDW) if self.user_calc_params.get("luse_vdw") and "ASE_VASP_VDW" not in os.environ: - raise EnvironmentError( + raise OSError( "VASP_VDW setting was not provided, yet you requested a vdW functional." ) diff --git a/src/quacc/recipes/dftb/core.py b/src/quacc/recipes/dftb/core.py index 6ce96909fd..46aecbdb74 100644 --- a/src/quacc/recipes/dftb/core.py +++ b/src/quacc/recipes/dftb/core.py @@ -195,13 +195,13 @@ def _base_job( if SETTINGS.CHECK_CONVERGENCE: if check_logfile(LOG_FILE, "SCC is NOT converged"): - msg = "SCC is not converged" - raise ValueError(msg) + msg = f"SCC is not converged in {LOG_FILE}" + raise RuntimeError(msg) if flags.get("Driver_") == "GeometryOptimization" and not check_logfile( LOG_FILE, "Geometry converged" ): - msg = "Geometry did not converge" - raise ValueError(msg) + msg = f"Geometry optimization did not complete in {LOG_FILE}" + raise RuntimeError(msg) return summarize_run( final_atoms, diff --git a/src/quacc/recipes/gulp/core.py b/src/quacc/recipes/gulp/core.py index 28fbf29984..fca32c9eb3 100644 --- a/src/quacc/recipes/gulp/core.py +++ b/src/quacc/recipes/gulp/core.py @@ -248,7 +248,7 @@ def _base_job( and not final_atoms.calc.get_opt_state() ): msg = "Optimization did not converge." - raise ValueError(msg) + raise RuntimeError(msg) return summarize_run( final_atoms, diff --git a/src/quacc/runners/calc.py b/src/quacc/runners/calc.py index ae4bd8cfdd..6419543321 100644 --- a/src/quacc/runners/calc.py +++ b/src/quacc/runners/calc.py @@ -66,7 +66,11 @@ def run_calc( atoms, tmpdir, job_results_dir = _calc_setup(atoms, copy_files=copy_files) # Run calculation via get_potential_energy() - atoms.get_potential_energy() + try: + atoms.get_potential_energy() + except Exception: + msg = f"Calculation failed. Check the logfiles at {Path.cwd()}" + raise RuntimeError(msg) # Most ASE calculators do not update the atoms object in-place with a call # to .get_potential_energy(), which is important if an internal optimizer is @@ -168,7 +172,11 @@ def run_ase_opt( # Run calculation with traj, optimizer(atoms, **optimizer_kwargs) as dyn: - dyn.run(fmax=fmax, steps=max_steps, **run_kwargs) + try: + dyn.run(fmax=fmax, steps=max_steps, **run_kwargs) + except Exception: + msg = f"Calculation failed. Check the logfiles at {Path.cwd()}" + raise RuntimeError(msg) # Store the trajectory atoms dyn.traj_atoms = read(traj_filename, index=":") @@ -214,7 +222,13 @@ def run_ase_vib( # Run calculation vib = Vibrations(atoms, name=str(tmpdir / "vib"), **vib_kwargs) - vib.run() + try: + vib.run() + except Exception: + msg = f"Calculation failed. Check the logfiles at {Path.cwd()}" + raise RuntimeError(msg) + + # Summarize run vib.summary(log=str(tmpdir / "vib_summary.log")) # Perform cleanup operations diff --git a/src/quacc/schemas/ase.py b/src/quacc/schemas/ase.py index 6edc29f4f0..7687a5c2df 100644 --- a/src/quacc/schemas/ase.py +++ b/src/quacc/schemas/ase.py @@ -163,8 +163,8 @@ def summarize_opt_run( # Check convergence is_converged = dyn.converged() if check_convergence and not is_converged: - msg = "Optimization did not converge." - raise ValueError(msg) + msg = f"Optimization did not converge. Refer to {Path.cwd()}" + raise RuntimeError(msg) # Get trajectory if not trajectory: diff --git a/src/quacc/schemas/cclib.py b/src/quacc/schemas/cclib.py index 83aeee5fbf..d2f36e4e5c 100644 --- a/src/quacc/schemas/cclib.py +++ b/src/quacc/schemas/cclib.py @@ -111,8 +111,8 @@ def cclib_summarize_run( metadata = attributes["metadata"] if check_convergence and attributes.get("optdone") is False: - msg = "Optimization not complete." - raise ValueError(msg) + msg = f"Optimization not complete. Refer to {dir_path}" + raise RuntimeError(msg) # Now we construct the input Atoms object. Note that this is not necessarily # the same as the initial Atoms from the relaxation because the DFT @@ -316,7 +316,7 @@ def _cclib_calculate( if not proatom_dir: if os.getenv("PROATOM_DIR") is None: msg = "PROATOM_DIR environment variable or proatom_dir kwarg needs to be set." - raise ValueError(msg) + raise OSError(msg) proatom_dir = os.path.expandvars(os.environ["PROATOM_DIR"]) if not Path(proatom_dir).exists(): msg = f"Protatom directory {proatom_dir} does not exist. Returning None." diff --git a/src/quacc/schemas/vasp.py b/src/quacc/schemas/vasp.py index 392d5058f7..ced428bb88 100644 --- a/src/quacc/schemas/vasp.py +++ b/src/quacc/schemas/vasp.py @@ -85,7 +85,9 @@ def vasp_summarize_run( # Check for calculation convergence if check_convergence and vasp_task_doc["state"] != "successful": - raise ValueError("VASP calculation did not converge. Will not store task data.") + raise RuntimeError( + f"VASP calculation did not converge. Will not store task data. Refer to {dir_path}" + ) base_task_doc = summarize_run(atoms, prep_next_run=prep_next_run, store=False) @@ -229,7 +231,7 @@ def _chargemol_runner( # Check environment variable if atomic_densities_path is None and "DDEC6_ATOMIC_DENSITIES_DIR" not in os.environ: msg = "DDEC6_ATOMIC_DENSITIES_DIR environment variable not defined." - raise EnvironmentError(msg) + raise OSError(msg) # Run Chargemol analysis chargemol_stats = ChargemolAnalysis( diff --git a/src/quacc/utils/files.py b/src/quacc/utils/files.py index 754e57e00e..31aa2779ad 100644 --- a/src/quacc/utils/files.py +++ b/src/quacc/utils/files.py @@ -112,7 +112,7 @@ def load_yaml_calc(yaml_path: str | Path) -> dict: if not yaml_path.exists(): msg = f"Cannot find {yaml_path}" - raise ValueError(msg) + raise FileNotFoundError(msg) # Load YAML file with yaml_path.open() as stream: diff --git a/tests/core/calculators/vasp/test_vasp.py b/tests/core/calculators/vasp/test_vasp.py index 2ae3790018..d159e2fa72 100644 --- a/tests/core/calculators/vasp/test_vasp.py +++ b/tests/core/calculators/vasp/test_vasp.py @@ -409,7 +409,7 @@ def test_lasph(): def test_vdw(): atoms = bulk("Cu") - with pytest.raises(EnvironmentError): + with pytest.raises(OSError): Vasp(atoms, xc="beef-vdw") @@ -736,5 +736,5 @@ def test_bad(): with pytest.raises(ValueError): Vasp(atoms, auto_kpts={"test": [100]}) - with pytest.raises(ValueError): + with pytest.raises(FileNotFoundError): Vasp(atoms, preset="BadRelaxSet") diff --git a/tests/core/recipes/dftb_recipes/test_dftb_recipes.py b/tests/core/recipes/dftb_recipes/test_dftb_recipes.py index 9d4971fd34..ff67cbafb1 100644 --- a/tests/core/recipes/dftb_recipes/test_dftb_recipes.py +++ b/tests/core/recipes/dftb_recipes/test_dftb_recipes.py @@ -78,7 +78,7 @@ def test_static_job_cu_kpts(tmpdir): def test_static_errors(tmpdir): tmpdir.chdir() - with pytest.raises(ValueError): + with pytest.raises(RuntimeError): atoms = molecule("H2O") static_job(atoms, calc_swaps={"Hamiltonian_MaxSccIterations": 1}) @@ -153,7 +153,7 @@ def test_relax_job_cu_supercell_cell_relax(tmpdir): def test_relax_job_cu_supercell_errors(tmpdir): tmpdir.chdir() - with pytest.raises(ValueError): + with pytest.raises(RuntimeError): atoms = bulk("Cu") * (2, 1, 1) atoms[0].position += 0.5 relax_job( @@ -161,3 +161,14 @@ def test_relax_job_cu_supercell_errors(tmpdir): kpts=(3, 3, 3), calc_swaps={"MaxSteps": 1, "Hamiltonian_MaxSccIterations": 100}, ) + + +def test_child_errors(tmpdir): + tmpdir.chdir() + with pytest.raises(RuntimeError): + atoms = bulk("Cu") + static_job(atoms) + + with pytest.raises(RuntimeError): + atoms = bulk("Cu") + relax_job(atoms) diff --git a/tests/core/recipes/orca_recipes/test_orca_fails/test_failed_orca_recipes.py b/tests/core/recipes/orca_recipes/test_orca_fails/test_failed_orca_recipes.py index dd70d2d8cb..77a7639b14 100644 --- a/tests/core/recipes/orca_recipes/test_orca_fails/test_failed_orca_recipes.py +++ b/tests/core/recipes/orca_recipes/test_orca_fails/test_failed_orca_recipes.py @@ -8,5 +8,5 @@ def test_static_job(tmpdir): tmpdir.chdir() atoms = molecule("H2") - with pytest.raises(ValueError): + with pytest.raises(RuntimeError): relax_job(atoms, 0, 1) diff --git a/tests/core/recipes/qchem_recipes/test_qchem_recipes.py b/tests/core/recipes/qchem_recipes/test_qchem_recipes.py index 6f87cf52bc..46b00042c8 100644 --- a/tests/core/recipes/qchem_recipes/test_qchem_recipes.py +++ b/tests/core/recipes/qchem_recipes/test_qchem_recipes.py @@ -501,7 +501,7 @@ def test_irc_job_v1(monkeypatch, tmpdir, test_atoms): def test_irc_job_v2(tmpdir, test_atoms): tmpdir.chdir() - with pytest.raises(ValueError): + with pytest.raises(RuntimeError): irc_job(test_atoms, 0, 1, "straight") with pytest.raises(ValueError): diff --git a/tests/core/schemas/test_ase.py b/tests/core/schemas/test_ase.py index 26a06abefc..e3ae002189 100644 --- a/tests/core/schemas/test_ase.py +++ b/tests/core/schemas/test_ase.py @@ -132,7 +132,7 @@ def test_summarize_opt_run(tmpdir): dyn.run(steps=5) traj = read("test.traj", index=":") - with pytest.raises(ValueError): + with pytest.raises(RuntimeError): summarize_opt_run(dyn) # Make sure info tags are handled appropriately diff --git a/tests/core/schemas/test_cclib_schema.py b/tests/core/schemas/test_cclib_schema.py index 2160614141..20e46a5353 100644 --- a/tests/core/schemas/test_cclib_schema.py +++ b/tests/core/schemas/test_cclib_schema.py @@ -220,7 +220,7 @@ def test_cclib_calculate(tmpdir, cclib_obj): proatom_dir="does_not_exists", ) - with pytest.raises(ValueError): + with pytest.raises(OSError): _cclib_calculate( cclib_obj, method="ddec6", diff --git a/tests/core/schemas/test_vasp_pop_analysis.py b/tests/core/schemas/test_vasp_pop_analysis.py index 7b4ef5b46f..cb4a3ac795 100644 --- a/tests/core/schemas/test_vasp_pop_analysis.py +++ b/tests/core/schemas/test_vasp_pop_analysis.py @@ -97,7 +97,7 @@ def test_chargemol_erorr(tmpdir): tmpdir.chdir() prep_files() - with pytest.raises(EnvironmentError): + with pytest.raises(OSError): _chargemol_runner() os.remove("CHGCAR")