Skip to content

Commit f6d3d00

Browse files
vsraccubitsclaude
andcommitted
fix: address Gemini code review findings for PR #1
Simplify fetch_catalog_sync to use asyncio.run() instead of ThreadPoolExecutor, guard empty models in example script, refactor merger fallback/loop logic to reduce duplication, remove obsolete sync-from-loop test, and add uv.lock. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent a601f12 commit f6d3d00

5 files changed

Lines changed: 942 additions & 73 deletions

File tree

example/fetch_catalog.py

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,22 @@ def main() -> None:
5656
print(f"... and {len(provider_counts) - 10} more providers")
5757

5858
# ── 4. Inspect a single model ───────────────────────────────────────
59-
first_key = next(iter(result.models))
60-
model = result.models[first_key]
61-
print(f"\nSample model: {first_key}")
62-
for field in ("litellm_provider", "max_tokens", "max_input_tokens", "max_output_tokens",
63-
"input_cost_per_token", "output_cost_per_token"):
64-
if field in model:
65-
print(f" {field}: {model[field]}")
59+
if result.models:
60+
first_key = next(iter(result.models))
61+
model = result.models[first_key]
62+
print(f"\nSample model: {first_key}")
63+
for field in (
64+
"litellm_provider",
65+
"max_tokens",
66+
"max_input_tokens",
67+
"max_output_tokens",
68+
"input_cost_per_token",
69+
"output_cost_per_token",
70+
):
71+
if field in model:
72+
print(f" {field}: {model[field]}")
73+
else:
74+
print("\nNo models in catalog.")
6675

6776
# ── 5. Optional JSON dump ────────────────────────────────────────────
6877
if args.output:

src/bud_model_catalog/client.py

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,16 +43,8 @@ async def _safe_ai_models() -> FetchResult | None:
4343
return merge(litellm_result, ai_models_result, self._config)
4444

4545
def fetch_catalog_sync(self) -> CatalogResult:
46-
"""Sync wrapper — safe to call from both sync and async contexts."""
47-
try:
48-
loop = asyncio.get_running_loop()
49-
except RuntimeError:
50-
loop = None
51-
52-
if loop and loop.is_running():
53-
import concurrent.futures
54-
55-
with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool:
56-
return pool.submit(asyncio.run, self.fetch_catalog()).result()
46+
"""Sync wrapper for fetch_catalog.
5747
48+
This is a blocking call. In async code, await fetch_catalog() instead.
49+
"""
5850
return asyncio.run(self.fetch_catalog())

src/bud_model_catalog/merger.py

Lines changed: 22 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -54,13 +54,12 @@ def merge(
5454
fallback_models = dict(litellm_models)
5555
fallback_deprecated = 0
5656
else:
57-
fallback_models = {}
58-
fallback_deprecated = 0
59-
for key, entry in litellm_models.items():
60-
if _is_litellm_deprecated(entry):
61-
fallback_deprecated += 1
62-
else:
63-
fallback_models[key] = entry
57+
fallback_models = {
58+
key: entry
59+
for key, entry in litellm_models.items()
60+
if not _is_litellm_deprecated(entry)
61+
}
62+
fallback_deprecated = total_litellm - len(fallback_models)
6463
return CatalogResult(
6564
models=fallback_models,
6665
stats=MergeStats(
@@ -86,40 +85,29 @@ def merge(
8685
for tz_key, tz_entry in litellm_models.items():
8786
litellm_provider = tz_entry.get("litellm_provider", "")
8887

89-
# Check if provider is mapped to ai-models
90-
if litellm_provider not in LITELLM_TO_RESEARCH:
91-
if not config.include_deprecated and _is_litellm_deprecated(tz_entry):
92-
deprecated_removed += 1
93-
continue
94-
output[tz_key] = tz_entry
95-
unmatched += 1
96-
continue
97-
98-
# Extract model name for lookup
99-
original_key = tz_entry.get("metadata", {}).get("original_key", "")
100-
model_name = extract_model_name(litellm_provider, original_key)
101-
research_provider = LITELLM_TO_RESEARCH[litellm_provider]
88+
# Try to find a matching ai-models entry
89+
research_entry: dict | None = None
90+
if litellm_provider in LITELLM_TO_RESEARCH:
91+
original_key = tz_entry.get("metadata", {}).get("original_key", "")
92+
model_name = extract_model_name(litellm_provider, original_key)
93+
research_provider = LITELLM_TO_RESEARCH[litellm_provider]
94+
research_entry = research_lookup.get((research_provider, model_name))
95+
96+
# Determine deprecation status
97+
is_deprecated = _is_litellm_deprecated(tz_entry) or (
98+
research_entry is not None and research_entry.get("isDeprecated", False)
99+
)
102100

103-
# Look up in ai-models
104-
research_key = (research_provider, model_name)
105-
research_entry = research_lookup.get(research_key)
101+
if is_deprecated and not config.include_deprecated:
102+
deprecated_removed += 1
103+
continue
106104

105+
# No match — keep original LiteLLM entry as-is
107106
if research_entry is None:
108-
if not config.include_deprecated and _is_litellm_deprecated(tz_entry):
109-
deprecated_removed += 1
110-
continue
111107
output[tz_key] = tz_entry
112108
unmatched += 1
113109
continue
114110

115-
# Check deprecated (ai-models flag OR LiteLLM deprecation_date)
116-
is_deprecated = research_entry.get("isDeprecated", False) or _is_litellm_deprecated(
117-
tz_entry
118-
)
119-
if is_deprecated and not config.include_deprecated:
120-
deprecated_removed += 1
121-
continue
122-
123111
# Overlay cost fields from ai-models
124112
research_costs = research_entry.get("costs", {})
125113
updated_entry = dict(tz_entry)

tests/test_client.py

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import asyncio
21
from unittest.mock import AsyncMock
32

43
import httpx
@@ -124,23 +123,3 @@ async def test_non_source_fetch_error_in_ai_models_is_caught(config):
124123
assert len(result.models) > 0
125124
# ai-models was not available
126125
assert result.ai_models_fetched_at is None
127-
128-
129-
# ---------- BUG-7: fetch_catalog_sync from a running event loop ----------
130-
131-
132-
def test_fetch_catalog_sync_from_running_loop(config):
133-
"""fetch_catalog_sync works when called from within a running event loop (ThreadPoolExecutor path)."""
134-
zip_bytes = build_ai_models_zip()
135-
136-
async def _run_in_thread() -> None:
137-
with respx.mock:
138-
respx.get(LITELLM_URL).mock(return_value=httpx.Response(200, json=SAMPLE_LITELLM_DATA))
139-
respx.get(AI_MODELS_URL).mock(return_value=httpx.Response(200, content=zip_bytes))
140-
client = CatalogClient(config)
141-
loop = asyncio.get_running_loop()
142-
result = await loop.run_in_executor(None, client.fetch_catalog_sync)
143-
assert len(result.models) > 0
144-
assert result.stats.total_litellm > 0
145-
146-
asyncio.run(_run_in_thread())

0 commit comments

Comments
 (0)