-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
[Mypy] Part 3 fix typing for nested directories for most of directory #4161
Changes from 9 commits
6ee091c
d6dc881
fdb2674
58f852e
d2c9a96
81313dc
dcb4d00
ca8e87b
f8030a1
58199f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,8 +108,7 @@ def ncclGetUniqueId() -> NcclUniqueId: | |
] | ||
|
||
|
||
# enums | ||
class ncclDataType_t(ctypes.c_int): | ||
class ncclDataType_t: | ||
ncclInt8 = 0 | ||
ncclChar = 0 | ||
ncclUint8 = 1 | ||
|
@@ -128,7 +127,7 @@ class ncclDataType_t(ctypes.c_int): | |
ncclNumTypes = 10 | ||
|
||
@classmethod | ||
def from_torch(cls, dtype: torch.dtype) -> 'ncclDataType_t': | ||
def from_torch(cls, dtype: torch.dtype) -> int: | ||
if dtype == torch.int8: | ||
return cls.ncclInt8 | ||
if dtype == torch.uint8: | ||
|
@@ -157,7 +156,7 @@ class ncclRedOp_t(ctypes.c_int): | |
ncclNumOps = 5 | ||
|
||
@classmethod | ||
def from_torch(cls, op: ReduceOp) -> 'ncclRedOp_t': | ||
def from_torch(cls, op: ReduceOp) -> int: | ||
if op == ReduceOp.SUM: | ||
return cls.ncclSum | ||
if op == ReduceOp.PRODUCT: | ||
|
@@ -180,8 +179,8 @@ def from_torch(cls, op: ReduceOp) -> 'ncclRedOp_t': | |
_c_ncclAllReduce = nccl.ncclAllReduce | ||
_c_ncclAllReduce.restype = ctypes.c_int | ||
_c_ncclAllReduce.argtypes = [ | ||
ctypes.c_void_p, ctypes.c_void_p, ctypes.c_size_t, ncclDataType_t, | ||
ncclRedOp_t, ctypes.c_void_p, ctypes.c_void_p | ||
ctypes.c_void_p, ctypes.c_void_p, ctypes.c_size_t, ctypes.c_int, | ||
ctypes.c_void_p, ctypes.c_void_p | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not intuitive. Let's keep Does mypy understand the call signature of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found ncclDataType_t being a random enum and type at the same time a bit weird though. Why don't we then just
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, that's also doable. Please use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sgtm! |
||
] | ||
|
||
# equivalent to c declaration: | ||
|
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 weird that mypy can't handle this, it should defenitely return the
c_int
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.
Hmm when I added a breakpoint, it does return int.
Do you think it is a bug?
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.
Well, technically, this function indeed returns
int
.ctypes
will automatically convert int toc_int
. Note thatctypes.c_int
is a class/container to hold anint
, and itself is not anint
.TL;DR;
def from_torch(cls, dtype: torch.dtype) -> int:
is correct.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.
I fixed this way so that it returns the correct type.
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.
oh @youkaichao I just saw your msg. So just returning int makes more sense if there's automatic conversion then ^?
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.
I think automatic conversion makes more sense. The above code is difficult to read.
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.
I think technically inheriting c_int here is not necessary then? it is just like a simple enum
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.
yes, that's ChatGPT's fault 😉
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.
Fixed. PTAL!
81313dc