From 996a2027ddb1f4fcc8ca0291d5a4a8e2a9a36ae5 Mon Sep 17 00:00:00 2001 From: Ernest Hysa <59969602+ErnestHysa@users.noreply.github.com> Date: Tue, 2 Jun 2026 12:34:52 +0100 Subject: [PATCH] Cookbook: surface pip install failures in logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _pip_install_fallback_chain silently discarded pip stderr via 2>/dev/null on every attempt. When pip failed (network error, venv mismatch, disk full), the wrapper exited 0 and the Cookbook UI showed the download as running — the silent-failure mode from #354. Extract _pip_install_attempt() which wraps each pip invocation in a bash -c subshell that captures output to a temp file, prints tail -5 on failure, cleans up, and exits with pip's real exit code. This avoids the | tail pipefail masking (the first blocker on #363) while surfacing the last 5 lines of pip output in the tmux log so users can see what went wrong. Both local wrapper and remote SSH runner use the same helper through _pip_install_fallback_chain, so the fix is symmetric. --- routes/cookbook_helpers.py | 45 +++++++++--- tests/test_cookbook_helpers.py | 125 +++++++++++++++++++++++++++++++-- 2 files changed, 156 insertions(+), 14 deletions(-) diff --git a/routes/cookbook_helpers.py b/routes/cookbook_helpers.py index 17714ee..9e9e8ce 100644 --- a/routes/cookbook_helpers.py +++ b/routes/cookbook_helpers.py @@ -148,15 +148,39 @@ def _local_tooling_path_export(executable: str) -> str: return f'export PATH="{esc}:$PATH"' -def _pip_install_fallback_chain(package: str, *, python_cmd: str = "python3 -m pip", upgrade: bool = False) -> str: - """Build a bash pip install fallback chain. +def _pip_install_attempt(pip_cmd: str) -> str: + """Wrap a single pip install command so its exit status survives the + fallback chain and its stderr is visible in the tmux log on failure. - Try the active interpreter/environment first. `--user` is invalid inside - many venvs, so only attempt the --user fallback when NOT inside a venv. + Without this wrapper, `pip … 2>&1 | tail -5` returns ``tail``'s exit + code (0), masking pip's real failure and preventing the next fallback + from running. The generated snippet captures all output to a temp + file, prints the last 5 lines on failure (so the Cookbook log panel + shows useful diagnostics), cleans up, and exits with pip's original + status. + """ + return ( + "bash -c '" + f'_out=$(mktemp) && {pip_cmd} >"$_out" 2>&1; _rc=$?; ' + 'tail -5 "$_out"; rm -f "$_out"; exit $_rc' + "'" + ) + + +def _pip_install_fallback_chain(package: str, *, python_cmd: str = "python3 -m pip", upgrade: bool = False) -> str: + """Build a bash pip install fallback chain that surfaces errors. + + Try the active interpreter/environment first. ``--user`` is invalid + inside many venvs, so only attempt the ``--user`` fallback when NOT + inside a venv. + + Each attempt is wrapped via :func:`_pip_install_attempt` so pip's real + exit code is preserved (no ``| tail`` masking) and the last 5 lines of + pip output appear in the Cookbook log on failure. """ upgrade_flag = " -U" if upgrade else "" - base = f"{python_cmd} install -q{upgrade_flag} {package} 2>/dev/null" - user = f"{python_cmd} install --user --break-system-packages -q{upgrade_flag} {package} 2>/dev/null" + base = _pip_install_attempt(f"{python_cmd} install -q{upgrade_flag} {package}") + user = _pip_install_attempt(f"{python_cmd} install --user --break-system-packages -q{upgrade_flag} {package}") # Derive the python executable for the venv detection check. # Must use the same interpreter that pip belongs to; hardcoding # python3 breaks when pip lives in a venv that only has "python". @@ -169,8 +193,13 @@ def _pip_install_fallback_chain(package: str, *, python_cmd: str = "python3 -m p else: python_exe = "python3" venv_check = f'{python_exe} -c "import sys; sys.exit(0 if sys.prefix != sys.base_prefix else 1)"' - # venv_check exits 0 (true) when IN a venv; --user is only valid outside one. - return f"{base} || {{ {venv_check} || {user}; }}" + # Negated: `! venv_check` succeeds (exit 0) when NOT in a venv → `&&` tries + # --user. When IN a venv `! venv_check` fails → `&&` skips --user and the + # group exits non-zero, propagating the base-install failure instead of + # masking it as success (the `|| { venv_check || … }` shape from #903 + # swallowed the exit code because venv_check's exit-0 became the group's + # result). + return f"{base} || {{ ! {venv_check} && {user}; }}" def _cached_model_scan_script(model_dirs: list[str] | None = None) -> str: diff --git a/tests/test_cookbook_helpers.py b/tests/test_cookbook_helpers.py index 073631e..9d88b40 100644 --- a/tests/test_cookbook_helpers.py +++ b/tests/test_cookbook_helpers.py @@ -10,6 +10,7 @@ from routes.cookbook_helpers import ( _append_serve_exit_code_lines, _append_serve_preflight_exit_lines, _local_tooling_path_export, + _pip_install_attempt, _pip_install_fallback_chain, _ollama_bind_from_cmd, _safe_env_prefix, @@ -87,17 +88,129 @@ def test_local_tooling_path_export_preserves_spaces_and_expands_path(): def test_pip_install_fallback_chain_prefers_venv_safe_install(): chain = _pip_install_fallback_chain("huggingface_hub", upgrade=True) - assert chain.startswith("python3 -m pip install -q -U huggingface_hub") - assert "|| python3 -m pip install --user --break-system-packages -q -U huggingface_hub" in chain + # First attempt: plain install, wrapped in status-preserving subshell + assert chain.startswith("bash -c '") + assert "python3 -m pip install -q -U huggingface_hub" in chain + # Second attempt: --user --break-system-packages, also wrapped + assert "--user --break-system-packages" in chain + assert "python3 -m pip install --user --break-system-packages -q -U huggingface_hub" in chain + # No bare `| tail` (which would mask pip's exit code) + assert "| tail" not in chain + # Negated venv check with && — so failure in a venv propagates instead of + # being masked as success by the venv_check's exit-0. + assert "! python3 -c" in chain + # The group uses && (not ||) between venv check and user attempt + assert "&&" in chain def test_pip_install_fallback_chain_allows_custom_python_command(): chain = _pip_install_fallback_chain("hf_transfer", python_cmd="pip", upgrade=False) - assert chain == ( - 'pip install -q hf_transfer 2>/dev/null || { ' - 'python -c "import sys; sys.exit(0 if sys.prefix != sys.base_prefix else 1)"' - ' || pip install --user --break-system-packages -q hf_transfer 2>/dev/null; }' + assert "pip install -q hf_transfer" in chain + assert "pip install --user --break-system-packages -q hf_transfer" in chain + # venv check uses the python executable derived from the pip command + assert 'python -c "import sys; sys.exit(0 if sys.prefix != sys.base_prefix else 1)"' in chain + # Both attempts are wrapped in bash -c subshells + assert chain.count("bash -c '") == 2 + + +def test_pip_install_fallback_chain_propagates_failure_in_venv(): + """When base install fails inside a venv, the chain must exit non-zero. + + The old `{ venv_check || user }` shape from #903 masked the failure: + venv_check exited 0 (in venv), || short-circuited, and the group + reported success even though nothing was installed. The negated + `{ ! venv_check && user }` shape propagates the failure correctly. + """ + import shlex + py = shlex.quote(sys.executable) + # Use the venv python so venv_check detects we're in a venv. + # Base install fails, venv_check exits 0, negated to 1, + # && skips user, group exits 1. + script = ( + f"{py} -c 'import sys; sys.exit(1)' || " + f"{{ ! {py} -c \"import sys; sys.exit(0 if sys.prefix != sys.base_prefix else 1)\" " + f"&& echo user_attempt; }}" ) + result = subprocess.run( + ["bash", "-c", script], + capture_output=True, text=True, timeout=10, + ) + assert "user_attempt" not in result.stdout + assert result.returncode != 0, "Chain should propagate failure when base fails in venv" + + +def test_pip_install_fallback_chain_tries_user_outside_venv(): + """When base install fails outside a venv, the chain should try --user.""" + # Force "not in venv" by making venv_check return 1 directly. + script = ( + "bash -c '" + "python3 -c \"import sys; sys.exit(1)\" || " + "{ ! python3 -c \"import sys; sys.exit(1)\" " # venv_check=1 → negated to 0 → user runs + "&& echo user_attempt; }" + "'" + ) + result = subprocess.run( + ["bash", "-c", script], + capture_output=True, text=True, timeout=10, + ) + assert "user_attempt" in result.stdout, "Chain should try --user when not in venv and base fails" + + +def test_pip_install_attempt_wraps_in_status_preserving_subshell(): + """Each pip attempt must be a bash -c subshell that captures output, + prints tail, cleans up, and exits with pip's real status — not tail's.""" + snippet = _pip_install_attempt("pip install -q huggingface_hub") + assert snippet.startswith("bash -c '") + assert "$(mktemp)" in snippet + assert "_rc=$?" in snippet + assert "tail -5" in snippet + assert "rm -f" in snippet + assert "exit $_rc" in snippet + + +def test_pip_install_attempt_no_bare_pipe_tail(): + """A bare `| tail` pipeline would mask pip's exit code — must not appear.""" + snippet = _pip_install_attempt("pip install -q huggingface_hub") + assert "| tail" not in snippet + + +def test_pip_install_attempt_failure_propagates_real_exit_code(): + """Run the generated snippet against a deliberately broken pip install + to confirm the subshell exits with pip's non-zero status.""" + snippet = _pip_install_attempt("python3 -m pip install __nonexistent_package_12345__") + result = subprocess.run( + ["bash", "-c", snippet], + capture_output=True, + text=True, + timeout=60, + ) + assert result.returncode != 0, "pip install of a nonexistent package should fail" + + +def test_pip_install_attempt_success_exits_zero(): + """When pip succeeds, the subshell should exit 0.""" + snippet = _pip_install_attempt("python3 -c 'pass'") + result = subprocess.run( + ["bash", "-c", snippet], + capture_output=True, + text=True, + timeout=15, + ) + assert result.returncode == 0 + + +def test_pip_install_attempt_surfaces_stderr_on_failure(): + """On failure, the last 5 lines of pip output should appear in stdout.""" + snippet = _pip_install_attempt("python3 -m pip install __nonexistent_package_12345__") + result = subprocess.run( + ["bash", "-c", snippet], + capture_output=True, + text=True, + timeout=60, + ) + # pip's error message should be visible in the output (not swallowed) + combined = result.stdout + result.stderr + assert "nonexistent" in combined.lower() or result.returncode != 0 def test_serve_preflight_failure_keeps_tmux_pane_visible():