[LEADS-572] Storing result in langfuse dataset (optional) - #312
[LEADS-572] Storing result in langfuse dataset (optional)#312bsatapat-jpg wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughLangfuse storage now accepts an optional ChangesLangfuse dataset export
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Evaluation
participant LangfuseStorage
participant LangfuseAPI as Langfuse API
Evaluation->>LangfuseStorage: finalize evaluation
LangfuseStorage->>LangfuseAPI: write per-turn traces and aggregate scores
LangfuseAPI-->>LangfuseStorage: return trace IDs
LangfuseStorage->>LangfuseAPI: create or reuse dataset
LangfuseStorage->>LangfuseAPI: upsert dataset items and create run items
LangfuseStorage->>LangfuseAPI: flush export
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lightspeed_evaluation/core/storage/langfuse_storage.py`:
- Around line 251-255: Update the item_id construction in the Langfuse storage
export flow to include dataset_name along with conversation_group_id and
turn_id, ensuring IDs are unique across the Langfuse project. Preserve the
existing truncation limit and conversation fallback while preventing identical
conversation/turn values from different datasets from sharing an ID.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 26035653-3900-4027-8a45-c414397fca92
📒 Files selected for processing (6)
README.mdconfig/system.yamldocs/configuration.mdsrc/lightspeed_evaluation/core/storage/config.pysrc/lightspeed_evaluation/core/storage/langfuse_storage.pytests/unit/core/storage/test_langfuse_storage.py
8c13a68 to
38b974b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/lightspeed_evaluation/core/storage/langfuse_storage.py (2)
135-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the inline pylint suppressions with explicit exception types.
The repository guideline prohibits inline lint suppressions in Python files. This change adds
# pylint: disable=broad-exception-caughtat Line 135, Line 435, and Line 448. The Langfuse SDK raisesRuntimeError,ValueError,OSError,ConnectionError, andValidationErrorin this module;initialize()andclose()already catch those explicitly. Catch the same set here, or move the suppression into the pylint configuration with a rationale.Based on learnings: pylint disable directives are allowed in test files, but the prohibition still applies to production code under
src/**/*.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lightspeed_evaluation/core/storage/langfuse_storage.py` at line 135, Replace the broad exception handlers at the affected storage methods with explicit catches for RuntimeError, ValueError, OSError, ConnectionError, and ValidationError, matching the existing handling in initialize() and close(). Remove the inline pylint suppressions at all three production-code locations and preserve the current exception-handling behavior.Sources: Coding guidelines, Learnings
330-337: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNarrow the tolerated
ValidationErrorfor run items.
_upsert_dataset_itemre-raises unknown validation errors._create_dataset_run_itemswallows everyValidationError. A genuine payload error, for example an invalidrun_nameortrace_id, is then reported only as a warning and the run item is silently missing. Apply the same missing-field check that the item path uses.♻️ Proposed change
except ValidationError as exc: # Same class of skew as dataset items: HTTP 2xx then strict parse fails. + if not _is_missing_field_validation_error(exc, "media_references"): + raise logger.warning( "langfuse: dataset run item for %r likely created; " "ignoring SDK response parse error: %s", dataset_item_id, exc, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lightspeed_evaluation/core/storage/langfuse_storage.py` around lines 330 - 337, Update the ValidationError handling in _create_dataset_run_item to tolerate only errors caused by the known missing response fields, matching the check used by _upsert_dataset_item. Re-raise validation errors for invalid inputs such as run_name or trace_id, while preserving the warning-and-ignore behavior for the recognized HTTP-2xx response parse skew.tests/unit/core/storage/test_langfuse_storage.py (2)
35-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive each span a distinct trace ID.
_wire_mock_spanreturns the sametrace_idfor everystart_as_current_observationcall. The dataset test creates two turn traces, so it cannot detect a swapped or stale trace link between the two dataset items. Accept a sequence of trace IDs and return the next one per call.♻️ Proposed change
def _wire_mock_span( - mock_client: Any, mocker: MockerFixture, trace_id: str = "trace-abc-123" + mock_client: Any, mocker: MockerFixture, *trace_ids: str ) -> Any: """Attach a context-manager span mock to a Langfuse client mock.""" - mock_span = mocker.MagicMock() - mock_span.trace_id = trace_id - mock_client.start_as_current_observation.return_value.__enter__ = mocker.MagicMock( - return_value=mock_span - ) + ids = list(trace_ids) or ["trace-abc-123"] + spans = [] + for trace_id in ids: + span = mocker.MagicMock() + span.trace_id = trace_id + spans.append(span) + mock_client.start_as_current_observation.return_value.__enter__ = mocker.MagicMock( + side_effect=lambda: spans[min( + mock_client.start_as_current_observation.call_count - 1, len(spans) - 1 + )] + ) mock_client.start_as_current_observation.return_value.__exit__ = mocker.MagicMock( return_value=False ) - return mock_span + return spans[0]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/core/storage/test_langfuse_storage.py` around lines 35 - 47, Update _wire_mock_span to accept a sequence of trace IDs and provide the next distinct ID for each start_as_current_observation call, rather than reusing one default ID. Preserve the context-manager mock behavior while ensuring successive span creations receive successive IDs.
342-365: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for a failure after partial dataset export.
This test injects the failure at
_ensure_dataset, so no turn is exported before the fallback. The uncovered case is a failure on a later turn, where scores are already in Langfuse and the fallback re-emits every score. See the related finding insrc/lightspeed_evaluation/core/storage/langfuse_storage.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/core/storage/test_langfuse_storage.py` around lines 342 - 365, The test test_finalize_dataset_failure_falls_back_to_scores currently fails during initial dataset setup and does not cover partial export. Add a later-turn failure after at least one turn has been successfully exported, then assert the fallback re-emits scores for all results while preserving the existing dataset-export error and dataset-name assertions; verify the expected dataset item and score calls using the existing mock_client setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lightspeed_evaluation/core/storage/langfuse_storage.py`:
- Around line 130-145: Track the number of turns successfully exported by
_export_dataset_run in finalize, and only call _write_trace_and_scores when no
turns were exported; preserve the existing fallback for failures before any
export. Add a unit test in
tests/unit/core/storage/test_langfuse_storage.py:342-365 that makes
create_dataset_item fail on the second turn and verifies scores are not
duplicated. The storage implementation change belongs at
src/lightspeed_evaluation/core/storage/langfuse_storage.py:130-145; the test
site requires the described regression coverage.
- Around line 470-478: Update _is_missing_field_validation_error so it returns
True only when a structured validation error has type "missing" and identifies
the requested field; remove the unconditional field-in-str(exc) fallback.
Preserve the existing location checks within the structured error loop.
---
Nitpick comments:
In `@src/lightspeed_evaluation/core/storage/langfuse_storage.py`:
- Line 135: Replace the broad exception handlers at the affected storage methods
with explicit catches for RuntimeError, ValueError, OSError, ConnectionError,
and ValidationError, matching the existing handling in initialize() and close().
Remove the inline pylint suppressions at all three production-code locations and
preserve the current exception-handling behavior.
- Around line 330-337: Update the ValidationError handling in
_create_dataset_run_item to tolerate only errors caused by the known missing
response fields, matching the check used by _upsert_dataset_item. Re-raise
validation errors for invalid inputs such as run_name or trace_id, while
preserving the warning-and-ignore behavior for the recognized HTTP-2xx response
parse skew.
In `@tests/unit/core/storage/test_langfuse_storage.py`:
- Around line 35-47: Update _wire_mock_span to accept a sequence of trace IDs
and provide the next distinct ID for each start_as_current_observation call,
rather than reusing one default ID. Preserve the context-manager mock behavior
while ensuring successive span creations receive successive IDs.
- Around line 342-365: The test
test_finalize_dataset_failure_falls_back_to_scores currently fails during
initial dataset setup and does not cover partial export. Add a later-turn
failure after at least one turn has been successfully exported, then assert the
fallback re-emits scores for all results while preserving the existing
dataset-export error and dataset-name assertions; verify the expected dataset
item and score calls using the existing mock_client setup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad7f2c2a-25ce-4a8b-8003-000dc8384694
📒 Files selected for processing (6)
README.mdconfig/system.yamldocs/configuration.mdsrc/lightspeed_evaluation/core/storage/config.pysrc/lightspeed_evaluation/core/storage/langfuse_storage.pytests/unit/core/storage/test_langfuse_storage.py
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- config/system.yaml
- src/lightspeed_evaluation/core/storage/config.py
- docs/configuration.md
5a8eedc to
d7232b8
Compare
asamal4
left a comment
asamal4
left a comment
There was a problem hiding this comment.
Thanks !!
I would recommend to merge the two Langfuse paths into one unified flow that always writes per-turn traces and scores (like MLflow does per-result) along with aggregation at the end.
a3c8648 to
2e25fdf
Compare
asamal4
left a comment
asamal4
left a comment
There was a problem hiding this comment.
Thanks !! two minor comments..
BTW the code has grown significantly, we need to refactor
| except (RuntimeError, ValueError, OSError, ConnectionError): | ||
| logger.exception("langfuse: failed to write trace and scores") | ||
| self._export_results() | ||
| except _LANGFUSE_EXPORT_ERRORS: |
There was a problem hiding this comment.
This seems to handle more exceptions, than existing initialize and close.. is this intentional ?
There was a problem hiding this comment.
Yes, there are total of 7 types of possible errors so better to handle all these.
2 participants
Description
Optional Langfuse Dataset run linking for evaluation results (LEADS-572), on top of a unified Langfuse export path (MLflow-aligned).
Unified export
save_run:(conversation_group_id, turn_id)with CSV-shaped metadatadataset_nameis set (same turns → Datasets → Runs):{run_label}__{run_id[:8]}finalize(success=...):aggregate/*scores (mean score, pass rate, per-metric means)eval_statusmetadata andeval/successscore (complete/failed)When
dataset_nameis omitted or blank, traces/scores/aggregates are still exported; only Dataset linking is skipped.Self-hosted Langfuse / SDK v4 response skew (
media_referencesmissing after HTTP 2xx) is tolerated for item and run-item creates. UnknownValidationErrors are re-raised. Dataset ensure/link failures and individual turn export failures are logged; remaining turns and end-of-run aggregates still run.Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
pip install 'lightspeed-evaluation[langfuse]'(oruv sync --extra langfuse).system.yaml(optionaldataset_namefor Dataset linking).LANGFUSE_PUBLIC_KEY/LANGFUSE_SECRET_KEY(and host if not inline).eval/successat finalize.dataset_nameset, run twice and compare under Datasets → Runs.uv run pytest tests/unit/core/storage/test_langfuse_storage.py.Known notes
media_referencesin DatasetItem / run-item responses; treated as non-fatal after a successful create.Summary by CodeRabbit
New Features
dataset_namefor side-by-side comparisons and reusable runs.Bug Fixes
Documentation