Add names= argument to torch.tensor ctor

[zou3519]

Stack from ghstack:

• Add names= argument to torch.tensor ctor #25424 Add names= argument to torch.tensor ctor

Test Plan

• new tests [namedtensor ci]

Differential Revision: D17120399

[nairbv]

seems like this code should be refactored a bit.

E.g. as far as I can tell the body of the two USE_NUMPY conditions are nearly identical, so if you refactor you'd need fewer #ifdef BUILD_NAMEDTENSOR blocks, which would make the code more readable. Ideally you'd only need one. I think with better code re-use this code would be more readable regardless of your change.

[zou3519]

If I refactored this into a function, that function would require a guard on one of its arguments based on #ifdef BUILD_NAMEDTENSOR because it would take in a names argument.

We'd still have the same number of #ifdef BUILD_NAMEDTENSOR blocks (actually we'd have ~2 more because of the function declaration and definition), so this would not clean up the code.

I agree that this code could use a refactor, however. I can file a task and take care of it when I delete the build_namedtensor flag (that is slated to happen sometime hopefully next week).

@nairbv Did you have any other concerns on the code?

[nairbv]

I don't (necessarily) mean separate functions.

e.g. if we just wrote if (PyObject_HasAttrString(data, "__cuda_array_interface__") || PyArray_Check(data)) above instead of having two separate conditions, the only thing that differs inside the if block is the call to get the tensor. The repeated code doesn't necessarily need to be moved to a helper function to be combined.

I guess this can be refactored some other time though.

[zou3519]

e.g. if we just wrote if (PyObject_HasAttrString(data, "cuda_array_interface") || PyArray_Check(data)) above instead of having two separate conditions, the only thing that differs inside the if block is the call to get the tensor. The repeated code doesn't necessarily need to be moved to a helper function to be combined.

I agree that it would be marginally cleaner, but not to the extent that we should block this PR on. A full refactor of the code that would make the logic in this function a lot clearer, in my opinion, would split up each of the codepaths into distinct cases (in the form of helper functions): build_tensor_from_cuda_array, build_tensor_from_numpy, build_tensor_from_pylist, build_tensor_from_tensor. build_tensor_from_cuda_array would share a lot of common code with build_tensor_from_numpy, and all four of these functions would share a lot of common code in the place where we coerce the input to a device/dtype/names. However, if I were to do this right now, each function would be littered with #ifdef BUILD_NAMEDTENSOR blocks.

[nairbv]

is it intentional that you test names for this propagate call but not for the others?

[zou3519]

Yes. This is case where we're passing a tensor to the torch.tensor constructor. It should behave differently if we pass in names=None (those are ignored) as opposed to names=<something>.

For example,

x: Tensor[N, C]
(1) torch.tensor(x): Tensor[N, C]. names=None implicitly.
(2) torch.tensor(x, names=None): Tensor[N, C]
torch.tensor(x, names=[N, D]): Error!

This is because we can't distinguish between cases (1) and (2).

[nairbv]

I can imagine that if we have a x=tensor([1,2,3], names=['foo']) and we try to call y=torch.tensor(x), we would want to make sure names is preserved in y. Is that what you're trying to handle here?

What's the case that should error? I assume we should allow a user to change the names of a tensor in copy construction?

[nairbv]

I can see from the examples in tests that we don't allow changing names in construction. Should we / why don't we?

[zou3519]

We don’t allow it because there’s no use case for it and it is better to be conservative. If there is a use case for it, then there is a workaround and it is easy to add once someone asks for it.

[vadimkantorov]

A naming question: if a general tensor.name attribute is to be introduced at a later point (e.g. for deubgging or graph inspection purposes), it would be confusing to have both name and names. A suggestion: names -> dim_names = 'NCHW'

[zou3519]

A naming question: if a general tensor.name attribute is to be introduced at a later point (e.g. for deubgging or graph inspection purposes), it would be confusing to have both name and names.

I agree.

A suggestion: names -> dim_names = 'NCHW'

Thank you for your suggestion, we'll keep it in mind. I've thought about names, dims, dim_names as potential attributes. For now, because there isn't a widely used tensor.name (it actually does exist and it is used for ONNX), I'd like to keep the named tensor API nice and simple with names.

[facebook-github-bot]

@zou3519 merged this pull request in 4231287.