New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MPI ready GooFit with Application Object #36

Merged
merged 63 commits into from Mar 2, 2017

Conversation

Projects
None yet
3 participants
@henryiii
Copy link
Member

henryiii commented Feb 7, 2017

This MR adds an application object to GooFit. These is based on my CLI11 library, and syntax for it in GooFit is based on ROOT's TApplication object:

#include "goofit/Application.h"
GooFit::Application app("description", argc, argv);

// Add options
int value;
app.add_option("-v,--value", value, "Description of value")->required();

// Parse
try{
    app.run();
} catch(GooFit::ParseError &e) {
    return app.exit(e);
};

FindMPI and MPI setup/teardown are included. This also adds default command line arguments:

$ ./examples/addition/addition -h
Addition example
Usage: ./examples/addition/addition [OPTIONS...]

Options:
  -h,--help                   Print this help message and exit
  --gpu-dev INT=0             GPU device to use
  --show-gpus                 Show the available GPU devices and exit
  --goofit-details            Output system and threading details

To see a full-scale usage of this, look at the pipipi0 example. The syntax for calling that one has changed, please see the help. All other examples were directly converted. Two examples (multiply and convolution) were also cleaned up to remove memory leakage.

Comments and suggestions on both this and the CLI11 library are appreciated. Once GooFit hits version 2.0, I'll also version CLI11 to 1.0. This PR address #33.

hassec and others added some commits Jan 29, 2017

Delete .gitmodules
This is a relic from, the integration of MCBooster.
As we don't use gitmodules we can get rid of this.
@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented Feb 7, 2017

@galapaegos, please see if this fills your requirements for MPI setup/teardown.
@hassec, please check the reworked examples
Note that zachFit command line has changed

henryiii added some commits Feb 20, 2017

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented Mar 2, 2017

Ready for review, this is becoming too important to live outside of master.

@henryiii henryiii requested a review from hassec Mar 2, 2017

@henryiii henryiii added this to the V 2.0 milestone Mar 2, 2017

henryiii added some commits Mar 2, 2017

@galapaegos
Copy link
Contributor

galapaegos left a comment

Looks good. A few things I noticed that will need to be updated for a future commit:

  • CountingVariable needs to be used for any Variable that does number tracking and referenced as an event number. I have the fixes for these in my commits.
  • MPI_CXX_COMPILE_FLAGS isn't passed properly to the generated Makefile. Adding 'set (CMAKE_CXX_FLAGS "-DGOOFIT_MPI")' fixes the issue, though this may be done differently?
  • I think setting the device with the Application object will need to be done prior to MPI calls, and we will overwrite this value based on the number of MPI processes and GPU's, but allow an override for 1 process to set the device.

@henryiii henryiii removed the request for review from hassec Mar 2, 2017

@henryiii henryiii merged commit 1b7eafd into master Mar 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented Mar 2, 2017

Sorry about the flags, I don't have any real MPI code in the branch, so didn't catch that.

Do you mean the MPI_Init call, or the first MPI calls?

@galapaegos

This comment has been minimized.

Copy link
Contributor

galapaegos commented Mar 2, 2017

No worries!

The flag being set '-DGOOFIT_MPI' is correct. MPI_CXX_COMPILE_FLAGS is not being included when the compilation actually happens, I think that is the issue. When I turned on the verbosity to show the compilation flags, I did not see the '-DGOOFIT_MPI' flag being passed.

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented Mar 2, 2017

I think MPI_CXX_COMPILE_FLAGS was breaking the add_def command, so the final part wasn't being included. Did you check the new PR to see if that method fixes it?

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented Mar 2, 2017

I believe you've added CountVariable; could you make a PR with at least that portion so I can start moving to using it? Even if it's just something like:

class CountVariable : public Variable {
    using Variable::Variable;
};

@henryiii henryiii modified the milestone: V 2.0 Jun 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment