Skip to content

[CI][CTS] Add fixed ref for cts #17074

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

Jump to bottom
Merged
merged 5 commits into from
Feb 26, 2025
Merged

[CI][CTS] Add fixed ref for cts #17074

merged 5 commits into from
Feb 26, 2025

Conversation

KornevNikita
Copy link
Contributor

This patch adds a new input to fix the specific version of cts. In general it's needed for sycl-rel as the cts is continuously changing.

@KornevNikita
[CI][CTS] Add fixed ref for cts
9810e5b 
This patch adds a new input to fix the specific version of cts. In
general it's needed for sycl-rel as the cts is continuously changing.
@KornevNikita KornevNikita requested a review from a team as a code owner February 19, 2025 18:22
aelovikov-intel
aelovikov-intel reviewed Feb 19, 2025
View reviewed changes
.github/workflows/sycl-linux-run-tests.yml Outdated
Comment on lines 51 to 55
sycl_cts_ref:
description: "Commit SHA or branch to checkout"
type: string
default: "main"
required: False
Copy link
Contributor

@aelovikov-intel aelovikov-intel Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can we have more opaque parameters like devops_ref and tests_ref so that all the actions would use the same name at the high-level entry point? I haven't unified them as much as I'd wish, but I'd really like converging the interfaces rather than pulling them more apart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It this what you mean? bac35ea

KornevNikita
KornevNikita commented Feb 20, 2025
View reviewed changes
devops/actions/run-tests/cts/action.yml
@@ -21,8 +25,7 @@ runs:
with:
path: khronos_sycl_cts
repository: 'KhronosGroup/SYCL-CTS'
ref: 'main'
default_branch: 'main'
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 forgot to mention - deleting this as it's not used at all.

aelovikov-intel
aelovikov-intel reviewed Feb 20, 2025
View reviewed changes
.github/workflows/sycl-linux-run-tests.yml Outdated
@@ -314,3 +316,4 @@ jobs:
sycl_cts_artifact: ${{ inputs.sycl_cts_artifact }}
target_devices: ${{ inputs.target_devices }}
retention-days: ${{ inputs.retention-days }}
tests_ref: ${{ inputs.tests_ref }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs more alignment with e2e still. Something has to be done with line 302. I think the devops/actions/run-tests/* should accept single ref input (as in line 302). And the job of the .yml file is to translate tests_ref/ref/etc. into such an argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, right, likely we don't need a new parameter, we can just use ref for this purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I get it right? 28c7483

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer

ref:  #maybe rename to llvm_ref/repo_ref/toolchain_ref or something
tests_ref:
  required: False  # no default
  type: string

jobs:
...
   e2e:
     ref: ${{ tests_ref || ref || "main" }}

   cts:
     ref: ${{ tests_ref || "main" }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

83da408
Let's rename ref to repo_ref in a follow-up patch as it requires changes for all workflows that use sycl-linux-run-tests.yml.

I can't really imagine a good case when we need different branch for the compiler build and e2e tests, and ref is required: True, therefore "main" is never used. So I left this string as is. I can change it if you insist.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with delayed renaming, but e2e branch has to check for tests_ref first, even if it isn't set anywhere currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 599deaf

@KornevNikita
Copy link
Contributor Author

@intel/llvm-gatekeepers could you please merge?
Failure is unrelated - #17177

@martygrant martygrant merged commit 36f40ed into sycl Feb 26, 2025
27 of 29 checks passed
@martygrant martygrant deleted the cts-fixed-hash branch February 26, 2025 10:16
KornevNikita added a commit to KornevNikita/llvm that referenced this pull request Feb 28, 2025
@KornevNikita
[CI] Update sycl-linux-run-tests.yml
ec5c919 
As discussed in intel#17074 rename ref to repo_ref.
@KornevNikita KornevNikita mentioned this pull request Mar 3, 2025
Merged
uditagarwal97 pushed a commit that referenced this pull request Mar 5, 2025
@KornevNikita
[CI][NFC] Renamed ref to repo_ref in sycl-linux-run-tests.yml (#1…
8eb3bb0 
…7277)

As discussed in #17074, renamed ref to
repo_ref.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uditagarwal97 uditagarwal97 uditagarwal97 approved these changes

@aelovikov-intel aelovikov-intel aelovikov-intel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KornevNikita @uditagarwal97 @aelovikov-intel @martygrant