Add option to reevaluate key during save #989

mattjohnsonpint · 2022-11-19T01:04:34Z

Description

In save.ts there's a comment that describes that because inputs are reevaluated before the post action, the cache key is retrieved from state instead of input:

cache/src/save.ts

Lines 29 to 30 in 5c79b3f

    
           // Inputs are re-evaluted before the post action, so we want the original key used for restore 
        
           const primaryKey = core.getState(State.CachePrimaryKey);

In some cases, it is actually quite desirable to have the input re-evaluated and a different cache key used for save than was used during restore.

This PR adds an option reevaluate-key which when set true will re-evaluate the key input in the post action instead of retrieving it from state.

The default is false, retaining the existing behavior.

Motivation and Context

I'm primarily a .NET developer, and I often use this cache action to cache NuGet packages for my build pipelines. When doing so, I have to create a cache key from some reasonable source of the packages in my repository. If I've opted in to using lock files, then I can use the packages.lock.json files that are created when the restore phase of my build executes. Indeed, this is what the examples file shows. But there are downsides to using lock files, and thus they are off by default in .NET.

Without lock files, one can base the hash on project files (.csproj, .props, etc.), but there are many reasons that project files might change - not just package changes. Also that technique doesn't take into account any floating versions that might be defined in the projects, such as <PackageReference Include="SomePackage" Version="2.*" />. There could be a new version of the package without any change to the file, and thus the cache would never get updated.

Since lock files are still the best source of information about which packages are installed, the easiest thing to do is create them during restore with dotnet restore --use-lock-file. The problem is that the cache restore has to run first.

So, by having an option to re-evaluate the key, we can save a cache based on the hash of the generated lock files. If anything ever changes, a new cache gets created automatically and used on subsequent runs.

Without the option, there's no way to base the cache key on the generated lock files. Generating them before the cache restore would restore all packages without using the cache, invalidating the reason to have a cache in the first place.

I'm reasonably certain that this unblocks many other similar scenarios. It's not just for .NET developers. Pretty much any time you need to base the cache key on something that depends on the files being cached, you'll need to re-evaluate the key.

How Has This Been Tested?

I added unit tests to this PR
I built and deployed a branch on my fork, then used it in a workflow of a test project on another repo to validate its behavior.
I also tested without the option set, to ensure that the default behavior does not change.

Screenshots:

First Run (nothing in the cache):

Cache created by first run:

Second Run (has cache hit):

Workflow for above tests is as follows (using my fork):

name: Build

on:
  workflow_dispatch:
  push:
    branches: [ main ]

jobs:

  build:
    runs-on: ubuntu-latest
    steps:

      - name: Checkout
        uses: actions/checkout@v3

      - name: Cache
        uses: mattjohnsonpint/gha-cache@dist
        with:
          path: ~/.nuget/packages
          key: nuget-${{ runner.os }}-${{ hashFiles('**/packages.lock.json') }}
          restore-keys: nuget-${{ runner.os }}
          reevaluate-key: true

      - name: Restore .NET Dependencies
        run: dotnet restore --use-lock-file --nologo

      - name: Build
        run: dotnet build -c Release --no-restore --nologo

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (add or update README or docs)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mattjohnsonpint · 2022-11-23T17:16:49Z

@vsvipul - I have updated to resolve the issues. Will you please re-run the workflows so I can see if there's anything else I that needs to be done?

Is there anything else you need from me to merge this? Thanks.

mattjohnsonpint · 2022-11-25T07:45:11Z

Does your process also require I build and commit the dist folder? That would seem strange to me, and it's not described in the contributing docs. Please advise. Thanks.

mattjohnsonpint · 2022-11-26T17:41:23Z

Seeing that other PRs have indeed been including the dist folder, I have done the same. But really this should be added to the contributing docs, as most other projects would expect you only commit source files to a PR. Thanks.

vsvipul · 2022-11-28T08:01:52Z

Thanks @mattjohnsonpint for contributing. Yes dist/ changes are indeed required for tests to pass.
I understand what change you are proposing but have some questions -

If the lockfile is changing , it would normally be committed to the repository and not remain unchanged. Why would people use stale lockfiles in their repos?
We are also trying to give users the ability to decide where in the workflow they want to run restore and/or save separately. Would that help solve this problem?

mattjohnsonpint · 2022-11-28T08:56:32Z

If the lockfile is changing , it would normally be committed to the repository and not remain unchanged. Why would people use stale lockfiles in their repos?

In .NET, lock files are not normally used. The tooling does not create them unless specifically asked to do so. Thus by default, there's no complete list of which packages and versions are required to restore a project - which means no good source of information to create a cache key. One way to get that information is to create a lock file on the fly during the restore phase of a CI build. It's never committed to the repository, so it's never stale. It's not actually used to lock anything, but rather as the basis for the cache key. Since the cache-restore doesn't require an exact match on the key, it can easily restore the last cache that was created regardless of the full key. Then by creating the lock file and using it to compute the cache key, a save only occurs if the key has changed - because the lock file has changed in some way.

One important use case is when floating versions are used. For example, I use floating versions in unit test projects for the libraries I develop, such that my library is always being tested against the latest version of any dependencies.

Sure, I could instead switch to using lock files and commit them. But then I would have to also have a process to separately update the lock files any time one of my dependencies had an update. On larger projects, this can lead to a disproportionately large number of pull requests that just bump versions of dependencies for no other purpose than testing. Tooling like Dependabot can help automate that, but it still leads to a lot of maintenance overhead. I prefer just using a floating version in my test projects, and having a test workflow fail when a newer version of a dependency breaks something.

We are also trying to give users the ability to decide where in the workflow they want to run restore and/or save separately. Would that help solve this problem?

