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 workflow to run android tests on mobile phones #13024

Merged
merged 15 commits into from
Apr 19, 2023
Merged

Add CI workflow to run android tests on mobile phones #13024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e21da5b
Add android test workflow
Apr 11, 2023
92fd782
Fix write-caches type
Apr 11, 2023
e8de8d5
Use gcloud cli
Apr 11, 2023
a69687f
Fix is-pr type
Apr 11, 2023
0c6d22b
Use matrix
Apr 12, 2023
845e7a8
Fix variable name
Apr 12, 2023
3661aac
Don't pack *.o and *.a
Apr 13, 2023
c795166
Add job to summary
Apr 13, 2023
4576004
Cross-compile all then test on real devices
Apr 18, 2023
1d199a2
Test pr check
Apr 19, 2023
33f0865
Revert "Test pr check"
Apr 19, 2023
41b2fc7
Revert "Cross-compile all then test on real devices"
Apr 19, 2023
eb27ee3
Better matrix job name
Apr 19, 2023
759d30e
Add comment to explain the separate test workflow
Apr 19, 2023
9787e96
Enable postsubmit guard
Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 144 additions & 0 deletions .github/workflows/build_and_test_android.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
# Copyright 2023 The IREE Authors
#
# Licensed under the Apache License v2.0 with LLVM Exceptions.
# See https://llvm.org/LICENSE.txt for license information.
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
#
# Workflow for android cross-compilation and tests jobs. It is designed to be
# called from the main workflow ci.yml. The concurrency of this workflow is
# controlled by the caller's job.

name: Build and test android

on:
workflow_call:
inputs:
runner-group:
required: true
type: string
runner-env:
required: true
type: string
write-caches:
required: true
type: string
is-pr:
required: true
type: boolean
build-dir:
required: true
type: string
build-dir-archive:
required: true
type: string
build-dir-gcs-artifact:
required: true
type: string

env:
# This duplicates the variable from ci.yml. The variable needs to be in env
# instead of the outputs of setup because it contains the run attempt and we
# want that to be the current attempt, not whatever attempt the setup step
# last ran in. It therefore can't be passed in via inputs because the env
# context isn't available there.
GCS_DIR: gs://iree-github-actions-${{ github.event_name == 'pull_request' && 'presubmit' || 'postsubmit' }}-artifacts/${{ github.run_id }}/${{ github.run_attempt }}

