Add kwarg support to nn.Module hooks
#82042
Closed
+159
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
🔗 Helpful links
❌ 19 New FailuresAs of commit 0a19b0413a (more details on the Dr. CI page): Expand to see more
🕵️ 19 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages
|
Padarn
commented
Jul 23, 2022
ezyang
added
the
triaged
Contributor
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
Contributor
|
/easycla As part of the transition to the PyTorch Foundation, this project now requires contributions be covered under the new CLA. See #85559 for additional details. This comment will trigger a new check of this PR. If you are already covered, you will simply see a new "EasyCLA" check that passes. If you are not covered, a bot will leave a new comment with a link to sign. |
|
|
Collaborator
|
Hi @Padarn! Some of us are very interested in the change in this PR and would like this landed in the next week. We were wondering if you had to bandwidth to try to finish it. We would probably want unit tests, to fix the lint errors, and to add the |
Contributor
|
+1 for this feature |
Contributor
Author
Collaborator
I will let @albanD comment on whether he is content with the exact approach to add an I think you can go ahead and finish it up because any change to the exact logic to differentiate the two code paths should not be too hard to replace. |
Contributor
Author
|
Makes sense, I'll get on it in the next day or so.
…On Sat, 29 Oct 2022, 9:50 pm Andrew Gu, ***@***.***> wrote:
Hi @awgu <https://github.com/awgu>, yes I'm happy to finish it up! I was
really just waiting for some feedback (as noted #35643
<#35643>) before I added
docs/tests.
If no problems with the proposed implementation I'll do that.
I will let @albanD <https://github.com/albanD> comment on whether he is
content with the exact approach to add an with_kwargs attribute on the
hook, but I think the general approach to have two code paths
differentiated by some flag is fine.
I think you can go ahead and finish it up because any change to the exact
logic to differentiate the two code paths should not be too hard to replace.
—
Reply to this email directly, view it on GitHub
<#82042 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGRPNYEUQ2L23YRKQSY3O3WFUTRBANCNFSM54NGIALQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
By communicating with Grab Holdings Limited and/or its subsidiaries,
associate companies and jointly controlled entities (collectively, “Grab”),
you are deemed to have consented to the processing of your personal data as
set out in the Privacy Notice which can be viewed at
https://grab.com/privacy/ <https://grab.com/privacy/>
This email
contains confidential information that may be privileged and is only for
the intended recipient(s). If you are not the intended recipient(s), please
do not disseminate, distribute or copy this email. Please notify Grab
immediately if you have received this by mistake and delete this email from
your system. Email transmission may not be secure or error-free as any
information could be intercepted, corrupted, lost, destroyed, delayed or
incomplete, or contain viruses. Grab does not accept liability for any
errors or omissions in this email that arise as a result of email
transmission. All intellectual property rights in this email and any
attachments shall remain vested in Grab, unless otherwise provided by law
|
Padarn
force-pushed
the
padarn/kwargs-hooks
branch
from
0a19b04 to
533e30a
Compare
Padarn
requested review from
H-Huang,
awgu,
kwen2501,
mrshenli,
pritamdamania87,
rohan-varma and
zhaojuanmao
as code owners
Contributor
Author
|
I've updated with a test and some cleanup of the docs. The MyPy lint is complaining because I'm setting a property on the module that doesn't exist. Does anyone have a suggestion for a nicer way to do that? I'm also wondering if we should find a way to make this compatible with |
Padarn
changed the title
kwarg support to nn.Module hookskwarg support to nn.Module hooks
mrshenli
added a commit
that referenced
this pull request
Nov 21, 2022
… to take kwargs" closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042, this PR adds a `with_kwargs` argument to `register_forward_pre_hook` and `register_forward_hook` methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hood, the hook is wrapped into `_ForwardHook` and `_ForwardPreHook` types to avoid backward compatibility issues. cc jerryzh168 jianyuh raghuramank100 jamesr66a vkuzo jgong5 Xia-Weiwen leslie-fang-intel albanD mruberry jbschlosser walterddr kshitij12345 saketh-are Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 21, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042, this PR adds a `with_kwargs` argument to `register_forward_pre_hook` and `register_forward_hook` methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hood, the hook is wrapped into `_ForwardHook` and `_ForwardPreHook` types to avoid backward compatibility issues. cc jerryzh168 jianyuh raghuramank100 jamesr66a vkuzo jgong5 Xia-Weiwen leslie-fang-intel albanD mruberry jbschlosser walterddr kshitij12345 saketh-are Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 21, 2022
closes #35643 This PR is mostly copied from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042, this PR adds a `with_kwargs` argument to `register_forward_pre_hook` and `register_forward_hook` methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hood, the hook is wrapped into `_ForwardHook` and `_ForwardPreHook` types to avoid backward compatibility issues. ghstack-source-id: 9332b4f Pull Request resolved: #89389
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
… to take kwargs" closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
… to take kwargs" closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
… to take kwargs" closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
… to take kwargs" closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks @Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. ghstack-source-id: 3a4a013 Pull Request resolved: #89389
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
… to take kwargs" closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) cc albanD mruberry jbschlosser walterddr kshitij12345 saketh-are [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) cc albanD mruberry jbschlosser walterddr kshitij12345 saketh-are [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
… to take kwargs" closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) cc albanD mruberry jbschlosser walterddr kshitij12345 saketh-are [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) cc albanD mruberry jbschlosser walterddr kshitij12345 saketh-are [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
… to take kwargs" closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) cc albanD mruberry jbschlosser walterddr kshitij12345 saketh-are [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) cc albanD mruberry jbschlosser walterddr kshitij12345 saketh-are [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
… to take kwargs" closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) cc albanD mruberry jbschlosser walterddr kshitij12345 saketh-are [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) cc albanD mruberry jbschlosser walterddr kshitij12345 saketh-are [ghstack-poisoned]
mrshenli
added a commit
that referenced
this pull request
Nov 22, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks @Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. ghstack-source-id: 1f40df4 Pull Request resolved: #89389
pytorchmergebot
pushed a commit
that referenced
this pull request
Nov 23, 2022
closes #35643 This PR is mostly borrowed from #82042. Thanks @Padarn for implementing the first version and debugging into the errors. Based on the discussion in #82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) Pull Request resolved: #89389 Approved by: https://github.com/soulitzer
kulinseth
pushed a commit
to kulinseth/pytorch
that referenced
this pull request
Dec 10, 2022
) closes pytorch#35643 This PR is mostly borrowed from pytorch#82042. Thanks @Padarn for implementing the first version and debugging into the errors. Based on the discussion in pytorch#82042 this PR adds a with_kwargs argument to register_forward_pre_hook and register_forward_hook methods. When the arg is set to true, the provided hook must accept kwargs args. Under the hook, this PR adds a `_forward_pre_hooks_with_kwargs` and a `_forward_hook_with_kwargs` set to keep track of which hooks accept kwargs. Differential Revision: [D41431111](https://our.internmc.facebook.com/intern/diff/D41431111) Pull Request resolved: pytorch#89389 Approved by: https://github.com/soulitzer
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This change adds support for
kwargsin hooks. Currently, registered hooks do not receive, and cannot modify, thekwargsprovided to the moduleforward.The proposed way to address this is to add an annotation to the hook function so that it is possible to determine whether the hook should keep the previous behaviour, or instead also expect and return
kwargs.Issue
This change addresses the issue #35643 where alternative implementations
are also discussed
Testing
There is a test implemented for this functionality. Its a little basic, but shows the use of both forward and pre_forward hooks using this change.