Add FutureWarning for default filesetter external engines

openpathsampling/deprecations.py

@@ -204,6 +204,20 @@ def version_tuple_to_string(version_tuple):
     deprecated_in=(1, 6, 0)
 )
 
+NEW_DEFAULT_FILENAME_SETTER =  Deprecation(
+    problem=("The default FilenameSetter for external engines is now a counter"
+             ", but will become a random string. This is more robust against "
+             "accidental overwrites."),
+    remedy=("If you want to keep the old counting behavior, add "
+            '{"filename_setter": '
+            "paths.engines.external_engine.FilenameSetter() } to the 'options'"
+            " of this engine. Otherwise, this behavior will automatically "
+            "change to RandomStringFilenames in OPS 2.0."),
+    remove_version=(2, 0),
+    deprecated_in=(1, 6, 0)
+)
+
+
 
 # has_deprecations and deprecate hacks to change docstrings inspired by:
 # https://stackoverflow.com/a/47441572/4205735

openpathsampling/engines/external_engine.py

@@ -1,7 +1,8 @@
 from openpathsampling.netcdfplus import StorableNamedObject
 from openpathsampling.engines.dynamics_engine import DynamicsEngine
 from openpathsampling.engines.snapshot import BaseSnapshot, SnapshotDescriptor
-from openpathsampling.engines.toy import ToySnapshot
+from openpathsampling.deprecations import NEW_DEFAULT_FILENAME_SETTER
+
 import numpy as np
 import os
 
@@ -16,10 +17,10 @@
 if sys.version_info > (3, ):
     long = int
 
-import linecache
 
 logger = logging.getLogger(__name__)
 
+
 def close_file_descriptors(basename):
     """Close file descriptors for the given filename.
 
@@ -49,6 +50,7 @@ def _debug_open_files(where=None, ext=".trr"):
 
             raise Exception("\n".join(message))
 
+
 def _debug_snapshot_loading(snapshot):
     snapshot.load_details()
     snapshot.clear_cache()
@@ -90,6 +92,7 @@ class RandomStringFilenames(FilenameSetter):
         string containing the allowed characters to return
     """
     _allowed = 'abcdefghijklmnopqrstuvwxyz0123456789'
+
     def __init__(self, length=8, allowed=None):
         super(RandomStringFilenames, self).__init__()
         self.length = length
@@ -99,6 +102,7 @@ def __init__(self, length=8, allowed=None):
     def __call__(self):
         return "".join(np.random.choice(self.allowed, self.length))
 
+
 class _InternalizedEngineProxy(DynamicsEngine):
     """Wrapper that allows snapshots to be "internalized."
 
@@ -134,14 +138,14 @@ class ExternalEngine(DynamicsEngine):
     """
 
     _default_options = {
-        'n_frames_max' : 10000,
-        'name_prefix' : "test",
-        'default_sleep_ms' : 100,
-        'auto_optimize_sleep' : True,
-        'engine_sleep' : 100,
-        'engine_directory' : "",
-        'n_spatial' : 1,
-        'n_atoms' : 1,
+        'n_frames_max': 10000,
+        'name_prefix': "test",
+        'default_sleep_ms': 100,
+        'auto_optimize_sleep': True,
+        'engine_sleep': 100,
+        'engine_directory': "",
+        'n_spatial': 1,
+        'n_atoms': 1,
         'n_poll_per_step': 1,
         'filename_setter': FilenameSetter(),
     }
