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] cache yarn instead of node_modules #25834

Merged
merged 7 commits into from
Feb 6, 2023
Merged

Conversation

kassens
Copy link
Member

@kassens kassens commented Dec 6, 2022

The current caching of steps of node_modules doesn't work reliable as is because it includes arch in the cache key. arch might be different across workers in the same commit.

I couldn't find a way to optionally restore caches, so what this PR does is:

  • remove the setup step that ran before all other steps and essentially just populates a circle CI cache key
  • all other steps now do: restore yarn cache, 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.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 6, 2022
@kassens
Copy link
Member Author

kassens commented Dec 6, 2022

Not sure why just "4 successful checks" show up here, clicking into the steps they show the same number of jobs…

@react-sizebot
Copy link

react-sizebot commented Feb 3, 2023

Comparing: 4bf2113...e11c23d

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 154.84 kB 154.84 kB = 49.12 kB 49.12 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 156.85 kB 156.85 kB = 49.78 kB 49.78 kB
facebook-www/ReactDOM-prod.classic.js = 533.79 kB 533.79 kB = 95.06 kB 95.06 kB
facebook-www/ReactDOM-prod.modern.js = 518.81 kB 518.81 kB = 92.82 kB 92.82 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against e11c23d

@sophiebits
Copy link
Collaborator

Another option could be to remove arch from the cache key and run yarn rebuild? Not sure if that is better or worse.

@kassens
Copy link
Member Author

kassens commented Feb 6, 2023

yarn rebuild doesn't exist for me? yarn install would probably do that, but I don't think that's faster then just only restoring the yarn cache.

@kassens kassens merged commit 1445acf into facebook:main Feb 6, 2023
@kassens kassens deleted the node-modules-ci branch February 6, 2023 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants