From b34202daed0ff1492af87f97264fb7a39cbd3847 Mon Sep 17 00:00:00 2001 From: Bauke Brenninkmeijer Date: Tue, 1 Sep 2026 10:52:58 +0200 Subject: [PATCH 1/4] fix: count a job-reported error as a failed row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `process_job` read only `name` and `output` from a job's return value, so `JobResult.error` was set only when the job raised. A job that catches its own failure and reports it instead — which is what `wrap_simulation_agent` does, since one dead row must not kill the batch — came back indistinguishable from a clean one: `Failed Jobs 0`, `Success Rate 100%`, exit 0, on a run whose conversation never happened. Reproduced with an invalid `ORQ_API_KEY`: the simulation 401'd, the runner returned `terminated_by=error`, and the run reported a full pass. The signal already existed and was thrown away. `to_open_responses` records `error` and `status: 'failed'`, but nested inside `output`, where nothing reads it. Two parts. `process_job` now honours a top-level `error` key, keeping the output so the transcript survives for diagnosis — unlike the raise path, which discards it. `wrap_simulation_agent`'s job emits the key unconditionally, `None` on success and the runner's reason when it ended in `error` or `timeout`. This gives the CLAUDE.md house rule — a job that calls a target emits the `error` key unconditionally — a mechanism on the core `evaluatorq()` path, where it previously had none; only the red team path honoured it. `check_pass_failures(treat_errors_as_failure=True)` now catches these rows. A dict payload is flattened to its `message` rather than stringified, because `str()` on a dict renders a Python repr that reaches the results table and any judge reading the field. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + src/evaluatorq/evaluatorq.py | 5 +- src/evaluatorq/processings.py | 41 ++++++++- src/evaluatorq/simulation/api.py | 10 ++- src/evaluatorq/simulation/wrap_agent.py | 7 ++ src/evaluatorq/types.py | 8 +- .../simulation/test_simulate_job_error_key.py | 87 +++++++++++++++++++ tests/simulation/test_wrap_agent.py | 34 ++++++++ tests/unit/test_processings.py | 85 ++++++++++++++++++ 9 files changed, 272 insertions(+), 6 deletions(-) create mode 100644 tests/simulation/test_simulate_job_error_key.py diff --git a/CHANGELOG.md b/CHANGELOG.md index fa2c299df..8cf015a78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ All notable changes to `evaluatorq` are documented here. - **`sim_model=` is removed from every simulation entry point; `llm_config=` carries the model.** `simulate()`, `generate_and_simulate()`, `generate()`, `generate_personas()` / `generate_scenarios()` (and their singular forms) and `extend_from_experiment()` no longer accept the keyword — pass `llm_config=LLMCallConfig(model=...)`, which says the same thing and can carry `temperature`, `reasoning_effort`, `timeout_ms`, `extra_body` and a client beside it. Two spellings of one setting could not report an explicitly-passed default as a contradiction: `simulate(sim_model=DEFAULT_MODEL, llm_config=LLMCallConfig(model='other'))` ran on `other` and warned about nothing. **Update any call passing `sim_model=`** — there is no deprecation shim. Unaffected: the CLI's `--sim-model` flag, which now builds that config for you, and the `model=` argument on `SimulationRunner`, the generators and the trace helpers, which still folds into a config beside it. - **`JudgeAgent` pins itself to the Responses API and logs when a config says otherwise.** `llm_config.api` is one knob for both simulation agents, and only one of them can honour every value: the judge sends function tools and `reasoning_effort` in one request, which chat completions answers with a 400 on models like `gpt-5.4-mini`, while the user simulator's plain completion works on either endpoint. Same model, two roles, two viable protocols. `LLMCallConfig(api='chat_completions')` now applies to the user simulator and is overridden on the judge with a `WARNING` naming it, where it previously reached the judge and broke the run. The pin is `JudgeAgent.REQUIRED_API`; the general default is still `BaseAgent.DEFAULT_API`. - **Every simulation agent defaults to the Responses API when its `LLMCallConfig` leaves `api` unset**, overriding that class's own `chat_completions` default. `JudgeAgentConfig` and `UserSimulatorAgentConfig` supplied this before; a caller handing an agent a bare `LLMCallConfig` used to get chat completions instead, which returns a 400 on models like `gpt-5.4-mini` the moment the judge sends function tools and `reasoning_effort` together. The default is `BaseAgent.DEFAULT_API` — a class attribute, not an environment variable, because it is a per-call setting. Set `LLMCallConfig(api='chat_completions')` to opt out. +- **A job that returns a top-level `error` key now marks the row as failed, instead of counting as a clean success.** `process_job` read only `name` and `output` from a job's return value, so `JobResult.error` was set only when the job *raised*. A job that caught its own failure and reported it — which is what `wrap_simulation_agent` does, because one dead row must not kill the batch — came back with `error=None`, and a run whose conversation never happened printed `Failed Jobs 0`, `Success Rate 100%` and exited 0. `wrap_simulation_agent`'s job now emits `error` unconditionally (`None` on success, the runner's reason when it ended in `error` or `timeout`), and `process_job` honours the key while **keeping** the output, so the transcript survives for diagnosis. `check_pass_failures(results, treat_errors_as_failure=True)` catches these rows as a result. A dict payload is flattened to its `message`; any other job already returning an `error` key for a non-failure reason will now be counted as failed. - **`LLMCallConfig.temperature` has no default — unset means the parameter is not sent, and the provider applies its own.** It previously defaulted to `1.0`, and evaluatorq's own call sites layered literals of their own on top (`0.8` for persona and first-message generation, `0.9` for edge-case scenarios, `0.7` for the executive summary and chat-completions agent calls, `0.3` for trace analysis, `0.0` for the judge). Reasoning-class models reject the parameter outright rather than clamping it — `gpt-5.6-luna` answers `400 Unsupported parameter: 'temperature' is not supported with this model` — so a hardcoded temperature on the default model turned every persona x scenario pair into a failure and `simulate()` into a `RuntimeError`. No call site sends a temperature now; a caller who wants one sets `LLMCallConfig(temperature=...)`, and a per-call `temperature=None` means unset rather than an explicit null. `BaseAgent._resolved_temperature` lost its `fallback` argument accordingly and now gates on `model_fields_set` like its sibling resolvers, so an explicit `LLMCallConfig(temperature=None)` opts an agent out rather than deferring to a call site. **The judge is the one behaviour change worth planning around: its scoring calls are no longer pinned to `temperature=0.0`, so judgments are no longer reproducible run-to-run by default.** That affects every `simulate()` caller, not only those on a reasoning model. Pass `JudgeAgentConfig(temperature=0.0)` to restore it — on a model that accepts the parameter. - **A set-but-empty `ORQ_OTEL_*` tuning variable now logs a `WARNING` and falls back to the default, instead of falling back silently.** `_env_int` treated an empty string like an unset variable, so an unresolved workflow variable in a CI `env:` block expands to the empty string and disabled the knob with no signal. Whitespace-only values are treated the same way. Related: `ORQ_OTEL_MAX_BATCH_SIZE` larger than `ORQ_OTEL_MAX_QUEUE_SIZE` is still clamped down to the queue size, but the clamp now announces itself with a `WARNING` rather than happening silently. - **`EVALUATORQ_REASONING_EFFORT` has no default — unset means the parameter is not sent, and the model applies its own.** It previously fell back to `"medium"` for the simulator's own calls (user simulator, judge). A global effort is the wrong default in both directions: on a model that does not accept the parameter it costs a rejected request plus a retry per `(model, tool shape)` — memoised per process, so a short run or CI job never amortises it — and on a model that does, it silently overrides the provider's own tuned value. Set the env var, or `LLMCallConfig.reasoning_effort` on the agent's config, when you actually want a specific effort. **Simulation only**; red teaming's `target_reasoning_effort` was already opt-in. diff --git a/src/evaluatorq/evaluatorq.py b/src/evaluatorq/evaluatorq.py index c5f6ad099..7ce8f6fdc 100644 --- a/src/evaluatorq/evaluatorq.py +++ b/src/evaluatorq/evaluatorq.py @@ -53,7 +53,10 @@ def check_pass_failures(results: EvaluatorqResult, *, treat_errors_as_failure: b (e.g. every judge call raised), also counts as a failure. Without this, an errored job has no evaluator scores and an errored evaluator leaves ``pass_`` unset, so both would be invisible here, letting a run with no usable - responses or no usable scores exit successfully. + responses or no usable scores exit successfully. A job that *raised* has no + evaluator scores; one that reported its own failure through a top-level + ``error`` key keeps its output and is still scored, so such a row can carry + both an error and a passing score — this flag is what makes it count. Returns: True if any evaluator failed (pass_=False), False otherwise diff --git a/src/evaluatorq/processings.py b/src/evaluatorq/processings.py index 33b1a2fd2..b974d9b13 100644 --- a/src/evaluatorq/processings.py +++ b/src/evaluatorq/processings.py @@ -1,4 +1,5 @@ import asyncio +import json from collections.abc import Awaitable from inspect import isawaitable from typing import TYPE_CHECKING, cast @@ -105,6 +106,35 @@ async def run_job_with_semaphore(job: Job) -> JobResult: ] +def _job_reported_error(raw: object) -> str | None: + """Flatten a job's top-level ``error`` value to the ``str`` ``JobResult`` holds. + + A job may report a failure it handled rather than raised — a target that + answered with an HTTP error, a simulation that ended in ``terminated_by=error``. + ``JobResult.error`` is a ``str``, so a dict payload is reduced to its message + rather than stringified: ``str()`` on a dict renders a Python repr, which then + reaches the results table and any judge reading the field. A dict carrying none + of the known message keys is JSON-encoded for the same reason. + """ + if raw is None: + return None + if isinstance(raw, str): + return raw or None + if isinstance(raw, dict): + for key in ('message', 'error', 'detail'): + # Membership, not truthiness: a present-but-falsy message ('' or 0) is the + # payload's answer, and falling through to the next key would report a + # different field's text as the failure. + if key in raw: + value = raw[key] + return None if value is None else str(value) or None + logger.warning('job reported an error payload with no message key: {}', sorted(raw)) + # json, not str(): repr on a dict is exactly what this function exists to keep + # out of the results table and out of any judge reading the field. + return json.dumps(raw, default=str) + return str(raw) + + async def process_job( job: Job, data_point: DataPoint, @@ -158,6 +188,15 @@ async def process_job( result = await job(data_point, row_index) job_name = cast('str', result['name']) output = cast('Output', result['output']) + # A job that reached its target and got a failure back reports it in a + # top-level 'error' key rather than raising, so its output survives for + # diagnosis. Honouring it here is the only thing separating such a run + # from a clean one: nothing raised, so without this the row counts as a + # success and the run reports a 100% pass rate over a dead target. Unlike + # the raise path, the row keeps its output and is still scored, so it can + # carry both an error and evaluator scores — + # check_pass_failures(treat_errors_as_failure=True) is what fails it. + error = _job_reported_error(result.get('error')) # Set job name on span after execution set_job_name_attribute(job_span, job_name) @@ -218,7 +257,7 @@ async def run_evaluator_with_semaphore(evaluator: Evaluator) -> EvaluatorScore: return JobResult( job_name=job_name, output=output, - error=None, + error=error, evaluator_scores=evaluator_scores, ) diff --git a/src/evaluatorq/simulation/api.py b/src/evaluatorq/simulation/api.py index a01ee8f54..6c7186434 100644 --- a/src/evaluatorq/simulation/api.py +++ b/src/evaluatorq/simulation/api.py @@ -2088,9 +2088,9 @@ def _build_simulation_job_and_cache( """ from evaluatorq.common.async_utils import await_maybe from evaluatorq.simulation.convert import to_open_responses + from evaluatorq.simulation.evaluators.scorers import UNEVALUATED_TERMINATIONS from evaluatorq.simulation.hooks import DefaultHooks from evaluatorq.simulation.runner.simulation import SimulationRunner, _error_result - from evaluatorq.simulation.types import TerminatedBy runner = SimulationRunner( target=target, @@ -2155,11 +2155,15 @@ async def job_fn(data: DataPoint, _row: int) -> dict[str, Any]: ) result.metadata['datapoint_id'] = sim_dp.id result_cache[id(data)] = result - if result.terminated_by in (TerminatedBy.error, TerminatedBy.timeout): + reason: str | None = None + if result.terminated_by in UNEVALUATED_TERMINATIONS: reason = result.metadata.get('error') or result.reason await await_maybe(resolved_hooks.on_datapoint_error(sim_dp, RuntimeError(reason))) await await_maybe(resolved_hooks.on_datapoint_complete(result)) - return {'name': job_name, 'output': to_open_responses(result, model)} + # Emitted unconditionally, None on success. The built-in scorers already report + # such a row as pass=False, but 'Failed Jobs' counts JobResult.error, so without + # the key a run whose conversation never happened still reads as 0 failures. + return {'name': job_name, 'output': to_open_responses(result, model), 'error': reason} return job_fn, result_cache, runner diff --git a/src/evaluatorq/simulation/wrap_agent.py b/src/evaluatorq/simulation/wrap_agent.py index d2558e59f..e5f8fe575 100644 --- a/src/evaluatorq/simulation/wrap_agent.py +++ b/src/evaluatorq/simulation/wrap_agent.py @@ -11,6 +11,7 @@ from evaluatorq.simulation._datapoint_io import _extract_single_datapoint from evaluatorq.simulation.adapters import from_orq_deployment from evaluatorq.simulation.convert import to_open_responses +from evaluatorq.simulation.evaluators.scorers import UNEVALUATED_TERMINATIONS from evaluatorq.simulation.types import ( DEFAULT_MODEL, Message, @@ -96,10 +97,16 @@ def wrap_simulation_agent( async def job_fn(data: DataPoint, _row: int) -> dict[str, Any]: sim_dp = _extract_single_datapoint(data) result = await runner.run(datapoint=sim_dp, max_turns=max_turns) + # Emitted unconditionally, None on success. A run the runner ended in error + # or timeout never reached the judge, so it has no verdict to score; without + # this key process_job sees a returned dict, counts the row as a success, + # and a conversation that never happened reports a 100% pass rate. + failed = result.terminated_by in UNEVALUATED_TERMINATIONS return { 'name': name, # The runner's, not `model`: `llm_config.model` wins the resolution and is what ran. 'output': to_open_responses(result, runner.model), + 'error': result.reason if failed else None, } async def aclose() -> None: diff --git a/src/evaluatorq/types.py b/src/evaluatorq/types.py index 83b8af454..2d03a55fe 100644 --- a/src/evaluatorq/types.py +++ b/src/evaluatorq/types.py @@ -138,10 +138,16 @@ class DataPointResult(BaseModel): class JobReturn(TypedDict): - """Job return structure""" + """Job return structure. + + ``error`` is optional and reports a failure the job *handled* rather than raised — + ``None`` on success, the reason otherwise. A row carrying it counts as failed, and + keeps its output for diagnosis. A job that lets failures raise omits the key. + """ name: str output: Output + error: NotRequired[str | None] Job = Callable[[DataPoint, int], Awaitable[dict[str, Any]]] diff --git a/tests/simulation/test_simulate_job_error_key.py b/tests/simulation/test_simulate_job_error_key.py new file mode 100644 index 000000000..1864d4cd0 --- /dev/null +++ b/tests/simulation/test_simulate_job_error_key.py @@ -0,0 +1,87 @@ +"""simulate()'s job must report a runner-handled failure through the top-level +``error`` key. The built-in scorers already fail such a row, but 'Failed Jobs' +counts JobResult.error, so without the key a dead run still reads as 0 failures. +""" + +from __future__ import annotations + +# ruff: noqa: S101 +from typing import Any + +import pytest + +from evaluatorq.simulation.api import _build_simulation_job_and_cache +from evaluatorq.simulation.runner.simulation import _error_result +from evaluatorq.simulation.types import ( + CommunicationStyle, + EmotionalArc, + Message, + Persona, + Scenario, + SimulationDatapoint, + SimulationResult, + StartingEmotion, + TerminatedBy, +) +from evaluatorq.types import DataPoint + + +async def _stub_target(messages: list[Message]) -> str: + return 'hi' + + +def _datapoint() -> SimulationDatapoint: + persona = Persona( + name='P', + patience=0.5, + assertiveness=0.5, + politeness=0.5, + technical_level=0.5, + communication_style=CommunicationStyle.terse, + background='bg', + emotional_arc=EmotionalArc.stable, + ) + scenario = Scenario(name='S', goal='g', context='c', starting_emotion=StartingEmotion.neutral, criteria=[]) + return SimulationDatapoint(id='dp1', persona=persona, scenario=scenario, user_system_prompt='', first_message='hi') + + +async def _run_job(result: SimulationResult) -> dict[str, Any]: + data = DataPoint(inputs={'text': 'hi'}) + sim_dp = _datapoint() + job_fn, _cache, runner = _build_simulation_job_and_cache( + job_name='sim', + sim_dp_by_id={id(data): sim_dp}, + target=_stub_target, + target_agent=None, + model='gpt-5.6-luna', + max_turns=3, + user_simulator=None, + judge=None, + generation_client=None, + hooks=None, + ) + + async def fake_run(*_args: Any, **_kwargs: Any) -> SimulationResult: + return result + + runner._run_with_timeout = fake_run # type: ignore[method-assign] # noqa: SLF001 + return await job_fn(data, 0) + + +@pytest.mark.asyncio +async def test_simulate_job_reports_a_runner_error() -> None: + out = await _run_job(_error_result('401 authentication_error')) + assert out['error'] == '401 authentication_error' + # The transcript survives the failure, as on the wrap_simulation_agent path. + assert out['output'] is not None + + +@pytest.mark.asyncio +async def test_simulate_job_emits_the_key_as_none_on_a_judged_run() -> None: + result = _error_result('unused') + result.terminated_by = TerminatedBy.judge + result.reason = 'goal achieved' + out = await _run_job(result) + # Emitted, not omitted: a missing key and a clean run must not look the same. + assert 'error' in out + assert out['error'] is None diff --git a/tests/simulation/test_wrap_agent.py b/tests/simulation/test_wrap_agent.py index e710b51ff..63cabc2ba 100644 --- a/tests/simulation/test_wrap_agent.py +++ b/tests/simulation/test_wrap_agent.py @@ -182,3 +182,37 @@ async def test_wrap_simulation_agent_rejects_removed_evaluators_kwarg(): target=lambda _msgs: "ok", evaluators=["goal_achieved"], # type: ignore[call-arg] ) + + +class TestJobReportsTermination: + """The job's top-level ``error`` key, which is what makes a dead run visible.""" + + async def _run(self, monkeypatch, terminated_by: TerminatedBy, reason: str): + result = _make_result("whatever") + result.terminated_by = terminated_by + result.reason = reason + + async def fake_run(**_kwargs): + return result + + from evaluatorq.simulation.runner import simulation as sim_runner_mod + + monkeypatch.setattr(sim_runner_mod.SimulationRunner, "run", AsyncMock(side_effect=fake_run)) + job = wrap_simulation_agent(target=lambda _messages: "unused") + return await job( + DataPoint(inputs={"persona": _FULL_PERSONA, "scenario": _FULL_SCENARIO}), + 0, + ) + + @pytest.mark.asyncio + @pytest.mark.parametrize("terminated_by", [TerminatedBy.error, TerminatedBy.timeout]) + async def test_reports_a_run_that_never_reached_the_judge(self, monkeypatch, terminated_by): + out = await self._run(monkeypatch, terminated_by, "401 authentication_error") + assert out["error"] == "401 authentication_error" + + @pytest.mark.asyncio + @pytest.mark.parametrize("terminated_by", [TerminatedBy.judge, TerminatedBy.max_turns]) + async def test_key_is_present_and_none_on_a_run_that_did(self, monkeypatch, terminated_by): + out = await self._run(monkeypatch, terminated_by, "Goal achieved") + assert "error" in out + assert out["error"] is None diff --git a/tests/unit/test_processings.py b/tests/unit/test_processings.py index 0a9008058..23fa3e35e 100644 --- a/tests/unit/test_processings.py +++ b/tests/unit/test_processings.py @@ -74,3 +74,88 @@ async def failing_promise() -> DataPoint: result = results[0] assert result.error is not None assert result.data_point.inputs == {'row_index': 7} + + +@pytest.mark.asyncio +async def test_process_job_honours_a_job_reported_error() -> None: + """A job that reports a failure it handled must not count as a successful row. + + The failure is returned, not raised, so nothing else in the run can see it: this + is the branch that keeps a dead target from reporting a 100% pass rate. + """ + from evaluatorq.evaluatorq import check_pass_failures + from evaluatorq.processings import process_job + from evaluatorq.types import DataPointResult + + async def reporting_job(_data: DataPoint, _row: int) -> dict[str, Any]: + return {'name': 'sim', 'output': {'status': 'failed'}, 'error': {'message': '401 unauthorized'}} + + result = await process_job(reporting_job, DataPoint(inputs={'text': 'hi'}), row_index=0) + + assert result.error == '401 unauthorized' + # The output survives the error — it is the transcript you diagnose from. + assert result.output == {'status': 'failed'} + assert check_pass_failures( + [DataPointResult(data_point=DataPoint(inputs={'text': 'hi'}), job_results=[result])], + treat_errors_as_failure=True, + ) + + +@pytest.mark.asyncio +async def test_process_job_leaves_error_unset_on_success() -> None: + """A job that omits the key, or sets it to None, still reports a clean row.""" + from evaluatorq.processings import process_job + + async def clean_job(_data: DataPoint, _row: int) -> dict[str, Any]: + return {'name': 'sim', 'output': 'fine', 'error': None} + + async def silent_job(_data: DataPoint, _row: int) -> dict[str, Any]: + return {'name': 'sim', 'output': 'fine'} + + for job in (clean_job, silent_job): + result = await process_job(job, DataPoint(inputs={'text': 'hi'}), row_index=0) + assert result.error is None + + +def test_job_reported_error_flattens_every_payload_shape() -> None: + """The flattener must never hand a Python repr to the results table. + + A present-but-falsy message is the payload's answer, not an absent one, and a dict + with no known message key is JSON — `str()` on it renders a repr that reaches + `collect_errors` and any judge reading `JobResult.error`. + """ + from evaluatorq.processings import _job_reported_error + + assert _job_reported_error({'message': 'boom', 'code': 401}) == 'boom' + # 'error'/'detail' must not win over a present-but-falsy 'message'. + assert _job_reported_error({'message': 0, 'error': 'other'}) == '0' + assert _job_reported_error({'message': '', 'detail': 'other'}) is None + assert _job_reported_error({'message': None, 'detail': 'other'}) is None + assert _job_reported_error({'detail': 'from detail'}) == 'from detail' + assert _job_reported_error({'weird': 'shape'}) == '{"weird": "shape"}' + unknown = _job_reported_error({'weird': object}) + assert unknown is not None and unknown.startswith('{"weird": " None: + """A reported-error row keeps its output and is still scored, unlike the raise path.""" + from evaluatorq.processings import process_job + from evaluatorq.types import EvaluationResult, Evaluator, ScorerParameter + + async def reporting_job(_data: DataPoint, _row: int) -> dict[str, Any]: + return {'name': 'sim', 'output': 'dead', 'error': 'boom'} + + async def scorer(_params: ScorerParameter) -> EvaluationResult: # noqa: RUF029 + return EvaluationResult(value=1) + + evaluator: Evaluator = {'name': 'always', 'scorer': scorer} + result = await process_job(reporting_job, DataPoint(inputs={'text': 'hi'}), row_index=0, evaluators=[evaluator]) + + assert result.error == 'boom' + assert result.evaluator_scores is not None + assert len(result.evaluator_scores) == 1 From 2a883ffd4499bca5e02a679998d291ce42919f69 Mon Sep 17 00:00:00 2001 From: Bauke Brenninkmeijer Date: Tue, 1 Sep 2026 12:41:15 +0200 Subject: [PATCH 2/4] fix: never let a reported failure flatten back to a clean row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies the agreed findings from the review of this branch. The flattener collapsed a blank message to None, so a job that reported a failure with nothing to say produced a clean row again — the bug this branch exists to close, for one payload shape. Presence is now the failure signal: an unreadable payload becomes a named placeholder and logs, and the runner's reason falls back to the termination when it is empty. Flattening delegates to common.output_adapters.output_error_text rather than a second key ladder, so a job-level and an output-level error payload cannot disagree about what the same dict means. simulate() and wrap_simulation_agent derived the failure reason two different ways — one read metadata['error'] first, the other read reason alone — so the same dead run described itself differently depending on the entry point. Both now call simulation.evaluators.scorers.failure_reason. A failed row's job span closed as OK in a trace viewer while the summary table called it failed. All three failure paths in process_job now mark the span. Docs: the simulation guide still taught the workaround for this bug, and the evaluation reference documented no way for a custom job to report a failure it handled. Both updated, including the fact that check_pass_failures gates on errors only with treat_errors_as_failure=True. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 +- docs/evaluation-reference.md | 22 +++++++- docs/guides/simulation-in-evaluatorq.md | 11 ++-- src/evaluatorq/evaluatorq.py | 8 +-- src/evaluatorq/processings.py | 52 ++++++++----------- src/evaluatorq/simulation/api.py | 11 ++-- .../simulation/evaluators/scorers.py | 16 ++++++ src/evaluatorq/simulation/wrap_agent.py | 10 ++-- src/evaluatorq/types.py | 13 +++-- tests/simulation/test_wrap_agent.py | 6 +++ tests/unit/test_processings.py | 49 +++++++++++------ 11 files changed, 124 insertions(+), 76 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cf015a78..d82e36ff9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ All notable changes to `evaluatorq` are documented here. - **`sim_model=` is removed from every simulation entry point; `llm_config=` carries the model.** `simulate()`, `generate_and_simulate()`, `generate()`, `generate_personas()` / `generate_scenarios()` (and their singular forms) and `extend_from_experiment()` no longer accept the keyword — pass `llm_config=LLMCallConfig(model=...)`, which says the same thing and can carry `temperature`, `reasoning_effort`, `timeout_ms`, `extra_body` and a client beside it. Two spellings of one setting could not report an explicitly-passed default as a contradiction: `simulate(sim_model=DEFAULT_MODEL, llm_config=LLMCallConfig(model='other'))` ran on `other` and warned about nothing. **Update any call passing `sim_model=`** — there is no deprecation shim. Unaffected: the CLI's `--sim-model` flag, which now builds that config for you, and the `model=` argument on `SimulationRunner`, the generators and the trace helpers, which still folds into a config beside it. - **`JudgeAgent` pins itself to the Responses API and logs when a config says otherwise.** `llm_config.api` is one knob for both simulation agents, and only one of them can honour every value: the judge sends function tools and `reasoning_effort` in one request, which chat completions answers with a 400 on models like `gpt-5.4-mini`, while the user simulator's plain completion works on either endpoint. Same model, two roles, two viable protocols. `LLMCallConfig(api='chat_completions')` now applies to the user simulator and is overridden on the judge with a `WARNING` naming it, where it previously reached the judge and broke the run. The pin is `JudgeAgent.REQUIRED_API`; the general default is still `BaseAgent.DEFAULT_API`. - **Every simulation agent defaults to the Responses API when its `LLMCallConfig` leaves `api` unset**, overriding that class's own `chat_completions` default. `JudgeAgentConfig` and `UserSimulatorAgentConfig` supplied this before; a caller handing an agent a bare `LLMCallConfig` used to get chat completions instead, which returns a 400 on models like `gpt-5.4-mini` the moment the judge sends function tools and `reasoning_effort` together. The default is `BaseAgent.DEFAULT_API` — a class attribute, not an environment variable, because it is a per-call setting. Set `LLMCallConfig(api='chat_completions')` to opt out. -- **A job that returns a top-level `error` key now marks the row as failed, instead of counting as a clean success.** `process_job` read only `name` and `output` from a job's return value, so `JobResult.error` was set only when the job *raised*. A job that caught its own failure and reported it — which is what `wrap_simulation_agent` does, because one dead row must not kill the batch — came back with `error=None`, and a run whose conversation never happened printed `Failed Jobs 0`, `Success Rate 100%` and exited 0. `wrap_simulation_agent`'s job now emits `error` unconditionally (`None` on success, the runner's reason when it ended in `error` or `timeout`), and `process_job` honours the key while **keeping** the output, so the transcript survives for diagnosis. `check_pass_failures(results, treat_errors_as_failure=True)` catches these rows as a result. A dict payload is flattened to its `message`; any other job already returning an `error` key for a non-failure reason will now be counted as failed. +- **A job that returns a top-level `error` key now marks the row as failed, instead of counting as a clean success.** `process_job` read only `name` and `output` from a job's return value, so `JobResult.error` was set only when the job *raised*. A job that caught its own failure and reported it — which is what the simulation jobs do, because one dead row must not kill the batch — came back with `error=None`, and a run whose conversation never happened printed `Failed Jobs 0`, `Success Rate 100%` and exited 0. Both simulation jobs — `simulate()`'s own and `wrap_simulation_agent`'s — now emit `error` unconditionally (`None` on success, the runner's reason when the run ended in `error` or `timeout`), and `process_job` honours the key while **keeping** the output, so the transcript survives for diagnosis. The job's OTel span is now marked `ERROR` on every failed row, including the two raise paths, which previously closed as `OK`. Presence is the failure signal, not the message's truthiness: a payload that flattens to nothing is reported as `job reported a failure with no readable message` and logged, rather than silently becoming a clean row. `check_pass_failures(results, treat_errors_as_failure=True)` catches these rows as a result — **that flag still defaults to `False`, so a caller who does not pass it gets the same exit code as before**; the change is to the reported counts, not to any exit status. Any other job already returning a top-level `error` key for a non-failure reason will now be counted as failed. - **`LLMCallConfig.temperature` has no default — unset means the parameter is not sent, and the provider applies its own.** It previously defaulted to `1.0`, and evaluatorq's own call sites layered literals of their own on top (`0.8` for persona and first-message generation, `0.9` for edge-case scenarios, `0.7` for the executive summary and chat-completions agent calls, `0.3` for trace analysis, `0.0` for the judge). Reasoning-class models reject the parameter outright rather than clamping it — `gpt-5.6-luna` answers `400 Unsupported parameter: 'temperature' is not supported with this model` — so a hardcoded temperature on the default model turned every persona x scenario pair into a failure and `simulate()` into a `RuntimeError`. No call site sends a temperature now; a caller who wants one sets `LLMCallConfig(temperature=...)`, and a per-call `temperature=None` means unset rather than an explicit null. `BaseAgent._resolved_temperature` lost its `fallback` argument accordingly and now gates on `model_fields_set` like its sibling resolvers, so an explicit `LLMCallConfig(temperature=None)` opts an agent out rather than deferring to a call site. **The judge is the one behaviour change worth planning around: its scoring calls are no longer pinned to `temperature=0.0`, so judgments are no longer reproducible run-to-run by default.** That affects every `simulate()` caller, not only those on a reasoning model. Pass `JudgeAgentConfig(temperature=0.0)` to restore it — on a model that accepts the parameter. - **A set-but-empty `ORQ_OTEL_*` tuning variable now logs a `WARNING` and falls back to the default, instead of falling back silently.** `_env_int` treated an empty string like an unset variable, so an unresolved workflow variable in a CI `env:` block expands to the empty string and disabled the knob with no signal. Whitespace-only values are treated the same way. Related: `ORQ_OTEL_MAX_BATCH_SIZE` larger than `ORQ_OTEL_MAX_QUEUE_SIZE` is still clamped down to the queue size, but the clamp now announces itself with a `WARNING` rather than happening silently. - **`EVALUATORQ_REASONING_EFFORT` has no default — unset means the parameter is not sent, and the model applies its own.** It previously fell back to `"medium"` for the simulator's own calls (user simulator, judge). A global effort is the wrong default in both directions: on a model that does not accept the parameter it costs a rejected request plus a retry per `(model, tool shape)` — memoised per process, so a short run or CI job never amortises it — and on a model that does, it silently overrides the provider's own tuned value. Set the env var, or `LLMCallConfig.reasoning_effort` on the agent's config, when you actually want a specific effort. **Simulation only**; red teaming's `target_reasoning_effort` was already opt-in. diff --git a/docs/evaluation-reference.md b/docs/evaluation-reference.md index f7c083f5a..c387139c9 100644 --- a/docs/evaluation-reference.md +++ b/docs/evaluation-reference.md @@ -81,6 +81,24 @@ await evaluatorq( ) ``` +### Reporting a failure the job handled + +A job that lets a failure raise needs nothing: `process_job` records it and the row counts as failed. A job that *catches* its failure to keep the rest of the batch alive — a target that answered `401`, a simulation the runner ended in `error` — must say so, because a returned dict looks like a clean run: + +```python +async def resilient_job(data: DataPoint, row: int) -> dict: + try: + answer = await call_my_agent(data.inputs["text"]) + except MyAgentError as exc: + # Keeps the partial output for diagnosis, and still fails the row. + return {"name": "my-agent", "output": None, "error": str(exc)} + return {"name": "my-agent", "output": answer, "error": None} +``` + +Emit `error` on every path, `None` on success: an omitted key and a clean run are indistinguishable, so a job that forgets the key on one branch reports a dead target as a passing one. A row with a non-empty `error` is counted in the summary table's `Failed Jobs`, keeps its output, and is still scored — so it can carry both an error and a passing evaluator score. `check_pass_failures(results, treat_errors_as_failure=True)` is what turns it into a CI failure; the default (`False`) gates on evaluator `pass_` alone. + +This is the raw-dict job contract. `@job()` wraps a function's return value into `{"name", "output"}`, so an `error` key returned from a decorated function lands *inside* `output` and is not read as a row failure — a decorated job reports failures by raising. + ## Data sources `data` accepts inline `DataPoint`s, an Orq dataset, or awaitables that resolve to `DataPoint`s — the last of which lets you stream rows in from a slow source without blocking the run: @@ -151,10 +169,12 @@ When any evaluator returns `pass_: False`, `evaluatorq()` returns the results; t from evaluatorq.evaluatorq import check_pass_failures results = await evaluatorq(...) -if check_pass_failures(results): +if check_pass_failures(results, treat_errors_as_failure=True): raise SystemExit(1) ``` +`treat_errors_as_failure=True` also gates on rows that errored — a job that raised, a job that reported its own failure, and an evaluator whose every call failed. It defaults to `False`, which gates on evaluator `pass_` alone, so a run whose target was dead throughout can pass a gate that leaves it off. + The results table gains a pass rate row — `Pass Rate | 75% (3/4)`. ## Controlling the run diff --git a/docs/guides/simulation-in-evaluatorq.md b/docs/guides/simulation-in-evaluatorq.md index 95f999af2..606adbb7c 100644 --- a/docs/guides/simulation-in-evaluatorq.md +++ b/docs/guides/simulation-in-evaluatorq.md @@ -129,12 +129,11 @@ async def main() -> None: metadata = job_result.output.get("metadata", {}) print("metadata keys:", sorted(metadata)) print("criteria:", metadata.get("criteria_results")) - # evaluatorq reports a 100% success rate for this run even when the - # conversation itself never happened: the job returned a result dict, - # so nothing errored from its point of view. Check terminated_by, or - # a dead target reads as a passing one. - if metadata.get("terminated_by") in {"error", "timeout"}: - raise SystemExit(f"simulation did not run: {metadata.get('reason')}") + # A run the runner ended in error or timeout lands in job_result.error + # and in the table's "Failed Jobs"; this exits on it instead of reading + # a partial transcript as a result. + if job_result.error: + raise SystemExit(f"simulation did not run: {job_result.error}") asyncio.run(main()) diff --git a/src/evaluatorq/evaluatorq.py b/src/evaluatorq/evaluatorq.py index 7ce8f6fdc..58ffa304e 100644 --- a/src/evaluatorq/evaluatorq.py +++ b/src/evaluatorq/evaluatorq.py @@ -53,10 +53,10 @@ def check_pass_failures(results: EvaluatorqResult, *, treat_errors_as_failure: b (e.g. every judge call raised), also counts as a failure. Without this, an errored job has no evaluator scores and an errored evaluator leaves ``pass_`` unset, so both would be invisible here, letting a run with no usable - responses or no usable scores exit successfully. A job that *raised* has no - evaluator scores; one that reported its own failure through a top-level - ``error`` key keeps its output and is still scored, so such a row can carry - both an error and a passing score — this flag is what makes it count. + responses or no usable scores exit successfully. A job that reported its own + failure through a top-level ``error`` key keeps its output and is still + scored, so such a row can carry both an error and a passing score — this + flag is what makes it count. Returns: True if any evaluator failed (pass_=False), False otherwise diff --git a/src/evaluatorq/processings.py b/src/evaluatorq/processings.py index b974d9b13..052815efb 100644 --- a/src/evaluatorq/processings.py +++ b/src/evaluatorq/processings.py @@ -1,11 +1,11 @@ import asyncio -import json from collections.abc import Awaitable from inspect import isawaitable from typing import TYPE_CHECKING, cast from loguru import logger +from .common.output_adapters import output_error_text from .job_helper import JobError from .progress import Phase, ProgressService, safe_update_progress from .types import ( @@ -106,33 +106,25 @@ async def run_job_with_semaphore(job: Job) -> JobResult: ] +_UNREADABLE_JOB_ERROR = 'job reported a failure with no readable message' + + def _job_reported_error(raw: object) -> str | None: """Flatten a job's top-level ``error`` value to the ``str`` ``JobResult`` holds. - A job may report a failure it handled rather than raised — a target that - answered with an HTTP error, a simulation that ended in ``terminated_by=error``. - ``JobResult.error`` is a ``str``, so a dict payload is reduced to its message - rather than stringified: ``str()`` on a dict renders a Python repr, which then - reaches the results table and any judge reading the field. A dict carrying none - of the known message keys is JSON-encoded for the same reason. + Presence is the failure signal, not the message's truthiness: a job that reported a + failure with nothing to say still fails its row, so a blank or unreadable payload + becomes a placeholder rather than ``None``. Flattening goes through + ``output_error_text`` — the repo's single reader of an error payload — so a + job-level and an output-level error cannot disagree on what the same payload means. """ if raw is None: return None - if isinstance(raw, str): - return raw or None - if isinstance(raw, dict): - for key in ('message', 'error', 'detail'): - # Membership, not truthiness: a present-but-falsy message ('' or 0) is the - # payload's answer, and falling through to the next key would report a - # different field's text as the failure. - if key in raw: - value = raw[key] - return None if value is None else str(value) or None - logger.warning('job reported an error payload with no message key: {}', sorted(raw)) - # json, not str(): repr on a dict is exactly what this function exists to keep - # out of the results table and out of any judge reading the field. - return json.dumps(raw, default=str) - return str(raw) + text = output_error_text({'error': raw}) + if text: + return text + logger.warning('job reported an error with no readable message: {!r}', raw) + return _UNREADABLE_JOB_ERROR async def process_job( @@ -160,6 +152,7 @@ async def process_job( JobResult containing job output and evaluator scores """ # Import tracing utilities lazily to avoid import errors when OTEL is not installed + from .common.tracing import set_span_error from .tracing.spans import ( JobSpanOptions, set_job_name_attribute, @@ -188,15 +181,12 @@ async def process_job( result = await job(data_point, row_index) job_name = cast('str', result['name']) output = cast('Output', result['output']) - # A job that reached its target and got a failure back reports it in a - # top-level 'error' key rather than raising, so its output survives for - # diagnosis. Honouring it here is the only thing separating such a run - # from a clean one: nothing raised, so without this the row counts as a - # success and the run reports a 100% pass rate over a dead target. Unlike - # the raise path, the row keeps its output and is still scored, so it can - # carry both an error and evaluator scores — - # check_pass_failures(treat_errors_as_failure=True) is what fails it. + # Returned, not raised: without this the row counts as a success. Unlike + # the raise path the output is kept and still scored, so the row can carry + # both an error and evaluator scores. See `JobReturn` for the contract. error = _job_reported_error(result.get('error')) + if error is not None: + set_span_error(job_span, error) # Set job name on span after execution set_job_name_attribute(job_span, job_name) @@ -210,6 +200,7 @@ async def process_job( job_name = e.job_name error = str(e.original_error) set_job_name_attribute(job_span, job_name) + set_span_error(job_span, error) # Return early with error if job failed return JobResult( @@ -220,6 +211,7 @@ async def process_job( ) except Exception as e: error = str(e) + set_span_error(job_span, error) # Return early with error if job failed return JobResult( diff --git a/src/evaluatorq/simulation/api.py b/src/evaluatorq/simulation/api.py index 6c7186434..e2bf2229e 100644 --- a/src/evaluatorq/simulation/api.py +++ b/src/evaluatorq/simulation/api.py @@ -2088,7 +2088,7 @@ def _build_simulation_job_and_cache( """ from evaluatorq.common.async_utils import await_maybe from evaluatorq.simulation.convert import to_open_responses - from evaluatorq.simulation.evaluators.scorers import UNEVALUATED_TERMINATIONS + from evaluatorq.simulation.evaluators.scorers import failure_reason from evaluatorq.simulation.hooks import DefaultHooks from evaluatorq.simulation.runner.simulation import SimulationRunner, _error_result @@ -2155,14 +2155,11 @@ async def job_fn(data: DataPoint, _row: int) -> dict[str, Any]: ) result.metadata['datapoint_id'] = sim_dp.id result_cache[id(data)] = result - reason: str | None = None - if result.terminated_by in UNEVALUATED_TERMINATIONS: - reason = result.metadata.get('error') or result.reason + reason = failure_reason(result) + if reason is not None: await await_maybe(resolved_hooks.on_datapoint_error(sim_dp, RuntimeError(reason))) await await_maybe(resolved_hooks.on_datapoint_complete(result)) - # Emitted unconditionally, None on success. The built-in scorers already report - # such a row as pass=False, but 'Failed Jobs' counts JobResult.error, so without - # the key a run whose conversation never happened still reads as 0 failures. + # Emitted unconditionally: an omitted key is indistinguishable from a clean run. return {'name': job_name, 'output': to_open_responses(result, model), 'error': reason} return job_fn, result_cache, runner diff --git a/src/evaluatorq/simulation/evaluators/scorers.py b/src/evaluatorq/simulation/evaluators/scorers.py index 19c002d36..21426d4e9 100644 --- a/src/evaluatorq/simulation/evaluators/scorers.py +++ b/src/evaluatorq/simulation/evaluators/scorers.py @@ -19,6 +19,22 @@ # so the reported pass/fail cannot disagree with the score computed here. UNEVALUATED_TERMINATIONS = (TerminatedBy.error, TerminatedBy.timeout) + +def failure_reason(result: SimulationResult) -> str | None: + """The reason a run ended before the judge could audit it, or ``None`` if it did. + + One derivation for every caller. `simulate()`'s job read `metadata['error']` first + while `wrap_simulation_agent`'s job read `reason` alone, so the same dead run + described itself two ways depending on the entry point. The fallback keeps the + string non-empty: a job's `error` key is a failure signal, and an empty one reads + downstream as a clean row. + """ + if result.terminated_by not in UNEVALUATED_TERMINATIONS: + return None + metadata_error = result.metadata.get('error') + return str(metadata_error or result.reason or '') or f'simulation terminated by {result.terminated_by.value}' + + SimulationScorer = Callable[[SimulationResult], float] _WEIGHT_SUM_TOLERANCE = 1e-9 diff --git a/src/evaluatorq/simulation/wrap_agent.py b/src/evaluatorq/simulation/wrap_agent.py index e5f8fe575..5b482f092 100644 --- a/src/evaluatorq/simulation/wrap_agent.py +++ b/src/evaluatorq/simulation/wrap_agent.py @@ -11,7 +11,7 @@ from evaluatorq.simulation._datapoint_io import _extract_single_datapoint from evaluatorq.simulation.adapters import from_orq_deployment from evaluatorq.simulation.convert import to_open_responses -from evaluatorq.simulation.evaluators.scorers import UNEVALUATED_TERMINATIONS +from evaluatorq.simulation.evaluators.scorers import failure_reason from evaluatorq.simulation.types import ( DEFAULT_MODEL, Message, @@ -97,16 +97,12 @@ def wrap_simulation_agent( async def job_fn(data: DataPoint, _row: int) -> dict[str, Any]: sim_dp = _extract_single_datapoint(data) result = await runner.run(datapoint=sim_dp, max_turns=max_turns) - # Emitted unconditionally, None on success. A run the runner ended in error - # or timeout never reached the judge, so it has no verdict to score; without - # this key process_job sees a returned dict, counts the row as a success, - # and a conversation that never happened reports a 100% pass rate. - failed = result.terminated_by in UNEVALUATED_TERMINATIONS return { 'name': name, # The runner's, not `model`: `llm_config.model` wins the resolution and is what ran. 'output': to_open_responses(result, runner.model), - 'error': result.reason if failed else None, + # Emitted unconditionally: an omitted key is indistinguishable from a clean run. + 'error': failure_reason(result), } async def aclose() -> None: diff --git a/src/evaluatorq/types.py b/src/evaluatorq/types.py index 2d03a55fe..72d756868 100644 --- a/src/evaluatorq/types.py +++ b/src/evaluatorq/types.py @@ -141,8 +141,15 @@ class JobReturn(TypedDict): """Job return structure. ``error`` is optional and reports a failure the job *handled* rather than raised — - ``None`` on success, the reason otherwise. A row carrying it counts as failed, and - keeps its output for diagnosis. A job that lets failures raise omits the key. + ``None`` on success, the reason otherwise. A row whose ``error`` flattens to a + non-empty string is counted in the summary table's ``Failed Jobs`` and fails + ``check_pass_failures(treat_errors_as_failure=True)``; it keeps its output and is + still scored, so it can carry both an error and passing evaluator scores. An + omitted key and an explicit ``None`` are indistinguishable to the consumer — + emitting ``None`` on success is a producer convention, so that a job that forgot + the key cannot be mistaken for one that reported a clean run. A job that lets its + failures raise omits the key. Note that ``Job`` types this return as + ``dict[str, Any]``, so nothing type-checks a job against this shape. """ name: str @@ -151,7 +158,7 @@ class JobReturn(TypedDict): Job = Callable[[DataPoint, int], Awaitable[dict[str, Any]]] -"""Job function type - returns a dict with 'name' and 'output' keys""" +"""Job function type - returns a ``JobReturn``-shaped dict ('name', 'output', optional 'error')""" class ScorerParameter(TypedDict): diff --git a/tests/simulation/test_wrap_agent.py b/tests/simulation/test_wrap_agent.py index 63cabc2ba..81bf0bb06 100644 --- a/tests/simulation/test_wrap_agent.py +++ b/tests/simulation/test_wrap_agent.py @@ -216,3 +216,9 @@ async def test_key_is_present_and_none_on_a_run_that_did(self, monkeypatch, term out = await self._run(monkeypatch, terminated_by, "Goal achieved") assert "error" in out assert out["error"] is None + + @pytest.mark.asyncio + async def test_a_failure_with_no_message_still_reports_one(self, monkeypatch): + """An empty reason must not flatten to None downstream — that is a clean row again.""" + out = await self._run(monkeypatch, TerminatedBy.error, "") + assert out["error"] == "simulation terminated by error" diff --git a/tests/unit/test_processings.py b/tests/unit/test_processings.py index 23fa3e35e..b724278b3 100644 --- a/tests/unit/test_processings.py +++ b/tests/unit/test_processings.py @@ -88,7 +88,7 @@ async def test_process_job_honours_a_job_reported_error() -> None: from evaluatorq.types import DataPointResult async def reporting_job(_data: DataPoint, _row: int) -> dict[str, Any]: - return {'name': 'sim', 'output': {'status': 'failed'}, 'error': {'message': '401 unauthorized'}} + return {'name': 'sim', 'output': {'status': 'failed'}, 'error': '401 unauthorized'} result = await process_job(reporting_job, DataPoint(inputs={'text': 'hi'}), row_index=0) @@ -117,30 +117,45 @@ async def silent_job(_data: DataPoint, _row: int) -> dict[str, Any]: assert result.error is None -def test_job_reported_error_flattens_every_payload_shape() -> None: - """The flattener must never hand a Python repr to the results table. +def test_job_reported_error_never_loses_a_reported_failure() -> None: + """Presence is the failure signal — a blank payload must not flatten to a clean row. - A present-but-falsy message is the payload's answer, not an absent one, and a dict - with no known message key is JSON — `str()` on it renders a repr that reaches - `collect_errors` and any judge reading `JobResult.error`. + An empty message is exactly the case that put this PR here: nothing raised, so a + row whose ``error`` flattened to ``None`` counted as a success over a dead target. + Flattening delegates to ``output_error_text`` so a job-level and an output-level + error payload cannot disagree about what the same dict means. """ - from evaluatorq.processings import _job_reported_error + from evaluatorq.processings import _UNREADABLE_JOB_ERROR, _job_reported_error - assert _job_reported_error({'message': 'boom', 'code': 401}) == 'boom' - # 'error'/'detail' must not win over a present-but-falsy 'message'. - assert _job_reported_error({'message': 0, 'error': 'other'}) == '0' - assert _job_reported_error({'message': '', 'detail': 'other'}) is None - assert _job_reported_error({'message': None, 'detail': 'other'}) is None - assert _job_reported_error({'detail': 'from detail'}) == 'from detail' - assert _job_reported_error({'weird': 'shape'}) == '{"weird": "shape"}' - unknown = _job_reported_error({'weird': object}) - assert unknown is not None and unknown.startswith('{"weird": " None: + """The user-visible claim: `Failed Jobs` and `Success Rate` move off 0 and 100%.""" + from evaluatorq.table_display import create_summary_display + from evaluatorq.types import DataPointResult, JobResult + + dead = DataPointResult( + data_point=DataPoint(inputs={'text': 'hi'}), + job_results=[JobResult(job_name='sim', output='dead', error='401 unauthorized', evaluator_scores=[])], + ) + rows = {cells[0]: cells[1].plain for cells in zip(*(col._cells for col in create_summary_display([dead]).columns))} + + assert rows['Failed Jobs'] == '1' + assert rows['Success Rate'] == '0%' + + @pytest.mark.asyncio async def test_process_job_scores_a_reported_error_row() -> None: """A reported-error row keeps its output and is still scored, unlike the raise path.""" From a48037fbe3d806adff1a73d994dd0b3e0b7726a9 Mon Sep 17 00:00:00 2001 From: Bauke Brenninkmeijer Date: Tue, 1 Sep 2026 13:25:32 +0200 Subject: [PATCH 3/4] fix: stop scoring a row the job reported dead, and report LangChain target failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit process_job skipped nothing when a job reported its own failure, so a dead transcript still ran every evaluator — an LLM judge call per row for a verdict on a conversation that never happened. The row keeps its output for diagnosis and skips its evaluators, matching the raise path's zero scores. wrap_langchain_agent now catches a failing invoke/ainvoke and reports it through the top-level error key instead of raising; a caller mistake (no prompt and no usable messages column) still raises. --- CHANGELOG.md | 2 +- docs/evaluation-reference.md | 4 +-- src/evaluatorq/evaluatorq.py | 8 ++--- .../langchain_integration/wrap_agent.py | 29 +++++++++++++--- src/evaluatorq/processings.py | 17 +++++++--- src/evaluatorq/types.py | 5 +-- tests/unit/test_processings.py | 17 +++++++--- tests/unit/test_wrap_agent.py | 34 +++++++++++++++++++ 8 files changed, 95 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d82e36ff9..b13273614 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ All notable changes to `evaluatorq` are documented here. - **`sim_model=` is removed from every simulation entry point; `llm_config=` carries the model.** `simulate()`, `generate_and_simulate()`, `generate()`, `generate_personas()` / `generate_scenarios()` (and their singular forms) and `extend_from_experiment()` no longer accept the keyword — pass `llm_config=LLMCallConfig(model=...)`, which says the same thing and can carry `temperature`, `reasoning_effort`, `timeout_ms`, `extra_body` and a client beside it. Two spellings of one setting could not report an explicitly-passed default as a contradiction: `simulate(sim_model=DEFAULT_MODEL, llm_config=LLMCallConfig(model='other'))` ran on `other` and warned about nothing. **Update any call passing `sim_model=`** — there is no deprecation shim. Unaffected: the CLI's `--sim-model` flag, which now builds that config for you, and the `model=` argument on `SimulationRunner`, the generators and the trace helpers, which still folds into a config beside it. - **`JudgeAgent` pins itself to the Responses API and logs when a config says otherwise.** `llm_config.api` is one knob for both simulation agents, and only one of them can honour every value: the judge sends function tools and `reasoning_effort` in one request, which chat completions answers with a 400 on models like `gpt-5.4-mini`, while the user simulator's plain completion works on either endpoint. Same model, two roles, two viable protocols. `LLMCallConfig(api='chat_completions')` now applies to the user simulator and is overridden on the judge with a `WARNING` naming it, where it previously reached the judge and broke the run. The pin is `JudgeAgent.REQUIRED_API`; the general default is still `BaseAgent.DEFAULT_API`. - **Every simulation agent defaults to the Responses API when its `LLMCallConfig` leaves `api` unset**, overriding that class's own `chat_completions` default. `JudgeAgentConfig` and `UserSimulatorAgentConfig` supplied this before; a caller handing an agent a bare `LLMCallConfig` used to get chat completions instead, which returns a 400 on models like `gpt-5.4-mini` the moment the judge sends function tools and `reasoning_effort` together. The default is `BaseAgent.DEFAULT_API` — a class attribute, not an environment variable, because it is a per-call setting. Set `LLMCallConfig(api='chat_completions')` to opt out. -- **A job that returns a top-level `error` key now marks the row as failed, instead of counting as a clean success.** `process_job` read only `name` and `output` from a job's return value, so `JobResult.error` was set only when the job *raised*. A job that caught its own failure and reported it — which is what the simulation jobs do, because one dead row must not kill the batch — came back with `error=None`, and a run whose conversation never happened printed `Failed Jobs 0`, `Success Rate 100%` and exited 0. Both simulation jobs — `simulate()`'s own and `wrap_simulation_agent`'s — now emit `error` unconditionally (`None` on success, the runner's reason when the run ended in `error` or `timeout`), and `process_job` honours the key while **keeping** the output, so the transcript survives for diagnosis. The job's OTel span is now marked `ERROR` on every failed row, including the two raise paths, which previously closed as `OK`. Presence is the failure signal, not the message's truthiness: a payload that flattens to nothing is reported as `job reported a failure with no readable message` and logged, rather than silently becoming a clean row. `check_pass_failures(results, treat_errors_as_failure=True)` catches these rows as a result — **that flag still defaults to `False`, so a caller who does not pass it gets the same exit code as before**; the change is to the reported counts, not to any exit status. Any other job already returning a top-level `error` key for a non-failure reason will now be counted as failed. +- **A job that returns a top-level `error` key now marks the row as failed, instead of counting as a clean success.** `process_job` read only `name` and `output` from a job's return value, so `JobResult.error` was set only when the job *raised*. A job that caught its own failure and reported it — which is what the simulation jobs do, because one dead row must not kill the batch — came back with `error=None`, and a run whose conversation never happened printed `Failed Jobs 0`, `Success Rate 100%` and exited 0. Both simulation jobs — `simulate()`'s own and `wrap_simulation_agent`'s — now emit `error` unconditionally (`None` on success, the runner's reason when the run ended in `error` or `timeout`), and `process_job` honours the key while **keeping** the output, so the transcript survives for diagnosis — its evaluators are skipped, though, because scoring a conversation already known to be dead buys nothing and costs an LLM judge call per row. `wrap_langchain_agent` now catches a failing `invoke`/`ainvoke` and reports it the same way rather than letting it raise; a caller mistake (no prompt and no usable `messages` column) still raises. The job's OTel span is now marked `ERROR` on every failed row, including the two raise paths, which previously closed as `OK`. Presence is the failure signal, not the message's truthiness: a payload that flattens to nothing is reported as `job reported a failure with no readable message` and logged, rather than silently becoming a clean row. `check_pass_failures(results, treat_errors_as_failure=True)` catches these rows as a result — **that flag still defaults to `False`, so a caller who does not pass it gets the same exit code as before**; the change is to the reported counts, not to any exit status. Any other job already returning a top-level `error` key for a non-failure reason will now be counted as failed. - **`LLMCallConfig.temperature` has no default — unset means the parameter is not sent, and the provider applies its own.** It previously defaulted to `1.0`, and evaluatorq's own call sites layered literals of their own on top (`0.8` for persona and first-message generation, `0.9` for edge-case scenarios, `0.7` for the executive summary and chat-completions agent calls, `0.3` for trace analysis, `0.0` for the judge). Reasoning-class models reject the parameter outright rather than clamping it — `gpt-5.6-luna` answers `400 Unsupported parameter: 'temperature' is not supported with this model` — so a hardcoded temperature on the default model turned every persona x scenario pair into a failure and `simulate()` into a `RuntimeError`. No call site sends a temperature now; a caller who wants one sets `LLMCallConfig(temperature=...)`, and a per-call `temperature=None` means unset rather than an explicit null. `BaseAgent._resolved_temperature` lost its `fallback` argument accordingly and now gates on `model_fields_set` like its sibling resolvers, so an explicit `LLMCallConfig(temperature=None)` opts an agent out rather than deferring to a call site. **The judge is the one behaviour change worth planning around: its scoring calls are no longer pinned to `temperature=0.0`, so judgments are no longer reproducible run-to-run by default.** That affects every `simulate()` caller, not only those on a reasoning model. Pass `JudgeAgentConfig(temperature=0.0)` to restore it — on a model that accepts the parameter. - **A set-but-empty `ORQ_OTEL_*` tuning variable now logs a `WARNING` and falls back to the default, instead of falling back silently.** `_env_int` treated an empty string like an unset variable, so an unresolved workflow variable in a CI `env:` block expands to the empty string and disabled the knob with no signal. Whitespace-only values are treated the same way. Related: `ORQ_OTEL_MAX_BATCH_SIZE` larger than `ORQ_OTEL_MAX_QUEUE_SIZE` is still clamped down to the queue size, but the clamp now announces itself with a `WARNING` rather than happening silently. - **`EVALUATORQ_REASONING_EFFORT` has no default — unset means the parameter is not sent, and the model applies its own.** It previously fell back to `"medium"` for the simulator's own calls (user simulator, judge). A global effort is the wrong default in both directions: on a model that does not accept the parameter it costs a rejected request plus a retry per `(model, tool shape)` — memoised per process, so a short run or CI job never amortises it — and on a model that does, it silently overrides the provider's own tuned value. Set the env var, or `LLMCallConfig.reasoning_effort` on the agent's config, when you actually want a specific effort. **Simulation only**; red teaming's `target_reasoning_effort` was already opt-in. diff --git a/docs/evaluation-reference.md b/docs/evaluation-reference.md index c387139c9..501cc96fe 100644 --- a/docs/evaluation-reference.md +++ b/docs/evaluation-reference.md @@ -95,9 +95,9 @@ async def resilient_job(data: DataPoint, row: int) -> dict: return {"name": "my-agent", "output": answer, "error": None} ``` -Emit `error` on every path, `None` on success: an omitted key and a clean run are indistinguishable, so a job that forgets the key on one branch reports a dead target as a passing one. A row with a non-empty `error` is counted in the summary table's `Failed Jobs`, keeps its output, and is still scored — so it can carry both an error and a passing evaluator score. `check_pass_failures(results, treat_errors_as_failure=True)` is what turns it into a CI failure; the default (`False`) gates on evaluator `pass_` alone. +Emit `error` on every path, `None` on success: an omitted key and a clean run are indistinguishable, so a job that forgets the key on one branch reports a dead target as a passing one. A row with a non-empty `error` is counted in the summary table's `Failed Jobs` and keeps its output for diagnosis, but its evaluators are **skipped** — scoring a transcript you already know is dead buys nothing and costs an LLM judge call per row. `check_pass_failures(results, treat_errors_as_failure=True)` is what turns it into a CI failure; the default (`False`) gates on evaluator `pass_` alone. -This is the raw-dict job contract. `@job()` wraps a function's return value into `{"name", "output"}`, so an `error` key returned from a decorated function lands *inside* `output` and is not read as a row failure — a decorated job reports failures by raising. +This is the raw-dict job contract. `@job()` wraps a function's return value into `{"name", "output"}`, so an `error` key returned from a decorated function lands *inside* `output`, where a judge reads it as the target's failure rather than the runner reading it as the row's. A decorated job reports a row failure by raising. ## Data sources diff --git a/src/evaluatorq/evaluatorq.py b/src/evaluatorq/evaluatorq.py index 58ffa304e..3f47c7153 100644 --- a/src/evaluatorq/evaluatorq.py +++ b/src/evaluatorq/evaluatorq.py @@ -53,10 +53,10 @@ def check_pass_failures(results: EvaluatorqResult, *, treat_errors_as_failure: b (e.g. every judge call raised), also counts as a failure. Without this, an errored job has no evaluator scores and an errored evaluator leaves ``pass_`` unset, so both would be invisible here, letting a run with no usable - responses or no usable scores exit successfully. A job that reported its own - failure through a top-level ``error`` key keeps its output and is still - scored, so such a row can carry both an error and a passing score — this - flag is what makes it count. + responses or no usable scores exit successfully. Neither errored path is scored: + a job that raised has no output to score, and one that reported its own + failure through a top-level ``error`` key keeps its output but skips its + evaluators — so this flag is the only thing that fails such a row. Returns: True if any evaluator failed (pass_=False), False otherwise diff --git a/src/evaluatorq/integrations/langchain_integration/wrap_agent.py b/src/evaluatorq/integrations/langchain_integration/wrap_agent.py index 20c135844..1f16c3790 100644 --- a/src/evaluatorq/integrations/langchain_integration/wrap_agent.py +++ b/src/evaluatorq/integrations/langchain_integration/wrap_agent.py @@ -6,6 +6,7 @@ from typing import TYPE_CHECKING, Any from langchain_core.tools import BaseTool +from loguru import logger from .convert import convert_to_open_responses @@ -131,10 +132,26 @@ async def job(data: DataPoint, _row: int) -> dict[str, Any]: # Invoke the LangChain agent off the event loop; prefer the native # async entry point when the agent exposes one. - if hasattr(agent, 'ainvoke'): - result = await agent.ainvoke({'messages': messages}) - else: - result = await asyncio.to_thread(agent.invoke, {'messages': messages}) + # + # The agent call is the one step here that talks to a live target, so its + # failure is reported through the top-level 'error' key rather than raised: + # a raise is per-row invisible to anything that reads JobResult.error only + # when the job caught nothing. Everything above this — a missing prompt, an + # unusable messages column — is a caller mistake and still raises. + try: + if hasattr(agent, 'ainvoke'): + result = await agent.ainvoke({'messages': messages}) + else: + result = await asyncio.to_thread(agent.invoke, {'messages': messages}) + except Exception as agent_error: # noqa: BLE001 - a target's failure is reported, not raised + logger.warning('langchain agent invocation failed: {}: {}', type(agent_error).__name__, agent_error) + # No transcript to keep — the call never returned one. The row carries the + # reason and no output, which is what an evaluator scoring it will see. + return { + 'name': name, + 'output': None, + 'error': f'{type(agent_error).__name__}: {agent_error}', + } # Extract messages from result result_messages: list[BaseMessage] = result.get('messages', []) @@ -148,6 +165,10 @@ async def job(data: DataPoint, _row: int) -> dict[str, Any]: return { 'name': name, 'output': open_responses_output, + # Emitted unconditionally, None on success: a job that omits the key on a + # good row and sets it on a bad one is indistinguishable from one that + # never reports failures at all. + 'error': None, } return job diff --git a/src/evaluatorq/processings.py b/src/evaluatorq/processings.py index 052815efb..2eefbd2f4 100644 --- a/src/evaluatorq/processings.py +++ b/src/evaluatorq/processings.py @@ -181,9 +181,10 @@ async def process_job( result = await job(data_point, row_index) job_name = cast('str', result['name']) output = cast('Output', result['output']) - # Returned, not raised: without this the row counts as a success. Unlike - # the raise path the output is kept and still scored, so the row can carry - # both an error and evaluator scores. See `JobReturn` for the contract. + # Returned, not raised: without this the row counts as a success. The + # output is kept — unlike the raise path, which has none — but evaluators + # are skipped either way: scoring a transcript we already know is dead + # buys nothing and costs an LLM judge call per row. See `JobReturn`. error = _job_reported_error(result.get('error')) if error is not None: set_span_error(job_span, error) @@ -224,7 +225,15 @@ async def process_job( # Process evaluators if any and job was successful evaluator_scores: list[EvaluatorScore] = [] - if evaluators: + if evaluators and error is not None: + logger.warning( + 'job {!r} reported an error, skipping {} evaluator(s) for row {}: {}', + job_name, + len(evaluators), + row_index, + error, + ) + elif evaluators: # Update phase to evaluating if progress_service: await safe_update_progress( diff --git a/src/evaluatorq/types.py b/src/evaluatorq/types.py index 72d756868..79c43c4e5 100644 --- a/src/evaluatorq/types.py +++ b/src/evaluatorq/types.py @@ -143,8 +143,9 @@ class JobReturn(TypedDict): ``error`` is optional and reports a failure the job *handled* rather than raised — ``None`` on success, the reason otherwise. A row whose ``error`` flattens to a non-empty string is counted in the summary table's ``Failed Jobs`` and fails - ``check_pass_failures(treat_errors_as_failure=True)``; it keeps its output and is - still scored, so it can carry both an error and passing evaluator scores. An + ``check_pass_failures(treat_errors_as_failure=True)``. It keeps its output for + diagnosis but its evaluators are **skipped** — scoring a transcript already known + to be dead buys nothing and costs an LLM judge call per row. An omitted key and an explicit ``None`` are indistinguishable to the consumer — emitting ``None`` on success is a producer convention, so that a job that forgot the key cannot be mistaken for one that reported a clean run. A job that lets its diff --git a/tests/unit/test_processings.py b/tests/unit/test_processings.py index b724278b3..e87146a6a 100644 --- a/tests/unit/test_processings.py +++ b/tests/unit/test_processings.py @@ -157,20 +157,29 @@ def test_summary_table_counts_a_job_reported_error() -> None: @pytest.mark.asyncio -async def test_process_job_scores_a_reported_error_row() -> None: - """A reported-error row keeps its output and is still scored, unlike the raise path.""" +async def test_process_job_skips_evaluators_on_a_reported_error_row() -> None: + """A reported-error row keeps its output but is not scored. + + Scoring a transcript already known to be dead buys nothing and costs an LLM judge + call per row — the raise path has never scored one either. + """ from evaluatorq.processings import process_job from evaluatorq.types import EvaluationResult, Evaluator, ScorerParameter async def reporting_job(_data: DataPoint, _row: int) -> dict[str, Any]: return {'name': 'sim', 'output': 'dead', 'error': 'boom'} + calls: list[str] = [] + async def scorer(_params: ScorerParameter) -> EvaluationResult: # noqa: RUF029 + calls.append('scored') return EvaluationResult(value=1) evaluator: Evaluator = {'name': 'always', 'scorer': scorer} result = await process_job(reporting_job, DataPoint(inputs={'text': 'hi'}), row_index=0, evaluators=[evaluator]) assert result.error == 'boom' - assert result.evaluator_scores is not None - assert len(result.evaluator_scores) == 1 + # The output survives for diagnosis even though nothing scored it. + assert result.output == 'dead' + assert result.evaluator_scores == [] + assert calls == [] diff --git a/tests/unit/test_wrap_agent.py b/tests/unit/test_wrap_agent.py index 7a3dbd488..6d83acc89 100644 --- a/tests/unit/test_wrap_agent.py +++ b/tests/unit/test_wrap_agent.py @@ -315,6 +315,40 @@ async def test_ainvoke_is_awaited_with_expected_payload_and_invoke_is_skipped(se agent.invoke.assert_not_called() +class TestWrapAgentReportsAgentFailures: + @pytest.mark.asyncio + async def test_a_failing_agent_reports_the_error_instead_of_raising(self) -> None: + """A dead target must reach JobResult.error, which only reads a returned key.""" + agent = MagicMock(spec=["invoke", "ainvoke", "nodes"]) + agent.ainvoke = AsyncMock(side_effect=RuntimeError("401 authentication_error")) + agent.nodes = {} + + job = wrap_langchain_agent(agent, name="t") + result = await job(DataPoint(inputs={"prompt": "hi"}), 0) + + assert result["error"] == "RuntimeError: 401 authentication_error" + # The call never returned a transcript, so there is nothing to keep. + assert result["output"] is None + + @pytest.mark.asyncio + async def test_a_bad_datapoint_still_raises(self) -> None: + """A caller mistake is not a target failure and must not be swallowed.""" + agent = _make_agent() + job = wrap_langchain_agent(agent, name="t") + + with pytest.raises(ValueError, match="neither was provided"): + await job(DataPoint(inputs={}), 0) + + @pytest.mark.asyncio + async def test_a_good_row_emits_the_key_as_none(self) -> None: + """Emitted, not omitted: a clean row and a job that never reports must differ.""" + job = wrap_langchain_agent(_make_agent(), name="t") + result = await job(DataPoint(inputs={"prompt": "hi"}), 0) + + assert "error" in result + assert result["error"] is None + + class TestWrapLangGraphAgentAlias: def test_alias_is_same_function(self) -> None: assert wrap_langgraph_agent is wrap_langchain_agent From 39f014f423f606325ff4f2bb3c6d6049918e6f46 Mon Sep 17 00:00:00 2001 From: Bauke Brenninkmeijer Date: Tue, 1 Sep 2026 13:26:04 +0200 Subject: [PATCH 4/4] fix: exit_on_failure counts a run that ended in error or timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check counted rows missing from the result cache. A run the runner ended in error is *in* the cache — it produced a result, just not a conversation — so simulate() and eq sim returned normally over a target that was 401 throughout, which is exactly what the caller asked to exit on. The message now names both counts. exit_on_failure=False is unchanged: the same rows are reported as a warning and the partial results come back. Scorer verdicts are still reporting only, so an agent that answered badly does not raise. Two tests whose subject is something else (the terminal hook, the per-simulation wall clock) opt out explicitly rather than relying on a dead row exiting 0. CLAUDE.md gains the distinction this branch's review got wrong: the error key it legislates is the one inside a target or LLM invocation's output payload, which decides what a judge reads. JobReturn['error'] is a different key at the top level of a job's return value, deciding whether the row counts as failed. @job() nesting everything under output is the contract, not a gap. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + CLAUDE.md | 2 + docs/guides/agent-simulation.md | 4 +- docs/guides/simulation-in-evaluatorq.md | 2 +- src/evaluatorq/simulation/api.py | 38 +++++++++----- tests/simulation/test_hooks.py | 51 +++++++++++++++++++ .../simulation/test_per_simulation_timeout.py | 3 ++ 7 files changed, 85 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b13273614..f05b984f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ All notable changes to `evaluatorq` are documented here. - **`JudgeAgent` pins itself to the Responses API and logs when a config says otherwise.** `llm_config.api` is one knob for both simulation agents, and only one of them can honour every value: the judge sends function tools and `reasoning_effort` in one request, which chat completions answers with a 400 on models like `gpt-5.4-mini`, while the user simulator's plain completion works on either endpoint. Same model, two roles, two viable protocols. `LLMCallConfig(api='chat_completions')` now applies to the user simulator and is overridden on the judge with a `WARNING` naming it, where it previously reached the judge and broke the run. The pin is `JudgeAgent.REQUIRED_API`; the general default is still `BaseAgent.DEFAULT_API`. - **Every simulation agent defaults to the Responses API when its `LLMCallConfig` leaves `api` unset**, overriding that class's own `chat_completions` default. `JudgeAgentConfig` and `UserSimulatorAgentConfig` supplied this before; a caller handing an agent a bare `LLMCallConfig` used to get chat completions instead, which returns a 400 on models like `gpt-5.4-mini` the moment the judge sends function tools and `reasoning_effort` together. The default is `BaseAgent.DEFAULT_API` — a class attribute, not an environment variable, because it is a per-call setting. Set `LLMCallConfig(api='chat_completions')` to opt out. - **A job that returns a top-level `error` key now marks the row as failed, instead of counting as a clean success.** `process_job` read only `name` and `output` from a job's return value, so `JobResult.error` was set only when the job *raised*. A job that caught its own failure and reported it — which is what the simulation jobs do, because one dead row must not kill the batch — came back with `error=None`, and a run whose conversation never happened printed `Failed Jobs 0`, `Success Rate 100%` and exited 0. Both simulation jobs — `simulate()`'s own and `wrap_simulation_agent`'s — now emit `error` unconditionally (`None` on success, the runner's reason when the run ended in `error` or `timeout`), and `process_job` honours the key while **keeping** the output, so the transcript survives for diagnosis — its evaluators are skipped, though, because scoring a conversation already known to be dead buys nothing and costs an LLM judge call per row. `wrap_langchain_agent` now catches a failing `invoke`/`ainvoke` and reports it the same way rather than letting it raise; a caller mistake (no prompt and no usable `messages` column) still raises. The job's OTel span is now marked `ERROR` on every failed row, including the two raise paths, which previously closed as `OK`. Presence is the failure signal, not the message's truthiness: a payload that flattens to nothing is reported as `job reported a failure with no readable message` and logged, rather than silently becoming a clean row. `check_pass_failures(results, treat_errors_as_failure=True)` catches these rows as a result — **that flag still defaults to `False`, so a caller who does not pass it gets the same exit code as before**; the change is to the reported counts, not to any exit status. Any other job already returning a top-level `error` key for a non-failure reason will now be counted as failed. +- **`simulate(exit_on_failure=True)` now raises when a run ended in `error` or `timeout`, not only when a row was dropped.** The check counted rows missing from the result cache, and a run the runner ended in `error` *is* in the cache — so `eq sim` and `simulate()` returned normally over a target that was 401 throughout, which is what the caller asked to exit on. `SimulationDroppedError`'s message now names both counts. `exit_on_failure=False` is unchanged: the same rows are reported as a `WARNING`. Scorer verdicts are still reporting only — an agent that answered badly does not raise. - **`LLMCallConfig.temperature` has no default — unset means the parameter is not sent, and the provider applies its own.** It previously defaulted to `1.0`, and evaluatorq's own call sites layered literals of their own on top (`0.8` for persona and first-message generation, `0.9` for edge-case scenarios, `0.7` for the executive summary and chat-completions agent calls, `0.3` for trace analysis, `0.0` for the judge). Reasoning-class models reject the parameter outright rather than clamping it — `gpt-5.6-luna` answers `400 Unsupported parameter: 'temperature' is not supported with this model` — so a hardcoded temperature on the default model turned every persona x scenario pair into a failure and `simulate()` into a `RuntimeError`. No call site sends a temperature now; a caller who wants one sets `LLMCallConfig(temperature=...)`, and a per-call `temperature=None` means unset rather than an explicit null. `BaseAgent._resolved_temperature` lost its `fallback` argument accordingly and now gates on `model_fields_set` like its sibling resolvers, so an explicit `LLMCallConfig(temperature=None)` opts an agent out rather than deferring to a call site. **The judge is the one behaviour change worth planning around: its scoring calls are no longer pinned to `temperature=0.0`, so judgments are no longer reproducible run-to-run by default.** That affects every `simulate()` caller, not only those on a reasoning model. Pass `JudgeAgentConfig(temperature=0.0)` to restore it — on a model that accepts the parameter. - **A set-but-empty `ORQ_OTEL_*` tuning variable now logs a `WARNING` and falls back to the default, instead of falling back silently.** `_env_int` treated an empty string like an unset variable, so an unresolved workflow variable in a CI `env:` block expands to the empty string and disabled the knob with no signal. Whitespace-only values are treated the same way. Related: `ORQ_OTEL_MAX_BATCH_SIZE` larger than `ORQ_OTEL_MAX_QUEUE_SIZE` is still clamped down to the queue size, but the clamp now announces itself with a `WARNING` rather than happening silently. - **`EVALUATORQ_REASONING_EFFORT` has no default — unset means the parameter is not sent, and the model applies its own.** It previously fell back to `"medium"` for the simulator's own calls (user simulator, judge). A global effort is the wrong default in both directions: on a model that does not accept the parameter it costs a rejected request plus a retry per `(model, tool shape)` — memoised per process, so a short run or CI job never amortises it — and on a model that does, it silently overrides the provider's own tuned value. Set the env var, or `LLMCallConfig.reasoning_effort` on the agent's config, when you actually want a specific effort. **Simulation only**; red teaming's `target_reasoning_effort` was already opt-in. diff --git a/CLAUDE.md b/CLAUDE.md index f0f6a6a70..eff4046e3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -190,6 +190,8 @@ Do not add a directory tree, a file inventory, or anything else the filesystem a The reverse failure is quieter and worse: a job that returns **no** `error` key at all makes `output_error_text` return `None`, so the judge scores the literal `[ERROR: ...]` marker as a genuine agent reply and a dead target comes back RESISTANT. Both static legs now emit the key unconditionally (`None` on success). A new job that calls a target must do the same. +**Two different `error` keys, one word.** The one above is *inside the output payload* of a target or LLM invocation, and it decides what a judge reads. `JobReturn['error']` is at the **top level of a job's return value**, and it decides whether the row counts as failed in `Failed Jobs` and under `check_pass_failures(treat_errors_as_failure=True)`. An invocation lives inside a job, so a job can carry both, one, or neither. The rule above is about the nested one; a job that swallows a failure to keep the batch alive — `simulate()`'s and `wrap_simulation_agent()`'s do — also emits the top-level one, `None` on success. `@job()` nests everything it is handed under `output`, so a decorated job reports failures by raising; that is the contract, not a gap. + ### Adding New Features - New vulnerabilities: see `docs/custom-evaluators-and-frameworks.md` diff --git a/docs/guides/agent-simulation.md b/docs/guides/agent-simulation.md index 6ed49bddf..75bf31824 100644 --- a/docs/guides/agent-simulation.md +++ b/docs/guides/agent-simulation.md @@ -145,9 +145,9 @@ The fastest start: `generate_and_simulate()` synthesizes the personas, scenarios `agent_description` drives generation; `num_personas × num_scenarios` is how many conversations run. The simulation-side LLMs resolve their provider by precedence: an explicitly passed `generation_client` wins, then `llm_config.client`, then `ORQ_API_KEY` (the Orq AI Router), then `OPENAI_API_KEY`. See [Configuration](../configuration.md). !!! note "CI and local runs" - Dropped simulations raise by default; ordinary failed goals remain in the returned results. Set `exit_on_failure=False` for exploratory runs. When `ORQ_API_KEY` is available, results upload to Orq by default; pass `upload_results=False` to suppress the Experiment upload. That is not an offline mode — see [What gets uploaded](simulation-in-evaluatorq.md#what-gets-uploaded). + A simulation that produced no conversation — dropped, or ended in `error`/`timeout` — raises by default; ordinary failed goals remain in the returned results. Set `exit_on_failure=False` for exploratory runs. When `ORQ_API_KEY` is available, results upload to Orq by default; pass `upload_results=False` to suppress the Experiment upload. That is not an offline mode — see [What gets uploaded](simulation-in-evaluatorq.md#what-gets-uploaded). - `exit_on_failure` gates on dropped datapoints, not on scores, so it will not fail a build for an agent that simply answered badly. For a gate on the scores themselves — turning evaluator results into an exit code, with the env vars and workflow step to go with it — see [In an evaluatorq Run › In CI](simulation-in-evaluatorq.md#in-ci). + `exit_on_failure` gates on datapoints that never produced a conversation, not on scores, so it will not fail a build for an agent that simply answered badly. For a gate on the scores themselves — turning evaluator results into an exit code, with the env vars and workflow step to go with it — see [In an evaluatorq Run › In CI](simulation-in-evaluatorq.md#in-ci). ## Seed by archetype diff --git a/docs/guides/simulation-in-evaluatorq.md b/docs/guides/simulation-in-evaluatorq.md index 606adbb7c..fc64d23d5 100644 --- a/docs/guides/simulation-in-evaluatorq.md +++ b/docs/guides/simulation-in-evaluatorq.md @@ -255,7 +255,7 @@ Three things change for a non-interactive run: - `EVALUATORQ_DIR` pointed at a scratch directory, so the run store does not persist between jobs and one workflow cannot resolve another's runs. - `ORQ_DISABLE_TRACING=1` if you do not want CI spans in your traces. -If you are arriving from `simulate()`, note what does and does not carry over. `simulate()` takes `exit_on_failure`, which defaults to `True` and raises `SimulationDroppedError` when a datapoint is *dropped* — a job raised and no result was cached. That is an infrastructure gate, not a quality gate: scorer verdicts are reporting only there too, so an agent that answered every question badly still exits 0. Moving to `wrap_simulation_agent()` costs you that infrastructure gate, because the parameter belongs to `simulate()` and there is no equivalent on `evaluatorq()`. +If you are arriving from `simulate()`, note what does and does not carry over. `simulate()` takes `exit_on_failure`, which defaults to `True` and raises `SimulationDroppedError` when a datapoint produced no conversation — *dropped* (a job raised and no result was cached) or ended in `error`/`timeout`. That is an infrastructure gate, not a quality gate: scorer verdicts are reporting only there too, so an agent that answered every question badly still exits 0. Moving to `wrap_simulation_agent()` costs you that infrastructure gate, because the parameter belongs to `simulate()` and there is no equivalent on `evaluatorq()` — gate on `job_result.error` yourself, as the example above does. `evaluatorq()` does not fail the process for you either — it returns results and exits 0 whatever the scores are. So on this path the build verdict is yours to write, and it is one line: diff --git a/src/evaluatorq/simulation/api.py b/src/evaluatorq/simulation/api.py index e2bf2229e..de7b0b4d7 100644 --- a/src/evaluatorq/simulation/api.py +++ b/src/evaluatorq/simulation/api.py @@ -350,14 +350,14 @@ async def simulate( (``upload_results=True``). Leave ``None`` for local-only runs. orq_results_path: Optional Orq folder path (e.g. ``"MyProject/MyFolder"``). exit_on_failure: When ``True`` (the default), exit non-zero if any - datapoint was *dropped* — a job raised with no result cached — - by raising ``SimulationDroppedError`` from ``simulate()`` itself - (the "CI gating for free" benefit). Scorer verdicts (``pass_``, - e.g. goal not achieved) are reporting only and never exit the - process: an underperforming but otherwise healthy run still - returns its results. Pass ``False`` for interactive / exploratory - runs where even dropped rows should surface as warnings + error - metadata instead. + datapoint failed to produce a conversation — *dropped* (a job raised + with no result cached) or ended in ``error``/``timeout`` — by raising + ``SimulationDroppedError`` from ``simulate()`` itself (the "CI gating + for free" benefit). Scorer verdicts (``pass_``, e.g. goal not + achieved) are reporting only and never exit the process: an + underperforming but otherwise healthy run still returns its results. + Pass ``False`` for interactive / exploratory runs where a dead row + should surface as a warning + error metadata instead. save: When ``True``, persist the completed run to the local run store (``.evaluatorq/sim-runs/`` unless ``report`` is set). Unlike the CLI (which auto-saves to ``.evaluatorq/sim-runs/`` by default), the SDK @@ -2320,6 +2320,7 @@ async def _simulate_via_evaluatorq( from evaluatorq.common.tracing import set_span_attrs from evaluatorq.evaluatorq import evaluatorq from evaluatorq.simulation.evaluators import get_evaluator + from evaluatorq.simulation.evaluators.scorers import failure_reason from evaluatorq.types import DataPoint evaluation_name = config.evaluation_name @@ -2427,11 +2428,22 @@ async def _simulate_via_evaluatorq( expected = len(eq_datapoints) missing = [i for i, eq in enumerate(eq_datapoints) if id(eq) not in result_cache] - if missing: - msg = ( - f'{caller}(): {len(missing)} of {expected} simulation job(s) failed ' - f'and produced no result (missing rows: {missing})' - ) + # A run the runner ended in error or timeout produced a result, so it is not + # 'missing' — but its conversation never happened, which is the same failure to a + # caller that asked to exit on one. Without this a run that was 401 throughout + # returned normally, because every row was cached. + dead = [ + i + for i, eq in enumerate(eq_datapoints) + if id(eq) in result_cache and failure_reason(result_cache[id(eq)]) is not None + ] + if missing or dead: + parts: list[str] = [] + if missing: + parts.append(f'{len(missing)} produced no result (rows: {missing})') + if dead: + parts.append(f'{len(dead)} ended in error or timeout (rows: {dead})') + msg = f'{caller}(): {len(missing) + len(dead)} of {expected} simulation(s) failed — ' + '; '.join(parts) if exit_on_failure: raise SimulationDroppedError(msg, partial_results=results) logger.warning(msg) diff --git a/tests/simulation/test_hooks.py b/tests/simulation/test_hooks.py index f34d4262f..e4080088d 100644 --- a/tests/simulation/test_hooks.py +++ b/tests/simulation/test_hooks.py @@ -653,6 +653,9 @@ async def boom(messages): max_turns=1, evaluator_names=['goal_achieved'], hooks=hooks, + # This test is about the terminal hook firing, not the exit gate: a dead + # target now raises under the default exit_on_failure=True. + exit_on_failure=False, ) assert hooks.started is True assert hooks.completed_with is not None # terminal fired @@ -731,6 +734,54 @@ async def fake_run(self, *, datapoint, max_turns, thread_id=None): assert hooks.completed_with[0].goal_achieved is True +@pytest.mark.asyncio +async def test_exit_on_failure_raises_for_a_run_that_ended_in_error(datapoint_factory, monkeypatch): + """A row the runner ended in error is cached, so it was never 'dropped' — and a + run that was 401 throughout returned normally, which is what the caller asked to + exit on.""" + from evaluatorq.simulation.api import SimulationDroppedError, simulate + from evaluatorq.simulation.runner.simulation import SimulationRunner, _error_result + + async def fake_run(self, *, datapoint, max_turns, thread_id=None): + return _error_result('401 authentication_error') + + monkeypatch.setattr(SimulationRunner, 'run', fake_run) + + with pytest.raises(SimulationDroppedError, match='ended in error or timeout'): + await simulate( + datapoints=[datapoint_factory('dp-dead')], + target=_ok_target, + user_simulator=_StubUserSim(), # pyright: ignore[reportArgumentType] + judge=_StubJudge(terminate=True), # pyright: ignore[reportArgumentType] + max_turns=1, + evaluator_names=['goal_achieved'], + ) + + +@pytest.mark.asyncio +async def test_exit_on_failure_false_warns_instead_of_raising_for_an_error_run(datapoint_factory, monkeypatch): + """The opt-out still returns the partial results, with the row reported.""" + from evaluatorq.simulation.api import simulate + from evaluatorq.simulation.runner.simulation import SimulationRunner, _error_result + + async def fake_run(self, *, datapoint, max_turns, thread_id=None): + return _error_result('401 authentication_error') + + monkeypatch.setattr(SimulationRunner, 'run', fake_run) + + results = await simulate( + datapoints=[datapoint_factory('dp-dead')], + target=_ok_target, + user_simulator=_StubUserSim(), # pyright: ignore[reportArgumentType] + judge=_StubJudge(terminate=True), # pyright: ignore[reportArgumentType] + max_turns=1, + evaluator_names=['goal_achieved'], + exit_on_failure=False, + ) + assert len(results) == 1 + assert results[0].terminated_by is TerminatedBy.error + + @pytest.mark.asyncio async def test_simulation_dropped_error_is_simulation_error(): """SimulationDroppedError must be catchable as SimulationError and carry the diff --git a/tests/simulation/test_per_simulation_timeout.py b/tests/simulation/test_per_simulation_timeout.py index 31be8697e..014552839 100644 --- a/tests/simulation/test_per_simulation_timeout.py +++ b/tests/simulation/test_per_simulation_timeout.py @@ -120,6 +120,9 @@ async def test_per_simulation_timeout_s_terminates_a_stalled_conversation(monkey judge=_StubJudge(), # pyright: ignore[reportArgumentType] upload_results=False, executive_summary=False, + # The subject here is the wall clock, not the exit gate: a timed-out row + # raises under the default exit_on_failure=True. + exit_on_failure=False, ) assert len(results) == 1 assert results[0].terminated_by == TerminatedBy.timeout