Make detach return an alias even under inference mode

[ezyang]

Stack from ghstack:

• Make detach return an alias even under inference mode #59633 Make detach return an alias even under inference mode

Fixes #59614

This fix isn't 100% correct but it appears to stem the bleeding.
A better fix would be understand how to detect when function
implementations don't uphold required invariants, leading to
refcount disaster.

Signed-off-by: Edward Z. Yang ezyang@fb.com

Differential Revision: D28962183

[facebook-github-bot]

💊 CI failures summary and remediations

As of commit 67e5ad9 (more details on the Dr. CI page):

---

• 1/1 failures possibly* introduced in this PR
  • 1/1 non-scanned failure(s)

---

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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

[ezyang]

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[gchanan]

this is BC-breaking, no?

[gchanan]

I guess only if AutoNonVariableType was on, which is rare.

[gchanan]

This fix isn't 100% correct but it appears to stem the bleeding.

why isn't it correct? Btw does this work with sparse? I think it will, I just recall we didn't bind alias in python previously and x[:] didn't work.

A better fix would be understand how to detect when function implementations don't uphold required invariants, leading to refcount disaster.

is it actually not legal to return non-aliases?

[ezyang]

this is BC-breaking, no? I guess only if AutoNonVariableType was on, which is rare.

Yeah, this shouldn't actually be BC breaking, because prior to inference_mode it wasn't actually possible to trigger this codepath from Python, and I think in C++ we don't really exercise this. One possibility is this may cause some minor regressions on inference in production if people were calling detach.

why isn't it correct?

I think that this specific change only improves correctness, but what I am currently missing is a good explanation of how all of these "autograd only" operations should operate when AutoNonVariableType is on. For example, we also make detach_ a no op (not modified in this PR) and I think this is wrong, but I have to think a lot more carefully about what a good resolution in that case is. I'm also unsure about whether or not you should ever be actually exercising this code

Btw does this work with sparse? I think it will, I just recall we didn't bind alias in python previously and x[:] didn't work.

Nope it doesn't.

>>> with torch.inference_mode(): torch.sparse_coo_tensor([[1]], [2]).detach()
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: sparse tensors do not have strides

is it actually not legal to return non-aliases?

The correctness conditions here are a little vague (e.g., because contiguous is OK with returning the same Tensor), but in general anywhere in PyTorch where we take a returned output and mutate it, the result MUST be an alias because otherwise you will mutate an input (not intended). One concrete example of this (but not applicable to the bug this PR fixes) is in autograd, where we mutate output tensors to set their grad_fn.

cc @robieta this is the same thing we are arguing about in #59389

[ezyang]

blegh mobile is throwing a hissy fit. Gonna do this slightly differently

[ezyang]

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@ezyang merged this pull request in 6ca141f.