Repository navigation
feat(tier3): enforce structured JSON schemas and rate-limit retries in LLM judges - #145
Conversation
… for LLM judges Implement zero-dependency exponential backoff with full jitter, header-aware Retry-After parsing, and defensive environment variable overrides for LLM judge calls across both the Harbor container verifier (eval.py) and the host runtime client (LLMClient). - Add skillevaluator.inference.retry with Full Jitter backoff, RFC-7231 HTTP date parsing, UTC timezone normalization, and safe defaults. - Embed self-contained retry loop in Harbor verifier template (eval.py) with socket cleanup and fail-fast behavior on non-retriable errors. - Forward retry configuration (SKILL_EVAL_LLM_MAX_RETRIES, SKILL_EVAL_LLM_RETRY_BASE_DELAY, SKILL_EVAL_LLM_RETRY_MAX_DELAY and aliases) into container task.toml via adapter allowlist. - Wrap LLMClient.completions with retry while strictly preserving agent execution logs, token counts, and trial telemetry. - Add comprehensive unit test coverage for retry logic, header parsing, container template execution, and task configuration forwarding. Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
…22 downgrade Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
…ate schema fallback Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
rng1995
left a comment
There was a problem hiding this comment.
@kweinmeister Two issues remain in the new retry/schema behavior: SDK retries multiply the configured budget, and unrelated HTTP 400/422 responses permanently disable structured output for the target. Details and reproductions are inline.
Local validation: 879 focused tests passed. Separate reproductions using the real OpenAI/Anthropic SDKs with mock HTTP transports exposed these gaps. GitHub reports all 17 checks passing. Please address the two cases and add transport-level regressions before approval.
… schema errors Disable SDK-level retries in OpenAI and Anthropic clients by passing max_retries=0 to ensure the outer backoff loop owns the retry budget. Add http_client parameter to LLMClient to support transport-level testing. Restrict schema error detection to match genuine unsupported-option errors and defer target memoization until downgrade succeeds. Mirror schema capability checks and deferred memoization in the Harbor container verifier template. Add transport-level request count regressions and unrelated 400 schema persistence tests. Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
|
@rng1995 Thanks for the review and the clear reproductions. Both issues have been resolved in commit 7280545:
All 6,519 tests and |
Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
rng1995
left a comment
There was a problem hiding this comment.
@kweinmeister Thanks for addressing both earlier findings. I verified that SDK retries are disabled and that schema fallback only caches a genuine unsupported-option response after a successful downgrade. Both existing threads can remain resolved.
One integration issue remains: the normal Harbor runner drops the three retry environment variables before staging and launching the verifier, so the documented settings are ignored there. Details and the reproduction are inline. Please forward those controls through the runner and add an integration regression before approval.
Validation on 248d129: 764 focused tests passed locally, plus independent SDK/schema and environment-forwarding probes. GitHub reports all 17 checks passing; the PR has no merge conflicts.
Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
|
@rng1995 Thanks for catching that. I pushed an update to forward the three retry environment variables through the Harbor runner into both task staging and the Harbor subprocess environment, added an integration regression test covering the full path from |
rng1995
left a comment
There was a problem hiding this comment.
@kweinmeister Thank you for your contribution and the updates. The original SDK retry and schema-fallback findings remain fixed, and host forwarding now works. Two existing blockers remain: native-task overrides can produce invalid TOML, and Bedrock ignores the configured judge retry policy. I confirmed both independently and followed up in the existing threads.
Local validation: 839 focused tests passed, one deselected; Ruff and diff checks passed. GitHub reports all 17 CI checks passing and no merge conflicts. Approval is pending the two remaining fixes.
…k retries - Replace colliding [verifier.env] keys in-place during native task staging and strip inline TOML comments - Enforce bounded judge retry policy and disable internal botocore retries for Bedrock Converse - Add configurable SKILL_EVAL_LLM_JUDGE_BUDGET_SEC wall-time deadline with 180s fallback - Refactor loose tuples to SchemaTargetKey and EvalRetryConfig NamedTuples - Consolidate shared test fixtures in conftest.py, parameterize retry tests, and sanitize dummy credentials Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
|
Pushed update in commit c7efb5a addressing the latest review feedback and code standards:
|
Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
Signed-off-by: Karl Weinmeister <kweinmeister@google.com> # Conflicts: # CHANGELOG.md # src/skillevaluator/tier3/harbor/adapter.py # src/skillevaluator/tier3/harbor/runner.py
rng1995
left a comment
There was a problem hiding this comment.
@kweinmeister Thank you for your patience through several review rounds, and for your contribution to SkillEvaluator!
I re-reviewed c992b1f. Both remaining blockers are fixed:
- Native-task
[verifier.env]collisions: colliding retry settings are now replaced by key, so stagedtask.tomlfiles parse and resolve to the host value for all three retry settings indefaultanddefault_plus_custom. Authored values are kept when no host override is set. - Bedrock judge retries: botocore retries are disabled, and Converse now follows the configured judge retry count and delay bounds. Against a loopback Converse endpoint,
MAX_RETRIES=0withAWS_MAX_ATTEMPTS=4sends one request, andMAX_RETRIES=3withAWS_MAX_ATTEMPTS=1succeeds on the third request. The new transport-level tests cover both cases.
Validation: Ruff passes, as do 1,076 focused tests and the full local suite (9,696 passed, 18 skipped). All 17 CI checks are green.
A non-blocking follow-up that also exists on main, so not something this PR needs to fix: if an authored task spells the verifier env table another way (env.KEY = ... under [verifier], env = { ... }, or [ verifier.env ]), staging writes a duplicate [verifier.env] table. The staged file then fails to parse, and nothing reports the error. Parsing the staged task.toml after editing would surface it clearly.
Approved.
Resolve conflicts with NVIDIA#145 (LLM judge retries and structured schemas): - inference/client.py: keep the ADC token refresh on 401 inside the OpenAI-compatible branch of _invoke_provider, now wrapping _call_with_schema_fallback, under retry_call_with_backoff. - templates/eval.py: keep the Vertex OpenAPI helpers next to main's schema builders, and apply the in-process ADC refresh on 401 around _urlopen_with_schema_fallback, rebuilding the request with the new token. - tests/test_tier3_public_runtime.py and CHANGELOG.md: keep both sides. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Resolve conflicts with NVIDIA#145 (LLM judge retries): - runner.py: import both _VERIFIER_RETRY_ENV_VARS and _native_entry_id. - tests/test_tier3_public_runtime.py: keep both sides' new tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
…aluation Brings in 266f597 "feat(tier3): enforce structured JSON schemas and rate-limit retries in LLM judges (#145)". Four files conflicted; the rest (llm_judge.py, inference/client.py, inference/retry.py, adapter.py and #145's new tests) merged cleanly. One judge retry path, not two - Both sides added retries to the Harbor verifier (templates/eval.py). The PR wrapped every judge call in its own loop (_call_judge_model_with_retries with _retry_judge_call: 2 s and 6 s delays, a 200 s budget). #145 retries inside _urlopen_with_retry / _urlopen_with_schema_fallback (full-jitter backoff, Retry-After, SKILL_EVAL_LLM_MAX_RETRIES / RETRY_BASE_DELAY / RETRY_MAX_DELAY forwarded through task.toml, and the per-judge deadline _ACTIVE_JUDGE_DEADLINE with SIGALRM). Keeping both would multiply attempts (3 outer x 4 inner) and break the time budget, so the merge keeps only #145's framework. _call_public_llm_with_provenance is #145's single loop. - Removed the PR-only helpers: _JUDGE_REQUEST_TIMEOUT_SEC, _JUDGE_RETRY_DELAYS_SEC, _JUDGE_RETRY_BUDGET_SEC, _judge_retry_sleep, _retry_judge_call and _call_judge_model_with_retries. Request timeouts are #145's 90 s, capped by the time left for the judge. - The PR's error rules that #145 lacked now live in #145's classifier, pulled out of _urlopen_with_retry as _is_transient_judge_error (the name the CI smoke script already calls): * HTTP 408 is retried (#145's Bedrock classifier already did this); otherwise #145's 429/500/502/503/504 list stands, so the PR's "any 5xx" is narrowed to it. * http.client.IncompleteRead (a body cut short) is retried. * A failed TLS certificate check (SSLCertVerificationError, bare or inside URLError) is never retried. * A URLError whose reason is not a network error (OSError) is retried only when it says "timed out", as in the PR. * An exhausted judge time budget is never retried. * Python 3.9 socket.timeout stays retried (it is an OSError). - Tests: the PR tests in tests/tier3/test_load_census_truth.py now check the merged path. They record #145's sleeps through a time shim, set SKILL_EVAL_LLM_MAX_RETRIES=2 so the three-503 case still gives up after three attempts, add a 408 case, check the full-jitter bounds, add classifier tables (socket.timeout, IncompleteRead, cert errors, budget exhaustion, HTTP codes) and a "cert failure is not retried" call test. The time-budget test now runs under the judge deadline: the retry only gets the time left, and no retry starts once it is spent. tests/test_verifier_error_redaction.py drops the _judge_retry_sleep stub. Its redaction checks are unchanged, and its transient-error tests already stub time.sleep. scripts/ci/smoke_harbor_verifier.py also checks a certificate failure on the old-Python smoke run. _call_required_judge - Keeps the PR's allow_not_applicable keyword and N/A normalization, and #145's deadline, SIGALRM timer, and ContextVar token reset in finally. Also keeps the PR's _reward_overall and _format_log_score helpers. Behavior evidence (eval.py build_behavior_evidence) - Main only re-wrapped lines in the old function. The merge takes the PR's rewrite ("show judges what the agent wrote"). final_response_limit and the SKILL_EVAL_BEHAVIOR_CHECK_BUDGET / final-response budgets flow through build_behavior_evidence -> _behavior_evidence as the rewrite intends. Partial behavior-judge salvage - The PR rule wins in both copies. A salvaged verdict that does not judge every behavior (steps 1..N exactly once) leaves the required judge unscored (judge error), never scored as partial. The host llm_judge.py already merged to that rule (salvaged=True + _covers_every_step), and the template matches it. - tests/tier3/test_judge_parse_robustness.py keeps the PR's parametrized test_template_rejects_partial_salvage_like_eval_core and main's test_template_schema_and_payload_match_eval_core, and drops main's 3/7 salvage test. That test expected a partial salvage to score 3/7, which contradicts the stricter rule; its anti-inflation goal is covered because an unjudged behavior can no longer count at all. Template and host agree on schemas, payloads and salvage (parity tests pass). Other files - runner.py imports both _VERIFIER_RETRY_ENV_VARS (main) and _load_mcp_servers (PR), in ruff/isort order. - eval.py imports are the union (posixpath, random, signal, stat, threading, ...) plus http.client and ssl for the classifier. - CHANGELOG.md keeps both sides' bullets. The PR's verifier-retry bullet is reworded to the merged rules (408, IncompleteRead, no cert retries). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
The merge narrowed the judge retry to #145's list (429, 500, 502, 503, 504) plus 408. That dropped Anthropic's 529 "overloaded", which is its normal "busy, try again" answer. One 529 then failed the required judge and the trial. The PR retried every 5xx, so this restores that intent. - Verifier (templates/eval.py): new _is_retriable_http_status. It retries 408, 429 and every 5xx except 501 and 505, which never change on a retry. Both the urllib classifier (_is_transient_judge_error) and the Bedrock classifier use it. _RETRIABLE_HTTP_CODES is gone. - Host (inference/retry.py): is_retriable_status_code uses the same rule, so the host LLMClient also retries 408 and 529. - Tests: 529, 520, 501 and 505 rows in both status tables, an end-to-end 529-then-200 and a 501 no-retry case for the verifier, and a parity test that the host and the verifier retry exactly the same statuses. - CHANGELOG: the verifier retry bullet lists the wider status rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
The structured-output check (#145) reads the body of every HTTP 400/422 to see if the provider rejected response_format / output_config. It then set error.fp = io.BytesIO(body), but HTTPError.read() goes through a tempfile wrapper that reads error.file and caches the bound read, so the next read returned b"". Two things broke: the model fallback (LLM_JUDGE_FALLBACK_MODELS) no longer fired on a 400/422 that said "invalid model" or "model not found", and the judge error shrank to "HTTP 400: Bad Request" with no provider detail. The PR side had working 400/422 fallback; main had the same bug. - Verifier (templates/eval.py): new _peek_http_error_body reads the body and points error.fp and error.file at a fresh BytesIO, dropping the cached read, so later readers get the body again. Checked on Python 3.9, 3.11, 3.12, 3.13 and 3.14. - Host (inference/client.py): _is_schema_unsupported_error uses the same helper. The host goes through the SDKs, so this path is not hit today, but the two copies now match. - Tests: a 400 and a 422 "invalid model" with a schema request now fall back to the next model and keep the (redacted) body in the error; the unrelated-400 template test checks the body reaches the judge reason; a direct test checks error.read() still returns the body after the check, on both host and verifier. All of these fail without the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
The merge said the judge retry never retries a failed certificate check, but that only held for the urllib providers. Two paths still retried it: - Bedrock in the verifier: botocore's SSLError is an OSError, so _classify_bedrock_retry_error took it for a network blip and retried up to SKILL_EVAL_LLM_MAX_RETRIES times with backoff. The PR never retried Bedrock; #145 added Bedrock retries without the cert exception. - The host LLMClient (#145's inference/retry.py): the OpenAI and Anthropic SDKs raise APIConnectionError with the cert error chained under httpx's ConnectError. That type name is retried, so one judge call against a self-signed cert made 4 connections. Fix: one helper, _is_certificate_failure, in both copies. It walks the error, URLError.reason, botocore's kwargs["error"], args, __cause__ and __context__ (bounded, cycle-safe) looking for ssl.SSLCertVerificationError. The verifier's urllib classifier and Bedrock classifier both use it, and the host's is_retriable_exception checks it before the type-name rules. A dropped connection is still retried. Tests: classifier rows for a chained cert error and for botocore's SSLError (as botocore raises it over urllib3, and with the cert error directly in kwargs), a Bedrock transport test (one request, no sleep), a host LLMClient test over httpx (one connection), host classifier rows for urllib and SDK-wrapped cert errors, and a host/verifier parity test. All new cert tests fail without the fix. The CHANGELOG now says the rule covers every judge path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
…mages #145's _parse_retry_after in the verifier did `from datetime import UTC` inside a broad `except Exception`. UTC only exists on Python 3.11+, so on the 3.9/3.10 task images the verifier supports, an HTTP-date Retry-After was silently replaced by the 1 s base delay, and a date past SKILL_EVAL_LLM_RETRY_MAX_DELAY no longer failed fast. Numeric values were fine. The merge brought the two sides' intents together (3.9 support from the PR, the parser from #145), so neither side alone was wrong. - templates/eval.py: use datetime.timezone.utc (noqa UP017, as the repo does for other Python 3.9 shims). The host copy in inference/retry.py runs on the host's Python 3.12+ and keeps datetime.UTC. - tests/test_retry.py: with datetime.UTC removed, a date 20 s ahead parses to about 20 s and a date an hour ahead still fails fast. It fails without the fix. - scripts/ci/smoke_harbor_verifier.py: the CI 3.9/3.11 smoke run now checks an HTTP-date Retry-After and that a 400 body survives the structured-output check (that fix leans on HTTPError internals, so it is worth running on old Pythons). It passes on 3.9.6 and 3.11.14 locally and fails on 3.9 with the previous template. - CHANGELOG: one Fixed bullet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
…ue-79-harbor-upgrade Bring the Harbor upgrade up to date with main (v0.4.0 and later). Conflict resolutions: - Case identity: the runner's trusted selector map stays authoritative. Main's staged-task resolver (#161) applies only to callers without a map, where task_id.path now outranks task_name. Grader-authored rewards can no longer set collector-owned identity flags. - Agent import paths: only the OpenAI-compatible gateway Codex wrapper (#152) may run on Harbor-native cloud backends. It is passed through Harbor's unified --agent selector, not the deprecated --agent-import-path. - Verifier env: under standard grading, native tasks still may not author judge retry or evidence-budget values (#145, #160). Operators stage them as ${NAME} placeholders, which are forwarded with the other provider controls. - Native task TOML edits stay structural and Harbor-validated; main's line-based helpers are removed. - Redaction keeps main's linear-time private-key handling and terminal-control stripping plus the PR's URI userinfo masking. Whole environment values shorter than four characters are no longer exact secrets, so flags such as "1" cannot erase counts in result summaries; short URI userinfo fragments are still redacted. - The tier3 workflow (#152) forwards --environment-kwarg to preflight and defers remote ack and openshift checks. Parallel catalog children forward --environment-kwarg. - CHANGELOG: the PR's entries move back under Unreleased. Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
When running Tier 3 evaluations with higher concurrency or across multi-case datasets, LLM judge grading (
accuracy,goal_accuracy, andbehavior_check) can fail in two ways:_parse_json_responseto fail even withmax_tokens=4096.HTTP 429rate limits or transient502/503/504gateway errors.This change updates both the host judge path (
src/skillevaluator/inference/client.py,src/skillevaluator/tier3/eval_core/llm_judge.py) and the container verifier (src/skillevaluator/tier3/harbor/templates/eval.py) to:response_schemaandschema_namedefinitions (ACCURACY_JUDGE_SCHEMA,GOAL_ACCURACY_JUDGE_SCHEMA,BEHAVIOR_CHECK_JUDGE_SCHEMA) to the LLM client (response_formatfor OpenAI-compatible endpoints andoutput_configfor Anthropic endpoints).response_format={"type": "json_object"}for OpenAI-compatible, or prompt-based JSON for Anthropic) if a provider returnsHTTP 400or422, logging a warning and memoizing the target so subsequent judge calls do not repeat failed schema requests.HTTP 429and transient5xx/connection errors using exponential backoff with full jitter andRetry-Afterheader parsing, configurable viaSKILL_EVAL_LLM_MAX_RETRIES,SKILL_EVAL_LLM_RETRY_BASE_DELAY, andSKILL_EVAL_LLM_RETRY_MAX_DELAY.Verification
make lintmake testmake buildRelease Impact
CHANGELOG.md