[bot] Merge master/45892f79 into rel/dev - #1774
Merged
Merged
Conversation
Finding a gen-ai trace means polling until Langfuse has ingested it -- anywhere
from one round trip to the full retry budget. None of that work produces a
verdict; the pass/fail is already decided by the time it starts. Charging the
item's latency for it both slowed the run and made the reported agent latency
wrong.
Each evaluate_agentic_* now hands its Langfuse block to a linker instead of
running it inline. The CLI injects a BackgroundTraceLinker that collects the
queue and drains it after the agent phase, before any report is rendered, so
scores are always final before the command exits. Direct library callers keep
the synchronous default and are behaviour-compatible.
Alongside that:
- Per-phase latency (agent / judge / simulated user / Langfuse) recorded per
run and aggregated across K runs, exposed as an additive latency_breakdown_s.
- --concurrency reaches the agentic path, partitioned by an explicit
PARALLEL_SAFE_TEST_KINDS allowlist; anything absent runs serially.
- --timers gates the per-turn [timer] output (off by default). --judge-model
selects the LLM-as-judge model (default gpt-4o).
- An unreadable judge response raises JudgeResponseError instead of scoring 0.
- An item's user_context (a WIDGET/VIEW attachment the question refers to) is
relayed to the chat request as userContext, and survives the Langfuse dataset
round trip.
FIXES FOUND WHILE REVIEWING THE ABOVE
Each was reproduced broken first, then re-verified by re-injecting the defect
and confirming the new test fails.
Judge contract:
- A judge fault on one run of K discarded every run already graded, dropped
their Langfuse scores, and reported an item whose pass@K was ALREADY
satisfied as a failure -- the same "a parse bug reads as a pass-rate drop"
that JudgeResponseError exists to prevent, one layer up. A fault is now
confined to its own run; pass@K holds on the graded runs, pass^K requires
every run graded, and an item with no graded run at all errors.
dashboard_summary needed it most: it judges once PER CRITERION, so one bad
body lost all of them.
- choices == [] (content filters, gateway error envelopes) escaped as a bare
IndexError, past the typed error, with none of the body or metadata.
- {"score": 2} was reported as a confident FAIL -- int(score) == 1 was the last
place an invented 0 survived. Only 0 and 1 are verdicts now.
- {"score": "1"} regressed; JSON mode quotes numbers routinely, so it is
coerced again.
- The temperature fallback matched "temperature" in str(exc), and the openai
SDK stringifies the whole response body into the message. Gateways echo the
request inside it, so any 400 -- context_length_exceeded included -- was
misread, silently dropping temperature=0 from every later verdict. Now read
off the provider's structured error.
- openai>=1.45 is required: 1.40-1.44 lack max_completion_tokens (verified
against the wheel) and would TypeError on every judge call.
Interrupts:
- A bare `with ThreadPoolExecutor(...)` exits via shutdown(wait=True) with
cancel_futures left False, so Ctrl-C ran every QUEUED item to completion
first. Reproduced: SIGINT at 0.30s, interrupt observed at 9.00s, all 6 items
run. Both pools now cancel; drain() had the same defect and sits outside the
abandon() guard, so it cancels on the pool itself.
Langfuse:
- The sessionId filter never reached the server: _TraceAPI.list had no such
parameter and it is the only client make_langfuse_client returns. So every
attempt downloaded a full limit=100 page of the whole window and filtered it
locally -- and the endpoint returns newest-first, so once a window held more
than 100 traces the item's OWN trace was evicted and it spent its entire
budget on a page that could never contain it.
- The 120s budget is affordable only because the batch blocks nobody. Direct
library callers poll inline, on their own critical path (the tavern e2e suite
under a step timeout), where 120s tripled a miss from ~35s to ~110s. The
budget now follows the mode: 35s inline (identical to the old ladder, to the
second), 120s batched.
- A local --dataset cannot be attached to a Langfuse run, but linking still
happens off exported credentials, so every conversation earned a raw 404 from
dataset-run-items at the very end of the run. Now warned before the run
starts and reported once per run with its cause.
Reporting:
- pass@K answers "did any run pass", so a 5/5 item and a 1/5 item were
identical in every output -- quality_score reads the best run alone. Every
agentic kind already computed the count and dropped it. Now surfaced as
runs_passed / pass_power_k per item and passed_all_runs per run, with "4/5
runs passed" in the console.
- agentic_conversation takes no k and drives its fixture once, but runs = k was
set unconditionally, so --runs 5 claimed five runs and divided one
conversation's latency by five.
- An errored item lost the phase timings it had managed to take.
Tests that certified nothing:
- The window-pinning guard counted call arity, so inlining _dt.now() inside
_link_traces -- the exact drift its docstring describes -- passed.
- abandon() had no effective coverage: the only test reaching it never calls
drain(), so replacing its body with `pass` left 69 tests green.
- test_resolve_connection_uses_profile read an exported GOODDATA_TOKEN instead
of the profile it stubs, and one test popped TAVERN_E2E_SKIP_TRACE_LINK with
no guard, unsetting it for every module collected afterwards.
Docs:
- Audited every factual claim in the README. The retry budget is no longer one
number; runs_passed / pass_power_k / passed_all_runs were undocumented; the
local-dataset linking behaviour was unexplained; and the experiment run name
was wrong (and had been on master): the code builds
{dataset_name}_{timestamp}_{model} with _effort-{level} and _run{N} suffixes.
- --concurrency's --help omitted agentic_kda_skill from the forced-serial list.
SIMPLIFICATION PASS
types-check was failing on this branch, and the local check disagreed because
CI runs `uv run ty` (the locked version), not `uvx ty` (latest). Two real
diagnostics in langfuse_source._infer_test_kind: isinstance(metadata, dict)
does not narrow the following subscript past object. Binding the value before
the isinstance check fixes it and drops a double lookup. A first attempt also
proved that py314 defers annotation evaluation (PEP 649), so a missing TypeVar
passed the suite here and would have NameError'd at import on py310-313.
The eight evaluate_agentic_* functions each carried a byte-identical ~36-line
Langfuse prologue, so every change above had to be made eight times -- and two
AST-walking tests existed only to stop the eight copies from drifting. That
block is now one helper: RunIdentity / RunTraceContext / submit_trace_scoring.
`def _link_traces` 8 -> 1, `build_run_context(` 9 -> 2, `suffix_needed` 12 ->
0. The structural tests were retargeted at the invariant's new home rather than
deleted, and conversation.py resolves its dataset name eagerly so a queued task
no longer retains the whole ConversationFixture until drain.
Also: emit_line replaces six hand-rolled stdout.write+flush pairs; a shared
_first_of collapses three input-then-metadata lookup ladders; runs_total
replaces an expression duplicated between ItemReport and the console renderer;
PhaseTimings.as_dict() is wired into the JSON report, which was dead code, as
was the langfuse_s field it now populates; the unused context-manager protocol
and the dead total_s are gone; _response_metadata's five copy-pasted try/except
blocks collapse to one guarded helper; summary._grade no longer writes
detail[key] for its caller to overwrite. Comment bloat trimmed where one idea
was stated four to eight times over.
The three slowest tests were time.sleep(0.3) negative assertions -- slow, and
timing-flaky on a loaded box. They now join the pool for real, which is what
would actually let a queued task start.
Behaviour preservation was checked rather than assumed: all 33 score-writing
calls were compared against the pre-refactor tree (identical modulo the
mechanical renames), as were the conversation-id expressions, the run-name
suffix policy and the dataset names. The three retargeted structural tests and
the three interrupt tests were each mutation-tested by re-breaking the code
they guard and confirming the right test fails.
SECOND SIMPLIFICATION PASS
The first pass traded duplicated logic for duplicated argument plumbing of
about the same size, so it removed only ~74 net lines. This pass went after the
plumbing:
- The eight *AssertionError classes each redeclared the same seven-attribute
payload (and two of the eight declared `timings` while the runner getattr'd
it from all eight). They now share an AgenticAssertionError base in
core/models.py.
- The eight-line preamble (datetime aliases, client fallback, window_start)
that opened every evaluate_agentic_* is one call to open_trace_window().
- RunTraceContext gained observe()/score()/quality(), so the deferred _langfuse
import and the five-argument observe() call disappear from all eight kinds.
- Seven of eight kinds built their `detail` dict twice -- once on the failure
path, once on success -- with nothing keeping the two literals in step.
Hoisted to one local per kind, proven equivalent by AST comparison on both
paths.
Across the eight agentic kinds: `__tracebackhide__` 8 -> 0, deferred `_langfuse
import (` 8 -> 0, `from datetime import` 17 -> 2, `try_make_langfuse_client` 18
-> 4, `suffix_needed` 12 -> 0, `def _link_traces` 8 -> 1.
Tests: the six recurring patch-block shapes are now file-local context
managers, 41 copies of an inline MagicMock scaffold are gone, and the three
slowest tests (0.3s sleeps used as negative assertions, flaky on a loaded box)
now join the pool for real. Suite wall time 4.1s -> 2.6s.
REVIEW FINDINGS FIXED
Eight CodeRabbit findings, each reproduced before it was touched, each now
covered by a regression test that fails when the fix is reverted:
- An item that errored after earlier runs passed reached runs_passed ==
runs_total and was reported as pass^K -- unanimity claimed for an item whose
last run had no verdict at all.
- The JSON report counted an errored item as both `failed` and `errored`,
because `failed` was computed by subtraction.
- A blank `test_kind` ("") beat both structural inference and the CLI --kind
default, so the item was skipped as an unsupported kind.
- Avg/run divided by the requested K while the Runs column showed
runs_effective, so agentic_conversation reported a per-run latency for four
runs that never happened. Fixed on ItemReport.avg_latency_s so the JSON
report gets it too.
- A CLI test let _apply_judge_model write GD_EVAL_JUDGE_MODEL straight into
os.environ without monkeypatch recording it, leaking the judge model into
every later test.
- Ctrl-C during the batched drain could hang for the rest of the batch budget.
cancel_futures only drops what has not started; a poll already running sits
in find_traces_per_conversation's backoff, and the interpreter joins executor
workers at exit. Verified in the real CLI against staging, with Langfuse
pointed at a closed port: Ctrl-C during the drain exited after 105.4s before
this change and 1.1s after. The linker now publishes a cancellation Event for
the duration of one drain -- fresh per drain, so one run's interrupt cannot
stop the next --model pass -- and the backoff is served in slices that check
it. A set event is left in place when the drain unwinds, because
shutdown(wait=False) returns before the workers notice and clearing it would
send a late worker back to an uninterruptible sleep.
- Fixing the blank test_kind above introduced a second bug: _first_of returned
the first *string* it found, so a blank expectedOutput.test_kind shadowed a
valid metadata.test_kind. The sources are now checked one at a time.
- The README claimed both that a direct library caller gets langfuse_s = 0.0 and
that langfuse_s is populated whenever credentials are exported. Linking does
happen either way; only the CLI path measures its duration.
LIVE VALIDATION
Measured on staging (18 agentic_general_question items x 2 runs, gpt-5.6-luna):
--concurrency 1, master 462s vs 233s here; --concurrency 2, master 774s vs
114s. --concurrency has no effect on master's agentic path, so both master runs
did the same serial work and differed only by ingestion lag -- and one of them
orphaned two traces. Reported latency per run falls 12.7s -> 5.2s, which is not
a speedup but the removal of Langfuse polling from a number that is supposed to
measure the agent.
Pass rates unchanged: 18/18 on both trees, plus 33/33 on the 33-item guardrail
dataset. pass^K differed there (32 vs 29); re-running the three differing items
three times per tree put master at 2/3, 1/3, 2/3 and this branch at 2/3, 2/3,
2/3, so that gap is agent non-determinism on borderline refusal prompts, not a
regression.
REVIEW FEEDBACK
Rationale for the change itself has been taken back out of the source comments and
left here, where it belongs: the openai>=1.45 justification above stood duplicated
above the pin in pyproject.toml, the pass@K/pass^K asymmetry ran to a twelve-line
essay arguing its own history, and the same "keeping two literals in step was a
standing drift hazard" paragraph had been pasted into eight modules. Comments that
state a live constraint a future editor must not break -- the two ThreadPoolExecutor
shutdown notes, the PARALLEL_SAFE_TEST_KINDS invariant, the `is not None` session
filter -- are kept. Comment lines on this diff's source additions: 337 -> 301.
A ticket key that had been left in a test docstring is gone with them; the
GDAI-2179 literals that remain are Langfuse dataset names, which is what a real
one is called.
Verification: 685 pass, 0 failures -- also per test file standalone, in reverse
module order, and with GOODDATA_TOKEN, TAVERN_E2E_SKIP_TRACE_LINK,
GD_EVAL_TIMERS and GD_EVAL_JUDGE_MODEL exported. ruff check, ruff format
--check and `uv run ty` clean. Behaviour preservation was proven, not assumed:
all 41 score/observe calls and every detail dict compared against the
pre-refactor tree, 1126 assertion statements compared across 35 test files, and
the six safety-critical tests mutation-tested by re-breaking the code they
guard.
risk: medium
…nking Judge faults reach every kind the same way. general_question and guardrail called judge.score directly, so an unreadable body raised out of the runner's K loop: the item errored while pass_at_k stayed True, and the remaining runs never ran. Both now go through score_run and return an ungraded run; the runner counts those in ItemReport.runs_ungraded, keeps evaluating, and errors the item only when no run was graded. dashboard_summary returns the same shape instead of raising, so one all-ungraded run out of K no longer errors the item. dashboard_summary could report PASS with zero gating criteria graded: the no-verdict guard accepted any graded bool, rubric bools included, and an ungraded gating criterion was a no-op for `passed`. An ungraded mandatory criterion now disqualifies the pass, and the guard keys on gating criteria. The local sessionId filter in _fetch_traces_for_session only ran when the client lacked the parameter, which the httpx client now declares, so a server that ignores the parameter returned the whole window and the max-latency pick could attach scores to a foreign trace. It is an unconditional post-check now. The drain cancellation Event was a module global left set after an interrupt, so a later inline link read it and broke before its first fetch, and the test suite's order decided whether link_cancel_event() was None. It is a ContextVar set per task on the worker thread: late workers still see it, nothing else can. _rejects_temperature stringified a non-dict body, which for the openai SDK is the raw response text a gateway echoes the request into. A non-dict body is not read at all. JSON `runs` and EvalReport.total_runs use runs_total, so agentic_conversation no longer reports the requested K or divides one run's latency by it. ChatClient.ask forwards item.user_context, which the single-turn path dropped. risk: medium -- pass/fail semantics change for dashboard_summary items with an ungraded gating criterion (PASS becomes FAIL) and for general_question / guardrail items with a judge fault on one run (ERROR becomes PASS/FAIL over the graded runs). Covered by 14 new tests, each verified red before the fix.
perf: take Langfuse trace linking off the eval item critical path
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## rel/dev #1774 +/- ##
===========================================
+ Coverage 80.82% 81.58% +0.75%
===========================================
Files 272 275 +3
Lines 19414 19863 +449
===========================================
+ Hits 15692 16205 +513
+ Misses 3722 3658 -64 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚀 Automated PR to perform merge from master into rel/dev with changes up to 45892f7 (created by https://github.com/gooddata/gooddata-python-sdk/actions/runs/33765025522).