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

Build acl cache sequentially #2388

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

Ryo-not-rio
Copy link
Contributor

When using github actions caches with the matrix strategy, the cache-hit variable is shared amongst all the strategies, potentially causing a race condition when run in parallel. This work creates just the caches sequentially, avoiding this problem

@Ryo-not-rio Ryo-not-rio force-pushed the fix-cache-up branch 7 times, most recently from 76f8c25 to 6b22724 Compare January 16, 2025 17:56
@Ryo-not-rio Ryo-not-rio marked this pull request as ready for review January 17, 2025 10:38
@Ryo-not-rio Ryo-not-rio requested a review from a team as a code owner January 17, 2025 10:38
cmake --build $ACL_ROOT_DIR/build
cd $ACL_ROOT_DIR
set -x
scons $MP Werror=0 debug=$ACL_DEBUG neon=1 opencl=0 embed_kernels=0 \
Copy link
Contributor

Choose a reason for hiding this comment

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

Did CMake not work at all? It is surprising that it was building fine before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, wasn't working for gcc or macOS, I imagine you hadn't changed ACL versions after switching to cmake so we were always reusing the cached build

@theComputeKid
Copy link
Contributor

Looks good. Just need to squash commits.

pull_request:
types: [opened, synchronize, reopened]
paths:
- '.github/automation/ci-aarch64.json'
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this work correctly when we update ACL in a PR and the ci-aarch64.yml runs at the same time as the aarch64-acl.yml? Should ci-aarch64.yml not depend on aarch64-acl.yml finishing first?

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've changed this workflow to a reusable workflow so this problem should be solved

Copy link
Contributor

@theComputeKid theComputeKid left a comment

Choose a reason for hiding this comment

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

It is unfortunate that we have to build ACL sequentially so that we can cache it properly. This is a limitation of GitHub's API. But thanks for spotting it and fixing it.

Copy link
Contributor

@Radu2k Radu2k left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@theComputeKid theComputeKid merged commit d702ab2 into uxlfoundation:main Jan 23, 2025
18 of 19 checks passed
Ryo-not-rio added a commit to Ryo-not-rio/oneDNN that referenced this pull request Jan 27, 2025
The aarch64 nightly ci is failing due to a recent change in
uxlfoundation#2388 where the
ACL caching mechanism was changed. This updates
the nightly pipeline to use the new caching system
theComputeKid pushed a commit that referenced this pull request Jan 28, 2025
The aarch64 nightly ci is failing due to a recent change in
#2388 where the
ACL caching mechanism was changed. This updates
the nightly pipeline to use the new caching system
@vpirogov vpirogov added the devops Github automation label Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops Github automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants