From 4e497f4878f94aae9bc0050ee418f8d70302caa3 Mon Sep 17 00:00:00 2001 From: Cookiejunky <131141009+Cookiejunky@users.noreply.github.com> Date: Tue, 9 Jun 2026 00:10:20 +0300 Subject: [PATCH] fix(cookbook): guard break-system-packages pip flag (#3510) --- routes/cookbook_helpers.py | 89 +++++++++++++++++++++++++++++----- routes/cookbook_routes.py | 23 ++++++--- tests/test_cookbook_helpers.py | 46 ++++++++++++++++-- 3 files changed, 135 insertions(+), 23 deletions(-) diff --git a/routes/cookbook_helpers.py b/routes/cookbook_helpers.py index a154f3718..39a18f715 100644 --- a/routes/cookbook_helpers.py +++ b/routes/cookbook_helpers.py @@ -197,6 +197,20 @@ def _pip_install_attempt(pip_cmd: str) -> str: ) +def _pip_command(python_cmd: str) -> str: + """Return a pip command for either a pip executable or a Python executable.""" + cmd = python_cmd.strip() + if " -m pip" in cmd or cmd in {"pip", "pip3"}: + return python_cmd + if cmd in {"python", "python3", "python.exe"} or cmd.endswith(("/python", "/python3", "\\python.exe")): + return f"{python_cmd} -m pip" + return python_cmd + + +def _pip_break_system_packages_check(pip_cmd: str) -> str: + return f"{pip_cmd} install --help 2>/dev/null | grep -q -- --break-system-packages" + + 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. @@ -221,27 +235,31 @@ def _pip_install_fallback_chain(package: str, *, python_cmd: str = "python3 -m p if "llama-cpp-python" in package: pkg += " --extra-index-url https://abetlen.github.io/llama-cpp-python/whl/cpu" - base = _pip_install_attempt(f"{python_cmd} install -q{upgrade_flag} {pkg}") - user = _pip_install_attempt(f"{python_cmd} install --user --break-system-packages -q{upgrade_flag} {pkg}") + pip_cmd = _pip_command(python_cmd) + base = _pip_install_attempt(f"{pip_cmd} install -q{upgrade_flag} {pkg}") + user = _pip_install_attempt(f"{pip_cmd} install --user -q{upgrade_flag} {pkg}") + user_break_system = _pip_install_attempt(f"{pip_cmd} install --user --break-system-packages -q{upgrade_flag} {pkg}") + user_fallback = f"( {user} || {{ {_pip_break_system_packages_check(pip_cmd)} && {user_break_system}; }} )" # 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". - if " -m pip" in python_cmd: - python_exe = python_cmd.replace(" -m pip", "") - elif python_cmd.strip() == "pip": + if " -m pip" in pip_cmd: + python_exe = pip_cmd.replace(" -m pip", "") + elif pip_cmd.strip() == "pip": python_exe = "python" - elif python_cmd.strip() == "pip3": + elif pip_cmd.strip() == "pip3": python_exe = "python3" else: python_exe = "python3" venv_check = f'{python_exe} -c "import sys; sys.exit(0 if sys.prefix != sys.base_prefix else 1)"' - # Negated: `! venv_check` succeeds (exit 0) when NOT in a venv → `&&` tries - # --user. When IN a venv `! venv_check` fails → `&&` skips --user and the + # 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}; }}" + # result). `--break-system-packages` is only attempted when the active pip + # supports it; older pip versions abort with "no such option" otherwise. + return f"{base} || {{ ! {venv_check} && {user_fallback}; }}" def _venv_safe_local_pip_install_cmd(cmd: str, *, local: bool, in_venv: bool) -> str: @@ -272,6 +290,55 @@ def _venv_safe_local_pip_install_cmd(cmd: str, *, local: bool, in_venv: bool) -> return shlex.join(stripped) +def _pip_install_command_without_break_system_packages(cmd: str) -> str: + try: + parts = shlex.split(cmd) + except ValueError: + return cmd + stripped = [part for part in parts if part != "--break-system-packages"] + return shlex.join(stripped) + + +def _pip_install_help_check_from_cmd(cmd: str) -> str | None: + try: + parts = shlex.split(cmd) + except ValueError: + return None + try: + install_index = parts.index("install") + except ValueError: + return None + if install_index <= 0: + return None + pip_prefix = parts[:install_index] + return f"{shlex.join(pip_prefix + ['install', '--help'])} 2>/dev/null | grep -q -- --break-system-packages" + + +def _append_pip_install_runner_lines(runner_lines: list[str], cmd: str) -> None: + """Append a pip install command, guarding --break-system-packages support. + + The Dependencies UI may submit ``python3 -m pip install --user + --break-system-packages ...`` for non-venv installs. That flag is useful on + PEP-668-locked distros, but older pip (including Ubuntu 22.04's apt pip in + the NVIDIA CUDA base image) aborts with "no such option". Branch at runner + time so stale browser JS and remote targets are handled by the server too. + """ + if "--break-system-packages" not in (cmd or ""): + runner_lines.append(cmd) + return + help_check = _pip_install_help_check_from_cmd(cmd) + without_break = _pip_install_command_without_break_system_packages(cmd) + if not help_check or without_break == cmd: + runner_lines.append(cmd) + return + runner_lines.append(f"if {help_check}; then") + runner_lines.append(f" {cmd}") + runner_lines.append("else") + runner_lines.append(' echo "[odysseus] pip does not support --break-system-packages; installing without it."') + runner_lines.append(f" {without_break}") + runner_lines.append("fi") + + def _user_shell_path_bootstrap() -> list[str]: return [ 'ODYSSEUS_USER_SHELL="${SHELL:-}"', @@ -1034,4 +1101,4 @@ async def run_ssh_command_async( proc.kill() await proc.communicate() raise - return proc.returncode or 0, stdout, stderr \ No newline at end of file + return proc.returncode or 0, stdout, stderr diff --git a/routes/cookbook_routes.py b/routes/cookbook_routes.py index 228660ef3..7a1ee85c6 100644 --- a/routes/cookbook_routes.py +++ b/routes/cookbook_routes.py @@ -47,6 +47,7 @@ from routes.cookbook_helpers import ( _append_serve_exit_code_lines, _append_llama_cpp_linux_accel_build_lines, _cached_model_scan_script, _append_vllm_linux_preflight_lines, _ollama_bind_from_cmd, _pip_install_fallback_chain, _pip_install_no_cache, _user_shell_path_bootstrap, _venv_safe_local_pip_install_cmd, + _append_pip_install_runner_lines, _diagnose_serve_output, run_ssh_command_async, ModelDownloadRequest, ServeRequest, ) @@ -435,7 +436,7 @@ def setup_cookbook_routes() -> APIRouter: # Install hf CLI + optional hf_transfer best-effort. Retries disable # hf_transfer because the Rust parallel path is fast but has been # flaky near the end of very large multi-file downloads. - # Use --break-system-packages on PEP-668 systems (Arch, newer Debian) so it doesn't bail. + # The helper tries active pip first, then guarded user-site fallbacks. runner_lines.append(f"command -v hf >/dev/null 2>&1 || {_pip_install_fallback_chain('huggingface_hub', python_cmd='pip', upgrade=True)}") if req.disable_hf_transfer: runner_lines.append("export HF_HUB_ENABLE_HF_TRANSFER=0") @@ -1177,7 +1178,10 @@ def setup_cookbook_routes() -> APIRouter: runner_lines, keep_shell_open=not local_windows, ) - runner_lines.append(req.cmd) + if is_pip_install: + _append_pip_install_runner_lines(runner_lines, req.cmd) + else: + runner_lines.append(req.cmd) if local_windows: # Detached background process — no interactive shell to keep open. # Print the exit marker the status poller looks for, then stop. @@ -1338,8 +1342,8 @@ def setup_cookbook_routes() -> APIRouter: cmd = f"ssh {pf}{host} '{setup_script}'" else: # Linux: auto-install tmux (via whichever package manager is available) - # and huggingface_hub + hf_transfer (falling back to --user/--break-system-packages - # on PEP-668 locked distros like Arch / newer Debian). + # and huggingface_hub + hf_transfer (falling back to --user, then + # guarded --break-system-packages on PEP-668 locked distros). setup_script = ( # Install tmux if missing — try common package managers; skip if no sudo "if ! command -v tmux >/dev/null 2>&1; then " @@ -1351,10 +1355,15 @@ def setup_cookbook_routes() -> APIRouter: " fi; " "fi; " "command -v tmux >/dev/null 2>&1 || echo 'WARNING: tmux missing and auto-install failed (need passwordless sudo). Install manually.'; " - # Install Python bits. Try system install first; fall back to --user --break-system-packages on PEP 668 systems. + # Install Python bits. Try system install first; fall back to --user, + # then use --break-system-packages only when pip supports it. "pip install -q huggingface_hub hf_transfer 2>/dev/null || " - "pip install --user --break-system-packages -q huggingface_hub hf_transfer 2>/dev/null || " - "pip3 install --user --break-system-packages -q huggingface_hub hf_transfer 2>/dev/null; " + "pip install --user -q huggingface_hub hf_transfer 2>/dev/null || " + "( pip install --help 2>/dev/null | grep -q -- --break-system-packages && " + "pip install --user --break-system-packages -q huggingface_hub hf_transfer 2>/dev/null ) || " + "pip3 install --user -q huggingface_hub hf_transfer 2>/dev/null || " + "( pip3 install --help 2>/dev/null | grep -q -- --break-system-packages && " + "pip3 install --user --break-system-packages -q huggingface_hub hf_transfer 2>/dev/null ); " "python3 -c 'from huggingface_hub import snapshot_download; print(\"OK\")'" ) cmd = f"ssh {pf}{host} '{setup_script}'" diff --git a/tests/test_cookbook_helpers.py b/tests/test_cookbook_helpers.py index 5666a7fd2..2a5f4b715 100644 --- a/tests/test_cookbook_helpers.py +++ b/tests/test_cookbook_helpers.py @@ -9,6 +9,7 @@ from fastapi import HTTPException from routes.cookbook_helpers import ( _cached_model_scan_script, _append_llama_cpp_linux_accel_build_lines, + _append_pip_install_runner_lines, _append_serve_exit_code_lines, _append_serve_preflight_exit_lines, _llama_cpp_rebuild_cmd, @@ -148,7 +149,9 @@ def test_pip_install_fallback_chain_prefers_venv_safe_install(): # 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 + # Fallback: --user first, then guarded --break-system-packages for PEP-668 pip. + assert "python3 -m pip install --user -q -U huggingface_hub" in chain + assert "python3 -m pip install --help 2>/dev/null | grep -q -- --break-system-packages" in chain 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) @@ -163,11 +166,23 @@ def test_pip_install_fallback_chain_prefers_venv_safe_install(): def test_pip_install_fallback_chain_allows_custom_python_command(): chain = _pip_install_fallback_chain("hf_transfer", python_cmd="pip", upgrade=False) assert "pip install -q hf_transfer" in chain + assert "pip install --user -q hf_transfer" in chain + assert "pip install --help 2>/dev/null | grep -q -- --break-system-packages" 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 + # All install attempts are wrapped in bash -c subshells + assert chain.count("bash -c '") == 3 + + +def test_pip_install_fallback_chain_accepts_python_executable(): + chain = _pip_install_fallback_chain("llama-cpp-python[server]", python_cmd="python") + + assert "python -m pip install -q 'llama-cpp-python[server]'" in chain + assert "python -m pip install --user -q 'llama-cpp-python[server]'" in chain + assert "python -m pip install --help 2>/dev/null | grep -q -- --break-system-packages" in chain + assert "python install " not in chain + assert 'python -c "import sys; sys.exit(0 if sys.prefix != sys.base_prefix else 1)"' in chain def test_pip_install_fallback_chain_propagates_failure_in_venv(): @@ -219,8 +234,8 @@ def test_pip_install_fallback_chain_quotes_extras_spec(): (which pulls in starlette_context for ``python -m llama_cpp.server``) is actually installed instead of a bare ``llama-cpp-python`` (issue #730).""" chain = _pip_install_fallback_chain("llama-cpp-python[server]", python_cmd="pip") - # Quoted in both the plain and the --user attempt. - assert chain.count("'llama-cpp-python[server]'") == 2 + # Quoted in the plain, --user, and guarded --break-system-packages attempts. + assert chain.count("'llama-cpp-python[server]'") == 3 # llama-cpp installs must prefer prebuilt wheels to avoid fragile source builds. assert "--extra-index-url https://abetlen.github.io/llama-cpp-python/whl/cpu" in chain # Never the unquoted form (bracket-glob risk). @@ -281,6 +296,27 @@ def test_venv_safe_local_pip_install_strips_user_flags_only_for_local_venv(): assert _venv_safe_local_pip_install_cmd(cmd, local=True, in_venv=False) == cmd +def test_pip_install_runner_guards_break_system_packages(): + lines = [] + _append_pip_install_runner_lines( + lines, + 'python3 -m pip install --no-cache-dir --user --break-system-packages "llama-cpp-python[server]"', + ) + script = "\n".join(lines) + + assert "python3 -m pip install --help 2>/dev/null | grep -q -- --break-system-packages" in script + assert 'python3 -m pip install --no-cache-dir --user --break-system-packages "llama-cpp-python[server]"' in script + assert "python3 -m pip install --no-cache-dir --user 'llama-cpp-python[server]'" in script + assert "pip does not support --break-system-packages" in script + + +def test_pip_install_runner_leaves_plain_commands_unchanged(): + lines = [] + _append_pip_install_runner_lines(lines, "python3 -m pip install --no-cache-dir vllm") + + assert lines == ["python3 -m pip install --no-cache-dir vllm"] + + 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."""