jobs:
cross_compile:
runs-on:
- self-hosted # must come first
- runner-group=${{ inputs.runner-group }}
- environment=${{ inputs.runner-env }}
- cpu
- os-family=Linux
env:
HOST_BUILD_DIR: ${{ inputs.build-dir }}
HOST_BUILD_DIR_ARCHIVE: ${{ inputs.build-dir-archive }}
HOST_BUILD_DIR_GCS_ARTIFACT: ${{ inputs.build-dir-gcs-artifact }}
IREE_WRITE_REMOTE_CCACHE: ${{ inputs.write-caches }}
outputs:
target-build-dir: ${{ steps.build.outputs.target-build-dir }}
target-build-dir-archive: ${{ steps.archive.outputs.target-build-dir-archive }}
target-build-dir-gcs-artifact: ${{ steps.upload.outputs.target-build-dir-gcs-artifact }}
steps:
- name: "Checking out repository"
uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0
- name: "Checking out runtime submodules"
run: ./build_tools/scripts/git/update_runtime_submodules.sh
- name: "Downloading build dir archive"
run: gcloud storage cp "${HOST_BUILD_DIR_GCS_ARTIFACT}" "${HOST_BUILD_DIR_ARCHIVE}"
- name: "Extracting build dir archive"
run: tar -xf "${HOST_BUILD_DIR_ARCHIVE}" "${HOST_BUILD_DIR}/install"
- name: "Build cross-compiling target"
id: build
env:
TARGET_BUILD_DIR: build-android-arm_64
run: |
./build_tools/github_actions/docker_run.sh \
--env "IREE_CCACHE_GCP_TOKEN=$(gcloud auth application-default print-access-token)" \
--env "IREE_WRITE_REMOTE_CCACHE=${IREE_WRITE_REMOTE_CCACHE}" \
--env "CCACHE_NAMESPACE=${DOCKER_IMAGE}" \
--env "IREE_TARGET_BUILD_DIR=${TARGET_BUILD_DIR}" \
--env "BUILD_PRESET=test" \
--env "IREE_HOST_BIN_DIR=${HOST_BUILD_DIR}/install/bin" \
gcr.io/iree-oss/android@sha256:3f641d25786b1e5e430ee4cacb8bfe57540fda5ecaa7ca2802c179c26e77ce09 \
build_tools/cmake/build_android.sh
echo "target-build-dir=${TARGET_BUILD_DIR}" >> "${GITHUB_OUTPUT}"
- name: "Creating archive of target build dir"
id: archive
env:
TARGET_BUILD_DIR: ${{ steps.build.outputs.target-build-dir }}
TARGET_BUILD_DIR_ARCHIVE: build-android-arm_64.tar
run: |
tar -cf "${TARGET_BUILD_DIR_ARCHIVE}" \
--exclude="*.o" \
--exclude="*.a" \
"${TARGET_BUILD_DIR}"
echo "target-build-dir-archive=${TARGET_BUILD_DIR_ARCHIVE}" >> "${GITHUB_OUTPUT}"
- name: "Uploading target build dir archive"
id: upload
env:
TARGET_BUILD_DIR_ARCHIVE: ${{ steps.archive.outputs.target-build-dir-archive }}
TARGET_BUILD_DIR_GCS_ARTIFACT: ${{ env.GCS_DIR }}/${{ steps.archive.outputs.target-build-dir-archive }}
run: |
gcloud storage cp "${TARGET_BUILD_DIR_ARCHIVE}" "${TARGET_BUILD_DIR_GCS_ARTIFACT}"
echo "target-build-dir-gcs-artifact=${TARGET_BUILD_DIR_GCS_ARTIFACT}" >> "${GITHUB_OUTPUT}"
test:
# These physical devices are not scalable. Only run on postsubmit for now.
if: (! inputs.is-pr)
needs: cross_compile
strategy:
matrix:
# TODO(#9855): Add Pixel-6-Pro and XT2201-2
target:
- device-name: Pixel-4
label-exclude: vulkan
name: test_on_${{ matrix.target.device-name }}
runs-on:
- self-hosted # must come first
- runner-group=${{ inputs.runner-group }}
- environment=${{ inputs.runner-env }}
- machine-type=${{ matrix.target.device-name }}
env:
TARGET_BUILD_DIR: ${{ needs.cross_compile.outputs.target-build-dir }}
TARGET_BUILD_DIR_ARCHIVE: ${{ needs.cross_compile.outputs.target-build-dir-archive }}
TARGET_BUILD_DIR_GCS_ARTIFACT: ${{ needs.cross_compile.outputs.target-build-dir-gcs-artifact }}
steps:
- name: "Checking out repository"
uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0
- name: "Downloading target build archive"
run: |
gcloud storage cp "${TARGET_BUILD_DIR_GCS_ARTIFACT}" "${TARGET_BUILD_DIR_ARCHIVE}"
- name: "Extracting target build dir archive"
run: tar -xf "${TARGET_BUILD_DIR_ARCHIVE}" "${TARGET_BUILD_DIR}"
- name: "Running tests"
env:
LABEL_EXCLUDE: ${{ matrix.target.label-exclude }}
run: |
ctest -j 4 \
--test-dir "${TARGET_BUILD_DIR}" \
--timeout=900 \
--output-on-failure \
--no-tests=error \
--label-exclude "${LABEL_EXCLUDE}"
26 changes: 20 additions & 6 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,12 +861,6 @@ jobs:
strategy:
matrix:
target:
- platform: android
arch: armv8.2-a
abi: arm64-v8a
docker_image: "gcr.io/iree-oss/android@sha256:3f641d25786b1e5e430ee4cacb8bfe57540fda5ecaa7ca2802c179c26e77ce09"
build_script: "./build_tools/cmake/build_android.sh"
# No test_script
Comment on lines -864 to -869
Copy link
Member

Choose a reason for hiding this comment

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

It breaks out the Android target from the matrix cross_compile_and_test. I think it makes sense to do so as Android has its own matrix to run tests on multiple devices.

