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

[do not merge] JIT CI better cache invalidation #5236

Draft
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

neduard
Copy link
Contributor

@neduard neduard commented Jul 17, 2024

Overview

From the following discussion: #5224 (comment)

Currently the CI cache for the JIT tests misses a few things which might change (and hence break the tests). At the very least they are:

  • the template in ci.yaml that's used to generate the JIT sources
  • the actual UCM binary for when running the tests (I think?)

As such, we use hashFiles(..) to ensure that when those dependencies change the source generation & tests are re-run.

Interesting/controversial decisions

I'm not super familiar with what triggers a rebuild. Is it just the hashes? If so, it seems like we would always want to hash the ucm binary since we would always want to rerun the tests if our main binary changes?

Also currently because we have the JIT source generation template in ci.yaml, any (unrelated) change in that file would cause a regeneration of the JIT sources. As such, ideally, we should move the JIT source generation into its own .yaml file. I haven't looked into that since it would take a bit more time to get familiar with github actions.

We could also remove caching all-together to be on the safe side, but I'm guessing caching was added for a reason. I'm thinking it would mean pulling from @unison/internal each time which might mean extra bandwidth costs?

Test coverage

n/a - relying on CI

Loose ends

n/a see "interesting/controversial decisions" above

@@ -372,7 +372,7 @@ jobs:
id: cache-generated-source
with:
path: ${{ env.jit_generated_src_scheme }}
key: jit_generated_src_scheme-jit_${{env.jit_version}}-${{env.jit_causalhash}}
key: jit_generated_src_scheme-jit_${{env.jit_version}}-${{env.jit_causalhash}}-${{ hashFiles('**/ci.yaml') }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wondering if we should also hash env.ucm here given we rely on a transcript to generate the jit sources?

@sellout
Copy link
Contributor

sellout commented Jul 18, 2024

Ah, is this why CI on my fork always fails in “generate jit source”?

I had made a note to figure out what’s going on, but hadn’t gotten around to it.

It has certainly caused me to overlook failures in my PRs, because I’m used to every push resulting in emails complaining about this.

@neduard
Copy link
Contributor Author

neduard commented Jul 18, 2024

Ah, is this why CI on my fork always fails in “generate jit source”?

hmm looks very likely 😅 I've definitely seen that error message before. Looking at the "create transcript" step , you can see it uses the old-style empty project .> in the template, which should now be scratch/main>

@neduard neduard closed this Jul 18, 2024
@neduard neduard deleted the jit-ci-cache-investigation branch July 18, 2024 11:01
@neduard neduard restored the jit-ci-cache-investigation branch July 18, 2024 11:02
@neduard neduard reopened this Jul 18, 2024
@aryairani
Copy link
Contributor

Yeah I think we've seen a few clues that this is broken, and I would like to fix it (I don't have it all paged in at the moment to evaluate this PR), but I also think including ci.yaml is too coarse. Including ucm might be too, I forget.

The main reason for the caching was to get CI times down, especially for when editing the .yaml files themselves. 20, 40 minutes per edit vs 4 or less is a big deal.

@neduard
Copy link
Contributor Author

neduard commented Jul 19, 2024

Sounds good thanks, I've raised #5240 to only update the generation template and hopefully make CI happy. We can then circle back on this one and get a proper fix in 👍

@neduard neduard marked this pull request as draft July 23, 2024 14:35
@neduard neduard changed the title JIT CI better cache invalidation [do not merge] JIT CI better cache invalidation Jul 23, 2024
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.

3 participants