Fix typing on nn.Parameter #25586
Fix typing on nn.Parameter #25586
Conversation
pytorchbot
added
module: nn
zou3519
left a comment
zou3519
left a comment
There was a problem hiding this comment.
We didn't have a mypy check for this file before. Is it possible to add one to prevent regressions?
| import builtins | ||
|
|
||
| class Parameter(Tensor): | ||
| def __init__(self, data: Tensor, requires_grad: builtins.bool): ... |
There was a problem hiding this comment.
Why do we need to use builtin.bools? Does bool work?
There was a problem hiding this comment.
On this I took inspiration on torch/__init__.pyi.in, without knowing the ins and outs of the decision that lead them to use this one rather than the simple bool.
Can revert to simple if need be.
There was a problem hiding this comment.
In torch/__init__.pyi we cannot use bool directly because we define torch.bool, and that means that the local definition always gets picked up over the builtin. I don't know if that applies here, but it's harmless enough to use builtins.
I'm sure we can, but onclear on how to proceed |
zou3519
requested review from
ezyang
and removed request for
apaszke and
zou3519
|
@ezyang what is our testing strategy for type hints? |
|
The way the mypy test works is that we collect up all docblock examples, and then typecheck them to see if the usages typecheck correctly. If it makes sense to add a Parameter example here, that would be an easy way to test this particular change (but it's only one test for very many that we could have better coverage for). Another possibility is to add some more "test files" which we test type checking against. In any case, I don't think that should block the fix here. |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@jongwook Yes absolutely. Do you think you could submit a PR for this? Tag me as reviewer |
7 participants
Fixes #25399
As per #25580 I'm pushing this to test my changes on the CI