fix: improve exception handling with try-except, debug logging, and specific error types - #34
Conversation
jakepresent
left a comment
There was a problem hiding this comment.
Nice work - the contextual error messages and debug tracebacks will make 1k+ scale runs much more debuggable.
One question before I approve: re-raising LLMRateLimitError and LLMProviderError from the rollout/judge workers (vs the previous catch-all-Exception path) means a single transient Azure hiccup at e.g. seed 500 of a 1k run will now kill the whole stage instead of marking one row failed. Is there retry-with-backoff above the worker layer that I'm missing, or is the expectation that callers rely on cache-resume from #32 to restart the run? Want to make sure that's intentional before signing off, especially with scaling work landing this week.
Two tiny things:
import aiohttpinsideHTTPEndpointSession.run_turnshould probably hoist to module level - it's imported on every turn call. Minor.HTTPEndpointSessionErrorTestuses port 1 to force connection-refused; could be flaky in containers/CI environments where port 1 behavior differs. A closed high port would be more portable. Nit.
Wrap unguarded file reads and parse calls with specific exception handlers that include the file path in the error message: - config.py: load_config catches FileNotFoundError, PermissionError, yaml.YAMLError and re-raises as ConfigError - otel.py: _parse_otlp_json catches FileNotFoundError, JSONDecodeError - tools.py: load_toolset_file catches FileNotFoundError, yaml.YAMLError - systematization_convert.py: run_systematization_to_policy catches FileNotFoundError, JSONDecodeError
- CallableSession.open: catch ModuleNotFoundError, AttributeError with actionable messages including the callable ref string - OTelTracedSession.open: same pattern for OTel-traced callable imports - HTTPEndpointSession.run_turn: catch aiohttp.ClientResponseError and ClientError with endpoint URL and HTTP status in the message
- judge.py: catch JSONDecodeError when loading policy file, add LLM error types and json/ValueError before the catch-all in the worker, log traceback via log.debug for all caught worker exceptions - policy.py: include model name and raw response text in the error message when policy generation returns non-JSON output
- rollout.py worker: add LLM error types re-raise before catch-all, add ValueError/KeyError handler, log.debug with traceback and seed_id for all caught worker exceptions - rollout.py auditor: log.debug with traceback on each retry failure - systematization.py: catch JSONDecodeError on json.loads fallback with raw text snippet in the error message
Catch ConnectionError separately from generic Exception in PhoenixCollector.get_spans with a targeted message about Phoenix connectivity. Validate 'context.trace_id' column existence before filtering.
Cover all new error handling across config, otel, tools, session, otel_session, judge, systematization_convert, rollout, and collector. Verify exception types, error messages include context (file paths, module names, endpoint URLs), and debug logging fires with tracebacks in worker failures.
CI environment doesn't include pandas (optional dependency). The PhoenixCollector.get_spans method does 'import pandas' at the top, which fails before our exception handling runs.
…cache Align artifact_cache.py and viewer_read_model.py with the centralized logging convention from PR #22. Replace 10 print(file=sys.stderr) calls with log.warning() using structured placeholders so they respect --quiet, --log-file, and --output json. Remove unused sys imports from both modules.
dc7b854 to
8b03345
Compare
… test - Move 'import aiohttp' from run_turn to open(), store as self._aiohttp to avoid re-importing on every turn call - Use port 59123 instead of port 1 in HTTPEndpointSession test for better portability across CI/container environments
tangym
left a comment
There was a problem hiding this comment.
Thanks for the thorough review Jake!
Re: LLMRateLimitError / LLMProviderError re-raise killing the stage — this is intentional and there's a two-layer safety net:
-
Retry with backoff (PR #17, already merged):
model_client.pywraps allgeneratecalls with_with_retries— rate limits get 5 retries with coordinated per-model cooldown (honorsRetry-After), provider 5xx gets exponential backoff (1s × 2^attempt, capped at 120s). By the timeLLMRateLimitErrorreaches the worker, it's already exhausted all retry attempts. -
Cache-resume (PR #32, already merged): If the stage does abort after max retries,
p2m runpicks up where it left off — completed seeds are hashed and skipped on the next run. So the user just re-runs the same command and only the failed seeds are re-attempted.
The flow is: transient failure → _with_retries handles it silently (up to 5×) → if still failing, worker raise → stage aborts → user re-runs → cache-resume skips completed seeds.
Without the re-raise, a sustained rate limit would silently mark every seed as failed (since the old catch-all just returned the error), and the user would get a full "scores" artifact with 100% errors — misleading.
Nits fixed in b551234:
- Hoisted
import aiohttpfromrun_turntoopen(), stored asself._aiohttp - Changed test port from 1 to 59123 for container/CI portability
jakepresent
left a comment
There was a problem hiding this comment.
Thanks Yeming, that fully clears it - the _with_retries layer below the worker plus cache-resume above is the right shape, and you're right that silent-error-with-full-artifact would be worse than abort-with-resume. The fail-fast signal is more useful at the worker boundary.
Nits look good. Approving.
…ng the main merge Earlier merge commit (81258cf) brought main into terminology-migration-pr21 and resolved the 11 base-conflict files. This commit lands the follow-on rename sweep that the merge surfaced but didn't itself apply, so the rename is consistent across the merged code: * run_systematization_to_policy -> run_systematization_to_taxonomy * "Rollout worker" log strings -> "Inference worker" * tests/test_exception_handling.py: JudgePolicyParseErrorTest -> JudgeTaxonomyParseErrorTest; RolloutWorkerLoggingTest -> InferenceWorkerLoggingTest; imports updated to TesterConfig / InferenceConfig / run_inference / run_systematization_to_taxonomy. Conflict-resolution rule (uniform): take main's structure/logic (try/except, ctx fallbacks, runSeedRows helper, rewrite_seed_path logic), keep HEAD's renames (taxonomy_path, taxonomy_json, inference, tester, spec). 613 unit tests pass with the 4 known Windows-only flakes deselected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Absorb second wave from main: PR #34 (exception handling) plus PR #32 trailing commits (artifact-cache atomic version + manifest path validation, rewrite_seed_path inference flag). Apply renames consistently to all incoming code: - "Rollout worker" -> "Inference worker" - run_systematization_to_policy -> run_systematization_to_taxonomy - JudgePolicyParseErrorTest -> JudgeTaxonomyParseErrorTest - RolloutWorkerLoggingTest -> InferenceWorkerLoggingTest - rollout module / log tag -> inference - policy_path / policy_json -> taxonomy_path / taxonomy_json - auditor -> tester, concept -> spec Conflict rule: take main's structure/logic (try/except, ctx fallbacks, runSeedRows helper, rewrite_seed_path, atomic version allocation), keep HEAD's renames. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Catch up the viewer-fix branch with main (17 commits behind, including PR #32 artifact-cache + PR #34 exception handling + PR #40 seeds minItems schema fix). Conflicts: - p2m/viewer_read_model.py: kept HEAD's bumped SCHEMA_VERSION = 2 + GENERATOR_VERSION = "viewer-read-model-v2", added main's log = logging.getLogger(__name__). - viewer/src/lib/server/artifacts.ts: same (kept v2 + added main's SUITE_ARTIFACTS_DIR = 'artifacts'). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Problem
Several code paths across p2m modules could throw raw, unhandled exceptions with no context — missing file paths in
FileNotFoundError, lost tracebacks in concurrent workers, and bareexcept Exceptioncatch-alls that obscure the root cause.Changes
File I/O & parse errors — Wrap unguarded
read_text(),yaml.safe_load(), andjson.loads()calls with specific exception handlers that include the file path in the error message:config.py,otel.py,tools.py,systematization_convert.pySession imports — Catch
ModuleNotFoundErrorandAttributeErrorinCallableSession.open(),OTelTracedSession.open()with actionable messages. Catchaiohttperrors inHTTPEndpointSession.run_turn().Worker tracebacks — Add
log.debug()with full traceback and seed_id in rollout and judge workers before returning errors, so tracebacks are preserved when--verboseis used.Specific exceptions before catch-alls — Add
LLMAuthError/LLMInputError/LLMRateLimitError/LLMProviderErrorre-raise andValueError/KeyError/JSONDecodeErrorhandlers before the genericexcept Exceptionin rollout and judge workers.Phoenix collector — Catch
ConnectionErrorseparately from genericExceptionwith a targeted connectivity message.Policy & systematization — Include model name, raw response text, or file path in error messages for parse failures.
Tests
21 new test cases in
tests/test_exception_handling.pycovering all error paths. Full suite: 565 passed, 0 regressions.