Skip to content
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

[Core ML] Implement intermediate tensor logging #4557

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

cymbalrush
Copy link
Collaborator

@cymbalrush cymbalrush commented Aug 6, 2024

  • Updated ModelEventLoggerImpl to log intermediate tensor values.
  • Updated sdk_example_runner to build Core ML when specified.
  • Added debugger_cli script to compare intermediate tensor values of a program executed using optimized kernels and delegated to Core ML .

Testing:

  • Ran python examples/apple/coreml/scripts/debugger_cli.py -m softmax to compare intermediate tensors
Screenshot 2024-08-09 at 2 31 27 PM

Copy link

pytorch-bot bot commented Aug 6, 2024

🔗 Helpful Links

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

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

❌ 1 New Failure, 1 Cancelled Job

As of commit eca7a38 with merge base 2718dd4 (image):

NEW FAILURE - The following job has failed:

CANCELLED JOB - The following job was cancelled. Please retry:

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 Aug 6, 2024
@cymbalrush cymbalrush force-pushed the log_intermediate_tensors branch 2 times, most recently from 80276dc to 4728a65 Compare August 6, 2024 15:23
@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@Olivia-liu Olivia-liu left a comment

Choose a reason for hiding this comment

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

Hi, it looks good! Thanks for working on the integration. Just one line in the cmake file I need clarification on if you don't mind

examples/sdk/build_sdk_example_runner.sh Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

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

@Olivia-liu
Copy link
Contributor

Thanks for addressing the comment! I just imported again. By the way, next time when the PR is ready for review again, can you click the Re-request review button, please?

@Olivia-liu Olivia-liu self-requested a review August 14, 2024 22:11
@Olivia-liu
Copy link
Contributor

Olivia-liu commented Aug 14, 2024

There are actually OSS CI failures. Can you rebase on main and commit again, please? I see the OSS CI tests are also broken on main, so I'm landing this now from my side. Hope it goes through.

Copy link
Contributor

@Olivia-liu Olivia-liu left a comment

Choose a reason for hiding this comment

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

Please try rebasing to resolve the 2 OSS CI failures.

@facebook-github-bot facebook-github-bot merged commit a9ed835 into pytorch:main Aug 15, 2024
88 of 93 checks passed
@cccclai
Copy link
Contributor

cccclai commented Aug 15, 2024

Hi @cymbalrush, sorry we're reverting this pr because it breaks the CI. Do you mind sending out the PR again and address the IC? Thanks!

kirklandsign pushed a commit to kirklandsign/executorch that referenced this pull request Aug 15, 2024
Differential Revision: D60838372

Pull Request resolved: pytorch#4557
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants