GENIE-MC / Generator

The popular GENIE Generator product is used by nearly all accelerator neutrino experiments and it plays a key role in the exploitation of neutrino data. The Generator implements a modern software framework and it includes state-of-the-art physics modules. It captures the latest results of the GENIE global analysis of neutrino scattering data and includes several tunes that were produced using the proprietary Comparisons and Tuning products. The GENIE physics model is universal and comprehensive: It handles all neutrinos and targets, and all processes relevant from MeV to PeV energy scales. The Generator includes several tools (flux drivers, detector geometry navigators, specialized event generation apps, event reweighting engines) to simulate complex experimental setups in full detail and to support generator-related analysis tasks.
http://www.genie-mc.org
42 stars 92 forks source link

[WIP] Argument Parsing #322

Closed 8me closed 9 months ago

8me commented 1 year ago

In order not to reinvent the wheel I suggest to use libraries for some packaging stuff; in this case why not just use boost for getting the program arguments parsed.

Marked as WIP, as not everything is migrated yet. gEvGen was modified as an example how it could look like and to discuss this on :wink:

candreop commented 1 year ago

Wow, this was super fast Johannes! We only discussed this a couple of hrs ago! The main point here is the boost dependency: It may be something we want to do, and there could be opportunities to use even more tools from boost. We need a cost-benefits analysis. I am sure @mroda88 and @nusense have informed opinions about this.

mroda88 commented 1 year ago

Boost is a solid library, I used it for years. For us it could be useful also to write integration tests. So nothing against its choice. The question is: do we want to add this as a dependency? This cannot be an optional dependency like INCL. For people using genie on machines with cvmfs it's an easy addition. For those who run in other systems it might introduce a problem. We can ask at the forum. Although I'm not sure we reach those people via the forum.

candreop commented 1 year ago

Perhaps we should decide what we want. What else is there in boost that we may want to use? You already mentioned one possibility. Then we can make an informed choice by ourselves. Perhaps at the next core mtg? We can listen to users, but… , we know that many will always resist to change and prefer they convenience of established ways. I do not see any user enthusiastically supporting this. I guess, opinions will range from negative to neutral. In the end, they don’t have to maintain GENIE. They will have to install boost, but see little benefit for it (unlike the developers).

nusense commented 1 year ago

I think the additional requirement for "boost" might be a bit of a heavy-ish lift for some users -- on the other hand, it comes essentially for free for those in the LArSoft / NOvASoft environments because that's already a product that's supplied with those for other reasons. But is there more to "boost" that we want to make use of than command line argument handling? @mroda88 mentioned unit/integration testing that that sounds interesting but historically those rarely pan out in many projects (after an initial start).

Here I also think we need to note that currently some of the command line argument handling is not done by the individual applications' void GetCommandLineArgs(int argc, char ** argv) , but instead by Framework/RunOpt where some common options are dealt with (notably EventGeneratorList and Tune) so that would need adjustments as well for consistency. Can po::store(po::parse_command_line(argc, argv, desc), vm) be run more than once? If not then we'd need to re-factor this factorization. Also, is there a means of marking some options "required" -- to be throw some kind of exception/usage message if not set?

Also the Make.include modification aren't sufficient for cases where boost isn't installed in /usr/lib but instead elsewhere (such as the case when using UPS versions).

But as a proof-of-principle this is an intriguing start.

8me commented 1 year ago

@nusense

Looking this all over, it's an interesting proposal. Is there a "boost" helper function that takes the desc.add_options() list and pretty prints it for the case of --help and/or cases where required args aren't set (e.g. -p / --neutrino_pdg ) aren't set.

The help section should be covered by:

if (vm.count("help")) {
    LOG("gevgen", pNOTICE) << desc << "\n";
    exit(-1);
  }
8me commented 1 year ago

I totally understand that a additional requirement is a big discussion for a C++ project, but as boost provides some more very helpful functions one could use and with handing the maintenance to others :wink:

(I know this opens a whole new chapter but maybe it could be a way to go to recommend docker usage, in this case not providing an official image which is quite hard to do with the possible combinations of dependencies but by providing an example docker file with the "most common" dependencies in it. So the users could adapt it easily and don't really have to deal with the most basics,)

nusense commented 1 year ago

The help section should be covered by:

if (vm.count("help")) {
    LOG("gevgen", pNOTICE) << desc << "\n";
    exit(-1);
  }

Ah, I guess I did see that and didn't appreciate what it entailed. Could you show what that would actually generate as I'm curious.

Additionally, this would be an incomplete list with the way RunOpt splits out some of the common command line flags (again e.g. EventGeneratorList and Tune). So there's some thought that needs to be put into it to refactorize that handling so it's more coherent.

8me commented 1 year ago

So the help output would then read like:

1693108229 NOTICE gevgen : [n] <gEvGen.cxx::GetCommandLineArgs (679)> : gEvGen:
  -h [ --help ]                       print help message
  -n [ --events ] arg (=0)            Number of events to be generated
  -r [ --run ] arg (=0)               MC run number
  -e [ --energy ] arg                 Energy (range) to be used for the 
                                      generation
  -p [ --neutrino_pdg ] arg           PDG of the initial neutrino
  -t [ --target ] arg                 Scattering target
  -f [ --flux ] arg                   Flux description
  -F [ --flux_factors ] arg           Flux factors
  -o [ --outfile ] arg (=output.root) Output file
  -w [ --weighted ]                   Generate weighted
  --force-flux-ray-interaction 
  -s [ --seed ] arg (=-1)             Rnd seed
  -x [ --cross-sections ] arg         Cross section (xml) input file

... I'm about to look what would be a nice solution for the RunOpt stuff

8me commented 1 year ago

@nusense finally I had time to look into this for RunOpt which has it's own po basen arg parsing now ... the only disadvantage now is that I had to ignore unkown args in order to be able to do 2 different parsings from the same arguments. (Before there was an exception if there is a typo on the args)