Add lookup-only option

[cdce8p]

Description

Add an advanced option to skip downloading the cache entry.

Note
Requires actions/toolkit#1286

This PR currently links a custom version of actions/toolkit with the PR changes to be able to test it.

Motivation and Context

Fixes #901
Fixes #831
#1020 (comment)
#1020 (comment)

If dependency install / cache setup and the actual tests are split across multiple jobs, it's often unnecessary to restore the cache if an exact cache-hit occurred. Similarly, if the cache is created from scratch, it's only necessary to know if a cache entry already exists.

Using lookup-only in these cases saves time and resources, especially for larger caches which don't need to be restored.

How Has This Been Tested?

Added test case and run a test workflow using the feature branch.

Screenshots (if appropriate):

--

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.

[kotewar]

Did you try running a sample workflow of this version of action with cache action?
As cache action doesn't have this input and only restore action has, I'm curious if it throws any warning with this implementation?

[cdce8p]

Not sure I understand your question correctly.

The dry-run input is defined for both the normal cache and also the cache/restore action with default: 'false'.

I'm curious if it throws any warning with this implementation?

Even if it wouldn't be defined, the code shouldn't throw an error. utils.getInputAsBool uses core.getInput under the hood which returns an empty string for undefined variables. The result would thus still be false.

cache/src/utils/actionUtils.ts

Lines 55 to 60 in 22cbf49

	export function getInputAsBool(
	name: string,
	options?: core.InputOptions
	): boolean {
	const result = core.getInput(name, options);
	return result.toLowerCase() === "true";

actions/toolkit -> core.ts

[cdce8p]

Pushed a small update for the readmes to reflect the changes in action.yml.

[kotewar]

Got it, I was under assumption that this input is only for the restore action.
Which brings me to my next question, how is the behaviour in case of the cache action?
Usually the cache action saves cache in the post step when cache-hit output is set to false. Wouldn't unnecessary caches be saved when dry-run:true and no cache is found?

[cdce8p]

Which brings me to my next question, how is the behaviour in case of the cache action?
Usually the cache action saves cache in the post step when cache-hit output is set to false. Wouldn't unnecessary caches be saved when dry-run:true and no cache is found?

I don't think so. The main use case I have in mind for it would depend on saving the cache afterwards. dry-run: 'true' is meant to speed up the case where the cache would be created from scratch anyway. If no exact cache is found, it still needs to be created and stored after that to be able to use it for the next run.

This works best, if restore-keys is left empty as downloading those would be skipped as well. However, it can still be archived by stacking the cache/restore (with dry-run: 'true') and normal cache (with restore-keys) action. The first would be there to filter out exact matches, while the following steps would only run if cache-hit is false.

[cdce8p]

Can we name it better such that the name suits both actions/cache and actions/cache/restore action?

Sure, dry-run was just the first name that came to mind but I can see how it can be confusing.
What do you think of skip-download?

I also though about restore-dry-run or skip-restore but those don't capture the essence of this option or can be misleading as well. I'm open to other suggestions though.

[kotewar]

skip-download won't make sense for the restore action right? skip-download sounds more like save-only.

restore-dry-run is still close. My suggestion would be to use something like check-key (although not quite sure what the perfect name here could be), @bishal-pdMSFT do suggest us something that you feel would be good here.

[cdce8p]

skip-download won't make sense for the restore action right?

I'd disagree. Skipping the download is what's happing internally, regardless of cache or restore action. It's also the term I used naturally to describe the parameter in the readme. That's why I thought it might be a good fit.

My suggestion would be to use something like check-key

Just thinking about it, I could imagine that users could be confused by that, especially if the default is false. Is the key not checked unless it's set to true?

[kotewar]

skip-download won't make sense for the restore action right?

My reason of saying the same was - downloading is the main job of the restore action and the restore step in the cache action. For cache action, the name skip-download is still fine, but for restore action, if restore (download) itself is not happening then I see a chance of confusion for the users thinking what it would even do if not download the cache itself. 😕 Hence I was tilting more towards something that tell check-key(-but-dont-download)

Few more suggestions from my end...

How about going in the direction of peek like we use in stacks, just see but don't do anything.
Examples:
peek-key / peek-cache
or
lookup / lookup-only
or
spy

Is the key not checked unless it's set to true?

True, it will create a confusion. 😅

[cdce8p]

For cache action, the name skip-download is still fine, but for restore action, if restore (download) itself is not happening then I see a chance of confusion for the users thinking what it would even do if not download the cache itself.

Just a thought, would it be enough to expose the option only for cache? On the one hand it would address the main use case, on the other there is some value in exposing the option for all actions.

Hence I was tilting more towards something that tell check-key(-but-dont-download)

lookup-only sounds like it could fit. It would make sense in the docs as well. Something like

* `lookup-only` - Skip downloading cache. Only check if cache entry exists. Default: false

[kotewar]

Hi @cdce8p, 👋🏽

We had an internal discussion within our team and we felt like this feature might affect the extensibility of the code and feel that having a new action for the same (something like check as suggested by some users) would make more sense from maintenance perspective.

For now we have added the work needed for this new action in our backlog and we will pick this up in the future. In the meantime, if you would want to contribute for the same, you are welcome to do so. :)

