GH-36154: [JS][CI] Use `jest` cache in CI #36373

abetomo · 2023-06-29T09:07:03Z

Rationale for this change

Use the cache of test to reduce test time.

What changes are included in this PR?

Use jest cache with actions/cache@v3.

Are these changes tested?

In GitHub Actions, test time was reduced.

Are there any user-facing changes?

Closes: [JS][CI] Timeout on macos in GitHub Actions #36154

Signed-off-by: abetomo <abe@enzou.tokyo>

kou · 2023-06-29T13:46:43Z

.github/workflows/js.yml

+        uses: actions/cache@v3
+        with:
+          path: js/.jest-cache
+          key: js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-${{ hashFiles('js/yarn.lock') }}


Suggested change

key: js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-${{ hashFiles('js/yarn.lock') }}

key: js-jest-cache-${{ runner.os }}-${{ matrix.node }}-${{ hashFiles('js/yarn.lock') }}

Oh, thanks. Fixed.

kou · 2023-06-29T13:46:52Z

.github/workflows/js.yml

+          path: js/.jest-cache
+          key: js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-${{ hashFiles('js/yarn.lock') }}
+          restore-keys: |
+            js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-


Suggested change

js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-

js-jest-cache-${{ runner.os }}-${{ matrix.node }}-

kou · 2023-06-29T13:47:17Z

.github/workflows/js.yml

+          key: js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-${{ hashFiles('js/yarn.lock') }}
+          restore-keys: |
+            js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-
+            js-jest-cache-${{ runner.os }}-


Do we need this?
(Can we reuse cache for different Node.js version?)

The cache is reused even if different Node.js versions are used.
I have confirmed this by switching Node.js versions locally.

Then, can we remove -${{ matrix.node }} from key and restore-keys?

Will you be testing on more than one node in the future?
(like node: [18, 20].)

I just tested a few times locally and it seemed a little faster to use the cache of the same Node.js version.
It is certainly unnecessary if you don't test with multiple node.

We'll not use multiple Node.js versions unless we reduce CI time. (40+ minutes is too long to run multiple jobs. If we want to test against multiple Node.js versions, we'll use nightly CI that are defined in dev/tasks/tasks.yml.)

If the difference between with/without -${{ matrix.node }}, we want to unify it to reduce cache size. apache/arrow has multiple language implementations. So cache limit provided by GitHub Actions is small for us. So we want to reduce cache size as much as possible.

I got it.
I removed matrix.node.

kou · 2023-06-29T13:47:36Z

.github/workflows/js.yml

+          key: js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-${{ hashFiles('js/yarn.lock') }}
+          restore-keys: |
+            js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-


Suggested change

key: js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-${{ hashFiles('js/yarn.lock') }}

restore-keys: |

js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-

key: js-jest-cache-${{ runner.os }}-${{ matrix.node }}-${{ hashFiles('js/yarn.lock') }}

restore-keys: |

js-jest-cache-${{ runner.os }}-${{ matrix.node }}-

domoritz · 2023-06-29T14:47:10Z

Great. Do you have some before and after measurements?

Signed-off-by: abetomo <abe@enzou.tokyo>

abetomo · 2023-06-29T23:31:13Z

@domoritz Thanks for the review.
Here is the link to the measurement results on GitHub Actions.
#36154 (comment)

Signed-off-by: abetomo <abe@enzou.tokyo>

domoritz · 2023-06-30T04:46:49Z

Great. The times seem faster although not by as much as I expected. Are we sure that caching doesn’t introduce issues where tests are stale? What is in the cache?

abetomo · 2023-06-30T06:05:50Z

https://jestjs.io/docs/code-transformation

Jest will cache the result of a transformation and attempt to invalidate that result based on a number of factors, such as the source of the file being transformed and changing configuration.

Jest caches transformed codes.
In most cases, the cache should not cause problems.

The documentation notes the following.
https://jestjs.io/docs/cli#--cache

The cache should only be disabled if you are experiencing caching related problems.

So, it is not always possible to say that problems will not occur.

domoritz · 2023-06-30T06:10:30Z

So should we use the cache across lock files as well then?

