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

aelovikov-intel · 2023-06-06T15:48:24Z

No description provided.

bader

Please, fix CI failures.

bader · 2023-06-07T02:16:11Z

.github/workflows/sycl_detect_changes.yml

@@ -0,0 +1,48 @@
+name: Determine which check-* targets could be affected by the PR


Let's use something short. AFAIK, this name appears in places where only first N symbols are displayed. For example https://github.com/intel/llvm/actions (see left side). Another example - https://github.com/intel/llvm/actions/runs/5193089878.

How about "Identify impacted LIT tests"?

Do we have to name the workflow? I see that need_check name is very similar to this.

bader · 2023-06-07T02:38:44Z

devops/actions/lit-tests/action.yml

@@ -0,0 +1,81 @@
+name: Run device-agnostic LIT tests (check-*) as part of the build


Suggested change

name: Run device-agnostic LIT tests (check-*) as part of the build

name: Device-agnostic LIT tests

aelovikov-intel · 2023-06-07T14:46:59Z

Please, fix CI failures.

~~Yes, I'm working on this in #9762 - testing/developing CI changes is painful...~~

Updated: ~~strikethrough~~

aelovikov-intel · 2023-06-07T18:49:07Z

Tested in
https://github.com/intel/llvm/actions/runs/5202677130/jobs/9384599493
https://github.com/intel/llvm/actions/runs/5203035591/jobs/9385369569

Unfortunately, composite actions don't provide nice groupings (see https://github.com/orgs/community/discussions/21276) so it might get harder to deal with failures.

bader · 2023-06-07T20:02:50Z

Tested in https://github.com/intel/llvm/actions/runs/5202677130/jobs/9384599493 https://github.com/intel/llvm/actions/runs/5203035591/jobs/9385369569

Unfortunately, composite actions don't provide nice groupings (see https://github.com/orgs/community/discussions/21276) so it might get harder to deal with failures.

That's bad. New action output is quite big. It will be difficult to find an error.
Removing always() && could help - the execution will stop after the first failed check-*. But I think having results for all lit checks is quite important for the development.

aelovikov-intel · 2023-06-07T20:26:29Z

That's bad. New action output is quite big. It will be difficult to find an error.

Does it mean that you're against this unification?

bader · 2023-06-07T20:35:02Z

That's bad. New action output is quite big. It will be difficult to find an error.

Does it mean that you're against this unification?

I would say I'm against this implementation. I don't think removing code duplications justifies such user experience degradation.
Alternative implementation might be running just one check-*, which includes all the tests we run as a separate steps today and we filter out tests via LIT_FILTER variable.

aelovikov-intel · 2023-06-07T20:42:14Z

What if I group the logs via

echo "::group::check-llvm"
echo "::endgroup::"

? They will be folded by default and would lack success/failure marking but at least the grouping would be there.

bader · 2023-06-07T20:59:45Z

What if I group the logs via
echo "::group::check-llvm"
echo "::endgroup::"
? They will be folded by default and would lack success/failure marking but at least the grouping would be there.

You would still need to click through all of the groups to find where tests failed.

aelovikov-intel · 2023-06-08T15:14:53Z

Changes are being tested in https://github.com/intel/llvm/actions/runs/5212842934/jobs/9407002813?pr=9776.

aelovikov-intel · 2023-06-08T15:25:03Z

.github/workflows/sycl_windows_build_and_test.yml

@@ -94,35 +71,29 @@ jobs:
      run: |
        cmake --build build --target sycl-toolchain
    - name: check-llvm
-      shell: bash


I can keep shell: bash if you'd like (although it's noisy), but it seems to work with default shell (CMD) as well.

It's the former that we don't want to run.

bader · 2023-06-08T16:35:42Z

.github/workflows/sycl_precommit.yml

@@ -27,53 +27,8 @@ permissions:
  contents: read

 jobs:


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

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

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

Two reasons:

I don't feel comfortable doing the switch yet.

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.

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?

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

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?

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

aelovikov-intel requested a review from a team as a code owner June 6, 2023 15:48

aelovikov-intel temporarily deployed to aws June 6, 2023 15:51 — with GitHub Actions Inactive

aelovikov-intel force-pushed the refactor-filter branch from 2701b4e to c29c8e1 Compare June 6, 2023 15:54

aelovikov-intel temporarily deployed to aws June 6, 2023 16:03 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to aws June 6, 2023 16:41 — with GitHub Actions Inactive

aelovikov-intel force-pushed the refactor-filter branch from c29c8e1 to a4a83e3 Compare June 6, 2023 17:35

aelovikov-intel temporarily deployed to aws June 6, 2023 17:51 — with GitHub Actions Inactive

bader requested changes Jun 7, 2023

View reviewed changes

bader reviewed Jun 7, 2023

View reviewed changes

aelovikov-intel force-pushed the refactor-filter branch from a4a83e3 to e5d0159 Compare June 7, 2023 17:58

aelovikov-intel temporarily deployed to aws June 7, 2023 18:41 — with GitHub Actions Inactive

aelovikov-intel requested a review from bader June 7, 2023 18:47

aelovikov-intel requested a review from steffenlarsen June 7, 2023 18:49

aelovikov-intel temporarily deployed to aws June 7, 2023 19:31 — with GitHub Actions Inactive

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

09f8f07

aelovikov-intel force-pushed the refactor-filter branch from e5d0159 to 09f8f07 Compare June 8, 2023 15:14

aelovikov-intel changed the title ~~[CI] Unify check-* in Build/LIT task between Linux/Windows~~ [CI] Refactor changes detection for check-* in Build/LIT tasks Jun 8, 2023

aelovikov-intel commented Jun 8, 2023

View reviewed changes

Change libclc -> libdevice check on Windows

990bbe0

It's the former that we don't want to run.

aelovikov-intel temporarily deployed to aws June 8, 2023 16:39 — with GitHub Actions Inactive

bader approved these changes Jun 8, 2023

View reviewed changes

aelovikov-intel temporarily deployed to aws June 8, 2023 18:20 — with GitHub Actions Inactive

aelovikov-intel merged commit a230a62 into intel:sycl Jun 8, 2023

aelovikov-intel deleted the refactor-filter branch June 8, 2023 19:08

whitneywhtsang mentioned this pull request Jun 9, 2023

[SYCL-MLIR] Merge from intel/llvm sycl branch #9805

Merged

		@@ -0,0 +1,48 @@
		name: Determine which check-* targets could be affected by the PR

		@@ -0,0 +1,81 @@
		name: Run device-agnostic LIT tests (check-*) as part of the build

	name: Run device-agnostic LIT tests (check-*) as part of the build
	name: Device-agnostic LIT tests

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

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

Uh oh!

Conversation

aelovikov-intel commented Jun 6, 2023

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aelovikov-intel commented Jun 7, 2023

Uh oh!

bader commented Jun 7, 2023

Uh oh!

aelovikov-intel commented Jun 7, 2023

Uh oh!

bader commented Jun 7, 2023

Uh oh!

aelovikov-intel commented Jun 7, 2023

Uh oh!

bader commented Jun 7, 2023

Uh oh!

aelovikov-intel commented Jun 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aelovikov-intel commented Jun 7, 2023 •

edited

Loading