add type annotations to torch.nn.modules.normalization by guilhermeleobas · Pull Request #49035 · pytorch/pytorch

guilhermeleobas · 2020-12-08T20:19:42Z

codecov · 2020-12-08T23:51:44Z

Codecov Report

Merging #49035 (59aba29) into master (e29082b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #49035      +/-   ##
==========================================
- Coverage   80.71%   80.70%   -0.01%     
==========================================
  Files        1904     1904              
  Lines      206598   206598              
==========================================
- Hits       166750   166742       -8     
- Misses      39848    39856       +8

rgommers

Thanks @guilhermeleobas. Just a couple of minor comments.

rgommers

LGTM now, thanks @guilhermeleobas

walterddr · 2020-12-30T00:04:24Z

            input, self.normalized_shape, self.weight, self.bias, self.eps)

-    def extra_repr(self) -> Tensor:
+    def extra_repr(self) -> str:


wonder why it was not caught before.

walterddr · 2020-12-30T00:10:30Z



-_shape_t = Union[int, List[int], Size]
+_shape_t = Union[int, Sequence[int], Size]


do we have a test to validate JIT-ability? since we knew Sequence & Tuple doesnt work on JIT.

Actually this is not necessary. I am reverting this particular change

guilhermeleobas · 2020-12-31T17:30:41Z

Do not merge this PR until one checks if the annotations introduce any regression. See:
#49564 (comment)

facebook-github-bot

@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

guilhermeleobas · 2021-01-08T15:50:02Z

Tests are covered by test cases in the TestJitGeneratedModule. See:

test/test_jit.py::TestJitGeneratedModule::test_nn_LayerNorm_1d_elementwise_affine
test/test_jit.py::TestJitGeneratedModule::test_nn_LayerNorm_1d_empty_elementwise_affine
test/test_jit.py::TestJitGeneratedModule::test_nn_LayerNorm_1d_no_elementwise_affine
test/test_jit.py::TestJitGeneratedModule::test_nn_LayerNorm_3d_elementwise_affine
test/test_jit.py::TestJitGeneratedModule::test_nn_LayerNorm_3d_no_elementwise_affine

walterddr

looks good. one minor comment.

also could you confirm the coverage for LayerNorm1d constructor arguments in torch.test._internal.common_nn actually consist of non-tuple type input? from what I see all of those constructor_args are lists. not sure if they collapse into single integer

walterddr · 2021-01-08T16:36:46Z

+            normalized_shape = (normalized_shape,)  # type: ignore[assignment]
+        self.normalized_shape = tuple(normalized_shape)  # type: ignore[arg-type]


normalized_

Suggested change

normalized_shape = (normalized_shape,) # type: ignore[assignment]

self.normalized_shape = tuple(normalized_shape) # type: ignore[arg-type]

normalized_shape_ = (normalized_shape,)

self.normalized_shape = tuple(normalized_shape_)

also wondering do we even need the second tuple() wrapper?

I guess the second tuple() is to make sure self.normalized_shape is a tuple and not a list, for instance.

Also, I do not think the suggested change works as expected. If the input arg normalized_shape is a list, then the if block will never be executed and normalized_shape_ will not be defined.

facebook-github-bot

@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-01-12T15:45:36Z

@walterddr merged this pull request in 4411b5a.

Summary: Fixes pytorch#49034 Pull Request resolved: pytorch#49035 Test Plan: Imported from GitHub, without a `Test Plan:` line. Force rebased to deal with merge conflicts Reviewed By: zhangguanheng66 Differential Revision: D25767065 Pulled By: walterddr fbshipit-source-id: ffb904e449f137825824e3f43f3775a55e9b011b

guilhermeleobas added the module: typing Related to mypy type annotations label Dec 8, 2020

guilhermeleobas self-assigned this Dec 8, 2020

facebook-github-bot added the cla signed label Dec 8, 2020

pytorchbot added the open source label Dec 8, 2020

guilhermeleobas marked this pull request as ready for review December 9, 2020 14:51

guilhermeleobas requested a review from rgommers December 9, 2020 14:51

H-Huang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Dec 10, 2020

rgommers mentioned this pull request Dec 13, 2020

[typing] overly restrictive List[int] #40659

Open

rgommers requested changes Dec 13, 2020

View reviewed changes

Comment thread torch/nn/modules/normalization.py Outdated

Comment thread torch/nn/modules/normalization.py Outdated

guilhermeleobas added 3 commits December 15, 2020 22:20

add type annotations to torch.nn.modules.normalization

4cfa0ac

address comments

3d9b430

closes gh-49034

fa5cc8c

guilhermeleobas requested a review from rgommers December 15, 2020 22:28

rgommers approved these changes Dec 19, 2020

View reviewed changes

rgommers requested review from ezyang and malfet December 19, 2020 13:15

walterddr reviewed Dec 30, 2020

View reviewed changes

guilhermeleobas mentioned this pull request Dec 31, 2020

add type annotations to torch.nn.modules.conv #49564

Closed

guilhermeleobas marked this pull request as draft December 31, 2020 17:30

facebook-github-bot reviewed Jan 4, 2021

View reviewed changes

guilhermeleobas added 2 commits January 8, 2021 15:11

Merge remote-tracking branch 'upstream/master' into normalization

58d987d

revert change from List to Sequence

1c2b04d

guilhermeleobas marked this pull request as ready for review January 8, 2021 15:49

walterddr reviewed Jan 8, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into normalization

59aba29

facebook-github-bot reviewed Jan 11, 2021

View reviewed changes

malfet approved these changes Jan 11, 2021

View reviewed changes

walterddr approved these changes Jan 11, 2021

View reviewed changes

facebook-github-bot closed this in 4411b5a Jan 12, 2021

facebook-github-bot added the Merged label Jan 12, 2021



		_shape_t = Union[int, List[int], Size]
		_shape_t = Union[int, Sequence[int], Size]

		normalized_shape = (normalized_shape,) # type: ignore[assignment]
		self.normalized_shape = tuple(normalized_shape) # type: ignore[arg-type]

Conversation

guilhermeleobas commented Dec 8, 2020

Uh oh!

codecov Bot commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rgommers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rgommers left a comment

Choose a reason for hiding this comment

Uh oh!

walterddr Dec 30, 2020

Choose a reason for hiding this comment

Uh oh!

walterddr Dec 30, 2020

Choose a reason for hiding this comment

Uh oh!

guilhermeleobas Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

guilhermeleobas commented Dec 31, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

guilhermeleobas commented Jan 8, 2021

Uh oh!

walterddr left a comment

Choose a reason for hiding this comment

Uh oh!

walterddr Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

walterddr Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

guilhermeleobas Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

guilhermeleobas Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov Bot commented Dec 8, 2020 •

edited

Loading