Skip to content

Conversation

RossBrunton
Copy link
Contributor

@RossBrunton RossBrunton commented Apr 2, 2025

A number of changes to UR won't have any observable effects on SYCL
builds, so no point in running a full SYCL build and e2e test for them.

@RossBrunton RossBrunton requested a review from a team April 2, 2025 14:46
@martygrant
Copy link
Contributor

couple files in the root dir we wouldn't want to run CI for like the README.md etc, from the discussion in this issue last year oneapi-src/unified-runtime#1511 maybe do the opposite and only run CI jobs if the change touches source, include and scripts?

@kbenzie
Copy link
Contributor

kbenzie commented Apr 2, 2025

couple files in the root dir we wouldn't want to run CI for like the README.md etc, from the discussion in this issue last year oneapi-src/unified-runtime#1511 maybe do the opposite and only run CI jobs if the change touches source, include and scripts?

Only needs to be include and source. If scripts changes result in different the code those will already be captured.

@kbenzie
Copy link
Contributor

kbenzie commented Apr 2, 2025

I think this workflow is also relevant sycl-detect-changes.yml as its the one that runs before the other workflows and decides which actually run.

@RossBrunton
Copy link
Contributor Author

couple files in the root dir we wouldn't want to run CI for like the README.md etc, from the discussion in this issue last year oneapi-src/unified-runtime#1511 maybe do the opposite and only run CI jobs if the change touches source, include and scripts?

@martygrant A quick skim of the docs suggests that it's non-trivial to add rules to both "allow" and "disallow" paths, and such a thing feels like it would be confusing to understand. Also, if some new directory gets added in the future without updating this, I think it's better to accidentality run more jobs than to skip jobs that could fail.

I think this workflow is also relevant sycl-detect-changes.yml as its the one that runs before the other workflows and decides which actually run.

@kbenzie I had a look into that, but to use it I think I would have to have the job have a long list of contains(inputs.changes, 'llvm') && contains(inputs.changes, 'sycl') && .... The upside to doing this like this is that we don't need to run the SYCL pre-commit job, which means one less node needing to be allocated.

@RossBrunton RossBrunton marked this pull request as ready for review April 2, 2025 15:53
@RossBrunton RossBrunton requested a review from a team as a code owner April 2, 2025 15:53
A number of changes to UR won't have any observable effects on SYCL
builds, so no point in running a full SYCL build and e2e test for them.
@aelovikov-intel
Copy link
Contributor

Title needs to be updated. It's not all UR changes that skip CI.

@RossBrunton RossBrunton changed the title [UR] Skip SYCL e2e testing for UR-only changes [UR] Don't run SYCL testing for changes that only affect UR Apr 3, 2025
@RossBrunton RossBrunton changed the title [UR] Don't run SYCL testing for changes that only affect UR [CI] Don't run SYCL testing for changes that only affect UR Apr 3, 2025
@kbenzie kbenzie merged commit 48a08bf into intel:sycl Apr 4, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants