Skip to content

Conversation

cyyever
Copy link
Collaborator

@cyyever cyyever commented Jul 18, 2025

Reland of #157050, which is incidentally closed.

@cyyever cyyever requested review from albanD and soulitzer as code owners July 18, 2025 04:20
Copy link

pytorch-bot bot commented Jul 18, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/158625

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 782add0 with merge base 1274297 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from here,
cc @jeanschmidt who reverted the original PR in case you have more information on what was the concern.

@cyyever
Copy link
Collaborator Author

cyyever commented Jul 19, 2025

@albanD I have no permission to access the report so I can only guess some inner code explicitly uses c10::string_view, but should be easy to fix.

@jeanschmidt
Copy link
Contributor

I double-checked, it seems it was raising from executorch tests the error:

    raise RuntimeError(f"Unsupported cpp type {ref.src_cpp_type}")
RuntimeError: Unsupported cpp type ::std::string_view

So I suspect that we might need to coordinate the executorch changes to land internally in sync with those (if there are needed changes from executorch side). I don't have the context to tell if there is need to update executorch or this can be done with retro-compatibility, in the 2nd case I believe we're good to go. Otherwise maybe you will want to codev those changes internally.

Current oncall is @ZainRizvi

@cyyever
Copy link
Collaborator Author

cyyever commented Jul 22, 2025

Thank you,now there is some clue to continue. I can submit related changes there.

@cyyever
Copy link
Collaborator Author

cyyever commented Jul 22, 2025

@jeanschmidt executorch compares with std::string_view, our change here uses ::std::string_view to avoid conflicting with the std operation. Let me check whether std::string_view can work.

@cyyever cyyever force-pushed the svtorchgen2 branch 5 times, most recently from d524569 to 2bf52e5 Compare July 22, 2025 15:45
@albanD albanD added the executorch-needs-help Add this label to your issue/PR if you need help from the ExecuTorch team label Jul 22, 2025
@swolchok
Copy link
Contributor

why not just patch the executorch check so that "::std::string_view" is also acceptable?

elif ref.src_cpp_type == STRING or ref.src_cpp_type == "::" + STRING or ref.src_cpp_type == OLD_STRING:

@cyyever
Copy link
Collaborator Author

cyyever commented Jul 23, 2025

why not just patch the executorch check so that "::std::string_view" is also acceptable?

elif ref.src_cpp_type == STRING or ref.src_cpp_type == "::" + STRING or ref.src_cpp_type == OLD_STRING:

That solution of course works, but if STRING is used in other places we have to repeat the patch. I have proposed a more thorough solution, see
pytorch/executorch#12706.

Signed-off-by: Yuanyuan Chen <[email protected]>
Signed-off-by: Yuanyuan Chen <[email protected]>
Signed-off-by: Yuanyuan Chen <[email protected]>
@cyyever
Copy link
Collaborator Author

cyyever commented Sep 12, 2025

The latest changes don't break executorch and make CI green.