Follow up on freezing #34786
Follow up on freezing #34786
Conversation
1) Rename 'HashIValue' to 'HashAliasedIValue' 2) Added Object case in getSubValues function 3) Hashes tensors to their storage 4) Added Dict case in orverrideGradient 5) nit clean up [ghstack-poisoned]
facebook-github-bot
added
the
oncall: jit
💊 CircleCI build failures summary and remediationsAs of commit 7163bf3 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no CircleCI 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 on the GitHub issue tracker. This comment has been revised 34 times. |
1) Rename 'HashIValue' to 'HashAliasedIValue' 2) Added Object case in getSubValues function 3) Hashes tensors to their storage 4) Added Dict case in orverrideGradient 5) nit clean up [ghstack-poisoned]
1) Rename 'HashIValue' to 'HashAliasedIValue' 2) Added Object case in getSubValues function 3) Hashes tensors to their storage 4) Added Dict case in orverrideGradient 5) nit clean up [ghstack-poisoned]
| @@ -227,6 +227,13 @@ class AttributePropagator { | |||
| elems.set(i, overrideGradient(elems.extract(i))); | |||
| } | |||
| attr = std::move(elems); | |||
| } else if (attr.isGenericDict()) { | |||
There was a problem hiding this comment.
For the override gradient implementation here, can you call getSubValues and then remove the gradient for all tensor subvalues ?
There was a problem hiding this comment.
the gradient is not overridden inplace because it is detached so that the original module has no side effects so I can really use getSubValues
There was a problem hiding this comment.
Okay, that makes sense. If that's the case don't you need to return the recursively constructed new attribute and reset it as the attribute ?
here for example we're not returning a new tuple with the detatched gradients. so it's either inplace, which we're not trying to do, or we're not changing anything.
There was a problem hiding this comment.
cc @Krovatkin @smessmer @zdevito how to do correct detaching of gradients in these cases
| case Tag::Object: { | ||
| subValues.insert(*this); | ||
| auto obj_type = type()->expect<ClassType>(); | ||
| auto obj_value = toObject(); |
There was a problem hiding this comment.
https://github.com/pytorch/pytorch/pull/32178/files#r385953414 for this comment, could you address or clarify in the code ?
1) Rename 'HashIValue' to 'HashAliasedIValue' 2) Added Object case in getSubValues function 3) Hashes tensors to their storage 4) Added Dict case in orverrideGradient 5) nit clean up [ghstack-poisoned]
| @@ -227,6 +227,13 @@ class AttributePropagator { | |||
| elems.set(i, overrideGradient(elems.extract(i))); | |||
| } | |||
| attr = std::move(elems); | |||
| } else if (attr.isGenericDict()) { | |||
There was a problem hiding this comment.
Okay, that makes sense. If that's the case don't you need to return the recursively constructed new attribute and reset it as the attribute ?
here for example we're not returning a new tuple with the detatched gradients. so it's either inplace, which we're not trying to do, or we're not changing anything.
1) Rename 'HashIValue' to 'HashAliasedIValue' 2) Added Object case in getSubValues function 3) Hashes tensors to their storage 4) Added Dict case in orverrideGradient 5) nit clean up [ghstack-poisoned]
1) Rename 'HashIValue' to 'HashAliasedIValue' 2) Added Object case in getSubValues function 3) Hashes tensors to their storage 4) Added Dict case in orverrideGradient 5) nit clean up [ghstack-poisoned]
1) Rename 'HashIValue' to 'HashAliasedIValue' 2) Added Object case in getSubValues function 3) Hashes tensors to their storage 4) Added Dict case in orverrideGradient 5) nit clean up Differential Revision: [D20585270](https://our.internmc.facebook.com/intern/diff/D20585270) [ghstack-poisoned]
Stack from ghstack:
Differential Revision: D20585270