Skip to content

Use additional npm / Yarn options to speed up installation #128

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
brianhelba wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use additional npm / Yarn options to speed up installation #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions hatch_jupyter_builder/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,25 +94,27 @@ def npm_builder(
if isinstance(npm, str):
npm = [npm]

is_yarn = (abs_path / "yarn.lock").exists()
Copy link
Contributor Author

@brianhelba brianhelba Sep 9, 2023

Choose a reason for hiding this comment

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

It's always been possible that this heuristic is incorrect, but the risk is slightly magnified now that we rely on it even if npm (the executable location) is explicitly provided.

I don't see a better alternative though, without attempting to look within npm to do pattern-matching for "yarn", which could still be incorrect (e.g. npm = "/home/john.yarn/bin/npm").

Copy link
Contributor

@blink1073 blink1073 Sep 17, 2023

Choose a reason for hiding this comment

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

Specifically this change is what I am uncertain about.


# Find a suitable default for the npm command.
if npm is None:
is_yarn = (abs_path / "yarn.lock").exists()
if is_yarn and not which("yarn"):
log.warning("yarn not found, ignoring yarn.lock file")
is_yarn = False

npm = ["yarn"] if is_yarn else ["npm"]

npm_cmd = normalize_cmd(npm)

install_cmd = ["install", "--immutable"] if is_yarn else ["install", "--no-audit", "--no-fund"]
Copy link
Contributor Author

@brianhelba brianhelba Sep 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It might be reasonable to use npm ci instead of npm install. npm ci is intended for automated environments where determinism is crucial, and it's roughly equivalent to yarn install --immutable.

However, npm ci does remove and reinstall node_modules. With package download caches, this doesn't use additional bandwidth, but it's definitely slower than npm install.

Do we care more about absolute correctness or speed when building internal JS assets for a Python package?

Copy link
Contributor

@blink1073 blink1073 Sep 14, 2023

Choose a reason for hiding this comment

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

I'm wary of changing any of this default behavior, as I'm not a JS developer anymore and I'm not sure what affect this would have on existing consumers.

Copy link
Contributor Author

@brianhelba brianhelba Sep 14, 2023

Choose a reason for hiding this comment

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

Ok, are you comfortable with this change as-is?

  • --no-fund only changes what's written to stdout, to make it cleaner in logs
  • --no-audit speeds up the command by preventing a background HTTP request and prevents additional output to stdout
  • For Yarn, --immetable is already the behavior when this is run in CI (determined via environment variables), so explicitly setting it makes the hook more consistent across all environments. Substantively, it's an important check, as it ensures that the yarn.lock will not be modified by the hook, which is very important to deterministic package builds.


if build_dir and source_dir and not force:
should_build = is_stale(build_dir, source_dir)
else:
should_build = True

if should_build:
log.info("Installing build dependencies with npm. This may take a while...")
run([*npm_cmd, "install"], cwd=str(abs_path))
run([*npm_cmd, *install_cmd], cwd=str(abs_path))
if build_cmd:
run([*npm_cmd, "run", build_cmd], cwd=str(abs_path))
else:
Expand Down
14 changes: 7 additions & 7 deletions tests/test_npm_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def test_npm_builder(mocker, repo):
npm_builder("wheel", "standard", path=repo)
run.assert_has_calls(
[
call(["foo", "install"], cwd=str(repo)),
call(["foo", "install", "--no-audit", "--no-fund"], cwd=str(repo)),
call(["foo", "run", "build"], cwd=str(repo)),
]
)
Expand Down Expand Up @@ -49,7 +49,7 @@ def test_npm_builder_yarn(mocker, repo):
npm_builder("wheel", "standard", path=repo)
run.assert_has_calls(
[
call(["foo", "install"], cwd=str(repo)),
call(["foo", "install", "--immutable"], cwd=str(repo)),
call(["foo", "run", "build"], cwd=str(repo)),
]
)
Expand All @@ -63,7 +63,7 @@ def test_npm_builder_missing_yarn(mocker, repo):
npm_builder("wheel", "standard", path=repo)
run.assert_has_calls(
[
call(["foo", "install"], cwd=str(repo)),
call(["foo", "install", "--no-audit", "--no-fund"], cwd=str(repo)),
call(["foo", "run", "build"], cwd=str(repo)),
]
)
Expand All @@ -76,7 +76,7 @@ def test_npm_builder_path(mocker, tmp_path):
npm_builder("wheel", "standard", path=tmp_path)
run.assert_has_calls(
[
call(["foo", "install"], cwd=str(tmp_path)),
call(["foo", "install", "--no-audit", "--no-fund"], cwd=str(tmp_path)),
call(["foo", "run", "build"], cwd=str(tmp_path)),
]
)
Expand All @@ -89,7 +89,7 @@ def test_npm_builder_editable(mocker, repo):
npm_builder("wheel", "editable", path=repo, editable_build_cmd="foo")
run.assert_has_calls(
[
call(["foo", "install"], cwd=str(repo)),
call(["foo", "install", "--no-audit", "--no-fund"], cwd=str(repo)),
call(["foo", "run", "foo"], cwd=str(repo)),
]
)
Expand All @@ -102,7 +102,7 @@ def test_npm_builder_npm_str(mocker, repo):
npm_builder("wheel", "standard", path=repo, npm="npm")
run.assert_has_calls(
[
call(["npm", "install"], cwd=str(repo)),
call(["npm", "install", "--no-audit", "--no-fund"], cwd=str(repo)),
call(["npm", "run", "build"], cwd=str(repo)),
]
)
Expand All @@ -113,7 +113,7 @@ def test_npm_builder_npm_build_command_none(mocker, repo):
run = mocker.patch("hatch_jupyter_builder.utils.run")
which.return_value = "npm"
npm_builder("wheel", "standard", path=repo, build_cmd=None)
run.assert_has_calls([call(["npm", "install"], cwd=str(repo))])
run.assert_has_calls([call(["npm", "install", "--no-audit", "--no-fund"], cwd=str(repo))])


def test_npm_builder_not_stale(mocker, repo):
Expand Down