Skip to content
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

Create gridpack::environment with existing MPI communicator #119

Merged
merged 31 commits into from
Dec 13, 2023

Conversation

wperkins
Copy link
Member

This branch adds a facility to create a gridpack::environment with an existing MPI communicator. This allows an external application to call MPI_Initialize() and instantiate multiple GridPACK environments on an externally distribution of MPI ranks.
NOTE: This GridPACK branch depends on a specific branch of Global Arrays (feature/init-comm). This PR should not be accepted until the necessary changes have been merged into the GA master branch (and the GridPACK GA submodule is updated and actually works).

@wperkins wperkins self-assigned this Dec 14, 2022
@wperkins wperkins marked this pull request as draft December 14, 2022 16:33
@wperkins wperkins requested a review from bjpalmer December 14, 2022 16:34
@abhyshr
Copy link
Collaborator

abhyshr commented Oct 7, 2023

  1. This branch needs to resolve the conflicts first.
  2. Don't understand what is the additional functionality being added here. Is the ability to set an MPI_Comm with GridPACK's environment object & being able to set it via Python?

@wperkins
Copy link
Member Author

2. Don't understand what is the additional functionality being added here. Is the ability to set an MPI_Comm with GridPACK's environment object & being able to set it via Python?

GridPACK expects to control MPI, that is GridPACK calls MPI_Init() and MPI_Finalize() to set up an environment. The changes in this PR simply allows a GridPACK environment to be created with an externally created MPI_Comm. And, not just in Python.

This was set up for the ExaLearn folks, who drive multiple GridPACK instances with an MPI-based driver.

@wperkins
Copy link
Member Author

  1. This branch needs to resolve the conflicts first.

Agreed. It should be noted, however, that this will require reconciling the HADREC application with current dynamic simulation API without alienating the ExaLearn application of HADREC. I do not know details.

@wperkins wperkins added this to the GridPACK 3.5 Release milestone Oct 12, 2023
@wperkins
Copy link
Member Author

Note to self: The value of the CMake variable ENABLE_ENVIRONMENT_FROM_COMM needs to be in GridPACK.cmake and the Python interface adjusted to allow ENABLE_ENVIRONMENT_FROM_COMM=FALSE.

Environment::~Environment(void)
{
// Finalize math libraries
gridpack::math::Finalize();
if (p_from_comm) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note the artifacts from rebase.
I don't understand the destructor. It seems to me that gridpack::math::Finalize(); and GA_Terminate() should be called no matter the state of p_from_comm.

@@ -198,4 +215,11 @@ install(TARGETS dsf2.x DESTINATION bin)
# run application as test
# -------------------------------------------------------------
gridpack_add_run_test("dynamic_simulation_full_y" dsf.x input_145.xml)
<<<<<<< HEAD
Copy link
Member Author

Choose a reason for hiding this comment

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

Rebase artifacts.

@bjpalmer
Copy link
Contributor

bjpalmer commented Nov 1, 2023

I'm in the process of cleaning up after the rebase. I'm not sure how I managed to overlook some of the artifacts since the code doesn't compile. I noticed the same thing about the destructor and fixed it.

//gridpack::utility::Configuration::CursorPtr cursor;
cursor = config->getCursor("Configuration.Dynamic_simulation");
std::vector<gridpack::dynamic_simulation::Event> faults;
faults = ds_app.getFaults(cursor);
Copy link
Member Author

Choose a reason for hiding this comment

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

Also, getFaults() is no longer a method. I think it's getEvents() now. I don't know if there is an internal difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

FIxed.

@abhyshr
Copy link
Collaborator

abhyshr commented Nov 17, 2023

@bjpalmer: any updates on when GA will be updated to have fewture/init-comm in develop?

@bjpalmer
Copy link
Contributor

The stuff in feature/init-comm should already be in the develop branch of GA. I've been linking GridPACK against the develop branch and it seems to work okay.

@wperkins
Copy link
Member Author

The stuff in feature/init-comm should already be in the develop branch of GA. I've been linking GridPACK against the develop branch and it seems to work okay.

FYI. In the GridPACK feature/init-comm branch, if you specify -D BUILD_GA=YES the GA develop branch will be built.

@abhyshr
Copy link
Collaborator

abhyshr commented Nov 17, 2023

So then can we just use the develop branch of GA? And merge GridPack's feature/init-comm to develop? I can update the install script to use GA's develop branch. Thoughts?

@abhyshr
Copy link
Collaborator

abhyshr commented Nov 17, 2023

First we need to resolve the issue with GridPACK python application hanging on multiple processors. @bjpalmer: do you have time to take a look at this?

@bjpalmer
Copy link
Contributor

You should be able to switch the install script to use develop. Is the code hanging when using the progress ranks runtime in GA? You need to be careful with that runtime to get the code to exit cleanly.

@abhyshr
Copy link
Collaborator

abhyshr commented Nov 17, 2023

I am not using progress ranks. You can try the updated 39 bus python example I've pushed to the branch

@wperkins wperkins changed the title DRAFT: Create gridpack::environment with existing MPI communicator Create gridpack::environment with existing MPI communicator Nov 29, 2023
@wperkins wperkins marked this pull request as ready for review November 29, 2023 16:27
@wperkins
Copy link
Member Author

You should be able to switch the install script to use develop. Is the code hanging when using the progress ranks runtime in GA? You need to be careful with that runtime to get the code to exit cleanly.

You can still use the official GA release to build this branch. But, if you do, you cannot specify -D ENABLE_ENVIRONMENT_FROM_COMM=TRUE. It would be nice to rework FindGA.cmake to test for the necessary GA routine and set that option automatically. That's a lot of work though.

Personally, I think we should merge this. The audience for -D ENABLE_ENVIRONMENT_FROM_COMM=TRUE is small and they are already navigating it.

@wperkins wperkins mentioned this pull request Dec 13, 2023
49 tasks
wperkins and others added 21 commits December 13, 2023 10:57
…hese may

confuse builds outside of gridpack.
…nts from

some of the code and cleaned up the environment destructor so that it works
properly for regular environments.
…ations that

are initialized by a communicator and to prevent problems with tests using the
GA progress ranks runtime.
@wperkins
Copy link
Member Author

@bjpalmer, I've added some very brief documentation. If it's enough, please approve this PR and I'll merge it. Thanks.

@abhyshr abhyshr self-requested a review December 13, 2023 20:08
@abhyshr
Copy link
Collaborator

abhyshr commented Dec 13, 2023

Good to merge after rebase.

@wperkins wperkins merged commit c8cf948 into develop Dec 13, 2023
@wperkins wperkins deleted the feature/init-comm branch December 13, 2023 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants