Port themefinder#148: switch evals to LiteLLM gateway, add dynamic model discovery - #1548
Conversation
|
Claude finished @saashanair's task in 2m 23s —— View job PR ReviewClean port with solid test coverage. Two issues worth addressing: 🔴 DO — Both early-exit paths use bare # line 989
if missing:
console.print(...)
return # ← sys.exit(1) needed here
# line 1004
if not selected:
console.print(...)
return # ← and hereFix this →%20after%20%60console.print(f%22%5Bred%5DNo%20matching%20models...%22)%60%20around%20line%20989%2C%20and%202)%20after%20%60console.print(%22%5Bred%5DNo%20models%20matched...%22)%60%20around%20line%201005.%20Both%20are%20error%20conditions%20that%20should%20exit%20non-zero%20for%20CI%20to%20catch%20them.&repo=i-dot-ai/consult) 🟡 TRY —
# current
supports_reasoning=item["supports_reasoning"],
# safer
supports_reasoning=item.get("supports_reasoning", False), |
6d7d3bd to
ec266f2
Compare
|
Claude finished @saashanair's task in 1m 2s —— View job PR Review
|
|
Claude finished @saashanair's task in 47s —— View job PR Review
|
057f6a8 to
c5a4503
Compare
|
Claude finished @saashanair's task in 1m 9s —— View job PR Review
Both previous issues are resolved and no new ones found. Clean port. ✅ |
c5a4503 to
0a5f7cc
Compare
|
Claude finished @saashanair's task in 2m 22s —— View job PR Review
Clean port. Both previous DO/TRY issues ( One small nit: ⚪️ NIT — The comment survived from the Azure port but "medium reasoning" isn't an API parameter being set — it describes an assumed default of the # before
# Initialise LLM for response generation (gpt-5-nano with medium reasoning)
# Medium reasoning ≈ o1 performance, 2x faster throughput than mini/low
# after (or just drop the comment — the constant is self-documenting)
# Initialise LLM for response generation |
0a5f7cc to
d87685b
Compare
|
Claude finished @saashanair's task in 1m 4s —— View job PR Review
All previous DO/TRY issues ( One carry-over from the last review: ⚪️ NIT — The two-line comment still describes Azure-specific behaviour that no longer applies: # Initialise LLM for response generation (gpt-5-nano with medium reasoning)
# Medium reasoning ≈ o1 performance, 2x faster throughput than mini/lowThe constant |
|
Claude finished @saashanair's task in 1m 9s —— View job PR Review
All previously flagged issues are resolved: One open thread from 252afh's review: ⚪️ NIT —
|
Ports the code from i-dot-ai/themefinder#148 ("Switch evals/benchmark to LiteLLM gateway, add dynamic model discovery"), raised against the old standalone themefinder repo before it was subtree-merged into consult. Replays that PR's diff under the themefinder/ prefix; file paths are the only thing that changed, logic is unmodified. Replaces the static, hand-maintained model list in evals/benchmark.py with live discovery via the gateway's /model_group/info and /health/latest endpoints (evals/utils_gateway.py), so evals stop targeting deployment names that have quietly gone stale on the gateway. Adds httpx as an explicit themefinder dependency (utils_gateway.py's gateway calls) and regenerates uv.lock accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
themefinder-eval.yml: drop AZURE_OPENAI_ENDPOINT, AZURE_OPENAI_API_KEY, and OPENAI_API_VERSION secrets - nothing in evals/ reads them anymore after the LiteLLM gateway migration. themefinder-ci.yml: scope the coverage-gated pytest run to tests/ only. It was running bare `pytest -v -s`, which would now also sweep up the new evals/tests/ into the 95% coverage gate. Added as a separate, uncounted step instead, matching how the original themefinder repo's CI handled the same evals/tests/ addition. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both early-exit paths (no gateway match for a named model, nothing selected at all) used a bare `return`, so main() completed normally and the process exited 0 even though nothing ran. CI review flagged this: on a misconfigured environment, a run that did nothing would still show green. Kept the existing fail-fast behavior for a missing named model (abort rather than silently run a smaller comparison than requested) but fixed the exit code, and added a comment explaining why that one stays strict rather than warn-and-continue like the unhealthy case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
item["supports_reasoning"] and the per-row check["model_name"]/ ["checked_at"]/["status"] accesses were all direct dict lookups, so one malformed entry from the gateway would KeyError and take down model discovery entirely - CI review flagged the first one, the same class of bug was present in latest_health_by_model too. supports_reasoning now falls back to False (matches GatewayModel's own default; defaulting True risks sending a reasoning_effort param to a model that doesn't support it, defaulting False just skips an optional sweep). A malformed health-check row is now skipped rather than crashing the whole call - the model still gets discovered, it just falls back to the existing "unknown" health state, same as a stale or missing check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two failure modes we hit debugging this port surfaced as opaque errors deep in discover_chat_models(): - A key without access to /model_group/info or /health/latest failed with a raw httpx.HTTPStatusError - now caught around the gather() call and re-raised as a RuntimeError naming the exact route and pointing at the gateway UI's allowed-paths setting for the key. - A key whose model grant is a wildcard (e.g. "all-team-models") can get /model_group/info back unexpanded - a single "*" row instead of real model names - which would otherwise silently become a fake, unusable "*"-named model selectable via --all. Now detected and raised explicitly before any filtering happens. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Five new cases, none previously covered: - supports_reasoning missing from a gateway entry defaults to False - a malformed health-check row (missing model_name/checked_at/status) is skipped rather than crashing latest_health_by_model - a 401/403 from either gateway route raises RuntimeError - a non-401/403 error (e.g. 500) still propagates unchanged, rather than getting misconverted into the permissions-error message - an unexpanded '*' model_group entry raises RuntimeError Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_FAMILY_SUBSTRINGS/_GPT_MARKERS require manually adding new model name patterns by hand (e.g. a future o-series release) rather than deriving family from real metadata. Flagged as a TODO rather than fixed now, since the gateway doesn't currently expose family as a field. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…plies The two-line comment at generate_synthetic.py's LLM init duplicated RESPONSE_GENERATION_MODEL (self-documenting) and asserted a stale, unverifiable performance claim disconnected from any actual parameter in that code. Dropped per review NIT. Moved the substance of that comment (why gpt-5-nano uses medium reasoning) to response_generator.py, right where reasoning_effort is actually set, instead of losing it entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3aa3cc2 to
e420574
Compare
|
Claude finished @saashanair's task in 1m 19s —— View job PR Review
All previously flagged DO/TRY issues remain fixed ( One carry-over NIT still open: ⚪️ NIT — The TODO noting the limitation is in place, but the markers themselves haven't been extended. |


Context
Model access should be allowed only through the central i.AI LLM gateway (LiteLLM). This PR makes that true everywhere in
themefinder/— evals, synthetic data generation, and the benchmark CLI — and replaces the hardcoded model list with one that updates automatically as the gateway's model list changes.This ports i-dot-ai/themefinder#148, raised against the old standalone
themefinder. That PR was raised without realising that themefinder had been subtree-merged into consult — this replays that PR's diff under thethemefinder/prefix. File paths are the only thing that changed; the logic below is unmodified from the original PR.Stacked on #1546 (re-adding the missing
themes.jsonground-truth files) — needed forevals/tests/and the live eval CI job to actually pass here. This PR will auto-retarget tomainonce that one merges.Changes proposed in this pull request
Dynamic model discovery (new)
evals/utils_gateway.pyfetches the gateway's/model_group/infoand/health/latestand turns them into a list of chat-capable models, each with vendor family and health resolved. Filtering is composable (split_unhealthy,filter_by_family,select_by_name) so callers decide what to do with unhealthy or unknown matches.benchmark.py— no more hardcoded model listThe old
MODEL_REGISTRY(Azure/Vertex/locai) is gone, along with the Vertex code path (it never worked —create_llm()unconditionally raisedNotImplementedErrorfor it). Relevant models are now found dynamically by querying the gateway.--provider {azure,vertex,locai,all}--models <name>.../--family {gpt,claude,gemini,locai}.../--all(exactly one required)reasoning_efforthardcoded per model name; no way to sweep effort levels--reasoning-effort {low,medium,high}...across all models whosesupports_reasoningflag, read from the gateway, is trueModel selection (
--modelsvs.--family/--all) is two small functions,_select_named_models/_select_healthy_models, each returning only the fields relevant to its own mode —--modelswarns on unhealthy matches but still runs them (explicit request by name);--family/--alldrops unhealthy matches and reports what was dropped.Synthetic data generation — off direct Azure
evals/synthetic/*now talks to the gateway (openai.AsyncOpenAI(base_url=LLM_GATEWAY_URL, ...)) instead of constructing anAsyncAzureOpenAIclient directly. The two models it uses are now named constants (DRAFTING_MODEL,RESPONSE_GENERATION_MODEL) instead of repeated string literals.Gateway credentials — one source of truth
LLM_GATEWAY_URL/CONSULT_EVAL_LITELLM_API_KEYwere each read directly viaos.getenvin 10 separate places acrossbenchmark.py,metrics.py, all foureval_*.pyfiles,synthetic/cli.py, andgenerate_synthetic.py— onlyutils_gateway.py's own client validated them before use; everywhere else silently passedNone/NoneintoOpenAILLM/AsyncOpenAIon a misconfigured environment. Addedutils_gateway.gateway_credentials()as the single source of truth; every call site now gets the same fail-fast validation.Config cleanup
.env.exampleand the eval workflow no longer listAZURE_OPENAI_*/GOOGLE_CLOUD_*/LOCAI_*— confirmed unused anywhere in the repo post-migration.LLM_GATEWAY_URL/CONSULT_EVAL_LITELLM_API_KEYare documented (previously required but missing from.env.example).AUTO_EVAL_4_1_SWEDEN_DEPLOYMENT(read by the eval judge/scoring path) is now prefilled with a known-working value plus a TODO, instead of being left blank and failing with a confusing 400 partway through a run.Tests
35 tests (
evals/tests/test_utils_gateway.py,test_benchmark.py) covering the discovery and selection logic against mocked gateway responses.Guidance to review
Verified in the original PR, against the old standalone repo:
generate_synthetic.pyruns end-to-end (1 question, 10 responses) — verified real output on diskbenchmark.py --models <unhealthy-name>through a full run — the warn-but-still-run behavior is unit-tested; no currently-unhealthy model was available on the live gateway to exercise it end-to-endRe-verified directly in consult, after porting:
uv run pytest evals/tests/ -v)discover_chat_models()runs live against the real gateway, 57 chat-capable models discovered with real health statusbenchmark.py --quickruns end-to-end here too;mapping/condensation/refinementproduce real scores.generationcurrently errors on an unrelated pre-existing bug insrc/themefinder's batch-processing retry path (not introduced by this PR — happy to open a separate issue)benchmark.pywith no selector exits 2 with the expected argparse error, matchingtest_no_selector_is_invalidbenchmark.py --models gpt-4.1-nano-sweden --evals mappingruns end-to-end, real scores (f1: 0.479 / 0.503 across the two question parts)benchmark.py --family locai --evals mapping— model discovered, selected by family filter, and received live API calls through the gateway. Reproduces the exact same known, pre-existing locai structured-output failure the original PR documented (malformed/non-JSON responses to the mapping prompt) — not a regression from this PRThings to check
.env.testfiles in the repon/a — no new env vars. Required a routing/permissions change on the gateway side instead: i-dot-ai/core-llm-gateway#232 which fixed the reading of available models from the gateway, alongside updating the routes that CI key (
CONSULT_EVAL_LITELLM_API_KEY) can access.