Fix pipelined self-copy ranks#147
Conversation
romerojosh
left a comment
romerojosh
left a comment
There was a problem hiding this comment.
Hi again @fallintoplace,
So for this PR, it seems the addition here is to ensure that both dst_rank and src_rank are consistently self. To be clear, this is not fixing any observable bug in the code. In multi-rank paths, only dst_rank is used and in the single rank paths that use src_rank, getAlltoallPeerRanks only returns self. However, setting src_rank explicitly to comm_rank is useful hardening for any future edits that might use this value.
Please remove the added asserts and I will approve the PR. Thanks!
| src_rank = comm_rank; | ||
| dst_rank = comm_rank; | ||
| } | ||
| assert(dst_rank >= 0 && dst_rank < static_cast<int>(splits_a.size())); |
There was a problem hiding this comment.
Remove the added assert calls in this PR. They are asserting things that should already be true by construction and don't provide a lot of utility.
There was a problem hiding this comment.
Thank you for your attention @romerojosh.
I have amended the PR with DCO.
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
fallintoplace
force-pushed
the
fix/pipelined-self-copy-ranks
branch
from
0ebfbbd to
8bd27fe
Compare
|
/build |
|
🚀 Build workflow triggered! View run |
|
✅ Build workflow passed! View run |
romerojosh
left a comment
romerojosh
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the contribution.
2 participants
Summary
Validation
git diff --check