-
Notifications
You must be signed in to change notification settings - Fork 486
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
Add out-of-place reduce-scatter coalescing #6058
Conversation
6c1923f
to
a560206
Compare
a560206
to
edd095f
Compare
I don't understand why the profiler test failed with:
|
@jonb377 hmm is it possible that |
I suppose it could be, the capture and assertion are async. Let's rerun the workflow and see if it passes, I'll also open a PR to add more headroom to the test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly lgtm, I think we should add a test too
2d2b532
to
87fd463
Compare
87fd463
to
451f9c5
Compare
@JackCaoG is is failing due to flaky test? It was green until I rebased.
|
Done adding new test. Please take a look. Thanks. |
yea that flaky test is already fixed in head, you can ignore it for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I will take care of the backport once #6059 (review) is also merged |
In #5956 we added reduce-scatter coalescing, but the out-of-place was combined with the in-place processing, making it hard to understand/maintain the code.
Per recommendation from reviewer, this PR adds the out-of-place version of reduce-scatter coalescing.