didn't you already set this to variable_data above? So you can just skip this?

It seems unclear whether we have invariant that the SavedVariable will only be unpacked once. (I tried removing detach/variable_data here and all tests pass.) If we do have that invariant, we should always use detach/variable_data here, so that the unpacked Variable is always a shallow copy of the SavedVariable.

Two points:

1. Is there any reason this can't use re-use the version counter, i.e. detach semantics rather than data?

2. If we are going with detach semantics, passing a boolean to allow the metadata change seems reasonable -- have we not done a release yet where the "allow_metadata_change" is in?

I don't understand what's going on here, can you explain?

This is only called for legacy_apply functions? These should always be used with variable_data for some reason?

This is called for legacy_apply functions and also DelayedError::apply. The main reason we need to change this from tensor_list to variable_list is because outputs are Variables now instead of Tensors (because we now use .detach() instead of .tensor_data() to build up the outputs list in legacy_apply and DelayedError::apply).

I get that we had tensors and now we have variables, I'm trying to understand what the constraints of this code actually are. So this is used with legacy apply, which we want to get rid of, right? (#16947). How does legacy apply actually work? The forwards is wrapped in a no-grad block?

For legacy autograd functions, the forward is wrapped in a no-grad block

In legacy_apply, we have to make a shallow-copy of the output from the backward function (via detach or variable_data) before calling wrap_outputs, because for a backward function that directly pass grad_x through:

class PassthroughFunction(Function):
    ...
    def backward(self, grad_x):
        return grad_x

if grad_x requires grad, we will attach a new gradient edge to grad_x (via autograd::create_gradient_edge(output, grad_fn)), which will replace any previous gradient edge that grad_x has (which is an incorrect behavior). Hence we should make a shallow-copy of the output from the backward function in legacy_apply before calling wrap_outputs.

It seems unclear whether we have invariant that the SavedVariable will only be unpacked once. (I tried removing detach/variable_data here and all tests pass.) If we do have that invariant, we should always use detach/variable_data here.

how would you figure out if we have that invariant or not?

In the following use case, a SavedVariable can be unpacked more than once:

>>> a = torch.zeros(2, 2).fill_(3).requires_grad_()
>>> b = torch.zeros(2, 2).fill_(5).requires_grad_()
>>> c = a*b
# SavedVariable being saved:  3  3
#  3  3
# [ CPUFloatType{2,2} ]
# SavedVariable being saved:  5  5
#  5  5
# [ CPUFloatType{2,2} ]

>>> d = c.sum()
>>> d.backward(retain_graph=True)
# SavedVariable being unpacked:  3  3
#  3  3
# [ CPUFloatType{2,2} ]
# SavedVariable being unpacked:  5  5
#  5  5
# [ CPUFloatType{2,2} ]

>>> d.backward()
# SavedVariable being unpacked:  3  3
#  3  3
# [ CPUFloatType{2,2} ]
# SavedVariable being unpacked:  5  5
#  5  5
# [ CPUFloatType{2,2} ] 

In the above example, a and b are unpacked more than once, hence we should always use detach/variable_data to make a shallow-copy of the SavedVariable here, so that each unpacking does not affect the original SavedVariable.

We specifically want to use variable_data instead of detach here, because the comment above this function in NOTE [ Version Counter in comm.*_coalesced ] mentions:

// We thus re-wrap these Variables after broadcasting (i.e., effetively doing
// what is equivalent to .data in Python), and give them individual version
// counters. ...

variable_data is the strict equivalent of .data in Python, which creates new version counter for the returned tensor, and is exactly what we want.

Please see comment above.