Perhaps. I would need two separate actions - one to restore the cache and a separate one to save it. I'd also need to have the ability to get the key that was restored (as an output), such that I could conditionally compare it to the hash of the generated lockfile to decide whether or not to save. Otherwise I'd end up saving every run, overwriting a cache that already exists with the exact same content.

I'm not against the idea, but it's a lot simpler with just asking for the key to be re-evaluated. By doing so, the save phase already recognizes it as a cache hit and doesn't attempt to save again (despite it actually being a cache miss in the restore phase).

bishal-pdMSFT · 2022-11-28T19:13:24Z

I'd also need to have the ability to get the key that was restored (as an output), such that I could conditionally compare it to the hash of the generated lockfile to decide whether or not to save. Otherwise I'd end up saving every run, overwriting a cache that already exists with the exact same content.

If I am not wrong, you would still need to get the restored key as output and compare it - so as to decide whether to set reevaluate-key to true or not.

I'm not against the idea, but it's a lot simpler with just asking for the key to be re-evaluated. By doing so, the save phase already recognizes it as a cache hit and doesn't attempt to save again (despite it actually being a cache miss in the restore phase).

While it will be simpler in this particular case, there are many asks to add more such knobs to the action e.g. force save cache even if job fails, or save cache even if a cache hit occurred earlier etc etc. Individually each ask is a small one and good to do, but in sum total it becomes one too many knobs bringing down usability and actually making it more opaque and prone to misconfiguration.
That's why a more transparent (though a bit more verbose) approach is to have two separate actions and use them in combination by leveraging the conditionals which workflow schema offers.

mattjohnsonpint · 2022-11-28T20:02:58Z

In the reevaluate-key approach, I don't need to gather output or do any comparisons. It just works.

I do understand your reasonings with wanting the separate save/restore steps, and I welcome that instead of this if you are heading that direction anyway. Though as I mentioned, I will need the restored key as an output with that approach. Currently the only output is cache-hit. If restored-key was also an output, and I had the ability to separate save from restore, then I wouldn't need reevaluate-key to accomplish this.

kotewar · 2023-01-03T06:21:52Z

Hey @mattjohnsonpint 👋🏽

After considering multiple asks from our customers and this use case, we have published two new actions restore and save with restore action outputting the restore key as cache-matched-key.

Also the save action can re-evaluate the key in realtime when the step executes or reuse the primary key passed as input in the restore action using one of the outputs.

Hope it solves the use case. :)

More details about the new actions here - #1028
Beta release feedback from other users - #1020

Closing this now, feel free to reopen if need be, thanks!

mattjohnsonpint added 2 commits November 18, 2022 16:34

Add option reevaluate the key

fa2f415

Add use case to workarounds doc

ebd6587

mattjohnsonpint requested a review from a team as a code owner November 19, 2022 01:04

github-actions bot requested a review from vsvipul November 19, 2022 01:04

mattjohnsonpint added 4 commits November 18, 2022 17:16

Update README.md

5dbe0df

Prettify to fix check

1e55496

Merge branch 'main' into reevaluate-key

bdfce95

fix typo

1bd6a22

build and commit dist

1001c07

vsvipul self-assigned this Nov 28, 2022

Slava848 mentioned this pull request Nov 28, 2022

Does your process also require I build and commit the dist folder? That would seem strange to me, and it's not described in the contributing docs. Please advise. Thanks. jaakkopasanen/AutoEq#567

Closed

vsvipul added the area:granular-control Issues related to granular control in saving or restoring cache label Dec 7, 2022

vsvipul requested review from kotewar and removed request for vsvipul December 7, 2022 09:22

vsvipul assigned kotewar and unassigned vsvipul Dec 7, 2022

kotewar closed this Jan 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add option to reevaluate key during save #989

Add option to reevaluate key during save #989

Uh oh!

mattjohnsonpint commented Nov 19, 2022 •

edited

Loading

Uh oh!

mattjohnsonpint commented Nov 23, 2022

Uh oh!

mattjohnsonpint commented Nov 25, 2022

Uh oh!

mattjohnsonpint commented Nov 26, 2022 •

edited

Loading

Uh oh!

vsvipul commented Nov 28, 2022

Uh oh!

mattjohnsonpint commented Nov 28, 2022 •

edited

Loading

Uh oh!

bishal-pdMSFT commented Nov 28, 2022

Uh oh!

mattjohnsonpint commented Nov 28, 2022 •

edited

Loading

Uh oh!

kotewar commented Jan 3, 2023

Uh oh!

Uh oh!

	// Inputs are re-evaluted before the post action, so we want the original key used for restore
	const primaryKey = core.getState(State.CachePrimaryKey);

Add option to reevaluate key during save #989

Add option to reevaluate key during save #989

Uh oh!

Conversation

mattjohnsonpint commented Nov 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Screenshots:

Types of changes

Checklist:

Uh oh!

mattjohnsonpint commented Nov 23, 2022

Uh oh!

mattjohnsonpint commented Nov 25, 2022

Uh oh!

mattjohnsonpint commented Nov 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsvipul commented Nov 28, 2022

Uh oh!

mattjohnsonpint commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bishal-pdMSFT commented Nov 28, 2022

Uh oh!

mattjohnsonpint commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotewar commented Jan 3, 2023

Uh oh!

Uh oh!

mattjohnsonpint commented Nov 19, 2022 •

edited

Loading

mattjohnsonpint commented Nov 26, 2022 •

edited

Loading

mattjohnsonpint commented Nov 28, 2022 •

edited

Loading

mattjohnsonpint commented Nov 28, 2022 •

edited

Loading