From ab90613cf127bbd285c30140acea517e701b65e9 Mon Sep 17 00:00:00 2001 From: "M. Eric Irrgang" Date: Mon, 28 Sep 2020 21:11:28 +0300 Subject: [PATCH] Clarifications for ResourceAssignment interface. Add and update some documentation. Improve error messages. Improve file naming. Refs #3644 --- api/gmxapi/CMakeLists.txt | 4 +-- api/gmxapi/cpp/CMakeLists.txt | 2 +- api/gmxapi/cpp/context.cpp | 15 +++++++---- api/gmxapi/cpp/context_impl.h | 25 ++++++++++++++++--- ...ngresources.cpp => resourceassignment.cpp} | 2 +- api/gmxapi/include/gmxapi/mpi/gmxapi_mpi.h | 20 +++++++++------ ...cmakein.h => resourceassignment.cmakein.h} | 6 ++--- 7 files changed, 50 insertions(+), 24 deletions(-) rename api/gmxapi/cpp/{multiprocessingresources.cpp => resourceassignment.cpp} (98%) rename api/gmxapi/include/{multiprocessingresources.cmakein.h => resourceassignment.cmakein.h} (97%) diff --git a/api/gmxapi/CMakeLists.txt b/api/gmxapi/CMakeLists.txt index be182daa33..fa145fceef 100644 --- a/api/gmxapi/CMakeLists.txt +++ b/api/gmxapi/CMakeLists.txt @@ -67,7 +67,7 @@ set_target_properties(gmxapi PROPERTIES # The include directory should be mostly empty so that we can use it internally as # the public interface include directory during build and testing. configure_file(include/gmxapiversion.h.in include/gmxapi/version.h) -configure_file(include/multiprocessingresources.cmakein.h include/gmxapi/mpi/multiprocessingresources.h) +configure_file(include/resourceassignment.cmakein.h include/gmxapi/mpi/resourceassignment.h) target_include_directories(gmxapi PUBLIC $ $ @@ -90,6 +90,6 @@ install(DIRECTORY include/gmxapi # Install "configured" files from the build tree. install(FILES ${CMAKE_CURRENT_BINARY_DIR}/include/gmxapi/version.h DESTINATION include/gmxapi) -install(FILES ${CMAKE_CURRENT_BINARY_DIR}/include/gmxapi/mpi/multiprocessingresources.h +install(FILES ${CMAKE_CURRENT_BINARY_DIR}/include/gmxapi/mpi/resourceassignment.h DESTINATION include/gmxapi/mpi) diff --git a/api/gmxapi/cpp/CMakeLists.txt b/api/gmxapi/cpp/CMakeLists.txt index f9d5a98373..8c6fc2992d 100644 --- a/api/gmxapi/cpp/CMakeLists.txt +++ b/api/gmxapi/cpp/CMakeLists.txt @@ -35,7 +35,7 @@ # This list file provides the Gromacs::gmxapi cmake target. target_sources(gmxapi PRIVATE - multiprocessingresources.cpp + resourceassignment.cpp context.cpp exceptions.cpp gmxapi.cpp diff --git a/api/gmxapi/cpp/context.cpp b/api/gmxapi/cpp/context.cpp index 9769d03b84..903f3ce56d 100644 --- a/api/gmxapi/cpp/context.cpp +++ b/api/gmxapi/cpp/context.cpp @@ -67,7 +67,7 @@ #include "gromacs/utility/init.h" #include "gromacs/utility/smalloc.h" -#include "gmxapi/mpi/multiprocessingresources.h" +#include "gmxapi/mpi/resourceassignment.h" #include "gmxapi/exceptions.h" #include "gmxapi/session.h" #include "gmxapi/status.h" @@ -89,9 +89,14 @@ namespace gmxapi */ template using hasLibraryMpi = std::bool_constant; -using gmxThreadMpi = hasLibraryMpi; -using gmxLibMpi = hasLibraryMpi; -using MpiType = std::conditional_t; +/* Note that a no-MPI build still uses the tMPI headers to define MPI_Comm for the + * gmx::SimulationContext definition. The dispatching in this file accounts for + * these two definitions of SimulationContext. gmxThreadMpi here does not imply + * that the library was necessarily compiled with thread-MPI enabled. + */ +using gmxThreadMpi = hasLibraryMpi; +using gmxLibMpi = hasLibraryMpi; +using MpiType = std::conditional_t; using MpiContextInitializationError = BasicException; @@ -228,7 +233,7 @@ ContextImpl::ContextImpl(MpiContextManager&& mpi) noexcept(std::is_nothrow_const { // Confirm our understanding of the MpiContextManager invariant. GMX_ASSERT(mpi_.communicator() == MPI_COMM_NULL ? !GMX_LIB_MPI : GMX_LIB_MPI, - "Precondition is an appropriate communicator for the library environment."); + "Precondition violated: inappropriate communicator for the library environment."); // Make sure we didn't change the data members and overlook implementation details. GMX_ASSERT(session_.expired(), "This implementation assumes an expired weak_ptr at initialization."); diff --git a/api/gmxapi/cpp/context_impl.h b/api/gmxapi/cpp/context_impl.h index 2f57c1404e..4f9296d379 100644 --- a/api/gmxapi/cpp/context_impl.h +++ b/api/gmxapi/cpp/context_impl.h @@ -87,19 +87,25 @@ public: * Construct a valid instance with an appropriate default value for the * base communicator. (Note that appropriate default value depends on whether * the library was compiled with an external MPI library.) + * + * \throws BasicException if library cannot be initialized. */ MpiContextManager(); /*! - * \brief Borrow a communicator and ensure that MPI is initialized, if applicable. - * - * \param communicator Optional communicator representing a client-managed MPI environment. + * \brief Allow the communicator to be specified. * + * Supports use cases in which a client provides a non-default communicator. + * Ensures that library environment is properly initialized and finalized, + * whether or not an externally managed communicator has been provided. * + * \param communicator Optional communicator representing a client-managed MPI environment. * * Note that the communicator must be MPI_COMM_NULL if and only if GROMACS was built without an * external MPI library. * + * \throws BasicException if library cannot be initialized. + * * \todo (#3650?) Decide whether to exclude this from tMPI environments or find a sensible invariant. */ explicit MpiContextManager(MPI_Comm communicator); @@ -281,7 +287,18 @@ private: explicit ContextImpl(MpiContextManager&& mpi) noexcept(std::is_nothrow_constructible_v); }; - +/*! + * \brief Allow client code to hold a library communicator. + * + * MPI-enabled client code and thread-MPI GROMACS libraries cannot share headers + * in which MPI symbols (such as MPI_Comm) are defined. This class allows a + * client to refer to a library communicator opaquely. See gmxapi::offerComm(). + * + * In the initial gmxapi::ResourceAssignment implementation, CommHandle objects + * are the only possible recipients of assigned resources. If future implementations + * allow richer resource assignment, CommHandle may be superseded by a more + * elaborate interface. Otherwise, all we need is this simple type proxy. + */ class CommHandle { public: diff --git a/api/gmxapi/cpp/multiprocessingresources.cpp b/api/gmxapi/cpp/resourceassignment.cpp similarity index 98% rename from api/gmxapi/cpp/multiprocessingresources.cpp rename to api/gmxapi/cpp/resourceassignment.cpp index 2c2f107110..877ce94aa3 100644 --- a/api/gmxapi/cpp/multiprocessingresources.cpp +++ b/api/gmxapi/cpp/resourceassignment.cpp @@ -42,7 +42,7 @@ * \author "M. Eric Irrgang resources) * - * The client provides a communicator for work executed within the scope of the Context. - * Client remains responsible for freeing the communicator and finalizing the MPI environment. - * * The communicator resource type is a template parameter because MPI_Comm is commonly a C * typedef that varies between implementations, so we do not want to couple our * API to it, but we cannot forward-declare it. * * See also https://gitlab.com/gromacs/gromacs/-/issues/3650 + * + * \throws UsageError if the provided resource is not usable. */ template std::unique_ptr assignResource(CommT communicator) diff --git a/api/gmxapi/include/multiprocessingresources.cmakein.h b/api/gmxapi/include/resourceassignment.cmakein.h similarity index 97% rename from api/gmxapi/include/multiprocessingresources.cmakein.h rename to api/gmxapi/include/resourceassignment.cmakein.h index 8aa69d0906..420b5958b1 100644 --- a/api/gmxapi/include/multiprocessingresources.cmakein.h +++ b/api/gmxapi/include/resourceassignment.cmakein.h @@ -33,8 +33,8 @@ * the research papers on the package. Check out http://www.gromacs.org. */ -#ifndef GMXAPI_MULTIPROCESSINGRESOURCES_H -#define GMXAPI_MULTIPROCESSINGRESOURCES_H +#ifndef GMXAPI_RESOURCEASSIGNMENT_H +#define GMXAPI_RESOURCEASSIGNMENT_H /*! \file * \brief Provide build-specific overloads for client-MPI-dependent stuff. @@ -132,4 +132,4 @@ void offerComm(MPI_Comm src, CommHandle* dst); } // end namespace gmxapi -#endif // GMXAPI_MULTIPROCESSINGRESOURCES_H +#endif // GMXAPI_RESOURCEASSIGNMENT_H -- 2.22.0