mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-21 00:21:49 +00:00
test(rust): pin child interpreters to the parent's litellm and lint for it
Children spawned as [sys.executable, -c, ...] put the working directory first on sys.path, so under 'make test-rust-extension' a source checkout shadows the installed wheel and the child imports a litellm with no compiled extension. A shared helper spawns them with -I and asserts the child resolved the same litellm.__file__ as the parent, and a new TQ009 rule flags un-isolated sys.executable spawns. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
parent
1bcd8d704f
commit
18a1491bd2
6 changed files with 110 additions and 10 deletions
|
|
@ -60,6 +60,13 @@ TQ007 A module global that a conftest saves before every test and restores aft
|
|||
names are read from the keys the conftest assigns directly and from whatever the
|
||||
save loop iterates, including a module-level tuple or dict it names rather than
|
||||
spells out.
|
||||
TQ009 A child interpreter spawned as `subprocess.run([sys.executable, ...])` without
|
||||
`-I`/`-P` as its first flag. Without isolation the child's sys.path leads with
|
||||
the working directory, so a source checkout shadows the installed package and
|
||||
the child tests a different `litellm` than the parent imported -- TQ003 is the
|
||||
same working-directory hazard seen from the child's side. Use
|
||||
tests.test_litellm_rust.support.child_interpreter.run_child_interpreter, which
|
||||
also asserts the child resolved the same `litellm.__file__` as the parent.
|
||||
|
||||
Every rule is suppressible with `# test-quality-ok: <reason>` on the reported
|
||||
line, following the repo's `*-ok: <reason>` convention. A suppression without a
|
||||
|
|
@ -140,6 +147,9 @@ SKIP_CALLS: Final = frozenset(("pytest.skip", "skip"))
|
|||
CONFTEST_NAME: Final = "conftest.py"
|
||||
SDK_MODULE: Final = "litellm"
|
||||
|
||||
SUBPROCESS_SPAWNS: Final = frozenset(("run", "Popen", "check_output", "check_call", "call"))
|
||||
INTERPRETER_ISOLATION_FLAGS: Final = frozenset(("-I", "-P"))
|
||||
|
||||
CREDENTIAL_NAME_RE: Final = re.compile(
|
||||
r"(?:API_KEY|_KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL|DATABASE_URL|ACCESS_KEY_ID)$"
|
||||
)
|
||||
|
|
@ -709,6 +719,35 @@ def _snapshotted_names(tree: ast.Module) -> Iterator[tuple[str, int]]:
|
|||
yield from _string_members(iterable)
|
||||
|
||||
|
||||
def iter_child_interpreter_violations(path: Path, tree: ast.Module) -> Iterator[Violation]:
|
||||
for node in ast.walk(tree):
|
||||
if not (isinstance(node, ast.Call) and node.args):
|
||||
continue
|
||||
if _dotted_name(node.func).rsplit(".", 1)[-1] not in SUBPROCESS_SPAWNS:
|
||||
continue
|
||||
argv: Final = node.args[0]
|
||||
if not isinstance(argv, (ast.List, ast.Tuple)) or not argv.elts:
|
||||
continue
|
||||
if _dotted_name(argv.elts[0]) != "sys.executable":
|
||||
continue
|
||||
isolated: Final = (
|
||||
len(argv.elts) > 1
|
||||
and isinstance(argv.elts[1], ast.Constant)
|
||||
and argv.elts[1].value in INTERPRETER_ISOLATION_FLAGS
|
||||
)
|
||||
if isolated:
|
||||
continue
|
||||
yield Violation(
|
||||
path,
|
||||
node.lineno,
|
||||
"TQ009",
|
||||
"child interpreter spawned without -I/-P; the working directory lands on sys.path "
|
||||
"and a source checkout can shadow the installed package, use "
|
||||
"tests.test_litellm_rust.support.child_interpreter.run_child_interpreter or pass -I "
|
||||
f"(suppress: `# {SUPPRESSION_TOKEN}: <reason>`)",
|
||||
)
|
||||
|
||||
|
||||
def iter_conftest_inventory_violations(path: Path, tree: ast.Module) -> Iterator[Violation]:
|
||||
if path.name != CONFTEST_NAME:
|
||||
return
|
||||
|
|
@ -746,6 +785,7 @@ def check_file(path: Path) -> tuple[Violation, ...]:
|
|||
*iter_credential_skip_violations(path, tree),
|
||||
*iter_conftest_inventory_violations(path, tree),
|
||||
*iter_internal_patch_violations(path, tree),
|
||||
*iter_child_interpreter_violations(path, tree),
|
||||
)
|
||||
if violation.line not in skip
|
||||
)
|
||||
|
|
|
|||
|
|
@ -22,5 +22,8 @@
|
|||
},
|
||||
"TQ008": {
|
||||
"limit": 10993
|
||||
},
|
||||
"TQ009": {
|
||||
"limit": 59
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -11,11 +11,11 @@ def _reserve_with(monkeypatch: pytest.MonkeyPatch, native: object) -> None:
|
|||
|
||||
|
||||
def test_missing_extension_has_nothing_to_reserve(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
_reserve_with(monkeypatch, None)
|
||||
assert _reserve_with(monkeypatch, None) is None
|
||||
|
||||
|
||||
def test_extension_built_before_reservation_existed_passes(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
_reserve_with(monkeypatch, SimpleNamespace())
|
||||
assert _reserve_with(monkeypatch, SimpleNamespace()) is None
|
||||
|
||||
|
||||
def test_unused_extension_is_reserved(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
|
|
|
|||
|
|
@ -737,3 +737,28 @@ def test_a_fanned_out_run_reports_each_generated_file_exactly_once(tmp_path):
|
|||
assert len(reported) == len(paths)
|
||||
assert len({line.split(":")[0] for line in reported}) == len(paths)
|
||||
assert all(" TQ001 " in line for line in reported)
|
||||
|
||||
|
||||
def test_sys_executable_child_without_isolation_flag_is_flagged(tmp_path):
|
||||
source = 'import subprocess, sys\nsubprocess.run([sys.executable, "-c", "pass"])\n'
|
||||
assert _codes(tmp_path, source) == ["TQ009"]
|
||||
|
||||
|
||||
def test_sys_executable_child_with_dash_i_is_clean(tmp_path):
|
||||
source = 'import subprocess, sys\nsubprocess.run([sys.executable, "-I", "-c", "pass"])\n'
|
||||
assert _codes(tmp_path, source) == []
|
||||
|
||||
|
||||
def test_sys_executable_child_with_dash_p_is_clean(tmp_path):
|
||||
source = 'import subprocess, sys\nsubprocess.run([sys.executable, "-P", "-c", "pass"])\n'
|
||||
assert _codes(tmp_path, source) == []
|
||||
|
||||
|
||||
def test_non_interpreter_subprocess_call_is_untouched(tmp_path):
|
||||
source = 'import subprocess\nsubprocess.run(["python", "-c", "pass"])\n'
|
||||
assert _codes(tmp_path, source) == []
|
||||
|
||||
|
||||
def test_popen_sys_executable_tuple_is_flagged(tmp_path):
|
||||
source = 'import subprocess, sys\nsubprocess.Popen((sys.executable, "script.py"))\n'
|
||||
assert _codes(tmp_path, source) == ["TQ009"]
|
||||
|
|
|
|||
36
tests/test_litellm_rust/support/child_interpreter.py
Normal file
36
tests/test_litellm_rust/support/child_interpreter.py
Normal file
|
|
@ -0,0 +1,36 @@
|
|||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
from collections.abc import Mapping
|
||||
from typing import Final
|
||||
|
||||
import litellm
|
||||
|
||||
PARENT_LITELLM_FILE: Final = "LITELLM_TEST_PARENT_LITELLM_FILE"
|
||||
|
||||
_PROLOGUE: Final = (
|
||||
"import os as _os, litellm as _litellm; _parent = _os.environ.pop({key!r}); "
|
||||
'assert _litellm.__file__ == _parent, f"child imported litellm from {{_litellm.__file__}}, parent from {{_parent}}"; '
|
||||
"del _os, _litellm, _parent\n"
|
||||
)
|
||||
|
||||
|
||||
def run_child_interpreter(
|
||||
source: str, *, env: Mapping[str, str] | None = None, timeout: float
|
||||
) -> subprocess.CompletedProcess[str]:
|
||||
"""Run `source` in a fresh interpreter that imports the same `litellm` as this process.
|
||||
|
||||
`-I` keeps the working directory off sys.path so a source checkout cannot shadow an
|
||||
installed wheel, and the prologue fails fast with both paths if the child still
|
||||
resolves a different package.
|
||||
"""
|
||||
environment: Final = {**(os.environ if env is None else env), PARENT_LITELLM_FILE: litellm.__file__}
|
||||
return subprocess.run(
|
||||
[sys.executable, "-I", "-c", _PROLOGUE.format(key=PARENT_LITELLM_FILE) + source],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=timeout,
|
||||
env=environment,
|
||||
)
|
||||
|
|
@ -1,10 +1,10 @@
|
|||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import textwrap
|
||||
|
||||
import pytest
|
||||
|
||||
from tests.test_litellm_rust.support.child_interpreter import run_child_interpreter
|
||||
|
||||
pytestmark = pytest.mark.requires_rust_extension
|
||||
|
||||
_NATIVE_CONTRACT = textwrap.dedent(
|
||||
|
|
@ -53,9 +53,7 @@ _NATIVE_CONTRACT = textwrap.dedent(
|
|||
def test_compiled_extension_forbids_the_master_and_frees_its_workers() -> None:
|
||||
env = {**os.environ, "OBJC_DISABLE_INITIALIZE_FORK_SAFETY": "YES"}
|
||||
|
||||
result = subprocess.run(
|
||||
[sys.executable, "-I", "-c", _NATIVE_CONTRACT], capture_output=True, text=True, timeout=60, env=env
|
||||
)
|
||||
result = run_child_interpreter(_NATIVE_CONTRACT, env=env, timeout=60)
|
||||
|
||||
assert result.returncode == 0, result.stderr
|
||||
|
||||
|
|
@ -143,8 +141,6 @@ def test_sdk_call_in_a_child_forked_after_native_use_raises_instead_of_hanging()
|
|||
"LITELLM_LOCAL_MODEL_COST_MAP": "True",
|
||||
}
|
||||
|
||||
result = subprocess.run(
|
||||
[sys.executable, "-I", "-c", _SDK_CONTRACT], capture_output=True, text=True, timeout=120, env=env
|
||||
)
|
||||
result = run_child_interpreter(_SDK_CONTRACT, env=env, timeout=120)
|
||||
|
||||
assert result.returncode == 0, result.stderr
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue