File open fix (#471)

* Fix too many files being left open in packing.py `tempfile.mkstemp` provides a low level interface for interacting with temporary files. However, garbage collection (closing files, deleting files after use) must be manually taken care of. Packing.py uses temp files when packing or solvating a system, these files are never closed. This can cause the program to reach the limit of open files for a process set by the OS level `ulimit`. These changes migrate from using `mkstemp` to a higher level interface `tempfile.NamedTemporaryFile`, providing the same type of temporary file, but with saner garbage collection and scope. The sections where temp files are made are now contained in `try finally` sections, to ensure proper file closure and deletion when the program finishes, or is interrupted. * Syntax changes NamedTemporaryFile provides a file-like object to interact with compared to mkstemp. To get the full path, the `name` attribute of the NamedTemporaryFile is needed. * remove nested try-finally blocks * Forgot to pass in path for the solvate method. * Move file creation to method, move topology generation to file. * Bump to version 0.8.1 * Update changelog to 0.8.1 * Update gitignore file for VSCode Using Microsoft's VSCode generates an addtional directory and files to assist the various plugins, save states, etc. This is not needed for other users. This has been added to the .gitignore file to prevent these files from being committed erroneously. * Fixes issues with packmol input files (#474) * Fixes issues with packmol input file Also reports error based on process code instead of output, which prevented report of error about input issues. * Bump to version 0.8.1 * Update changelog to 0.8.1 * Small formatting nits * Run `activate base` before `conda build` in Appveyor (#477) * Run `activate base` before `conda build` See ContinuumIO/anaconda-issues#10211 (comment) * Update changelog * GSD files now include 1,4 special pairs for use in OPLS (#473) * Update dependency requirements (#457) * Make foyer an optional dependency * Split dependencies into required and required for development * Update developer requirements * Marked appropriate tests to check if Foyer is installed * Remove duplicate line * Remove openbabel and gsd from Appveyor testing * Attempt to fix coverage dependency issue (#466) * Attempt to fix coverage dependency issue Currently, the most recent version of python-coveralls requires `coverage==4.0.3`, while pytest-cov requires `Coverage>=4.4`. The current fix seems to be to pin `pytest-cov` to a previous version. This can be changed once: z4r/python-coveralls#66 has been resolved. * Hotfix for MDTraj MOL2 file issues MDTraj has merged a fix for MOL2 file reading mdtraj/mdtraj#1378 However, it has not been included in a new release on `conda` yet. Pinning to an older version without the MOL2 fixes currently. * Forgot to update Appveyor build script * Bump to version 0.8.1 * Update changelog to 0.8.1 * Fixes issues with packmol input files (#474) * Fixes issues with packmol input file Also reports error based on process code instead of output, which prevented report of error about input issues. * Bump to version 0.8.1 * Update changelog to 0.8.1 * Small formatting nits * Run `activate base` before `conda build` See ContinuumIO/anaconda-issues#10211 (comment) * Update changelog * Re-pin mdtraj and pytest-cov * Syntax changes NamedTemporaryFile provides a file-like object to interact with compared to mkstemp. To get the full path, the `name` attribute of the NamedTemporaryFile is needed. * Ensure all files generated in the for loops are closed and unlinked * Increase the box dims for testing fill_region a test for `fill_region` was packing a very small volume (~2nm^3) with many water molecules. This led to parts of the molecules outside the defined region. The region has been increased. * Remove support for pytest-ignore-flaky Recently, the tests on travis for the example notebooks are failing due to some internal error with pytest-ignore-flaky plugin. It seems like the feature can be replicated well enough using @pytest.mark.xfail with the strict setting to False. * Update CI as well to remove 'pytest-ignore-flaky' * Change the overlap parameter for a test, was causing floating point errors in PACKMOL * Changelog.md * Include information about the in changelog.md
mosdef-hub · Jan 8, 2019 · 2bff0dd · 2bff0dd
1 parent 8379c12
commit 2bff0dd
Show file tree

Hide file tree

Showing 7 changed files with 174 additions and 85 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -30,7 +30,7 @@ install:
   - pip install -e .
 
 script:
-  - python -m pytest --ignore-flaky -v --cov=mbuild  --cov-report= --pyargs mbuild
+  - python -m pytest  -v --cov=mbuild  --cov-report= --pyargs mbuild
 
 after_success:
   - coveralls

diff --git a/changelog.md b/changelog.md
@@ -11,6 +11,16 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ## 0.8.2 (unreleased)
 ### Misc and Bugfixes
 * Fixed a bug that prevented Appveyor builds from running (#477)
+* Temporary PDB files left behind by `packing.py` are now removed (#471)
+    * Packing.py uses temp files when packing or solvating a system,
+these files are never closed. This can cause the program to
+reach the limit of open files for a process set by the OS level
+`ulimit`
+    * These files are now only present when required, and when they are
+not needed anymore, they are deleted
+* Removed pytest-ignore-flaky as a dependency for the unit tests (#471)
+    * This `pytest` plugin is now broken on python2.7
+    * The `pytest xfail` decorator provides similar enough support
 
 ## 0.8.1 (2018-11-28)
 ### Features

diff --git a/devtools/conda-recipe/meta.yaml b/devtools/conda-recipe/meta.yaml
@@ -30,15 +30,14 @@ test:
     - nbformat
     - ipykernel
     - ipyext
-    - pytest-ignore-flaky
     - foyer
 
   source_files:
     - mbuild/examples/*
 
   commands:
     - set LNAME=mosdef
-    - pytest --ignore-flaky -v --pyargs mbuild
+    - pytest -v --pyargs mbuild
 
 about:
   home: http://mosdef-hub.github.io/mbuild

diff --git a/mbuild/examples/test_examples.py b/mbuild/examples/test_examples.py
@@ -19,7 +19,7 @@
 @pytest.mark.skipif(sys.platform in ['win32'],
                     reason="Not testing examples on Appveyor")
 @pytest.mark.parametrize("filepath", EXAMPLE_NOTEBOOKS)
-@pytest.mark.flaky("filepath", EXAMPLE_NOTEBOOKS)
+@pytest.mark.xfail(strict=False)
 def test_examples(filepath):
     check_one_notebook(filepath)
 

diff --git a/mbuild/packing.py b/mbuild/packing.py
@@ -109,7 +109,7 @@ def fill_box(compound, n_compounds=None, box=None, density=None, overlap=0.2,
     arg_count = 3 - [n_compounds, box, density].count(None)
     if arg_count != 2:
         msg = ("Exactly 2 of `n_compounds`, `box`, and `density` "
-            "must be specified. {} were given.".format(arg_count))
+               "must be specified. {} were given.".format(arg_count))
         raise ValueError(msg)
 
     if box is not None:
@@ -131,11 +131,10 @@ def fill_box(compound, n_compounds=None, box=None, density=None, overlap=0.2,
             msg = ("`compound`, `n_compounds`, and `fix_orientation` must be of equal length.")
             raise ValueError(msg)
 
-
     if density is not None:
         if box is None and n_compounds is not None:
             total_mass = np.sum([n*np.sum([a.mass for a in c.to_parmed().atoms])
-                for c,n in zip(compound, n_compounds)])
+                                for c, n in zip(compound, n_compounds)])
             # Conversion from (amu/(kg/m^3))**(1/3) to nm
             L = (total_mass/density)**(1/3)*1.1841763
             if aspect_ratio is None:
@@ -176,27 +175,39 @@ def fill_box(compound, n_compounds=None, box=None, density=None, overlap=0.2,
     box_maxs -= edge * 10
 
     # Build the input file for each compound and call packmol.
-    filled_pdb = tempfile.mkstemp(suffix='.pdb')[1]
-    input_text = PACKMOL_HEADER.format(overlap, filled_pdb, seed)
-
-    for comp, m_compounds, rotate in zip(compound, n_compounds, fix_orientation):
-        m_compounds = int(m_compounds)
-        compound_pdb = tempfile.mkstemp(suffix='.pdb')[1]
-        comp.save(compound_pdb, overwrite=True)
-        input_text += PACKMOL_BOX.format(compound_pdb, m_compounds,
-                           box_mins[0], box_mins[1], box_mins[2],
-                           box_maxs[0], box_maxs[1], box_maxs[2],
-                           PACKMOL_CONSTRAIN if rotate else "")
-
-    _run_packmol(input_text, filled_pdb, temp_file)
-
-    # Create the topology and update the coordinates.
-    filled = Compound()
-    for comp, m_compounds in zip(compound, n_compounds):
-        for _ in range(m_compounds):
-            filled.add(clone(comp))
-    filled.update_coordinates(filled_pdb)
-    filled.periodicity = np.asarray(box.lengths, dtype=np.float32)
+    filled_pdb = _new_pdb_file()
+
+    # create a list to contain the file handles for the compound temp files
+    compound_pdb_list = list()
+    try:
+        input_text = PACKMOL_HEADER.format(overlap, filled_pdb.name, seed)
+        for comp, m_compounds, rotate in zip(compound, n_compounds, fix_orientation):
+            m_compounds = int(m_compounds)
+
+            compound_pdb = _new_pdb_file()
+            compound_pdb_list.append(compound_pdb)
+
+            comp.save(compound_pdb.name, overwrite=True)
+            input_text += PACKMOL_BOX.format(compound_pdb.name, m_compounds,
+                                             box_mins[0], box_mins[1],
+                                             box_mins[2], box_maxs[0],
+                                             box_maxs[1], box_maxs[2],
+                                             PACKMOL_CONSTRAIN if rotate else "")
+
+        _run_packmol(input_text, filled_pdb, temp_file)
+
+        # Create the topology and update the coordinates.
+        filled = Compound()
+        filled = _create_topology(filled, compound, n_compounds)
+        filled.update_coordinates(filled_pdb.name)
+        filled.periodicity = np.asarray(box.lengths, dtype=np.float32)
+
+    finally:
+        for file_handle in compound_pdb_list:
+            file_handle.close()
+            os.unlink(file_handle.name)
+        filled_pdb.close()
+        os.unlink(filled_pdb.name)
     return filled
 
 
@@ -230,8 +241,10 @@ def fill_region(compound, n_compounds, region, overlap=0.2,
     -------
     filled : mb.Compound
 
-    If using mulitple regions and compounds, the nth value in each list are used in order.
-    For example, if the third compound will be put in the third region using the third value in n_compounds.
+    If using mulitple regions and compounds, the nth value in each
+    list are used in order.
+    For example, if the third compound will be put in the third
+    region using the third value in n_compounds.
     """
     _check_packmol(PACKMOL)
 
@@ -251,9 +264,8 @@ def fill_region(compound, n_compounds, region, overlap=0.2,
             msg = ("`compound`, `n_compounds`, and `fix_orientation` must be of equal length.")
             raise ValueError(msg)
 
-
     # See if region is a single region or list
-    if isinstance(region, Box): # Cannot iterate over boxes
+    if isinstance(region, Box):  # Cannot iterate over boxes
         region = [region]
     elif not any(isinstance(reg, (list, set, Box)) for reg in region):
         region = [region]
@@ -263,29 +275,41 @@ def fill_region(compound, n_compounds, region, overlap=0.2,
     overlap *= 10
 
     # Build the input file and call packmol.
-    filled_pdb = tempfile.mkstemp(suffix='.pdb')[1]
-    input_text = PACKMOL_HEADER.format(overlap, filled_pdb, seed)
-
-    for comp, m_compounds, reg, rotate in zip(compound, n_compounds, region, fix_orientation):
-        m_compounds = int(m_compounds)
-        compound_pdb = tempfile.mkstemp(suffix='.pdb')[1]
-        comp.save(compound_pdb, overwrite=True)
-        reg_mins = reg.mins * 10
-        reg_maxs = reg.maxs * 10
-        reg_maxs -= edge * 10 # Apply edge buffer
-        input_text += PACKMOL_BOX.format(compound_pdb, m_compounds,
-                                        reg_mins[0], reg_mins[1], reg_mins[2],
-                                        reg_maxs[0], reg_maxs[1], reg_maxs[2],
-                                        PACKMOL_CONSTRAIN if rotate else "")
-
-    _run_packmol(input_text, filled_pdb, temp_file)
-
-    # Create the topology and update the coordinates.
-    filled = Compound()
-    for comp, m_compounds in zip(compound, n_compounds):
-        for _ in range(m_compounds):
-            filled.add(clone(comp))
-    filled.update_coordinates(filled_pdb)
+    filled_pdb = _new_pdb_file()
+
+    # List to hold file handles for the temporary compounds
+    compound_pdb_list = list()
+    try:
+        input_text = PACKMOL_HEADER.format(overlap, filled_pdb.name, seed)
+
+        for comp, m_compounds, reg, rotate in zip(compound, n_compounds, region, fix_orientation):
+            m_compounds = int(m_compounds)
+
+            compound_pdb = _new_pdb_file()
+            compound_pdb_list.append(compound_pdb)
+
+            comp.save(compound_pdb.name, overwrite=True)
+            reg_mins = reg.mins * 10
+            reg_maxs = reg.maxs * 10
+            reg_maxs -= edge * 10  # Apply edge buffer
+            input_text += PACKMOL_BOX.format(compound_pdb.name, m_compounds,
+                                             reg_mins[0], reg_mins[1],
+                                             reg_mins[2], reg_maxs[0],
+                                             reg_maxs[1], reg_maxs[2],
+                                            PACKMOL_CONSTRAIN if rotate else "")
+
+        _run_packmol(input_text, filled_pdb, temp_file)
+
+        # Create the topology and update the coordinates.
+        filled = Compound()
+        filled = _create_topology(filled, compound, n_compounds)
+        filled.update_coordinates(filled_pdb.name)
+    finally:
+        for file_handle in compound_pdb_list:
+            file_handle.close()
+            os.unlink(file_handle.name)
+        filled_pdb.close()
+        os.unlink(filled_pdb.name)
     return filled
 
 
@@ -336,7 +360,6 @@ def solvate(solute, solvent, n_solvent, box, overlap=0.2,
         msg = ("`n_solvent` and `n_solvent` must be of equal length.")
         raise ValueError(msg)
 
-
     # In angstroms for packmol.
     box_mins = box.mins * 10
     box_maxs = box.maxs * 10
@@ -347,29 +370,45 @@ def solvate(solute, solvent, n_solvent, box, overlap=0.2,
     box_maxs -= edge * 10
 
     # Build the input file for each compound and call packmol.
-    solvated_pdb = tempfile.mkstemp(suffix='.pdb')[1]
-    solute_pdb = tempfile.mkstemp(suffix='.pdb')[1]
-    solute.save(solute_pdb, overwrite=True)
-    input_text = (PACKMOL_HEADER.format(overlap, solvated_pdb, seed) +
-                  PACKMOL_SOLUTE.format(solute_pdb, *center_solute))
-
-    for solv, m_solvent, rotate in zip(solvent, n_solvent, fix_orientation):
-        m_solvent = int(m_solvent)
-        solvent_pdb = tempfile.mkstemp(suffix='.pdb')[1]
-        solv.save(solvent_pdb, overwrite=True)
-        input_text += PACKMOL_BOX.format(solvent_pdb, m_solvent,
-                           box_mins[0], box_mins[1], box_mins[2],
-                           box_maxs[0], box_maxs[1], box_maxs[2],
-                           PACKMOL_CONSTRAIN if rotate else "")
-    _run_packmol(input_text, solvated_pdb, temp_file)
-
-    # Create the topology and update the coordinates.
-    solvated = Compound()
-    solvated.add(solute)
-    for solv, m_solvent in zip(solvent, n_solvent):
-        for _ in range(m_solvent):
-            solvated.add(clone(solv))
-    solvated.update_coordinates(solvated_pdb)
+    solvated_pdb = _new_pdb_file()
+    solute_pdb = _new_pdb_file()
+
+    # generate list of temp files for the solvents
+    solvent_pdb_list = list()
+    try:
+        solute.save(solute_pdb.name, overwrite=True)
+        input_text = (PACKMOL_HEADER.format(overlap, solvated_pdb.name, seed) +
+                      PACKMOL_SOLUTE.format(solute_pdb.name, *center_solute))
+
+        for solv, m_solvent, rotate in zip(solvent, n_solvent, fix_orientation):
+            m_solvent = int(m_solvent)
+
+            solvent_pdb = _new_pdb_file()
+            solvent_pdb_list.append(solvent_pdb)
+
+            solv.save(solvent_pdb.name, overwrite=True)
+            input_text += PACKMOL_BOX.format(solvent_pdb.name, m_solvent,
+                                             box_mins[0], box_mins[1],
+                                             box_mins[2], box_maxs[0],
+                                             box_maxs[1], box_maxs[2],
+                                             PACKMOL_CONSTRAIN if rotate else "")
+        _run_packmol(input_text, solvated_pdb, temp_file)
+
+        # Create the topology and update the coordinates.
+        solvated = Compound()
+        solvated.add(solute)
+        solvated = _create_topology(solvated, solvent, n_solvent)
+        solvated.update_coordinates(solvated_pdb.name)
+
+    finally:
+        for file_handle in solvent_pdb_list:
+            file_handle.close()
+            os.unlink(file_handle.name)
+        solvated_pdb.close()
+        solute_pdb.close()
+        os.unlink(solvated_pdb.name)
+        os.unlink(solute_pdb.name)
+
     return solvated
 
 
@@ -387,6 +426,42 @@ def _validate_box(box):
     return box
 
 
+def _new_pdb_file():
+    """Generate PDB file using tempfile.NamedTemporaryFile.
+
+    Return
+    ------
+    _ : file-object
+        Temporary PDB file.
+    """
+
+    return tempfile.NamedTemporaryFile(suffix='.pdb', delete=False)
+
+
+def _create_topology(container, comp_to_add, n_compounds):
+    """Return updated mBuild compound with new coordinates.
+
+    Parameters
+    ----------
+    container : mb.Compound, required
+        Compound containing the updated system generated by PACKMOL.
+    comp_to_add : mb.Compound or list of mb.Compounds, required
+        Compound(s) to add to the container.
+    container : int or list of int, required
+        Amount of comp_to_add to container.
+
+    Return
+    ------
+    container : mb.Compound
+        Compound with added compounds from PACKMOL.
+    """
+
+    for comp, m_compound in zip(comp_to_add, n_compounds):
+            for _ in range(m_compound):
+                container.add(clone(comp))
+    return container
+
+
 def _packmol_error(out, err):
     """Log packmol output to files. """
     with open('log.txt', 'w') as log_file, open('err.txt', 'w') as err_file:
@@ -413,15 +488,17 @@ def _run_packmol(input_text, filled_pdb, temp_file):
                "the .pdb_FORCED file instead. This may not be a "
                "sufficient packing result.")
         warnings.warn(msg)
-        os.system('cp {0}_FORCED {0}'.format(filled_pdb))
+        os.system('cp {0}_forced {0}'.format(filled_pdb.name))
+
     if 'ERROR' in out or proc.returncode != 0:
         _packmol_error(out, err)
     else:
         # Delete input file if success
         os.remove(packmol_inp.name)
 
     if temp_file is not None:
-        os.system('cp {0} {1}'.format(filled_pdb, os.path.join(temp_file)))
+        os.system('cp {0} {1}'.format(filled_pdb.name, os.path.join(temp_file)))
+
 
 
 def _check_packmol(PACKMOL):

diff --git a/mbuild/tests/test_packing.py b/mbuild/tests/test_packing.py
@@ -38,11 +38,15 @@ def test_fill_box_compound_ratio(self, h2o, ethane):
 
     def test_fill_region(self, h2o):
         filled = mb.fill_region(h2o, n_compounds=50,
-                                region=[3, 2, 2, 4, 4, 3])
+                                region=[3, 2, 2, 5, 5, 5])
         assert filled.n_particles == 50 * 3
         assert filled.n_bonds == 50 * 2
         assert np.min(filled.xyz[:,0]) >= 3
-        assert np.max(filled.xyz[:,2]) <= 3
+        assert np.min(filled.xyz[:,1]) >= 2
+        assert np.min(filled.xyz[:,2]) >= 2
+        assert np.max(filled.xyz[:,0]) <= 5
+        assert np.max(filled.xyz[:,1]) <= 5
+        assert np.max(filled.xyz[:,2]) <= 5
 
     def test_fill_region_box(self, h2o):
         mybox = mb.Box([4, 4, 4])
@@ -151,7 +155,7 @@ def test_packmol_error(self, h2o):
 
     def test_packmol_warning(self, h2o):
         with pytest.warns(UserWarning):
-            filled = mb.fill_box(h2o, n_compounds=10, box=[1, 1, 1], overlap=100)
+            filled = mb.fill_box(h2o, n_compounds=10, box=[1, 1, 1], overlap=10)
 
     def test_rotate(self, h2o):
         filled = mb.fill_box(h2o, 2, box=[1, 1, 1], fix_orientation=True)

diff --git a/requirements-dev.txt b/requirements-dev.txt
@@ -28,4 +28,3 @@ python-coveralls
 # https://github.com/pywbem/pywbem/commit/d85a3e73e08d2846073087733b790b5a8864d93f
 pytest-cov>=2.4.0,<2.6
 pytest-faulthandler
-pytest-ignore-flaky