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

CI: try to make caching more reliable #25259

Merged
merged 11 commits into from
Sep 14, 2022
Merged

Conversation

kassens
Copy link
Member

@kassens kassens commented Sep 13, 2022

  • ~/.yarn/cache is now restored from an hierarchical cache key, if no precise match is found, we fallback to less precise ones.
  • The yarn install in fixtures/dom is also cached. Notably, is utilizes the cache from root, but stores into its more precise key.
  • Steps running in root no longer have a yarn install and rely on the cache from the setup step.
  • Retry yarn install once on failure.

# all jobs run on this branch with the same lockfile.
name: Save node_modules cache
# This cache key is per branch, a yarn install in setup is required.
key: v2-node-{{ arch }}-{{ .Branch }}-{{ checksum "yarn.lock" }}-{{ checksum "workspace_info.txt" }}-node-modules
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why we would include the branch name in the caching. This seems to just needlessly split the cache keys (wastes cache storage and more cache misses)

Copy link
Member

Choose a reason for hiding this comment

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

What if you add a new dependency in a branch? Doesn't the cache need to break so you install the dependency?

Copy link
Member

Choose a reason for hiding this comment

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

Answer: In that case the yarn.lock would change for that branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

But that doesn't have anything todo with the branch (name?) right? We might install a new dependency on the main branch or re-use a branch name across multiple PRs.

My thinking is that yarn.lock + workspace_info should capture everything relevant.

If we're feeling unsure about re-using the node_modules cache across PRs, we could also use the git revision for the key (I saw that's available in the docs)

name: Save node_modules cache
key: v1-node_modules-{{ arch }}-{{ checksum "yarn.lock" }}-{{ checksum "workspace_info.txt" }}
paths:
- node_modules
Copy link
Member Author

Choose a reason for hiding this comment

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

Just node_modules is actually insufficient as yarn workspaces create node_modules directories in each project.

This feels tricky to get stable. Maybe we should include the yarn install, but restore the yarn cache so the yarn install can make use of that.

Copy link
Member

Choose a reason for hiding this comment

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

I thought that that was what we do?

@kassens
Copy link
Member Author

kassens commented Sep 13, 2022

So something is still missing here. I'll keep investigating.

@kassens kassens force-pushed the ci-caching branch 2 times, most recently from 91c2884 to 41333e2 Compare September 13, 2022 22:41
@kassens
Copy link
Member Author

kassens commented Sep 14, 2022

I have 2 working configurations now:

  • yarn install everywhere, no more caching of node_modules. This simplifies the config and makes it more robust. sample run
  • Cache node_modules including the node_modules in each package. This avoids the yarn install which takes about 30s per step, but it's a bit fragile as it lists out all the node_module directories for each package. I also think there's a possibility of a cache injection by a malicious PR, so I included the epoch in the key to give every build its own cache. sample run

@rickhanlonii
Copy link
Member

I'd prefer option 2 as 30s increases the feedback loop by 50% for most of the tests.

@poteto
Copy link
Member

poteto commented Sep 14, 2022

@kassens An additional thing we could try is to retry the yarn install command if we see one of those 503 errors, which could still happen despite caching.

We could also try to see if caching is working with running the subsequent yarn install commands with the --offline flag which I think will error if it has to reach out to the network. If there are then maybe we need to make the initial setup job actually install all the packages.

@kassens
Copy link
Member Author

kassens commented Sep 14, 2022

@poteto if we remove the yarn installs from all subsequent steps and rely on node_modules caching, I don't think we need the offline flag. I do like the idea of adding a retry though for the yarn install at the very beginning. I'll add that here too.

@kassens
Copy link
Member Author

kassens commented Sep 14, 2022

I think this is finally ready for review!

@kassens kassens requested a review from poteto September 14, 2022 16:59
@kassens kassens merged commit c327b91 into facebook:main Sep 14, 2022
@kassens kassens deleted the ci-caching branch September 14, 2022 17:22
rickhanlonii pushed a commit that referenced this pull request Oct 5, 2022
- `~/.yarn/cache` is now restored from an hierarchical cache key, if no precise match is found, we fallback to less precise ones. 
- The yarn install in `fixtures/dom` is also cached. Notably, is utilizes the cache from root, but stores into its more precise key.
- Steps running in root no longer have a `yarn install` and rely on the cache from the setup step.
- Retry `yarn install` once on failure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants