add type annotations to torch.nn.modules.module #49045
Conversation
facebook-github-bot
added
cla signed
oncall: distributed
💊 CI failures summary and remediationsAs of commit b9382f2 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. This comment has been revised 33 times. |
Codecov Report
@@ Coverage Diff @@
## master #49045 +/- ##
==========================================
- Coverage 80.72% 80.72% -0.01%
==========================================
Files 1904 1904
Lines 206764 206764
==========================================
- Hits 166908 166907 -1
- Misses 39856 39857 +1 |
guilhermeleobas
requested review from
mingzhe09088,
mrshenli,
pritamdamania87,
rohan-varma and
zhaojuanmao
as code owners
guilhermeleobas
removed request for
rohan-varma,
pritamdamania87,
mrshenli,
mingzhe09088 and
zhaojuanmao
H-Huang
added
the
triaged
|
@guilhermeleobas could you also add comments here about the ignores you added? That helps both with review (reproducing the failures and using |
|
Also, in your next commit message, can you add that it closes gh-48640? |
|
Hi @rgommers, thanks for the review. I've addressed your comments |
There was a problem hiding this comment.
Looks good now, thanks @guilhermeleobas.
CI notifications seem stuck, but CI is done - see https://app.circleci.com/pipelines/github/pytorch/pytorch?branch=pull%2F49045. Failures are unrelated.
torch/fx/symbolic_trace.py
Outdated
| # Type ignore here is required because "n" and "p" have different types | ||
| # a few lines above | ||
| for n, p in self.root.named_buffers(): # type: ignore[assignment] |
There was a problem hiding this comment.
why cant we just use different variable names?
There was a problem hiding this comment.
Yes, that works too
torch/nn/modules/module.py
Outdated
| load = None # break load->load reference cycle | ||
| # instruction "load = None" => break load->load reference cycle | ||
| # type ignore[...] is required because mypy isn't happy with the redef of "load" | ||
| load = None # type: ignore[assignment] |
There was a problem hiding this comment.
I am not python expert. but cant we use del load to remove the function reference itself? (after all the load has been done, yes?)
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
|
Do not merge this PR until one checks if the annotations introduce any regression. See: |
There was a problem hiding this comment.
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -279,7 +279,7 @@ def parameters(self, recurse: bool = True) -> Iterator[Parameter]: | |||
|
|
|||
| def named_parameters( # type: ignore[return] | |||
There was a problem hiding this comment.
shouldn't we remove this type ignore as well since we fixed the return Parameter
There was a problem hiding this comment.
torch/distributed/nn/api/remote_module.py:280: error: Missing return statement [return]
There was a problem hiding this comment.
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
actually please rebase master --> seems like merge conflict is preventing fb internal CI to run |
There was a problem hiding this comment.
@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 5f8e1a1. |
Fixes #49044