mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-10 03:28:53 +00:00
fix(ci): drop unsupported arithmetic from the job timeout expression
GitHub expressions have no arithmetic operators, so
`${{ inputs.timeout-minutes + inputs.setup-timeout-minutes }}` was not a value
but a startup failure. The proxy-db workflow died before creating any job on
both prior commits, which posts no check run at all: the entire suite stopped
running while the PR's checks stayed green.
Pass the job backstop in as `job-timeout-minutes` instead of computing it, and
size it as the test budget plus the 30 minutes of setup ceilings plus 5 minutes
of runner overhead the job clock charges but no step owns.
check_workflow_startup_safety.py makes this class of mistake visible before
merge, since CI cannot report it: it rejects arithmetic inside an expression
and checks every caller of the reusable workflow keeps a job budget large
enough that the deadline cannot preempt pytest inside its own budget.
This commit is contained in:
parent
efd98bb1b4
commit
c22a749f70
4 changed files with 186 additions and 7 deletions
17
.github/workflows/_test-unit-base.yml
vendored
17
.github/workflows/_test-unit-base.yml
vendored
|
|
@ -25,15 +25,18 @@ on:
|
|||
required: false
|
||||
type: number
|
||||
default: 20
|
||||
setup-timeout-minutes:
|
||||
job-timeout-minutes:
|
||||
description: >-
|
||||
Timeout allowance for everything before the test step. Must stay >= the
|
||||
sum of the per-step timeouts on the setup steps below, which is what
|
||||
makes the test budget above a floor rather than a hope: setup cannot
|
||||
overrun into it without failing its own step first.
|
||||
Backstop for the whole job. Keep it >= `timeout-minutes` plus 35: 30 for
|
||||
the per-step ceilings on the setup steps below, and 5 for the runner
|
||||
overhead the job clock charges but no step owns (job init, step
|
||||
transitions, post-job cleanup). That headroom is what makes the test
|
||||
budget a floor rather than a hope, since setup cannot overrun into it
|
||||
without failing its own step first. GitHub expressions have no
|
||||
arithmetic, so the sum is passed in rather than computed.
|
||||
required: false
|
||||
type: number
|
||||
default: 30
|
||||
default: 55
|
||||
max-failures:
|
||||
description: "Stop after this many failures"
|
||||
required: false
|
||||
|
|
@ -56,7 +59,7 @@ jobs:
|
|||
run:
|
||||
name: Run tests
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: ${{ inputs.timeout-minutes + inputs.setup-timeout-minutes }}
|
||||
timeout-minutes: ${{ inputs.job-timeout-minutes }}
|
||||
outputs:
|
||||
decision: ${{ steps.changes.outputs.decision }}
|
||||
|
||||
|
|
|
|||
3
.github/workflows/test-code-quality.yml
vendored
3
.github/workflows/test-code-quality.yml
vendored
|
|
@ -68,6 +68,9 @@ jobs:
|
|||
- name: check_prisma_binary_cache
|
||||
run: uv run --no-sync python ./tests/code_coverage_tests/check_prisma_binary_cache.py
|
||||
|
||||
- name: check_workflow_startup_safety
|
||||
run: uv run --no-sync python ./tests/code_coverage_tests/check_workflow_startup_safety.py
|
||||
|
||||
- name: router_code_coverage
|
||||
run: uv run --no-sync python ./tests/code_coverage_tests/router_code_coverage.py
|
||||
|
||||
|
|
|
|||
|
|
@ -76,4 +76,5 @@ jobs:
|
|||
workers: 4
|
||||
reruns: 2
|
||||
timeout-minutes: 60
|
||||
job-timeout-minutes: 95
|
||||
artifact-name: proxy-server
|
||||
|
|
|
|||
172
tests/code_coverage_tests/check_workflow_startup_safety.py
Normal file
172
tests/code_coverage_tests/check_workflow_startup_safety.py
Normal file
|
|
@ -0,0 +1,172 @@
|
|||
"""Catch workflow mistakes that GitHub reports as nothing at all.
|
||||
|
||||
A workflow whose YAML is valid but whose expressions are not fails at *startup*:
|
||||
the run is marked failed, no jobs are created, and no check run is ever posted.
|
||||
Nothing turns red on the PR, so an entire test suite can silently stop running
|
||||
while the checks list stays green. These invariants have to be enforced here
|
||||
because CI cannot enforce them on itself.
|
||||
|
||||
1. No arithmetic inside ``${{ }}``. GitHub expressions support grouping, index,
|
||||
dereference, ``!``, the comparisons, ``&&`` and ``||``, and nothing else. A
|
||||
``${{ a + b }}`` is a startup failure, not a value. Only ``+`` and ``*`` are
|
||||
flagged: ``-`` appears in hyphenated input names like ``inputs.timeout-minutes``
|
||||
and ``/`` inside ref strings, so neither can be told apart from arithmetic by
|
||||
inspection alone.
|
||||
2. Callers of the reusable unit-test workflow keep the job timeout at or above
|
||||
the test budget plus the setup ceilings. Otherwise the job deadline preempts
|
||||
pytest inside its own advertised budget, which is the failure the split
|
||||
timeouts exist to prevent, and it shows up as a cancelled shard whose tests
|
||||
were passing.
|
||||
"""
|
||||
|
||||
import re
|
||||
import sys
|
||||
from collections.abc import Iterator, Mapping, Sequence
|
||||
from pathlib import Path
|
||||
from typing import Final
|
||||
|
||||
import yaml
|
||||
from pydantic import BaseModel, Field, ValidationError
|
||||
|
||||
REPO_ROOT: Final = Path(__file__).resolve().parent.parent.parent
|
||||
WORKFLOWS_DIR: Final = REPO_ROOT / ".github" / "workflows"
|
||||
BASE_WORKFLOW: Final = "./.github/workflows/_test-unit-base.yml"
|
||||
BASE_WORKFLOW_PATH: Final = WORKFLOWS_DIR / "_test-unit-base.yml"
|
||||
|
||||
# Runner time the job clock charges but no step owns: job init, the gaps between
|
||||
# steps, and post-job cleanup. Without it a job capped at exactly test + setup
|
||||
# would still preempt pytest inside its own budget.
|
||||
JOB_OVERHEAD_MINUTES: Final = 5
|
||||
|
||||
EXPRESSION: Final = re.compile(r"\$\{\{(?P<body>.*?)\}\}", re.DOTALL)
|
||||
QUOTED: Final = re.compile(r"'[^']*'")
|
||||
ARITHMETIC: Final = re.compile(r"[+*]")
|
||||
MATRIX_REF: Final = re.compile(r"^\$\{\{\s*matrix\.(?P<key>[\w-]+)\s*\}\}$")
|
||||
|
||||
|
||||
class WorkflowStartupError(Exception):
|
||||
pass
|
||||
|
||||
|
||||
class ReusableCall(BaseModel):
|
||||
uses: str | None = None
|
||||
with_: Mapping[str, object] = Field(default_factory=dict, alias="with")
|
||||
strategy: Mapping[str, object] = Field(default_factory=dict)
|
||||
steps: tuple[Mapping[str, object], ...] = ()
|
||||
|
||||
model_config = {"populate_by_name": True}
|
||||
|
||||
|
||||
class WorkflowFile(BaseModel):
|
||||
jobs: Mapping[str, ReusableCall] = Field(default_factory=dict)
|
||||
|
||||
|
||||
def parse_workflow(text: str) -> WorkflowFile | str:
|
||||
parsed: Final = yaml.safe_load(text)
|
||||
try:
|
||||
return WorkflowFile.model_validate(parsed if isinstance(parsed, dict) else {})
|
||||
except ValidationError as exc:
|
||||
return f"does not parse as a workflow: {exc.error_count()} schema error(s)"
|
||||
|
||||
|
||||
def arithmetic_expressions(text: str) -> Iterator[str]:
|
||||
for match in EXPRESSION.finditer(text):
|
||||
body: Final = match.group("body")
|
||||
if ARITHMETIC.search(QUOTED.sub("", body)):
|
||||
yield body.strip()
|
||||
|
||||
|
||||
def setup_ceiling_minutes(base_text: str) -> int:
|
||||
"""Sum the per-step timeouts on everything the base workflow runs before pytest."""
|
||||
base: Final = yaml.safe_load(base_text)
|
||||
steps: Final = base["jobs"]["run"]["steps"]
|
||||
return sum(
|
||||
s["timeout-minutes"]
|
||||
for s in steps
|
||||
if s.get("name") != "Run tests" and isinstance(s.get("timeout-minutes"), int)
|
||||
)
|
||||
|
||||
|
||||
def base_default(base_text: str, name: str) -> int:
|
||||
base: Final = yaml.safe_load(base_text)
|
||||
return base[True]["workflow_call"]["inputs"][name]["default"]
|
||||
|
||||
|
||||
def resolve_budgets(job: ReusableCall, key: str, fallback: int) -> Sequence[int]:
|
||||
"""A caller passes a literal, or `${{ matrix.x }}` naming a column of its matrix."""
|
||||
value: Final = job.with_.get(key)
|
||||
if value is None:
|
||||
return (fallback,)
|
||||
if isinstance(value, int):
|
||||
return (value,)
|
||||
|
||||
matrix_ref: Final = MATRIX_REF.match(str(value))
|
||||
if not matrix_ref:
|
||||
return ()
|
||||
|
||||
include: Final = job.strategy.get("matrix", {})
|
||||
entries: Final = include.get("include", ()) if isinstance(include, dict) else ()
|
||||
return tuple(
|
||||
e[matrix_ref.group("key")]
|
||||
for e in entries
|
||||
if isinstance(e, dict) and isinstance(e.get(matrix_ref.group("key")), int)
|
||||
)
|
||||
|
||||
|
||||
def timeout_contract_errors(rel: Path, workflow: WorkflowFile, ceiling: int, base_text: str) -> Iterator[str]:
|
||||
for job_name, job in workflow.jobs.items():
|
||||
if job.uses != BASE_WORKFLOW:
|
||||
continue
|
||||
|
||||
test_budgets: Final = resolve_budgets(job, "timeout-minutes", base_default(base_text, "timeout-minutes"))
|
||||
job_budgets: Final = resolve_budgets(job, "job-timeout-minutes", base_default(base_text, "job-timeout-minutes"))
|
||||
for test_budget in test_budgets:
|
||||
for job_budget in job_budgets:
|
||||
required = test_budget + ceiling + JOB_OVERHEAD_MINUTES
|
||||
if job_budget < required:
|
||||
yield (
|
||||
f"{rel}: job `{job_name}` gives pytest {test_budget}m but caps the job at "
|
||||
f"{job_budget}m. Setup can use up to {ceiling}m plus {JOB_OVERHEAD_MINUTES}m of "
|
||||
f"runner overhead, so the job deadline would preempt pytest; raise "
|
||||
f"job-timeout-minutes to at least {required}."
|
||||
)
|
||||
|
||||
|
||||
def workflow_errors(rel: Path, text: str, ceiling: int, base_text: str) -> Iterator[str]:
|
||||
for expression in arithmetic_expressions(text):
|
||||
yield (
|
||||
f"{rel}: `${{{{ {expression} }}}}` uses arithmetic, which GitHub expressions do not "
|
||||
"support. The workflow will fail at startup with no jobs and no check run."
|
||||
)
|
||||
|
||||
workflow: Final = parse_workflow(text)
|
||||
if isinstance(workflow, str):
|
||||
yield f"{rel}: {workflow}"
|
||||
return
|
||||
|
||||
yield from timeout_contract_errors(rel, workflow, ceiling, base_text)
|
||||
|
||||
|
||||
def main() -> None:
|
||||
base_text: Final = BASE_WORKFLOW_PATH.read_text()
|
||||
ceiling: Final = setup_ceiling_minutes(base_text)
|
||||
errors: Final = tuple(
|
||||
error
|
||||
for path in sorted(WORKFLOWS_DIR.glob("*.y*ml"))
|
||||
for error in workflow_errors(path.relative_to(REPO_ROOT), path.read_text(), ceiling, base_text)
|
||||
)
|
||||
|
||||
if errors:
|
||||
raise WorkflowStartupError(
|
||||
"Workflow startup invariants violated:\n - " + "\n - ".join(errors)
|
||||
)
|
||||
|
||||
print(f"Workflow startup invariants hold (setup ceiling {ceiling}m)")
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
try:
|
||||
main()
|
||||
except WorkflowStartupError as exc:
|
||||
print(f"ERROR: {exc}", file=sys.stderr)
|
||||
sys.exit(1)
|
||||
Loading…
Add table
Reference in a new issue