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

Add non-recursive module.to_empty option #104197

Conversation

mikaylagawarecki
Copy link
Contributor

@mikaylagawarecki mikaylagawarecki commented Jun 26, 2023

@pytorch-bot
Copy link

pytorch-bot bot commented Jun 26, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/104197

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit a0116cd:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added release notes: nn release notes category labels Jun 26, 2023
mikaylagawarecki added a commit that referenced this pull request Jun 26, 2023
ghstack-source-id: c3ce9b2281a8b659f8a59c459f622b38ece572c1
Pull Request resolved: #104197
@mikaylagawarecki mikaylagawarecki added topic: new features topic category topic: improvements topic category and removed topic: new features topic category labels Jun 26, 2023
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.

SGTM
Note that this will break any Module that re-defines _apply with the current signature. No one should be doing that but there is a non-zero probability it is happening in the wild.

@mikaylagawarecki
Copy link
Contributor Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jun 26, 2023
@mikaylagawarecki
Copy link
Contributor Author

mikaylagawarecki commented Jun 26, 2023

@albanD ah that is true, hm is there any way to workaround this?

(just to check it only breaks those overrides of _apply that don't define *args and **kwargs is that correct?)

Upon looking it seems like we do override _apply within nn modules let me fix these

@pytorch pytorch deleted a comment from pytorch-bot bot Jun 26, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

The merge job was canceled. If you believe this is a mistake,then you can re trigger it through pytorch-bot.

mikaylagawarecki added a commit that referenced this pull request Jun 26, 2023
ghstack-source-id: 1391e6aedd6ab492d9d4551af095cd3203b811e2
Pull Request resolved: #104197
mikaylagawarecki added a commit that referenced this pull request Jun 26, 2023
ghstack-source-id: e6e4edb7410125ed07c53e24cc47bc35625afb4e
Pull Request resolved: #104197
@@ -65,7 +65,7 @@ def __init__(self, cpp_module):
if not attr.startswith("_"):
setattr(self, attr, getattr(self.cpp_module, attr))

def _apply(self, fn):
def _apply(self, fn, recurse=True):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this override technically already has the recurse=False behavior and never calls super()._apply, but adding the kwarg here just so nn.ModuleWrapper can compose with nn.Module.to_empty()

@mikaylagawarecki
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

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.

(just to check it only breaks those overrides of _apply that don't define *args and **kwargs is that correct?)

Correct!
Should we change these to *args, **kwargs btw?

@mikaylagawarecki mikaylagawarecki linked an issue Jun 26, 2023 that may be closed by this pull request
@mikaylagawarecki
Copy link
Contributor Author

mikaylagawarecki commented Jun 26, 2023

@albanD should we also make that change for other methods other than _apply? I feel as if that might apply to most of our nn.Module methods which are probably overriden in some of our core modules.

I can do this in a follow up if that makes sense!

@albanD
Copy link
Collaborator

albanD commented Jun 26, 2023

Yes that can be a follow up for sure.
Also our internal Module should not override these these days I suspect and should be using the proper hooks!

@mikaylagawarecki
Copy link
Contributor Author

mikaylagawarecki commented Jun 26, 2023

Hmm true, does that mean we should have hooks for private _apply 🤔

@facebook-github-bot facebook-github-bot deleted the gh/mikaylagawarecki/135/head branch June 30, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: nn release notes category topic: improvements topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[nn.Module] Non-recursive _apply()
3 participants