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

[CI] Prebuild E2E tests as part of build workflow #16682

Merged
merged 1 commit into from
Jan 24, 2025
Merged

[CI] Prebuild E2E tests as part of build workflow #16682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 36 additions & 2 deletions .github/workflows/sycl-linux-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ on:
build_image:
type: string
required: false
default: "ghcr.io/intel/llvm/ubuntu2404_build:latest"
default: "ghcr.io/intel/llvm/ubuntu2404_intel_drivers:alldeps"
build_ref:
type: string
required: false
Expand Down Expand Up @@ -70,7 +70,7 @@ on:
build_image:
type: choice
options:
- "ghcr.io/intel/llvm/sycl_ubuntu2204_nightly:build"
- 'ghcr.io/intel/llvm/sycl_ubuntu2404_nightly:latest'
uditagarwal97 marked this conversation as resolved.
Show resolved Hide resolved
cc:
type: choice
options:
Expand Down Expand Up @@ -252,3 +252,37 @@ jobs:
name: sycl_linux_${{ inputs.build_artifact_suffix }}
path: ${{ steps.artifact_info.outputs.ARCHIVE_NAME }}
retention-days: ${{ inputs.retention-days }}

- name: Copy toolchain
if: ${{ always() && !cancelled() && steps.build.conclusion == 'success' }}
# We must have the compiler in the same location as it will be in the E2E
# run-tests job.
run: cp -r $GITHUB_WORKSPACE/build/install $GITHUB_WORKSPACE/toolchain

- name: Source OneAPI TBB vars.sh
shell: bash
run: |
# https://github.com/actions/runner/issues/1964 prevents us from using
# the ENTRYPOINT in the image.
env | sort > env_before
if [ -e /runtimes/oneapi-tbb/env/vars.sh ]; then
source /runtimes/oneapi-tbb/env/vars.sh;
elif [ -e /opt/runtimes/oneapi-tbb/env/vars.sh ]; then
source /opt/runtimes/oneapi-tbb/env/vars.sh;
else
echo "no TBB vars in /opt/runtimes or /runtimes";
fi
env | sort > env_after
comm -13 env_before env_after >> $GITHUB_ENV
rm env_before env_after
Comment on lines +262 to +277
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean that whenever we call .github/workflows/sycl-linux-build.yml, we also build E2E tests? IIRC, we don't use pre-built test binaries in post-commit and nightly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we do, and no, we don't. That said I imagine we can start doing so relatively soon. Alternatively, we can make this addition optional, but I thought that build-only mode of e2e tests isn't that different from check-sycl that we do unconditionally everywhere (well, if sycl is modified in pre-commit). Writing this, I can even imagine re-unification of sycl/test and sycl/test-e2e now!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would personally be of the opinion that we make this addition optional - don't build E2E tests by default when sycl-linux-build.yml is called, unless the caller explicitly asks for it.
Once the "split-test" mode is mature enough that we use it for all workflows and other runners as well, we can build E2E tests by default, just like sycl/test.

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 think it will just complicate implementation with very little benefit... Nightly/post traffic is several times smaller than pre-commit (because PR usually take several iterations). Also, unlike pre-commit, nightly/post don't skip any check-* at build stage, so having e2e built there takes less percentage of the overall duration.

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 udit if the implementation cost is low, buf it's high as andrei says im fine with enabling it always

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will just complicate implementation with very little benefit...

If it complicates the implementation, I'm fine with the current PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'll plan to work on it the coming days/weeks. Either make sure the e2e artifacts get used more or make optional, don't believe we need to delay progress by making extra changes and extra testing. Too easy to make a typo and too long to run the full CI cycle. Very much would like to merge while it's green :)


- name: Build E2E tests
if: ${{ always() && !cancelled() && steps.build.conclusion == 'success' }}
uses: ./devops/actions/run-tests/e2e
with:
ref: ${{ inputs.ref || github.sha }}
merge_ref: ${{ inputs.merge_ref }}
e2e_testing_mode: build-only
target_devices: all
artifact_suffix: default
cxx_compiler: $GITHUB_WORKSPACE/toolchain/bin/clang++
Copy link
Contributor

@sarnex sarnex Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

if we dont pass this, it falls back to which clang++, so i assume the built clang++ isn't on the path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed offline, we need to make sure CMake sees the same CXX compiler at build/run stages.

19 changes: 2 additions & 17 deletions .github/workflows/sycl-linux-precommit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ jobs:
build_artifact_suffix: "default"
build_cache_suffix: "default"
# Docker image has last nightly pre-installed and added to the PATH
build_image: "ghcr.io/intel/llvm/sycl_ubuntu2204_nightly:build"
build_image: "ghcr.io/intel/llvm/sycl_ubuntu2404_nightly:latest"
uditagarwal97 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just delete the build image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed offline, we only create *nightly:latest now, the :build is getting increasingly stale/outdated.

cc: clang
cxx: clang++
changes: ${{ needs.detect_changes.outputs.filters }}
Expand All @@ -74,23 +74,8 @@ jobs:
echo 'arc_tests="Matrix/"' >> "$GITHUB_OUTPUT"
fi
build_e2e_tests:
needs: [build]
if: ${{ always() && !cancelled() && needs.build.outputs.build_conclusion == 'success' }}
uses: ./.github/workflows/sycl-linux-run-tests.yml
with:
name: Build e2e tests
runner: '["Linux", "build"]'
image: ghcr.io/intel/llvm/ubuntu2404_intel_drivers:alldeps
image_options: -u 1001
ref: ${{ github.sha }}
merge_ref: ''
sycl_toolchain_artifact: sycl_linux_default
sycl_toolchain_archive: ${{ needs.build.outputs.artifact_archive_name }}
sycl_toolchain_decompress_command: ${{ needs.build.outputs.artifact_decompress_command }}
e2e_testing_mode: 'build-only'
run_prebuilt_e2e_tests:
needs: [build, build_e2e_tests]
needs: [build]
if: ${{ always() && !cancelled() && needs.build.outputs.build_conclusion == 'success' }}
strategy:
fail-fast: false
Expand Down
4 changes: 3 additions & 1 deletion devops/actions/run-tests/e2e/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ inputs:
required: false
retention-days:
required: false
cxx_compiler:
required: false


runs:
Expand Down Expand Up @@ -58,7 +60,7 @@ runs:
if: inputs.e2e_binaries_artifact == ''
shell: bash
run: |
cmake -GNinja -B./build-e2e -S./llvm/sycl/test-e2e -DCMAKE_CXX_COMPILER="$(which clang++)" -DLLVM_LIT="$PWD/llvm/llvm/utils/lit/lit.py" ${{ steps.cmake_opts.outputs.opts }}
cmake -GNinja -B./build-e2e -S./llvm/sycl/test-e2e -DCMAKE_CXX_COMPILER="${{ inputs.cxx_compiler || '$(which clang++)'}}" -DLLVM_LIT="$PWD/llvm/llvm/utils/lit/lit.py" ${{ steps.cmake_opts.outputs.opts }}
- name: SYCL End-to-end tests
shell: bash {0}
env:
Expand Down
Loading