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

Jump to bottom

Add caching of node_modules and eslintcache to Github Actions #4

Merged
merged 11 commits into from
Apr 30, 2021
Merged

Add caching of node_modules and eslintcache to Github Actions #4

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
433bda5
Add more caching to build actions
mattrunyon Apr 27, 2021
50e5c8f
Add missing space to actions file. Conveniently reruns check to test …
mattrunyon Apr 27, 2021
90d1c21
Try removing lerna hoist, node_modules cache, and parallel tests
mattrunyon Apr 28, 2021
4929ee6
Readd hoist, bootstrap cache. Set eslint to use content instead of me…
mattrunyon Apr 29, 2021
290d54f
Hopefully fix for linter cache file globs
mattrunyon Apr 29, 2021
c16951d
Add maxWorkers=2 for unit tests
mattrunyon Apr 29, 2021
58abf59
Trying runInBand for tests
mattrunyon Apr 29, 2021
1c150bf
Remove runInBand for final test
mattrunyon Apr 29, 2021
58f9088
Swap build/test so packages aonly need to be built once
mattrunyon Apr 30, 2021
2c39c9f
Update caching rule globs and set maxWorkers=2 for tests
mattrunyon Apr 30, 2021
9e0b4e3
Hash all package.json for skipping npm ci
mattrunyon Apr 30, 2021
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
41 changes: 36 additions & 5 deletions .github/workflows/node.js.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,57 @@ jobs:

steps:
- uses: actions/checkout@v2

- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v2
with:
node-version: ${{ matrix.node-version }}
- name: Cache node modules

- name: Cache node_modules
id: cache-node-modules
uses: actions/cache@v2
with:
# If no package-locks or the main package.json have changed, it should be safe to restore all node_modules as they were
# Changing the main package.json may change the postinstall scripts, so it should be unchanged to cache install results
path: |
node_modules
packages/*/node_modules
key: ${{ runner.os }}-modules-${{ hashFiles('package.json', 'package-lock.json', 'packages/*/package-lock.json') }}
Copy link
Member

Choose a reason for hiding this comment

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

Should include packages/*/package.json as well. In the case where someone accidentally updates a package.json but doesn't update the associated package-lock.json, we don't want to pull from the cache

If dependencies in the package lock do not match those in package.json, npm ci will exit with an error, instead of updating the package lock.


- name: Cache npm cache
uses: actions/cache@v2
env:
cache-name: cache-node-modules
cache-name: cache-npm-cache
with:
# npm cache files are stored in `~/.npm` on Linux/macOS
path: ~/.npm
key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ matrix.node-version }}-${{ hashFiles('**/package-lock.json') }}
key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ matrix.node-version }}-${{ hashFiles('package-lock.json', 'packages/*/package-lock.json') }}
restore-keys: |
${{ runner.os }}-build-${{ env.cache-name }}-${{ matrix.node-version }}
${{ runner.os }}-build-${{ env.cache-name }}-
${{ runner.os }}-build-
${{ runner.os }}-

- name: Cache linter caches
uses: actions/cache@v2
with:
path: |
.eslintcache
packages/*/.eslintcache
.stylelintcache
packages/*/.stylelintcache
key: ${{ runner.os }}-lintcache-${{ github.head_ref }}
restore-keys: |
${{ runner.os }}-lintcache-
${{ runner.os }}-

# Only need to install and bootstrap deps if package-locks changed
- name: Install dependencies
if: steps.cache-node-modules.outputs.cache-hit != 'true'
run: npm ci
Comment on lines 66 to 68
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 very knowledgeable about this repo or NPM, but shouldn't npm ci be a fast no-op if node_modules is already populated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't take too long (it actually only installs the dependencies of the root project), but the post install script which uses lerna to bootstrap and link the packages (and install their much larger list of dependencies) is slow (60-120s).

I know we can speed up lerna bootstrap by removing the hoist flag at the cost of space from redundant dependencies being installed in multiple packages. It will still take 45-60s though I think.

Copy link
Member

Choose a reason for hiding this comment

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

So, using the presence of a cache hit to skip a full step is a bit hacky, and in general pretty dangerous. Is there any sort of lerna cache we could take advantage of? Is there a more appropriate task that we should be running, and we shouldn't need to do post install / lerna stuff?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree it's a bit hacky. I don't know if it's necessarily that dangerous though since it's matching only if the hash of all package-locks is unchanged. The actual files can still change and be used correctly since lerna symlinks node_modules within the project (e.g. client-ui depends on grid. node_modules/@deephaven/grid is symlinked to packages/grid). If package-lock hasn't changed then that indicates the exact dependencies are the same as they were before.

