-
Notifications
You must be signed in to change notification settings - Fork 93
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
Asymmetric memory #1000
Asymmetric memory #1000
Conversation
df217dc
to
b4d4338
Compare
Is it ready for review? If so, please add the label |
It is ready for review. I’m OOO, so I can’t add the label. Please let me know your opinion though on my question about supporting non-rooted collectivesOn Jul 18, 2024, at 7:27 AM, samnordmann ***@***.***> wrote:
Is it ready for review? If so, please add the label
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
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! It looks great overall! I just left a bunch of very minor comments. A couple of further question/notes:
- The CI error looks relevant
- For my own curiosity: Is there a concrete motivation for this patch? I mean, is it a user's request, or do we only think it is a nice feature to have?
- Can we hope (in the future) to have a more efficient way to handle asymmetric memory, or does it need to be performed, as here, by emulating symmetric memory with a scratch buffer and copying in/out to the user's buffer?
0bc716f
to
62d54e9
Compare
Thanks, I will take a look
A hang was reported by the HPC SDK team (I think that's the name, I'm not sure, I heard it from Tommy) in the case of asymmetric memory in a rooted collective. Consider this case:
I believe ucp can handle mismatched memory without any problems which is why I was wondering about implementing a copy-out mechanism like this for non-rooted collectives. I will test it and report back. For rooted collectives, I'm not sure yet if there's a better way. |
5e24e86
to
d17d878
Compare
6f7a658
to
a446007
Compare
456ad4b
to
6869d73
Compare
Hi @Sergei-Lebedev , I have addressed all of your comments. For scatter and scatterv, a new src buf will be allocated and copied into. To support this as a persistent collective I moved the copy-in to ucc_collective_post. Then I added a persistent gtest that will:
|
CI is failing with:
ucc test had a jenkins issue according to Artem |
bot:retest |
e9d3913
to
a1d55ea
Compare
@nsarka LGTM, please fix commit messages to pass codestyle check |
2dd8f56
to
37526d1
Compare
37526d1
to
6bdcf2e
Compare
The CI is failing on:
This seems unrelated to my changes |
In our UCC call, we defined two types of "asymmetric memory":
Since strong asymmetric memory would require an allreduce for every
ucc_collective_init
, this PR focuses on weak asymmetric memory.Previously, what would happen is when the src/dst mismatch we can see that they mismatch on every rank, so everybody errors out saying we don't support asymmetric memory. But on rooted collectives, the root will do the same, but on non-root ranks there is only one src OR dst buffer, so there's no way to know that the root is going to error out. This caused a hang.
So, this PR will enable asymmetric memory for rooted collectives. On the root, it assumes the src buffer is the "true" mem_type, and makes a scratch allocation for the dst buffer of size
dst.info.count * dt_size(dst.info.mem_type)
with mem_typesrc.info.mem_type
. After this, it will run the collective. Once the collective completes, it will copy out from the scratch allocation into the old dst buffer.@Sergei-Lebedev My only remaining question is what should we do with non-rooted collectives? Should they still error out? Or should this apply to them too?