fix(batch_evaluation): route batch_evaluation through v2 observations API - #1898
passionworkeer wants to merge 4 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Resolve the metadata omission and cross-page trace collapsing issue; correct the inaccurate field-group comment.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Migrates batch_evaluation to the v2 observations API for Langfuse events_only deployments.
Changes:
- Adds cursor-based v2 observation fetching and field mapping.
- Collapses observations into trace representatives.
- Adds unit coverage for fetching, grouping, pagination, and field selection.
| File | Summary | Findings |
|---|---|---|
tests/unit/test_batch_evaluation_fetch.py |
Tests v2 fetching, grouping, filtering, pagination, and field mapping. | No findings. |
langfuse/batch_evaluation.py |
Implements v2 observation fetching and trace collapsing. | Critical: default fields omit metadata. Moderate: trace collapsing is page-local and can duplicate traces. Nit: comment incorrectly describes supported field groups. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # The legacy ``/api/public/traces`` ``io`` / ``scores`` / ``observations`` / | ||
| # ``metrics`` field groups are not selectable on the v2 read endpoint because | ||
| # it returns one observation at a time. | ||
| _DEFAULT_V2_OBSERVATION_FIELDS = "core,basic,io,usage,model,trace_context" |
There was a problem hiding this comment.
Fixed in 6021c60 — metadata added to the default v2 field groups.
| def _collapse_observations_to_traces( | ||
| observations: List[ObservationV2], | ||
| ) -> List[ObservationV2]: | ||
| """Collapse a flat list of observations to one observation per trace_id. | ||
|
|
||
| The v2 observations endpoint has no trace-level read; this helper takes | ||
| whatever observations the page returned and returns one representative | ||
| per trace, preferring the observation that the server already marked as | ||
| the root (``is_root_observation=True``) and falling back to the first | ||
| observation seen for that trace. |
There was a problem hiding this comment.
Fixed in 6021c60 — the runner now tracks seen trace IDs across cursor pages.
… API BatchEvaluationRunner._fetch_batch_with_retry used the v3 read APIs (GET /api/public/traces and GET /api/public/observations via legacy.observations_v1) to fetch items for batch evaluation. Both endpoints return HTTP 400 on Langfuse platform v4 events_only deployments, so every batch evaluation against an event-store-backed self-hosted Langfuse v4 fails on the first page (langfuse/langfuse#1861). This change routes the runner through GET /api/public/v2/observations, which is the only read path that works on v4 events_only (and remains available on v3, with the v3 endpoint scheduled for removal on 2026-11-16). The pagination state switches from page-based to cursor-based. For scope=traces, observations are collapsed to one representative per trace (preferring is_root_observation=True), since the v2 endpoint has no trace-level read. The MapperFunction protocol accepts the v2 ObservationV2 in addition to the legacy TraceWithFullDetails / ObservationsView, so existing mappers keep working with the input/output/metadata fields they already read.
…e order
The v2 observations endpoint may not return observations with the trace root
first. The previous 2-dict implementation skipped the trace after its first
appearance, so a non-root observation could prevent the root from being
picked even when the root appeared later in the page.
Switch _collapse_observations_to_traces to a single-dict form that replaces
the chosen representative whenever a strictly better candidate appears.
Also expand unit tests:
- test_fetch_batch_prefers_root_observation_regardless_of_page_order
exercises the bug above.
- four test_v2_observations_fields_* tests cover the
fetch_trace_fields to v2 field-groups translation.
Cursor-loop comment is corrected to match the actual behaviour (the
implementation has always stopped on an empty page; the previous comment
about preserving the cursor on transient empty pages was wrong).
571f327 to
26eceed
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if scope == "traces": | ||
| items = cast( | ||
| List[Union[TraceWithFullDetails, ObservationsView, ObservationV2]], | ||
| _collapse_observations_to_traces(list(response.data)), # type: ignore[arg-type] | ||
| ) |
There was a problem hiding this comment.
scope="traces", this path returns an ObservationV2, but the runner still uses item.id as the trace ID. That is the observation’s ID, not item.trace_id, so trace-level scores and optional tags are sent with the wrong trace ID and may be missing from the intended trace.
Knowledge Base Used: Experiments and batch evaluation
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/batch_evaluation.py
Line: 1276-1280
Comment:
**Observation IDs used as trace IDs** When `scope="traces"`, this path returns an `ObservationV2`, but the runner still uses `item.id` as the trace ID. That is the observation’s ID, not `item.trace_id`, so trace-level scores and optional tags are sent with the wrong trace ID and may be missing from the intended trace.
**Knowledge Base Used:** [Experiments and batch evaluation](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/experiments-and-batch-evaluation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 6021c60 — _get_item_id returns trace_id for scope='traces'.
| # The legacy ``/api/public/traces`` ``io`` / ``scores`` / ``observations`` / | ||
| # ``metrics`` field groups are not selectable on the v2 read endpoint because | ||
| # it returns one observation at a time. | ||
| _DEFAULT_V2_OBSERVATION_FIELDS = "core,basic,io,usage,model,trace_context" |
There was a problem hiding this comment.
metadata field group; io supplies only input and output. With these defaults, observation metadata is absent, so a mapper that reads it receives None. The documented mapper’s observation.metadata.get(...) path then fails for non-generation observations.
| _DEFAULT_V2_OBSERVATION_FIELDS = "core,basic,io,usage,model,trace_context" | |
| _DEFAULT_V2_OBSERVATION_FIELDS = "core,basic,io,metadata,usage,model,trace_context" |
Knowledge Base Used: Experiments and batch evaluation
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/batch_evaluation.py
Line: 41
Comment:
**Default fields omit metadata** The v2 API requires a separate `metadata` field group; `io` supplies only input and output. With these defaults, observation metadata is absent, so a mapper that reads it receives `None`. The documented mapper’s `observation.metadata.get(...)` path then fails for non-generation observations.
```suggestion
_DEFAULT_V2_OBSERVATION_FIELDS = "core,basic,io,metadata,usage,model,trace_context"
```
**Knowledge Base Used:** [Experiments and batch evaluation](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/experiments-and-batch-evaluation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 6021c60 — metadata added to the default v2 field groups.
| if scope == "traces": | ||
| items = cast( | ||
| List[Union[TraceWithFullDetails, ObservationsView, ObservationV2]], | ||
| _collapse_observations_to_traces(list(response.data)), # type: ignore[arg-type] | ||
| ) |
There was a problem hiding this comment.
Knowledge Base Used: Experiments and batch evaluation
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/batch_evaluation.py
Line: 1276-1280
Comment:
**Traces repeat across pages** If a trace’s observations span cursor pages, this helper chooses a representative independently on each page. The runner evaluates the trace again on the next page; a child on one page and the root on another can therefore produce separate evaluations for the same trace instead of one.
**Knowledge Base Used:** [Experiments and batch evaluation](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/experiments-and-batch-evaluation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 6021c60 — the runner now tracks seen trace IDs across cursor pages.
| if scope == "traces": | ||
| items = cast( | ||
| List[Union[TraceWithFullDetails, ObservationsView, ObservationV2]], | ||
| _collapse_observations_to_traces(list(response.data)), # type: ignore[arg-type] | ||
| ) |
There was a problem hiding this comment.
scope="traces", the selected ObservationV2 has start_time but no timestamp. The runner consequently saves an empty resume timestamp, then applies a timestamp > "" filter to the v2 observations request. The resume token cannot reliably continue from the last processed item.
Knowledge Base Used: Experiments and batch evaluation
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/batch_evaluation.py
Line: 1276-1280
Comment:
**Trace resumes lack a timestamp** If a later page fails during `scope="traces"`, the selected `ObservationV2` has `start_time` but no `timestamp`. The runner consequently saves an empty resume timestamp, then applies a `timestamp > ""` filter to the v2 observations request. The resume token cannot reliably continue from the last processed item.
**Knowledge Base Used:** [Experiments and batch evaluation](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/experiments-and-batch-evaluation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 6021c60 — uses the observation's start_time and the startTime filter column.
| response = self.client.api.observations.get_many( # type: ignore[union-attr] | ||
| cursor=cursor, | ||
| limit=limit, | ||
| filter=filter, | ||
| request_options={"max_retries": max_retries}, | ||
| fields=v2_fields, | ||
| ) |
There was a problem hiding this comment.
scope="traces", the original filter now goes to the observations endpoint unchanged. A name = "checkout" filter that previously selected traces by name now selects observations by name; a matching trace whose root observation has a different name is omitted, while an observation in an unrelated trace can match.
Knowledge Base Used: Experiments and batch evaluation
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/batch_evaluation.py
Line: 1264-1270
Comment:
**Trace filters change meaning** For `scope="traces"`, the original filter now goes to the observations endpoint unchanged. A `name = "checkout"` filter that previously selected traces by name now selects observations by name; a matching trace whose root observation has a different name is omitted, while an observation in an unrelated trace can match.
**Knowledge Base Used:** [Experiments and batch evaluation](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/experiments-and-batch-evaluation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if max_items is not None and total_items_fetched >= max_items: | ||
| has_more = True # More items exist but we're stopping | ||
| break |
There was a problem hiding this comment.
max_items is reached on a page whose v2 cursor is None, this branch sets has_more back to True. The result then reports has_more_items=True even though the server says there are no further results, which can prompt callers to look for work that does not exist.
Knowledge Base Used: Experiments and batch evaluation
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/batch_evaluation.py
Line: 1169-1171
Comment:
**Final page reports more items** When `max_items` is reached on a page whose v2 cursor is `None`, this branch sets `has_more` back to `True`. The result then reports `has_more_items=True` even though the server says there are no further results, which can prompt callers to look for work that does not exist.
**Knowledge Base Used:** [Experiments and batch evaluation](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/experiments-and-batch-evaluation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 6021c60 — has_more stays false when the server already returned cursor=None.
Copilot and Greptile flagged six concerns against the previous two commits; this commit resolves them in one place. - Default v2 field groups: add ``metadata``. The ``io`` group only carries input/output strings; ``metadata`` is a separate v2 field group and was missing from the defaults, so every ``ObservationV2`` passed to a mapper had ``metadata=None``. The defaults are now ``core,basic,io,metadata,model,usage,trace_context``. - Cross-page trace collapse state: ``BatchEvaluationRunner`` now tracks the trace IDs already collapsed on an earlier page in ``self._seen_trace_ids`` and ``_collapse_observations_to_traces`` accepts an optional set so the same trace is never evaluated twice across cursor pages. - ``_get_item_id`` for ``scope=traces``: the item is now an ``ObservationV2`` whose ``id`` is the observation ID, not the trace ID. Use ``trace_id`` instead so downstream score-create calls attach to the intended trace. - ``_get_item_timestamp`` and ``_get_timestamp_field_for_scope``: use the observation ``start_time`` as a proxy for the trace timestamp; the v2 filter column is ``startTime``. Resume tokens continue to work; legacy ``TraceWithFullDetails.timestamp`` is still consulted as a fallback. - Final-page ``has_more`` report: do not set ``has_more = True`` when the server already returned ``cursor=None``; the previous code reported ``has_more_items=True`` even though the server said there were no further results. - ``_translate_trace_filter``: rewrite ``name`` -> ``traceName`` and ``timestamp`` -> ``startTime`` in v3-shaped trace filters so a trace name filter selects the same set of traces on the v2 endpoint as it did on v3. The translation only runs for ``scope=traces``. Tests: 6 new unit tests cover the cross-page collapse, filter translation, the missing-metadata field group, the scope-aware ``_get_item_id`` and ``_get_item_timestamp`` paths, and the post-cursor-empty max-items path. All 16 tests in tests/unit/test_batch_evaluation_fetch.py pass; the larger unit suite shows 695 passed, 2 skipped, and the same 18 pre-existing test_prompt.py credential errors that exist on base_sha.
|
cc @hassiebp for review when you have a moment. Review feedback is addressed in 6021c60 and 6865554 (filter translation, trace-id usage, cross-page dedupe, resume timestamps, field defaults, has_more edge cases — details in the commit messages). One open question: would you prefer the v2 read path as the default (v3 reads are deprecated with EOL 2026-11-16) or gated behind a flag? Happy to split a flag-gated rollout out as a follow-up if that's easier to land. |
… paths Follow-up to the review-fix commit, from a local pre-merge adversarial review (12-item checklist run by a reviewer subagent). Docstring corrections (no runtime change): the public docstrings for run_batched_evaluation and BatchEvaluationRunner.run_async still claimed the runner reads the v3 endpoints and "is not yet supported with platform v4" — the exact opposite of what this PR does. They now describe the v2 observations endpoint, cursor pagination, the per-trace collapse, and the v2 field-group semantics of fetch_trace_fields (legacy-only groups are dropped; user-supplied groups are merged into the defaults). Hardening: - Trace filter translation now also rewrites ``id`` to ``traceId``; a v3 trace-id filter previously matched against v2 observation ids and silently returned an empty selection. - An empty page no longer leaves ``has_more`` set when the server also returned a cursor; the run is treated as exhausted, matching the pre-migration behaviour. - The fetch-failure early return now flushes the client before building the resume token, so scores already created for earlier pages are not lost with the process (pre-existing gap on base, one-line fix). Tests: five new run_async-level tests cover the max-items-on-last-page reporting (completed=True / has_more_items=False), max-items mid-stream (has_more_items=True), the empty-first-page completion, the _seen_trace_ids reset between consecutive runs, and the flush before the resume-token return. The filter-translation test now asserts the ``id -> traceId`` rewrite. 21 tests in the file; full unit suite 700 passed, 2 skipped, 18 pre-existing test_prompt.py credential errors.
What does this PR do?
Fixes #1861 —
batch_evaluationfails on Langfuse v4 events_only deployments becauseBatchEvaluationRunner._fetch_batch_with_retrystill calls the v3 read endpoints (GET /api/public/tracesand legacyGET /api/public/observations), both of which return:This PR routes both
scope='traces'andscope='observations'throughGET /api/public/v2/observations, which is the only read path that works on events_only deployments and is also the direction Langfuse is taking as v3 deprecates toward the 2026-11-16 EOL.Implementation:
_fetch_batch_with_retrynow callsclient.api.observations.get_manywithcursor/limit/filter/fields, replacing both thetrace.listand thelegacy.observations_v1.get_manycalls. The page-modepageparameter is replaced with cursor pagination (response.meta.cursor)._v2_observations_fieldstranslates the legacyfetch_trace_fieldsargument into the v2 field-groups understood by/api/public/v2/observations. Caller-supplied groups are merged with the v2 defaults; legacy-only groups (observations,scores) are dropped because the v2 endpoint returns one observation at a time and does not expose trace-level sub-collections._collapse_observations_to_tracescollapses a page of v2 observations to one representative per trace, preferring the one the server already marked as the root (is_root_observation=True) regardless of its position in the page.MapperFunction's Protocol union now includesObservationV2additively. The existing user-supplied mapper signature stays the same; mappers that readitem.input/item.output/item.metadatawork unchanged.meta.cursor=None.Behavioural change for
scope='traces'Items passed to the mapper go from
TraceWithFullDetails(which carriedobservations,scores,metricsarrays per trace) to oneObservationV2per trace (the root). Mappers that previously consumed the fullobservations/scoreslists on a trace need to adapt. Mappers that read the root observation'sinput,output,metadata,model,usagework as-is. This change is the same direction the issue reporter suggested (workaround: "fetch v2 observations grouped by traceId, root observation for trace-level io") and aligns withlangfuse/langfuse-python#1867(batch evaluation marked as legacy, v2-only path).scope='observations'items now also come from/api/public/v2/observationsinstead of the v1 read endpoint; items areObservationV2instances withinput,output,metadata,model,usageandtrace_contextpopulated.Known limitations (disclosed, not fixed here)
metadatavalues to 200 characters by default (expandMetadatareturns full values). The runner does not currently passexpandMetadata, so mappers reading very long metadata values get truncated strings where the v3 path returned full values. Passing afetch_expand_metadata-style option throughrun_batched_evaluationwould be a natural follow-up.scope='traces'resume boundary: resume filters on the representative observation'sstart_timewithstartTime > T. A trace whose root started at or beforeTbut has children starting afterTwill re-enter the resumed run through those children (the cross-pageseenset is cleared on resume), and may be re-evaluated once with a child observation as the representative. The v2 filter grammar has no not-equal operator ontraceIdand no trace-level read, so there is no clean fix at the API layer; documenting the behaviour seemed better than silently accepting or over-engineering around it.Type of change
scope='traces'; see above)Verification
Commands run on
68655544against65392c73(base_sha, current upstreammain):End-to-end against a real Langfuse v4.42.0 events_only docker-compose stack (postgres + clickhouse + redis + minio + langfuse-web + langfuse-worker):
65392c73):batch_evaluationreturns 0 scores; every retry fails with HTTP 404 events_only onGET /api/public/v3/tracesand the v3 observations endpoint.045f98aa): seeded 3 traces via SDK; ranbatch_evaluationwithscope='observations';total_items_processed=22,total_scores_created=15,completed=true. New scores visible atGET /api/public/v2/scores.Full end-to-end log:
oss-agent/logs/20260923T103452-nightly-5f51a956/e2e-after-fix.log.Checklist
code_review.md.tests/unit/test_batch_evaluation_fetch.py)..env.templateif needed — not needed; no user-facing config touched.langfuse/api/was not touched.ObservationV2is imported from the generatedlangfuse.apinamespace.