KEMBAR78
Modules note v2 by jbschlosser · Pull Request #63963 · pytorch/pytorch · GitHub
Skip to content

Modules note v2 #63963

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
wants to merge 6 commits into from
Closed

Modules note v2 #63963

wants to merge 6 commits into from

Conversation

@jbschlosser
Copy link
Contributor

@jbschlosser jbschlosser commented Aug 25, 2021 •
edited
Loading

This PR expands the note on modules with additional info for 1.10.

It adds the following:

  • Examples of using hooks
  • Examples of using apply()
  • Examples for ParameterList / ParameterDict
  • register_parameter() / register_buffer() usage
  • Discussion of train() / eval() modes
  • Distributed training overview / links
  • TorchScript overview / links
  • Quantization overview / links
  • FX overview / links
  • Parametrization overview / link to tutorial

@jbschlosser jbschlosser requested review from albanD and mruberry August 25, 2021 16:44
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 25, 2021 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

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

mruberry
mruberry reviewed Aug 25, 2021
View reviewed changes
mruberry
mruberry reviewed Aug 25, 2021
View reviewed changes
mruberry
mruberry reviewed Aug 25, 2021
View reviewed changes
mruberry
mruberry reviewed Aug 25, 2021
View reviewed changes
mruberry
mruberry reviewed Aug 25, 2021
View reviewed changes
mruberry
mruberry reviewed Aug 25, 2021
View reviewed changes
mruberry
mruberry reviewed Aug 25, 2021
View reviewed changes
mruberry
mruberry reviewed Aug 25, 2021
View reviewed changes
mruberry
mruberry reviewed Aug 25, 2021
View reviewed changes
docs/source/notes/modules.rst Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

(I'm skipping this section since @albanD is our hookmaster)

mruberry
mruberry approved these changes Aug 25, 2021
View reviewed changes
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Overall looks really good! I made a few inline suggestions and left hooks for @albanD.

albanD
albanD reviewed Aug 26, 2021
View reviewed changes
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

I would love to see the Module structure for skip_init and device placement!

docs/source/notes/modules.rst Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

isinstance?
Or if you want exact match, is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stole this from the Module.apply() docs - should probably be updated there as well!

@jbschlosser jbschlosser requested a review from albanD August 26, 2021 16:26
albanD
albanD reviewed Aug 26, 2021
View reviewed changes
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

I only have comments around the device placement/skip initialization part. Everything else looks good.
I am happy to continue this discussion in a follow up PR if you think this is one is big enough already as well.

albanD
albanD approved these changes Aug 27, 2021
View reviewed changes
Copy link
Collaborator

@albanD albanD 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 for the update!

@facebook-github-bot
Copy link
Contributor

@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

@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

@jbschlosser merged this pull request in 196fd3e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albanD albanD albanD approved these changes

@mruberry mruberry mruberry approved these changes

Assignees

No one assigned

Labels

cla signed Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jbschlosser @facebook-github-bot @albanD @mruberry