Skip to content

[CI] Refactor changes detection for check-* in Build/LIT tasks #9760

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

Merged
merged 2 commits into from
Jun 8, 2023
Merged

[CI] Refactor changes detection for check-* in Build/LIT tasks #9760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09f8f07
[CI] Refactor changes detection for check-* in Build/LIT tasks
aelovikov-intel Jun 6, 2023
990bbe0
Change libclc -> libdevice check on Windows
aelovikov-intel Jun 8, 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
50 changes: 50 additions & 0 deletions .github/workflows/sycl_detect_changes.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
name: Identify impacted LIT tests
# Outlined into a separate reusable workflow to ease testing for new rules.

on:
workflow_call:
outputs:
filters:
description: Matched filters
value: ${{ jobs.need_check.outputs.filters }}

jobs:
need_check:
name: Decide which tests could be affected by the changes
# Github's ubuntu-* runners are slow to allocate. Use our CUDA runner since
# we don't use it for anything right now.
runs-on: cuda
timeout-minutes: 3
outputs:
filters: ${{ steps.changes.outputs.changes }}
steps:
- name: Check file changes
uses: dorny/paths-filter@4512585405083f25c027a35db413c2b3b9006d50
id: changes
with:
filters: |
llvm: &llvm
- 'llvm/**'
llvm_spirv: &llvm_spirv
- *llvm
- 'llvm-spirv/**'
clang: &clang
- *llvm
- 'clang/**'
sycl_fusion: &sycl-fusion
- *llvm
- 'sycl-fusion/**'
xptifw: &xptifw
- 'xptifw/**'
libclc: &libclc
- *llvm_spirv
- *clang
- 'libclc/**'
sycl:
- *clang
- *sycl-fusion
- *llvm_spirv
- *xptifw
- *libclc
- 'sycl/*'
- 'sycl/!(test-e2e)/**'
47 changes: 12 additions & 35 deletions .github/workflows/sycl_linux_build_and_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,34 +64,11 @@ on:
type: string
required: false
default: 'SYCL-2020'
check_llvm:
check_filters:
type: string
description: 'Filter matches for the changed files in the PR'
default: '[llvm, clang, sycl, llvm_spirv, xptifw, libclc, libdevice]'
required: false
default: 'true'
check_clang:
type: string
required: false
default: 'true'
check_sycl:
type: string
required: false
default: 'true'
check_llvm_spirv:
type: string
required: false
default: 'true'
check_xptifw:
type: string
required: false
default: 'true'
check_libclc:
type: string
required: false
default: 'true'
check_libdevice:
type: string
required: false
default: 'true'

jobs:
build:
Expand Down Expand Up @@ -136,36 +113,36 @@ jobs:
- name: Compile
id: build
run: cmake --build $GITHUB_WORKSPACE/build
# TODO allow to optionally disable in-tree checks
- name: check-llvm
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_llvm == 'true' }}
if: always() && !cancelled() && contains(inputs.check_filters, 'llvm')
run: |
cmake --build $GITHUB_WORKSPACE/build --target check-llvm
- name: check-clang
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_clang == 'true' }}
if: always() && !cancelled() && contains(inputs.check_filters, 'clang')
run: |
# Can we move this to Dockerfile? Hopefully, noop on Windows.
export XDG_CACHE_HOME=$GITHUB_WORKSPACE/os_cache
cmake --build $GITHUB_WORKSPACE/build --target check-clang
- name: check-sycl
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_sycl == 'true' }}
if: always() && !cancelled() && contains(inputs.check_filters, 'sycl')
run: |
# TODO consider moving this to Dockerfile
# TODO consider moving this to Dockerfile.
export LD_LIBRARY_PATH=/usr/local/cuda/compat/:/usr/local/cuda/lib64:$LD_LIBRARY_PATH
cmake --build $GITHUB_WORKSPACE/build --target check-sycl
- name: check-llvm-spirv
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_llvm_spirv == 'true' }}
if: always() && !cancelled() && contains(inputs.check_filters, 'llvm_spirv')
run: |
cmake --build $GITHUB_WORKSPACE/build --target check-llvm-spirv
- name: check-xptifw
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_xptifw == 'true' }}
if: always() && !cancelled() && contains(inputs.check_filters, 'xptifw')
run: |
cmake --build $GITHUB_WORKSPACE/build --target check-xptifw
- name: check-libclc
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_libclc == 'true' }}
if: always() && !cancelled() && contains(inputs.check_filters, 'libclc')
run: |
cmake --build $GITHUB_WORKSPACE/build --target check-libclc
- name: check-libdevice
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_libdevice == 'true' }}
if: always() && !cancelled() && contains(inputs.check_filters, 'libdevice')
run: |
cmake --build $GITHUB_WORKSPACE/build --target check-libdevice
- name: Install
Expand Down
67 changes: 6 additions & 61 deletions .github/workflows/sycl_precommit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,53 +27,8 @@ permissions:
contents: read

jobs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand it correctly that we leave post-commit as is because the method to detect impacted tests is different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't do such detection/filtering there at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know and my question is "why?". It looks like a low hanging fruit to speed-up CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two reasons:

  1. I don't feel comfortable doing the switch yet.
  2. I don't think it's necessarily the right choice. Imagine an xptifw test failing. We change that part so rarely that we won't be running it for weeks if not months and it would be easy for the bugs to slip through.

Copy link
Contributor

Choose a reason for hiding this comment

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

2. We change that part so rarely that we won't be running it for weeks if not months and it would be easy for the bugs to slip through.

What king of bugs do you expect to slip through, if there are no code changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commit1 - green
Commit2 - red, regression
CommitN..M - green, because didn't touch that component, regression is still present

Copy link
Contributor

Choose a reason for hiding this comment

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

Commit 2 - red, regression <-- this is where the bug is caught, so I don't how it can "slip through".

Are you trying to say that the gatekeeper who merged commit 2 can skip reporting this issue and failing post-commits for commit N..M expose this issue to other gatekeepers, who will report it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That or just a constant reminder that there is an issue if we always test.

need_check:
name: Decide which tests could be affected by the changes
runs-on: ubuntu-22.04
timeout-minutes: 3
outputs:
llvm: ${{ steps.changes.outputs.llvm == 'true' }}
llvm_spirv: ${{ steps.changes.outputs.llvm_spirv == 'true' }}
clang: ${{ steps.changes.outputs.clang == 'true' }}
xptifw: ${{ steps.changes.outputs.xptifw == 'true' }}
libclc: ${{ steps.changes.outputs.libclc == 'true' }}
sycl: ${{ steps.changes.outputs.sycl == 'true' }}
steps:
- name: Check file changes
uses: dorny/paths-filter@4512585405083f25c027a35db413c2b3b9006d50
id: changes
with:
filters: |
llvm: &llvm
- 'llvm/**'
llvm_spirv: &llvm_spirv
- *llvm
- 'llvm-spirv/**'
clang: &clang
- *llvm
- 'clang/**'
sycl_fusion: &sycl-fusion
- *llvm
- 'sycl-fusion/**'
xptifw: &xptifw
- 'xptifw/**'
libclc: &libclc
- *llvm_spirv
- *clang
- 'libclc/**'
libdevice: &libdevice
- *llvm_spirv
- *clang
- 'libdevice/**'
sycl:
- *clang
- *sycl-fusion
- *llvm_spirv
- *xptifw
- *libclc
- *libdevice
- 'sycl/*'
- 'sycl/!(test-e2e)/**'
detect_changes:
uses: ./.github/workflows/sycl_detect_changes.yml