@@ -161,6 +165,11 @@ def __init__(self, options, descriptor, template,
         self._current_snapshot = template
         self.n_frames_since_start = None
         self.internalized_engine = _InternalizedEngineProxy(self)
+        if 'filename_setter' not in options:
+            # Level 6 is needed to raise it to the initialization of a
+            # gromacs engine. This is a FutureWarning to also warn
+            # users, not only developers.
+            NEW_DEFAULT_FILENAME_SETTER.warn(6, category=FutureWarning)
 
     def to_dict(self):
         dct = super(ExternalEngine, self).to_dict()
@@ -207,25 +216,26 @@ def generate_next_frame(self):
                     next_frame = None
                 else:
                     raise
-            #print self.frame_num, next_frame # DEBUG LOGGER
+            # print self.frame_num, next_frame # DEBUG LOGGER
             now = time.time()
             if next_frame == "partial":
                 if self.proc.poll() is not None:
                     raise RuntimeError("External engine died unexpectedly")
-                time.sleep(0.001) # wait a millisec and rerun
+                time.sleep(0.001)  # wait a millisec and rerun
             elif next_frame is None:
                 if self.proc.poll() is not None:
                     raise RuntimeError("External engine died unexpectedly")
                 logger.debug("Sleeping for {:.2f}ms".format(self.sleep_ms))
                 time.sleep(self.sleep_ms/1000.0)
-            elif isinstance(next_frame, BaseSnapshot): # success
+            elif isinstance(next_frame, BaseSnapshot):  # success
                 self.n_frames_since_start += 1
                 logger.debug("Found frame %d", self.n_frames_since_start)
                 self.current_snapshot = next_frame
                 next_frame_found = True
                 self.frame_num += 1
             else:  # pragma: no cover
-                raise RuntimeError("Strange return value from read_next_frame_from_file")
+                raise RuntimeError("Strange return value from "
+                                   "read_next_frame_from_file")
             if self.auto_optimize_sleep and self.n_frames_since_start > 0:
                 n_poll_per_step = self.options['n_poll_per_step']
                 elapsed = now - self.start_time
@@ -244,15 +254,14 @@ def start(self, snapshot=None):
         self.write_frame_to_file(self.input_file, self.current_snapshot, "w")
         self.prepare()
 
-        cmd = shlex.split(self.engine_command())
         self.start_time = time.time()
         try:
             logger.info(self.engine_command())
             # TODO: add the ability to have handlers for stdin and stdout
             self.proc = psutil.Popen(shlex.split(self.engine_command()),
                                      preexec_fn=os.setsid)
         except OSError:  # pragma: no cover
-            raise  #TODO: need to handle this, but do what?
+            raise  # TODO: need to handle this, but do what?
         else:
             logger.info("Started engine: " + str(self.proc))
 
@@ -317,4 +326,3 @@ def set_filenames(self, number):
     def engine_command(self):
         """Generates a string for the command to run the engine."""
         raise NotImplementedError()
-

openpathsampling/tests/test_external_engine.py

@@ -7,6 +7,7 @@
 from openpathsampling.engines.external_engine import *
 
 import numpy as np
+import pytest
 
 import psutil
 
@@ -101,13 +102,15 @@ def setup(self):
             'n_frames_max': 10000,
             'engine_sleep': 100,
             'name_prefix': "test",
-            'engine_directory': engine_dir
+            'engine_directory': engine_dir,
+            'filename_setter': FilenameSetter()
         }
         fast_options = {
             'n_frames_max': 10000,
             'engine_sleep': 0,
             'name_prefix': "test",
-            'engine_directory': engine_dir
+            'engine_directory': engine_dir,
+            'filename_setter': FilenameSetter()
         }
         self.template = peng.toy.Snapshot(coordinates=np.array([[0.0]]),
                                           velocities=np.array([[1.0]]))
@@ -119,6 +122,16 @@ def setup(self):
                                                  self.template)
         self.ensemble = paths.LengthEnsemble(5)
 
+    def test_deprecation(self):
+        slow_options = {'n_frames_max': 10000,
+                        'engine_sleep': 100,
+                        'name_prefix': "test",
+                        'engine_directory': engine_dir}
+        with pytest.warns(FutureWarning, match='filename_setter'):
+            _ = ExampleExternalEngine(slow_options,
+                                      self.descriptor,
+                                      self.template)
+
     def test_start_stop(self):
         eng = self.fast_engine
         # check that it isn't running yet