Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MNMG Neighborhood Sampling #2073
MNMG Neighborhood Sampling #2073
Changes from all commits
acf15bc
a584e0b
338b2d4
efd7e9a
a23ce0d
22b32d8
e5042fc
ca02ee4
79409e7
360c293
e57f261
a63cdea
7e6bf4c
6f3b342
fbf5f36
ffacad0
8db2e74
e560fd8
a897416
6a8000c
8b363bb
a8b3f84
1a7660c
72a90d0
a76a743
1ad5e71
fd0a383
7434d33
3a0953b
acd2b52
e22200b
f2df103
c5c50ff
225277f
b3b0304
6c00250
d284931
ecee30a
199e18b
91ad3dd
8d310e4
ba9fc30
1e70cd0
4c703c7
8550020
d88c80e
79de6c3
a1c06d0
08ad3da
c8ff67d
bd3e069
5a89958
d75e64f
97dc281
c6ffb50
5d67c25
488b352
ca1a8a8
73fbe37
08cd16d
1137531
620dbb1
cb70d7d
c47f0a7
774b1b3
e0a4200
1fd0e50
481a4f6
6f55fcf
626dbf8
cbedc59
596277f
ca4dcdb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
A thought to consider. The name and template type name for this parameter seem imply a particular implementation for the calling program. The notion of a
rank
is really an MPI notion that we use in our comms work. If a program calling this isn't using MPI or our comms then it's not necessarily clear what they would do here.It seems to me that what we need is some sort of label that identifies where we want to direct the results of the sampling for the corresponding seed. Perhaps
label_t
andptr_d_labels
might be a bit more generic.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.
This is targeting a very specific implementation with MPI (NCCL) using MPI/NCCL prims. For this very reason alone we should actually suggest the affiliation with ranks. In addition, this is what the user (GNN consumer) wants to call them.
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.
FWIW, I also found the word
rank
confusing in this context at first since it wasn't clear if it was intended to be a GPU rank for use by the sampling algo itself, or a rank used by the caller (ie. a client may have their own notion of rank independent of the MG rank used by this algo). I think a more general-purpose term likelabel
orid
not only works in this case, but can also be used as a more generic mechanism for associating results to a caller's seeds. Lastly, the GNN client will likely not be directly calling this API anyway (they'll be using python), so we can make something more application-specific for them in python if necessary.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.
If the users wouldn't call directly this API then they don't have to worry about how we call this internally. The world
rank
came in GNN meetings and emerged as part of what that community of developers seemed comfortable with.And again, if
rank
suggests MPI, it's because it's supposed to. This implementation is very much tied to MPI (via NCCL) and is not generic enough to justify other names, likelabel
. Other approaches, like say NVSHMEM won't have ranks but then they wouldn't need the labels either. This so-called labels have one meaning only: ranks.