Skip to content

MPI ready GooFit with Application Object #36

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 63 commits into from
Mar 2, 2017
Merged

MPI ready GooFit with Application Object #36

merged 63 commits into from
Mar 2, 2017

Conversation

henryiii
Copy link
Member

@henryiii 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 29 commits January 29, 2017 22:35
@hassec
Delete .gitmodules
72daea5 
This is a relic from, the integration of MCBooster.
As we don't use gitmodules we can get rid of this.
@henryiii
Merge remote-tracking branch 'origin/dev' into dev
da0d648
@henryiii
Starting App for goofit
4b1c242
@henryiii
Adding original constructor back
9c7da7e
@henryiii
Trying an example with GooFit::Application
5bf58cf
@henryiii
Adding update scripts, along with latest update
1f9cfe6
@henryiii
Adding usage of Application
91807dc
@henryiii
Move to CLI for most examples
a62a6da
@henryiii
Moved to latest CLI, rough unworking draft
6e45f33
@henryiii
Fixes to get compile working
ec05089
@henryiii
Improvents to CLI with subcommands
fb07475
@henryiii
Clean up formatting
1102056
@henryiii
Updating download scripts
ce110dd
@henryiii
Prepared to run new version
ca5a9c6
@henryiii
Fix for typo in toy fit
e716924
@henryiii
Adding TApplication style Application
a1d91a9
@henryiii
Fix run inherited method
3727785
@henryiii
Update travis.sh for multi threading
2f8bfab
@henryiii
Fix Caps error
93e6746
@henryiii
Removing Google Mock for now
547a343
@henryiii
Cleaning up Convolution and Product examples
103c61f
@henryiii
Moved to CLI v0.1
43d4007
@henryiii
Moving to newer version of CLI11
734af82
@henryiii
Fixing download for redirect, adding v0.2 (actual)
243c1eb
@henryiii
Fix typo
3c10ee2
@henryiii
Fixing remaining Default flag
dabe0df
@henryiii
Adding slightly newer version of CLI11
756ff0b
@henryiii
Fixed to use constructor again
bdf151a
@henryiii
Adding exit to print devices, updated zachfit
55d70a6
@henryiii
Copy link
Member Author

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
Copy link
Member Author

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 March 2, 2017 20:00
@henryiii henryiii added this to the V 2.0 milestone Mar 2, 2017
galapaegos
galapaegos approved these changes Mar 2, 2017
View reviewed changes
Copy link
Contributor

@galapaegos galapaegos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 March 2, 2017 20:42
@henryiii henryiii merged commit 1b7eafd into master Mar 2, 2017
@henryiii
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

henryiii commented Mar 2, 2017 •
edited
Loading

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
Reviewers

@galapaegos galapaegos galapaegos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
V 2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@henryiii @galapaegos @hassec