Using yarn might fix part of the speed issue, but I was having other problems with yarn locally with the license files being symlinks (it couldn't find the file).

The vast majority of our PRs (90%+, probably higher) don't touch package-lock files. If we can speed up lerna + npm then this wouldn't be necessary, but npm is painfully slow when doing a no-op install.

Also I think npm ci actually deletes node_modules to start. Which it should only be deleting the small set of dependencies from the root. That on its own takes ~10 seconds for me locally when I remove the postinstall script.

Copy link
Member

Choose a reason for hiding this comment

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

@devinrsmith why is using the cache hit to skip a step hacky? That's exactly what this action is for: https://github.com/actions/cache#skipping-steps-based-on-cache-hit

@mattrunyon There's no more sym linked license files so you should be good from there to try yarn again. I think npm ci should be clearing all the node_modules first, to make sure it has a false pass when a package is removed or something.

- name: Test
run: npm run test:ci

- name: Build
run: npm run build

- name: Test
run: npm run test:ci
5 changes: 2 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"clean:build": "lerna run clean --stream",
"clean:modules": "lerna clean --yes",
"clean": "npm run clean:build && npm run clean:modules",
"bootstrap": "lerna bootstrap --hoist",
"bootstrap": "lerna bootstrap --hoist -- --no-audit --prefer-offline",
"build": "lerna run build --stream",
"build:app": "lerna run --scope=@deephaven/code-studio build",
"build:packages": "lerna run --ignore=@deephaven/code-studio build --stream",
Expand All @@ -19,9 +19,8 @@
"start:app": "lerna run start --scope=@deephaven/code-studio --stream",
"start:packages": "lerna run watch --ignore=@deephaven/code-studio --stream --parallel",
"prestart": "lerna run prestart --stream",
"pretest:ci": "npm run build:packages",
"test": "lerna run test --stream --parallel",
"test:ci": "lerna run test:ci --stream --parallel",
"test:ci": "lerna run test:ci --stream -- -- --maxWorkers=2",
"sync-version": "lerna version ${DEEPHAVEN_VERSION:-error} --no-git-tag-version --yes",
"publish": "lerna publish"
},
Expand Down
1 change: 1 addition & 0 deletions packages/code-studio/src/test/eslint.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ it('eslint', async () => {
const eslint = new ESLint({
extensions: ['js', 'jsx', 'ts', 'tsx'],
cache: true,
cacheStrategy: 'content',
});
const results = await eslint.lintFiles('./src');
const formatter = await eslint.loadFormatter();
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/test/eslint.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ it('eslint', async () => {
const eslint = new ESLint({
extensions: ['js', 'jsx', 'ts', 'tsx'],
cache: true,
cacheStrategy: 'content',
});
const results = await eslint.lintFiles('./src');
const formatter = await eslint.loadFormatter();
Expand Down
1 change: 1 addition & 0 deletions packages/jsapi-shim/src/test/eslint.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ it('eslint', async () => {
const eslint = new ESLint({
extensions: ['js', 'jsx', 'ts', 'tsx'],
cache: true,
cacheStrategy: 'content',
});
const results = await eslint.lintFiles('./src');
const formatter = await eslint.loadFormatter();
Expand Down
1 change: 1 addition & 0 deletions packages/log/src/test/eslint.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ it('eslint', async () => {
const eslint = new ESLint({
extensions: ['js', 'jsx', 'ts', 'tsx'],
cache: true,
cacheStrategy: 'content',
});
const results = await eslint.lintFiles('./src');
const formatter = await eslint.loadFormatter();
Expand Down
1 change: 1 addition & 0 deletions packages/react-hooks/src/test/eslint.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ it('eslint', async () => {
const eslint = new ESLint({
extensions: ['js', 'jsx', 'ts', 'tsx'],
cache: true,
cacheStrategy: 'content',
});
const results = await eslint.lintFiles('./src');
const formatter = await eslint.loadFormatter();
Expand Down
1 change: 1 addition & 0 deletions packages/utils/src/test/eslint.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ it('eslint', async () => {
const eslint = new ESLint({
extensions: ['js', 'jsx', 'ts', 'tsx'],
cache: true,
cacheStrategy: 'content',
});
const results = await eslint.lintFiles('./src');
const formatter = await eslint.loadFormatter();
Expand Down