Skip to content

Use std::string_view and std::optional #10541

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 11, 2025
Merged

Use std::string_view and std::optional #10541

merged 5 commits into from
Jun 11, 2025

Conversation

@cyyever
Copy link
Contributor

@cyyever cyyever commented Apr 29, 2025 •
edited
Loading

Summary

This PR moves executorch::aten::{string_view,optional} to std counterparts.

@pytorch-bot
Copy link

pytorch-bot bot commented Apr 29, 2025 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 02757cc with merge base c6c3616 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 29, 2025
@cyyever cyyever force-pushed the std_sv branch 2 times, most recently from 0208795 to a935a30 Compare April 29, 2025 04:10
@cyyever
Copy link
Contributor Author

cyyever commented Apr 29, 2025

@pytorchbot label "topic: not user facing"

@cyyever cyyever changed the title Use std::string_view and std::optional instead of PyTorch alias Use std::string_view and std::optional instead of c10 alias Apr 29, 2025
@cyyever cyyever changed the title Use std::string_view and std::optional instead of c10 alias Use std::string_view and std::optional Apr 29, 2025
@tarun292
Copy link
Contributor

tarun292 commented May 2, 2025

@swolchok any thoughts on this PR?

@GregoryComer
Copy link
Member

GregoryComer commented May 3, 2025

I had proposed this change previously, but @dbort brought up that the std types throw exceptions, which may be a problem for no-exception environments. Has there been more discussion on this point?

@cyyever
Copy link
Contributor Author

cyyever commented May 3, 2025

Currently executorch::aten::{string_view,optional} are aliases of std types, so they are not noexcept, they throw.

swolchok
swolchok suggested changes May 8, 2025
View reviewed changes
Copy link
Contributor

@swolchok swolchok left a comment

Choose a reason for hiding this comment

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

seems fine, but please put the submodule back and then ping me for accept

@cyyever
Copy link
Contributor Author

cyyever commented May 18, 2025

@swolchok Ping

@cyyever
Copy link
Contributor Author

cyyever commented May 18, 2025

@pytorchbot label "release notes: none"

@pytorch-bot pytorch-bot bot added the release notes: none Do not include this in the release notes label May 18, 2025
swolchok
swolchok approved these changes Jun 2, 2025
View reviewed changes
Copy link
Contributor

@swolchok swolchok left a comment

Choose a reason for hiding this comment

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

thanks!

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 2, 2025

@swolchok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@swolchok
Copy link
Contributor

swolchok commented Jun 3, 2025

this appears to have changed numerics for test-llava-runner-linux: https://github.com/pytorch/executorch/actions/runs/15386371381/job/43336641649?pr=10541

however, that test is known to segfault from time to time. rerunning it one more time to see if the output change persists.

@swolchok
Copy link
Contributor

swolchok commented Jun 3, 2025

@larryliu0820 are you the llava PoC? this diff appears to change numerics slightly but only for the llava test; should that be blocking?

@swolchok
Copy link
Contributor

swolchok commented Jun 3, 2025

hm, this really shouldn't change numerics; I'm pretty sure that torch::executor::optional and torch::executor::string_view are the std equivalents in all cases. we can verify that by sending an interim PR that forces the exec_aten types to be typedefs for the std types in all cases.

@swolchok swolchok mentioned this pull request Jun 3, 2025
Merged
swolchok added a commit that referenced this pull request Jun 3, 2025
@swolchok
Make exec_aten::optional and string_view direct aliases for std versions
35f7c55 
I'm seeing unexpected numerics changes on #10541. Doing this as an incremental step to make sure that the actual change to optional/string_view is not causing them.


ghstack-source-id: f8219a7
ghstack-comment-id: 2936633948
Pull-Request-resolved: #11321
swolchok added a commit that referenced this pull request Jun 4, 2025
@swolchok
Make exec_aten::optional and string_view direct aliases for std versi…
911fb0b 
…ons (#11321)

I'm seeing unexpected numerics changes on #10541. Doing this as an
incremental step to make sure that the actual change to
optional/string_view is not causing them.
@swolchok
Copy link
Contributor

swolchok commented Jun 11, 2025

OK, we landed #11321 , rebasing to see what CI has to say about that. (sorry for the delay; I was out for a couple days and am catching back up.)

@cyyever
Copy link
Contributor Author

cyyever commented Jun 11, 2025

@swolchok The current failures are unrelated

@swolchok
Copy link
Contributor

swolchok commented Jun 11, 2025

@swolchok The current failures are unrelated

I agree; I'll rerun them once test-llava-runner-linux (very slow, part of the same workflow) is done

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 11, 2025

@swolchok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@swolchok
Copy link
Contributor

swolchok commented Jun 11, 2025

nothing alarming showing up internally! merging.

@swolchok swolchok merged commit d719e8e into pytorch:main Jun 11, 2025
167 of 171 checks passed
@cyyever cyyever deleted the std_sv branch June 13, 2025 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JacobSzwejbka JacobSzwejbka Awaiting requested review from JacobSzwejbka JacobSzwejbka is a code owner

@lucylq lucylq Awaiting requested review from lucylq lucylq is a code owner

@manuelcandales manuelcandales Awaiting requested review from manuelcandales manuelcandales is a code owner

@jackzhxng jackzhxng Awaiting requested review from jackzhxng jackzhxng is a code owner

@iseeyuan iseeyuan Awaiting requested review from iseeyuan

@larryliu0820 larryliu0820 Awaiting requested review from larryliu0820 larryliu0820 is a code owner

@kimishpatel kimishpatel Awaiting requested review from kimishpatel kimishpatel is a code owner

@GregoryComer GregoryComer Awaiting requested review from GregoryComer GregoryComer is a code owner

@tarun292 tarun292 Awaiting requested review from tarun292

@Gasoonjia Gasoonjia Awaiting requested review from Gasoonjia Gasoonjia is a code owner

1 more reviewer

@swolchok swolchok swolchok approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: none Do not include this in the release notes topic: not user facing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cyyever @tarun292 @GregoryComer @facebook-github-bot @swolchok