-
Notifications
You must be signed in to change notification settings - Fork 24.3k
Fix allow_mutation_on_saved_tensors for inplace foreach #145520
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
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/145520
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit d8db1d6 with merge base e57cdb8 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
# The same exact tensor has been cloned already | ||
continue | ||
ctx.cloned[handle] = ctx.original[handle].clone() | ||
del ctx.original[handle] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unchanged - factored out to a function
[ghstack-poisoned]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
del ctx.original[handle] | ||
if arg.is_out: | ||
maybe_clone(kwargs["out"]) | ||
elif isinstance(args[idx], list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are arguments always normalized as lists here? Or they could be any sequence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so - we rely the jit logic to convert ivalues to PyObjects which should always map ivalue list types to py::list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
Fixes #117140 Pull Request resolved: #145531 Approved by: https://github.com/mikaylagawarecki ghstack dependencies: #145520
Fixes #93045 #93044 From previous discussion #93045 (comment) the resolution is that we're okay with removing this. Some older attempts: - #102581 - #109249 Pull Request resolved: #145533 Approved by: https://github.com/lezcano, https://github.com/malfet ghstack dependencies: #145520, #145531
Pull Request resolved: pytorch#145520 Approved by: https://github.com/albanD
Fixes pytorch#117140 Pull Request resolved: pytorch#145531 Approved by: https://github.com/mikaylagawarecki ghstack dependencies: pytorch#145520
Fixes pytorch#93045 pytorch#93044 From previous discussion pytorch#93045 (comment) the resolution is that we're okay with removing this. Some older attempts: - pytorch#102581 - pytorch#109249 Pull Request resolved: pytorch#145533 Approved by: https://github.com/lezcano, https://github.com/malfet ghstack dependencies: pytorch#145520, pytorch#145531
…torch#145399) Fixes pytorch#141292 Pull Request resolved: pytorch#145399 Approved by: https://github.com/jbschlosser ghstack dependencies: pytorch#145520, pytorch#145531, pytorch#145533
ghstack-source-id: da6c632 Pull Request resolved: pytorch/pytorch#145520
Stack from ghstack (oldest at bottom):