From 09c3a1922300831e9b5f8894396ecec59fd67661 Mon Sep 17 00:00:00 2001 From: "M. Eric Irrgang" Date: Mon, 28 Sep 2020 15:24:03 +0300 Subject: [PATCH] Relocate MpiContextManager. * Change the scope of the MpiContextManager instance from the Session to the ContextImpl. * Tighten up some details to help maintain clarity over subsequent changes. Refs #3644 --- api/gmxapi/cpp/context.cpp | 45 +++++++++++++-- api/gmxapi/cpp/context_impl.h | 103 +++++++++++++++++++++++++++++----- api/gmxapi/cpp/session.cpp | 56 ------------------ api/gmxapi/cpp/session_impl.h | 21 +++---- 4 files changed, 137 insertions(+), 88 deletions(-) diff --git a/api/gmxapi/cpp/context.cpp b/api/gmxapi/cpp/context.cpp index 436ff90701..c1555579a7 100644 --- a/api/gmxapi/cpp/context.cpp +++ b/api/gmxapi/cpp/context.cpp @@ -54,15 +54,20 @@ #include "gromacs/commandline/pargs.h" #include "gromacs/commandline/filenm.h" #include "gromacs/commandline/pargs.h" +#include "gromacs/gmxlib/network.h" #include "gromacs/mdlib/stophandler.h" #include "gromacs/mdrunutility/logging.h" #include "gromacs/mdrunutility/multisim.h" #include "gromacs/mdrun/runner.h" #include "gromacs/mdrunutility/handlerestart.h" #include "gromacs/utility/arraysize.h" +#include "gromacs/utility/basenetwork.h" #include "gromacs/utility/fatalerror.h" +#include "gromacs/utility/gmxmpi.h" +#include "gromacs/utility/init.h" #include "gromacs/utility/smalloc.h" +#include "gmxapi/mpi/multiprocessingresources.h" #include "gmxapi/exceptions.h" #include "gmxapi/session.h" #include "gmxapi/status.h" @@ -76,15 +81,44 @@ namespace gmxapi { -ContextImpl::ContextImpl() +MpiContextManager::MpiContextManager() { + gmx::init(nullptr, nullptr); + GMX_RELEASE_ASSERT(!GMX_LIB_MPI || gmx_mpi_initialized(), + "MPI should be initialized before reaching this point."); +}; + +MpiContextManager::~MpiContextManager() +{ + // This is always safe to call. It is a no-op if + // thread-MPI, and if the constructor completed then the + // MPI library is initialized with reference counting. + gmx::finalize(); +} + +ContextImpl::~ContextImpl() = default; + +Context createContext() +{ + MpiContextManager contextmanager; + auto impl = ContextImpl::create(); + GMX_ASSERT(impl, "ContextImpl creation method should not be able to return null."); + auto context = Context(impl); + return context; +} + +ContextImpl::ContextImpl() noexcept(std::is_nothrow_constructible_v) : + mpi_(MpiContextManager()) +{ + // 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."); } -std::shared_ptr ContextImpl::create() +std::shared_ptr ContextImpl::create() { - std::shared_ptr impl = std::make_shared(); + std::shared_ptr impl; + impl.reset(new ContextImpl()); return impl; } @@ -164,14 +198,13 @@ std::shared_ptr ContextImpl::launch(const Workflow& work) ArrayRef multiSimDirectoryNames = opt2fnsIfOptionSet("-multidir", ssize(options_.filenames), options_.filenames.data()); - // Set up the communicator, where possible (see docs for - // SimulationContext). - MPI_Comm communicator = GMX_LIB_MPI ? MPI_COMM_WORLD : MPI_COMM_NULL; + // The SimulationContext is necessary with gmxapi so that // resources owned by the client code can have suitable // lifetime. The gmx wrapper binary uses the same infrastructure, // but the lifetime is now trivially that of the invocation of the // wrapper binary. + auto communicator = GMX_LIB_MPI ? MPI_COMM_WORLD : MPI_COMM_NULL; SimulationContext simulationContext(communicator, multiSimDirectoryNames); diff --git a/api/gmxapi/cpp/context_impl.h b/api/gmxapi/cpp/context_impl.h index 0b5f9a69b2..59acbaa67e 100644 --- a/api/gmxapi/cpp/context_impl.h +++ b/api/gmxapi/cpp/context_impl.h @@ -47,8 +47,6 @@ #include "gromacs/mdrun/legacymdrunoptions.h" #include "gromacs/mdtypes/mdrunoptions.h" -// Above are headers for dependencies. -// Following are public headers for the current module. #include "gmxapi/context.h" #include "gmxapi/session.h" @@ -56,26 +54,66 @@ namespace gmxapi { /*! - * \brief Context implementation base class. + * \brief Provide RAII management of communications resource state. * - * Execution contexts have a uniform interface specified by the API. Implementations for - * particular execution environments can specialize / derive from this base. + * To acquire an MpiContextManager is to have assurance that any external MPI + * environment is ready to use. When the MpiContextManager is released or + * goes out of scope, the destructor finalizes the resources. + * + * Note that thread-MPI chooses the number of ranks and constructs its + * MPI communicator internally, so does not and is unlikely to ever + * participate here. + * + * \todo There is no resource for logging or reporting errors during initialization * - * \todo Separate interface and implementation. * \ingroup gmxapi */ -class ContextImpl final : public std::enable_shared_from_this +class MpiContextManager { public: + MpiContextManager(); + + ~MpiContextManager(); + /*! - * \brief Default constructor. + * \brief Exclusive ownership of a scoped context means copying is impossible. * - * Don't use this. Use create() to get a shared pointer right away. - * Otherwise, shared_from_this() is potentially dangerous. + * \{ + */ + MpiContextManager(const MpiContextManager&) = delete; + MpiContextManager& operator=(const MpiContextManager&) = delete; + //! \} + + /*! + * \brief Move semantics are non-trivial. * - * \todo Make default constructor private or otherwise reduce brittleness of construction. + * \{ */ - ContextImpl(); + MpiContextManager(MpiContextManager&&) noexcept = delete; + MpiContextManager& operator=(MpiContextManager&&) noexcept = delete; + //! \} +}; + +/*! + * \brief Context implementation. + * + * Execution contexts have a uniform interface specified by the API. Implementations for + * particular execution environments can specialize / derive from this base. + * + * \todo Separate interface and implementation. + * + * \warning Definition and semantics depend on configure-time details. For example, + * MPI-enabled libraries always hold a valid MPI communicator via MpiContextManager, + * whereas tMPI and non-MPI builds hold a meaningless MpiContextManager. + * + * \todo Provide functions or traits for introspection. + * + * \ingroup gmxapi + */ +class ContextImpl final : public std::enable_shared_from_this +{ +public: + ~ContextImpl(); /*! * \brief Factory function @@ -84,10 +122,12 @@ public: * that it never exists without a shared_ptr owning it. * * If we can confirm `shared_from_this` is no longer necessary, implementation may change. + * \todo: Use registration/deregistration of launched Sessions to log warnings on shutdown + * instead of letting Session keep ContextImpl alive. * * \return ownership of a new object */ - static std::shared_ptr create(); + static std::shared_ptr create(); /*! * \brief Copy disallowed because Session state would become ambiguous. @@ -103,7 +143,9 @@ public: /*! * \brief Objects are not trivial to move. * - * \todo Implement move semantics. + * \todo Implement move semantics. Requires a moveable const MpiContextManager and + * LegacyMdrunOptions members. + * * \{ */ ContextImpl(ContextImpl&&) = delete; @@ -123,6 +165,8 @@ public: * are still evolving. * \todo Hide lifetime management and ownership from handle object. * We can achieve the necessary aspects of this shared_ptr at a lower level of implementation. + * Note also that returned value policies can be implemented in higher level wrappers to ensure + * correct object lifetime scope. (See pybind, for instance.) */ std::shared_ptr launch(const Workflow& work); @@ -131,6 +175,9 @@ public: * * The client owns the Session launched by a Context, but it is helpful * for the Context to know if it has an active Session associated with it. + * + * \todo Use registration/deregistration protocol instead. + * Requires logging facility. */ std::weak_ptr session_; @@ -152,6 +199,34 @@ public: * duplicate definitions e.g. of command-line options. */ gmx::LegacyMdrunOptions options_; + + /*! + * \brief Scoped MPI management. + * + * Part of the ContextImpl invariant establishes a point where MPI initialization status is + * known. + * + * To ensure the MpiContextManager is initialized only once, we use a const member that must + * be initialized at construction. + */ + const MpiContextManager mpi_; + +private: + /*! + * \brief Basic constructor. + * + * Don't use this. Use create() to get a shared pointer right away. + * Otherwise, shared_from_this() is potentially dangerous. + */ + explicit ContextImpl() noexcept(std::is_nothrow_constructible_v); +}; + + +class CommHandle +{ +public: + using commType = MPI_Comm; + MPI_Comm communicator{ MPI_COMM_NULL }; }; } // end namespace gmxapi diff --git a/api/gmxapi/cpp/session.cpp b/api/gmxapi/cpp/session.cpp index 5a9d0f274a..2b909663b0 100644 --- a/api/gmxapi/cpp/session.cpp +++ b/api/gmxapi/cpp/session.cpp @@ -57,60 +57,6 @@ namespace gmxapi { -/*! - * \brief Provide RAII management of communications resource state. - * - * To acquire an MpiContextManager is to have assurance that any external MPI - * environment is ready to use. When the MpiContextManager is released or - * goes out of scope, the destructor finalizes the resources. - * - * Note that thread-MPI chooses the number of ranks and constructs its - * MPI communicator internally, so does not and is unlikely to ever - * participate here. - * - * \todo There is no resource for logging or reporting errors during initialization - * \todo Remove this class by managing the MPI context with mpi4py and so - * configuring the SimulationContext externally - * - * \ingroup gmxapi - */ -class MpiContextManager -{ -public: - MpiContextManager() - { - gmx::init(nullptr, nullptr); - GMX_RELEASE_ASSERT(!GMX_LIB_MPI || gmx_mpi_initialized(), - "MPI should be initialized before reaching this point."); - }; - - ~MpiContextManager() - { - // This is always safe to call. It is a no-op if - // thread-MPI, and if the constructor completed then the - // MPI library is initialized with reference counting. - gmx::finalize(); - } - - /*! - * \brief Exclusive ownership of a scoped context means copying is impossible. - * - * \{ - */ - MpiContextManager(const MpiContextManager&) = delete; - MpiContextManager& operator=(const MpiContextManager&) = delete; - //! \} - - /*! - * \brief Move semantics are trivial. - * - * \{ - */ - MpiContextManager(MpiContextManager&&) noexcept = default; - MpiContextManager& operator=(MpiContextManager&&) noexcept = default; - //! \} -}; - SignalManager::SignalManager(gmx::StopHandlerBuilder* stopHandlerBuilder) : state_(std::make_shared(gmx::StopSignal::noSignal)) { @@ -185,12 +131,10 @@ SessionImpl::SessionImpl(std::shared_ptr context, gmx::SimulationContext&& simulationContext, gmx::LogFilePtr fplog) : context_(std::move(context)), - mpiContextManager_(std::make_unique()), simulationContext_(std::move(simulationContext)), logFilePtr_(std::move(fplog)) { GMX_ASSERT(context_, "SessionImpl invariant implies valid ContextImpl handle."); - GMX_ASSERT(mpiContextManager_, "SessionImpl invariant implies valid MpiContextManager guard."); // \todo Session objects can have logic specialized for the runtime environment. diff --git a/api/gmxapi/cpp/session_impl.h b/api/gmxapi/cpp/session_impl.h index 9d07ac7cb2..3d9699e5cf 100644 --- a/api/gmxapi/cpp/session_impl.h +++ b/api/gmxapi/cpp/session_impl.h @@ -60,9 +60,8 @@ namespace gmxapi { // Forward declaration -class MpiContextManager; // Locally defined in session.cpp -class ContextImpl; // locally defined in context.cpp -class SignalManager; // defined in mdsignals_impl.h +class ContextImpl; // locally defined in context.cpp +class SignalManager; // defined in mdsignals_impl.h /*! * \brief Implementation class for executing sessions. @@ -202,18 +201,12 @@ private: * \brief Extend the life of the owning context. * * The session will get handles for logging, UI status messages, - * and other facilities through this interface. + * and other facilities through this interface. This is a facility + * provided by the client to the Session implementation during + * Context.launch(). */ std::shared_ptr context_; - /*! - * \brief RAII management of gmx::init() and gmx::finalize() - * - * Uses smart pointer to avoid exposing type definition. - * \todo Not fully implemented. - */ - std::unique_ptr mpiContextManager_; - /*! * \brief Simulation runner object. * @@ -224,6 +217,10 @@ private: /*! * \brief An active session owns the resources it is using. + * + * This encapsulate details of the run time context that the + * Session makes available to the simulator, tied to the + * lifetime of the Session. */ gmx::SimulationContext simulationContext_; -- 2.22.0