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.

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

Add CI/CD for Aarch64 pip wheels using GitHub Actions #56025

Closed
wants to merge 8 commits into from
Closed

Add CI/CD for Aarch64 pip wheels using GitHub Actions #56025

wants to merge 8 commits into from

Conversation

mseth10
Copy link
Contributor

@mseth10 mseth10 commented May 9, 2022 •
edited

Added GitHub workflow to build, test and publish Aarch64 pip wheels for Python versions 3.7, 3.8, 3.9 and 3.10
Workflow jobs run on a self-hosted runner on an AWS Graviton instance and triggers for:

  • Pull requests against r2.9 branch
  • New commits to r2.9 branch
  • New tags starting with v2.

Following tests currently fail and are skipped:

  1. tensorflow/python:nn_grad_test_cpu
  2. tensorflow/python/eager:forwardprop_test_cpu
  3. tensorflow/python/framework:node_file_writer_test_cpu
  4. tensorflow/python/grappler:memory_optimizer_test
  5. tensorflow/python/keras/engine:training_arrays_test
  6. tensorflow/python/kernel_tests/linalg:linear_operator_householder_test_cpu
  7. tensorflow/python/kernel_tests/linalg:linear_operator_inversion_test_cpu
  8. tensorflow/python/kernel_tests/linalg:linear_operator_block_diag_test_cpu
  9. tensorflow/python/kernel_tests/linalg:linear_operator_block_lower_triangular_test_cpu
  10. tensorflow/python/kernel_tests/linalg:linear_operator_kronecker_test_cpu
  11. tensorflow/python/kernel_tests/math_ops:batch_matmul_op_test_cpu
  12. tensorflow/python/kernel_tests/nn_ops:conv_ops_test_cpu
  13. tensorflow/python/kernel_tests/nn_ops:conv2d_backprop_filter_grad_test_cpu
  14. tensorflow/python/kernel_tests/nn_ops:conv3d_backprop_filter_v2_grad_test_cpu
  15. tensorflow/python/kernel_tests/nn_ops:atrous_conv2d_test_cpu
  16. tensorflow/python/ops/parallel_for:math_test_cpu

@mseth10 mseth10 requested a review from rohan100jain as a code owner May 9, 2022 12:09
@google-cla
Copy link

google-cla bot commented May 9, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation May 9, 2022
mihaimaruseac
mihaimaruseac previously approved these changes May 9, 2022
View reviewed changes
Copy link
Collaborator

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

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

Thank you very much

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer May 9, 2022
@mihaimaruseac
Copy link
Collaborator

Can you sign CLA please?

@mihaimaruseac
Copy link
Collaborator

Also, I think this should be opened on master?

@elfringham
Copy link
Contributor

I think the use of no_aarch64 tag to exclude tests that fail when TF_ENABLE_ONEDNN_OPT=1 is not the best thing to do. The flag is currently used to exclude tests that can never work on AARCH64. This would add a group of tests that we hope are only temporarily broken and so would have their no_aarch64 tag removed at some later point. It also stops those tests being run at all on AARCH64 even for the Eigen build, where they currently pass. I think some other mechanism would be better.

elfringham
elfringham suggested changes May 10, 2022
View reviewed changes
Copy link
Contributor

@elfringham elfringham left a comment

Choose a reason for hiding this comment

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

This is not the right way to temporarily exclude these tests. It hides the exclusion away in multiple places and also prevents these tests from being run for the default Eigen build where they are passing.

@mseth10
Copy link
Contributor Author

mseth10 commented May 11, 2022

This is not the right way to temporarily exclude these tests. It hides the exclusion away in multiple places and also prevents these tests from being run for the default Eigen build where they are passing.

Hi @elfringham , I agree with your concern. We can introduce a new tag like no_aarch64_onednn_acl and use it to exclude these tests. Do you think that's an acceptable solution?

@nSircombe
Copy link
Contributor

Could they not simply be listed explicitly in TF_TEST_TARGETS with the other test exclusions?

elfringham
elfringham reviewed May 11, 2022
View reviewed changes
tensorflow/tools/ci_build/rel/ubuntu/cpu_arm64_pip.sh Outdated
# Export optional variables for running pip.sh
export TF_BUILD_FLAGS="--config=mkl_aarch64 ${extra_args}"
export TF_TEST_FLAGS="--config=mkl_aarch64 ${extra_args} --test_env=TF_ENABLE_ONEDNN_OPTS=1 --test_env=TF2_BEHAVIOR=1 --define=no_tensorflow_py_deps=true --test_lang_filters=py --verbose_failures=true --test_keep_going"
export TF_TEST_TARGETS="${DEFAULT_BAZEL_TARGETS} -//tensorflow/lite/... -//tensorflow/compiler/mlir/lite/tests:const-fold.mlir.test -//tensorflow/compiler/mlir/lite/tests:prepare-tf.mlir.test"
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @nSircombe , this would be the place to put exclusions for tests that fail. Using tags is not the best solution as it hides that information in multiple places which makes it harder to check the status and make updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @nSircombe , this would be the place to put exclusions for tests that fail. Using tags is not the best solution as it hides that information in multiple places which makes it harder to check the status and make updates.

Thanks for the suggestion @nSircombe @elfringham . Addressed.

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes May 12, 2022
@mseth10
Copy link
Contributor Author

mseth10 commented May 12, 2022

Hi @mihaimaruseac , I have signed the Google Individual CLA.

Also, I think this should be opened on master?

I have only tested this change on r2.9 branch as of now. I was thinking after the release, we can cherry-pick it into master.
Are you suggesting we do it the other way around - create this PR on master branch and when it gets merged, cherry-pick into r2.9 branch?

@mihaimaruseac
Copy link
Collaborator

Yes please. We never cherrypick from release branch back into master.

@mseth10
Copy link
Contributor Author

mseth10 commented May 13, 2022

Yes please. We never cherrypick from release branch back into master.

@mihaimaruseac I have opened another PR on master branch #56097 . Will close this PR and will cherry-pick into r2.9 later.

@mseth10 mseth10 closed this May 13, 2022
PR Queue automation moved this from Reviewer Requested Changes to Closed/Rejected May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elfringham elfringham elfringham requested changes

@mihaimaruseac mihaimaruseac mihaimaruseac left review comments

@rohan100jain rohan100jain Awaiting requested review from rohan100jain

Assignees

@mihaimaruseac mihaimaruseac

Labels
None yet
Projects
PR Queue
  
Closed/Rejected
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mseth10 @mihaimaruseac @elfringham @nSircombe