Skip to content

ExecuTorch Scalar to() supports fewer types than c10::Scalar, breaking source compatibility #9500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
swolchok opened this issue Mar 21, 2025 · 3 comments · May be fixed by #10040
Open

ExecuTorch Scalar to() supports fewer types than c10::Scalar, breaking source compatibility #9500

swolchok opened this issue Mar 21, 2025 · 3 comments · May be fixed by #10040
Labels
good first issue Good for newcomers module: runtime Issues related to the core runtime and code under runtime/ triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@swolchok
Copy link
Contributor

swolchok commented Mar 21, 2025

🐛 Describe the bug

some_scalar.to<float>() is legal for c10::Scalar, but not ExecuTorch's Scalar. https://github.com/pytorch/executorch/blob/main/runtime/core/portable_type/scalar.h#L26 says it is a "source-compatible subset" of c10::Scalar.

Versions

main

cc @larryliu0820 @JacobSzwejbka

@JacobSzwejbka JacobSzwejbka added the good first issue Good for newcomers label Mar 24, 2025
@JacobSzwejbka JacobSzwejbka added the module: runtime Issues related to the core runtime and code under runtime/ label Mar 24, 2025
@github-project-automation github-project-automation bot moved this to To triage in ExecuTorch Core Mar 24, 2025
@JacobSzwejbka JacobSzwejbka added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Mar 24, 2025
@ahbpp
Copy link

ahbpp commented Mar 29, 2025

Hi @swolchok

I would be happy to work on this issue

Adding to<float>() looks like a quick fix.

I'm also happy to add support for other types, but I'm unsure which types should be supported.

It looks like c10::Scalar supports AT_FORALL_SCALAR_TYPES_WITH_COMPLEX + uint16/32/64_t (see: https://github.com/pytorch/pytorch/blob/29b3fdab0178d88a897ca48b85d1f7893d906ae0/c10/core/Scalar.h#L141), which includes various ints, floats and c10:complex types.

I was thinking about adding a similar implementation for exectutorch Scalar (but with executorch::runtime::etensor::complex instead of c10:complex). Does that sound like the right direction?

But I'm also happy to fix to<float>() issue first

@JacobSzwejbka
Copy link
Contributor

@ahbpp yes that sounds like the right direction. I'm happy to review any PRs you put up or provide further guidance if needed

@ahbpp
Copy link

ahbpp commented Apr 10, 2025

Hi @JacobSzwejbka

I created a PR implementing the to method for float, ::executorch::runtime::etensor::Half, and ::executorch::runtime::etensor::BFloat16, but I need advice on the best (expected) way to implement more complex logic:

  1. I'm thinking about whether it’s better to create a separate TypeCast.h for typecasting (similar to PyTorch c10). Would this be better than having the casting code in the Scalar class?
  2. For overflow logic, is it better to copy c10::overflows.h to related executorch/runtime/core/portable_type/c10 or create a separate overflow.h file somewhere in executorch/runtime/core/portable_type ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers module: runtime Issues related to the core runtime and code under runtime/ triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
Status: To triage
3 participants