Add specific communicator for neighborhood communication #1588

MarcelKoch · 2024-04-04T10:40:53Z

This PR adds a communicator that only handles neighborhood all-to-all communication. It implements the new interface collective_communicator, which provides different implementations for a selected set of collective mpi routines. Currently, this only includes the non-blocking all-to-all.

The communication uses a fixed pattern, i.e. the send/recv sizes are fixed when the neighborhood communicator is constructed. I would have liked to decouple that, but this would require some knowledge of how the sizes are stored at the interface level. If someone has an idea for that, please let me know.

This is the first part of splitting up #1546.

The neighborhood all-to-all has a bug in OpenMPI < v4.1.0, which makes it necessary to disable the neighborhood communicator in this case. As replacement, there is also a dense all-to-all communicator.

Todo:

documentation

PR Stack:

upsj

First pass, mostly interface and implementation

What is the moved-from state of a Communicator? Should it match that of an MPI communicator wrapper, and be MPI_NULL, or preserve the MPI communicator?

include/ginkgo/core/distributed/collective_communicator.hpp

core/distributed/dense_communicator.cpp

core/distributed/neighborhood_communicator.cpp

MarcelKoch · 2024-10-23T11:38:44Z

@upsj I think the moved-from that should be using MPI_COMM_NULL as communicator. Then trying to launch a communication will fail quite badly.

yhmtsai · 2024-12-09T10:16:12Z

include/ginkgo/config.hpp.in

-#cmakedefine GINKGO_FORCE_SPMV_BLOCKING_COMM
+#cmakedefine01 GINKGO_HAVE_OPENMPI_PRE_4_1_X


public interface break?
but I might also consider it is under experimental feature

I can add the old macro again, but I'm not really sure how we usually consider macros for interface stability. IMO we either should state which macros are public, and which are private. Because this macro should definitely be private.

some of them I might consider it is public like version and propabably mixed precision.
This is only used in the experimental feature, so I also think it is free to change

include/ginkgo/core/distributed/dense_communicator.hpp

include/ginkgo/core/distributed/neighborhood_communicator.hpp

yhmtsai · 2024-12-09T10:20:20Z

include/ginkgo/core/distributed/neighborhood_communicator.hpp

+}  // namespace gko
+
+#endif


Suggested change

} // namespace gko

#endif

} // namespace gko

#endif

yhmtsai · 2024-12-09T12:34:15Z

include/ginkgo/core/distributed/neighborhood_communicator.hpp

+     * Default constructor with empty communication pattern
+     * @param base  the base communicator


Suggested change

* Default constructor with empty communication pattern

* @param base the base communicator

* Default constructor with empty communication pattern

*

* @param base the base communicator

yhmtsai · 2024-12-16T03:23:15Z

include/ginkgo/core/distributed/collective_communicator.hpp

+     * @tparam RecvType  the type of the elements to receive
+     * @param exec  the executor for the communication


Suggested change

* @tparam RecvType the type of the elements to receive

* @param exec the executor for the communication

* @tparam RecvType the type of the elements to receive

*

* @param exec the executor for the communication

yhmtsai · 2024-12-16T03:24:13Z

include/ginkgo/core/distributed/dense_communicator.hpp

+     * Default constructor with empty communication pattern
+     * @param base  the base communicator


Suggested change

* Default constructor with empty communication pattern

* @param base the base communicator

* Default constructor with empty communication pattern

*

* @param base the base communicator

yhmtsai · 2024-12-16T03:24:24Z

include/ginkgo/core/distributed/dense_communicator.hpp

+     * @tparam GlobalIndexType  the global index type of the map
+     * @param base  the base communicator


Suggested change

* @tparam GlobalIndexType the global index type of the map

* @param base the base communicator

* @tparam GlobalIndexType the global index type of the map

*

* @param base the base communicator

yhmtsai · 2024-12-16T03:27:09Z

include/ginkgo/core/distributed/dense_communicator.hpp

+     * Equality is defined as having identical or congruent communicators and
+     * their communication pattern is equal. No communication is done, i.e.
+     * there is no reduction over the local equality check results.


