Skip to content

[#9971] Gracefully error out in ETDump part 3 for *profiling_delegate #10147

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
Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

[#9971] Gracefully error out in ETDump part 3 for *profiling_delegate #10147

wants to merge 8 commits into from

Conversation

@zhongmingyuan
Copy link
Contributor

@zhongmingyuan zhongmingyuan commented Apr 14, 2025 •
edited
Loading

Summary

Gracefully throw error for

start_profiling_delegate
end_profiling_delegate
end_profiling_delegate

  1. Changed the method signature to Result<>
  2. Use ET_CHECK_OR_RETURN_ERROR
    next: check_ready_to_add_event will need to come in later PRs

Test plan

test/run_oss_cpp_tests.sh

Not sure how to trigger CI in draft mode

@Gasoonjia @zhenyan-zhang-meta

@pytorch-bot
Copy link

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

🔗 Helpful Links

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

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

✅ No Failures

As of commit a47a195 with merge base e84c319 (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 14, 2025
@zhongmingyuan
Copy link
Contributor Author

zhongmingyuan commented Apr 14, 2025

@pytorchbot label "topic: not user facing"

@zhongmingyuan zhongmingyuan changed the title [#9971] Gracefully error out in ETDump for *profiling_delegate [#9971] Gracefully error out in ETDump part 3 for *profiling_delegate Apr 14, 2025
@zhongmingyuan zhongmingyuan marked this pull request as ready for review April 14, 2025 19:54
Gasoonjia
Gasoonjia reviewed Apr 14, 2025
View reviewed changes
Copy link
Contributor

@Gasoonjia Gasoonjia left a comment

Choose a reason for hiding this comment

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

Thanks @zhongmingyuan for working on this! Expecially love to see you updating our doc.
Overall looks good and left some comments.
The main thing is remaining the functions interface in runtime/core/event_tracer_hooks_delegate.h as they are right now.

@Gasoonjia
Copy link
Contributor

Gasoonjia commented Apr 14, 2025

Also please solve the ci issue.

@zhenyan-zhang-meta
Copy link
Contributor

zhenyan-zhang-meta commented Apr 14, 2025

Not sure how to trigger CI in draft mode

It has to be from us to manually trigger it for you. I've just done it.

Could you also regularly merge the latest changes from main into this branch?

@Gasoonjia
Copy link
Contributor

Gasoonjia commented Apr 14, 2025

@zhenyan-zhang-meta I've merged this onto the latest main branch 15 minutes ago so it should be ok now.

@zhongmingyuan zhongmingyuan force-pushed the issue9971-profile branch 2 times, most recently from 53b7092 to ca14066 Compare April 15, 2025 17:26
@zhongmingyuan
Copy link
Contributor Author

zhongmingyuan commented Apr 15, 2025

Also please solve the ci issue.

Most of errors are related to huggingface. I guess they are temporary? Please feel free to suggest how to reproduce it locally here. Thanks

OSError: We couldn't connect to 'https://huggingface.co' to load this file, couldn't find it in the cached files and it looks like google/mobilebert-uncased is not the path to a directory containing a file named config.json.

Gasoonjia
Gasoonjia approved these changes Apr 15, 2025
View reviewed changes
Copy link
Contributor

@Gasoonjia Gasoonjia left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for update!

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 15, 2025

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

@Gasoonjia
Copy link
Contributor

Gasoonjia commented Apr 16, 2025

Please let me merge it -- there're some internal issue i need to fix

@zhongmingyuan
Copy link
Contributor Author

zhongmingyuan commented Apr 18, 2025

Please let me merge it -- there're some internal issue i need to fix

@Gasoonjia Thanks!. Do you need me to close this PR and wait for you?

@zhongmingyuan
Copy link
Contributor Author

zhongmingyuan commented May 16, 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 16, 2025
@larryliu0820
Copy link
Contributor

larryliu0820 commented May 28, 2025

@Gasoonjia please help merge this

@github-actions
Copy link

github-actions bot commented Aug 31, 2025

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the stale PRs inactive for over 60 days label Aug 31, 2025
@nil-is-all nil-is-all removed the stale PRs inactive for over 60 days label Sep 3, 2025
@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the stale PRs inactive for over 60 days label Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Gasoonjia Gasoonjia Gasoonjia approved these changes

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

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

@swolchok swolchok Awaiting requested review from swolchok

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

@tarun292 tarun292 Awaiting requested review from tarun292

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 stale PRs inactive for over 60 days topic: not user facing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@zhongmingyuan @Gasoonjia @zhenyan-zhang-meta @facebook-github-bot @larryliu0820 @nil-is-all