added non-blocking root communicator #1478

gberg617 · 2024-12-11T20:19:41Z

Summary

This PR is a feature which adds a communicator for sending messages from any rank to the root rank non-collectively. This can be useful in cases where an arbitrary rank throws an error that needs to be sent to the root rank to output to a file.

gberg617 · 2024-12-11T20:20:14Z

Unit testing and documentation will be added to this PR in follow-up commits.

src/axom/lumberjack/MPIUtility.cpp

rhornung67 · 2024-12-11T20:24:21Z

src/axom/lumberjack/MPIUtility.cpp

+  MPI_Status mpiStatus;
+
+  // Get size and source of MPI message
+  int mpiFlag = true;


Similar comment here and below. You could define static constexpr integer variables that use names containing true and false to make the code more readable and avoid magic numbers.

src/axom/lumberjack/MPIUtility.cpp

src/axom/lumberjack/MPIUtility.hpp

src/axom/lumberjack/NonBlockingRootCommunicator.cpp

src/axom/lumberjack/MPIUtility.cpp

bmhan12 · 2024-12-11T22:32:16Z

src/axom/lumberjack/MPIUtility.cpp

+
+  // Get size and source of MPI message
+  int mpiFlag = true;
+  MPI_Iprobe(MPI_ANY_SOURCE, tag, comm, &mpiFlag, &mpiStatus);


MPI_Iprobe is nonblocking here, so is there a chance the mpiFlag is not set to true when it is expected to be? Would it be better to have this be a blocking MPI_Probe? Basing this comment off this stackoverflow post: https://stackoverflow.com/questions/43823458/mpi-iprobe-vs-mpi-probe

Additionally, if using MPI_Iprobe, should mpiFlag default be set to false, so it can be set to true only by a successful function call?

I think the mpiFlag will be set in either context to either true or false, but to your point, it is safer to initialize this as false.

The stackoverflow example illustrates an interesting but slightly different approach than what I'm intending to do. They are calling MPI_Iprobe in a while loop that does not exit until it returns a flag that is non-zero. In my case, I am checking to see if any messages need to be received only once, and if there are no messages, the function exits by returning nullptr. This intent in the stackoverflow example is to continuously monitor the status, whereas I'm only intending to periodically monitor the status whenever the code path enters into this function. Both could be relevant to the problem I'm trying to solve with this communicator, where the root rank needs to receive information from other ranks that they are aborting. I had a preference toward the latter option (periodically monitoring the status whenever the root rank reaches a point where it enters this code path) because it seemed to me like the more efficient option, even if it comes at a cost of sometimes not receiving the status before the program aborts. But I'm not really sure which option is best for this scenario. I'd be curious to hear your thoughts.

I had a preference toward the latter option (periodically monitoring the status whenever the root rank reaches a point where it enters this code path) because it seemed to me like the more efficient option, even if it comes at a cost of sometimes not receiving the status before the program aborts.

I agree, I would expect the latter option to have less overhead, doing a single poll with MPI_Iprobe instead of spinning on MPI_Iprobe until status is updated in the former case. Nevertheless, I might not be considering something, so am also curious if others have ideas.

bmhan12

LGTM!

…or to NonCollectiveRootCommunicator

… and added unit testing

src/axom/lumberjack/MPIUtility.cpp

src/axom/lumberjack/MPIUtility.hpp

src/axom/lumberjack/NonCollectiveRootCommunicator.cpp

src/axom/lumberjack/NonCollectiveRootCommunicator.hpp

src/axom/lumberjack/tests/lumberjack_NonCollectiveRootCommunicator.hpp

Co-authored-by: Chris White <white238@llnl.gov>

…ator.hpp Co-authored-by: Chris White <white238@llnl.gov>

white238

The axom team needs to discuss the implications of this further before this is merged.

… duplicate

gberg617 · 2025-01-29T01:36:53Z

@white238 @bmhan12 I talked with @gunney1 and we decided the best path forward is to duplicate the MPI communicator passed in the initialize() call, and have this duplicate owned by the Lumberjack communicator object. With this change, we can avoid having to create MPI tags for each non-collective communicator object, and instead have each communicator object have its own MPI communicator using the same default MPI tag. Please let me know if you have any further concerns.

kennyweiss

Thanks @gberg617

Sounds like we'll need to discuss some details before merging this. I've added a few minor comments in the meantime.

kennyweiss · 2025-01-29T03:54:08Z

src/axom/lumberjack/NonCollectiveRootCommunicator.hpp

@@ -0,0 +1,135 @@
+// Copyright (c) 2017-2024, Lawrence Livermore National Security, LLC and


After merging in develop, please run our update_copyright_script to update the copyright on the new files to 2025.
See: https://github.com/LLNL/axom/blob/develop/scripts/update_copyright_date.sh

kennyweiss · 2025-01-29T03:56:35Z

src/axom/lumberjack/NonCollectiveRootCommunicator.hpp

+   * \param [in] ranksLimit Limit on how many ranks are individually tracked per
+   *  Message.


Suggestion:

Suggested change

* \param [in] ranksLimit Limit on how many ranks are individually tracked per

* Message.

* \param [in] ranksLimit Upper limit on number of ranks are tracked per Message.

kennyweiss · 2025-01-29T03:57:17Z

src/axom/lumberjack/NonCollectiveRootCommunicator.cpp

+  MPI_Comm_dup(comm, &m_mpiComm);
+  MPI_Comm_rank(m_mpiComm, &m_mpiCommRank);
+  MPI_Comm_size(m_mpiComm, &m_mpiCommSize);
+  m_ranksLimit = ranksLimit;


Presumably, this needs to be a positive number greater than or equal to 1. Should we check it w/ a SLIC_ASSERT?

kennyweiss · 2025-01-29T04:02:16Z

src/axom/lumberjack/MPIUtility.hpp

+ *  MPI communication uses default LJ_Tag. 
+ *****************************************************************************
+ */
+const char* mpiNonBlockingReceiveMessages(MPI_Comm comm);


Should we update the docs for this function (and mpiBlockingReceiveMessages) to indicate that the caller is responsible for deallocating the memory in the returned pointer?

@gberg617 We should update docs as @kennyweiss suggested.

…mmunicator

gunney1 · 2025-02-05T16:37:53Z

src/axom/lumberjack/MPIUtility.hpp

+/*!
+ *****************************************************************************
+ * \brief Receives any Message sent to this rank, if there are any messages 
+ *  that are sent. Returns null if no messages are sent.


When we return from a non-blocking probe, there could be sent messages that haven't arrived yet. I suggest replacing "are sent" with "have arrived." It's pedantic but can avoid confusion when the unexpected happens.

gunney1 · 2025-02-05T16:41:24Z

src/axom/lumberjack/NonCollectiveRootCommunicator.cpp

+    {
+      currPackedMessages = mpiNonBlockingReceiveMessages(m_mpiComm);
+
+      if(isPackedMessagesEmpty(currPackedMessages))


It looks odd to me that currPackedMessages is used before checking if it's null.

gberg617 · 2025-02-12T23:32:44Z

Update on the failing tests: The multiple_communicators test I added sporadically fails on Azure. This test is important to capture certain behavior when multiple communicators are sending/receiving messages. Looking into this, it sometimes fails when run with other unit tests (and only on Azure), but consistently passes when it is run by itself. I have heard that gtest+MPI is somewhat fragile in some cases, so I attempted to remove gtest from the tests I added by converting the ASSERT macros to my own version, and having each test become a function that is called within main() inside of lumberjack_NonCollectiveRootCommunicator.cpp. This fixes the issue, and both tests in this file always pass. Does the Axom team have any thoughts about potentially adding unit tests that do not use gtest? If there is a strong preference for requiring gtest, does anyone have thoughts on other solutions that can prevent these sporadic failures?

bmhan12 · 2025-02-12T23:53:08Z

Update on the failing tests...

For reference, this is the error that is seen on Azure Pipelines and when ran locally in Docker:

102: Test command: /usr/bin/mpirun "-np" "7" "/home/axom/axom/build-clang@14.0.0-release/tests/lumberjack_mpi_tests" "--gtest_filter=lumberjack_NonCollectiveRootCommunicator*"
...
...
...
102: /home/axom/axom/src/axom/lumberjack/tests/lumberjack_NonCollectiveRootCommunicator.hpp:121: Failure
102: Expected equality of these values:
102:   receivedPackedMessages_c1.size()
102:     Which is: 0
102:   1
102: 
102: 
102: lumberjack_mpi_tests:8831 terminated with signal 11 at PC=55a67bf82d09 SP=7ffe05eea260.  Backtrace:
102: /home/axom/axom/build-clang@14.0.0-release/tests/lumberjack_mpi_tests(+0x8d09)[0x55a67bf82d09]
102: /home/axom/axom/build-clang@14.0.0-release/lib/libgtest.so.1.13.0(_ZN7testing8internal35HandleExceptionsInMethodIfSupportedINS_4TestEvEET0_PT_MS4_FS3_vEPKc+0x48)[0x7f8043294ef8]
102: /home/axom/axom/build-clang@14.0.0-release/lib/libgtest.so.1.13.0(_ZN7testing4Test3RunEv+0xcf)[0x7f80432790ef]
102: /home/axom/axom/build-clang@14.0.0-release/lib/libgtest.so.1.13.0(_ZN7testing8TestInfo3RunEv+0xf9)[0x7f804327a3d9]
102: /home/axom/axom/build-clang@14.0.0-release/lib/libgtest.so.1.13.0(_ZN7testing9TestSuite3RunEv+0x285)[0x7f804327aeb5]
102: /home/axom/axom/build-clang@14.0.0-release/lib/libgtest.so.1.13.0(_ZN7testing8internal12UnitTestImpl11RunAllTestsEv+0x41c)[0x7f804328b81c]
102: /home/axom/axom/build-clang@14.0.0-release/lib/libgtest.so.1.13.0(_ZN7testing8internal35HandleExceptionsInMethodIfSupportedINS0_12UnitTestImplEbEET0_PT_MS4_FS3_vEPKc+0x48)[0x7f8043295c18]
102: /home/axom/axom/build-clang@14.0.0-release/lib/libgtest.so.1.13.0(_ZN7testing8UnitTest3RunEv+0x59)[0x7f804328b3c9]
102: /home/axom/axom/build-clang@14.0.0-release/tests/lumberjack_mpi_tests(main+0x35)[0x55a67bf83075]
102: /lib/x86_64-linux-gnu/libc.so.6(+0x29d90)[0x7f804015dd90]
102: /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80)[0x7f804015de40]
102: /home/axom/axom/build-clang@14.0.0-release/tests/lumberjack_mpi_tests(+0x4f15)[0x55a67bf7ef15]
102: 
102: ===================================================================================
102: =   BAD TERMINATION OF ONE OF YOUR APPLICATION PROCESSES
102: =   PID 8832 RUNNING AT e17873b86e18
102: =   EXIT CODE: 9
102: =   CLEANING UP REMAINING PROCESSES
102: =   YOU CAN IGNORE THE BELOW CLEANUP MESSAGES
102: ===================================================================================
102: YOUR APPLICATION TERMINATED WITH THE EXIT STRING: Killed (signal 9)
102: This typically refers to a problem with your application.
102: Please see the FAQ page for debugging suggestions

src/axom/lumberjack/tests/lumberjack_NonCollectiveRootCommunicator.hpp

…ator.hpp attempting to fix sporadic failures on Azure. Co-authored-by: Brian Han <han12@llnl.gov>

gberg617 requested review from white238, kennyweiss, rhornung67 and bmhan12 December 11, 2024 20:19

gberg617 self-assigned this Dec 11, 2024