I'd prefer to add to the existing matrix (android_pixel_4, android_pixel_6, etc.). We can add another setup script or more env vars as needed for that. This PR is adding a lot of boilerplate code right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK we are not able to do that because android test workflow needs two different machines: 1. build machine to do cross-compile and 2. mobile phones to run tests. So they must be run in different jobs, but the matrix only supports a single job.

Ideally in the test step we can call a reusable workflow to run tests on different devices, but I don't think it's possible right now (reusable workflow must be called on the job level: https://docs.github.com/en/actions/using-workflows/reusing-workflows#calling-a-reusable-workflow)

Copy link
Contributor Author

@pzread pzread Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Another way is having a matrix job to cross-compile and upload test artifacts, and having another matrix job to download and run tests on different machines. But that will introduce unnecessary deps between cross-compile and test jobs (android tests need to wait for all cross-compilation finished because it can't just wait on the android cross-compile in the matrix due to Github limitation)

Copy link
Member

Choose a reason for hiding this comment

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

because android test workflow needs two different machines: 1. build machine to do cross-compile and 2. mobile phones to run tests

Isn't that the point of cross compiling in general though? Every job in the matrix will want to

  1. run the already built compiler the generate program artifacts
  2. build the runtime (fast, so doesn't need a large build machine)
  3. start an emulator, connect to a device, etc.
  4. run tests on the emulated platform / connected device

Copy link
Contributor Author

@pzread pzread Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Correct, but previously we can run build and test in a single job because we only use emulators to run tests on the same build machine. However it's not the case now because android tests are running on mobile phones and those devices don't connect to the build machine.

So we want something like

jobs:
  cross_compile_and_test:
    matrix:
      - arch: arm_64
        test_device: pixel-4-rpi
      - arch: riscv_64
        test_device: riscv-emulator
    steps:
      cross_compile:
        run-on: x86_64_build_machine
        runs: ./cross_compile.sh ${matrix.arch}
      test:
        # This is not possible today because `run-on` can only be at the job level.
        run-on: ${matrix.test_device}
        runs: ./run_test.sh

But the problem is we can't run steps on different devices in a single job and the matrix is limited to a single job.

Copy link
Contributor

Choose a reason for hiding this comment

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

Desktop GPU machines should be plenty powerful enough to build the runtime and compile any artifacts they need though

? We're only talking about Android here. We definitely would like to run Android emulator tests, but we're still going to need physical devices for real GPU tests and benchmarking.

It's annoying that GitHub Actions doesn't give us the right tools here. Looking at the graph, it seems like making the Android tests depend on the full cross-compilation wouldn't be too awful:

image

If we do, we should definitely leave a note about what's going on though.

An alternative would be to factor cross-compile into a reusable workflow, so that there's less duplication. I'm not sure whether passing the all the arguments ends up being as complicated as just copy-pasting the whole thing though.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we put it in a reusable workflow, can we get a matrix to run multiple jobs? https://docs.github.com/en/actions/using-workflows/reusing-workflows#using-a-matrix-strategy-with-a-reusable-workflow

The reusable workflow would do both compilation and testing and that would be abstracted away from the caller

Copy link
Contributor Author

@pzread pzread Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we put it in a reusable workflow, can we get a matrix to run multiple jobs? https://docs.github.com/en/actions/using-workflows/reusing-workflows#using-a-matrix-strategy-with-a-reusable-workflow

The reusable workflow would do both compilation and testing and that would be abstracted away from the caller

I think that is a good solution to reduce boilerplate code. But Github CI decides to not show the dependency graph for the reusable workflow in a matrix... (the two jobs in the screenshot below should be cross-compile -> test). Feel like this makes the readability worse

image
(from the run https://github.com/openxla/iree/actions/runs/4735027150)

That's pretty slow for pure overhead of something we want to run on presubmit. Will that be a bottleneck if we have multiple PRs and merged commits all queuing to use a single RPi? We might be able to proceed anyways, but the time burnt on overhead and the extra workflow complexity concerns me.

I updated the PR description. This test workflow is intentionally to only run on postsubmit for now (as we are currently doing with the buildkite test pipeline), as we only have two devices for each model.

I also want to mention the big benefit to proceeding is that we can drop the buildkite pipeline.

Copy link
Contributor Author

@pzread pzread Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

After trying more with reusable workflows and matrix jobs, I decided to stick with the original separate android workflow. Added the comment to explain why: https://github.com/openxla/iree/pull/13024/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR935-R941

Basically the requirements of running on physical (non-scalable) devices and limited capabilities of matrix jobs make the Android test more complicated.

Eventually other platforms might also have physical devices to run. At that point we can generalize the current Android workflow to reuse it. But that will require passing more parameters and customisable/dynamic logic in the workflow and I don't think it will happen in the near future, so maybe we shouldn't over-generalized the solution right now

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, while I'm not thrilled that we have to do it this way, the other near-term options are not awesome either. I think we should merge this as-is

- platform: linux
arch: riscv_64
abi: lp64d
Expand Down Expand Up @@ -938,6 +932,25 @@ jobs:
"${DOCKER_IMAGE}" \
"${TEST_SCRIPT}"

# Android cross-compilation and test is separated from cross_compile_and_test
# because some tests need to run on physical devices instead of emulators. And
# we need a fine-control on which tests only run on postsubmit as some devices
# are not scalable (while we want to test with Android emulator on ARM VMs on
# presubmit), which requires dynamic matrix generation (because "if" condition
# can't access matrix variable to skip certain matrix jobs for presubmit).
build_and_test_android:
needs: [setup, build_all]
if: fromJson(needs.setup.outputs.should-run)
uses: ./.github/workflows/build_and_test_android.yml
with:
pzread marked this conversation as resolved.
Show resolved Hide resolved
runner-group: ${{ needs.setup.outputs.runner-group }}
runner-env: ${{ needs.setup.outputs.runner-env }}
write-caches: ${{ needs.setup.outputs.write-caches }}
is-pr: ${{ fromJson(needs.setup.outputs.is-pr) }}
build-dir: ${{ needs.build_all.outputs.build-dir }}
build-dir-archive: ${{ needs.build_all.outputs.build-dir-archive }}
build-dir-gcs-artifact: ${{ needs.build_all.outputs.build-dir-gcs-artifact }}

test_benchmark_suites:
needs: [setup, build_all, build_e2e_test_artifacts]
if: fromJson(needs.setup.outputs.should-run)
Expand Down Expand Up @@ -1051,6 +1064,7 @@ jobs:

# Crosscompilation
- cross_compile_and_test
- build_and_test_android

# Artifacts for e2e testing and benchmarking
- build_benchmark_tools
Expand Down
14 changes: 9 additions & 5 deletions build_tools/cmake/build_android.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,26 @@
# Cross-compile the runtime using CMake targeting Android
#
# The required IREE_HOST_BIN_DIR environment variable indicates the location
# of the precompiled IREE binaries. Also requires that IREE_TARGET_ABI and
# ANDROID_NDK variables be set. The BUILD_PRESET environment variable indicates
# how the project should be configured: "test", "benchmark",
# "benchmark-with-tracing", or "benchmark-suite-test". Defaults to "test".
# of the precompiled IREE binaries. Also requires that ANDROID_NDK variables be
# set. The BUILD_PRESET environment variable indicates how the project should be
# configured: "test", "benchmark", "benchmark-with-tracing", or
# "benchmark-suite-test". Defaults to "test".
#
# The desired build directory can be passed as the first argument. Otherwise, it
# uses the environment variable IREE_TARGET_BUILD_DIR, defaulting to
# "build-android". Designed for CI, but can be run manually. It reuses the build
# directory if it already exists. Expects to be run from the root of the IREE
# repository.
#
# The default Android ABI is arm64-v8a, you can specify it with the variable
# IREE_ANDROID_ABI. See https://developer.android.com/ndk/guides/abis for the
# supported ABIs.


set -xeuo pipefail

BUILD_DIR="${1:-${IREE_TARGET_BUILD_DIR:-build-android}}"
ANDROID_ABI="${IREE_TARGET_ABI}"
ANDROID_ABI="${IREE_ANDROID_ABI:-arm64-v8a}"
IREE_HOST_BIN_DIR="$(realpath ${IREE_HOST_BIN_DIR})"
E2E_TEST_ARTIFACTS_DIR="${E2E_TEST_ARTIFACTS_DIR:-build-e2e-test-artifacts/e2e_test_artifacts}"
BUILD_PRESET="${BUILD_PRESET:-test}"
Expand Down