Suggested change

* Equality is defined as having identical or congruent communicators and

* their communication pattern is equal. No communication is done, i.e.

* there is no reduction over the local equality check results.

* Equality is defined as having identical or congruent communicators and

* their communication pattern is equal. There is no communication need in this function, i.e.

* there is no reduction over the local equality check results.

core/test/mpi/distributed/dense_communicator.cpp

Co-authored-by: Pratik Nayak <[email protected]>

- fix include guards - update docs - implement copy/move constructors/assignment with tests - add equality test for collective communicators (needed for testing) - always enable neighborhood comm, just throw if openmpi is too old - define moved-from state as MPI_COMM_NULL + empty sizes/offsets - remove unnecessary namespace - make virtual function protected Co-authored-by: Pratik Nayak <[email protected]> Co-authored-by: Tobias Ribizel <[email protected]>

- documentation - formatting Co-authored-by: Yu-Hsiang M. Tsai <[email protected]> Signed-off-by: Marcel Koch <[email protected]>

- merge tests Co-authored-by: Yu-Hsiang M. Tsai <[email protected]>

yhmtsai

LGTM in general.

yhmtsai · 2024-12-23T08:34:26Z

include/ginkgo/core/distributed/dense_communicator.hpp

+     * This implementation uses the neighborhood communication
+     * MPI_Ineighbor_alltoallv. See MPI documentation for more details.


yhmtsai · 2024-12-23T08:47:56Z

include/ginkgo/core/distributed/collective_communicator.hpp

+     * @param recv_buffer  the receive buffer
+     * @return  a request handle


Suggested change

* @param recv_buffer the receive buffer

* @return a request handle

* @param recv_buffer the receive buffer

*

* @return a request handle

yhmtsai · 2024-12-23T08:52:54Z

core/distributed/dense_communicator.cpp

+    std::partial_sum(send_sizes_.begin(), send_sizes_.end(),
+                     send_offsets_.begin() + 1);
+    std::partial_sum(recv_sizes_.begin(), recv_sizes_.end(),
+                     recv_offsets_.begin() + 1);


Could you remind me this behavior?
the constructor with the default value T() is until c++11, but the others will rely on the allocator, which gives the uninitialized storage?

yhmtsai · 2024-12-23T08:58:41Z

include/ginkgo/core/distributed/collective_communicator.hpp

+     * The send_buffer must have allocated at least get_send_size number of
+     * elements, and the recv_buffer must have allocated at least get_recv_size
+     * number of elements.


it only accepts the plain pointer.
Should we use the gko::array to ensure the size fulfill the requirement?

include/ginkgo/core/base/mpi.hpp

yhmtsai · 2024-12-23T09:24:53Z

core/test/mpi/distributed/collective_communicator.cpp

+    ASSERT_TRUE(moved == spcomm);
+    ASSERT_TRUE(moved_from == empty_comm);


Suggested change

ASSERT_TRUE(moved == spcomm);

ASSERT_TRUE(moved_from == empty_comm);

ASSERT_EQ(moved, spcomm);

ASSERT_EQ(moved_from, empty_comm);

yhmtsai · 2024-12-23T09:25:07Z

core/test/mpi/distributed/collective_communicator.cpp

+    ASSERT_TRUE(moved == spcomm);
+    ASSERT_TRUE(moved_from == empty_comm);


Suggested change

ASSERT_TRUE(moved == spcomm);

ASSERT_TRUE(moved_from == empty_comm);

ASSERT_EQ(moved, spcomm);

ASSERT_EQ(moved_from, empty_comm);

yhmtsai · 2024-12-23T09:26:04Z

core/test/mpi/distributed/collective_communicator.cpp

