From 86a7c8e7d43a084bd0d93d8ad7261b6cf71f185e Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 10 Mar 2026 00:31:16 +0000 Subject: [PATCH 1/3] fix: reuse pre-built SDK sdist to speed up image builds Avoids running 'uv build' 500 times (once per image), saving ~15s per image and eliminating logging spam. --- benchmarks/utils/build_utils.py | 97 ++++++++++++++++++++++++++++++++- 1 file changed, 95 insertions(+), 2 deletions(-) diff --git a/benchmarks/utils/build_utils.py b/benchmarks/utils/build_utils.py index df5c10fa7..20426b04f 100644 --- a/benchmarks/utils/build_utils.py +++ b/benchmarks/utils/build_utils.py @@ -11,6 +11,8 @@ import sys import time import tomllib +import shutil +import tempfile from concurrent.futures import ProcessPoolExecutor, as_completed from datetime import UTC, datetime from pathlib import Path @@ -275,12 +277,89 @@ def get_build_parser() -> argparse.ArgumentParser: return parser + +def build_sdist_package() -> Path | None: + """Build the SDK sdist package once and return its path.""" + # Use a unique temp dir for the sdist to avoid conflicts + cache_dir = Path(tempfile.mkdtemp(prefix="oh-sdist-cache-")) + logger.info(f"Building SDK sdist package in {cache_dir}...") + + # SDK root is relative to this file: ../../../vendor/software-agent-sdk + benchmarks_root = Path(__file__).resolve().parent.parent.parent + sdk_path = benchmarks_root / "vendor" / "software-agent-sdk" + + if not sdk_path.exists(): + logger.warning(f"SDK path {sdk_path} does not exist. Skipping sdist build optimization.") + return None + + cmd = ["uv", "build", "--sdist", "--out-dir", str(cache_dir)] + # We allow stderr to print (or capture it if verbose?) + # Since we run this once, let's capture it to avoid spamming unless error. + try: + proc = subprocess.run(cmd, cwd=sdk_path, check=True, capture_output=True, text=True) + except subprocess.CalledProcessError as e: + logger.error(f"Failed to build sdist: {e}\nStdout: {e.stdout}\nStderr: {e.stderr}") + raise + + tarballs = list(cache_dir.glob("*.tar.gz")) + if not tarballs: + raise RuntimeError("Failed to build sdist: no tarball found") + + sdist_path = tarballs[0] + logger.info(f"Built sdist: {sdist_path}") + return sdist_path + + +@contextlib.contextmanager +def patch_uv_build(sdist_path: Path | None): + """ + Context manager to patch openhands.agent_server.docker.build._run + to skip 'uv build' and use pre-built sdist. + """ + if not sdist_path or not sdist_path.exists(): + yield + return + + # Import inside function to avoid circular imports or early import + try: + from openhands.agent_server.docker import build as build_module + except ImportError: + yield + return + + original_run = build_module._run + + def patched_run(cmd: list[str], cwd: str | None = None) -> subprocess.CompletedProcess: + # Check if this is the "uv build --sdist" command + # cmd is ["uv", "build", "--sdist", "--out-dir", ...] + if len(cmd) >= 4 and cmd[0] == "uv" and cmd[1] == "build" and "--sdist" in cmd: + try: + if "--out-dir" in cmd: + idx = cmd.index("--out-dir") + 1 + if idx < len(cmd): + out_dir = Path(cmd[idx]) + logger.info(f"Using pre-built sdist from {sdist_path} for {out_dir}") + shutil.copy(sdist_path, out_dir / sdist_path.name) + return subprocess.CompletedProcess(cmd, 0, stdout="Mocked uv build", stderr="") + except Exception as e: + logger.warning(f"Failed to use pre-built sdist: {e}") + + return original_run(cmd, cwd) + + build_module._run = patched_run + try: + yield + finally: + build_module._run = original_run + + def build_image( base_image: str, target_image: str, custom_tag: str, target: TargetType = "source-minimal", push: bool = False, + sdist_path: Path | None = None, ) -> BuildOutput: # Get SDK info from submodule to ensure tags use the correct SDK SHA git_ref, git_sha, sdk_version = _get_sdk_submodule_info() @@ -303,7 +382,8 @@ def build_image( if image_exists(t): logger.info("Image %s already exists. Skipping build.", t) return BuildOutput(base_image=base_image, tags=[t], error=None) - tags = build(opts) + with patch_uv_build(sdist_path): + tags = build(opts) return BuildOutput(base_image=base_image, tags=tags, error=None) @@ -312,6 +392,7 @@ def ensure_local_image( base_image: str, custom_tag: str, target: TargetType = "source-minimal", + sdist_path: Path | None = None, ) -> bool: """Build an agent-server image locally if it doesn't already exist. @@ -333,6 +414,7 @@ def ensure_local_image( custom_tag=custom_tag, target=target, push=False, + sdist_path=sdist_path, ) logger.info(f"Image build output: {output}") if output.error is not None: @@ -354,6 +436,7 @@ def _build_with_logging( push: bool = False, max_retries: int = 3, post_build_fn: Callable[[BuildOutput, bool], BuildOutput] | None = None, + sdist_path: Path | None = None, ) -> BuildOutput: """ Module-level function for building a single image with output capture. @@ -375,7 +458,7 @@ def _build_with_logging( ) time.sleep(2 + attempt * 2) try: - result = build_image(base_image, target_image, custom_tag, target, push) + result = build_image(base_image, target_image, custom_tag, target, push, sdist_path=sdist_path) except Exception as e: result = BuildOutput( base_image=base_image, @@ -480,6 +563,15 @@ def build_all_images( manifest_file = build_dir / "manifest.jsonl" manifest_file.parent.mkdir(parents=True, exist_ok=True) + if not dry_run: + try: + sdist_path = build_sdist_package() + except Exception as e: + logger.error(f"Failed to pre-build sdist: {e}. Will fall back to per-build sdist.") + sdist_path = None + else: + sdist_path = None + if dry_run: print("\n".join(base_images)) return 0 @@ -543,6 +635,7 @@ def _chunks(seq: list[str], size: int): push=push, max_retries=max_retries, post_build_fn=post_build_fn, + sdist_path=sdist_path, ) futures[fut] = base From 93ebaf65000de902f1e5ac39a1d73e93b0471b17 Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 10 Mar 2026 22:56:27 +0000 Subject: [PATCH 2/3] fix: Add cleanup for sdist temp directory to prevent memory leaks Convert build_sdist_package() to a context manager that: 1. Creates temp directory for sdist build 2. Yields the path for use in builds 3. Ensures cleanup in finally block (shutil.rmtree) This addresses potential OOM issues by guaranteeing cleanup even if builds fail or are interrupted. Co-authored-by: openhands --- benchmarks/utils/build_utils.py | 298 ++++++++++++++++++-------------- 1 file changed, 164 insertions(+), 134 deletions(-) diff --git a/benchmarks/utils/build_utils.py b/benchmarks/utils/build_utils.py index 20426b04f..9d7ec154f 100644 --- a/benchmarks/utils/build_utils.py +++ b/benchmarks/utils/build_utils.py @@ -7,12 +7,12 @@ import contextlib import io import os +import shutil import subprocess import sys +import tempfile import time import tomllib -import shutil -import tempfile from concurrent.futures import ProcessPoolExecutor, as_completed from datetime import UTC, datetime from pathlib import Path @@ -277,37 +277,56 @@ def get_build_parser() -> argparse.ArgumentParser: return parser +@contextlib.contextmanager +def build_sdist_package(): + """Build the SDK sdist package once and yield its path. -def build_sdist_package() -> Path | None: - """Build the SDK sdist package once and return its path.""" - # Use a unique temp dir for the sdist to avoid conflicts - cache_dir = Path(tempfile.mkdtemp(prefix="oh-sdist-cache-")) - logger.info(f"Building SDK sdist package in {cache_dir}...") - + This is a context manager that creates a temp directory, builds the sdist, + and ensures cleanup when done. + + Yields: + Path | None: Path to the built sdist tarball, or None if SDK path doesn't exist. + """ # SDK root is relative to this file: ../../../vendor/software-agent-sdk benchmarks_root = Path(__file__).resolve().parent.parent.parent sdk_path = benchmarks_root / "vendor" / "software-agent-sdk" - + if not sdk_path.exists(): - logger.warning(f"SDK path {sdk_path} does not exist. Skipping sdist build optimization.") - return None + logger.warning( + f"SDK path {sdk_path} does not exist. Skipping sdist build optimization." + ) + yield None + return - cmd = ["uv", "build", "--sdist", "--out-dir", str(cache_dir)] - # We allow stderr to print (or capture it if verbose?) - # Since we run this once, let's capture it to avoid spamming unless error. + # Use a unique temp dir for the sdist to avoid conflicts + cache_dir = Path(tempfile.mkdtemp(prefix="oh-sdist-cache-")) try: - proc = subprocess.run(cmd, cwd=sdk_path, check=True, capture_output=True, text=True) - except subprocess.CalledProcessError as e: - logger.error(f"Failed to build sdist: {e}\nStdout: {e.stdout}\nStderr: {e.stderr}") - raise + logger.info(f"Building SDK sdist package in {cache_dir}...") - tarballs = list(cache_dir.glob("*.tar.gz")) - if not tarballs: - raise RuntimeError("Failed to build sdist: no tarball found") - - sdist_path = tarballs[0] - logger.info(f"Built sdist: {sdist_path}") - return sdist_path + cmd = ["uv", "build", "--sdist", "--out-dir", str(cache_dir)] + # Capture output to avoid spamming logs; only show on error. + try: + subprocess.run( + cmd, cwd=sdk_path, check=True, capture_output=True, text=True + ) + except subprocess.CalledProcessError as e: + logger.error( + f"Failed to build sdist: {e}\nStdout: {e.stdout}\nStderr: {e.stderr}" + ) + raise + + tarballs = list(cache_dir.glob("*.tar.gz")) + if not tarballs: + raise RuntimeError("Failed to build sdist: no tarball found") + + sdist_path = tarballs[0] + logger.info(f"Built sdist: {sdist_path}") + yield sdist_path + finally: + # Clean up the temp directory + if cache_dir.exists(): + logger.info(f"Cleaning up sdist cache: {cache_dir}") + shutil.rmtree(cache_dir, ignore_errors=True) @contextlib.contextmanager @@ -329,7 +348,9 @@ def patch_uv_build(sdist_path: Path | None): original_run = build_module._run - def patched_run(cmd: list[str], cwd: str | None = None) -> subprocess.CompletedProcess: + def patched_run( + cmd: list[str], cwd: str | None = None + ) -> subprocess.CompletedProcess: # Check if this is the "uv build --sdist" command # cmd is ["uv", "build", "--sdist", "--out-dir", ...] if len(cmd) >= 4 and cmd[0] == "uv" and cmd[1] == "build" and "--sdist" in cmd: @@ -338,12 +359,16 @@ def patched_run(cmd: list[str], cwd: str | None = None) -> subprocess.CompletedP idx = cmd.index("--out-dir") + 1 if idx < len(cmd): out_dir = Path(cmd[idx]) - logger.info(f"Using pre-built sdist from {sdist_path} for {out_dir}") + logger.info( + f"Using pre-built sdist from {sdist_path} for {out_dir}" + ) shutil.copy(sdist_path, out_dir / sdist_path.name) - return subprocess.CompletedProcess(cmd, 0, stdout="Mocked uv build", stderr="") + return subprocess.CompletedProcess( + cmd, 0, stdout="Mocked uv build", stderr="" + ) except Exception as e: logger.warning(f"Failed to use pre-built sdist: {e}") - + return original_run(cmd, cwd) build_module._run = patched_run @@ -458,7 +483,14 @@ def _build_with_logging( ) time.sleep(2 + attempt * 2) try: - result = build_image(base_image, target_image, custom_tag, target, push, sdist_path=sdist_path) + result = build_image( + base_image, + target_image, + custom_tag, + target, + push, + sdist_path=sdist_path, + ) except Exception as e: result = BuildOutput( base_image=base_image, @@ -563,15 +595,6 @@ def build_all_images( manifest_file = build_dir / "manifest.jsonl" manifest_file.parent.mkdir(parents=True, exist_ok=True) - if not dry_run: - try: - sdist_path = build_sdist_package() - except Exception as e: - logger.error(f"Failed to pre-build sdist: {e}. Will fall back to per-build sdist.") - sdist_path = None - else: - sdist_path = None - if dry_run: print("\n".join(base_images)) return 0 @@ -599,118 +622,125 @@ def _chunks(seq: list[str], size: int): batches = list(_chunks(base_images, batch_size or len(base_images))) total_batches = len(batches) - with ( - manifest_file.open("w") as writer, - tqdm( - total=len(base_images), desc="Building agent-server images", leave=True - ) as pbar, - ): - _update_pbar(pbar, successes, failures, 0, None, "Queueing") - - for batch_idx, batch in enumerate(batches, start=1): - if not batch: - continue + # Use context manager to ensure sdist temp directory is cleaned up + with build_sdist_package() as sdist_path: + with ( + manifest_file.open("w") as writer, + tqdm( + total=len(base_images), desc="Building agent-server images", leave=True + ) as pbar, + ): + _update_pbar(pbar, successes, failures, 0, None, "Queueing") + + for batch_idx, batch in enumerate(batches, start=1): + if not batch: + continue - logger.info( - "Starting batch %d/%d (%d images)", batch_idx, total_batches, len(batch) - ) - in_progress: set[str] = set() - - with ProcessPoolExecutor(max_workers=max_workers) as ex: - futures = {} - for base in batch: - in_progress.add(base) - resolved_tag = ( - base_image_to_custom_tag_fn(base) - if base_image_to_custom_tag_fn - else "" - ) - fut = ex.submit( - _build_with_logging, - log_dir=build_log_dir, - base_image=base, - target_image=image, - custom_tag=resolved_tag, - target=target, - push=push, - max_retries=max_retries, - post_build_fn=post_build_fn, - sdist_path=sdist_path, - ) - futures[fut] = base - - _update_pbar( - pbar, - successes, - failures, - len(in_progress), - next(iter(in_progress), None), - f"Batch {batch_idx}/{total_batches} running", + logger.info( + "Starting batch %d/%d (%d images)", + batch_idx, + total_batches, + len(batch), ) + in_progress: set[str] = set() + + with ProcessPoolExecutor(max_workers=max_workers) as ex: + futures = {} + for base in batch: + in_progress.add(base) + resolved_tag = ( + base_image_to_custom_tag_fn(base) + if base_image_to_custom_tag_fn + else "" + ) + fut = ex.submit( + _build_with_logging, + log_dir=build_log_dir, + base_image=base, + target_image=image, + custom_tag=resolved_tag, + target=target, + push=push, + max_retries=max_retries, + post_build_fn=post_build_fn, + sdist_path=sdist_path, + ) + futures[fut] = base - for fut in as_completed(futures): - base = futures[fut] - status = None - try: - result: BuildOutput = fut.result() - except Exception as e: - logger.error("Build failed for %s: %r", base, e) - result = BuildOutput(base_image=base, tags=[], error=repr(e)) - - writer.write(result.model_dump_json() + "\n") - writer.flush() - - with mu: - if result.error or not result.tags: - failures += 1 - status = "❌ Failed" - else: - successes += 1 - status = "✅ Done" - - in_progress.discard(base) - pbar.update(1) _update_pbar( pbar, successes, failures, len(in_progress), next(iter(in_progress), None), - status, + f"Batch {batch_idx}/{total_batches} running", ) - used, total = buildkit_disk_usage() - if total > 0: - logger.info( - "BuildKit usage after batch %d/%d: %.2f%% (%0.2f GiB / %0.2f GiB)", - batch_idx, - total_batches, - (used / total) * 100, - used / (1 << 30), - total / (1 << 30), - ) + for fut in as_completed(futures): + base = futures[fut] + status = None + try: + result: BuildOutput = fut.result() + except Exception as e: + logger.error("Build failed for %s: %r", base, e) + result = BuildOutput( + base_image=base, tags=[], error=repr(e) + ) + + writer.write(result.model_dump_json() + "\n") + writer.flush() + + with mu: + if result.error or not result.tags: + failures += 1 + status = "❌ Failed" + else: + successes += 1 + status = "✅ Done" + + in_progress.discard(base) + pbar.update(1) + _update_pbar( + pbar, + successes, + failures, + len(in_progress), + next(iter(in_progress), None), + status, + ) - if prune_keep_storage_gb and prune_keep_storage_gb > 0: - pruned = maybe_prune_buildkit_cache( - keep_storage_gb=prune_keep_storage_gb, - threshold_pct=prune_threshold_pct, - filters=prune_filters, - ) - if pruned: + used, total = buildkit_disk_usage() + if total > 0: logger.info( - "Pruned BuildKit cache after batch %d/%d (keep=%d GiB, threshold=%.1f%%)", + "BuildKit usage after batch %d/%d: %.2f%% (%0.2f GiB / %0.2f GiB)", batch_idx, total_batches, - prune_keep_storage_gb, - prune_threshold_pct, + (used / total) * 100, + used / (1 << 30), + total / (1 << 30), ) - else: - logger.info( - "No prune needed after batch %d/%d (threshold %.1f%%)", - batch_idx, - total_batches, - prune_threshold_pct, + + if prune_keep_storage_gb and prune_keep_storage_gb > 0: + pruned = maybe_prune_buildkit_cache( + keep_storage_gb=prune_keep_storage_gb, + threshold_pct=prune_threshold_pct, + filters=prune_filters, ) + if pruned: + logger.info( + "Pruned BuildKit cache after batch %d/%d (keep=%d GiB, threshold=%.1f%%)", + batch_idx, + total_batches, + prune_keep_storage_gb, + prune_threshold_pct, + ) + else: + logger.info( + "No prune needed after batch %d/%d (threshold %.1f%%)", + batch_idx, + total_batches, + prune_threshold_pct, + ) logger.info( "Done. Built=%d Failed=%d Manifest=%s", successes, From e46379b796294493024fad662256367be71375fc Mon Sep 17 00:00:00 2001 From: openhands Date: Wed, 11 Mar 2026 16:00:04 +0000 Subject: [PATCH 3/3] fix: Store pre-built sdist in build_dir to prevent OOM The sdist reuse optimization stores the pre-built sdist in /tmp by default. On Kubernetes pods, /tmp is often mounted as tmpfs (backed by RAM), which can cause OOM when building many images. This fix: - Adds cache_dir parameter to build_sdist_package() - Stores the sdist in build_dir/sdist-cache/ (disk-backed) instead of /tmp - Keeps the sdist around for the entire build process (no cleanup) The monkey patch (patch_uv_build) still intercepts 'uv build --sdist' calls and copies the pre-built sdist instead of rebuilding, saving ~15s per image. Co-authored-by: openhands --- benchmarks/utils/build_utils.py | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/benchmarks/utils/build_utils.py b/benchmarks/utils/build_utils.py index 420a2e950..922216342 100644 --- a/benchmarks/utils/build_utils.py +++ b/benchmarks/utils/build_utils.py @@ -278,12 +278,17 @@ def get_build_parser() -> argparse.ArgumentParser: @contextlib.contextmanager -def build_sdist_package(): +def build_sdist_package(cache_dir: Path | None = None): """Build the SDK sdist package once and yield its path. This is a context manager that creates a temp directory, builds the sdist, and ensures cleanup when done. + Args: + cache_dir: Directory to store the built sdist. If None, uses a temp directory. + For Kubernetes/memory-constrained environments, pass a disk-backed + directory (e.g., build_dir/sdist-cache) to avoid tmpfs OOM. + Yields: Path | None: Path to the built sdist tarball, or None if SDK path doesn't exist. """ @@ -298,8 +303,14 @@ def build_sdist_package(): yield None return - # Use a unique temp dir for the sdist to avoid conflicts - cache_dir = Path(tempfile.mkdtemp(prefix="oh-sdist-cache-")) + # Use provided cache_dir or fall back to temp directory + if cache_dir is not None: + cache_dir.mkdir(parents=True, exist_ok=True) + should_cleanup = False + else: + cache_dir = Path(tempfile.mkdtemp(prefix="oh-sdist-cache-")) + should_cleanup = True + try: logger.info(f"Building SDK sdist package in {cache_dir}...") @@ -323,8 +334,8 @@ def build_sdist_package(): logger.info(f"Built sdist: {sdist_path}") yield sdist_path finally: - # Clean up the temp directory - if cache_dir.exists(): + # Only clean up if we created the temp directory + if should_cleanup and cache_dir.exists(): logger.info(f"Cleaning up sdist cache: {cache_dir}") shutil.rmtree(cache_dir, ignore_errors=True) @@ -626,8 +637,12 @@ def _chunks(seq: list[str], size: int): batches = list(_chunks(base_images, batch_size or len(base_images))) total_batches = len(batches) + # Use disk-backed directory for sdist cache to avoid OOM on Kubernetes + # where /tmp is often mounted as tmpfs (in-memory). + sdist_cache_dir = build_dir / "sdist-cache" + # Use context manager to ensure sdist temp directory is cleaned up - with build_sdist_package() as sdist_path: + with build_sdist_package(cache_dir=sdist_cache_dir) as sdist_path: with ( manifest_file.open("w") as writer, tqdm(