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] Prebuild E2E tests as part of build workflow #16682

Merged
merged 1 commit into from
Jan 24, 2025
Merged

Conversation

aelovikov-intel
Copy link
Contributor

@aelovikov-intel aelovikov-intel commented Jan 18, 2025

This will allow us to save time on runner setup and toolchain download/unpacking.

Comment on lines +262 to +277
- name: Source OneAPI TBB vars.sh
shell: bash
run: |
# https://github.com/actions/runner/issues/1964 prevents us from using
# the ENTRYPOINT in the image.
env | sort > env_before
if [ -e /runtimes/oneapi-tbb/env/vars.sh ]; then
source /runtimes/oneapi-tbb/env/vars.sh;
elif [ -e /opt/runtimes/oneapi-tbb/env/vars.sh ]; then
source /opt/runtimes/oneapi-tbb/env/vars.sh;
else
echo "no TBB vars in /opt/runtimes or /runtimes";
fi
env | sort > env_after
comm -13 env_before env_after >> $GITHUB_ENV
rm env_before env_after
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean that whenever we call .github/workflows/sycl-linux-build.yml, we also build E2E tests? IIRC, we don't use pre-built test binaries in post-commit and nightly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we do, and no, we don't. That said I imagine we can start doing so relatively soon. Alternatively, we can make this addition optional, but I thought that build-only mode of e2e tests isn't that different from check-sycl that we do unconditionally everywhere (well, if sycl is modified in pre-commit). Writing this, I can even imagine re-unification of sycl/test and sycl/test-e2e now!

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 personally be of the opinion that we make this addition optional - don't build E2E tests by default when sycl-linux-build.yml is called, unless the caller explicitly asks for it.
Once the "split-test" mode is mature enough that we use it for all workflows and other runners as well, we can build E2E tests by default, just like sycl/test.

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 think it will just complicate implementation with very little benefit... Nightly/post traffic is several times smaller than pre-commit (because PR usually take several iterations). Also, unlike pre-commit, nightly/post don't skip any check-* at build stage, so having e2e built there takes less percentage of the overall duration.

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree with udit if the implementation cost is low, buf it's high as andrei says im fine with enabling it always

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will just complicate implementation with very little benefit...

If it complicates the implementation, I'm fine with the current PR.

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. I'll plan to work on it the coming days/weeks. Either make sure the e2e artifacts get used more or make optional, don't believe we need to delay progress by making extra changes and extra testing. Too easy to make a typo and too long to run the full CI cycle. Very much would like to merge while it's green :)

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.

lgtm just minor kinda unrelated questions

@@ -47,7 +47,7 @@ jobs:
build_artifact_suffix: "default"
build_cache_suffix: "default"
# Docker image has last nightly pre-installed and added to the PATH
build_image: "ghcr.io/intel/llvm/sycl_ubuntu2204_nightly:build"
build_image: "ghcr.io/intel/llvm/sycl_ubuntu2404_nightly:latest"
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just delete the build image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed offline, we only create *nightly:latest now, the :build is getting increasingly stale/outdated.

e2e_testing_mode: build-only
target_devices: all
artifact_suffix: default
cxx_compiler: $GITHUB_WORKSPACE/toolchain/bin/clang++
Copy link
Contributor

@sarnex sarnex Jan 24, 2025

Choose a reason for hiding this comment

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

if we dont pass this, it falls back to which clang++, so i assume the built clang++ isn't on the path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed offline, we need to make sure CMake sees the same CXX compiler at build/run stages.

@aelovikov-intel aelovikov-intel merged commit 5f69e3e into sycl Jan 24, 2025
25 checks passed
@aelovikov-intel aelovikov-intel deleted the e2e-build branch January 24, 2025 17:03
aelovikov-intel added a commit that referenced this pull request Jan 24, 2025
aelovikov-intel added a commit that referenced this pull request Jan 24, 2025
aelovikov-intel added a commit that referenced this pull request Jan 24, 2025
Reduce number of input parameters and make the logic a bit
cleaner (IMO). This PR also uses that updated logic to make building E2E
tests optional in `sycl-linux-build.yml` and makes enabled in pre-commit
only for now, effectively fixing the regression in Nightly CI introduced
in #16682.
aelovikov-intel added a commit that referenced this pull request Jan 25, 2025
Reduce number of input parameters and make the logic a bit
cleaner (IMO). This PR also uses that updated logic to make building E2E
tests optional in `sycl-linux-build.yml` and makes enabled in pre-commit
only for now, effectively fixing the regression in Nightly CI introduced
in #16682.
aelovikov-intel added a commit that referenced this pull request Jan 25, 2025
Reduce number of input parameters and make the logic a bit
cleaner (IMO). This PR also uses that updated logic to make building E2E
tests optional in `sycl-linux-build.yml` and makes enabled in pre-commit
only for now, effectively fixing the regression in Nightly CI introduced
in #16682.
aelovikov-intel added a commit that referenced this pull request Jan 25, 2025
Reduce number of input parameters and make the logic a bit
cleaner (IMO). This PR also uses that updated logic to make building E2E
tests optional in `sycl-linux-build.yml` and makes enabled in pre-commit
only for now, effectively fixing the regression in Nightly CI introduced
in #16682.
aelovikov-intel added a commit that referenced this pull request Jan 27, 2025
)

Reduce number of input parameters and make the logic a bit cleaner
(IMO). This PR also uses that updated logic to make building E2E tests
optional in `sycl-linux-build.yml` and makes enabled in pre-commit only
for now, effectively fixing the regression in Nightly CI introduced in
#16682.
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.

3 participants