abetomo · 2023-06-30T06:23:02Z

Do you mean to cache node_modules as well?
That seems better.

domoritz · 2023-06-30T17:35:54Z

No, caching node modules is a separate issue and I don't think it'll help much. What I mean is that in https://github.com/apache/arrow/pull/36373/files#diff-9992b6f0e329a6dc49913c62f6efe2102f7a64f55d4bdab3d47df86292db5e87R97 we key by the lockfile but if the jest cache is automatically only used when its applicable, we can remove the restriction to specific node and lockfile versions.

abetomo · 2023-06-30T23:49:26Z

At first, I too thought that the key was js-jest-cache-${{ runner.os }} would be enough.
However, if the key is the same, the cache stored on CI is not updated and the same cache is restored all the time.
So I thought it would be better to update the jest cache on CI when the lock file is updated, so I included the lock file in the key.

domoritz · 2023-07-01T08:34:29Z

Why would it be an issue if the cache is not updated? It feels odd to update based on the lock file rather than some other test related property.

abetomo · 2023-07-01T13:51:32Z

I agree that using a lock file is not the best way to do this.
I would like to be able to key in changes to files in src and test, but which is best?
Is it better to use github.sha?

If the cache is not updated, the cache will not be usable and the speed improvement will be less effective.

kou · 2023-07-01T21:46:10Z

What are the inputs for jest's cache? We should use a hash value for them in key.

domoritz · 2023-07-01T22:10:41Z

What we really want is a way to use a cache and still update it. Anything else will not catch many cases and the CI will still time out.

abetomo · 2023-07-01T22:37:26Z

Since the cache for jest is a transformed code, it is created from the code in src and test.
I could not find a way to use those hash values for the key in a good way.
I thought github.sha seemed to be the most like it.

kou · 2023-07-02T21:31:27Z

Then, we can use js-jest-cache-${{ runner.os }}-${{ hashFiles('js/src/**/*.ts', ''js/test/**/*.ts') }} for key like other our workflow files. If we can't reuse most cache when we update dependencies, we can also add js/yarn.lock like js-jest-cache-${{ runner.os }}-${{ hashFiles('js/src/**/*.ts', ''js/test/**/*.ts', 'js/yarn.lock') }}.

We should not use github.sha. If we use it, our cache is always updated even when we don't change js/{src,test}/**/*.ts. It will waste our available cache limit.

abetomo · 2023-07-03T00:29:41Z

@kou Thanks for the information.
I did not know that you could use * in hashFiles or that you could specify more than one.
js-jest-cache-${{ runner.os }}-${{ hashFiles('js/src/**/*.ts', ''js/test/**/*.ts') }} looks good.

@domoritz What do you think?

assignUser · 2023-07-05T01:58:18Z

What we really want is a way to use a cache and still update it.

This is not possible due to the way actions/cache works once a cache with a certain key is created it is immutable. But using the ts file hashes in the key should work as you intend imo. (unless dependencies also come into play than you might want to ad the lock file as kou said)

domoritz · 2023-07-05T03:53:10Z

Makes sense that caches are immutable. Do we have a way to check whether the cache is used? If it’s used even if dependencies change, I think we should not include the lock file in the hash.

assignUser · 2023-07-05T04:42:48Z

I am not sure what you mean with if the cache is used? Used by jest? I don't know @abetomo? Though actions/cache sets an output value if there was a cache hit or not, if that is what you mean.

kou · 2023-07-05T05:22:17Z

An existing cache is used even when dependencies are only changed because we have restore-keys: js-jest-cache-${{ runner.os }}-.
But the cache isn't updated if we don't have 'js/yarn.lock' in hashFiles(). Because the cache key isn't changed.
If jest's cache depends on dependencies too, we should add js/yarn.lock to hashFiles() too.

domoritz · 2023-07-05T07:31:28Z

What I mean is that I’d like to make sure we know when the jest cache is actually used by jest or whether jest ignored it because the cache wasn’t applicable to the code.

abetomo · 2023-07-05T08:32:26Z

The cache is generated in cacheDirectory.
If there is no cache (the cache is not used), a new cache file will be generated.
If the cache is used, no new file is generated.
This will help us determine if you used the cache or not.

There does not seem to be any option in jest to more easily know if the cache is used or not.

abetomo · 2023-07-06T02:35:05Z

I tried it and when the package related to the code transform changes (when the lock file is updated), jest cache is also updated.

What about the key js-jest-cache-${{ runner.os }}-${{ hashFiles('js/src/**/*.ts', ''js/test/**/*.ts', 'js/yarn.lock') }}?

Signed-off-by: abetomo <abe@enzou.tokyo>

kou

+1

I'll merge in a few days unless nobody objects this.

abetomo · 2023-07-11T05:47:08Z

Thanks for the merge.

conbench-apache-arrow · 2023-07-21T17:44:18Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit b07e3f1.There were 3 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-07-18 21:03:14Z
- compression=uncompressed, dataset=fanniemae_2016Q4, file_type=feather, output_type=table
- compression=lz4, dataset=nyctaxi_2010-01, file_type=feather, output_type=table
and 1 more (see the report linked below)

The full Conbench report has more details.

apacheGH-36154: [JS][CI] Use jest cache in CI

9c481b5

Signed-off-by: abetomo <abe@enzou.tokyo>

abetomo requested review from assignUser, kou, raulcd, domoritz and trxcllnt as code owners June 29, 2023 09:07

github-actions bot added Component: JavaScript awaiting review Awaiting review labels Jun 29, 2023

kou reviewed Jun 29, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jun 29, 2023

fix matrix property name

82047e6

Signed-off-by: abetomo <abe@enzou.tokyo>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 29, 2023

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 30, 2023

Remove matrix.node from cache key

8c3dd54

Signed-off-by: abetomo <abe@enzou.tokyo>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 30, 2023

trxcllnt approved these changes Jul 5, 2023

View reviewed changes

ci: add js/src/**/*.ts and js/test/**/*.ts to key

c60f506

Signed-off-by: abetomo <abe@enzou.tokyo>

kou approved these changes Jul 9, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 9, 2023

kou merged commit b07e3f1 into apache:main Jul 11, 2023

kou removed the awaiting merge Awaiting merge label Jul 11, 2023

abetomo deleted the ci-jest-cache branch July 11, 2023 05:46

	key: js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-${{ hashFiles('js/yarn.lock') }}
	key: js-jest-cache-${{ runner.os }}-${{ matrix.node }}-${{ hashFiles('js/yarn.lock') }}

	js-jest-cache-${{ runner.os }}-${{ matrix.node-version }}-
	js-jest-cache-${{ runner.os }}-${{ matrix.node }}-

GH-36154: [JS][CI] Use jest cache in CI #36373

GH-36154: [JS][CI] Use jest cache in CI #36373

Conversation

abetomo commented Jun 29, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domoritz commented Jun 29, 2023

abetomo commented Jun 29, 2023

domoritz commented Jun 30, 2023

abetomo commented Jun 30, 2023

domoritz commented Jun 30, 2023

abetomo commented Jun 30, 2023

domoritz commented Jun 30, 2023

abetomo commented Jun 30, 2023

domoritz commented Jul 1, 2023

abetomo commented Jul 1, 2023

kou commented Jul 1, 2023

domoritz commented Jul 1, 2023

abetomo commented Jul 1, 2023

kou commented Jul 2, 2023 • edited Loading

abetomo commented Jul 3, 2023

assignUser commented Jul 5, 2023

domoritz commented Jul 5, 2023

assignUser commented Jul 5, 2023

kou commented Jul 5, 2023 • edited Loading

domoritz commented Jul 5, 2023

abetomo commented Jul 5, 2023

abetomo commented Jul 6, 2023

kou left a comment

Choose a reason for hiding this comment

abetomo commented Jul 11, 2023

conbench-apache-arrow bot commented Jul 21, 2023

GH-36154: [JS][CI] Use `jest` cache in CI #36373

GH-36154: [JS][CI] Use `jest` cache in CI #36373

abetomo commented Jun 29, 2023 •

edited by github-actions bot

Loading

kou commented Jul 2, 2023 •

edited

Loading

kou commented Jul 5, 2023 •

edited

Loading