Thanks for your contribution, we really appreciate your efforts here. ❤️

We can go ahead with the other PR of yours. I have requested a small test case addition in the same.

[cdce8p]

Hi @kotewar,

thanks for the update. After the other PR was done, I took some time today to think about this one some more.

I can understand your point about maintenance. So I did implement a separate new check action. However while writing it and especially the readme, I noticed something interesting. The outputs provided by the current restore actions are exactly the ones I would have chosen for the check one as well. To the point that the final implementation is fairly similar to the approach here, just not with a separate input but a dedicated action instead. That action just uses the extended restore implementation.

Furthermore, it's worth to consider the main use case again and what from the user perspective might be a good solution.

Use case
The main use is probably in a build job to check if a cache entry exists and, if not, to build and save a new one. This is especially useful if the cache doesn't change much over time while also being quite large >500 MB. Thus the new action would help save the time needed to restore (download + unzip) the cache that wouldn't get used anyway, since tests are in a separate job(-matrix).

This would probably look something like this. Note that it's basically necessary to duplicate all inputs from the initial cache action to cache/check and later cache/save. Additionally, a new cache-hit != 'true' check must be added to make sure we don't try to write to an existing key. AFAICT that's also the workflow proposed by MartijnHols/actions-cache in the Cache build output and skipping build section. I believe that might be where the proposal originally came from.

 build:
   steps:
     - uses: actions/checkout@v3

-    - users: actions/cache@v3
+    - users: actions/cache/check@v3
       id: cache-check
       with:
         path: path/to/dependencies
         key: ${{ runner.os }}-${{ hashFiles('**/lockfiles') }}

     - name: Build
       if: steps.cache-check.outputs.cache-hit != 'true'
       run: /build.sh
 
+    - uses: actions/cache/save@v3
+      if: steps.cache-check.outputs.cache-hit != 'true'
+      with:
+        path: path/to/dependencies
+        key: ${{ runner.os }}-${{ hashFiles('**/lockfiles') }}

 test:
   ...

How would that be different to the input parameter? As the lookup-only input is handled by the cache action directly, there is no need to create an additional step and duplicate inputs.

 build:
   steps:
     - uses: actions/checkout@v3

     - users: actions/cache@v3
       id: cache-check
       with:
+        lookup-only: true
         path: path/to/dependencies
         key: ${{ runner.os }}-${{ hashFiles('**/lockfiles') }}

     - name: Build
       if: steps.cache-check.outputs.cache-hit != 'true'
       run: /build.sh

 test:
   ...

Conclusion
I can fully understand the maintenance argument, tbh I don't know how other user would use it in their workflows. However, just from my perspective I would probably prefer to use just lookup-only: true. It's simpler and less error prone. There could be an argument to doing both actually. The separate check action is certainly more extendable so if other inputs / outputs only for it need to be added, that would be the way to go. Simultaneously, the lookup-only approach is likely the easiest for the most common use case and could be limited to support just that.

To make it easier to compare both, I've rebased this PR as well.

In any case, both approaches depend on some ground work: actions/toolkit#1286

--
I'm interested to hear your opinion on this.

[kotewar]

@cdce8p,
Okay I am in agreement with the last comment. Most use cases I saw wanted to do something when the cache doesn't exist. If they want to save the cache, they can use the cache action and if they want to do something else or not do anything, they can use the restore action with the lookup-only option

Did I understand it correctly?

And talking of the extensibility, if we feel this could be breaking in the future, we might have to remove it and do a major release instead with a different set of inputs.
For that reason, shall we instead add an environment variable like we have for SEGMENT_DOWNLOAD_TIMEOUT_MINS so that we can be sure about the extensibility and then?
I am also discussing this internally with my team, but want to get your views on the same.

[cdce8p]

@cdce8p, Okay I am in agreement with the last comment. Most use cases I saw wanted to do something when the cache doesn't exist. If they want to save the cache, they can use the cache action and if they want to do something else or not do anything, they can use the restore action with the lookup-only option

Did I understand it correctly?

Yes, exactly.

And talking of the extensibility, if we feel this could be breaking in the future, we might have to remove it and do a major release instead with a different set of inputs. For that reason, shall we instead add an environment variable like we have for SEGMENT_DOWNLOAD_TIMEOUT_MINS so that we can be sure about the extensibility and then?

From what I've seen the use case outline above is the only one I've seen (so far). If someone really wants some other options, I feel doing something like #1095 in the future, in addition to the lookup-only option, might be the better choice. "Make the common case easy, and the edge case possible."

As for the idea to use an environment variable. I'm not sure this would be a good user experience. SEGMENT_DOWNLOAD_TIMEOUT_MINS works as env variable since it applies to all cache steps in a workflow equally. In contrast lookup-only will probably only be used in a few select cases. Users who would benefit from it likely even have multiple different caches per job (my use cases have). As env variable there is the risk it might get misused accidentally, e.g. added to the workflow env and applied to all cache actions even if that wasn't planned. The correct usage would thus require it to be added to the step env individually. Adding lookup-only: true is just easier and would be a better UX IMO.

I am also discussing this internally with my team, but want to get your views on the same.

Thanks for keeping me in the loop here and valuing my opinion! Appreciate that ❤️

[cdce8p]

@kotewar Just wanted to check, are there any updates?

It would be awesome if we could get this over the finish line soon. Especially as not much work is left. Please let me know if I can help in any way.

[kotewar]

Sorry, I was looking into another issues and forgot about this one.
I've approved the toolkit PR. Let's get it released first and then we can release the cache action.

Update - released the toolkit.

[kotewar]

Hi @cdce8p,

I was testing the action, and found that the log says cache restored, I think that gives wrong info to the user as in the earlier logs we say skipping download

Lookup only - skipping download

Cache restored from key: key1

I was thinking it will better if we print something else or don't print the restored log when its a lookup-only case.

[bishal-pdMSFT]

Does it check for exact match only? If so, then restore-keys will be ignored. May be we should call that out here.

[kotewar]

restore-keys won't be ignored, we can update the description to something like

description: 'Check if a cache entry exists for the given input(s) (key, restore-keys) without downloading the cache'

[bishal-pdMSFT]

@kotewar @cdce8p what is the output when restore key matches? We can't set cache-hit to true as that is true only in case of exact match.

[kotewar]

The output cache-hit will be false in that case.

[pascallj]

What's the purpose of accepting the restore-keys argument if we only set cache-hit to true when an exact match is found? Or do we return the matched key as well just like the restore action? If I understand correctly there is currently no difference in output between no exact match but a partial match.

[cdce8p]

If I understand correctly there is currently no difference in output between no exact match but a partial match.

Not quite. cache-hit will be true for exact matches. Think of it like this: When adding lookup-only the action behaves just the same (incl. the outputs) as without the argument. The only difference being that the cache entry isn't actually downloaded and extracted.

[pascallj]

I see. Normally there is a purpose of using the restore-keys because you are actually restoring something, but cache-hit will still be false. So from the view of the action there is still a cache miss. With this option there are no side effects when a match has occurred on one of the restore-keys.

While writing this I'm wondering: did I always misinterpret the note on cache-hit and does it also return true when an exact hit has been found on one of the restore-keys but just not on a partial match on the restore-keys?

[cdce8p]

While writing this I'm wondering: did I always misinterpret the note on cache-hit and does it also return true when an exact hit has been found on one of the restore-keys but just not on a partial match on the restore-keys?

cache-hit will only be true for an exact match on the primary key.

cache/src/restoreImpl.ts

Lines 67 to 72 in 940f3d7

	const isExactKeyMatch = utils.isExactKeyMatch(
	core.getInput(Inputs.Key, { required: true }),
	cacheKey
	);
	core.setOutput(Outputs.CacheHit, isExactKeyMatch.toString());

That doesn't mean though there aren't no side effects now with lookup-only: true if the key doesn't match. cache-matched-key is still set.

cache/src/restoreImpl.ts

Lines 64 to 65 in 940f3d7

	// Store the matched cache key in states
	stateProvider.setState(State.CacheMatchedKey, cacheKey);

--
In practice, I suspect the option will most likely only be used to check for an exact key (i.e. without restore-keys) but there might be other use cases I haven't though about yet which could use it. In any way as explained here #1041 (comment) the current implementation is not only simpler then a dedicated check action #1095 (which would have provided the option to remove the input) but also provides a better UX.

[pascallj]

Thank you. If that's always the case, then cache-matched-key should be mentioned as an output of the main action. Currently the only documented output is cache-hit. Therefore I assumed there were no side-effects in that case. Or this option should only be available in the restore action where this output is actually documented.

-- In practice, I suspect the option will most likely only be used to check for an exact key (i.e. without restore-keys) but there might be other use cases I haven't though about yet which could use it. In any way as explained here #1041 (comment) the current implementation is not only simpler then a dedicated check action #1095 (which would have provided the option to remove the input) but also provides a better UX.

I currently have a usecase where I cache all images in a matrix, and use them in another different matrix later on. In the setup I assume that if one file is successfully cached, all files (with the same prefix) are successfully cached. If this assumption is incorrect the matrix workflow will still succeed but less efficient (however it is unlikely that it would ever happen). It would be a waste to spin up the entire caching matrix just to check each key for a match. During the matrix workflow, each job uses only one file so there is no need to cache them into one key.

But in this case I can check for a match on one particular key instead and it will still have the same result.

[kotewar]

I agree, cache-matched-key (and even cache-primary-key like we have in restore action) will be helpful in the main action's output. We can plan to add it. In case anyone wants to contribute for the same, please feel free to do so. 😄

[bishal-pdMSFT]

Should this test be moved to restoreImpl.tests.ts as it is a common functionality?

[cdce8p]

Happy to do so but I think restore.test.ts does fit. The way I understand it is that restoreImpl.test.ts include all tests directly related to the cache restore, e.g. what happens if a key is found / want when it isn't, the path matches / doesn't match. In contrast restore.test.ts tests additional functionality, like failOnCacheMiss or now lookupOnly.

[kotewar]

Can we instead a new test in restoreImpl.tests.ts to test the behaviour when lookupOnly is set to true? I think that way both files will cover their respective code.

[cdce8p]

Hope I understood you correctly. As the test already checked the behavior of lookupOnly: true, I moved it to restoreImpl.test.ts. Please let me know if you had something else in mind.

lookupOnly: false is implicitly tested in every other test case as this is the default behavior.

[kotewar]

The only thing I had in mind was to check for the behavior of lookupOnly: true in the restoreImpl tests, than the wrapper code. I think what you've done looks fine as restoreImpl.test.ts was missing a lookupOnly: true case which it has now. :)

[cdce8p]

I was thinking it will better if we print something else or don't print the restored log when its a lookup-only case.

I do believe it's useful to know which cache would have been restored. Updated the log statement.

[kotewar]

LGTM.

[kotewar]

Hey @cdce8p 👋🏽

Thanks a lot of your contribution and apologies for the delay in reviewing.

I've released the new version v3.3.0 to marketplace and also tagged v3 to the latest version.

Thanks again for your great work, we appreciate it. 😊 ❤️

[cdce8p]

Thanks @kotewar ❤️