-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[core] Fix l10n
GH workflow
#12895
[core] Fix l10n
GH workflow
#12895
Conversation
Deploy preview: https://deploy-preview-12895--material-ui-x.netlify.app/ |
- uses: pnpm/action-setup@a3252b78c470c02df07e9d59298aecedc3ccdd6d #v3.0.0 | ||
with: | ||
version: 8 | ||
run_install: false | ||
- name: Use Node.js 20.x | ||
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2 | ||
with: | ||
node-version: 20 | ||
cache: 'pnpm' # https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#caching-packages-dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT @mui/code-infra, does it make sense to remove the cache here as well?
We no longer use it in CircleCI pipelines with pnpm
. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to modify the on:
in this workflow file to also allow workflow_dispatch then you can manually test the effects of caching. Just remember to revert it before merging :)
We might alternatively want to use pnpm's cache instructions instead https://github.com/pnpm/action-setup?tab=readme-ov-file#use-cache-to-reduce-installation-time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, can we measure it easily? In any case, not a blocker for this PR, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing cache in Core helped us save some CI money. Run times were comparable but we don't spend on storage.
But your mileage may vary, so it's better to verify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaldudak We have also disabled caching for CircleCi, but here we are talking about GH Actions.
Are we even paying for GH action runtime given that the repositories are public?
P.S. Core also has an action with pnpm cache usage. However, it makes sense there because 3 test instances are being run. 👍
I'm not sure, can we measure it easily?
I'll try to test based on @JCQuintas suggestion. 😉
In any case, not a blocker for this PR, right?
Not at all. 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JCQuintas Thanks for all the input.
It's a great observation regarding the cache.
However, it seems that those caches are possibly this big because they haven't been updated for some time and could be holding old packages in there. 🤔
We can always manually purge the cache and see how it behaves after that.
P.S. I was unable to test this GH action on the branch because it uses the workflow code on the default branch.
It will be easier to observe the behavior and course correct later on if necessary. 👌
P.P.S. Core has way more caches and they are approaching the limit after which the caches would be purged. 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JCQuintas @Janpot @michaldudak
Runtime cache-miss: 65s (31s to install and 21s to save cache)
Runtime cache-hit: 34s (9s to install and 14s to restore cache)
IMHO, it makes sense to keep caching for GH Actions. Their hardware for running it is probably faster and the math is different in that case as compared to CircleCI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnpm
cache is only 500mb 😮
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it could be the result of package linking instead of duplication. 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They use copy-on-write mechanisms for their cache store: https://pnpm.io/motivation#saving-disk-space
- If you depend on different versions of the dependency, only the files that differ are added to the store. For instance, if it has 100 files, and a new version has a change in only one of those files,
pnpm update
will only add 1 new file to the store, instead of cloning the entire dependency just for the singular change.
Fix workflow broken after switching to
pnpm
: https://github.com/mui/mui-x/actions/runs/8818836993/job/24208602684.