diff --git a/CLAUDE.md b/CLAUDE.md index 1bc4d108da7..b8d48265f8b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,6 +43,8 @@ When you fix violations gated by `ruff-strict-budget.json`, `type-discipline-bud `make pre-commit` always saves its complete output to a per-worktree log file and prints that path as its first and last output lines. To inspect a run, read or grep that log instead of re-running the multi-minute checks just to see a different slice, and re-run only after the working tree actually changed +`make pre-commit` runs every gating CI check except basedpyright, which lives on `make typecheck` (both the `litellm/` budget gate and the zero-error `tests/e2e` pass). Run `make typecheck` too whenever you touch Python, otherwise CI can still come back red on types + If you're trying to create a new function that relies on untyped stuff, instead of adding more Any's and pushing `reportAny` / `reportExplicitAny` closer to their basedpyright ceilings, just validate it in the caller with Pydantic (a model or `TypeAdapter` that returns the typed thing or raises will do) and then pass the now typed variable in If you get an LIT001 or LIT002 fail, refactor the code to follow functional programming best practices rather than introducing mutable data structures. For example, build values in one shot with comprehensions or generators wrapped in `tuple()` / `frozenset()` instead of seeding an empty `list`/`dict`/`set` and mutating it over time. Ideally, `# mutable-ok` is never used; reach for it only as a genuine last resort when an immutable rewrite is truly impossible, and always pair it with a real reason diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d995ddcc87e..397feb1021d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,7 +14,7 @@ Here are the core requirements for any PR submitted to LiteLLM: - [ ] **Add testing** - Adding at least 1 test is a hard requirement - [see details](#adding-testing) - [ ] **Ensure your PR passes all checks**: - [ ] [Unit Tests](#running-unit-tests) - `make test-unit` - - [ ] [Linting / Formatting](#running-linting-and-formatting-checks) - `make lint` + - [ ] [Linting / Formatting](#running-linting-and-formatting-checks) - `make lint` and `make typecheck` #### UI PRs @@ -70,6 +70,7 @@ make format # Run all linting checks (matches CI exactly) make lint +make typecheck # Run unit tests to ensure nothing is broken make test-unit @@ -144,17 +145,19 @@ uv run pytest tests/test_litellm/test_your_file.py -v ### Running Linting and Formatting Checks -Run all linting checks (matches CI exactly): +Run all linting checks (matches CI exactly). `make lint` covers everything except type checking, which is slower and lives on its own target: ```bash make lint +make typecheck ``` Individual linting commands: ```bash make format-check # Check Black formatting make lint-ruff # Run Ruff linting -make lint-basedpyright # Run basedpyright type checking +make lint-basedpyright # Run basedpyright over litellm/ +make lint-e2e-basedpyright # Run basedpyright over tests/e2e make check-circular-imports # Check for circular imports make check-import-safety # Check import safety ``` @@ -184,6 +187,7 @@ To ensure your changes will pass CI, run the exact same checks locally: ```bash # This runs the same checks as the GitHub workflows make lint +make typecheck make test-unit ``` @@ -203,7 +207,8 @@ make install-proxy-dev # Install proxy development dependencies make install-test-deps # Install the full local test environment make format # Apply Black code formatting make format-check # Check Black formatting (matches CI) -make lint # Run all linting checks +make lint # Run all linting checks except type checking +make typecheck # Run basedpyright (litellm/ budget + tests/e2e) make test-unit # Run unit tests make test-integration # Run integration tests make test-unit-helm # Run Helm unit tests @@ -226,7 +231,7 @@ All checks must pass before your PR can be merged. ### 1. Linting Failures -If `make lint` fails: +If `make lint` or `make typecheck` fails: 1. **Formatting issues**: Run `make format` to auto-fix 2. **Ruff issues**: Check the output and fix manually diff --git a/Makefile b/Makefile index 3e82e141c77..a41a59286f6 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,7 @@ .PHONY: help test test-unit test-unit-llms test-unit-proxy-guardrails test-unit-proxy-core test-unit-proxy-misc \ test-unit-integrations test-unit-core-utils test-unit-other test-unit-root \ test-proxy-unit-a test-proxy-unit-b test-integration test-unit-helm \ - info lint lint-dev lint-checks format \ + info lint lint-dev lint-checks format typecheck typecheck-checks \ lint-basedpyright lint-e2e-basedpyright lint-basedpyright-budget-update lint-type-discipline lint-type-discipline-budget-update \ lint-ruff-budget lint-ruff-budget-update lint-budget-update lint-gate \ install-dev install-proxy-dev install-test-deps install-hooks \ @@ -23,9 +23,10 @@ help: @echo " make install-helm-unittest - Install helm unittest plugin" @echo " make install-hooks - Install git hooks (Conventional Commits + Branches)" @echo " make pre-commit - Run CI-equivalent lint on staged files (run before committing)" + @echo " make typecheck - Run every basedpyright pass (litellm budget + tests/e2e)" @echo " make format - Apply ruff format code formatting" @echo " make format-check - Check ruff format code formatting (matches CI)" - @echo " make lint - Run all linting (Ruff, basedpyright, format check, circular imports, import safety)" + @echo " make lint - Run all non-basedpyright linting (Ruff, format check, budgets, circular imports, import safety)" @echo " make lint-ruff - Run Ruff linting only" @echo " make lint-basedpyright - Run basedpyright strict, gated by per-rule error counts" @echo " make lint-e2e-basedpyright - Run basedpyright over tests/e2e (zero errors allowed)" @@ -220,18 +221,30 @@ check-circular-imports: $(LINT_DEP_INSTALL) check-import-safety: $(LINT_DEP_INSTALL) @$(UV_RUN) python -c "from litellm import *; print('[from litellm import *] OK! no issues!');" || (echo '🚨 import failed, this means you introduced unprotected imports! 🚨'; exit 1) -# Combined linting, isomorphic to test-linting.yml's lint job so a local pass means a -# green CI lint: it installs the same env (proxy-dev + generated Prisma client) and then -# runs the diff-scoped ruff format check, whole-tree ruff check, the strict-rule / -# type-discipline / basedpyright budgets as a delta vs the base, then the circular-import -# and import-safety checks. Steps that compare against the base resolve it the same way CI +# Every basedpyright pass test-linting.yml runs, split out of `lint` (and so out of +# `make pre-commit`) because it dominates the wall clock: the rest of the lint job +# finishes in seconds while basedpyright takes minutes, and the two answer different +# questions, so a dev fixing a ruff nit shouldn't have to sit through a type check. +# `make lint && make typecheck` is the CI-parity pair. Setup runs once here, then a +# sub-make fans the two passes out with the dep vars emptied, same as `lint`. +typecheck: lint-install lint-fetch-base + $(MAKE) -j $(LINT_JOBS) $(LINT_OUTPUT_SYNC) LINT_DEP_INSTALL= LINT_E2E_DEP_INSTALL= LINT_DEP_BASE= typecheck-checks + +typecheck-checks: lint-basedpyright lint-e2e-basedpyright + +# Combined linting: test-linting.yml's lint job minus its basedpyright steps (those are +# `make typecheck`). It installs the same env (proxy-dev + generated Prisma client) and +# then runs the diff-scoped ruff format check, whole-tree ruff check, the strict-rule and +# type-discipline budgets as a delta vs the base, then the circular-import and +# import-safety checks. Steps that compare against the base resolve it the same way CI # does (merge-base with origin/litellm_internal_staging). Setup (env sync, Prisma client, # base fetch) runs once up front; the checks themselves are independent, so a sub-make -# fans them out with -j and the fast ones finish under basedpyright's shadow. +# fans them out with -j. lint: lint-install lint-fetch-base $(MAKE) -j $(LINT_JOBS) $(LINT_OUTPUT_SYNC) LINT_DEP_INSTALL= LINT_E2E_DEP_INSTALL= LINT_DEP_BASE= lint-checks + @echo "lint: clean. basedpyright is not part of this target; run 'make typecheck' for it." -lint-checks: lint-format-check-changed lint-ruff lint-gate lint-type-discipline lint-basedpyright lint-e2e-basedpyright check-circular-imports check-import-safety +lint-checks: lint-format-check-changed lint-ruff lint-gate lint-type-discipline check-circular-imports check-import-safety # Faster linting for local development (only checks changed code) lint-dev: lint-format-changed check-circular-imports check-import-safety @@ -239,6 +252,7 @@ lint-dev: lint-format-changed check-circular-imports check-import-safety # Run the gating CI checks against your staged files right before committing. Mirrors # test-linting.yml (Python), test-litellm-ui-build.yml's frontend-lint (dashboard), and # check-ui-api-types.yml (API-type drift), skipping any whose files you didn't stage. +# basedpyright is the one CI check it leaves out; run `make typecheck` for that. # Not auto-installed as a git hook so it never slows an unrelated human commit. pre-commit: bootstrap ./scripts/pre_commit_lint.sh diff --git a/README.md b/README.md index 32b0160dbaa..2442f5331ac 100644 --- a/README.md +++ b/README.md @@ -614,7 +614,8 @@ git clone https://github.com/BerriAI/litellm.git cd litellm make install-dev # Install development dependencies make format # Format your code -make lint # Run all linting checks +make lint # Run all linting checks except type checking +make typecheck # Run basedpyright type checking make test-unit # Run unit tests make format-check # Check formatting only ``` diff --git a/scripts/pre_commit_lint.sh b/scripts/pre_commit_lint.sh index af8335e0e84..d8f5285620a 100755 --- a/scripts/pre_commit_lint.sh +++ b/scripts/pre_commit_lint.sh @@ -2,17 +2,19 @@ # # pre_commit_lint.sh — shift CI lint left. Run it (via `make pre-commit`) right # before `git commit`; it inspects your staged files and runs only the matching -# gating CI checks, so a clean run means a green CI lint: -# - litellm/ Python staged -> `make lint` (test-linting.yml's lint job) -# - tests/e2e Python staged -> `make lint-e2e-basedpyright` (test-linting.yml's e2e type-check step) -# + raw HTTP client ban (test-code-quality.yml's check_e2e_no_raw_requests) +# gating CI checks, so a clean run plus a clean `make typecheck` means a green CI lint: +# - litellm/ Python staged -> `make lint` (test-linting.yml's lint job, minus basedpyright) +# - tests/e2e Python staged -> raw HTTP client ban (test-code-quality.yml's check_e2e_no_raw_requests) # - dashboard staged -> prettier + eslint + lint budgets (test-litellm-ui-build.yml's frontend-lint) # - proxy/types staged -> regenerate dashboard API types and fail on drift (check-ui-api-types.yml) # -# Each block is skipped when no matching files are staged, so unrelated commits stay -# fast. This is intentionally not auto-installed as a git hook (see scripts/install_git_hooks.sh): -# the dashboard and basedpyright passes can take minutes, so it's run on demand rather -# than firing on every human commit. It is hook-compatible if you want that anyway: +# CI's two basedpyright steps are deliberately not here: they take minutes while +# everything above takes seconds, so they live in `make typecheck`, which this script +# reminds you to run when you stage Python. Each block is skipped when no matching files +# are staged, so unrelated commits stay fast. This is intentionally not auto-installed as +# a git hook (see scripts/install_git_hooks.sh): the dashboard pass can still take a +# while, so it's run on demand rather than firing on every human commit. It is +# hook-compatible if you want that anyway: # `ln -s ../../scripts/pre_commit_lint.sh .git/hooks/pre-commit`. set -eu @@ -97,10 +99,9 @@ EOF # Whole-folder lint budgets, exactly as the frontend-lint job runs them: the # counts are not diff-scoped, so a local pass here means the budget step will # pass in CI too. - report=$(mktemp) + report="$work_dir/eslint-report.json" npx eslint . -f json -o "$report" || true node scripts/check-lint-budgets.mjs "$report" eslint-budgets.json || rc=1 - rm -f "$report" exit $rc ) } @@ -127,29 +128,28 @@ python_checks() { return $rc } +# Every scratch file lives in one directory created before any job starts, so an +# interrupt can't leak one: cleanup is a single rm -rf that doesn't depend on which +# per-file variable happened to be assigned when the signal landed. on_interrupt() { trap - INT TERM - rm -f "${python_log:-}" "${dash_log:-}" "${gen_log:-}" for job_pid in ${python_pid:-} ${dash_pid:-} ${gen_pid:-}; do kill -- "-$job_pid" 2>/dev/null || true done + rm -rf "${work_dir:-}" exit 130 } trap on_interrupt INT TERM +work_dir=$(mktemp -d) if [ -n "$litellm_py_files" ]; then - python_log=$(mktemp) + python_log="$work_dir/python.log" set -m python_checks > "$python_log" 2>&1 & python_pid=$! set +m fi -if [ -n "$e2e_py_files" ] && [ -z "$litellm_py_files" ]; then - echo "pre-commit: type-checking tests/e2e (make lint-e2e-basedpyright)" - make lint-e2e-basedpyright || { echo "✗ tests/e2e basedpyright failed. Fix the errors above, then re-run make pre-commit." >&2; status=1; } -fi - if [ -n "$e2e_py_files" ]; then echo "pre-commit: checking tests/e2e raw HTTP client ban (check_e2e_no_raw_requests)" uv run --no-sync python tests/code_coverage_tests/check_e2e_no_raw_requests.py \ @@ -167,7 +167,7 @@ dashboard_checks() { } if [ -n "$ui_prettier_files" ] || [ -n "$ui_eslint_files" ]; then - dash_log=$(mktemp) + dash_log="$work_dir/dashboard.log" set -m dashboard_checks > "$dash_log" 2>&1 & dash_pid=$! @@ -205,7 +205,7 @@ genapi_checks() { } if [ -n "$spec_files" ]; then - gen_log=$(mktemp) + gen_log="$work_dir/genapi.log" set -m genapi_checks > "$gen_log" 2>&1 & gen_pid=$! @@ -214,15 +214,20 @@ fi if [ -n "${python_pid:-}" ]; then wait "$python_pid" || status=1 - cat "$python_log"; rm -f "$python_log" + cat "$python_log" fi if [ -n "${dash_pid:-}" ]; then wait "$dash_pid" || status=1 - cat "$dash_log"; rm -f "$dash_log" + cat "$dash_log" fi if [ -n "${gen_pid:-}" ]; then wait "$gen_pid" || status=1 - cat "$gen_log"; rm -f "$gen_log" + cat "$gen_log" +fi +rm -rf "$work_dir" + +if [ -n "$litellm_py_files" ] || [ -n "$e2e_py_files" ]; then + echo "pre-commit: basedpyright is not part of this run; CI still type-checks. Run: make typecheck" fi exit $status diff --git a/tests/e2e/CONTRIBUTING.md b/tests/e2e/CONTRIBUTING.md index 5999e5772d1..b241b939a7b 100644 --- a/tests/e2e/CONTRIBUTING.md +++ b/tests/e2e/CONTRIBUTING.md @@ -138,7 +138,7 @@ Mark live tests with `@pytest.mark.e2e` (on the class or the module). Pure cover Before you push -1. Run `make lint-e2e-basedpyright` (or `make pre-commit` with your changes staged); the harness is fully typed and the gate allows zero basedpyright errors, enforced in CI on any PR touching `tests/e2e/**/*.py` +1. Run `make typecheck` (or `make lint-e2e-basedpyright` for just this tree); the harness is fully typed and the gate allows zero basedpyright errors, enforced in CI on any PR touching `tests/e2e/**/*.py`. `make pre-commit` covers the rest of the CI lint, but not basedpyright 2. Add the models your test needs to the config your local proxy loads diff --git a/tests/test_litellm/test_makefile_lint_targets.py b/tests/test_litellm/test_makefile_lint_targets.py new file mode 100644 index 00000000000..6b7775ff04f --- /dev/null +++ b/tests/test_litellm/test_makefile_lint_targets.py @@ -0,0 +1,55 @@ +import subprocess +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[2] + +BASEDPYRIGHT_COMMANDS = ( + "uv run --no-sync python scripts/type_check_gate.py --base origin/litellm_internal_staging", + "uv run --no-sync basedpyright tests/e2e", +) + + +def _dry_run(target: str) -> str: + proc = subprocess.run( + ["make", "--dry-run", target], + cwd=ROOT, + capture_output=True, + text=True, + timeout=120, + ) + assert proc.returncode == 0, proc.stdout + proc.stderr + return proc.stdout + + +@pytest.mark.parametrize("command", BASEDPYRIGHT_COMMANDS) +def test_typecheck_runs_every_basedpyright_pass(command: str) -> None: + assert command in _dry_run("typecheck") + + +def test_typecheck_provisions_the_environment_basedpyright_resolves_against() -> None: + output = _dry_run("typecheck") + assert "uv sync --inexact --frozen --group proxy-dev --group e2e-dev" in output + assert "scripts/prisma_generate_if_needed.py" in output + assert "git fetch origin litellm_internal_staging" in output + + +@pytest.mark.parametrize("command", BASEDPYRIGHT_COMMANDS) +def test_lint_leaves_basedpyright_to_typecheck(command: str) -> None: + assert command not in _dry_run("lint") + + +@pytest.mark.parametrize( + "command", + [ + "ruff format --check", + "ruff check .", + "scripts/ruff_strict_gate.py --base origin/litellm_internal_staging", + "scripts/type_discipline_gate.py --base origin/litellm_internal_staging", + "tests/documentation_tests/test_circular_imports.py", + "from litellm import *", + ], +) +def test_lint_still_runs_the_other_gating_ci_checks(command: str) -> None: + assert command in _dry_run("lint") diff --git a/tests/test_litellm/test_pre_commit_lint.py b/tests/test_litellm/test_pre_commit_lint.py index 274d3c517f6..3e9f5a4b55d 100644 --- a/tests/test_litellm/test_pre_commit_lint.py +++ b/tests/test_litellm/test_pre_commit_lint.py @@ -29,6 +29,7 @@ BARRIER_HELPER = """barrier_sync() { MAKE_STUB = """#!/bin/sh . "$STUB_BIN/barrier.sh" +echo "$*" >> "$STUB_CALLS" case "$*" in lint) [ "${STUB_FAIL:-}" = "make-lint" ] && exit 1 @@ -52,6 +53,12 @@ case "$*" in "eslint --no-warn-ignored"*) [ "${STUB_FAIL:-}" = "eslint" ] && exit 1 ;; + "eslint . -f json"*) + if [ -n "${STUB_HANG_ESLINT_DIR:-}" ]; then + touch "$STUB_HANG_ESLINT_DIR/eslint.started" + sleep 60 + fi + ;; esac exit 0 """ @@ -85,11 +92,15 @@ def _write_executable(path: Path, body: str) -> None: path.chmod(0o755) -def _sandbox(tmp_path: Path) -> tuple[Path, Path]: +def _sandbox(tmp_path: Path, *, litellm_python: bool = True, e2e_python: bool = False) -> tuple[Path, Path]: repo = tmp_path / "repo" (repo / "litellm" / "proxy").mkdir(parents=True) - (repo / "litellm" / "foo.py").write_text("x = 1\n") - (repo / "litellm" / "proxy" / "spec.py").write_text("y = 2\n") + if litellm_python: + (repo / "litellm" / "foo.py").write_text("x = 1\n") + (repo / "litellm" / "proxy" / "spec.py").write_text("y = 2\n") + if e2e_python: + (repo / "tests" / "e2e").mkdir(parents=True) + (repo / "tests" / "e2e" / "test_flow.py").write_text("z = 3\n") dashboard = repo / "ui" / "litellm-dashboard" (dashboard / "src").mkdir(parents=True) (dashboard / "node_modules").mkdir() @@ -113,10 +124,22 @@ def _env(repo: Path, bin_dir: Path, extra_env: dict[str, str]) -> dict[str, str] "PATH": os.pathsep.join([str(bin_dir), "/usr/bin", "/bin"]), "HOME": str(repo.parent), "STUB_BIN": str(bin_dir), + "STUB_CALLS": str(_calls_file(bin_dir)), **extra_env, } +def _calls_file(bin_dir: Path) -> Path: + return bin_dir.parent / "make_calls.log" + + +def _make_calls(bin_dir: Path) -> list[str]: + calls = _calls_file(bin_dir) + if not calls.exists(): + return [] + return [line for line in calls.read_text().splitlines() if line] + + def _run(repo: Path, bin_dir: Path, extra_env: dict[str, str]) -> subprocess.CompletedProcess[str]: env = _env(repo, bin_dir, extra_env) return subprocess.run( @@ -142,6 +165,30 @@ def test_python_dashboard_and_gen_api_blocks_run_concurrently_with_grouped_outpu assert python_at < dashboard_at < gen_api_at +def test_staged_litellm_python_lints_without_type_checking_and_points_at_typecheck(tmp_path: Path) -> None: + repo, bin_dir = _sandbox(tmp_path) + proc = _run(repo, bin_dir, {}) + assert proc.returncode == 0, proc.stdout + proc.stderr + assert _make_calls(bin_dir) == ["lint"] + assert "make typecheck" in proc.stdout + + +def test_staged_e2e_python_skips_basedpyright_but_keeps_the_raw_http_ban(tmp_path: Path) -> None: + repo, bin_dir = _sandbox(tmp_path, litellm_python=False, e2e_python=True) + proc = _run(repo, bin_dir, {}) + assert proc.returncode == 0, proc.stdout + proc.stderr + assert _make_calls(bin_dir) == [] + assert "raw HTTP client ban" in proc.stdout + assert "make typecheck" in proc.stdout + + +def test_commit_without_python_gets_no_typecheck_reminder(tmp_path: Path) -> None: + repo, bin_dir = _sandbox(tmp_path, litellm_python=False) + proc = _run(repo, bin_dir, {}) + assert proc.returncode == 0, proc.stdout + proc.stderr + assert "typecheck" not in proc.stdout + + def test_all_blocks_passing_exits_zero(tmp_path: Path) -> None: repo, bin_dir = _sandbox(tmp_path) proc = _run(repo, bin_dir, {}) @@ -227,6 +274,31 @@ def test_interrupt_kills_background_jobs_and_removes_logs(tmp_path: Path) -> Non os.killpg(proc.pid, signal.SIGTERM) +def test_interrupt_mid_dashboard_leaves_no_scratch_files_behind(tmp_path: Path) -> None: + repo, bin_dir = _sandbox(tmp_path, litellm_python=False) + hang_dir = tmp_path / "hang" + hang_dir.mkdir() + tmp_dir = tmp_path / "tmpdir" + tmp_dir.mkdir() + extra = {"STUB_HANG_ESLINT_DIR": str(hang_dir), "TMPDIR": str(tmp_dir)} + proc = subprocess.Popen( + [str(SCRIPT)], + cwd=repo, + env=_env(repo, bin_dir, extra), + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + start_new_session=True, + ) + try: + assert _wait_until((hang_dir / "eslint.started").exists, 10) + os.killpg(proc.pid, signal.SIGINT) + assert proc.wait(timeout=10) != 0 + assert _wait_until(lambda: list(tmp_dir.iterdir()) == [], 5), list(tmp_dir.iterdir()) + finally: + with suppress(ProcessLookupError, PermissionError): + os.killpg(proc.pid, signal.SIGTERM) + + def test_interrupt_spares_the_invoking_process(tmp_path: Path) -> None: repo, bin_dir = _sandbox(tmp_path) hang_dir = tmp_path / "hang"