mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-13 23:11:40 +00:00
refactor(ci): ratchet the grandfathered extra="allow" list through the budget gate
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
mateo
parent
5fea87d9eb
commit
e1dcb740a0
7 changed files with 150 additions and 183 deletions
8
.github/workflows/test-code-quality.yml
vendored
8
.github/workflows/test-code-quality.yml
vendored
|
|
@ -119,14 +119,6 @@ jobs:
|
|||
- name: ban_pydantic_extra_allow
|
||||
run: uv run --no-sync python ./tests/code_coverage_tests/ban_pydantic_extra_allow.py
|
||||
|
||||
- name: ban_pydantic_extra_allow (grandfathered list only shrinks)
|
||||
if: github.event_name == 'pull_request'
|
||||
env:
|
||||
BASE_SHA: ${{ github.event.pull_request.base.sha }}
|
||||
run: |
|
||||
git fetch --no-tags --depth=1 origin "$BASE_SHA"
|
||||
uv run --no-sync python ./tests/code_coverage_tests/ban_pydantic_extra_allow.py --base "$BASE_SHA"
|
||||
|
||||
- name: check_fastuuid_usage
|
||||
run: uv run --no-sync python ./tests/code_coverage_tests/check_fastuuid_usage.py
|
||||
|
||||
|
|
|
|||
|
|
@ -43,7 +43,7 @@ When you fix violations gated by `ruff-strict-budget.json`, `type-discipline-bud
|
|||
|
||||
`make pre-commit` saves its complete output to a log file in .git (overwriting previous pre-commit logs) 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
|
||||
|
||||
New Pydantic models must declare the fields they accept. `extra="allow"` is banned by `tests/code_coverage_tests/ban_pydantic_extra_allow.py`, which grandfathers the models that already had it, so don't add it to a new model. That list only shrinks: the check fails when your branch adds an entry to it, so removing a model's `extra="allow"` is the only edit it accepts. `make pre-commit` runs it on any commit that touches `litellm/` Python
|
||||
New Pydantic models must declare the fields they accept. `extra="allow"` is banned by `tests/code_coverage_tests/ban_pydantic_extra_allow.py`, which grandfathers the models `extra-allow-budget.json` lists, so don't add it to a new model. That budget ratchets like the others: its `limit` must equal the number of models it lists, so grandfathering one more means raising the limit, which reds the non-gating `budget-ratchet` job for a human to accept. Clean a model up and you lower both. `make pre-commit` runs the ban on any commit that touches `litellm/` Python
|
||||
|
||||
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
|
||||
|
||||
|
|
|
|||
61
extra-allow-budget.json
Normal file
61
extra-allow-budget.json
Normal file
|
|
@ -0,0 +1,61 @@
|
|||
{
|
||||
"extra_allow_models": {
|
||||
"limit": 54,
|
||||
"models": [
|
||||
"litellm/llms/anthropic/common_utils.py::_ReplayedSearchQuery",
|
||||
"litellm/llms/anthropic/common_utils.py::_ReplayedServerToolUse",
|
||||
"litellm/llms/anthropic/common_utils.py::_ReplayedWebSearchResult",
|
||||
"litellm/llms/anthropic/common_utils.py::_ReplayedWebSearchToolResult",
|
||||
"litellm/llms/base_llm/ocr/transformation.py::OCRPage",
|
||||
"litellm/llms/base_llm/ocr/transformation.py::OCRPageImage",
|
||||
"litellm/llms/base_llm/ocr/transformation.py::OCRResponse",
|
||||
"litellm/llms/base_llm/ocr/transformation.py::OCRUsageInfo",
|
||||
"litellm/llms/base_llm/sandbox/transformation.py::CodeExecutionResult",
|
||||
"litellm/llms/base_llm/sandbox/transformation.py::ContainerHandle",
|
||||
"litellm/llms/base_llm/search/transformation.py::SearchResponse",
|
||||
"litellm/llms/base_llm/search/transformation.py::SearchResult",
|
||||
"litellm/proxy/_types.py::CoordinationRedisParams",
|
||||
"litellm/proxy/_types.py::ModelInfo",
|
||||
"litellm/proxy/_types.py::TeamDefaultSettings",
|
||||
"litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py::UISettings",
|
||||
"litellm/router_strategy/auto_router/litellm_encoder.py::CustomDenseEncoder",
|
||||
"litellm/router_strategy/complexity_router/config.py::ComplexityRouterConfig",
|
||||
"litellm/router_strategy/quality_router/config.py::QualityRouterConfig",
|
||||
"litellm/router_strategy/quality_router/config.py::RoutingPreferences",
|
||||
"litellm/types/agents.py::AgentCreateResponse",
|
||||
"litellm/types/agents.py::AgentDeleteResult",
|
||||
"litellm/types/agents.py::AgentListResponse",
|
||||
"litellm/types/agents.py::AgentVersionsResponse",
|
||||
"litellm/types/agents.py::LiteLLMSendMessageResponse",
|
||||
"litellm/types/completion.py::CompletionRequest",
|
||||
"litellm/types/embedding.py::EmbeddingRequest",
|
||||
"litellm/types/fine_tuning.py::OpenAIFineTuningHyperparameters",
|
||||
"litellm/types/guardrails.py::BaseLitellmParams",
|
||||
"litellm/types/llms/anthropic.py::AnthropicResponseContentBlockToolUse",
|
||||
"litellm/types/llms/anthropic.py::AnthropicResponseUsageBlock",
|
||||
"litellm/types/llms/base.py::BaseLiteLLMOpenAIResponseObject",
|
||||
"litellm/types/llms/base.py::HiddenParams",
|
||||
"litellm/types/llms/openai.py::GenericEvent",
|
||||
"litellm/types/llms/openai.py::Hyperparameters",
|
||||
"litellm/types/llms/openai.py::InputTokensDetails",
|
||||
"litellm/types/llms/openai.py::LiteLLMFineTuningJobCreate",
|
||||
"litellm/types/llms/openai.py::OutputTokensDetails",
|
||||
"litellm/types/llms/openai.py::ResponseAPIUsage",
|
||||
"litellm/types/prompts/init_prompts.py::PromptInfo",
|
||||
"litellm/types/prompts/init_prompts.py::PromptLiteLLMParams",
|
||||
"litellm/types/proxy/guardrails/guardrail_hooks/cisco_ai_defense.py::CiscoAIDefenseGuardrailConfigModelOptionalParams",
|
||||
"litellm/types/proxy/guardrails/guardrail_hooks/generic_guardrail_api.py::GuardrailToolParam",
|
||||
"litellm/types/proxy/guardrails/guardrail_hooks/straiker.py::StraikerWebhookResponse",
|
||||
"litellm/types/rag.py::RAGIngestRequest",
|
||||
"litellm/types/rag.py::RAGQueryRequest",
|
||||
"litellm/types/realtime.py::RealtimeSessionConfig",
|
||||
"litellm/types/realtime.py::RealtimeTranscriptionSessionRequest",
|
||||
"litellm/types/realtime.py::RealtimeTranscriptionSessionResponse",
|
||||
"litellm/types/router.py::Deployment",
|
||||
"litellm/types/router.py::GenericLiteLLMParams",
|
||||
"litellm/types/router.py::LiteLLM_Params",
|
||||
"litellm/types/router.py::ModelInfo",
|
||||
"litellm/types/utils.py::ImageResponse"
|
||||
]
|
||||
}
|
||||
}
|
||||
|
|
@ -1,8 +1,8 @@
|
|||
#!/usr/bin/env python3
|
||||
"""Non-gating ratchet guard: budget limits may only fall, never rise.
|
||||
|
||||
Every `*-budget.json` file (ruff-strict, type-discipline, basedpyright-code) is a
|
||||
one-way ratchet: each rule's ceiling is its `limit`, and that limit is meant to be
|
||||
Every `*-budget.json` file (ruff-strict, type-discipline, basedpyright-code, extra-allow)
|
||||
is a one-way ratchet: each rule's ceiling is its `limit`, and that limit is meant to be
|
||||
driven DOWN over time. This check compares every budget file against its own
|
||||
content at the merge-base with the target branch and fails (exits 1, red) if:
|
||||
|
||||
|
|
@ -39,6 +39,7 @@ DEFAULT_BUDGETS: tuple[str, ...] = (
|
|||
"ruff-strict-budget.json",
|
||||
"type-discipline-budget.json",
|
||||
"basedpyright-code-budget.json",
|
||||
"extra-allow-budget.json",
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -6,8 +6,8 @@
|
|||
# - 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)
|
||||
# - litellm/ Python or this
|
||||
# check's own list staged -> extra="allow" ban (test-code-quality.yml's ban_pydantic_extra_allow)
|
||||
# - litellm/ Python or the
|
||||
# grandfathered list staged -> extra="allow" ban (test-code-quality.yml's ban_pydantic_extra_allow)
|
||||
# - 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)
|
||||
#
|
||||
|
|
@ -49,9 +49,9 @@ litellm_py_files=$(staged_match '^litellm/.*\.py$')
|
|||
e2e_py_files=$(staged_match '^tests/e2e/.*\.py$')
|
||||
# ruff format (and CI's format step) skip enterprise; the rest of make lint covers it.
|
||||
fmt_files=$(printf '%s\n' "$litellm_py_files" | grep -v '^litellm/enterprise/' || true)
|
||||
# The extra="allow" ban reads all of litellm/, and its grandfathered list is data the
|
||||
# check itself gates, so editing either can turn ban_pydantic_extra_allow red.
|
||||
extra_allow_files=$(staged_match '^(litellm/.*\.py|tests/code_coverage_tests/ban_pydantic_extra_allow\.py)$')
|
||||
# The extra="allow" ban reads all of litellm/, and its grandfathered models are a budget
|
||||
# the check reads, so editing either can turn ban_pydantic_extra_allow red.
|
||||
extra_allow_files=$(staged_match '^(litellm/.*\.py|extra-allow-budget\.json|tests/code_coverage_tests/ban_pydantic_extra_allow\.py)$')
|
||||
# check-ui-api-types.yml triggers on any file under litellm/proxy or litellm/types
|
||||
# (Prisma schema and configs included, not just Python) plus the generator and its
|
||||
# lockfiles, so match that whole trigger set rather than a Python subset.
|
||||
|
|
@ -162,14 +162,11 @@ if [ -n "$e2e_py_files" ]; then
|
|||
fi
|
||||
|
||||
# Around two seconds over all of litellm/, so it runs inline rather than behind a job.
|
||||
# CI ratchets the grandfathered list against the base branch tip; locally the closest
|
||||
# equivalent is the merge-base, and the check reports the ref it couldn't read.
|
||||
# Growing the list is the budget-ratchet job's business, which is non-gating and CI-only.
|
||||
if [ -n "$extra_allow_files" ]; then
|
||||
echo "pre-commit: checking new pydantic models don't set extra=\"allow\" (ban_pydantic_extra_allow)"
|
||||
extra_allow_base=$(git merge-base origin/litellm_internal_staging HEAD 2>/dev/null || true)
|
||||
uv run --no-sync python tests/code_coverage_tests/ban_pydantic_extra_allow.py \
|
||||
${extra_allow_base:+--base "$extra_allow_base"} \
|
||||
|| { echo "✗ New extra=\"allow\" model or a grown grandfathered list. Declare the fields the model accepts, then re-run make pre-commit." >&2; status=1; }
|
||||
|| { echo "✗ New extra=\"allow\" model, or extra-allow-budget.json is stale. Declare the fields the model accepts, then re-run make pre-commit." >&2; status=1; }
|
||||
fi
|
||||
|
||||
dashboard_checks() {
|
||||
|
|
|
|||
|
|
@ -2,81 +2,34 @@
|
|||
|
||||
``extra="allow"`` silently accepts undeclared keys, so typos survive validation and
|
||||
real fields (pricing on ``ModelInfo``, for example) never get declared anywhere. The
|
||||
models listed in ``GRANDFATHERED`` predate this check and stay allowed; anything new
|
||||
must declare its fields.
|
||||
models listed in ``extra-allow-budget.json`` predate this check and stay allowed;
|
||||
anything new must declare its fields.
|
||||
|
||||
The list is one way. With ``--base <ref>`` the check also fails when it gained an entry
|
||||
relative to that ref, so the only edits to it that pass are removals.
|
||||
That list is a budget like the others, so ``scripts/budget_ratchet_check.py`` is what
|
||||
reds when it grows, and its ``limit`` must equal the number of models it lists.
|
||||
"""
|
||||
|
||||
import argparse
|
||||
import ast
|
||||
import json
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
from types import MappingProxyType
|
||||
from typing import Final, Iterator, NamedTuple, Sequence
|
||||
|
||||
SCAN_ROOT: Final = "litellm"
|
||||
SELF_PATH: Final = "tests/code_coverage_tests/ban_pydantic_extra_allow.py"
|
||||
BUDGET_PATH: Final = "extra-allow-budget.json"
|
||||
BUDGET_RULE: Final = "extra_allow_models"
|
||||
|
||||
GRANDFATHERED: Final = frozenset(
|
||||
{
|
||||
"litellm/llms/anthropic/common_utils.py::_ReplayedSearchQuery",
|
||||
"litellm/llms/anthropic/common_utils.py::_ReplayedServerToolUse",
|
||||
"litellm/llms/anthropic/common_utils.py::_ReplayedWebSearchResult",
|
||||
"litellm/llms/anthropic/common_utils.py::_ReplayedWebSearchToolResult",
|
||||
"litellm/llms/base_llm/ocr/transformation.py::OCRPage",
|
||||
"litellm/llms/base_llm/ocr/transformation.py::OCRPageImage",
|
||||
"litellm/llms/base_llm/ocr/transformation.py::OCRResponse",
|
||||
"litellm/llms/base_llm/ocr/transformation.py::OCRUsageInfo",
|
||||
"litellm/llms/base_llm/sandbox/transformation.py::CodeExecutionResult",
|
||||
"litellm/llms/base_llm/sandbox/transformation.py::ContainerHandle",
|
||||
"litellm/llms/base_llm/search/transformation.py::SearchResponse",
|
||||
"litellm/llms/base_llm/search/transformation.py::SearchResult",
|
||||
"litellm/proxy/_types.py::CoordinationRedisParams",
|
||||
"litellm/proxy/_types.py::ModelInfo",
|
||||
"litellm/proxy/_types.py::TeamDefaultSettings",
|
||||
"litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py::UISettings",
|
||||
"litellm/router_strategy/auto_router/litellm_encoder.py::CustomDenseEncoder",
|
||||
"litellm/router_strategy/complexity_router/config.py::ComplexityRouterConfig",
|
||||
"litellm/router_strategy/quality_router/config.py::QualityRouterConfig",
|
||||
"litellm/router_strategy/quality_router/config.py::RoutingPreferences",
|
||||
"litellm/types/agents.py::AgentCreateResponse",
|
||||
"litellm/types/agents.py::AgentDeleteResult",
|
||||
"litellm/types/agents.py::AgentListResponse",
|
||||
"litellm/types/agents.py::AgentVersionsResponse",
|
||||
"litellm/types/agents.py::LiteLLMSendMessageResponse",
|
||||
"litellm/types/completion.py::CompletionRequest",
|
||||
"litellm/types/embedding.py::EmbeddingRequest",
|
||||
"litellm/types/fine_tuning.py::OpenAIFineTuningHyperparameters",
|
||||
"litellm/types/guardrails.py::BaseLitellmParams",
|
||||
"litellm/types/llms/anthropic.py::AnthropicResponseContentBlockToolUse",
|
||||
"litellm/types/llms/anthropic.py::AnthropicResponseUsageBlock",
|
||||
"litellm/types/llms/base.py::BaseLiteLLMOpenAIResponseObject",
|
||||
"litellm/types/llms/base.py::HiddenParams",
|
||||
"litellm/types/llms/openai.py::GenericEvent",
|
||||
"litellm/types/llms/openai.py::Hyperparameters",
|
||||
"litellm/types/llms/openai.py::InputTokensDetails",
|
||||
"litellm/types/llms/openai.py::LiteLLMFineTuningJobCreate",
|
||||
"litellm/types/llms/openai.py::OutputTokensDetails",
|
||||
"litellm/types/llms/openai.py::ResponseAPIUsage",
|
||||
"litellm/types/prompts/init_prompts.py::PromptInfo",
|
||||
"litellm/types/prompts/init_prompts.py::PromptLiteLLMParams",
|
||||
"litellm/types/proxy/guardrails/guardrail_hooks/cisco_ai_defense.py::CiscoAIDefenseGuardrailConfigModelOptionalParams",
|
||||
"litellm/types/proxy/guardrails/guardrail_hooks/generic_guardrail_api.py::GuardrailToolParam",
|
||||
"litellm/types/proxy/guardrails/guardrail_hooks/straiker.py::StraikerWebhookResponse",
|
||||
"litellm/types/rag.py::RAGIngestRequest",
|
||||
"litellm/types/rag.py::RAGQueryRequest",
|
||||
"litellm/types/realtime.py::RealtimeSessionConfig",
|
||||
"litellm/types/realtime.py::RealtimeTranscriptionSessionRequest",
|
||||
"litellm/types/realtime.py::RealtimeTranscriptionSessionResponse",
|
||||
"litellm/types/router.py::Deployment",
|
||||
"litellm/types/router.py::GenericLiteLLMParams",
|
||||
"litellm/types/router.py::LiteLLM_Params",
|
||||
"litellm/types/router.py::ModelInfo",
|
||||
"litellm/types/utils.py::ImageResponse",
|
||||
}
|
||||
)
|
||||
|
||||
class Budget(NamedTuple):
|
||||
limit: int
|
||||
models: frozenset[str]
|
||||
|
||||
|
||||
def read_budget(path: str) -> Budget:
|
||||
with open(path, encoding="utf-8") as handle:
|
||||
rule: Final = MappingProxyType(json.load(handle)[BUDGET_RULE])
|
||||
return Budget(limit=int(rule["limit"]), models=frozenset(rule["models"]))
|
||||
|
||||
|
||||
class Violation(NamedTuple):
|
||||
|
|
@ -274,48 +227,13 @@ def find_extra_allow_models(base_dir: str) -> tuple[Violation, ...]:
|
|||
)
|
||||
|
||||
|
||||
def parse_grandfathered(source: str) -> frozenset[str]:
|
||||
"""The entries ``GRANDFATHERED`` lists in ``source``, so another revision of this file
|
||||
can be read without importing it. The entries are string literals, so they are collected
|
||||
from the assignment's subtree rather than evaluating the ``frozenset`` call around them."""
|
||||
return frozenset(
|
||||
node.value
|
||||
for statement in ast.parse(source).body
|
||||
for name, value in _assigned_names(statement)
|
||||
if name == "GRANDFATHERED"
|
||||
for node in ast.walk(value)
|
||||
if isinstance(node, ast.Constant) and isinstance(node.value, str)
|
||||
)
|
||||
|
||||
|
||||
def _source_at(ref: str) -> str | None:
|
||||
completed: Final = subprocess.run(
|
||||
["git", "show", f"{ref}:{SELF_PATH}"], capture_output=True, text=True, check=False
|
||||
)
|
||||
return completed.stdout if completed.returncode == 0 else None
|
||||
|
||||
|
||||
def added_grandfathered(base_ref: str) -> tuple[str, ...]:
|
||||
base_source: Final = _source_at(base_ref)
|
||||
if base_source is None:
|
||||
print(f"{SELF_PATH} does not exist at {base_ref}, so there is no list to ratchet against yet.")
|
||||
return ()
|
||||
return tuple(sorted(GRANDFATHERED - parse_grandfathered(base_source)))
|
||||
|
||||
|
||||
def main() -> int:
|
||||
parser: Final = argparse.ArgumentParser(description=__doc__)
|
||||
parser.add_argument(
|
||||
"--base",
|
||||
help="git ref to ratchet GRANDFATHERED against, failing when this branch added an entry",
|
||||
)
|
||||
args: Final = parser.parse_args()
|
||||
|
||||
base_dir = os.getcwd()
|
||||
found = find_extra_allow_models(base_dir)
|
||||
violations = tuple(violation for violation in found if violation.identifier() not in GRANDFATHERED)
|
||||
stale = tuple(sorted(GRANDFATHERED - {violation.identifier() for violation in found}))
|
||||
added = added_grandfathered(args.base) if args.base else ()
|
||||
base_dir: Final = os.getcwd()
|
||||
budget: Final = read_budget(os.path.join(base_dir, BUDGET_PATH))
|
||||
found: Final = find_extra_allow_models(base_dir)
|
||||
violations: Final = tuple(violation for violation in found if violation.identifier() not in budget.models)
|
||||
stale: Final = tuple(sorted(budget.models - {violation.identifier() for violation in found}))
|
||||
miscounted: Final = budget.limit != len(budget.models)
|
||||
|
||||
for violation in violations:
|
||||
print(f'{violation.file}:{violation.line}: {violation.model} sets extra="allow"')
|
||||
|
|
@ -324,25 +242,24 @@ def main() -> int:
|
|||
f'\nFound {len(violations)} new Pydantic model(s) using extra="allow".\n'
|
||||
'Declare the fields you accept instead. extra="allow" hides typos and\n'
|
||||
"leaves real fields undocumented and untyped. If a model genuinely has to\n"
|
||||
"forward opaque provider payloads, add it to GRANDFATHERED in\n"
|
||||
"tests/code_coverage_tests/ban_pydantic_extra_allow.py with a reason in the PR."
|
||||
f"forward opaque provider payloads, add it to {BUDGET_RULE} in {BUDGET_PATH},\n"
|
||||
"raise the limit to match, and say why in the PR: raising it reds the\n"
|
||||
"non-gating budget-ratchet check so the loosening is seen and accepted."
|
||||
)
|
||||
if stale:
|
||||
print(
|
||||
'\nThese GRANDFATHERED entries no longer use extra="allow" (or moved).\n'
|
||||
"Remove them so the list keeps ratcheting down:"
|
||||
f'\nThese {BUDGET_PATH} models no longer use extra="allow" (or moved).\n'
|
||||
"Remove them and lower the limit so it keeps ratcheting down:"
|
||||
)
|
||||
for entry in stale:
|
||||
print(f" {entry}")
|
||||
if added:
|
||||
if miscounted:
|
||||
print(
|
||||
f"\nThis branch added {len(added)} entry/entries to GRANDFATHERED. The list only\n"
|
||||
"shrinks, so declare the fields these models accept instead of grandfathering them:"
|
||||
f"\n{BUDGET_PATH} lists {len(budget.models)} models under a limit of {budget.limit}.\n"
|
||||
"They must match, since the limit is what the budget ratchet reads."
|
||||
)
|
||||
for entry in added:
|
||||
print(f" {entry}")
|
||||
|
||||
if violations or stale or added:
|
||||
if violations or stale or miscounted:
|
||||
return 1
|
||||
print('No new extra="allow" Pydantic models found.')
|
||||
return 0
|
||||
|
|
|
|||
|
|
@ -1,7 +1,8 @@
|
|||
"""Tests for the extra="allow" ban at tests/code_coverage_tests/ban_pydantic_extra_allow.py."""
|
||||
|
||||
import importlib.util
|
||||
import json
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
|
|
@ -13,6 +14,11 @@ import ban_pydantic_extra_allow as checker # noqa: E402
|
|||
|
||||
_REPO_ROOT = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..")
|
||||
|
||||
_RATCHET_PATH = os.path.join(_REPO_ROOT, "scripts", "budget_ratchet_check.py")
|
||||
_ratchet_spec = importlib.util.spec_from_file_location("budget_ratchet_check", _RATCHET_PATH)
|
||||
ratchet = importlib.util.module_from_spec(_ratchet_spec)
|
||||
_ratchet_spec.loader.exec_module(ratchet)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"source",
|
||||
|
|
@ -187,64 +193,57 @@ def test_reports_nested_class_with_qualified_name():
|
|||
assert [violation.identifier() for violation in violations] == ["litellm/types/thing.py::Outer.Inner"]
|
||||
|
||||
|
||||
def test_grandfathered_list_matches_repo():
|
||||
"""Every grandfathered entry must still exist, and nothing new may be added."""
|
||||
def test_budget_matches_repo():
|
||||
"""Every grandfathered model must still exist, and nothing new may be added."""
|
||||
budget = checker.read_budget(os.path.join(_REPO_ROOT, checker.BUDGET_PATH))
|
||||
found = frozenset(violation.identifier() for violation in checker.find_extra_allow_models(_REPO_ROOT))
|
||||
assert sorted(found - checker.GRANDFATHERED) == []
|
||||
assert sorted(checker.GRANDFATHERED - found) == []
|
||||
assert sorted(found - budget.models) == []
|
||||
assert sorted(budget.models - found) == []
|
||||
|
||||
|
||||
def test_parse_grandfathered_reads_the_list_of_another_revision():
|
||||
source = 'OTHER = frozenset({"nope"})\nGRANDFATHERED = frozenset(\n {\n "a.py::A",\n "b.py::B",\n }\n)\n'
|
||||
assert checker.parse_grandfathered(source) == frozenset({"a.py::A", "b.py::B"})
|
||||
def test_budget_limit_matches_the_models_it_lists():
|
||||
"""The limit is what the budget ratchet reads, so it can't drift from the list."""
|
||||
budget = checker.read_budget(os.path.join(_REPO_ROOT, checker.BUDGET_PATH))
|
||||
assert budget.limit == len(budget.models)
|
||||
|
||||
|
||||
def test_parse_grandfathered_reads_this_files_own_list():
|
||||
with open(os.path.join(_REPO_ROOT, checker.SELF_PATH), encoding="utf-8") as handle:
|
||||
assert checker.parse_grandfathered(handle.read()) == checker.GRANDFATHERED
|
||||
def _write_budget(path, limit, models):
|
||||
with open(path, "w", encoding="utf-8") as handle:
|
||||
json.dump({checker.BUDGET_RULE: {"limit": limit, "models": list(models)}}, handle)
|
||||
return path
|
||||
|
||||
|
||||
def _baseline_commit(repo, entries):
|
||||
"""A one-commit repo whose copy of the checker grandfathers exactly ``entries``."""
|
||||
target = os.path.join(repo, checker.SELF_PATH)
|
||||
os.makedirs(os.path.dirname(target), exist_ok=True)
|
||||
with open(target, "w", encoding="utf-8") as handle:
|
||||
handle.write("GRANDFATHERED = frozenset(\n {\n")
|
||||
handle.writelines(f' "{entry}",\n' for entry in entries)
|
||||
handle.write(" }\n)\n")
|
||||
for command in (
|
||||
["git", "init", "-q", "."],
|
||||
["git", "add", checker.SELF_PATH],
|
||||
["git", "-c", "user.email=t@t", "-c", "user.name=t", "commit", "-qm", "baseline"],
|
||||
):
|
||||
subprocess.run(command, cwd=repo, check=True)
|
||||
return subprocess.run(
|
||||
["git", "rev-parse", "HEAD"], cwd=repo, check=True, capture_output=True, text=True
|
||||
).stdout.strip()
|
||||
def test_read_budget_reads_the_limit_and_the_models(tmp_path):
|
||||
path = _write_budget(tmp_path / "extra-allow-budget.json", 2, ["a.py::A", "b.py::B"])
|
||||
assert checker.read_budget(str(path)) == checker.Budget(limit=2, models=frozenset({"a.py::A", "b.py::B"}))
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"baseline, expected",
|
||||
"limit, expected_exit",
|
||||
[
|
||||
pytest.param(checker.GRANDFATHERED, [], id="unchanged_list_passes"),
|
||||
pytest.param(
|
||||
checker.GRANDFATHERED | {"litellm/types/gone.py::Gone"},
|
||||
[],
|
||||
id="removing_an_entry_passes",
|
||||
),
|
||||
pytest.param(
|
||||
checker.GRANDFATHERED - {"litellm/types/utils.py::ImageResponse"},
|
||||
["litellm/types/utils.py::ImageResponse"],
|
||||
id="adding_an_entry_is_reported",
|
||||
),
|
||||
pytest.param(1, 0, id="limit_matching_the_single_model_passes"),
|
||||
pytest.param(2, 1, id="limit_above_the_models_listed_fails"),
|
||||
],
|
||||
)
|
||||
def test_added_grandfathered_only_reports_growth(tmp_path, monkeypatch, baseline, expected):
|
||||
base_sha = _baseline_commit(str(tmp_path), sorted(baseline))
|
||||
def test_main_requires_the_limit_to_match_the_list(tmp_path, monkeypatch, limit, expected_exit):
|
||||
"""A limit above the list would buy silent headroom the budget ratchet can't see."""
|
||||
model_dir = tmp_path / checker.SCAN_ROOT / "types"
|
||||
model_dir.mkdir(parents=True)
|
||||
(model_dir / "thing.py").write_text('class Foo(BaseModel):\n model_config = ConfigDict(extra="allow")\n')
|
||||
_write_budget(tmp_path / checker.BUDGET_PATH, limit, [f"{checker.SCAN_ROOT}/types/thing.py::Foo"])
|
||||
monkeypatch.chdir(tmp_path)
|
||||
assert list(checker.added_grandfathered(base_sha)) == expected
|
||||
assert checker.main() == expected_exit
|
||||
|
||||
|
||||
def test_added_grandfathered_skips_a_base_without_the_checker(tmp_path, monkeypatch):
|
||||
monkeypatch.chdir(tmp_path)
|
||||
assert checker.added_grandfathered("HEAD") == ()
|
||||
def test_raising_the_limit_reds_the_budget_ratchet():
|
||||
"""Grandfathering one more model means raising the limit, which the shared ratchet catches."""
|
||||
budget = checker.read_budget(os.path.join(_REPO_ROOT, checker.BUDGET_PATH))
|
||||
base = {checker.BUDGET_RULE: {"limit": budget.limit}}
|
||||
head = {checker.BUDGET_RULE: {"limit": budget.limit + 1}}
|
||||
regressions = ratchet.regressions_for(checker.BUDGET_PATH, base, head)
|
||||
assert [regression.rule for regression in regressions] == [checker.BUDGET_RULE]
|
||||
assert ratchet.regressions_for(checker.BUDGET_PATH, base, {checker.BUDGET_RULE: {"limit": budget.limit - 1}}) == []
|
||||
|
||||
|
||||
def test_the_budget_ratchet_watches_this_budget():
|
||||
assert checker.BUDGET_PATH in ratchet.DEFAULT_BUDGETS
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue