[CI] cache yarn instead of node_modules#25834
Merged
Merged
Conversation
Contributor
Author
|
Not sure why just "4 successful checks" show up here, clicking into the steps they show the same number of jobs… |
kassens
force-pushed
the
node-modules-ci
branch
from
December 6, 2022 23:31
5dbf921 to
11678e1
Compare
poteto
reviewed
Dec 7, 2022
eps1lon
reviewed
Dec 7, 2022
|
Comparing: 4bf2113...e11c23d Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
Collaborator
|
Another option could be to remove arch from the cache key and run |
Contributor
Author
|
|
acdlite
approved these changes
Feb 6, 2023
Contributor
|
@kassens Encountered a new CI issue regarding "node_modules". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current caching of steps of
node_modulesdoesn't work reliable as is because it includesarchin the cache key.archmight be different across workers in the same commit.I couldn't find a way to optionally restore caches, so what this PR does is:
yarn install, save yarn cache (fast if already exists)With this change the initial batch of jobs all race to populate the cache, but any subsequent jobs should find the existing cache. The expected downside would be slightly more worker CPU time with all the parallel jobs, but wall time might be shorter (1 step less in the critical path) and we should be more reliable as we no longer have the failure with multiple archs.
Alternative 1
Remove the
{arch}from the cache key.Downside: this might run into weird issues with native dependencies.
Alternative 2
Somehow check if the cache was restored and only then run a yarn install.
Downside: couldn't figure out if it's possible to only restore the yarn cache if restoring the node_modules cache failed. Without that we'd either always restore both the yarn and node_modules cache or do yarn installs w/o cache which are prone to failure in the past.