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

Reset cache when workflow file changes #17

Merged
merged 8 commits into from
Jul 15, 2024

Conversation

benfrankel
Copy link
Contributor

@benfrankel benfrankel commented Jul 14, 2024

While working on bevy-template, we noticed the CI was constantly recompiling. It turns out that when we modified the ci.yaml file with different cargo commands, the cache became outdated and every "cache hit" was a false positive. But the cache never updated because we never changed Cargo.toml or Cargo.lock.

@benfrankel
Copy link
Contributor Author

I'm not 100% on whether the workflow file hash changing should restore from fallback or fully reset the cache. Set to fully reset for now.

@benfrankel
Copy link
Contributor Author

May want to consider allowing cache-group to override the workflow file hash as well, so that cache-group can still work between different workflow files -- but that also means different versions of the same workflow file.

@benfrankel
Copy link
Contributor Author

benfrankel commented Jul 14, 2024

May want to consider allowing cache-group to override the workflow file hash as well

Did this.

Also fixed matrix jobs all being in the same cache-group by adding strategy.job-index (the index of the matrix job) to the default cache-group.

@TimJentzsch TimJentzsch self-assigned this Jul 14, 2024
@alice-i-cecile
Copy link
Contributor

I'm going to largely defer to Tim here: he knows this tool better than I do :)

@TimJentzsch
Copy link
Collaborator

@benfrankel How about this:

  • We add strategy.job-index to the cache-group default as you suggested, but move the workflow file hash out of it
  • We place the workflow file hash at the end of the key (not sure if before the Cargo.toml hashes or after... probably after?) and add an additional fallback such that if the workflow file didn't change that specific command we keep the old cache (but overwrite after the command finished)

I guess the disadvantage of this approach would be that cache-group cannot be effectively shared between different workflow files, but that should be fine.

@benfrankel
Copy link
Contributor Author

Sounds good to me. For best of both worlds, there could be a separate input to overwrite the workflow file hash. Not sure what to call it etc. though, so I'll defer that for now.

@benfrankel
Copy link
Contributor Author

I moved cache-group to fallbacks because it was a smaller change from the previous iteration. Let me know if I should split out the workflow file hash etc. as described before merge.

@TimJentzsch
Copy link
Collaborator

Ok one last thought from my side:

What would you think about hashing the entire workflow folder and adding it to the end of the key, with a fallback.

Modifying the workflows is realtively rare so it should be fine to hash the whole thing. The fallback ensures that we still have a cash if the change is unrelated.
And hashing the whole workflow folder allows caches to be reused across multiple workflows.

What do you think about that?
If you dont like it thats fine as well, then I will merge the current iteration

@benfrankel
Copy link
Contributor Author

benfrankel commented Jul 15, 2024

That's an interesting option. One drawback I can think of: Same job name + matrix index but in a different workflow file => same cache key and forced to share a cache, which causes recompiles until you rename one of the jobs.

Same job name in a different workflow file may not be common, but when it happens it'd be confusing.

Also I had the thought that putting cache-group on the other end of the fallback order would be better. Like you said, workflow files don't change very often. So, if Cargo.toml or Cargo.lock change, which is more common, we should prefer to fall back to different Cargo.toml but same workflow file / job. However, if the workflow file changes, there's no need to be picky about the exact Cargo.toml of the fallback.

Copy link
Collaborator

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

In order to keep things moving, lets merge this iteration for now and refine it down the road as we test it in practice.

I cant do a release today, but will try tomorrow.
Ill do a v2 version bump, this probably qualifies as breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants