-
Notifications
You must be signed in to change notification settings - Fork 795
[CI][SYCL] Enable E2E testing for New Offloading Model #20634
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.
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
base: sycl
Are you sure you want to change the base?
Conversation
49a3481 to
b8a7ff6
Compare
|
Hi @sarnex Could you please help with reviewing this change or recommend somebody for that? |
|
I can review, thanks |
| binaries_artifact: e2e_bin_preview | ||
| - name: New Offload Model | ||
| runner: '[Linux, "gen12"]' | ||
| target_devices: opencl:cpu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High level question, do we think changes that break the new offload model but nothing else are common enough to justify precommit testing? Last I heard we were only going to do nightly testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also considering that aspect. To being able to check E2E tests for changes related to new model we need either precommit functionality or some workflow other that sycl-nightly that we would run manually every time in PR. Perhaps, we could introduce some PR label "new-offload-model" that could enable this branch in precommit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to have it in pre or postcommit if you guys ( @YuriPlyakhin / @intel/dpcpp-tools-reviewers ) think there is a decent likelihood that some PR passes all other CI but fails on the new offload model. If we think that is pretty rare I would recommend we do nightly-only testing, if you want to manually test new offload stuff in a PR you can run the nightly explicitly though the github UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you decide to go with the label, then
llvm/.github/workflows/sycl-linux-precommit.yml
Lines 239 to 248 in 26e4c60
| test-perf: | |
| needs: [build, detect_changes] | |
| permissions: | |
| contents: write | |
| packages: read | |
| if: | | |
| !cancelled() | |
| && needs.build.outputs.build_conclusion == 'success' | |
| && (contains(github.event.pull_request.labels.*.name, 'run-perf-tests') | |
| || contains(needs.detect_changes.outputs.filters, 'perf-tests')) |
might be useful to look at. I don't remember why I decided to keep that as a separate job instead of extending regular matrix though.
| if: ${{ !cancelled() && needs.build-win.outputs.build_conclusion == 'success' }} | ||
| strategy: | ||
| fail-fast: false | ||
| uses: ./.github/workflows/sycl-windows-run-tests.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
permissions:
contents: write
packages: read
should work hopefully
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| permissions: read-all |
For some reason, sycl-windows-run-tests.yml requires many permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we just didn't restrict the permissions on that one, did you try restricting them and it failed?
|
Also you might want to rebase this PR against HEAD if you're going to run the nightly, I merged a fix today that should fix the AOT failures, they're unrelated to this PR. |
This patch has the following changes:
--param enable_new_offload_modelis added to SYCL E2E LIT test configuration