For testing

The to/from logic of this file got moved to utils.h

This file got split into tensor-struct.h which has everything before the PR, and tensor-inl.h which implements scalar_type as it relies on from/to. The reason for the code split is to allow the code to build without circular dependencies. Without the split, tensor.h would depend on library.h (for to/from) and library.h would depend on tensor.h (cuz to/from Tensor needs a Tensor def).

Now, utils.h (which has to/from) depends on tensor-struct.h, tensor-inl.h depends on both utils.h and tensor-struct.h, and users depend on tensor.h still, which depends on all of the above.

NOTE!! THIS IS WHERE I WANT REVIEW!! cc@albanD

Prior, if we had an IValue dtype that was qint8, from_ivalue would callfrom(ScalarType::Qint8), and the code would just reinterpret the enum and spit out the int32_t correspondingly. This was okay because ScalarType wasn't exposed to the end user, and all they had to work with was an abstracted int32_t that they would get from the C shim.

However, with this change today,from(ScalarType::Qint8) would error!!!! Because now, ScalarType is allowed to be used by the end user, and they can call this function, and naively reinterpreting the enum is no longer ok if the extension's ScalarType is different from libtorch's ScalarType! I think erroring is acceptable because these other types are infrequently used by people anyway, but maybe I am wrong about that. e.g.,@swolchok are the Bits ScalarTypes used in ET?

Answering your specific question:

I haven't (yet?) made any attempt to use PyTorch's ScalarType in ExecuTorch. ExecuTorch hashttps://github.com/pytorch/executorch/blob/52b45e2d2ac244b13a36ddf5d21a9ebe8d8aa17e/runtime/core/portable_type/scalar_type.h#L132

PyTorch's ScalarType will get used in ExecuTorch's ATen mode, though.https://github.com/pytorch/executorch/blob/52b45e2d2ac244b13a36ddf5d21a9ebe8d8aa17e/runtime/core/exec_aten/exec_aten.h#L82

I don't know what the Bits ScalarTypes even are, but ExecuTorch seems to have its own versions of them that it uses:https://github.com/search?q=repo%3Apytorch%2Fexecutorch+ScalarType%3A%3ABits+language%3AC%2B%2B&type=code&l=C%2B%2B

In general: it is not backward compatible to change functionality such that a call that previously succeeded (and really did work fine) is now an error.

I've concluded it is okay to BC break here given that GitHub search yields 0 users for the narrow use case for which this code would break. Updated the PR body consequently.

this code is a copy pasta EXCEPT for the specializations fortorch::headeronly::ScalarType

I firmly dislike naming things "utils" because it is synonymous with "stuff" and helps neither predict their current contents nor limit their future contents. Instead I would consider a specific name, like say StableIValueConversions.h .

renamed!!!!!! stableivalue_conversions.h

Answering your specific question:

I haven't (yet?) made any attempt to use PyTorch's ScalarType in ExecuTorch. ExecuTorch hashttps://github.com/pytorch/executorch/blob/52b45e2d2ac244b13a36ddf5d21a9ebe8d8aa17e/runtime/core/portable_type/scalar_type.h#L132

PyTorch's ScalarType will get used in ExecuTorch's ATen mode, though.https://github.com/pytorch/executorch/blob/52b45e2d2ac244b13a36ddf5d21a9ebe8d8aa17e/runtime/core/exec_aten/exec_aten.h#L82

I don't know what the Bits ScalarTypes even are, but ExecuTorch seems to have its own versions of them that it uses:https://github.com/search?q=repo%3Apytorch%2Fexecutorch+ScalarType%3A%3ABits+language%3AC%2B%2B&type=code&l=C%2B%2B

In general: it is not backward compatible to change functionality such that a call that previously succeeded (and really did work fine) is now an error.

This feels like it can benefit from define of sorts, that iterates over known dtypes (if dtypes are part of stable API...)

I was trying to figure out a way, but it did not seem worth it for this PR. (Also there's no "is dtype part of stable API" function we can call yet).

This feels very suspicious.. Why not pass the value as unique_ptr?

I'm not sure about passing it around as std::unique_ptr, but you could certainly declarestd::unique_ptr<StableIValue> sivp = to<StableIValue*>(val); above and insulate yourself against the innerto<T> throwing an exception.

will address in a followup

nit: is "stableivalue_conversions.h" really the conventional formatting here? I would've expected "StableIValueConversions.h", as with torch/headeronly/core/ScalarType.h

I think in general it would be good to document the header format, as I've seen bothCamelCase.h,snake_case.h andsomething-weird.h (for exampletensor-inl.h in this PR)

For example, aoti follows snake_case

idk the conventional format, but I was going under the guise of using Caps when there was a struct definition of the same name, and using lowercase for more thematic naming (like everything in this file relates to ____). And then the -dashes I'm copying from in c10/ Half and Half-inl.h where the dash means it's a continuation of a file, that these files would be together if possible but are broken apart for some other reason.

I'm happy to follow an existing header notation though, if there is one

I'm not sure about passing it around as std::unique_ptr, but you could certainly declarestd::unique_ptr<StableIValue> sivp = to<StableIValue*>(val); above and insulate yourself against the innerto<T> throwing an exception.

I think in general it would be good to document the header format, as I've seen bothCamelCase.h,snake_case.h andsomething-weird.h (for exampletensor-inl.h in this PR)

For example, aoti follows snake_case

Q: Why not add unisgned types here?

I was following the convention of the actual enum order

Is it possible to cast it to some enum and use switch statement? (As it will force devs to add options there when new dtype is added) This statement will be result of never ending series of "Added missing XYZ" here

Well the whole reason to have this is because the user binary ScalarType baked in isn't necessarily the same enum as the libtorch binary ScalarType, which is why I'm passing ints through the shim.

Why do you support unisgned dtypes here but not in the previous switch statement?

They're supported above too

See my above comment. Use either CamelCase or snake_case

okay, ill switch to snake case for these

Please please use macros to get rid of copy_pasta

It is more readable currently to see these APIs as is. If we need to add more, I will consider using a preprocessor macro.