Skip to content
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

[CI] Fix missing deps in dev igc installation #13610

Merged
merged 10 commits into from
May 3, 2024
Merged

[CI] Fix missing deps in dev igc installation #13610

merged 10 commits into from
May 3, 2024

Conversation

jsji
Copy link
Contributor

@jsji jsji commented Apr 30, 2024

All opencl tests with current dev igc installtation are failing.
Investigation shows that we are missing libopencl-clang.
Unfortunately dev igc deb package did not include libopencl-clang,
and Ubuntu did not have the correct version either, apt has up to
libopencl-clang13, while we need libopenc-clang14.

So the workaround is to backup the version installed by released igc
version, then install it back after installing dev igc.

This also seperate the installation of dev igc to new step,
so that we can always do checksum and remove force dependency option to dpkg.

All opencl tests with current dev igc installtation are failing.
Investigation shows that we are missing libopencl-clang.
Unfortunately dev igc deb package did not include libopencl-clang,
and Ubuntu did not have the correct version either, apt has up to
libopencl-clang13, while we need libopenc-clang14.

So the workaround is to backup the version installed by released igc
version, then install it back after installing dev igc.

This also seperate the installation of dev igc to new step,
so that we can always do checksum and remove force dependency option to dpkg.
@jsji
Copy link
Contributor Author

jsji commented May 1, 2024

Changes to .github/workflows/sycl-linux-precommit.yml does not take effect immediately, split it into #13614, will rebase and retest after #13614 is merged.

@jsji
Copy link
Contributor Author

jsji commented May 1, 2024

Changes to .github/workflows/sycl-linux-precommit.yml does not take effect immediately, split it into #13614, will rebase and retest after #13614 is merged.

It can take effect actually, so cancel #13614, fix it in this PR.

@jsji
Copy link
Contributor Author

jsji commented May 1, 2024

@jsji jsji marked this pull request as ready for review May 1, 2024 03:28
@jsji jsji requested a review from a team as a code owner May 1, 2024 03:28
@jsji jsji self-assigned this May 1, 2024
Comment on lines 130 to 131
# Dev IGC deb package did not include libopencl-clang
# So we need to backup and install it from release igc.
Copy link
Contributor

Choose a reason for hiding this comment

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

@bashbaug , do you know who can fix that on the IGC side?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest starting with @pszymich.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @aelovikov-intel and @bashbaug ! I had a quick look at https://github.com/intel/intel-graphics-compiler/blob/b077b43fa6193c97bcd2f3959f6d11e185d6fef3/.github/workflows/build-IGC.yml#L39C1-L41C56 , looks like IGC is also doing the hacky install/copy/package due to the fact that there is no libopencl-clang deb available.

I have contacted the libopencl-clang team to see whether we can build/release packaged libopencl-clang deb file in https://github.com/intel/opencl-clang/releases too. If that is fixed, then we should be able to remove this workaround, and also the workaround in IGC CI.

@@ -60,7 +60,9 @@ jobs:
- name: Determine Arc tests
id: arc_tests
run: |
if [ "${{ contains(needs.detect_changes.outputs.filters, 'drivers') }}" == "true" ]; then
if [ "${{ contains(needs.detect_changes.outputs.filters, 'devigccfg') }}" == "true" ]; then
echo 'arc_tests="Matrix/"' >> "$GITHUB_OUTPUT"
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 think we need to run all tests here as well.

Copy link
Contributor Author

@jsji jsji May 1, 2024

Choose a reason for hiding this comment

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

There are existing failures for non joint matrix tests.
eg: https://github.com/intel/llvm/actions/runs/8903576097/job/24452097484

We will only use igc dev driver for joint matrix for now, so we don't really need to run all e2e test,
only make sure joint matrix tests are clean is enough.

Running all tests would be an ideal solution, but to be honest, I don't think we have enough resource to triage and response to failures, especially in non joint matrix ones, so running all tests might effectively stop us from updating igc dev driver for joint matrix tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about ESIMD team's tests? + @sarnex

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH our tests take a long time to run. What is the use case of using the dev IGC driver? If I know that I can suggest if we need to run ESIMD tests with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dev IGC driver will be used in joint matrix pre-commit CI tests so that we can test some features that depends on newer IGC and get integrated test feedback earlier than before.

I would personally recommend that we also enable the pre-commit CI for ESIMD, because ESIMD features tend to depends on corresponding IGC changes as well. But it is up to your team to decide.

The cost of enabling ESIMD test here is that we may need to response to potential failures in CI , it would be great if the failures are real regressions that we need to resolve, but there will definitely be noise due to IGC changes etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks and yeah I agree ESIMD is very sensitive to IGC versions as well. I think we should enable dev IGC driver testing in ESIMD.

@v-klochkov FYI

devops/scripts/install_drivers.sh Show resolved Hide resolved
Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

esimd part lgtm

@jsji jsji requested a review from a team as a code owner May 2, 2024 21:39
@jsji jsji temporarily deployed to WindowsCILock May 2, 2024 22:20 — with GitHub Actions Inactive
@jsji jsji temporarily deployed to WindowsCILock May 2, 2024 22:31 — with GitHub Actions Inactive
@jsji jsji requested a review from sarnex May 3, 2024 01:13
@jsji
Copy link
Contributor Author

jsji commented May 3, 2024

@sarnex Would you please have a look at the workaround in sycl/test-e2e/ESIMD/hardware_dispatch.cpp .
The test is failing in with dev igc (https://github.com/intel/llvm/actions/runs/8926214421/job/24517873892?pr=13610) , with error error: Unsupported required sub group size. It is failing due to the auto-detection failures.

I had a detail look, it should be due to some tricky CI env exposed some unknown spirv tools bug -- I can reproduce it using spv, I built the igc using exact same steps and buildIGC.sh script within same docker as used in igc repo, it is passing. I also build it manually, it is also passing. Given it is passing in personal build, I think it is hard to get it fixed in CI. So I would propose we add the workaround to by pass the auto detection logic -- that is not our test point either.

This can help us set up a baseline for the pre-commit CI for dev igc. Thanks.

@sarnex
Copy link
Contributor

sarnex commented May 3, 2024

@jsji Workaround seems fine to me.

@jsji
Copy link
Contributor Author

jsji commented May 3, 2024

@intel/llvm-gatekeepers Can someone help merge this? Thanks.

@sarnex sarnex merged commit 83adbdf into sycl May 3, 2024
17 checks passed
@bader bader deleted the fixdevigcinstall branch May 14, 2024 15:55
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.

4 participants