fix(install.sh): fix pipx strategy to avoid PEP 668 errors#24327
fix(install.sh): fix pipx strategy to avoid PEP 668 errors#24327shivamrawat1 wants to merge 4 commits intomainfrom
Conversation
Replace direct `pip install` with a two-strategy approach that works on PEP 668 "externally-managed-environment" systems (Homebrew Python, Ubuntu 24.04+, Fedora 39+): Strategy 1 — pipx: try `upgrade` first (preserves existing flags like `--include-deps`), then `runpip` to ensure proxy extras are present, falling back to fresh `install` if not yet managed by pipx. Strategy 2 — venv: create an isolated venv at ~/.litellm/venv with built-in ensurepip (no system pip required). Also fixes: - Wrong `_pipx_home` default (~/.local → ~/.local/share/pipx) - Dead fallback binary search paths that never matched standard installs - PATH hint now computed dynamically from the resolved binary Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR updates Key changes:
Issues found:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| scripts/install.sh | Replaces direct pip install with a two-strategy approach (pipx → venv) to avoid PEP 668 errors. Good overall improvement, but pipx inject litellm litellm[proxy] creates circular pipx metadata that breaks future plain-pipx upgrade flows, and the venv bootstrap doesn't guard against pip absence after venv creation. |
| tests/test_litellm/scripts/test_install_sh.py | New static regression test suite for install.sh. Only runs bash -n for syntax validation, missing a dash/sh syntax check despite the script explicitly supporting POSIX sh invocation via `curl |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Start install.sh] --> B{Python 3.9+ found?}
B -- No --> Z1[die: Python not found]
B -- Yes --> C{pipx on PATH?}
C -- Yes --> D[info: Using pipx]
D --> E{pipx upgrade litellm 2>/dev/null}
E -- Success --> F[pipx inject litellm litellm proxy]
F -- Fail --> G[warn: could not inject proxy extras]
F -- Success --> H[_pipx_upgraded=1]
G --> H
E -- Fail --> I{pipx install litellm proxy}
I -- Success --> H
I -- Fail --> J[warn: pipx install failed]
J --> K[LITELLM_BIN still empty]
H --> L{Search binary at PIPX_BIN_DIR, PIPX_HOME/venvs}
L -- Found --> M[LITELLM_BIN set]
L -- Not found --> N{command -v litellm?}
N -- Yes --> M
N -- No --> O[warn: binary not found, falling back to venv]
O --> K
C -- No --> K
K --> P{LITELLM_BIN empty?}
M --> Q{LITELLM_BIN empty?}
P -- Yes --> R[Strategy 2: venv at ~/.litellm/venv]
R --> S{LITELLM_FORCE_VENV_RECREATE=1?}
S -- Yes --> T[venv --clear]
S -- No --> U{venv dir exists?}
U -- No --> V[venv create]
U -- Yes --> W{bin/python executable?}
W -- No --> T
W -- Yes --> X[reuse existing venv]
T --> Y
V --> Y
X --> Y
Y[pip install --upgrade pip] --> AA[pip install litellm proxy]
AA --> AB[LITELLM_BIN = venv/bin/litellm]
AB --> Q
Q -- No --> AC{litellm binary executable?}
Q -- Yes --> AC
AC -- No --> Z2[die: binary not found]
AC -- Yes --> AD[success banner + PATH hint]
AD --> AE[Setup wizard prompt]
Last reviewed commit: "fix(install.sh): use..."
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Warn and show output when pipx runpip fails to add proxy extras - Fall back to venv when pipx succeeds but litellm binary is missing - Reuse existing venv by default; LITELLM_FORCE_VENV_RECREATE for full reset Made-with: Cursor
|
@greptile review with the new commit that resolves above comments |
- Use yellow for warn() so it is distinct from info() - Document why runpip is used instead of pipx inject (older pipx) - Warn when pipx succeeds but binary is missing before venv fallback Made-with: Cursor
|
@gerptile review with new commit that resolves the raised issues and give the score out of 5. Note this specific PR does not need a test to be added so dont consider that in the score. |
Add unit tests for install script syntax and pipx paths. Made-with: Cursor
|
@greptile review and give a score out of 5 |
| def test_install_sh_passes_bash_syntax_check(): | ||
| result = subprocess.run( | ||
| ["bash", "-n", str(_INSTALL_SH)], | ||
| capture_output=True, | ||
| text=True, | ||
| check=False, | ||
| ) | ||
| assert result.returncode == 0, result.stderr or result.stdout |
There was a problem hiding this comment.
bash -n doesn't validate POSIX sh compatibility
The test only runs bash -n for syntax checking, but the install script's own usage comment says it is invoked as sh (i.e. curl … | sh), and the script explicitly notes that on Ubuntu/Debian dash is used when invoked this way. Bash is significantly more permissive than POSIX sh / dash — for instance, bash -n will happily accept bash-isms like arrays, [[, local declarations, process substitutions, and pipefail that would fail or behave differently under dash.
Since the whole point of the set -eu (not set -euo pipefail) note in the script is POSIX sh compatibility, a bash -n syntax check does not validate the thing the authors are explicitly guarding for. A dash -n or sh -n check (when dash is available) would surface any constructs that only work in bash. Consider running both:
def test_install_sh_passes_bash_syntax_check():
result = subprocess.run(
["bash", "-n", str(_INSTALL_SH)],
capture_output=True, text=True, check=False,
)
assert result.returncode == 0, result.stderr or result.stdout
@pytest.mark.skipif(
subprocess.run(["which", "dash"], capture_output=True).returncode != 0,
reason="dash not available",
)
def test_install_sh_passes_dash_syntax_check():
result = subprocess.run(
["dash", "-n", str(_INSTALL_SH)],
capture_output=True, text=True, check=False,
)
assert result.returncode == 0, result.stderr or result.stdout| if ! _inject_err="$(pipx inject litellm "${LITELLM_PACKAGE}" 2>&1)"; then | ||
| warn "could not inject proxy extras (proxy features may fail until manually fixed)." | ||
| printf '%s\n' "$_inject_err" >&2 | ||
| fi | ||
| unset _inject_err 2>/dev/null || true | ||
| _pipx_upgraded=1 |
There was a problem hiding this comment.
pipx inject of same package skips extras on future upgrades
pipx inject litellm litellm[proxy] records litellm[proxy] as an injected dependency inside the litellm pipx environment's metadata. This has a concrete maintenance consequence: a future plain pipx upgrade litellm only upgrades the main app, not injected packages. Users must know to run pipx upgrade litellm --include-injected to keep the proxy extras in sync with the upgraded core package.
In practice this means:
- A user who installed via this script and later runs
pipx upgrade litellmgets a newerlitellmcore but stale proxy extra dependencies. pipx listshowslitellmappearing as both the main app and an injected package, which is unusual and confusing.
The upgrade note should at minimum be surfaced in the success message or as an info hint. For example, after a successful pipx install path, add:
info "To upgrade later, run: pipx upgrade litellm --include-injected"This ensures users don't lose the proxy extras on their next upgrade cycle.
| "${LITELLM_VENV}/bin/pip" install -q --upgrade pip \ | ||
| || die "Failed to upgrade pip in venv. Try: ${LITELLM_VENV}/bin/pip install --upgrade pip" |
There was a problem hiding this comment.
venv/bin/pip may not exist before pip upgrade step
The venv creation path falls through three branches (lines 137–150): LITELLM_FORCE_VENV_RECREATE=1, fresh directory, and incomplete-venv recreation. After any of them, the script reaches:
"${LITELLM_VENV}/bin/pip" install -q --upgrade pipPython's venv module uses ensurepip to bootstrap pip. On some minimal environments (e.g. Ubuntu/Debian without the python3-venv package and without pip pre-installed), venv can succeed but create the venv without bin/pip — only bin/python and bin/pip3 may be present, depending on the Python build. If ${LITELLM_VENV}/bin/pip does not exist, this line fails with a confusing "not found" error rather than the die message.
The already-existing guard on line 145 ([ ! -x "${LITELLM_VENV}/bin/python" ]) checks for python, not pip. Consider adding an explicit check after venv creation:
if [ ! -x "${LITELLM_VENV}/bin/pip" ] && [ ! -x "${LITELLM_VENV}/bin/pip3" ]; then
die "pip was not bootstrapped in the venv. Try: sudo apt install python3-pip python3-venv, then re-run."
fi
VENV_PIP="${LITELLM_VENV}/bin/pip3"
[ -x "${LITELLM_VENV}/bin/pip" ] && VENV_PIP="${LITELLM_VENV}/bin/pip"
"${VENV_PIP}" install -q --upgrade pip ...1 participant
Updates scripts/install.sh so installing litellm[proxy] works on PEP 668 “externally managed environment” setups (e.g. Homebrew Python, many Linux distros) where pip install into the system/user interpreter is blocked or discouraged.
What changed
Strategy A — pipx (when available)
Prefer pipx upgrade litellm so existing pipx installs keep their options (e.g. --include-deps).
After a successful upgrade, run pipx runpip litellm install "litellm[proxy]" so proxy extras are present even if the user originally installed plain litellm (non-fatal if it fails).
If nothing is installed under pipx yet, fall back to pipx install litellm[proxy].
Strategy B — dedicated venv (~/.litellm/venv by default, overridable via LITELLM_VENV)
python -m venv --clear, upgrade pip inside the venv, then pip install litellm[proxy].
Avoids touching the system Python’s site-packages.
Removed the unconditional “system pip must exist” gate; pipx manages its own envs and the venv path uses the venv’s bundled pip/ensurepip.
pipx discovery fixes
Default PIPX_HOME fallback is ~/.local/share/pipx (not ~/.local), matching standard pipx layout.
Binary search uses PIPX_BIN_DIR and $PIPX_HOME/venvs/litellm/bin/litellm (removed the mistaken extra path segment).
PATH hint uses the directory of the resolved litellm binary (dirname), not a hardcoded scripts path.
Cause
PEP 668: OS/distro Python is marked “externally managed”; pip install to that environment fails or is unsafe. The old script assumed python -m pip install always worked after a pip check.
Wrong pipx defaults: Treating PIPX_HOME as ~/.local made fallback paths wrong and error messages misleading; standard pipx stores venvs under ~/.local/share/pipx.
pipx install --force: Reinstalling without preserving prior pipx flags could drop behavior such as --include-deps for users who had used it before.
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🆕 New Feature
🐛 Bug Fix
Changes