Skip to content

Refactor classes to inherit from nn.Module instead of nn.Sequential#17493

Merged
amyeroberts merged 6 commits intohuggingface:mainfrom
amyeroberts:implement-explicit-forward-methods
Jun 1, 2022
Merged

Refactor classes to inherit from nn.Module instead of nn.Sequential#17493
amyeroberts merged 6 commits intohuggingface:mainfrom
amyeroberts:implement-explicit-forward-methods

Conversation

@amyeroberts
Copy link
Contributor

What does this PR do?

Refactors classes that inherit from nn.Sequential to inherit from nn.Module instead. This is to make the code easier to debug and inspect.

Changes:

  • Explicit forward method implemented for classes
  • Iterating over layers and them registering to the module using add_module(str_ind, layer) in __init__. This provides backwards compatibility as the submodules will be named according to their position in the call stack, like in nn.Sequential, which is needed to load in the same checkpoints.

Note: This does not include other possible nn.Sequential refactoring within modules e.g. lxmert, or inheriting from ModuleList e.g. in Beit. These are more involved changes and should be address in separate PRs.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented May 31, 2022

The documentation is not available anymore as the PR was closed or merged.

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this! I don't think we will remove the use of nn.Sequential inside a module so this case should be left as is.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Great, thank you @amyeroberts!

@amyeroberts amyeroberts merged commit bdc0171 into huggingface:main Jun 1, 2022
@amyeroberts amyeroberts deleted the implement-explicit-forward-methods branch June 1, 2022 12:36
Narsil pushed a commit to Narsil/transformers that referenced this pull request Jun 7, 2022
…uggingface#17493)

* Adapt Maskformer, VAN, ResNet and RegNet modules to inherit from nn.Module
elusenji pushed a commit to elusenji/transformers that referenced this pull request Jun 12, 2022
…uggingface#17493)

* Adapt Maskformer, VAN, ResNet and RegNet modules to inherit from nn.Module
amyeroberts added a commit to amyeroberts/transformers that referenced this pull request Jun 16, 2022
…uggingface#17493)

* Adapt Maskformer, VAN, ResNet and RegNet modules to inherit from nn.Module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants