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.

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

feat: cache get-app artifacts by commit_hash #1519

Merged
merged 12 commits into from
Jan 23, 2024
110 changes: 105 additions & 5 deletions bench/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@
import shutil
import subprocess
import sys
import tarfile
import typing
from collections import OrderedDict
from datetime import date
from functools import lru_cache
from pathlib import Path
from urllib.parse import urlparse

# imports - third party imports
Expand All @@ -23,6 +25,7 @@
UNSET_ARG,
fetch_details_from_tag,
get_available_folder_name,
get_bench_cache_path,
is_bench_directory,
is_git_url,
is_valid_frappe_branch,
Expand Down Expand Up @@ -166,13 +169,15 @@ def __init__(
branch: str = None,
bench: "Bench" = None,
soft_link: bool = False,
commit_hash = None,
*args,
**kwargs,
):
self.bench = bench
self.soft_link = soft_link
self.required_by = None
self.local_resolution = []
self.commit_hash = commit_hash
super().__init__(name, branch, *args, **kwargs)

@step(title="Fetching App {repo}", success="App {repo} Fetched")
Expand Down Expand Up @@ -227,6 +232,7 @@ def install(
resolved=False,
restart_bench=True,
ignore_resolution=False,
using_cached=False
):
import bench.cli
from bench.utils.app import get_app_name
Expand All @@ -247,6 +253,7 @@ def install(
skip_assets=skip_assets,
restart_bench=restart_bench,
resolution=self.local_resolution,
using_cached=using_cached,
)

@step(title="Cloning and installing {repo}", success="App {repo} Installed")
Expand Down Expand Up @@ -283,6 +290,90 @@ def update_app_state(self):
branch=self.tag,
required=self.local_resolution,
)


"""
Get App Cache

Since get-app affects only the `apps`, `env`, and `sites`
bench sub directories. If we assume deterministic builds
when get-app is called, the `apps/app_name` sub dir can be
cached.

In subsequent builds this would save time by not having to:
- clone repository
- install frontend dependencies
- building frontend assets
as all of this is contained in the `apps/app_name` sub dir.

Code that updates the `env` and `sites` subdirs still need
to be run.
"""

def get_app_path(self) -> Path:
return Path(self.bench.name) / "apps" / self.app_name

def get_app_cache_path(self, is_compressed=False) -> Path:
assert self.commit_hash is not None

cache_path = get_bench_cache_path("apps")
ext = "tgz" if is_compressed else "tar"
tarfile_name = f"{self.app_name}-{self.commit_hash[:10]}.{ext}"
return cache_path / tarfile_name

def get_cached(self) -> bool:
if not self.commit_hash:
return False

cache_path = self.get_app_cache_path()
mode = "r"

# Check if cache exists without gzip
if not cache_path.is_file():
cache_path = self.get_app_cache_path(True)
mode = "r:gz"

# Check if cache exists with gzip
if not cache_path.is_file():
return

app_path = self.get_app_path()
if app_path.is_dir():
shutil.rmtree(app_path)

click.secho(f"Getting {self.app_name} from cache", fg="yellow")
with tarfile.open(cache_path, mode) as tar:
tar.extractall(app_path.parent)

return True

def set_cache(self, compress_artifacts=False) -> bool:
if not self.commit_hash:
return False

app_path = self.get_app_path()
if not app_path.is_dir() or not is_valid_app_dir(app_path):
return False

cwd = os.getcwd()
cache_path = self.get_app_cache_path(compress_artifacts)
mode = "w:gz" if compress_artifacts else "w"

message = "Caching get-app artifacts"
if compress_artifacts:
message += " (compressed)"
click.secho(message)

os.chdir(app_path.parent)
Copy link
Member

@ankush ankush Jan 17, 2024

Choose a reason for hiding this comment

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

If possible avoid changing directories. This has caused problems in past as bench assumes a certain directory to be cwd always.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here it's required cause of how tar functions, it stores the entire path and untarring requires moving the file out of nested directories. I've wrapped tar in a try finally so cwd should be restored irrespective: 30f301e

with tarfile.open(cache_path, mode) as tar:
tar.add(app_path.name)
os.chdir(cwd)
return True


def is_valid_app_dir(app_path: Path) -> bool:
# TODO: Check from content if valid frappe app root
return True


def make_resolution_plan(app: App, bench: "Bench"):
Expand Down Expand Up @@ -346,6 +437,8 @@ def get_app(
soft_link=False,
init_bench=False,
resolve_deps=False,
commit_hash=None,
compress_artifacts=False,
):
"""bench get-app clones a Frappe App from remote (GitHub or any other git server),
and installs it on the current bench. This also resolves dependencies based on the
Expand All @@ -360,14 +453,14 @@ def get_app(
from bench.utils.app import check_existing_dir

bench = Bench(bench_path)
app = App(git_url, branch=branch, bench=bench, soft_link=soft_link)
app = App(git_url, branch=branch, bench=bench, soft_link=soft_link, commit_hash=commit_hash)
git_url = app.url
repo_name = app.repo
branch = app.tag
bench_setup = False
restart_bench = not init_bench
frappe_path, frappe_branch = None, None

if resolve_deps:
resolution = make_resolution_plan(app, bench)
click.secho("Following apps will be installed", fg="bright_blue")
Expand Down Expand Up @@ -417,6 +510,10 @@ def get_app(
verbose=verbose,
)
return

if app.get_cached():
app.install(verbose=verbose, skip_assets=skip_assets, restart_bench=restart_bench, using_cached=True)
return

dir_already_exists, cloned_path = check_existing_dir(bench_path, repo_name)
to_clone = not dir_already_exists
Expand All @@ -442,6 +539,9 @@ def get_app(
or click.confirm("Do you want to reinstall the existing application?")
):
app.install(verbose=verbose, skip_assets=skip_assets, restart_bench=restart_bench)

app.set_cache(compress_artifacts)



def install_resolved_deps(
Expand All @@ -452,7 +552,6 @@ def install_resolved_deps(
verbose=False,
):
from bench.utils.app import check_existing_dir

if "frappe" in resolution:
# Terminal dependency
del resolution["frappe"]
Expand Down Expand Up @@ -550,6 +649,7 @@ def install_app(
restart_bench=True,
skip_assets=False,
resolution=UNSET_ARG,
using_cached=False,
):
import bench.cli as bench_cli
from bench.bench import Bench
Expand Down Expand Up @@ -577,14 +677,14 @@ def install_app(
if conf.get("developer_mode"):
install_python_dev_dependencies(apps=app, bench_path=bench_path, verbose=verbose)

if os.path.exists(os.path.join(app_path, "package.json")):
if not using_cached and os.path.exists(os.path.join(app_path, "package.json")):
yarn_install = "yarn install --verbose" if verbose else "yarn install"
bench.run(yarn_install, cwd=app_path)

bench.apps.sync(app_name=app, required=resolution, branch=tag, app_dir=app_path)

if not skip_assets:
build_assets(bench_path=bench_path, app=app)
build_assets(bench_path=bench_path, app=app, using_cached=using_cached)

if restart_bench:
# Avoiding exceptions here as production might not be set-up
Expand Down
8 changes: 8 additions & 0 deletions bench/commands/make.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,9 @@ def drop(path):
default=False,
help="Resolve dependencies before installing app",
)
@click.option("--commit-hash", default=None, help="Required for caching get-app artifacts.")
@click.option("--cache-artifacts", is_flag=True, default=False, help="Whether to cache get-app artifacts. Needs commit-hash.")
@click.option("--compress-artifacts", is_flag=True, default=False, help="Whether to gzip get-app artifacts that are to be cached.")
def get_app(
git_url,
branch,
Expand All @@ -160,6 +163,9 @@ def get_app(
soft_link=False,
init_bench=False,
resolve_deps=False,
commit_hash=None,
cache_artifacts=False,
compress_artifacts=False,
):
"clone an app from the internet and set it up in your bench"
from bench.app import get_app
Expand All @@ -172,6 +178,8 @@ def get_app(
soft_link=soft_link,
init_bench=init_bench,
resolve_deps=resolve_deps,
commit_hash=commit_hash if cache_artifacts else None,
compress_artifacts=compress_artifacts,
)


Expand Down
12 changes: 11 additions & 1 deletion bench/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@
import sys
from functools import lru_cache
from glob import glob
from pathlib import Path
from shlex import split
from typing import List, Tuple
from typing import List, Optional, Tuple

# imports - third party imports
import click
Expand Down Expand Up @@ -50,6 +51,15 @@ def is_frappe_app(directory: str) -> bool:

return bool(is_frappe_app)

def get_bench_cache_path(sub_dir: Optional[str]) -> Path:
relative_path = "~/.cache/bench"
if sub_dir and not sub_dir.startswith("/"):
relative_path += f"/{sub_dir}"

cache_path = os.path.expanduser(relative_path)
cache_path = Path(cache_path)
cache_path.mkdir(parents=True, exist_ok=True)
return cache_path

@lru_cache(maxsize=None)
def is_valid_frappe_branch(frappe_path: str, frappe_branch: str):
Expand Down
6 changes: 5 additions & 1 deletion bench/utils/bench.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,14 @@ def restart_process_manager(bench_path=".", web_workers=False):
exec_cmd(f"overmind restart {worker}", cwd=bench_path)


def build_assets(bench_path=".", app=None):
def build_assets(bench_path=".", app=None, using_cached=False):
command = "bench build"
if app:
command += f" --app {app}"

if using_cached:
command += " --using-cached"

exec_cmd(command, cwd=bench_path, env={"BENCH_DEVELOPER": "1"})


Expand Down
Loading