[OPIK-8012] [SDK] fix: bound LLM judge provider calls and retry empty responses - #7931
[OPIK-8012] [SDK] fix: bound LLM judge provider calls and retry empty responses#7931AndreiCautisanu wants to merge 5 commits into
Conversation
github-actions
Bot
added
python
⏱️ pre-commit per-hook timing
⏭️ 38 skipped (no matching files changed)
|
|
No test needed here. Bounding the LiteLLM timeout/num_retries and retrying EmptyLLMResponseError only shows up when the provider hangs or returns nothing — neither is reproducible on a fresh OSS install without stubbing litellm, and the judge needs a provider key regardless. The one e2e spec on this path, test-suites/test-suites-smoke.spec.ts, skips without ANTHROPIC_API_KEY/OPENAI_API_KEY and asserts a happy-path 3/3 pass, so it could not observe these defaults either way. The unit tests you added (test_litellm_chat_model.py timeout/caller-override cases, test_llm_judge.py retries-and-succeeds vs permanent-error-not-retried) sit at the right level for this; nothing to add on top. Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 20 Aug 13:11 UTC — nothing the verdict depends on changed. |
… responses An empty structured-output response raises BaseLLMError, which the judge's retry policy did not cover (LLMJudgeParseError extends OpikException, not BaseLLMError), so a transient blank cost the item its whole score. LiteLLMChatModel also forwarded no timeout and no num_retries, leaving litellm's 6000s default plus its ~3 internal HTTP retries stacked under two tenacity layers — a stalled provider blocked effectively forever. Observed stalls sit in TCP connect, hence the tighter connect ceiling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
03be770 to
7983cd1
Compare
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
03be770 to
7983cd1
Compare
Address review: retrying every BaseLLMError also retried permanent auth, invalid-request and config failures. Add EmptyLLMResponseError for the transient case and retry only that. The provider-response wrapper re-wrapped BaseLLMError subclasses, which erased the new type and doubled the message prefix; let classified errors through. _finish_reason now falls back to the raw choice, since normalise_choice's non-pydantic path drops it. Drops the API-key guidance from the empty-response message and renames the new tests to the convention in .agents/skills/python-sdk/testing.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| _RETRY_POLICY = tenacity.retry( | ||
| retry=tenacity.retry_if_exception_type(LLMJudgeParseError), | ||
| retry=tenacity.retry_if_exception_type((LLMJudgeParseError, EmptyLLMResponseError)), | ||
| stop=tenacity.stop_after_attempt(3), |
There was a problem hiding this comment.
Agentic scoring still drops transient empty responses
The retry predicate at metric.py:27-29 covers only LLMJudge._generate_and_parse; LLMJudge.score routes agentic traces through _score_agentic(), whose AgenticLLMJudge._generate_and_parse retries only LLMJudgeParseError, so EmptyLLMResponseError from generate_chat_completion propagates immediately in agentic scoring while one-shot scoring retries it. Should we include EmptyLLMResponseError in the agentic retry policy and score handling?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/evaluation/suite_evaluators/llm_judge/metric.py` around lines
27-29, update the retry behavior for the agentic scoring path as well as the one-shot
`LLMJudge` path. Ensure `AgenticLLMJudge._generate_and_parse` retries
`EmptyLLMResponseError` alongside `LLMJudgeParseError`, and update `_score_agentic()`
error handling if needed so transient empty responses follow the same documented retry
contract instead of propagating immediately.
There was a problem hiding this comment.
Correct — AgenticLLMJudge has its own _RETRY_POLICY at agentic/judge.py:34, so empty responses retried in one-shot scoring but propagated immediately in agentic scoring. Inconsistency was mine.
Added EmptyLLMResponseError to the agentic predicate so both paths behave identically. Exhausted-retry behaviour also matches: LLMJudgeParseError returns partial results via exc.results, everything else propagates — that's pre-existing in both paths, so I left it alone.
Test drives the decorator behaviourally rather than asserting on internals: empty response = 3 attempts, permanent BaseLLMError = 1.
There was a problem hiding this comment.
Commit 8ce942d addressed this comment by adding EmptyLLMResponseError to the agentic judge retry policy, matching one-shot behavior. Tests verify three retries for transient empty responses and no retries for permanent failures.
| except exceptions.BaseLLMError: | ||
| # Already classified (e.g. EmptyLLMResponseError); re-wrapping would | ||
| # erase the subclass callers retry on and double the message prefix. | ||
| raise |
There was a problem hiding this comment.
Classified LLM failures lose provider diagnostics
generate_chat_completion and generate_provider_response let classified errors, including EmptyLLMResponseError, bypass LOGGER.error("Failed to call LLM provider...") because except BaseLLMError: raise re-raises them from get_provider_response’s yield scope, so callers lose provider-context diagnostics — should we log before re-raising while preserving the exception type?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/evaluation/models/base_model.py` around lines 217-220 and 258-259,
update `get_provider_response` and `aget_provider_response` so classified `BaseLLMError`
exceptions are logged with the same provider-context diagnostic as other failures before
being re-raised. Preserve the original exception type and avoid wrapping or duplicating
the existing generic-error behavior.
There was a problem hiding this comment.
Confirmed and fixed — this one was a genuine regression I introduced, and an ironic one: the Failed to call LLM provider, reason: ... line is exactly what made the original CI failure diagnosable, and my except BaseLLMError: raise silenced it.
Verified with a captured log handler: before the fix, an EmptyLLMResponseError logged nothing at all. Now logs before re-raising, so the diagnostic survives and the exception type is still preserved for the retry predicate. Added a test asserting the log record is emitted.
There was a problem hiding this comment.
Commit 8ce942d addressed this comment by logging classified BaseLLMError exceptions before re-raising them in both synchronous and asynchronous provider-response paths, preserving their original types.
…stics Address review: the agentic judge has its own retry policy, so empty responses retried in one-shot scoring but propagated immediately in agentic scoring. Include EmptyLLMResponseError there too. Re-raising classified errors also skipped the LOGGER.error call, so the "Failed to call LLM provider" diagnostic vanished for exactly the failure it was needed for. Log before re-raising, preserving the type. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The async branches were only exercised indirectly. Adds ascore coverage for retry-then-succeed and no-retry-on-permanent-error, plus a direct aget_provider_response test asserting the type survives and the diagnostic is logged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| class FlakyAsyncModel: | ||
| async def agenerate_chat_completion(self, messages, response_format=None): | ||
| calls["n"] += 1 | ||
| if calls["n"] == 1: | ||
| raise exceptions.EmptyLLMResponseError( | ||
| "LLM returned no content and no tool calls" | ||
| ) | ||
| return {"role": "assistant", "content": valid} |
There was a problem hiding this comment.
Retry mutates judge request unnoticed
The retry test tracks only calls["n"], so a retry can change messages or response_format and still pass when the second response parses — should we record both arguments per invocation and assert they match across calls?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/evaluation/suite_evaluators/test_llm_judge.py` around lines
377-384, update `test_ascore__transient_empty_llm_response__retries_and_succeeds` so the
flaky model records the `messages` and `response_format` received on every invocation.
After scoring, assert that both arguments are identical between the first failed attempt
and the second successful retry, while preserving the existing retry and result
assertions.
Python SDK E2E Tests Results (Python 3.13)296 tests 289 ✅ 3m 35s ⏱️ Results for commit 685dda6. ♻️ This comment has been updated with latest results. |
2 participants
Details
Two SDK defects in the LiteLLM-backed LLM-judge path caused an intermittently failing E2E smoke test, plus one misleading error message that made them hard to diagnose. Impact is on any SDK user running
evaluate/run_testswith an LLM judge, not just tests.contentnortool_callsraisesBaseLLMError, but the judge's retry policy only caughtLLMJudgeParseError— which extendsOpikException, notBaseLLMError. The item lost its whole score even though the LLM call sits inside the retried_generate_and_parseand a retry would very likely have succeeded.litellm.completion. Neithertimeoutnornum_retrieswas forwarded, leaving litellm's 6000s (100 min) default plus its ~3 internal HTTP retries stacked on two existing tenacity layers. Thread dumps taken mid-hang show the stalls sit in TCP connect, so connect is bounded tighter (10s) than read (60s): a provider either accepts a connection promptly or is unreachable, whereas a long generation legitimately takes time. Both aresetdefault, so explicit caller values still win.modelandfinish_reason.Measured cost of one stalled call:
timeout=5withnum_retriesunset took 50.8s versus 17.1s pinned; a blackholed connect went from ~180s to 32.2s.Change checklist
Issues
Documentation
No documentation change needed. The new
timeout/num_retriesvalues are internal defaults that callers could already override by passing those kwargs toLiteLLMChatModel, and the retry-policy and error-message changes are not part of any documented API.AI-WATERMARK
AI-WATERMARK: yes
Testing
→ 321 passed, 2 skipped.
→ 794 passed, 45 failed. Those 45 are pre-existing: stashing this branch's changes and re-running gives the identical 45 (missing optional deps, plus test-ordering pollution — two
test_litellm_chat_model.pycases pass when the file runs scoped). This branch takes the suite from 790 to 794 passing.→ ruff, ruff-format, mypy all Passed.
Each of the 4 new tests was confirmed to fail without its corresponding fix (revert, re-run, restore), so none is vacuous.
End-to-end against a local stack with a real Anthropic judge: runs of the affected smoke test that previously aborted at the 90s client timeout now pass.
Not fully verified
The trigger for the first defect — a provider returning a blank
tool_useresponse — was never reproduced live. The unit test reproduces the exact exception path from the CI traceback and the fix is correct regardless, but confirming the flake is gone needs a few green CI runs after merge.For reviewers: many of my local reproductions turned out to be a broken IPv6 route on my dev machine (IPv4 connect 0.01s, IPv6 timeout after 12s) — an environment problem, not an Opik defect. GitHub-hosted Linux runners are IPv4-only, so that cannot explain the CI failures; the retry gap is what does.
Deliberately unchanged: the E2E test's 90s client timeout. CI already burns ~95s per attempt across 3 attempts and healthy judge latency is ~1.1s median, so raising it would only make failures slower.