[release] Add warning to stateless.functional_call for deprecated behavior #87079
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/87079
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 1eb4332: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
| def _named_members(mod, get_members_fn, prefix='', recurse=True, remove_duplicate=True): | ||
| r"""Helper method for yielding various names + members of modules.""" | ||
| memo = set() | ||
| modules = mod.named_modules(prefix=prefix) if recurse else [(prefix, mod)] |
There was a problem hiding this comment.
This will fix the issue with the test but does mean that we won't warn on the case where we have a recursive module (i.e. module = SomeModule(); module.m = module) and a user passes in values for multiple of the recursive values
There was a problem hiding this comment.
It would be good to use similar script as the ones in #68969 (comment) to see what is the impact of this.
| @@ -0,0 +1,32 @@ | |||
| # Polyfilled from pytorch core while we figure out the `remove_duplicate` issues. | |||
| def _named_members(mod, get_members_fn, prefix='', recurse=True, remove_duplicate=True): | |||
There was a problem hiding this comment.
These are not needed. We have them in core.
There was a problem hiding this comment.
It actually didn't make it into the release (source)
There was a problem hiding this comment.
Ho I though that landed a while back. So much happened in one week :p
| yield name, v | ||
|
|
||
|
|
||
| def _named_parameters(mod, prefix: str = '', recurse: bool = True, remove_duplicate: bool = True): |
There was a problem hiding this comment.
This one is still needed but we should really just add it to core
There was a problem hiding this comment.
Yeah I was going to leave that for post-release since it's BC-breaking and probably requires codevelopment
Just used that script and got some numbers:
So this is incurring ~15% regression on resnet |
|
Talked offline and per Alban's suggestion, moved the code to check to happen during the traversal. This reduced overhead to 5% on resnet Raw numbers
|
samdow
force-pushed
the
release/stateless_warning
branch
from
d810311
to
491fa24
Compare
We need to deprecate this behavior in 1.13 for silent correctness errors (current version gives unexpected results for users, but it is BC-breaking so we need a deprecation cycle)