-
Notifications
You must be signed in to change notification settings - Fork 25.7k
refactor torch/cuda/nccl.h to remove direct dependency on NCCL in libtorch_python #42687
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
Conversation
💊 CI failures summary and remediationsAs of commit fcf24c1 (more details on the Dr. CI page): ✅ None of the CI failures appear to be your fault 💚
🚧 1 fixed upstream failure:These were probably caused by upstream breakages that were already fixed.
Please rebase on the
|
torch/csrc/cuda/nccl.h
Outdated
| typedef enum { ncclSuccess = 0, | ||
| ncclUnhandledCudaError = 1, | ||
| ncclSystemError = 2, | ||
| ncclInternalError = 3, | ||
| ncclInvalidArgument = 4, | ||
| ncclInvalidUsage = 5, | ||
| ncclNumResults = 6 } ncclResult_t; | ||
|
|
||
| /* Reduction operation selector */ | ||
| typedef enum { ncclSum = 0, | ||
| ncclProd = 1, | ||
| ncclMax = 2, | ||
| ncclMin = 3, | ||
| ncclNumOps = 4 } ncclRedOp_t; | ||
|
|
||
| /* Data types */ | ||
| typedef enum { ncclInt8 = 0, ncclChar = 0, | ||
| ncclUint8 = 1, | ||
| ncclInt32 = 2, ncclInt = 2, | ||
| ncclUint32 = 3, | ||
| ncclInt64 = 4, | ||
| ncclUint64 = 5, | ||
| ncclFloat16 = 6, ncclHalf = 6, | ||
| ncclFloat32 = 7, ncclFloat = 7, | ||
| ncclFloat64 = 8, ncclDouble = 8, | ||
| ncclNumTypes = 9 } ncclDataType_t; | ||
|
|
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.
Can you use enum class instead of C-style enums (and if aliases are not allowed in enum class defined them using using construct?
| typedef enum { ncclSuccess = 0, | |
| ncclUnhandledCudaError = 1, | |
| ncclSystemError = 2, | |
| ncclInternalError = 3, | |
| ncclInvalidArgument = 4, | |
| ncclInvalidUsage = 5, | |
| ncclNumResults = 6 } ncclResult_t; | |
| /* Reduction operation selector */ | |
| typedef enum { ncclSum = 0, | |
| ncclProd = 1, | |
| ncclMax = 2, | |
| ncclMin = 3, | |
| ncclNumOps = 4 } ncclRedOp_t; | |
| /* Data types */ | |
| typedef enum { ncclInt8 = 0, ncclChar = 0, | |
| ncclUint8 = 1, | |
| ncclInt32 = 2, ncclInt = 2, | |
| ncclUint32 = 3, | |
| ncclInt64 = 4, | |
| ncclUint64 = 5, | |
| ncclFloat16 = 6, ncclHalf = 6, | |
| ncclFloat32 = 7, ncclFloat = 7, | |
| ncclFloat64 = 8, ncclDouble = 8, | |
| ncclNumTypes = 9 } ncclDataType_t; | |
| enum class ncclResult { | |
| Success = 0, | |
| UnhandledCudaError = 1, | |
| SystemError = 2, | |
| InternalError = 3, | |
| InvalidArgument = 4, | |
| InvalidUsage = 5, | |
| NumResults = 6, | |
| }; | |
| /* Reduction operation selector */ | |
| enum class ncclRedOpt_t { Sum = 0, | |
| Prod = 1, | |
| Max = 2, | |
| Min = 3, | |
| NumOps = 4 }; | |
| /* Data types */ | |
| enum class ncclDataType { | |
| Int8 = 0, | |
| Char = 0, | |
| Uint8 = 1, | |
| Int32 = 2, | |
| Uint32 = 3, | |
| Int64 = 4, | |
| Uint64 = 5, | |
| Float16 = 6 | |
| Float32 = 7 | |
| Float64 = 8, | |
| ncclNumTypes = 9 | |
| }; | |
| using , ncclDouble = ncclDataType:Float64; | |
b2e2881 to
e2aeb6e
Compare
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 for the review @malfet. there were some issues during build time after I rebase, I will try to fix them asap.
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.
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
2bc743a to
fcf24c1
Compare
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.
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@walterddr merged this pull request in 3eb3132. |
No description provided.