Reset grad attribute when called using del #16525
Conversation
|
@colesbury as we cannot control what PyObject gets, we need to add above check. |
|
Yes. This same problem occurs in some other setters in THPVariable_properties and maybe other classes. Including:
(i.e. they crash when the someone tries to delete the property) |
|
@colesbury Is |
test/test_autograd.py
Outdated
There was a problem hiding this comment.
Spaces before and after = would probably cause lint error, and so will no space after , in (5,5).
There was a problem hiding this comment.
Thanks @vishwakftw for pointing out
|
Lint is failing: |
|
@colesbury / @soumith please approve |
test/test_autograd.py
Outdated
There was a problem hiding this comment.
What does it mean for a Tensor .data field to be None ? What happens to x when you do del x.data ? Does it become unusable?
There was a problem hiding this comment.
then it's an undefined tensor and can not longer be accessed.
Will lead to Runtime error if attempted to access.
del x.data is equivalent to del x but not fine approach.
Not allowing deletion or setting data to None would be much better approach.
@colesbury @albanD what do you think?
There was a problem hiding this comment.
From my point of view, not allowing it would be better as this is a state you never want to be in (in python).
There was a problem hiding this comment.
agreed. Doing the change.
There was a problem hiding this comment.
I think this looks good, but I'll let @colesbury have another look at it
|
just fyi to all, @colesbury is on vacation. If @albanD also approves, I'm happy to ship it. |
There was a problem hiding this comment.
Could you do the same for THPVariable_set_grad_fn and forbid deleting it please (A user should never delete a grad_fn but detach() the Tensor) and THPVariable_set_backwards_hooks to make it have the same behavior as if None was passed (this shuold not be used by users, but I think it's better to remove the case where it segfaults)? Both these functions have the same problem as the one you fixed.
|
@albanD sounds good to me. |
|
@albanD not allowing deletion for |
|
But won't the check |
@albanD No. It won't as obj pointer is not same as one pointing to Py_None |
|
Ho my bad, for some reason I though it was the equality check that was causing issues. We are happy with deletion to behave like setting it to None ! |
FYI, I just pushed changes to assert if del is called on _backward_hooks |
|
This particular method is an internal one. It is just used internally at the moment. |
del Tensor.grad sets PyObject to nullptr and Tensor.grad = None sets PyObject to Py_None Handling both the cases now Handling del Tensor.requires_grad to error out requires_grad must be bool Not allowing del or setting Tensor.data to None deleting tensor data makes tensor undefined Not allowing del Tensor._grad_fn Not allowing del Tensor._backward_hooks ---Test cases added--- 1. Deletion of grad leads to grad being None 2. Deletion of data leads to Runtime error and setting to None leads to Type error 3. Deletion of requires_grad leads to Runtime error 4. Deletion of _grad_fn leads to Runtime error 5. Deletion of _backward_hooks leads to Runetime error
There was a problem hiding this comment.
@soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
8 participants
del Tensor.grad set PyObject to nullptr
and Tensor.grad = None set PyObject to Py_None
Handling both the cases now
fixes ##16471