lint:
runs-on: ubuntu-22.04
Expand Down Expand Up @@ -102,7 +57,7 @@ jobs:
name: Linux
# Only build and test patches, that have passed all linter checks, because
# the next commit is likely to be a follow-up on that job.
needs: [lint, test_matrix, need_check]
needs: [lint, test_matrix, detect_changes]
if: always() && (success() || contains(github.event.pull_request.labels.*.name, 'ignore-lint'))
uses: ./.github/workflows/sycl_linux_build_and_test.yml
secrets: inherit
Expand All @@ -114,24 +69,14 @@ jobs:
build_cache_suffix: "default"
lts_matrix: ${{ needs.test_matrix.outputs.lts_lx_matrix }}
lts_aws_matrix: ${{ needs.test_matrix.outputs.lts_aws_matrix }}
check_llvm: ${{ needs.need_check.outputs.llvm }}
check_llvm_spirv: ${{ needs.need_check.outputs.llvm_spirv }}
check_clang: ${{ needs.need_check.outputs.clang }}
check_xptifw: ${{ needs.need_check.outputs.xptifw }}
check_libclc: ${{ needs.need_check.outputs.libclc }}
check_sycl: ${{ needs.need_check.outputs.sycl }}
check_filters: ${{ needs.detect_changes.outputs.filters }}

windows_default:
name: Windows
needs: [lint, test_matrix, need_check]
needs: [lint, test_matrix, detect_changes]
if: github.repository == 'intel/llvm'
uses: ./.github/workflows/sycl_windows_build_and_test.yml
with:
lts_matrix: ${{ needs.test_matrix.outputs.lts_wn_matrix }}
build_ref: ${{ github.event.pull_request.head.sha }}
check_llvm: ${{ needs.need_check.outputs.llvm }}
check_llvm_spirv: ${{ needs.need_check.outputs.llvm_spirv }}
check_clang: ${{ needs.need_check.outputs.clang }}
check_xptifw: ${{ needs.need_check.outputs.xptifw }}
check_libclc: ${{ needs.need_check.outputs.libclc }}
check_sycl: ${{ needs.need_check.outputs.sycl }}
check_filters: ${{ needs.detect_changes.outputs.filters }}
47 changes: 9 additions & 38 deletions .github/workflows/sycl_windows_build_and_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,34 +18,11 @@ on:
type: string
required: false
default: ""
check_llvm:
check_filters:
type: string
description: 'Filter matches for the changed files in the PR'
default: '[llvm, clang, sycl, llvm_spirv, xptifw, libclc, libdevice]'
required: false
default: 'true'
check_clang:
type: string
required: false
default: 'true'
check_sycl:
type: string
required: false
default: 'true'
check_llvm_spirv:
type: string
required: false
default: 'true'
check_xptifw:
type: string
required: false
default: 'true'
check_libclc:
type: string
required: false
default: 'true'
check_libdevice:
type: string
required: false
default: 'true'

jobs:
build:
Expand Down Expand Up @@ -94,33 +71,27 @@ jobs:
run: |
cmake --build build --target sycl-toolchain
- name: check-llvm
shell: bash
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 can keep shell: bash if you'd like (although it's noisy), but it seems to work with default shell (CMD) as well.

if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_llvm == 'true' }}
if: always() && !cancelled() && contains(inputs.check_filters, 'llvm')
run: |
cmake --build build --target check-llvm
- name: check-clang
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_clang == 'true' }}
shell: bash
if: always() && !cancelled() && contains(inputs.check_filters, 'clang')
run: |
cmake --build build --target check-clang
- name: check-sycl
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_sycl == 'true' }}
shell: bash
if: always() && !cancelled() && contains(inputs.check_filters, 'sycl')
run: |
cmake --build build --target check-sycl
- name: check-llvm-spirv
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_llvm_spirv == 'true' }}
shell: bash
if: always() && !cancelled() && contains(inputs.check_filters, 'llvm_spirv')
run: |
cmake --build build --target check-llvm-spirv
- name: check-xptifw
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_xptifw == 'true' }}
shell: bash
if: always() && !cancelled() && contains(inputs.check_filters, 'xptifw')
run: |
cmake --build build --target check-xptifw
- name: check-libdevice
if: ${{ always() && !cancelled() && steps.build.outcome == 'success' && inputs.check_libdevice == 'true' }}
shell: bash
if: always() && !cancelled() && contains(inputs.check_filters, 'libdevice')
run: |
cmake --build build --target check-libdevice
- name: Install
Expand Down