From: M. Eric Irrgang Date: Sat, 26 Sep 2020 14:21:00 +0000 (+0300) Subject: Clarify behavior of LegacyMdrunOptions. X-Git-Url: http://biod.pnpi.spb.ru/gitweb/?a=commitdiff_plain;h=786d35fc9baace14845787f12fb1c47ed0d28cfc;p=alexxy%2Fgromacs.git Clarify behavior of LegacyMdrunOptions. Make notes about the lifetime management of LegacyMdrunOptions and its members. Note effects on the way it must be used by gmxapi::ContextImpl. Clarify how gmxapi::ContextImpl could be updated in the future if data scope details of LegacyMdrunOptions evolve. Relates to #3644, #3688, #3718 --- diff --git a/api/gmxapi/cpp/context.cpp b/api/gmxapi/cpp/context.cpp index 903f3ce56d..1b5cb813d1 100644 --- a/api/gmxapi/cpp/context.cpp +++ b/api/gmxapi/cpp/context.cpp @@ -315,13 +315,18 @@ std::shared_ptr ContextImpl::launch(const Workflow& work) auto mdModules = std::make_unique(); const char* desc[] = { "gmxapi placeholder text" }; - if (options_.updateFromCommandLine(argc, argv.data(), desc) == 0) + + // LegacyMdrunOptions needs to be kept alive for the life of ContextImpl, + // so we use a data member for now. + gmx::LegacyMdrunOptions& options = options_; + if (options.updateFromCommandLine(argc, argv.data(), desc) == 0) { return nullptr; } ArrayRef multiSimDirectoryNames = - opt2fnsIfOptionSet("-multidir", ssize(options_.filenames), options_.filenames.data()); + opt2fnsIfOptionSet("-multidir", ssize(options.filenames), options.filenames.data()); + // The SimulationContext is necessary with gmxapi so that // resources owned by the client code can have suitable @@ -335,39 +340,39 @@ std::shared_ptr ContextImpl::launch(const Workflow& work) SimulationContext simulationContext(communicator, multiSimDirectoryNames); - StartingBehavior startingBehavior = StartingBehavior::NewSimulation; - LogFilePtr logFileGuard = nullptr; - gmx_multisim_t* ms = simulationContext.multiSimulation_.get(); - std::tie(startingBehavior, logFileGuard) = - handleRestart(findIsSimulationMasterRank(ms, simulationContext.simulationCommunicator_), - communicator, ms, options_.mdrunOptions.appendingBehavior, - ssize(options_.filenames), options_.filenames.data()); + StartingBehavior startingBehavior = StartingBehavior::NewSimulation; + LogFilePtr logFileGuard = nullptr; + gmx_multisim_t* ms = simulationContext.multiSimulation_.get(); + std::tie(startingBehavior, logFileGuard) = handleRestart( + findIsSimulationMasterRank(ms, simulationContext.simulationCommunicator_), + simulationContext.simulationCommunicator_, ms, options.mdrunOptions.appendingBehavior, + ssize(options.filenames), options.filenames.data()); auto builder = MdrunnerBuilder(std::move(mdModules), compat::not_null(&simulationContext)); - builder.addSimulationMethod(options_.mdrunOptions, options_.pforce, startingBehavior); - builder.addDomainDecomposition(options_.domdecOptions); + builder.addSimulationMethod(options.mdrunOptions, options.pforce, startingBehavior); + builder.addDomainDecomposition(options.domdecOptions); // \todo pass by value - builder.addNonBonded(options_.nbpu_opt_choices[0]); + builder.addNonBonded(options.nbpu_opt_choices[0]); // \todo pass by value - builder.addElectrostatics(options_.pme_opt_choices[0], options_.pme_fft_opt_choices[0]); - builder.addBondedTaskAssignment(options_.bonded_opt_choices[0]); - builder.addUpdateTaskAssignment(options_.update_opt_choices[0]); - builder.addNeighborList(options_.nstlist_cmdline); - builder.addReplicaExchange(options_.replExParams); + builder.addElectrostatics(options.pme_opt_choices[0], options.pme_fft_opt_choices[0]); + builder.addBondedTaskAssignment(options.bonded_opt_choices[0]); + builder.addUpdateTaskAssignment(options.update_opt_choices[0]); + builder.addNeighborList(options.nstlist_cmdline); + builder.addReplicaExchange(options.replExParams); // Need to establish run-time values from various inputs to provide a resource handle to Mdrunner - builder.addHardwareOptions(options_.hw_opt); + builder.addHardwareOptions(options.hw_opt); // \todo File names are parameters that should be managed modularly through further factoring. - builder.addFilenames(options_.filenames); + builder.addFilenames(options.filenames); // TODO: Remove `s` and `-cpi` from LegacyMdrunOptions before launch(). #3652 - auto simulationInput = makeSimulationInput(options_); + auto simulationInput = makeSimulationInput(options); builder.addInput(simulationInput); // Note: The gmx_output_env_t life time is not managed after the call to parse_common_args. // \todo Implement lifetime management for gmx_output_env_t. // \todo Output environment should be configured outside of Mdrunner and provided as a resource. - builder.addOutputEnvironment(options_.oenv); + builder.addOutputEnvironment(options.oenv); builder.addLogFile(logFileGuard.get()); // Note, creation is not mature enough to be exposed in the external API yet. diff --git a/src/gromacs/mdrun/legacymdrunoptions.h b/src/gromacs/mdrun/legacymdrunoptions.h index 796e479490..474f6f0396 100644 --- a/src/gromacs/mdrun/legacymdrunoptions.h +++ b/src/gromacs/mdrun/legacymdrunoptions.h @@ -3,7 +3,7 @@ * * Copyright (c) 1991-2000, University of Groningen, The Netherlands. * Copyright (c) 2001-2004, The GROMACS development team. - * Copyright (c) 2011-2019, by the GROMACS development team, led by + * Copyright (c) 2011-2020, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -68,13 +68,19 @@ namespace gmx * support for the CLI and API without duplicating code. It should be * eliminated following the TODOs below. * + * \warning Instances provide lifetime scope for members that do not have + * effective lifetime management or which are frequently accessed unsafely. + * The caller is responsible for keeping a LegacyMdrunOptions object alive + * for as long as any consumers, direct or transitive. + * * \todo Modules in mdrun should acquire proper option handling so - * that all of these declarations and defaults are local to the - * modules. + * that all of these declarations and defaults are local to the + * modules. * * \todo Contextual aspects, such as working directory - * and environment variable handling are more properly - * the role of SimulationContext, and should be moved there */ + * and environment variable handling are more properly + * the role of SimulationContext, and should be moved there. + */ class LegacyMdrunOptions { public: