Skip to content

Add convolution unit test for Arm backend #2427

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
Closed
wants to merge 2 commits into from
Closed

Add convolution unit test for Arm backend #2427

wants to merge 2 commits into from

Conversation

freddan80
Copy link
Collaborator

  • Added both depthwise and regular conv
  • Removed corresponding conv legacy unit tests
  • Fix zero point usage solves issues with random input
  • Removed add/add2 legacy unit tests as they are already implemented
  • Bump serialization lib submodule to avoid warning spam
  • Fixed rounding issue tosa_test_utils

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Mar 14, 2024 •
edited
Loading

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit 3168421 with merge base 1c2ed7b (image):

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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 Mar 14, 2024
@freddan80 freddan80 added partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm ciflow/trunk labels Mar 14, 2024
@freddan80
Copy link
Collaborator Author

@digantdesai Rebase conflict expected with PR #2371 , so hold off merging this PR until that one has gone in. I'll rebase as soon as it's in.

@freddan80 freddan80 force-pushed the pr_conv_unittest branch 3 times, most recently from a566aa0 to add2cbb Compare March 20, 2024 21:29
@freddan80
Copy link
Collaborator Author

@digantdesai CI failure not related to this change.

digantdesai
digantdesai approved these changes Mar 21, 2024
View reviewed changes
backends/arm/test/ops/test_add.py
logger = logging.getLogger(__name__)
logger.setLevel(logging.INFO)

torch.manual_seed(42)
Copy link
Contributor

Choose a reason for hiding this comment

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

this too?

Copy link
Collaborator Author

@freddan80 freddan80 Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

torch.randn is used in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it is ok to run test each time with a new random value? You can set the seed for debugging locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see your point. Let us address that in coming PRs.

backends/arm/test/ops/test_conv.py
permute_memory_to_nhwc=True,
)
.export()
.to_edge()
Copy link
Contributor

Choose a reason for hiding this comment

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

check for conv nodes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dito

@freddan80
Add convolution unit test for Arm backend
6ed5abd 
* Added both depthwise and regular conv
* Removed corresponding conv legacy unit tests
* Fix zero point usage solves issues with random input
* Removed add/add2 legacy unit tests as they are already implemented
* Bump serialization lib submodule to avoid warning spam
* Fixed rounding issue tosa_test_utils
* Fixed and moved input data transpose

Change-Id: I5c9a1f3571b1fa2b44ff4f1a04dfed54fea841db
Signed-off-by: Fredrik Knutsson <[email protected]>
@freddan80
Addressing review comments
3168421 
Change-Id: Ic0fbbabfe840b5c18788589068b6c763e43674b9
@freddan80
Copy link
Collaborator Author

CI fail unrelated to this PR. Credential issues:

ExecuTorch: Failed to run runner.
ERROR conda.cli.main_run:execute(47): `conda run bash backends/apple/coreml/scripts/build_all.sh` failed. (See above for error)
    main()
  File "/Users/runner/work/executorch/executorch/test-infra/.github/scripts/run_with_env_secrets.py", line 60, in main
    run_cmd_or_die(f"bash { os.environ.get('RUNNER_TEMP', '') }/exec_script")
  File "/Users/runner/work/executorch/executorch/test-infra/.github/scripts/run_with_env_secrets.py", line 38, in run_cmd_or_die
    raise RuntimeError(f"Command {cmd} failed with exit code {exit_code}")
RuntimeError: Command bash /Users/runner/work/_temp/exec_script failed with exit code 1
Error: Process completed with exit code 1.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@digantdesai merged this pull request in 001cc5f.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@digantdesai digantdesai digantdesai approved these changes

@Jerry-Ge Jerry-Ge Jerry-Ge left review comments

Assignees
No one assigned
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. Merged partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@freddan80 @facebook-github-bot @digantdesai @Jerry-Ge