Skip to content

Add register_module alias to nn.Module #65174

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.

Sign up for GitHub

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

Jump to bottom
Closed
rodrigoberriel wants to merge 4 commits into from
Closed

Add register_module alias to nn.Module #65174

rodrigoberriel wants to merge 4 commits into from

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