From 62324768aaf8ba5ab89aec5c56206a42b19b2391 Mon Sep 17 00:00:00 2001 From: "M. Eric Irrgang" Date: Mon, 23 Jul 2018 16:55:03 +0300 Subject: [PATCH] Clean up. Remove dead code. Resolve "to-do"s and/or reference issue tracker. Prepare for release. --- RELEASE.txt | 2 +- docs/conf.py | 4 --- setup.py | 4 +-- src/gmx/__init__.py | 1 - src/gmx/context.py | 64 +++++++++++++++++++++++---------- src/gmx/core/export_system.cpp | 4 +-- src/gmx/system.py | 7 ++-- src/gmx/test/test_exceptions.py | 2 +- src/gmx/test/test_pymd.py | 14 ++++---- src/gmx/test/test_workflow.py | 1 - src/gmx/version.in | 4 +-- src/gmx/workflow.py | 7 ++-- 12 files changed, 69 insertions(+), 45 deletions(-) diff --git a/RELEASE.txt b/RELEASE.txt index 015c6edfbf..02e7b1a691 100644 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -8,7 +8,7 @@ Once the required features in gromacs-gmxapi have been merged to its master bran Similarly, update the `gromacs_url` embedded in setup.py After merging the gmxapi development branch to master and before tagging the release, -update setup.py so that it writes a version.py file with `release = True`. +update version.in so that it writes a version.py file with `release = True`. Once the release is tagged and any last-minute essentials are cherry-picked to the refreshed development branch, bump the version in the development branch in CMakeLists.txt _and_ in setup.py. If / when feature branches diff --git a/docs/conf.py b/docs/conf.py index bbbf982f8f..3f740c02d9 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -15,9 +15,6 @@ # add these directories to sys.path here. If the directory is relative to the # documentation root, use os.path.abspath to make it absolute, like shown here. # -# todo: find location of breathe package -import os -import sys from pkg_resources import get_distribution, DistributionNotFound # sys.path.insert(0, os.path.abspath('.')) #sys.path.insert(0, '@CMAKE_INSTALL_PREFIX@/@PYMOD_INSTALL_DIR@') @@ -42,7 +39,6 @@ # 'breathe', ] -# todo: need to figure out how to use breathe correctly... # Tell breathe about the projects #breathe_projects = {'gmxapi': "@GMXAPI_USERDOC_OUTPUT@/xml", # 'dev': "@GMXAPI_DEVDOC_OUTPUT@/xml"} diff --git a/setup.py b/setup.py index 044988ba89..1e269786b0 100644 --- a/setup.py +++ b/setup.py @@ -208,9 +208,7 @@ def build_extension(self, ext): # Linking is a pain because the package is relocated to the site-packages directory. We should really do this # in two stages. if build_gromacs: - # TODO! We need to distinguish dev branch builds from master branch builds or always build with the - # master branch of the dependency. For one thing, as is, this line needs to be toggled for every release. - gromacs_url = "https://github.com/kassonlab/gromacs-gmxapi/archive/devel.zip" + gromacs_url = "https://github.com/kassonlab/gromacs-gmxapi/archive/v0.0.6.zip" gmxapi_DIR = os.path.join(extdir, 'data/gromacs') if build_for_readthedocs: extra_cmake_args = ['-DCMAKE_INSTALL_PREFIX=' + gmxapi_DIR, diff --git a/src/gmx/__init__.py b/src/gmx/__init__.py index 7e8557a0a5..83e7c43bad 100644 --- a/src/gmx/__init__.py +++ b/src/gmx/__init__.py @@ -66,7 +66,6 @@ from __future__ import print_function from __future__ import unicode_literals -# TODO: what should happen when ``from gmx import *``? __all__ = ['Status', 'System', 'get_context', 'run'] # Import system facilities diff --git a/src/gmx/context.py b/src/gmx/context.py index 32d7af75c9..43bc9a41e7 100644 --- a/src/gmx/context.py +++ b/src/gmx/context.py @@ -41,10 +41,9 @@ class Context(object): any case, the operations allow a Context implementation to transform a work specification into a directed acyclic graph of schedulable work. """ - # \todo Put the following to-do someplace more appropriate - # \todo There should be a default Context active from the first `import gmx` or at least before the first logger call. # The Context is the appropriate entity to own or mediate access to an appropriate logging facility, # but right now we are using the module-level Python logger. + # Reference https://github.com/kassonlab/gmxapi/issues/135 def __init__(self, workflow=None): """Create new context bound to the provided workflow, if any. @@ -101,8 +100,6 @@ def check_workspec(cls, workspec, raises=False): is_valid = False if raises: raise exceptions.ApiError('WorkSpec must contain at least one source element') - for name in workspec.elements: - element = gmx.workflow.WorkElement.deserialize() return is_valid @@ -116,17 +113,23 @@ def __enter__(self): raise exceptions.Error('Already running.') # The API runner currently has an implicit context. try: - # \todo Pass the API context implementation object to launch + # launch() with no arguments is deprecated. + # Ref: https://github.com/kassonlab/gmxapi/issues/124 self._session = self.workflow.launch() - # \todo Let the session provide a reference to its parent context instance. except: self._session = None raise return self._session def __exit__(self, exception_type, exception_value, traceback): - """Implement Python context manager protocol.""" - # Todo: handle exceptions. + """Implement Python context manager protocol. + + Closing a session should not produce Python exceptions. Instead, exit + state is accessible through API objects like Status. + For evolving design points, see + - https://github.com/kassonlab/gmxapi/issues/41 + - https://github.com/kassonlab/gmxapi/issues/121 + """ self._session.close() self._session = None return False @@ -387,6 +390,18 @@ def work(self): @work.setter def work(self, work): + """Set `work` attribute. + + Raises: + gmx.exceptions.ApiError: work is not compatible with schema or + known operations. + gmx.exceptions.UsageError: Context can not access operations in + the name space given for an Element + gmx.exceptions.ValueError: assignment operation cannot be performed + for the provided object (rhs) + + For discussion on error handling, see https://github.com/kassonlab/gmxapi/issues/125 + """ if work is None: warnings.warn("A Context without a valid WorkSpec is iffy...") return @@ -398,20 +413,23 @@ def work(self, work): elif hasattr(work, 'workspec') and isinstance(work.workspec, WorkSpec): workspec = work.workspec else: - raise ValueError('work argument must provide a gmx.workflow.WorkSpec.') + raise exceptions.ValueError('work argument must provide a gmx.workflow.WorkSpec.') workspec._context = self # Make sure this context knows how to run the specified work. for e in workspec.elements: element = gmx.workflow.WorkElement.deserialize(workspec.elements[e]) - if element.namespace not in {'gmxapi', 'gromacs'} and element.namespace not in self.__operations: # Non-built-in namespaces are treated as modules to import. try: element_module = importlib.import_module(element.namespace) except ImportError as e: - raise exceptions.UsageError('This context does not know how to invoke {} from {}. ImportError: {}'.format(element.operation, element.namespace, e.message)) + raise exceptions.UsageError( + 'This context does not know how to invoke {} from {}. ImportError: {}'.format( + element.operation, + element.namespace, + e.message)) # Don't leave an empty nested dictionary if we couldn't map the operation. if element.namespace in self.__operations: @@ -424,7 +442,8 @@ def work(self, work): element_operation = getattr(element_module, element.operation) namespace_map[element.operation] = element_operation except: - raise exceptions.ApiError('Operation {} not found in {}.'.format(element.operation, element.namespace)) + raise exceptions.ApiError('Operation {} not found in {}.'.format(element.operation, + element.namespace)) # Set or update namespace map only if we have something to contribute. self.__operations[element.namespace] = namespace_map else: @@ -433,12 +452,16 @@ def work(self, work): assert element.namespace in self.__operations if not element.operation in self.__operations[element.namespace]: if self.rank < 1: - logger.error("Operation {} not found in map {}".format(element.operation, str(self.__operations))) + logger.error("Operation {} not found in map {}".format(element.operation, + str(self.__operations))) # This check should be performed when deciding if the context is appropriate for the work. # If we are just going to use a try/catch block for this test, then we should differentiate # this exception from those raised due to incorrect usage. - # \todo Consider distinguishing API misuse from API failures. - raise exceptions.ApiError('Specified work cannot be performed due to unimplemented operation {}.{}.'.format(element.namespace, element.operation)) + # The exception thrown here may evolve with https://github.com/kassonlab/gmxapi/issues/125 + raise exceptions.ApiError( + 'Specified work cannot be performed due to unimplemented operation {}.{}.'.format( + element.namespace, + element.operation)) self.__work = workspec @@ -734,10 +757,10 @@ def __enter__(self): # gmxapi::Session objects are exposed as gmx.core.MDSession and provide run() and close() methods. # # Here, I want to find the input appropriate for this rank and get an MDSession for it. - # \todo In the future, we should set up an object with "ports" configured and then instantiate. # E.g. Make a pass that allows meta-objects to bind (setting md_proxy._input_tpr and md_proxy._plugins, # and then call a routine implemented by each object to run whatever protocol it needs, such # as `system = gmx.core.from_tpr(md._input_tpr); system.add_potential(md._plugins) + # For future design plans, reference https://github.com/kassonlab/gmxapi/issues/65 if self._session_ensemble_size > 0: # print(graph) logger.debug(("Launching graph {}.".format(graph.graph))) @@ -808,14 +831,14 @@ def close(self): def __exit__(self, exception_type, exception_value, traceback): """Implement Python context manager protocol.""" - # Todo: handle exceptions. - # \todo: we should not have a None session but rather an API-compliant Session that just has no work. logger.info("Exiting session on context rank {}.".format(self.rank)) if self._session is not None: logger.info("Calling session.close().") self._session.close() self._session = None else: + # Note: we should not have a None session but rather an API-compliant Session that just has no work. + # Reference: https://github.com/kassonlab/gmxapi/issues/41 logger.info("No _session known to context or session already closed.") if hasattr(self, '_session_ensemble_communicator'): from mpi4py import MPI @@ -828,8 +851,11 @@ def __exit__(self, exception_type, exception_value, traceback): logger.debug("No ensemble subcommunicator on context rank {}.".format(self.rank)) os.chdir(self.__initial_cwd) logger.info("Session closed on context rank {}.".format(self.rank)) - # \todo Make sure session has ended on all ranks before continuing and handle final errors. + # Note: Since sessions running in different processes can have different work, sessions have not necessarily + # ended on all ranks. As a result, starting another session on the same resources could block until the + # resources are available. + # Python context managers return False when there were no exceptions to handle. return False def get_context(work=None): diff --git a/src/gmx/core/export_system.cpp b/src/gmx/core/export_system.cpp index 76c48cfcf6..de6a8f7ae3 100644 --- a/src/gmx/core/export_system.cpp +++ b/src/gmx/core/export_system.cpp @@ -70,14 +70,14 @@ void export_system(py::module &m) } else { - // Todo: Need to bind the exceptions... + // Note: Exception behavior is likely to change. + // Ref: https://github.com/kassonlab/gmxapi/issues/125 throw PyExc_RuntimeError; } }, "Set a restraint potential for the system."); // Export session class - // \todo relocate // We can't completely surrender ownership to Python because other API objects may refer to it. py::class_<::gmxapi::Session, std::shared_ptr<::gmxapi::Session>> session(m, "MDSession"); session.def("run", &::gmxapi::Session::run, "Run the simulation workflow"); diff --git a/src/gmx/system.py b/src/gmx/system.py index 4c07f71b84..114b247498 100644 --- a/src/gmx/system.py +++ b/src/gmx/system.py @@ -58,8 +58,11 @@ def _from_file(inputrecord): context and create a System object. If no Context is currently bound, a local default context is created and bound. - TODO: clarify the file location relative to the execution context. - Until then, this helper method should not be part of the public + We are still refining the meanings of paths and file locations, and the + semantics of turning file objects into API objects. For reference, see + https://github.com/kassonlab/gmxapi/issues/96 + + Until resolved, this helper method should not be part of the public interface. Args: diff --git a/src/gmx/test/test_exceptions.py b/src/gmx/test/test_exceptions.py index 253015facb..015ac76d33 100644 --- a/src/gmx/test/test_exceptions.py +++ b/src/gmx/test/test_exceptions.py @@ -6,7 +6,7 @@ Throwing of appropriate exceptions is tested using assertRaises in test for the components that throw them. -\todo these tests are stale but we probably should update and use them... +Reference: https://github.com/kassonlab/gmxapi/issues/125 """ import unittest diff --git a/src/gmx/test/test_pymd.py b/src/gmx/test/test_pymd.py index 409555dfb8..7a15bfd409 100644 --- a/src/gmx/test/test_pymd.py +++ b/src/gmx/test/test_pymd.py @@ -6,6 +6,7 @@ # I'm increasingly thinking that the CMake-managed C++ extension module should be managed separately than the setuptools # primary module. Then we can just do standard things like using CTest and googletest for the more complicated stuff. +import warnings import logging logging.getLogger().setLevel(logging.DEBUG) # create console handler @@ -140,12 +141,13 @@ def test_plugin(caplog): context.add_operation(potential_element.namespace, potential_element.operation, my_plugin) context.work = md - # \todo swallow warning about wide MPI context - # \todo use pytest context managers to turn raised exceptions into assertions. - with context as session: - if context.rank == 0: - print(context.work) - session.run() + with warnings.catch_warnings(): + # Swallow warning about wide MPI context + warnings.simplefilter("ignore") + with context as session: + if context.rank == 0: + print(context.work) + session.run() if __name__ == '__main__': diff --git a/src/gmx/test/test_workflow.py b/src/gmx/test/test_workflow.py index 59bb20d604..aa8f2295c9 100644 --- a/src/gmx/test/test_workflow.py +++ b/src/gmx/test/test_workflow.py @@ -90,7 +90,6 @@ def test_creation(self): # Release 0.0.4 will mark the finalization of workspec version 0.1. assert gmx.version.api_is_at_least(0,0,5) assert workspec.version == "gmxapi_workspec_0_1" - # \todo better python package version checking. def test_methods(self): workspec = gmx.workflow.WorkSpec() assert str(workspec) is not None diff --git a/src/gmx/version.in b/src/gmx/version.in index b2bbee4ecb..c4680cc46f 100644 --- a/src/gmx/version.in +++ b/src/gmx/version.in @@ -14,8 +14,8 @@ major = @PROJECT_VERSION_MAJOR@ minor = @PROJECT_VERSION_MINOR@ patch = @PROJECT_VERSION_PATCH@ -# Todo: Release status needs to be automatically maintained. -release = False +# Note: this is not automatically updated. See RELEASE.txt and https://github.com/kassonlab/gmxapi/issues/152 +release = True def api_is_at_least(major_version, minor_version=0, patch_version=0): """Allow client to check whether installed module supports the requested API level. diff --git a/src/gmx/workflow.py b/src/gmx/workflow.py index bf783ede1c..c3a4bf4ec1 100644 --- a/src/gmx/workflow.py +++ b/src/gmx/workflow.py @@ -253,7 +253,6 @@ def _chase_deps(self, source_set, name_list): name_list: name list to be expanded with dependencies and sequenced Note that source_set is a reference to an object that is modified arbitrarily. - .. todo:: Maybe this shouldn't be a member function, but a closure within WorkSpec.__iter__() """ assert isinstance(source_set, set) @@ -439,7 +438,8 @@ def __init__(self, namespace="gmxapi", operation=None, params=None, depends=()): else: raise exceptions.UsageError("Invalid argument type for operation.") - # \todo It is currently non-sensical to update any attributes after adding to a workspec, but nothing prevents it. + # Note: Nothing currently prevents attribute updates by assignment after adding the element to a workspec, + # but this protocol will be clarified with https://github.com/kassonlab/gmxapi/issues/92 if params is None: self.params = {} elif isinstance(params, dict): @@ -642,7 +642,8 @@ def from_tpr(input=None, **kwargs): arg_path = os.path.abspath(arg) raise exceptions.UsageError(usage + " Got {}".format(arg_path)) - # \todo These are runner parameters, not MD parameters, and should be in the call to gmx.run() instead of here. + # Note: These are runner parameters, not MD parameters, and should be in the call to gmx.run() instead of here. + # Reference https://github.com/kassonlab/gmxapi/issues/95 params = {} for arg_key in kwargs: if arg_key == 'grid' or arg_key == 'dd':