Clone tensor before mutation for in-place tests #2774
Conversation
kshitij12345
left a comment
kshitij12345
left a comment
There was a problem hiding this comment.
LGTM, thanks @shino16
|
|
||
|
|
||
| _inplace_to_out_of_place[tanhshrink] = tanhshrink, -1 | ||
|
|
riccardofelluga
left a comment
riccardofelluga
left a comment
There was a problem hiding this comment.
Nice!
Is there a way to test this? Should we add a test to check that this is always the case?
thunder/tests/test_update_aliases.py
Outdated
| actual = j_op(*args, **sample.kwargs) | ||
| expected = op.torch_reference(*args, **sample.kwargs) | ||
| expected = op.torch_reference(*args_ref, **sample.kwargs) | ||
| assert id(actual) != id(expected) |
There was a problem hiding this comment.
This assert checks that our testing infrastructure is correct, not that the source code is correct. It's an inexpensive check, but we need to always keep in mind what these inexpensive checks add up to when run for every in-place op. Isn't the more interesting thing to test that the id of actual matches the id of args[0]?
There was a problem hiding this comment.
Interesting! That makes a lot of sense.
|
I added
|
5 participants
Fixes #2773. It clones the first argument into
opso that jitted op and eager-mode op will mutate different tensors.Alongside, I removed
_inplace_to_out_of_place[tanhshrink] = tanhshrink, -1, becausetanhshrinkis not in-place (e.g. compare withrelu_). And I couldn't find an in-place version oftanhshrink.