feat: add allGather library #3886

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.

Sign up for GitHub

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

Jump to bottom

Merged

Sh0g0-1758 merged 46 commits into charmplusplus:main from anant37289:allGather

May 2, 2025

Contributor

Sh0g0-1758 commented Apr 20, 2025

cc: @lvkale @mayantaylor @ericjbohm

anant37289 and others added 2 commits

April 20, 2025 23:24


          feat: add allgather

51caff1


          feat: add header for allGather and clean up the cc and ci files

e60d9f7

mayantaylor reviewed

View reviewed changes

src/libs/ck-libs/allGather/README.md Outdated Show resolved Hide resolved

mayantaylor reviewed

View reviewed changes

src/libs/ck-libs/allGather/README.md Outdated Show resolved Hide resolved

mayantaylor reviewed

View reviewed changes

src/libs/ck-libs/allGather/README.md Outdated Show resolved Hide resolved

mayantaylor reviewed

View reviewed changes

src/libs/ck-libs/allGather/README.md Outdated Show resolved Hide resolved

Collaborator

mayantaylor commented Apr 23, 2025 •

edited by Sh0g0-1758

Loading

Additionally, can we do the following things:

add an example in examples/charm++
make sure the charm++ build also builds this library correctly (this might be happening already, but might require some cmake tweaks? not sure). This is also related to my comment about build. Do we want this to build by default?
maybe have @ericjbohm look at the usage of ncpy functions

stwhite91 reviewed

View reviewed changes

src/libs/ck-libs/allGather/README.md Outdated Show resolved Hide resolved

anant37289 added 3 commits

April 25, 2025 21:04


          docs: update readme

fbaf757


          feat: add example


          fix: remove n from all gther interface

9dd2338

ericjbohm added the feature label

ericjbohm self-requested a review

April 25, 2025 16:50


          chore: clean example Makefile

5675f1c

ericjbohm previously requested changes

View reviewed changes

Contributor

ericjbohm left a comment

Seems functional, but UNREG is unlikely to perform well in latency sensitive contexts.

src/libs/ck-libs/allGather/allGather.hh Outdated Show resolved Hide resolved

src/libs/ck-libs/allGather/allGather.ci Outdated Show resolved Hide resolved

stwhite91 reviewed

View reviewed changes

examples/charm++/allGather/user.cc Show resolved Hide resolved

examples/charm++/allGather/user.cc Outdated Show resolved Hide resolved

src/libs/ck-libs/allGather/allGather.hh Outdated Show resolved Hide resolved

src/libs/ck-libs/allGather/allGather.hh Outdated Show resolved Hide resolved

src/libs/ck-libs/allGather/allGather.cc Outdated Show resolved Hide resolved

Sh0g0-1758 and others added 7 commits

April 28, 2025 22:12


          nokeep and new in constructor

845b2ec


          clean

18b2e2b


          move new to constructor

f1ccbd3


          feat: change registration mode to prereg

6f4db77


          update ckreduction with nop

246532c


          add random seed

c4a512c


          free user rdma buffers

fb655c2

Sh0g0-1758 requested review from ericjbohm, lvkale, mayantaylor and stwhite91

April 29, 2025 16:16

Sh0g0-1758 added 3 commits

April 29, 2025 22:03


          fix unreg->prereg in ci

f0ac0ac


          make allGather to be built by default

dc7f9d4

nit

1f08aa9

Sh0g0-1758 and others added 5 commits

May 1, 2025 17:40


          simplify reduction

774a4c1


          simplify reduction - 2

79a5f46


          docs: update readme

af9e30a


          fix: unreg to reg

b12c215


          test all allGather alogs

68c5178

Sh0g0-1758 requested review from ritvikrao and stwhite91

May 1, 2025 13:21

stwhite91 reviewed

View reviewed changes

Collaborator

stwhite91 left a comment

Can you change all .hh files to .h and .cc to .C for consistency with the rest of Charm++? Otherwise it looks good to me!

src/libs/ck-libs/allGather/allGather.ci Outdated Show resolved Hide resolved

src/libs/ck-libs/allGather/allGather.ci Outdated Show resolved Hide resolved


          docs: remove bug warning from readme

d3eac41

Sh0g0-1758 mentioned this pull request

intermittent segfaults when running allGather #3892

Open

Sh0g0-1758 added 2 commits

May 1, 2025 20:53


          fix: remove early free of data

187d899


          pass vectors as C-style arrays

9cbbb28

stwhite91 reviewed

View reviewed changes

examples/charm++/allGather/user.cc Outdated Show resolved Hide resolved

Sh0g0-1758 and others added 3 commits

May 1, 2025 21:50


          put zero_copy_callback for src

74d1ba9


          fix: use std::copy

a2c8945


          fix: use std::copy

66a010d

Sh0g0-1758 requested a review from stwhite91

May 1, 2025 16:39

stwhite91 reviewed

View reviewed changes

src/libs/ck-libs/allGather/allGather.cc Show resolved Hide resolved

Sh0g0-1758 added 5 commits

May 1, 2025 22:54


          zero out state variables

c9421f0


          rename files to be consistent with other files in charm++

ce82cde


          fix for new file extensions

299e849


          fix for user tests

0dd68e8


          fix for main CMakeLists.txt

6b8a212

Sh0g0-1758 requested a review from stwhite91

May 1, 2025 17:38

stwhite91 reviewed

View reviewed changes

src/libs/ck-libs/allGather/allGather.ci Outdated Show resolved Hide resolved

src/libs/ck-libs/allGather/README.md Outdated Show resolved Hide resolved


          feat: add re-entrance test and remove hypercube store and index copies

637a559

Sh0g0-1758 requested a review from stwhite91

May 2, 2025 17:47

fmt

b9dd0fb

stwhite91 approved these changes

View reviewed changes

Collaborator

stwhite91 left a comment

Looks good to me!

Sh0g0-1758 merged commit 9b0830a into charmplusplus:main

22 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

anant37289 anant37289 left review comments

stwhite91 stwhite91 approved these changes

mayantaylor mayantaylor approved these changes

lvkale Awaiting requested review from lvkale

ericjbohm Awaiting requested review from ericjbohm

ritvikrao Awaiting requested review from ritvikrao

Labels