[CI][Bench] Combine all benchmark-related jobs in one place #20439

lukaszstolarczuk · 2025-10-23T13:20:18Z

plus cleanups. Each commit makes its own change - the most important part is the last commit.

PatKamin · 2025-10-24T08:25:52Z

.github/workflows/sycl-ur-perf-benchmarking.yml

 name: Run Benchmarks

 on:
+  # Only run on pull requests, when a benchmark-related files were changed.


Run on pull requests only when... - to not suggest that jobs run only on pull requests

right, done

PatKamin · 2025-10-24T08:33:38Z

.github/workflows/sycl-ur-perf-benchmarking.yml

-  sanitize_inputs:
+  # Manual trigger (dispatch) path:
+  sanitize_inputs_dispatch:
    name: Sanitize inputs


Add a condition on a github event (dispatch only)

Thx! I've checked these if's and triggers, like 10 times now, but still keep finding issues in there. Hopefully now it's all good (I'll see if CI run what I wanted)

ok, it does look promising, on PR only PR jobs were executed.

I also tested dispatch on my fork (https://github.com/lukaszstolarczuk/llvm/actions/runs/18778151347/job/53577517025) - I don't have the runners, but it looks promising.

simulated nightly on my fork also looks promising: https://github.com/lukaszstolarczuk/llvm/actions/runs/18778773351

lukaszstolarczuk · 2025-10-24T11:01:18Z

.github/workflows/sycl-ur-perf-benchmarking.yml

-  build_sycl:
-    name: Build SYCL
-    needs: [ sanitize_inputs ]
+  build_sycl_dispatch:


As a next step I'd like to verify if we could work with a single build job - right now I just copy-pasted build jobs from other workflows... but they differ in paramaters - e.g. sometimes we use clang compiler and sometimes (default) gcc - this would probably be discussed and we should just use the same build for all benchmark jobs, if possible.

Yes, I don't know the reason for different compilers usage, too. It seems a bit odd to be able to compare on-demand results with nightly ones with sycl built with a totally different compiler.

lukaszstolarczuk · 2025-10-24T11:01:33Z

.github/workflows/sycl-ur-perf-benchmarking.yml

 name: Run Benchmarks

 on:
+  # Only run on pull requests, when a benchmark-related files were changed.


right, done

lukaszstolarczuk · 2025-10-24T11:03:05Z

.github/workflows/sycl-ur-perf-benchmarking.yml

-  sanitize_inputs:
+  # Manual trigger (dispatch) path:
+  sanitize_inputs_dispatch:
    name: Sanitize inputs


Thx! I've checked these if's and triggers, like 10 times now, but still keep finding issues in there. Hopefully now it's all good (I'll see if CI run what I wanted)

lukaszstolarczuk · 2025-10-24T11:56:18Z

.github/workflows/sycl-ur-perf-benchmarking.yml

+    uses: ./.github/workflows/sycl-linux-run-tests.yml
+    with:
+      name: 'Framework test: PVC_PERF, L0, Minimal preset'
+      runner: '["PVC_PERF"]'


@intel/llvm-reviewers-benchmarking Perhaps we should change this to BMG runner? I guess PVC runner is used more often for manual runs, so we wouldn't have to wait for benchmarks to finish to only test the framework...? What do you think?

Once there are two stable BMG machines, this would be a good move. For now, the PVC machine is much, much, much more stable than the BMG one, so I would stay with the current config. Add TODO to consider the change in the future, though.

You're so right. TODO added.

lukaszstolarczuk · 2025-10-24T11:56:33Z

.github/workflows/sycl-ur-perf-benchmarking.yml

-  sanitize_inputs:
+  # Manual trigger (dispatch) path:
+  sanitize_inputs_dispatch:
    name: Sanitize inputs


ok, it does look promising, on PR only PR jobs were executed.

I also tested dispatch on my fork (https://github.com/lukaszstolarczuk/llvm/actions/runs/18778151347/job/53577517025) - I don't have the runners, but it looks promising.

simulated nightly on my fork also looks promising: https://github.com/lukaszstolarczuk/llvm/actions/runs/18778773351

PatKamin · 2025-10-24T12:40:45Z

.github/workflows/sycl-linux-run-tests.yml

    inputs:
      runner:
+        # While it's ok to run benchmarks in this workflow, "*_PERF" machines
+        # shouldn't be used on manual dispatch. Instead, use sycl-ur-perf-benchmarking.yml


This comment can be removed...?

I would leave it, so no one would add these perf machines here

Instead we could add an error to check the runner and error if it's not a supported one.

Why shouldn't they be used? We aren't really disallowing that (one can still push a branch with modified sycl-linux-run-tests.yml and then have a manual run from that branch).

The whole point of this workflow_dispatch is for debugging CI/ reducing CI load when making PRs to it. If UR benchmarking workflows do call sycl-linux-run-tests.yml with those parameters, then they are needed here for such experiments.

1. Right now changing only benchmark framework triggers all SYCL build. Thanks to this change only relevant changes are triggered for testing framework. 2. Nightly build was seperated. I believe keeping everything in one place makes it easier to maintain changes. No changes in logic/builds were made in this commit - only minor cleanups like names, plus changed the triggers.

…her SYCL workflows

sarnex · 2025-10-24T15:18:26Z

.github/workflows/sycl-linux-run-tests.yml

    inputs:
      runner:
+        # While it's ok to run benchmarks in this workflow, "*_PERF" machines
+        # shouldn't be used on manual dispatch. Instead, use sycl-ur-perf-benchmarking.yml


Instead we could add an error to check the runner and error if it's not a supported one.

sarnex · 2025-10-24T15:21:34Z

.github/workflows/sycl-ur-perf-benchmarking.yml

+
+  # Nightly benchmarking path:
+  build_nightly:
+    name: '[Nightly] Build SYCL'


I didn't look at the code for the jobs in depth but why do we need separate jobs depending on the workflow call type?

aelovikov-intel

Non-benchmarking workflow LGTM, my inline comment in sycl-linux-run-tests.yml is not affecting anybody outside benchmarking CI folks, so irrelevant.

Benchmarking workflow itself doesn't do anything dangerous/totally crazy so I'd be fine merging as-is. That said, I didn't review the benchmarking jobs in details (i.e., maintainability/best practices/etc.) but that's up to the folks involved with the benchmarking CI.

Please make sure @sarnex is fine with this before merging though.

sarnex · 2025-10-24T16:41:53Z

My only blocking question is about the justification for the duplication

lukaszstolarczuk had a problem deploying to WindowsCILock October 23, 2025 13:20 — with GitHub Actions Failure

lukaszstolarczuk temporarily deployed to WindowsCILock October 23, 2025 13:20 — with GitHub Actions Inactive

lukaszstolarczuk temporarily deployed to WindowsCILock October 23, 2025 13:47 — with GitHub Actions Inactive

lukaszstolarczuk temporarily deployed to WindowsCILock October 23, 2025 13:57 — with GitHub Actions Inactive

PatKamin reviewed Oct 24, 2025

View reviewed changes

lukaszstolarczuk added 2 commits October 24, 2025 12:55

[CI][Bench] Cleanups in workflows and bench-related scripts

e59f6c1

[CI][Bench] Remove unused workflow_call trigger in benchmark's workflow

d71c393

lukaszstolarczuk force-pushed the benchmarks-ci branch from 0060067 to c33827e Compare October 24, 2025 10:56

lukaszstolarczuk had a problem deploying to WindowsCILock October 24, 2025 10:56 — with GitHub Actions Failure

lukaszstolarczuk temporarily deployed to WindowsCILock October 24, 2025 10:56 — with GitHub Actions Inactive

lukaszstolarczuk commented Oct 24, 2025

View reviewed changes

lukaszstolarczuk requested a review from PatKamin October 24, 2025 11:03

lukaszstolarczuk temporarily deployed to WindowsCILock October 24, 2025 11:22 — with GitHub Actions Inactive

lukaszstolarczuk temporarily deployed to WindowsCILock October 24, 2025 11:50 — with GitHub Actions Inactive

lukaszstolarczuk commented Oct 24, 2025

View reviewed changes

lukaszstolarczuk marked this pull request as ready for review October 24, 2025 11:59

lukaszstolarczuk requested review from a team as code owners October 24, 2025 11:59

PatKamin reviewed Oct 24, 2025

View reviewed changes

lukaszstolarczuk added 2 commits October 24, 2025 15:20

[CI][Bench] Adjust benchmarks' workflow name to include 'SYCL', as ot…

5930203

…her SYCL workflows

lukaszstolarczuk force-pushed the benchmarks-ci branch from c33827e to 5930203 Compare October 24, 2025 13:20

lukaszstolarczuk had a problem deploying to WindowsCILock October 24, 2025 13:20 — with GitHub Actions Failure

lukaszstolarczuk temporarily deployed to WindowsCILock October 24, 2025 13:20 — with GitHub Actions Inactive

lukaszstolarczuk mentioned this pull request Oct 24, 2025

[CI][Benchmarks] do not build and run E2E test on benchmark only changes #20414

Closed

lukaszstolarczuk temporarily deployed to WindowsCILock October 24, 2025 13:57 — with GitHub Actions Inactive

lukaszstolarczuk temporarily deployed to WindowsCILock October 24, 2025 14:05 — with GitHub Actions Inactive

sarnex reviewed Oct 24, 2025

View reviewed changes

aelovikov-intel approved these changes Oct 24, 2025

View reviewed changes

Uh oh!

[CI][Bench] Combine all benchmark-related jobs in one place #20439

Are you sure you want to change the base?

[CI][Bench] Combine all benchmark-related jobs in one place #20439

Uh oh!

Conversation

lukaszstolarczuk commented Oct 23, 2025

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!

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!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

sarnex commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants