Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Log packing failures #1067

Merged
merged 2 commits into from
Oct 3, 2024
Merged

Log packing failures #1067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61b304d
TST: Test writing log file on convergence failure
mattwthompson Oct 2, 2024
d3a2465
ENH: Write errors to log file on packing failure
mattwthompson Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions openff/interchange/_tests/unit_tests/components/test_packmol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
Units tests for openff.interchange.components._packmol
"""

import pathlib

import numpy
import pytest
from openff.toolkit.topology import Molecule
Expand Down Expand Up @@ -422,3 +424,22 @@ def test_load_100_000_atoms(self):
tolerance=1.0 * unit.angstrom,
mass_density=0.1 * unit.grams / unit.milliliters,
)

@pytest.mark.parametrize("use_local_path", [False, True])
def test_save_error_on_convergence_failure(self, use_local_path):
with pytest.raises(
PACKMOLRuntimeError,
match="failed with error code 173",
):
pack_box(
molecules=[Molecule.from_smiles("CCO")],
number_of_copies=[100],
box_shape=UNIT_CUBE,
mass_density=100 * unit.grams / unit.milliliters,
working_directory="." if use_local_path else None,
)

if use_local_path:
assert "STOP 173" in open("packmol_error.log").read()
else:
assert not pathlib.Path("packmol_error.log").is_file()
24 changes: 17 additions & 7 deletions openff/interchange/components/_packmol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ def _create_solute_pdb(
),
)
# Write to pdb
solute_pdb_filename = "solvate.pdb"
solute_pdb_filename = "_PACKING_SOLUTE.pdb"
topology.to_file(
solute_pdb_filename,
file_format="PDB",
Expand All @@ -425,7 +425,7 @@ def _create_molecule_pdbs(molecules: list[Molecule]) -> list[str]:
# right order, we'll be able to read packmol's output, since we
# just load the PDB coordinates into a topology created from its
# component molecules.
pdb_file_name = f"{index}.pdb"
pdb_file_name = f"_PACKING_MOLECULE{index}.pdb"
pdb_file_names.append(pdb_file_name)

molecule.to_file(
Expand Down Expand Up @@ -706,15 +706,25 @@ def pack_box(
packmol_path,
stdin=file_handle,
stderr=subprocess.STDOUT,
).decode("utf-8")
)
except subprocess.CalledProcessError as error:
if "173" in str(error):
raise PACKMOLRuntimeError from error
# Custom error codes seem to start at 170
# https://github.com/m3g/packmol/blob/v20.15.1/src/exit_codes.f90#L13-L16
open("packmol_error.log", "w").write(error.stdout.decode("utf-8"))
Comment on lines +711 to +713
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to write this to a file instead of just dumping to stderr? print(error.stdout.decode("utf-8"), file=sys.stderr) would be about as terse and avoid having to warn about a temporary directory down below. Or you could write it into a StringIO and include it in the exception itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly that this is the easiest way I could think of dumping the error log in a way that the user could inspect later (but does require opting in to working_directory='.', retain_working_files=False). Also, it can be fairly long, like a few thousand lines for realistic systems. IIUC dumping to STDERR would display the same information to the user but potentially get stuff merged in with other errors (and maybe warnings?) which could be noisier

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. As a user I think I'd slightly prefer to have it all in one place, but I definitely see the tradeoff and think this is a reasonable choice too!


packmol_succeeded = result.find("Success!") > 0
raise PACKMOLRuntimeError(
f"PACKMOL failed with error code {error.returncode}. Wrote file packmol_error.log in working "
"directory, which might be a temporary directory. Set the argument `working_directory` to "
"point this to a persistent path.",
) from error

packmol_succeeded = result.decode("utf-8").find("Success!") > 0

if not packmol_succeeded:
raise PACKMOLRuntimeError(result)
raise PACKMOLRuntimeError(
"PACKMOL did not raise an error code, but 'Success!' not found in output. "
"Please raise an issue showing how you arrived at this error.",
)

positions = _load_positions(output_file_path)

Expand Down