Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add NCCL Broadcast operation #1579

Merged
merged 6 commits into from
Jan 8, 2020
Merged

Add NCCL Broadcast operation #1579

merged 6 commits into from
Jan 8, 2020

Conversation

nvcastet
Copy link
Collaborator

Horovod needs to be compiled with HOROVOD_GPU_BROADCAST=NCCL.
Implements #1521.

@jessebenson
Copy link
Contributor

Good to see more NCCL ops!

@tgaddair
Copy link
Collaborator

Hey @nvcastet, looks like some unit tests are failing:

ERROR:tensorflow:2 root error(s) found.
(0) Unknown: Type uint8 is not supported in NCCL mode.
[[node HorovodBroadcast_Cast_0 (defined at /usr/local/lib/python3.6/dist-packages/tensorflow_core/python/framework/ops.py:1748) ]]
[[All/_7]]
(1) Unknown: Type uint8 is not supported in NCCL mode.
[[node HorovodBroadcast_Cast_0 (defined at /usr/local/lib/python3.6/dist-packages/tensorflow_core/python/framework/ops.py:1748) ]]
0 successful operations.
0 derived errors ignored.

@nvcastet nvcastet force-pushed the add_nccl_bcast branch 6 times, most recently from e08fc16 to c140a81 Compare December 22, 2019 13:20
@nvcastet nvcastet force-pushed the add_nccl_bcast branch 3 times, most recently from 26fa419 to 225003a Compare January 6, 2020 14:46
@nvcastet
Copy link
Collaborator Author

nvcastet commented Jan 6, 2020

@tgaddair It is finally ready for review :)

tgaddair
tgaddair reviewed Jan 6, 2020
View reviewed changes
Copy link
Collaborator

@tgaddair tgaddair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, @nvcastet! Just a couple questions.

horovod/common/ops/nccl_operations.h Outdated

Status Execute(std::vector<TensorTableEntry>& entries,
const Response& response) override;
NCCLOp(NCCLContext* nccl_context, HorovodGlobalState* global_state)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a previous PR (https://github.com/horovod/horovod/pull/1480/files#diff-5798de9e6fdc83e7f7bf0fa8344a56c0R65) we opted to use delegation to solve this problem by introducing CUDAOpContext. It looks like we could do something similar here, which would make things in our codebase more consistent. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree to try to keep things consistent through the code base. Let me give it a try.

horovod/common/wire/message.fbs Outdated
@@ -28,7 +28,8 @@ enum DataType:byte {
HOROVOD_FLOAT16 = 6,
HOROVOD_FLOAT32 = 7,
HOROVOD_FLOAT64 = 8,
HOROVOD_BOOL = 9
HOROVOD_BOOL = 9,
HOROVOD_BYTE = 10
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused by HOROVOD_BYTE. In what cases would it be used in place of HOROVOD_UINT8?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that we needed to keep the enums consistent between enum DataType in message.h and enum DataType in message.fbs/message_generated.h. Am i incorrect? HOROVOD_BYTE was already in message.h.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what happened. The HOROVOD_BYTE dtype was added in aa605d6 to abstract an API call to MPI_Bcast, but was subsequently simplified in 669eb9e.

The result is that HOROVOD_BYTE is no longer used for anything, so I think we can safely remove it.

@nvcastet
Add NCCL Broadcast operation
8d3acfa 
Horovod needs to be compiled with HOROVOD_GPU_BROADCAST=NCCL.
Implements horovod#1521.

Signed-off-by: Nicolas V Castet <nvcastet@us.ibm.com>
@nvcastet
Add missing ifdef for HOROVOD_GPU_BROADCAST with NCCL
523206f 
Signed-off-by: Nicolas V Castet <nvcastet@us.ibm.com>
@nvcastet
Add NCCL int8 uint8 types
93d0350 
Signed-off-by: Nicolas V Castet <nvcastet@us.ibm.com>
@nvcastet
Use byte size to perform NCCL broadcast
1251021 
Signed-off-by: Nicolas V Castet <nvcastet@us.ibm.com>
@nvcastet
Add NCCL GPU broadcast tests
45ec23d 
Signed-off-by: Nicolas V Castet <nvcastet@us.ibm.com>
@nvcastet nvcastet force-pushed the add_nccl_bcast branch 3 times, most recently from 2ef7ed5 to db24f30 Compare January 7, 2020 15:28
@nvcastet
Address review comments
62c41cf 
Signed-off-by: Nicolas V Castet <nvcastet@us.ibm.com>
@nvcastet nvcastet requested a review from tgaddair January 7, 2020 18:23
tgaddair
tgaddair approved these changes Jan 8, 2020
View reviewed changes
Copy link
Collaborator

@tgaddair tgaddair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the quick turnaround following the review comments.

@tgaddair tgaddair merged commit 80167f6 into horovod:master Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgaddair tgaddair tgaddair approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nvcastet @jessebenson @tgaddair