Skip to content

add type annotations to torch.nn.modules.container#48969

Closed
guilhermeleobas wants to merge 7 commits intopytorch:masterfrom
guilhermeleobas:container
Closed

add type annotations to torch.nn.modules.container#48969
guilhermeleobas wants to merge 7 commits intopytorch:masterfrom
guilhermeleobas:container

Conversation

@guilhermeleobas
Copy link
Copy Markdown
Collaborator

Fixes #48968

@guilhermeleobas guilhermeleobas added the module: typing Related to mypy type annotations label Dec 7, 2020
@guilhermeleobas guilhermeleobas self-assigned this Dec 7, 2020
@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented Dec 8, 2020

💊 CI failures summary and remediations

As of commit 3a5247e (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.

@guilhermeleobas guilhermeleobas marked this pull request as ready for review December 9, 2020 15:26
@H-Huang 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
Copy link
Copy Markdown
Collaborator

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @guilhermeleobas. Just adding a couple of comments would be nice.

Comment thread torch/nn/modules/container.py Outdated
Comment thread torch/nn/modules/container.py Outdated
@guilhermeleobas
Copy link
Copy Markdown
Collaborator Author

Hi @rgommers, done!

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 16, 2020

Codecov Report

Merging #48969 (cf1301c) into master (001ff3a) will decrease coverage by 0.00%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master   #48969      +/-   ##
==========================================
- Coverage   80.60%   80.60%   -0.01%     
==========================================
  Files        1879     1879              
  Lines      202892   202894       +2     
==========================================
  Hits       163543   163543              
- Misses      39349    39351       +2     

Copy link
Copy Markdown
Collaborator

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now

@rgommers rgommers requested review from ezyang and malfet December 19, 2020 12:55
Copy link
Copy Markdown
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. thanks for the contribution!

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@guilhermeleobas
Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the only thing we need to add as additional test are

model = ModuleList(...) 
model += nn.Conv1d() 
model.append(nn.Conv2d()) 
model.extend([nn.Conv1d(), nn.Conv2d())

doesn't have to be convolution modules. we just need to validate that the iadd, append, extend APIs are ok with JIT

@guilhermeleobas
Copy link
Copy Markdown
Collaborator Author

Done! @walterddr

Should I include tests for ParameterList and Sequential as well?

@walterddr
Copy link
Copy Markdown
Contributor

yes good catch. for ParameterList it would be good if we add them. Seqeuential should be fine since you only annotated those dunder methods and they should already be covered by existing jit test.

@guilhermeleobas
Copy link
Copy Markdown
Collaborator Author

guilhermeleobas commented Jan 8, 2021

I am having some issues to add ParameterList:

class Param(nn.Module):
    def __init__(self):
        super(Param, self).__init__()
        self.params = nn.ParameterList([nn.Parameter(torch.randn(10, 10)) for i in range(10)])

    def forward(self, x):
        # ParameterList can act as an iterable, or be indexed using ints
        sz = len(self.params)
        for i in range(sz):
            p = self.params[i]
            x = self.params[i // 2].mm(x) + p.mm(x)
        return x

m = torch.jit.script(Param())

And the error:

    def create_methods_and_properties_from_stubs(concrete_type, method_stubs, property_stubs):
        method_defs = [m.def_ for m in method_stubs]
        method_rcbs = [m.resolution_callback for m in method_stubs]
        method_defaults = [get_default_args(m.original_method) for m in method_stubs]

        property_defs = [p.def_ for p in property_stubs]
        property_rcbs = [p.resolution_callback for p in property_stubs]

>       concrete_type._create_methods_and_properties(property_defs, property_rcbs, method_defs, method_rcbs, method_defaults)
E       RuntimeError:
E       Tried to access nonexistent attribute or method '__len__' of type '__torch__.torch.nn.modules.container.ParameterList'. Did you forget to initialize an attribute in __init__()?:
E         File "/home/guilhermel/git/pytorch/test/test_jit.py", line 848
E                       # ParameterList can act as an iterable, or be indexed using ints
E                       sz = len(self.params)
E                            ~~~ <--- HERE
E                       for i in range(sz):
E                           p = self.params[i]

And iterating directly in self.params gives:

    def create_methods_and_properties_from_stubs(concrete_type, method_stubs, property_stubs):
        method_defs = [m.def_ for m in method_stubs]
        method_rcbs = [m.resolution_callback for m in method_stubs]
        method_defaults = [get_default_args(m.original_method) for m in method_stubs]

        property_defs = [p.def_ for p in property_stubs]
        property_rcbs = [p.resolution_callback for p in property_stubs]

>       concrete_type._create_methods_and_properties(property_defs, property_rcbs, method_defs, method_rcbs, method_defaults)
E       RuntimeError:
E       Only constant Sequential, ModueList, or ModuleDict can be used as an iterable:
E         File "/home/guilhermel/git/pytorch/test/test_jit.py", line 847
E                   def forward(self, x):
E                       # ParameterList can act as an iterable, or be indexed using ints
E                       for p in self.params:
E                       ~~~~~~~~~~~~~~~~~~~~~
E                           x = p.mm(x)
E                           ~~~~~~~~~~ <--- HERE

@guilhermeleobas
Copy link
Copy Markdown
Collaborator Author

Last commit introduce tests for Sequential. I couldn't find a way to create a test for ParameterList.

@guilhermeleobas guilhermeleobas marked this pull request as ready for review January 8, 2021 20:18
@walterddr
Copy link
Copy Markdown
Contributor

that should be fine consider the annotations on ParameterList are essentially the same ones on ModuleList

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a 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
Copy link
Copy Markdown
Contributor

test test_ParameterList in test_nn.py actually failed for this. so yes I guess ParameterList is covered and didn't quite work. could you take a look?

@guilhermeleobas
Copy link
Copy Markdown
Collaborator Author

guilhermeleobas commented Jan 11, 2021

I had commit some changes by mistake in the __getitem__ function.

================================================================== test session starts ==================================================================
platform linux -- Python 3.9.1, pytest-6.2.1, py-1.10.0, pluggy-0.13.1 -- /home/guilhermel/.conda/envs/pytorch-cuda-dev/bin/python
cachedir: .pytest_cache
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase('/home/guilhermel/pytorch/.hypothesis/examples')
rootdir: /home/guilhermel/pytorch
plugins: hypothesis-6.0.0
collected 2054 items / 2051 deselected / 3 selected

test/test_nn.py::TestNN::test_ParameterList PASSED                                                                                                [ 33%]
test/test_nn.py::TestNN::test_parameterlistdict_pickle PASSED                                                                                     [ 66%]
test/test_nn.py::TestNN::test_parameterlistdict_setting_attributes PASSED                                                                         [100%]

Tests for ParameterList on test_nn.py are passing now. cc @walterddr

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a 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
Copy link
Copy Markdown
Contributor

walterddr commented Jan 11, 2021

rebase to try mitigate test failure

@guilhermeleobas
Copy link
Copy Markdown
Collaborator Author

Looks like CI is all green

@walterddr
Copy link
Copy Markdown
Contributor

walterddr commented Jan 14, 2021

CI is not working. only github hooks were triggered.
[Update] closing & reopening PR to trigger CI again

@walterddr walterddr closed this Jan 14, 2021
@walterddr walterddr reopened this Jan 14, 2021
@guilhermeleobas
Copy link
Copy Markdown
Collaborator Author

Fixed CI issue in my last commit

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@walterddr merged this pull request in a9e46f1.

laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
Summary:
Fixes pytorch#48968

Pull Request resolved: pytorch#48969

Reviewed By: mrshenli

Differential Revision: D25728987

Pulled By: walterddr

fbshipit-source-id: 02c3aa2078f4ed6cc6edd90ffe1177d789c328a9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed Merged module: typing Related to mypy type annotations open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable torch.nn.modules.container typechecks during CI

7 participants