Skip to content
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.

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

Sliding Window Allreduce Stubs #902

Merged

Conversation

nsarka
Copy link
Collaborator

@nsarka nsarka commented Jan 23, 2024

This PR (1 out of 2 PRs) defines the structs and function stubs for sliding window allreduce. It also adds a gtest for the algorithm. The next PR will have the function implementations.

@janjust @manjugv @samnordmann @Sergei-Lebedev

@swx-jenkins3
Copy link

Can one of the admins verify this patch?

@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch 2 times, most recently from 4772b81 to 0c58ad8 Compare January 23, 2024 19:06
@nsarka nsarka changed the title Sliding Window Allreduce Sliding Window Allreduce Stubs Jan 23, 2024
Copy link
Collaborator

@janjust janjust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix exposing internal structs, then should be good to go

@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch from e814a16 to fcdacf3 Compare January 24, 2024 18:10
@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch from fcdacf3 to 06fc8d3 Compare January 29, 2024 20:09
Copy link
Collaborator

@samnordmann samnordmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR, it looks good to me. I only left minor comments mostly on the tests

src/components/tl/ucp/allreduce/allreduce.h Outdated Show resolved Hide resolved
src/components/tl/ucp/allreduce/allreduce.h Outdated Show resolved Hide resolved
src/components/tl/ucp/allreduce/allreduce.h Outdated Show resolved Hide resolved
src/components/tl/ucp/allreduce/allreduce_sliding_window.c Outdated Show resolved Hide resolved
src/components/tl/ucp/allreduce/allreduce_sliding_window.c Outdated Show resolved Hide resolved
test/gtest/coll/test_allreduce_sliding_window.cc Outdated Show resolved Hide resolved
test/gtest/coll/test_allreduce_sliding_window.h Outdated Show resolved Hide resolved
test/gtest/coll/test_allreduce_sliding_window.cc Outdated Show resolved Hide resolved
test/gtest/coll/test_allreduce_sliding_window.cc Outdated Show resolved Hide resolved
@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch 2 times, most recently from ba703a0 to 7f44345 Compare February 27, 2024 22:22
@nsarka
Copy link
Collaborator Author

nsarka commented Feb 27, 2024

Hi @samnordmann , I have updated the review with your comments.

@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch from 7f44345 to 0984efa Compare February 27, 2024 22:47
@samnordmann
Copy link
Collaborator

Hi @samnordmann , I have updated the review with your comments.

I'm seeing that many comments are not addressed. Also, the CI needs to be green. Please, can you re-request for review through github when it is ready ?

@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch 2 times, most recently from 229b6ba to 358f022 Compare February 28, 2024 20:05
@janjust janjust requested a review from samnordmann February 28, 2024 20:30
@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch 2 times, most recently from ddc2eb1 to 748c552 Compare February 28, 2024 21:34
Copy link
Collaborator

@samnordmann samnordmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

test/gtest/coll/test_allreduce.cc Outdated Show resolved Hide resolved
test/gtest/coll/test_allreduce_sliding_window.cc Outdated Show resolved Hide resolved
test/gtest/coll/test_allreduce_sliding_window.cc Outdated Show resolved Hide resolved
test/gtest/coll/test_allreduce_sliding_window.cc Outdated Show resolved Hide resolved
@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch 7 times, most recently from 5f50edf to 88877f8 Compare March 1, 2024 17:14
@janjust
Copy link
Collaborator

janjust commented Mar 4, 2024

@Sergei-Lebedev whenever you get a chance, could you review?

src/components/tl/ucp/tl_ucp.c Outdated Show resolved Hide resolved
src/components/tl/ucp/tl_ucp.c Outdated Show resolved Hide resolved
src/components/tl/ucp/barrier/barrier.h Outdated Show resolved Hide resolved
src/components/tl/ucp/tl_ucp_coll.h Outdated Show resolved Hide resolved
src/components/tl/ucp/allreduce/allreduce.h Outdated Show resolved Hide resolved
src/components/tl/ucp/tl_ucp_coll.h Outdated Show resolved Hide resolved
src/components/tl/ucp/tl_ucp_coll.h Outdated Show resolved Hide resolved
src/components/tl/ucp/tl_ucp_coll.h Outdated Show resolved Hide resolved
test/gtest/Makefile.am Show resolved Hide resolved
@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch 5 times, most recently from 6d57dc1 to 60570b9 Compare March 11, 2024 14:19
@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch from 60570b9 to d79080d Compare March 25, 2024 15:15
@janjust
Copy link
Collaborator

janjust commented Mar 25, 2024

@Sergei-Lebedev we've removed the ep array ptr, in the follow up we'll just us a macro for a cleaner implementation, but we're going to rely on the hash look up (if it comes back to be less performant, we can make the change then and provide data).

Whenever you can, please give it a quick look - I'm very eager to get this in so we can open up follow up

@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch from d79080d to c872b8d Compare March 25, 2024 15:46
src/components/tl/ucp/allreduce/allreduce_sliding_window.h Outdated Show resolved Hide resolved
src/components/tl/ucp/tl_ucp_coll.h Outdated Show resolved Hide resolved
test/gtest/coll/test_allreduce.cc Outdated Show resolved Hide resolved
test/gtest/coll/test_allreduce_sliding_window.cc Outdated Show resolved Hide resolved
test/gtest/coll/test_allreduce_sliding_window.cc Outdated Show resolved Hide resolved
@Sergei-Lebedev
Copy link
Contributor

@Sergei-Lebedev we've removed the ep array ptr, in the follow up we'll just us a macro for a cleaner implementation, but we're going to rely on the hash look up (if it comes back to be less performant, we can make the change then and provide data).

Whenever you can, please give it a quick look - I'm very eager to get this in so we can open up follow up

Thanks, LGTM, please fix few minor comments

@nsarka nsarka force-pushed the nsarka/sliding-window-allreduce-stubs branch from c872b8d to 7007c7b Compare March 26, 2024 14:52
@Sergei-Lebedev Sergei-Lebedev force-pushed the nsarka/sliding-window-allreduce-stubs branch from 7007c7b to af4ec63 Compare March 26, 2024 16:28
@Sergei-Lebedev Sergei-Lebedev enabled auto-merge (squash) March 26, 2024 16:28
@Sergei-Lebedev Sergei-Lebedev merged commit 2c2a443 into openucx:master Mar 26, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants