-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Feature] Enable UVA sampling with CPU indices #3892
Conversation
To trigger regression tests:
|
I didn't follow the context very carefully but saw no change on user experience. I'm good with the PR. Will let @nv-dlasalle approve. |
Since @nv-dlasalle is out until next week, I'd like to approve this PR. |
Has anyone taken a look at how using this feature increases load on host memory? While this is an awesome solution for enabling larger graphs, it may cause problems from some memory BW-limited solutions. |
It seems that dgl.multiprocessing.spawn does not exist in main: |
@BarclayII Could you help to answer Kyle's question? |
Ah the docstring was wrong. It no longer requires |
I think the load increase should be fine since this PR only changes how we handle CPU indices, and we only require copying CPU indices (which has the same size of the minibatch itself) to GPU. |
Description
This PR enables specifying indices on CPU for UVA sampling to avoid duplicating the indices for every GPU.
It also adds a utility function
dgl.multiprocessing.call_once_and_share()
which calls a function in a single process and share the result to other processes. Requires usingdgl.multiprocessing.spawn
instead oftorch.multiprocessing.spawn
(the signatures are the same).Fixes #3855 and #3893 .
Checklist
Please feel free to remove inapplicable items for your PR.
or have been fixed to be compatible with this change