fix for MPI non-blocking collectives (issue #17) #32

antonl321 · 2025-01-08T16:02:57Z

This branch addresses the issue of random failures observed when using MPL collectives in non-blocking mode.

It is assumed that the cause of these failures is the missing displacements arrays references when MPL_wait is called if MPL_alltoallv or any other other collective operation uses the option to created the displacement arrays inside the MPL wrapper.

Solution description:

when mpl_alltoallv is used in non-blocking the displacement arrays are stored in a linked list together with the MPI_request value and number of mpi ranks.
when MPL_wait is called the linked list is inspected, if a node has the MPI_request value equal to the argument value then that node is removed from he linked list.

Implementation:

A new fortran module was created to store the linked list node and the operation routines.
a node contains the MPI request, nproc and two allocatable arrays for the send and receive arrays.
the link list is private, it is operated with the list_manager class that append a new node at the beginning of communication and deletes it at end of communication.
the list manager allocates the displacement arrays and returns pointer to them.
the MPL_preamble routine was modified to use pointers that point to various displacement array: local, arguments or to the linked list according to the input pattern.

This version needs more work but I think that a checkpoint on the implementation design is in order before polishing the details.

…he displacements data

… tests, extended to all data types

…gh pointers

FussyDuck · 2025-01-08T16:03:04Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ antonl321
❌ Lucian Anton

Lucian Anton seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

…he displacements data

… tests, extended to all data types

…gh pointers

wdeconinck

Hi @antonl321 thanks for your submission! It will be great to have this working.
I have not yet reviewed the actual changes to mpl_alltoallv using the linked_list, but rather the CMake and the testing.
You could already address my suggestions, and if you can contact me offline if you need clarification. Thanks! 🙏 😃

wdeconinck · 2025-01-09T09:24:22Z

CMakeLists.txt

+if( CMAKE_Fortran_COMPILER_ID MATCHES "Cray")
+  add_link_options("-homp")
+endif()


This should not be needed or needs to be implemented using the OpenMP::OpenMP_Fortran target depending on the content of HAVE_OMP variable. I thought this was already working.

wdeconinck · 2025-01-09T09:25:05Z

build.sh

Probably you did not mean to commit this file, it should not be committed.

Yes, I have tow auxiliary files: build.sh and env.sh to be removed when all is clear.

wdeconinck · 2025-01-09T09:25:16Z

env.sh

Probably you did not mean to commit this file, it should not be committed.

wdeconinck · 2025-01-09T09:30:46Z

tests/CMakeLists.txt

 if( HAVE_MPI AND MPIEXEC )
-  set( LAUNCH ${MPIEXEC} ${MPIEXEC_NUMPROC_FLAG} 1 )
+  set( LAUNCH ${MPIEXEC} ${MPI_OPTS} ${MPIEXEC_NUMPROC_FLAG} 1 )


In ecbuild_add_test we cater for something similar but name it MPI_ARGS instead. Perhaps you can use the following snippet instead.
https://github.com/ecmwf/ecbuild/blob/develop/cmake/ecbuild_add_test.cmake#L427-L432

wdeconinck · 2025-01-09T09:33:10Z

tests/CMakeLists.txt

+         COMMAND ${CMAKE_COMMAND}
+         "-DEXECUTABLE=$<TARGET_FILE:fiat-test-mpl-alltoallv-no-output>"
+         "-DLAUNCH=${LAUNCH2}"
+         -P ${CMAKE_CURRENT_SOURCE_DIR}/test_mpl_alltoallv_no_output.cmake )


I think you should be able to just use "ecbuild_add_test" with the MPI option. There's no need to parse the output of the test to detect failure. When the test itself crashes and exits with non-zero due to a failed assertion it should be failing automatically. The "_no_output" suffix is not needed.

I just used the pattern used by other tests.

tests/CMakeLists.txt

…after testing with nproc > 2

wdeconinck

Another round of review and some requested changes:

wdeconinck · 2025-03-12T09:10:04Z

build.sh

This file should not be committed

wdeconinck · 2025-03-12T09:10:24Z

env.sh

This file should not be committed

wdeconinck · 2025-03-12T09:18:44Z

src/fiat/mpl/linked_list_mod.F90

@@ -0,0 +1,233 @@
+module linked_list_mod


Just for safety, I'd rename this to mpl_linked_list_mod

wdeconinck · 2025-03-12T10:31:01Z

tests/alltoallv.F90

+
+end subroutine
+
+#define FAIL(msg) call fail_impl(msg,__FILE__,__LINE__)


This line is likely the cause for the need of -free-line-length-none flag.
I prefer not to use the flag as it is compiler specific and reduces portability to other platforms.
Please, replace __FILE__ with "alltoallv.F90" and remove the flag.

wdeconinck · 2025-03-12T10:32:30Z

tests/CMakeLists.txt

+ecbuild_add_executable(
+    TARGET  alltoallv
+    SOURCES alltoallv.F90 
+    LIBS    fiat
+    FFLAGS $<$<Fortran_COMPILER_ID:GNU>:-ffree-line-length-none>
+    LINKER_LANGUAGE Fortran
+    NOINSTALL )
+
+ecbuild_add_test( TARGET fiat_test_alltoallv
+                  COMMAND alltoallv
+                  MPI 2)


This can be simplified:

Suggested change

ecbuild_add_executable(

TARGET alltoallv

SOURCES alltoallv.F90

LIBS fiat

FFLAGS $<$<Fortran_COMPILER_ID:GNU>:-ffree-line-length-none>

LINKER_LANGUAGE Fortran

NOINSTALL )

ecbuild_add_test( TARGET fiat_test_alltoallv

COMMAND alltoallv

MPI 2)

ecbuild_add_test(

TARGET fiat_test_alltoallv

SOURCES alltoallv.F90

LIBS fiat

LINKER_LANGUAGE Fortran

MPI 2 )

wdeconinck · 2025-03-12T10:33:46Z

tests/CMakeLists.txt

+ecbuild_add_executable(
+    TARGET  linked_list
+    SOURCES linked_list.F90
+    LIBS    fiat
+    LINKER_LANGUAGE Fortran
+    NOINSTALL )
+
+ecbuild_add_test( TARGET fiat_test_linked_list
+                  COMMAND linked_list)
+


This can be simplified:

Suggested change

ecbuild_add_executable(

TARGET linked_list

SOURCES linked_list.F90

LIBS fiat

LINKER_LANGUAGE Fortran

NOINSTALL )

ecbuild_add_test( TARGET fiat_test_linked_list

COMMAND linked_list)

ecbuild_add_test(

TARGET fiat_test_mpl_linked_list

SOURCES linked_list.F90

LIBS fiat

LINKER_LANGUAGE Fortran )

wdeconinck · 2025-03-12T10:34:27Z

src/fiat/mpl/internal/mpl_wait_mod.F90

@@ -67,6 +67,7 @@ MODULE MPL_WAIT_MOD
 USE MPL_MPIF
 USE MPL_DATA_MODULE
 USE MPL_MESSAGE_MOD
+USE LINKED_LIST_MOD


Suggested change

USE LINKED_LIST_MOD

USE MPL_LINKED_LIST_MOD

wdeconinck · 2025-03-12T10:34:44Z

src/fiat/mpl/internal/mpl_alltoallv_mod.F90

@@ -72,6 +72,7 @@ MODULE MPL_ALLTOALLV_MOD
 USE MPL_STATS_MOD
 USE MPL_WAIT_MOD
 USE YOMMPLSTATS
+USE LINKED_LIST_MOD


Suggested change

USE LINKED_LIST_MOD

USE MPL_LINKED_LIST_MOD

antonl321 and others added 13 commits November 6, 2024 16:40

fix openmp flag for cray compiler at link stage

fb46832

files useful to build

1f42cab

basic test for mpl_alltoallv in + some auxilliry files

b10ca6d

extended the test to non-blocking; I could not get the random failure

4d8c686

started the linked list; backup

b4e82f1

cmake test code written; compiled the sources

523c05b

test for the linked list finished

99b0fc6

first step in adding displ linked list in alltoallv code

6cd571c

more work in alltroallv, using pointers to various array that keeps t…

8989e03

…he displacements data

some fixes, debug print in alltoallv

ca16ba2

fixed the data flow: req must be used after mpi_ialltoallv call, more…

da8a883

… tests, extended to all data types

change in interface, linked list internal arrays accesible only throu…

8dc931d

…gh pointers

testing nvhpc on lumi; fails with am bizzare mpi error

676a98c

wdeconinck changed the base branch from main to develop January 9, 2025 09:08

antonl321 added 14 commits January 9, 2025 10:19

fix openmp flag for cray compiler at link stage

716003c

files useful to build

1e9018b

basic test for mpl_alltoallv in + some auxilliry files

04388bc

extended the test to non-blocking; I could not get the random failure

577852b

started the linked list; backup

bab004a

cmake test code written; compiled the sources

06adef7

test for the linked list finished

366dbd1

first step in adding displ linked list in alltoallv code

eb12968

more work in alltroallv, using pointers to various array that keeps t…

58220d3

…he displacements data

some fixes, debug print in alltoallv

2200dd2

fixed the data flow: req must be used after mpi_ialltoallv call, more…

8c326ed

… tests, extended to all data types

change in interface, linked list internal arrays accesible only throu…

8d05ba1

…gh pointers

testing nvhpc on lumi; fails with am bizzare mpi error

3ead589

removed superfluous argument

b5cd669

wdeconinck force-pushed the bug/syla_develop_ialltoallv.issue17 branch from 2168149 to b5cd669 Compare January 9, 2025 09:21

wdeconinck requested changes Jan 9, 2025

View reviewed changes

antonl321 and others added 3 commits January 9, 2025 18:02

cmake bits corrected as suggested by Willem, fixes to alltoallv test …

ee97b42

…after testing with nproc > 2

added -ffree-line-length-node to gfortran (alltoallv test)

7ee5a6c

cmake bits corrected as suggested by Willem, fixes to alltoallv test …

4c61bf1

…after testing with nproc > 2

antonl321 mentioned this pull request Feb 7, 2025

Incorrect usage of non blocking MPI collectives #17

Open

wdeconinck requested changes Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for MPI non-blocking collectives (issue #17) #32

fix for MPI non-blocking collectives (issue #17) #32

antonl321 commented Jan 8, 2025

FussyDuck commented Jan 8, 2025 •

edited

Loading

wdeconinck left a comment

wdeconinck Jan 9, 2025

wdeconinck Jan 9, 2025

antonl321 Jan 9, 2025

wdeconinck Jan 9, 2025

wdeconinck Jan 9, 2025

wdeconinck Jan 9, 2025

antonl321 Jan 9, 2025

wdeconinck left a comment

wdeconinck Mar 12, 2025

wdeconinck Mar 12, 2025

wdeconinck Mar 12, 2025

wdeconinck Mar 12, 2025

wdeconinck Mar 12, 2025

wdeconinck Mar 12, 2025

wdeconinck Mar 12, 2025

wdeconinck Mar 12, 2025


		end subroutine

		#define FAIL(msg) call fail_impl(msg,__FILE__,__LINE__)

fix for MPI non-blocking collectives (issue #17) #32

Are you sure you want to change the base?

fix for MPI non-blocking collectives (issue #17) #32

Conversation

antonl321 commented Jan 8, 2025

FussyDuck commented Jan 8, 2025 • edited Loading

wdeconinck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdeconinck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FussyDuck commented Jan 8, 2025 •

edited

Loading