+    using communicator_type = typename TestFixture::communicator_type;
+    using part_type = typename TestFixture::part_type;
+    using map_type = typename TestFixture::map_type;
+    auto part = gko::share(part_type::build_from_global_size_uniform(


use create_default_comm()?

yhmtsai · 2024-12-23T09:26:50Z

core/test/mpi/distributed/collective_communicator.cpp

+    using communicator_type = typename TestFixture::communicator_type;
+    using part_type = typename TestFixture::part_type;
+    using map_type = typename TestFixture::map_type;
+    auto part = gko::share(part_type::build_from_global_size_uniform(


use create_default_comm()?

yhmtsai · 2024-12-23T09:33:29Z

core/test/mpi/distributed/collective_communicator.cpp

+
+    auto req = spcomm.i_all_to_all_v(this->ref, static_cast<int*>(nullptr),
+                                     static_cast<int*>(nullptr));
+    req.wait();


Suggested change

req.wait();

ASSERT_NO_THROW(req.wait());

maybe?

MarcelKoch self-assigned this Apr 4, 2024

ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. mod:core This is related to the core module. labels Apr 4, 2024

MarcelKoch requested a review from pratikvn April 4, 2024 10:41

MarcelKoch force-pushed the neighborhood-communicator branch from 6acf7c4 to 8aa6ab9 Compare April 4, 2024 11:00

MarcelKoch force-pushed the read-distributed-with-index-map branch from b42ab92 to 8f104fd Compare April 4, 2024 11:00

MarcelKoch modified the milestone: Ginkgo 1.8.0 Apr 5, 2024

MarcelKoch force-pushed the neighborhood-communicator branch from 8aa6ab9 to 77398bd Compare April 17, 2024 16:28

MarcelKoch requested a review from upsj April 19, 2024 09:19

MarcelKoch mentioned this pull request Apr 19, 2024

Adds sparse communicator class #1546

Closed

7 tasks

MarcelKoch force-pushed the neighborhood-communicator branch from 77398bd to d278cad Compare April 19, 2024 14:39

MarcelKoch force-pushed the read-distributed-with-index-map branch from 8f104fd to a0824a8 Compare April 19, 2024 14:39

This was referenced Apr 22, 2024

Add segmented array type #1545

Merged

Distributed Index Map #1543

Merged

Adds Index map device kernels #1579

Merged

Use index_map in distributed::matrix #1544

Merged

MarcelKoch force-pushed the neighborhood-communicator branch from d278cad to d6112ef Compare April 22, 2024 11:11

MarcelKoch force-pushed the read-distributed-with-index-map branch from a0824a8 to 8ad3f2f Compare April 22, 2024 11:11

MarcelKoch force-pushed the neighborhood-communicator branch from d6112ef to 1582673 Compare April 25, 2024 07:16

MarcelKoch mentioned this pull request Apr 30, 2024

Adds distributed row gatherer #1589

Open

6 tasks

MarcelKoch force-pushed the neighborhood-communicator branch from 1582673 to db9b48a Compare April 30, 2024 13:41

MarcelKoch force-pushed the read-distributed-with-index-map branch from 8ad3f2f to 26678b3 Compare April 30, 2024 13:41

MarcelKoch force-pushed the neighborhood-communicator branch from db9b48a to 72eafff Compare April 30, 2024 15:20

MarcelKoch force-pushed the read-distributed-with-index-map branch from 26678b3 to 006d67d Compare April 30, 2024 15:20

MarcelKoch force-pushed the neighborhood-communicator branch from 72eafff to 3c70106 Compare May 2, 2024 10:04

MarcelKoch force-pushed the read-distributed-with-index-map branch from 006d67d to b295b11 Compare May 2, 2024 10:04

MarcelKoch force-pushed the neighborhood-communicator branch from 3c70106 to a1567b8 Compare May 2, 2024 10:06

MarcelKoch requested a review from upsj August 27, 2024 12:05

MarcelKoch force-pushed the neighborhood-communicator branch from 1f49b91 to 4db050c Compare October 7, 2024 13:06

MarcelKoch force-pushed the index-map-pgm branch from 3ad5eee to 6395054 Compare October 7, 2024 13:06

upsj reviewed Oct 18, 2024

View reviewed changes

MarcelKoch force-pushed the neighborhood-communicator branch from 4db050c to 1ebe59f Compare October 23, 2024 13:32

MarcelKoch force-pushed the index-map-pgm branch 2 times, most recently from 604a6e9 to ba0982e Compare October 23, 2024 15:13

MarcelKoch force-pushed the neighborhood-communicator branch from 1ebe59f to e7d32a1 Compare October 30, 2024 15:10

MarcelKoch modified the milestones: Ginkgo 1.9.0, Ginkgo 1.10.0 Dec 9, 2024

yhmtsai requested changes Dec 16, 2024

View reviewed changes

MarcelKoch force-pushed the index-map-pgm branch from ba0982e to 23b4fe7 Compare December 17, 2024 12:11

MarcelKoch force-pushed the neighborhood-communicator branch from 1216932 to ceb6f2e Compare December 17, 2024 13:59

MarcelKoch and others added 13 commits December 18, 2024 10:27

Make comm_index_type avaiable in the mpi namespace

e6689c4

create owning communicator

10aa0ad

[imap] create variant type of supported index maps

0e13968

[coll-comm] adds interface for collective communicator

4d0bbd0

Co-authored-by: Pratik Nayak <[email protected]>

[coll-comm] add dense communication

a0a4343

[coll-comm] adds neighborhood implementation of collective communicator

892fca3

Co-authored-by: Pratik Nayak <[email protected]>

[coll-comm] disable neighbor comm if open-mpi version < 4.1.0

a053ac5

[mpi] define moved-from state for communicator

60d243f

[mpi] add explicit comparison for identical and congruent

b22447f

[coll-comm] remove envelop-based constructors

62a99b4

[mpi] add documentation on copy/move

30a8b3e

[coll-comm] review updates:

807118c

- documentation - formatting Co-authored-by: Yu-Hsiang M. Tsai <[email protected]> Signed-off-by: Marcel Koch <[email protected]>

MarcelKoch force-pushed the neighborhood-communicator branch from ceb6f2e to 807118c Compare December 18, 2024 09:27

[coll-comm] review updates:

6d548e6

- merge tests Co-authored-by: Yu-Hsiang M. Tsai <[email protected]>

yhmtsai requested changes Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add specific communicator for neighborhood communication #1588

Add specific communicator for neighborhood communication #1588

MarcelKoch commented Apr 4, 2024 •

edited

Loading

upsj left a comment

MarcelKoch commented Oct 23, 2024

yhmtsai Dec 9, 2024

MarcelKoch Dec 17, 2024

yhmtsai Dec 17, 2024

yhmtsai Dec 9, 2024

yhmtsai Dec 9, 2024

yhmtsai Dec 16, 2024

yhmtsai Dec 16, 2024

yhmtsai Dec 16, 2024

yhmtsai Dec 16, 2024

yhmtsai left a comment

yhmtsai Dec 23, 2024

yhmtsai Dec 23, 2024

yhmtsai Dec 23, 2024

yhmtsai Dec 23, 2024

yhmtsai Dec 23, 2024

yhmtsai Dec 23, 2024

yhmtsai Dec 23, 2024

yhmtsai Dec 23, 2024

yhmtsai Dec 23, 2024

		#cmakedefine GINKGO_FORCE_SPMV_BLOCKING_COMM
		#cmakedefine01 GINKGO_HAVE_OPENMPI_PRE_4_1_X

		* Default constructor with empty communication pattern
		* @param base the base communicator

		* @tparam RecvType the type of the elements to receive
		* @param exec the executor for the communication

		* @tparam GlobalIndexType the global index type of the map
		* @param base the base communicator

		* This implementation uses the neighborhood communication
		* MPI_Ineighbor_alltoallv. See MPI documentation for more details.

		* @param recv_buffer the receive buffer
		* @return a request handle

		ASSERT_TRUE(moved == spcomm);
		ASSERT_TRUE(moved_from == empty_comm);

Add specific communicator for neighborhood communication #1588

Are you sure you want to change the base?

Add specific communicator for neighborhood communication #1588

Conversation

MarcelKoch commented Apr 4, 2024 • edited Loading

upsj left a comment

Choose a reason for hiding this comment

MarcelKoch commented Oct 23, 2024

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

yhmtsai 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

Choose a reason for hiding this comment

MarcelKoch commented Apr 4, 2024 •

edited

Loading