Foreach op don't follow aten level debug asserts

[albanD]

Since #91846 has moved to make torch.nn.utils.clip_grad_norm_() use foreach ops, it throws the error message:
*** RuntimeError: t.storage().use_count() == 1 INTERNAL ASSERT FAILED at "caffe2/torch/csrc/autograd/autograd_not_implemented_fallback.cpp":189, please report a bug to PyTorch.

when PyTorch is built with debug asserts.

Given the assert, the error is that this foreach op should be returning a brand new Tensor but it is actually returning a Tensor that shares storage with at least another one.

• If this is done on purpose for a good reason, we should remove this assert
• If that is not expected, then it can be a bug in the implementation where it returns a view where it should not.

cc @ezyang @gchanan @zou3519 @crcrpar @mcarilli @ngimel

[albanD]

Setting high priority as this will prevent anyone with a debug build from using clip_grad_norm by default.
So we need to fix this either by reverting the change to foreach by default or fixing the problem here.

[crcrpar]

the implementation is based off of apex's multi_tensor_l2norm which returns a Tensor whose each element represents the norm of each input Tensor. _foreach_norm returns a list of Tensors and each of them shares storage with the result 1D Tensor

related part:

pytorch/aten/src/ATen/native/cuda/ForeachReduceOp.cu

Lines 180 to 184 in 98e1b3e

	std::vector<Tensor> result;
	result.reserve(ntensors);
	for (const auto& i : c10::irange(ntensors)) {
	result.emplace_back(ret_per_tensor[i]);
	}

(where ret_per_tensor is a 1D Tensor containing results)

[albanD]

Ok, that's what I was expecting. We should remove the debug assert for this function then.
Any other foreach op uses the same "trick" where all the outputs are a view into the same buffer?

[crcrpar]

I'm not aware of any other foreach with the "trick" :)

[ngimel]

Should we instead fix schema to return Tensor(a)[]?

[albanD]

Well, technically the outputs are not views as they look at independent parts of the Tensor. And (unless you do shady stuff) you cannot change the other Tensors from one Tensor. So they don't have to be marked as views.

[ngimel]

Cool, @crcrpar can you please send a PR exempting for_each_norm from that assert in autograd_not_implemented_fallback.cpp?

[albanD]

Updating the fallback kernel to special case for this one sounds ok to me: https://github.com/pytorch/pytorch/blob/master/torch/csrc/autograd/autograd_not_implemented_fallback.cpp#L189

[ezyang]

I get nervous though because functionalization uses storage to determine views and these are definitely sharing storages.

[albanD]

Well, these are views from the point of view of functionalization but not from the point of view of autograd? :)

[ngimel]

Here, the outputs are views into the intermediate tensor, so no one can modify the base, so for the purposes of functionalization it should be fine (unless someone does naughty stuff and reaches into a different tensor via as_strided and offsets)