Skip to content
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

Make standard container classes satisfy container Protocols. #109706

Open
randolf-scholz opened this issue Sep 20, 2023 · 2 comments
Open

Make standard container classes satisfy container Protocols. #109706

randolf-scholz opened this issue Sep 20, 2023 · 2 comments
Labels
module: nn Related to torch.nn needs research We need to decide whether or not this merits inclusion, based on research world triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@randolf-scholz
Copy link
Contributor

randolf-scholz commented Sep 20, 2023

🚀 The feature, motivation and pitch

The container classes in torch/nn/modules/container satisfy the necessary methods to use corresponding collections.abc Mixins, I propose to either subclass these, or to satisfy the protocols manually:

  • nn.Sequential -> MutableSequence[M]
  • nn.ModuleList -> MutableSequence[M]
  • nn.ModuelDict -> MutableMapping[str, M]
  • nn.ParameterList -> Sequence[P] (missing: __delitem__ and insert for MutableSequence[M])
  • nn.ParameterDict -> MutableMapping[str, P]
  • M would be a TypeVar bound to nn.Module.
  • P would be a TypeVar bound to nn.Parameter.

Alternatives

No response

Additional context

No response

cc @albanD @mruberry @jbschlosser @walterddr @mikaylagawarecki

@randolf-scholz randolf-scholz changed the title Make standard container classes satisfy corresponding Protocols. (nn.Sequential,nn.ModuleList -> MutableSequence satisfy the MutableSequence Protocol and ModuleDict-> MutableMapping`) Make standard container classes satisfy container Protocols. Sep 20, 2023
@soulitzer soulitzer added module: nn Related to torch.nn triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Sep 20, 2023
@mikaylagawarecki
Copy link
Contributor

Related to #80821

@mikaylagawarecki mikaylagawarecki added the needs research We need to decide whether or not this merits inclusion, based on research world label Sep 22, 2023
@mikaylagawarecki
Copy link
Contributor

It seems like something similar was tried previously in #89135 (comment), but this ended up changing the metaclass nn.ModuleList, which was BC-breaking, see the linked comment for an example of a situation where this was BC-breaking.

I tried this locally as well to verify and got before

>>> print(type(torch.nn.ModuleList))
<class 'type'>

after:

>>> print(type(torch.nn.ModuleList))
<class 'abc.ABCMeta'>

Could you share more details about why satisfying container Protocols would be helpful? Additionally, do you have ideas for how to prevent the BC-break?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: nn Related to torch.nn needs research We need to decide whether or not this merits inclusion, based on research world triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
Status: To pick up
Development

No branches or pull requests

3 participants