Feature #702

Introducing gmx_cmdline_t

Added by Mark Abraham almost 10 years ago. Updated over 7 years ago.

Target version:


There are lots of mdrun command line arguments, which makes for a growing morass of function arguments that have to flow through mdrunner() and maybe the actual integrator before taking effect. This clutters up name-spaces and makes for maintenance issues. For examples,
  • There are different labels for the same quantity in different places.
  • Why should someone implementing a new EM "integrator" have to be bothered working out that they can ignore repl_ex_nst and bCompact even though they have to have them as function parameters?
  • A developer in init_domain_decomposition() shouldn't have to trail back through four functions to learn that the sizex variable is derived from the hidden mdrun option -ddcsx.

Instead, using an object to contain all such variables derived from command-line arguments means that in adding new command-line arguments one only has to touch mdrun.c, the object header file and the place where it is used. This is rather like the use of t_inputrec, of course.

This will also make life easier creating and maintaining derivative works from mdrun.

To illustrate the new code, in include/types/cmdline.h we have

/** \file include/types/cmdlinerec.h                                                                       
 *  \brief Structure for mdrun command-line arguments                                                      
 *  This structure packages the command-line arguments to mdrun to                                         
 *  enable them to be passed into the lower-level routines in a way                                        
 *  that enhances future maintainability better than the old style of                                      
 *  passing many different arguments to many different functions. In                                       
 *  particular, maintaining several mdrun-work-alike utility programs                                     
 *  was becoming a nightmare.                                                                              
 *  \param fnm                   Structure for managing input filenames                                    
 *  \param nfile                 Size of the fnm structure                                                 
 *  \param oenv                  Controls the output environment                                           
 *  \param nstglobalcomm         Number of steps between global communication when running in parallel     
 *  \param dd_ncells             Number of DD cells in each direction                                      
 *  \param dd_node_order         Controls the mapping of MPMD processes to hardware when using DD-PME      
 *  \param dd_comm_distance_min  Maximum distance over which bonded interactions can occur when using DD   
 *  \param rconstr_max           Maximum distance over which P-LINCS constraints can act when using DD     
 *  \param dd_dlb_opt            Controls the use of dynamic load balancing with DD                        
 *  \param dd_dlb_scale          Minimum allowed scaling of the DD cell size with dynamic load balancing   
 *  \param dd_cell_size          DD cell sizes for static load balancing                                   
 *  \param nstepout              Number of steps between writing remaining runtime, etc.                   
 *  \param resetstep             Step at which to reset performance counters                               
 *  \param repl_ex_nst           Number of steps between replica-exchange attempts                         
 *  \param repl_ex_seed          Replica-exchange random-number generator seed                             
 *  \param pforce                Minimum force that will be dumped to stderr when debugging                
 *  \param cpt_period            Minutes between writing of checkpoint file                                
 *  \param max_hours             Terminate after 0.99 of this time (hours)                                 
 *  \param deviceOptions         Device option string                                                      

typedef struct
    t_filenm *fnm;
    int nfile;
    output_env_t oenv;
    int nstglobalcomm;
    ivec dd_ncells;
    int dd_node_order;
    real dd_comm_distance_min;
    real rconstr_max;
    const char *dd_dlb_opt;
    real dd_dlb_scale;
    char *dd_cell_size[DIM];
    int nstepout;
    int resetstep;
    int repl_ex_nst;
    int repl_ex_seed;
    real pforce;
    real cpt_period;
    real max_hours;
    const char *deviceOptions;
} gmx_cmdlinerec;

/* We can't actually have an abstract type, but for consistency here's                                     
 * a similar typedef */
typedef gmx_cmdlinerec *gmx_cmdlinerec_t;

In main() we now have a more crisp mdrunner() function call

  rc = mdrunner(nthreads, fplog, cr, cmdlinerec, Flags);

and, similarly, do_md() is now declared with

do_md(FILE *fplog,t_commrec *cr,
      gmx_cmdlinerec_t cmdlinerec,
      gmx_vsite_t *vsite,
      gmx_constr_t constr,
      t_inputrec *ir,
      gmx_mtop_t *top_global,
      t_fcdata *fcd,
      t_state *state_global,
      t_mdatoms *mdatoms,
      t_nrnb *nrnb,
      gmx_wallcycle_t wcycle,
      gmx_edsam_t ed,
      t_forcerec *fr,
      unsigned long Flags,
      gmx_runtime_t *runtime)

I've tested the new implementation in serial, MPI and threads in comparison with 4.5.3, and everything looks fine. I'm no threading expert, but I believe what I've done in src/kernel/runner.c is equivalent to what used to be there, and thus should be thread-safe. I can't test the OpenMM version, but I believe it will be fine, also.

One downside might be the need to use code like

  if (opt2bSet("-cpi",cmdlinerec->nfile,cmdlinerec->fnm))

which is fairly cumbersome. This will all go away once we can use C++ to say
  if (command_line.input_files.opt2bSet("-cpi"))

(or even if we just repackage t_filenm suitably in C). In some functions, I have made function-local copies of nfile, fnm and bVerbose as temporary convenience measures.

I've based this branch off release-4-5-patches, and am happy to roll it back into that branch if/when people are happy with it. Of course, anybody doing derivative work is likely to have a one-time merge to resolve, but I think the cost now will pay back in the future.

I have done a quick hack to make g_membed work (I had changed the argument list for init_domain_decomposition(), so had to make a corresponding change in src/tools/gmx_membed.c). I'm not inclined to do more, since it's already out of date with release-4-5-patches, and already has its own custom versions of the main()-<blah>-mdrunner()-do_md() hierarchy.

Associated revisions

Revision dd570916 (diff)
Added by Mark Abraham almost 10 years ago

Introduced gmx_cmdline_t

There are lots of mdrun command line arguments, which makes for
a growing morass of function arguments that have to flow through
mdrunner() and maybe the actual integrator before taking effect.
This clutters up namespaces and makes for maintenance issues.

Instead, using an object to contain them all means that adding
new command-line options only has to touch mdrun.c, the object
header file and the place where it is used. This will also make
life easier creating and maintaining derivative works.

IssueID #702


#1 Updated by Teemu Murtola almost 10 years ago

These parts are likely to undergo larger changes during the move towards a more object-oriented architecture that is now (slowly) starting to take place in master. In medium- to long-term, it would probably be good to move to use the options implementation from src/gromacs/options/ for all command-line parsing (as well as for mdp input), which would mean that modules that get input from command-line options would also define the options themselves, removing the burden of maintaining the options separately and passing them around in code that doesn't care about them at all. This does not solve the problem completely if more than one module needs to use the value of an option, but in that case there should probably be a container object that both stores the option values and defines the options, in spirit to what Mark has now implemented.

#2 Updated by Rossen Apostolov over 9 years ago

  • Target version deleted (git master)

#3 Updated by Teemu Murtola about 8 years ago

  • Target version set to 5.0

Should either be included as part of 5.0, or closed if we decide to use some other approach together with the current C++ options implementation. The linked 2-year-old implementation is likely outdated by 4.6 development.

#4 Updated by David van der Spoel about 8 years ago

This should definitely be done in C++. It may therefore be simpler to do it from scratch based on the options approach (although I admit I have not looked into that), but in order to reduce complexity it would be good to reuse as much as possible.

#5 Updated by Mark Abraham over 7 years ago

  • Status changed from New to Rejected

Yes, outdated by now. Can't imagine this would be useful now.

Also available in: Atom PDF