Skip to content

Add device type (public, private) and id #10496

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

Merged
merged 7 commits into from
Apr 29, 2025

Conversation

And also enable Apple benchmark there. This depends on pytorch/test-infra#6579

Signed-off-by: Huy Do <[email protected]>
Copy link

pytorch-bot bot commented Apr 25, 2025

🔗 Helpful Links

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

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

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 25, 2025
@huydhn huydhn temporarily deployed to upload-benchmark-results April 25, 2025 23:39 — with GitHub Actions Inactive
@huydhn huydhn temporarily deployed to upload-benchmark-results April 25, 2025 23:39 — with GitHub Actions Inactive
huydhn added 2 commits April 25, 2025 17:42
Signed-off-by: Huy Do <[email protected]>
@huydhn huydhn had a problem deploying to upload-benchmark-results April 26, 2025 01:32 — with GitHub Actions Failure
@huydhn huydhn had a problem deploying to upload-benchmark-results April 26, 2025 02:44 — with GitHub Actions Failure
Signed-off-by: Huy Do <[email protected]>
@huydhn huydhn changed the title Add a notation for private device Add device type (public, private) and id Apr 26, 2025
@huydhn huydhn temporarily deployed to upload-benchmark-results April 26, 2025 04:39 — with GitHub Actions Inactive
@huydhn huydhn temporarily deployed to upload-benchmark-results April 26, 2025 05:15 — with GitHub Actions Inactive
@huydhn huydhn requested review from guangy10 and yangw-dev April 26, 2025 07:14
@huydhn huydhn marked this pull request as ready for review April 26, 2025 07:15
@zingo
Copy link
Collaborator

zingo commented Apr 28, 2025

Regarding
(Revert b415af0 for testing as it currently breaks building Apple benchmark app, I'll put it back before landin...

We discovered in some internal flow that we where missing --recursive on git submodule update in some of our scripts and got into problems with that PR, maybe it might help you to to add it if not there.

e.g. change
git submodule update --init
to
git submodule update --init --recursive

I do not know if helps you in your case but could be a thing to check.

@huydhn
Copy link
Contributor Author

huydhn commented Apr 28, 2025

We discovered in some inetrnal flow that we where missing --recursive on git submodule update in some of our script maybe it might help you to to add it if not there.

e.g. change git submodule update --init to git submodule update --init --recursive

I do not know if helps you in your case but could be something to check.

I see. It looks like there is already a PR for this #10472, but let me check if it misses some spots

@guangy10
Copy link
Contributor

We discovered in some inetrnal flow that we where missing --recursive on git submodule update in some of our script maybe it might help you to to add it if not there.
e.g. change git submodule update --init to git submodule update --init --recursive
I do not know if helps you in your case but could be something to check.

I see. It looks like there is already a PR for this #10472, but let me check if it misses some spots

Yes, --recursive is required otherwise execturoch runtime will complain about missing CMakeLists.txt in the third-party library.

@huydhn
Copy link
Contributor Author

huydhn commented Apr 28, 2025

I think the failed build-benchmark-app job already checkout the submodule recursively https://github.com/pytorch/executorch/blob/main/.github/workflows/apple.yml#L282, but it's still failing to build after the tokenizer bump https://github.com/pytorch/executorch/actions/runs/14672135868/job/41180874589

2025-04-25T20:07:18.5106110Z Undefined symbols for architecture arm64:
2025-04-25T20:07:18.5109390Z   "tokenizers::create_regex(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)", referenced from:
2025-04-25T20:07:18.5110360Z       tokenizers::(anonymous namespace)::_create_regex(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) in tiktoken.o

@guangy10
Copy link
Contributor

I think the failed build-benchmark-app job already checkout the submodule recursively https://github.com/pytorch/executorch/blob/main/.github/workflows/apple.yml#L282, but it's still failing to build after the tokenizer bump https://github.com/pytorch/executorch/actions/runs/14672135868/job/41180874589

2025-04-25T20:07:18.5106110Z Undefined symbols for architecture arm64:
2025-04-25T20:07:18.5109390Z   "tokenizers::create_regex(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)", referenced from:
2025-04-25T20:07:18.5110360Z       tokenizers::(anonymous namespace)::_create_regex(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) in tiktoken.o

If it fails on main, we should tag the folks in the team to take a look

Signed-off-by: Huy Do <[email protected]>
@huydhn huydhn merged commit 9a8fcba into main Apr 29, 2025
90 of 93 checks passed
@huydhn huydhn deleted the enable-apple-benchmark-private-device branch April 29, 2025 03:04
@huydhn huydhn temporarily deployed to upload-benchmark-results April 29, 2025 03:33 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants