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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 25, 2024
Merged

[core] Fix l10n GH workflow #12895

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f9fc5ed
[core] Fix `l10n` GH workflow
LukasTy Apr 24, 2024
cce3d70
test install time without cache
LukasTy Apr 25, 2024
8344ea2
Revert - will monitor state on master
LukasTy Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .github/workflows/l10n.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,16 @@ jobs:
steps:
- run: echo "${{ github.actor }}"
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- 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
Copy link
Member Author

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. 🤔

Copy link
Member

@JCQuintas JCQuintas Apr 24, 2024 •
edited
Loading

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

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

@LukasTy LukasTy Apr 24, 2024 •
edited
Loading

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. 👌

Copy link
Member Author

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. 🙈

Copy link
Member Author

@LukasTy LukasTy Apr 25, 2024 •
edited
Loading

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.

Copy link
Member

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 😮

Copy link
Member Author

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. 🙈

Copy link
Member

@Janpot Janpot Apr 25, 2024 •
edited
Loading

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

  1. 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.

- run: pnpm install
- run: pnpm install --frozen-lockfile
- name: pnpm l10n --report
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Expand Down
Loading