Skip to content

Add register_module alias to nn.Module#65174

Closed
rodrigoberriel wants to merge 4 commits intopytorch:masterfrom
rodrigoberriel:add-register-module
Closed

Add register_module alias to nn.Module#65174
rodrigoberriel wants to merge 4 commits intopytorch:masterfrom
rodrigoberriel:add-register-module

Conversation

@rodrigoberriel
Copy link
Contributor

@rodrigoberriel rodrigoberriel commented Sep 16, 2021 •
edited
Loading

Fixes #60397. I'm not sure how aliases are supposed to be implemented, but this is the most basic/direct way, IMO. As a side-effect, this implementation results in a "duplicate" doc entry, inheriting the one from add_module:

monkey-patch

An alternative implementation could be:

def register_module(self, name: str, module: Optional['Module']) -> None:
    r"""Alias for :func:`add_module`."""
    self.add_module(name, module)

which results in this documentation:

image

Questions:

  1. Should I replicate the tests? There are two for add_module: test_add_module_raises_error_if_attr_exists and test_add_module.
  2. This PR only adds register_module to nn.Module. There is an add_module in _RemoteModule, which raises NotSupported, and there is another one in ConcreteModuleTypeBuilder, which means something else, I think. Should I do anything about them?

cc @ngimel @ssnl

@facebook-github-bot facebook-github-bot added oncall: jit Add this issue/PR to JIT oncall triage queue cla signed labels Sep 16, 2021
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 16, 2021 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

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

Click here to manually regenerate this comment.

jbschlosser
jbschlosser reviewed Sep 17, 2021
View reviewed changes
Copy link
Contributor

@jbschlosser jbschlosser left a comment

Choose a reason for hiding this comment

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

Fixes #60397. I'm not sure how aliases are supposed to be implemented, but this is the most basic/direct way, IMO. As a side-effect, this implementation results in a "duplicate" doc entry, inheriting the one from add_module:

I think this way looks good and it's nice that the docs are inherited.

Questions:

  1. Should I replicate the tests? There are two for add_module: test_add_module_raises_error_if_attr_exists and test_add_module.

Rather than duplicating the tests, maybe they could be slightly expanded to iterate over the two aliased functions with the the same test logic.

  1. This PR only adds register_module to nn.Module. There is an add_module in _RemoteModule, which raises NotSupported, and there is another one in ConcreteModuleTypeBuilder, which means something else, I think. Should I do anything about them?

cc @pritamdamania for _RemoteModule. I may be wrong, but I don't think you need to touch ConcreteModuleTypeBuilder since the add_module there is unrelated.

@ssnl
Copy link
Collaborator

ssnl commented Sep 17, 2021 •
edited
Loading

I think you should do the alternative impl, for 2 reasons:

  1. the duplication of doc is confusing
  2. things like _RemoteModule should be handled indeed, but you don't need to do that explicitly if you use the alternative impl.

^Just my 2 cents. I'll let others decide!

@rodrigoberriel
Copy link
Contributor Author

rodrigoberriel commented Sep 17, 2021

I agree with @ssnl w.r.t. the implementation. I changed to the alternative, because although @jbschlosser showed support for the current one, he didn't say anything against the alternative :) Please, let me know if I should actually keep the current impl.

@jbschlosser I modified the tests to iterate over the aliases, but I'm not sure if this impl is what you were expecting.

As far as I understood, as _RemoteModule will inherit register_module, I don't need to explictly implement the alias there too. Therefore, I didn't change anything in _RemoteModule.

@codecov
Copy link

codecov bot commented Sep 18, 2021

Codecov Report

Merging #65174 (f338a49) into master (873255c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #65174      +/-   ##
==========================================
+ Coverage   66.37%   66.39%   +0.02%     
==========================================
  Files         732      735       +3     
  Lines       93616    94047     +431     
==========================================
+ Hits        62135    62445     +310     
- Misses      31481    31602     +121

@jbschlosser
Copy link
Contributor

jbschlosser commented Sep 21, 2021

I agree with @ssnl w.r.t. the implementation. I changed to the alternative, because although @jbschlosser showed support for the current one, he didn't say anything against the alternative :) Please, let me know if I should actually keep the current impl.

Cool, I'm good with the alternative as well.

@jbschlosser I modified the tests to iterate over the aliases, but I'm not sure if this impl is what you were expecting.
As far as I understood, as _RemoteModule will inherit register_module, I don't need to explictly implement the alias there too. Therefore, I didn't change anything in _RemoteModule.

Looks good!

jbschlosser
jbschlosser approved these changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@jbschlosser jbschlosser left a comment

Choose a reason for hiding this comment

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

LGTM

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 21, 2021

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

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 22, 2021

@jbschlosser merged this pull request in b80bdcc.

@rodrigoberriel rodrigoberriel deleted the add-register-module branch September 23, 2021 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbschlosser jbschlosser jbschlosser approved these changes

@albanD albanD Awaiting requested review from albanD

Assignees

No one assigned

Labels

cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FR] Module.register_module alias for add_module

5 participants

@rodrigoberriel @facebook-github-bot @ssnl @jbschlosser @pytorchbot