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.

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

ENH: Fix caching-related warnings in GHA build-test-publish CI #475

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ENH: Fix caching-related warnings in GHA build-test-publish CI #475

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions .github/workflows/build-test-publish.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ jobs:
- uses: actions/checkout@v4
- uses: actions/cache@v4
with:
path: /var/lib/apt
path: ${{ runner.temp }}/cache-linux
Copy link
Member

Choose a reason for hiding this comment

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

Warning: Path Validation Error: Path(s) specified in the action for caching do(es) not exist, hence no cache is being saved.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just drop the APT cache, since it hasn't worked, and it would only save us up to 15s in 8m jobs.

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'm fine if we want to drop this; I do not have a strong opinion. Let me know if you firmly believe we should go ahead and remove it, or whether you want other maintainers to weigh in.

Copy link
Member

Choose a reason for hiding this comment

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

It's clearly not serving its purpose, so it's safe to remove. If you or @oesteban want to take another stab at reducing this time, you're welcome to, but if you're just motivated to reduce warnings, the simple path forward is remove the cache.

key: apt-cache-v3
restore-keys: |
apt-cache-v3
Expand All @@ -102,13 +102,16 @@ jobs:
libglu1-mesa-dev libglw1-mesa \
libxm4 build-essential

- uses: actions/cache@v4
- name: Restore cache for AFNI
id: cache-afni
uses: actions/cache@v4
with:
path: /opt/afni
key: afni-v1
key: afni-v1-${{ matrix.python-version }}-${{ matrix.dependencies }}-${{ matrix.marks }}
restore-keys: |
afni-v1
afni-v1-
- name: Install AFNI
if: steps.cache-afni.outputs.cache-hit != 'true'
Comment on lines +105 to +114
Copy link
Member

Choose a reason for hiding this comment

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

We set up this cache to not depend on the specific job, as its contents should not vary. Not saving multiple caches is a feature, not a bug.

Copy link
Contributor Author

@jhlegarreta jhlegarreta Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My memory is vague now, but looking at the commit message maybe GHA was showing some warning about name clashes (e.g. if two processes were trying to write to the same file)?

Copy link
Member

Choose a reason for hiding this comment

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

When there's a cache miss, each job will populate the directory, and the first to finish will successfully save the cache. That's fine and expected. The alternative would be to create a separate job to ensure the cache exists, and insert it into the workflow ahead of these.

run: |
if [[ ! -d "${AFNI_HOME}" ]]; then
curl -O https://afni.nimh.nih.gov/pub/dist/bin/misc/@update.afni.binaries && \
Expand Down
Loading