Skip to content

npm package untar not to use remote-cache #1974

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+7 −0
Open

npm package untar not to use remote-cache #1974

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9fd906a
npm package untar to not use remote-cache
fa93hws Oct 13, 2024
893c017
Update npm/private/npm_package_store.bzl
fa93hws Oct 13, 2024
b068aaa
add comment block
fa93hws Oct 13, 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
7 changes: 7 additions & 0 deletions npm/private/npm_package_store.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,13 @@ def _npm_package_store_impl(ctx):
"--directory",
package_store_directory.path,
],
# no need to cache tarball extraction for npm package remotely. Untar is faster than uploading/downloading
# cache in most of the cases, and untar creates mutliple outputs and sometimes the number of the output can be massive
# meaning it can takes much more time to upload the cache compared to the actual untar.
# see https://github.com/aspect-build/rules_js/pull/1974
execution_requirements = {
Copy link
Collaborator

@dzbarsky dzbarsky Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd believe this is a speedup for local builds, but I think it's a pessimization for remote builds, since you'd need to upload the results anyway to the remote cluster. Same exact reasoning as the execution requirements on bazel-lib CopyFile applies here. So let's make this configurable if we do land it

Copy link

Choose a reason for hiding this comment

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

I remembered the original CopyFile/Directory execution requirements didn't play well with BWOB and was worried about the same thing here.

Copy link
Member

Choose a reason for hiding this comment

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

Right, the problem is:

  • if inputs are local, and subsequent actions that depend on the outputs will run local, you want this action to run local and skip writing the result to remote cache
  • if inputs are remote, or subsequent actions are remote, you want this action to run remote and store result in the remote cache

So there isn't a reasonable default we can use for everyone. Perhaps a better answer is to document that you can use --modify_execution_info= NpmPackageExtract=+no-remote if you find your builds are better tuned with that setting

Copy link
Member

Choose a reason for hiding this comment

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

another possibility is to add that flag to a preset, like https://github.com/bazel-contrib/bazel-lib/blob/main/.aspect/bazelrc/javascript.bazelrc - this way more devs will discover it

"no-remote-cache": "1",
},
mnemonic = "NpmPackageExtract",
progress_message = "Extracting npm package {}@{}".format(package, version),
)
Expand Down