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

Add support for adding to cache key #41

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

Conversation

iainlane
Copy link

@iainlane iainlane commented Jul 12, 2024

When using this action in multiple matrix jobs in the same workflow, the generated cache key is the same for all of them, because they all get the same job ID. This means that all apart from the first job are unable to save the cache, and subsequent runs might restore the wrong cache.

The Swatinem/rust-cache action which we use for caching has a key input which it puts in its cache key. (It doesn't override the key, just adds to it.) Providing this as an input here will allow us to generate a unique cache key for each job in the matrix.

@iainlane iainlane force-pushed the iainlane/propagate-cache-key branch from 9ed8cf5 to d4e5302 Compare July 12, 2024 07:31
iainlane added a commit to iainlane/rssfilter that referenced this pull request Jul 12, 2024
We run the build for amd64 and arm64, and we use `setup-rust-toolchain` to
install and cache the Rust toolchain and project dependencies.

The caching part of that action doesn't currently work if it's run in multiple
jobs from the same workflow run. The cache key doesn't contain anything that
would disambiguate and there is no input to `setup-rust-toolchain` to allow it
to be customised.

To fix that, I've just [submitted a PR][PR] to the action to allow the cache key
to be customised. This will allow us to set a key that includes the OS and
architecture of the build, so that the caches are kept separate. We're making
use of that here.

[PR]: actions-rust-lang/setup-rust-toolchain#41
iainlane added a commit to iainlane/rssfilter that referenced this pull request Jul 12, 2024
We run the build for amd64 and arm64, and we use `setup-rust-toolchain` to
install and cache the Rust toolchain and project dependencies.

The caching part of that action doesn't currently work if it's run in multiple
jobs from the same workflow run. The cache key doesn't contain anything that
would disambiguate and there is no input to `setup-rust-toolchain` to allow it
to be customised.

To fix that, I've just [submitted a PR][PR] to the action to allow the cache key
to be customised. This will allow us to set a key that includes the OS and
architecture of the build, so that the caches are kept separate. We're making
use of that here.

[PR]: actions-rust-lang/setup-rust-toolchain#41
@jonasbb
Copy link
Member

jonasbb commented Jul 12, 2024

I am really wary of just adding too many cache-specific arguments to this action. I rather have the cache part simple and have people use Swatinem/rust-cache directly when they need all the bells and whistles. Messing with the cache key to me already goes into the more advanced territory.
For this change, I would like to check the reasoning. This is the description of the key argument of the rust-cache action:

    # An additional cache key that is added alongside the automatic `job`-based
    # cache key and can be used to further differentiate jobs.
    # default: empty
    key: ""

According to this, each job already uses a separate cache key. From the code, it looks like they include the GITHUB_JOB environment variable. So no effort should be necessary "to generate a unique cache key for each job."

action.yml Outdated
@@ -32,6 +32,9 @@ inputs:
description: "Also cache on workflow failures"
default: "true"
required: false
key:
Copy link
Member

Choose a reason for hiding this comment

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

This should be cache-key instead of key to make it clear that this is a cache related setting.

Copy link
Author

Choose a reason for hiding this comment

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

Cheers, fixed.

@iainlane
Copy link
Author

iainlane commented Jul 13, 2024

I would like to check the reasoning. This is the description of the key argument of the rust-cache action:

    # An additional cache key that is added alongside the automatic `job`-based
    # cache key and can be used to further differentiate jobs.
    # default: empty
    key: ""

According to this, each job already uses a separate cache key. From the code, it looks like they include the GITHUB_JOB environment variable. So no effort should be necessary "to generate a unique cache key for each job."

Thank you for the feedback.

It's made me understand that I could be more explicit in the description/commit message. I've edited those now. Let me know if you still consider this too advanced and we can close the pull request.

The problem occurs when using matrices. In that case, the job_id is the same. I've constructed an example in which one matrix job saves a cache which is then restored by the other matrix job.

In real usage, it's likely you'd be performing a build after calling this action, so the race would not be that matrix jobs can restore the wrong cache in the same run, but that they both try to save the cache at the end and one of them fails.

When using this action in multiple matrix jobs in the same workflow, the
generated cache key is the same for all of them, because they all get
the same job ID. This means that all apart from the first job are unable
to save the cache, and subsequent runs might restore the wrong cache.

The `Swatinem/rust-cache` action which we use for caching has a `key`
input which it puts in its cache key. (It doesn't override the key, just
adds to it.) Providing this as an input here will allow us to generate a
unique cache key for each job in the matrix.
@iainlane iainlane force-pushed the iainlane/propagate-cache-key branch from d4e5302 to b01657d Compare July 13, 2024 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants