Add .bcast() #192

lukashuebner · 2022-03-24T12:21:01Z

Closes #173

…ing .get_ptr() * Rework the Span<> class to be more similar with C++20's std::span. * Add a SingleElementModifyableBuffer using the new Span<>'s interface. * Update the existing Buffers' interface to use the new Span<> * Split the existing Buffers' .get_ptr() into .resize() and .data() * Adjust allreduce and alltoall to use the new Span<> and Buffers

lukashuebner · 2022-03-25T12:25:57Z

Okay, I've split up this PR into #199 and this, which is forked from #199. How can I mark this PR as dependent on #199 and stop github from displaying the whole change set here?

Hespian · 2022-03-25T12:32:08Z

I don't think github has a feature for that. You just leave the dependent-tag on this PR until #199 is merged. Then you merge main into feature-bcast. That should result in the parts from #199 being removed from the diff of this PR. Then you remove the dependent-tag and add a review-tag.

Hespian · 2022-03-25T15:48:14Z

include/kamping/collectives/bcast.hpp

+        KASSERT(
+            recv_buf_large_enough_on_all_processes(send_recv_buf, root.rank()),
+            "The receive buffer is too small on at least one rank.", assert::light_communication);


Don't we want an execution path where kamping resizes the buffer so that it is large enough?

What should the behavior be in the following cases:

send_recv_buf.size() == 1 on root and == 0 on all other ranks, send_counts not specified: Assume a default size of 1? Broadcast the size? -> doubling the number of broadcasts might be unexpected here; depend on if the ominous SingleElementBuffer is specified?

send_counts explicitly specified: resize the buffer on all but the root process? On all including the root process? What, if the send_recv_buf() on root is const?

send_counts not specified and not case 1: Broadcast the size of the send buffer, resize the receive buffers, broadcast the data?

I think @DanielSeemaier does something like this in #170. Bcast and Scatter should probably use the same mechanisms.

Postponed, see #170 .

We could do that in a future PR when we see what's best (in particular this might change with our ongoing discussion on reducing syntax overhead anyways)

Hm I still don't really like the way it's done here. We never expected the size of the receive buffers to be set by the user and I would like to keep it that way.

This would force the user to write different code for root and non-root ranks. Probably not ideal

What would you think about send_recv_buf being optional?

Year, I don't like splitting up the send_recv_buf into a send_buf and recv_buf, too. But iirc someone argued sternly for it, as it'll unify the interface.

How about this then? :-)

if I'm the root: assert, that I have a send_recv_buf if I have no recv_count specified: // assume, that the other ranks don't know how much data to receive broadcast the amount of data to receive else: // assume that all other ranks also know about their recv_count Check, that the recv_counts are the same on all ranks broadcast the data else: // if I'm not the root: if I do not have a send_recv_buf, allocate one if I have a recv_count: resize my send_recv_buf assert, that all ranks have the same recv_count (eq. to the one provided to the root) else: receive the broadcast of the amount of data to receive resize my send_recv_buf receive the broadcast of the data

Other than that resizing the send_recv_buf should happen after receiving the recv_count, I think that should work fine. Not sure I remember the part of the discussion about a unified interface. @kurpicz ?

Oh, I looked at the old protocols and we already voted in favor of "(Bcast) SendRecv-Buffer auf allen ranks oder wenn SendRecv-Buffer nicht gegeben ist, ist man kein Root und man bekommt das Recv Result zurück."

tests/collectives/mpi_bcast_test.cpp

include/kamping/collectives/bcast.hpp

include/kamping/collectives/alltoall.hpp

lukashuebner · 2022-04-01T08:02:35Z

On hold until we decided if we want all ranks to know about failed assertions or only some.

…s send_recv_buf to bcast

Hespian

Quite a few comments but I think they're all pretty minor.

include/kamping/collectives/bcast.hpp

tests/collectives/mpi_bcast_test.cpp

include/kamping/collectives/bcast.hpp

tests/collectives/mpi_bcast_test.cpp

Hespian · 2022-06-22T14:57:13Z

tests/collectives/mpi_bcast_test.cpp

+    std::iota(values.begin(), values.end(), comm.rank() * 10);
+    kamping::Span<int> transfer_view(values.data(), asserting_cast<size_t>(num_transferred_values));
+
+    /// @todo Once we can check assertions, check that providing an recv_count != transfer_view.size() fails.


You added tests for this elsewhere, so this comment can be removed

tests/collectives/mpi_bcast_test.cpp

include/kamping/collectives/bcast.hpp

Hespian

Probably uncomment the assertion tests in #329

Hespian

@DanielSeemaier was quicker merging #329 ;) Hopefully just uncommenting your tests makes them pass ;)

tests/collectives/mpi_bcast_test.cpp

lukashuebner · 2022-06-24T06:18:09Z

@DanielSeemaier was quicker merging #329 ;) Hopefully just uncommenting your tests makes them pass ;)

Nope ... my guess is, that his approach does not work when only some ranks fail the KASSERT.

Hespian

There's one KASSERT test that I don't understand why it should fail.

Other than that, saying EXPECT_KASSERT_FAILS is broken (needs a fix) seems a bit aggressive. You want an added feature ;)

tests/collectives/mpi_bcast_test.cpp

lukashuebner self-assigned this Mar 24, 2022

lukashuebner added feature New feature or request wip Work in progress labels Mar 24, 2022

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

lukashuebner force-pushed the feature-bcast branch 3 times, most recently from 0f6cd27 to 5ec8bb6 Compare March 25, 2022 11:31

Lukas Hübner added 2 commits March 25, 2022 13:17

Add the collective operation broadcast (bcast).

69f7a9c

lukashuebner force-pushed the feature-bcast branch from 5ec8bb6 to 69f7a9c Compare March 25, 2022 12:22

lukashuebner added the dependent label Mar 25, 2022

lukashuebner added this to the Proof of Concept v0.1.0 milestone Mar 25, 2022

lukashuebner linked an issue Mar 25, 2022 that may be closed by this pull request

MPI_Bcast #173

Closed

lukashuebner added review Ready for review and removed wip Work in progress labels Mar 25, 2022

Hespian reviewed Mar 25, 2022

View reviewed changes

include/kamping/collectives/bcast.hpp Outdated Show resolved Hide resolved

Hespian removed this from the Proof of Concept v0.1.0 milestone Mar 25, 2022

lukashuebner added this to the Synchronous Collective Operations 0.2.0 milestone Mar 28, 2022

Merge branch 'main' into feature-bcast

8f32da0

lukashuebner removed the dependent label Mar 30, 2022

Move a const to the east side

9b82c17

lukashuebner changed the title ~~Add .bcast() and single element modifyable buffers.~~ Add .bcast() Mar 31, 2022

niklas-uhl reviewed Mar 31, 2022

View reviewed changes

include/kamping/collectives/alltoall.hpp Outdated Show resolved Hide resolved

lukashuebner added the discussion label Apr 1, 2022

lukashuebner added 7 commits June 21, 2022 14:33

Fix a bug where user provded recv_counts are ignored in bcast

5783cbe

Add test for bcast: recv count and vector size differ

9e11330

Add check that recv_count == 1 if a single element buffer is passed a…

145f007

…s send_recv_buf to bcast

Enable recv_count_out in bcast

362abd0

Add test for partially transferred containers for bcast()

5f29ed8

Remove unneeded send_recv_count parameter.

024fe11

Remove unneeded send_recv_count parameter type.

140adfd

lukashuebner requested a review from Hespian June 21, 2022 13:33

Hespian requested changes Jun 21, 2022

View reviewed changes

Hespian reviewed Jun 21, 2022

View reviewed changes

tests/collectives/mpi_bcast_test.cpp Outdated Show resolved Hide resolved

Incorporate feedback from Demian

afd4166

lukashuebner force-pushed the feature-bcast branch from 0152a01 to afd4166 Compare June 22, 2022 14:45

lukashuebner requested a review from Hespian June 22, 2022 14:47

Hespian requested changes Jun 22, 2022

View reviewed changes

Clean up

fc546ee

lukashuebner requested a review from Hespian June 23, 2022 08:51

Hespian approved these changes Jun 23, 2022

View reviewed changes

Hespian requested changes Jun 23, 2022

View reviewed changes

tests/collectives/mpi_bcast_test.cpp Outdated Show resolved Hide resolved

tests/collectives/mpi_bcast_test.cpp Outdated Show resolved Hide resolved

tests/collectives/mpi_bcast_test.cpp Outdated Show resolved Hide resolved

tests/collectives/mpi_bcast_test.cpp Outdated Show resolved Hide resolved

Merge branch 'main' into feature-bcast

5baa5c4

lukashuebner added 4 commits June 24, 2022 08:25

Add some KASSERT_FAILS tests to Communicator::bcast(...)

edacedd

Mark variable as [[maybe_unused]] in bcast test.

0b99b71

Run clang-format

8eec8d0

.

bd75106

lukashuebner requested a review from Hespian June 24, 2022 06:33

Hespian approved these changes Jun 24, 2022

View reviewed changes

tests/collectives/mpi_bcast_test.cpp Show resolved Hide resolved

tests/collectives/mpi_bcast_test.cpp Outdated Show resolved Hide resolved

Reformulate comment

2d4ab53

lukashuebner merged commit 4aace80 into main Jun 24, 2022

lukashuebner deleted the feature-bcast branch June 24, 2022 08:18

Hespian mentioned this pull request Dec 13, 2022

Treat send_recv_buf as recv_buf in make_mpi_result #458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add .bcast() #192

Add .bcast() #192

lukashuebner commented Mar 24, 2022 •

edited

Loading

This comment was marked as resolved.

This comment was marked as outdated.

lukashuebner commented Mar 25, 2022 •

edited

Loading

Hespian commented Mar 25, 2022

Hespian Mar 25, 2022

lukashuebner Mar 31, 2022

Hespian Mar 31, 2022

lukashuebner Apr 4, 2022

Hespian Apr 4, 2022

Hespian May 13, 2022 •

edited

Loading

lukashuebner May 16, 2022

lukashuebner May 16, 2022 •

edited

Loading

Hespian May 16, 2022

lukashuebner May 16, 2022

lukashuebner commented Apr 1, 2022

Hespian left a comment

Hespian Jun 22, 2022

Hespian left a comment

Hespian left a comment

lukashuebner commented Jun 24, 2022

Hespian left a comment

Add .bcast() #192

Add .bcast() #192

Conversation

lukashuebner commented Mar 24, 2022 • edited Loading

This comment was marked as resolved.

This comment was marked as outdated.

lukashuebner commented Mar 25, 2022 • edited Loading

Hespian commented Mar 25, 2022

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

Hespian May 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukashuebner May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukashuebner commented Apr 1, 2022

Hespian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hespian left a comment

Choose a reason for hiding this comment

Hespian left a comment

Choose a reason for hiding this comment

lukashuebner commented Jun 24, 2022

Hespian left a comment

Choose a reason for hiding this comment

lukashuebner commented Mar 24, 2022 •

edited

Loading

lukashuebner commented Mar 25, 2022 •

edited

Loading

Hespian May 13, 2022 •

edited

Loading

lukashuebner May 16, 2022 •

edited

Loading