feat(evaluation): v2 native LLM-as-judge (ground truth) + Langfuse-free#1055
Open
Ayush8923 wants to merge 9 commits into
Open
feat(evaluation): v2 native LLM-as-judge (ground truth) + Langfuse-free#1055Ayush8923 wants to merge 9 commits into
Ayush8923 wants to merge 9 commits into
Conversation
github-actions
Bot
changed the title
📝 WalkthroughWalkthroughAdds a separate ChangesEvaluation v2 API and dataset flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
OpenAPI changes 🟢 8 non-breaking changesTip Safe to merge from an API-contract perspective. Full changelog ·
|
| Method | Path | Change | |
|---|---|---|---|
| 🟢 | GET |
/api/v1/evaluations |
added the optional property data/anyOf[subschema #1]/items/is_judge_run to the response with the 200 status |
| 🟢 | GET |
/api/v1/evaluations |
added the optional property data/anyOf[subschema #1]/items/per_item_ground_truth to the response with the 200 status |
| 🟢 | POST |
/api/v1/evaluations |
added the optional property data/anyOf[subschema #1: EvaluationRunPublic]/is_judge_run to the response with the 200 status |
| 🟢 | POST |
/api/v1/evaluations |
added the optional property data/anyOf[subschema #1: EvaluationRunPublic]/per_item_ground_truth to the response with the 200 status |
| 🟢 | GET |
/api/v1/evaluations/{evaluation_id} |
added the optional property data/anyOf[subschema #1: EvaluationRunPublic]/is_judge_run to the response with the 200 status |
| 🟢 | GET |
/api/v1/evaluations/{evaluation_id} |
added the optional property data/anyOf[subschema #1: EvaluationRunPublic]/per_item_ground_truth to the response with the 200 status |
| 🟢 | POST |
/api/v2/evaluations |
endpoint added |
| 🟢 | POST |
/api/v2/evaluations/datasets |
endpoint added |
main ↔ 0cb1b4b8 · generated by oasdiff
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Ayush8923
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/crud/evaluations/fast.py (1)
1110-1118: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude the required
categoryfield in everyTraceData.
TraceDatadeclarescategoryas required, but these v2 records omit it. This breaks the score-store contract and can fail consumers that access the field directly.Proposed fix
traces.append( { "trace_id": ref, "question": response.get("question", ""), "llm_answer": response.get("generated_output", ""), "ground_truth_answer": response.get("ground_truth", ""), "question_id": response.get("question_id"), + "category": response.get("category", ""), "scores": trace_scores, } )🤖 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 `@backend/app/crud/evaluations/fast.py` around lines 1110 - 1118, Update the trace record construction in the v2 evaluation flow to include the required category field in every TraceData appended to traces. Populate category from the corresponding response data using the established category value or default, while preserving the existing fields and score-store contract.backend/app/services/evaluations/fast.py (1)
247-268: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist
is_judge_runatomically with run creation.The run is committed first, then updated outside the startup
try. If that second commit fails, a pending run remains withis_judge_run=False, and its uniquerun_nameprevents a clean retry. Include the marker in the initial creation transaction or ensure this failure marks/removes the partially created run.🤖 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 `@backend/app/services/evaluations/fast.py` around lines 247 - 268, Update the run-creation flow around create_evaluation_run_or_409 so is_judge_run is persisted atomically with the initial evaluation-run creation, rather than via the subsequent update_evaluation_run call. Pass the marker through the creation helper or transaction, preserving the existing v1 behavior where is_judge_run is false and ensuring no committed run can remain with an incorrect marker if startup fails.
🧹 Nitpick comments (6)
backend/app/tests/services/evaluations/test_dataset_v2.py (1)
45-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract repeated string literal into a constant.
As per coding guidelines, repeated literals should be extracted into constants. Consider extracting
"s3://bucket/datasets/v2.csv"to a module-level constant (e.g.,_MOCK_S3_URL) since it is also used intest_stores_original_rows_and_runtime_dup_metadata.♻️ Proposed refactor
patch(f"{_DATASET}.get_cloud_storage", return_value=MagicMock()), patch( f"{_DATASET}.upload_csv_to_object_store", - return_value="s3://bucket/datasets/v2.csv", + return_value=_MOCK_S3_URL, ),Add the constant definition at the top of the file:
_DATASET = "app.services.evaluations.dataset" _MOCK_S3_URL = "s3://bucket/datasets/v2.csv"🤖 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 `@backend/app/tests/services/evaluations/test_dataset_v2.py` around lines 45 - 48, Extract the repeated S3 URL literal into a module-level constant such as _MOCK_S3_URL, alongside _DATASET, and replace its occurrences in the dataset evaluation tests, including test_stores_original_rows_and_runtime_dup_metadata and the upload_csv_to_object_store patch.Source: Coding guidelines
backend/app/services/evaluations/judge.py (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
"N/A"trace ID to a constant.As per coding guidelines, do not use magic values in code.
"N/A"is used as a fallback trace identifier across multiple files.
backend/app/services/evaluations/judge.py#L25-L25: replace the"N/A"default value with a shared constant (e.g.,DEFAULT_TRACE_ID).backend/app/api/routes/evaluations/evaluation_v2.py#L60-L60: reference the same extracted constant here instead of the string literal.🤖 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 `@backend/app/services/evaluations/judge.py` at line 25, Extract the shared "N/A" fallback into a DEFAULT_TRACE_ID constant and update the default trace_id in the judge definition at backend/app/services/evaluations/judge.py:25-25 to reference it. Update the evaluation route at backend/app/api/routes/evaluations/evaluation_v2.py:60-60 to use the same imported constant, removing both duplicated string literals.Source: Coding guidelines
backend/app/alembic/versions/074_add_judge_columns_to_evaluation_run.py (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd missing type hints.
As per coding guidelines, every function parameter and return value must have a type hint, even in migrations and tests.
backend/app/alembic/versions/074_add_judge_columns_to_evaluation_run.py#L28-L28: add-> None:toupgrade().backend/app/alembic/versions/074_add_judge_columns_to_evaluation_run.py#L55-L55: add-> None:todowngrade().backend/app/tests/api/routes/test_evaluation_v2.py#L87-L94: add_patch_dispatch: MagicMockand-> None:return type.backend/app/tests/api/routes/test_evaluation_v2.py#L120-L127: add_patch_dispatch: MagicMockand-> None:return type.backend/app/tests/api/routes/test_evaluation_v2.py#L150-L157: add_patch_dispatch: MagicMockand-> None:return type.🤖 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 `@backend/app/alembic/versions/074_add_judge_columns_to_evaluation_run.py` at line 28, Missing type hints must be added to all listed functions: update upgrade() and downgrade() in backend/app/alembic/versions/074_add_judge_columns_to_evaluation_run.py at lines 28 and 55 to return None, and update the three test functions in backend/app/tests/api/routes/test_evaluation_v2.py at lines 87-94, 120-127, and 150-157 to annotate _patch_dispatch as MagicMock and return None.Source: Coding guidelines
backend/app/services/evaluations/dataset.py (1)
182-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRevise docstrings to explain 'why' instead of recapping operations.
As per coding guidelines, avoid self-evident comments and obvious operation recaps. The following docstrings merely repeat what the code block clearly implements:
backend/app/services/evaluations/dataset.py#L182-L184: revise to explain why v2 bypasses Langfuse, or remove the self-evident procedural recap.backend/app/api/routes/evaluations/evaluation_v2.py#L40-L44: remove the self-evident "Start a v2 evaluation run" phrasing.backend/app/services/evaluations/judge.py#L27-L32: replace the step-by-step dispatch narration with the underlying intent.backend/app/crud/evaluations/cost.py#L128-L128: remove the self-evident operation recap.🤖 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 `@backend/app/services/evaluations/dataset.py` around lines 182 - 184, Revise the docstrings at backend/app/services/evaluations/dataset.py:182-184, backend/app/api/routes/evaluations/evaluation_v2.py:40-44, backend/app/services/evaluations/judge.py:27-32, and backend/app/crud/evaluations/cost.py:128 to describe the underlying purpose or rationale rather than restating obvious operations; specifically explain why v2 bypasses Langfuse in dataset.py, remove the “Start a v2 evaluation run” recap in evaluation_v2.py, replace judge.py’s dispatch steps with their intent, and remove the self-evident operation recap in cost.py.Source: Coding guidelines
backend/app/crud/evaluations/judge.py (1)
263-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the repository’s mandatory function-typing rule throughout the judge slice.
backend/app/crud/evaluations/judge.py#L263-L265: replace theAnyreturn with the concrete Responses SDK type or a narrow protocol.backend/app/tests/crud/evaluations/test_fast_judge.py#L114-L247: type helper parameters, callbacks, and return values.backend/app/tests/crud/evaluations/test_fast_judge.py#L261-L461: add-> Noneand fixture parameter types to test methods.backend/app/tests/crud/evaluations/test_judge.py#L37-L49: typeusageand the response helper’s return value.backend/app/tests/crud/evaluations/test_judge.py#L198-L218: type_capturefully.As per coding guidelines, every function parameter and return value must be typed, and
-> Anymust be narrowed.🤖 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 `@backend/app/crud/evaluations/judge.py` around lines 263 - 265, Apply the mandatory typing rule across all listed judge code: update _create_judge_response in backend/app/crud/evaluations/judge.py:263-265 to return the concrete Responses SDK type or a narrow protocol instead of Any; type helper parameters, callbacks, and returns in backend/app/tests/crud/evaluations/test_fast_judge.py:114-247; add -> None and fixture parameter annotations in backend/app/tests/crud/evaluations/test_fast_judge.py:261-461; type usage and the response helper return in backend/app/tests/crud/evaluations/test_judge.py:37-49; and fully annotate _capture in backend/app/tests/crud/evaluations/test_judge.py:198-218.Source: Coding guidelines
backend/app/core/security.py (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse built-in
tupleannotations here.typing.Tupleis deprecated on Python 3.12, so switch these annotations and drop the import.backend/app/core/security.py:16, 266, 291🤖 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 `@backend/app/core/security.py` at line 16, Replace typing.Tuple annotations in backend/app/core/security.py with the built-in tuple syntax, update the affected annotations near the referenced locations, and remove Tuple from the typing import while preserving the existing type parameters and behavior.Source: Linters/SAST tools
🤖 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 `@backend/app/api/routes/evaluations/dataset_v2.py`:
- Around line 34-62: Move the blocking upload_dataset_v2 operation out of the
async event loop by either making upload_dataset_v2_route synchronous or
dispatching the complete upload workflow to a worker thread. Ensure the database
session lifecycle is created and managed within that synchronous boundary,
rather than using the request-bound session during threaded execution, while
preserving the existing response behavior.
In `@backend/app/core/config.py`:
- Around line 214-218: Update the EVAL_JUDGE_REASONING_EFFORT setting in the
configuration settings class to use a Literal type containing only the
documented values: none, minimal, low, medium, high, and xhigh. Preserve the
default of minimal so settings initialization rejects invalid environment values
before they reach the judge client.
In `@backend/app/crud/evaluations/fast.py`:
- Around line 1016-1029: The judge stage around _judge_rows must be
retry-idempotent: persist each judge result with an explicit completion marker,
reload completed judge results when Stage 3 re-enters, and invoke the paid judge
only for unfinished rows. Ensure reloaded and newly completed results are both
used when attaching scores and calculating cost, preserving persisted scores
across retries.
In `@backend/app/crud/evaluations/judge.py`:
- Around line 1-4: Update the module docstring describing the judge pipeline to
state that v2 replaces cosine similarity entirely, rather than running after it,
and remove the claim that v1 invokes or depends on this judge. Preserve the
existing descriptions of one judge call per row and registry-driven metrics.
- Around line 1-4: Update the module docstring in
backend/app/crud/evaluations/judge.py at lines 1-4 to state that v2 runs judges
after generation without cosine and that v1 does not invoke the judge. Update
docs/srd-three-metric-evaluation-verdict.md at lines 17-19 to align release
scope and downstream API/schema requirements with the delivered contract:
fast-only, system-config-only, ground-truth-only, and cosine-free.
- Around line 312-320: The judge evaluation flow around _parse_judge_output must
preserve captured usage when parsing fails. Update the failure path in the
enclosing judge function and _judge_rows integration so malformed responses
produce a failure outcome or typed exception carrying the usage data, allowing
attach_cost to aggregate tokens from failed evaluations while retaining existing
successful-result behavior.
In `@backend/app/crud/evaluations/score.py`:
- Around line 32-59: Update the judge prompt construction around
JUDGE_SYSTEM_PREAMBLE, GROUND_TRUTH_JUDGE_PROMPT, and JUDGE_OUTPUT_INSTRUCTION
to explicitly treat the question, generated answer, and golden answer as
untrusted data and prohibit following instructions embedded in them. Delimit
each field clearly in the assembled prompt, preserving the existing JSON-only
output contract. Add an adversarial regression test covering an answer that
attempts to override the rubric while still producing valid JSON.
In `@backend/app/services/evaluations/fast.py`:
- Around line 63-88: Update load_run_dataset_items and every caller, including
the startup sizing and chunk-worker paths, to accept the judge-run/source
preference and select object_store_url for judge runs regardless of
dataset.langfuse_dataset_id. Ensure judge-run execution does not construct or
require a Langfuse client, while preserving the existing Langfuse fetch path for
non-judge runs. Add coverage for a judge run using a v1-created dataset.
---
Outside diff comments:
In `@backend/app/crud/evaluations/fast.py`:
- Around line 1110-1118: Update the trace record construction in the v2
evaluation flow to include the required category field in every TraceData
appended to traces. Populate category from the corresponding response data using
the established category value or default, while preserving the existing fields
and score-store contract.
In `@backend/app/services/evaluations/fast.py`:
- Around line 247-268: Update the run-creation flow around
create_evaluation_run_or_409 so is_judge_run is persisted atomically with the
initial evaluation-run creation, rather than via the subsequent
update_evaluation_run call. Pass the marker through the creation helper or
transaction, preserving the existing v1 behavior where is_judge_run is false and
ensuring no committed run can remain with an incorrect marker if startup fails.
---
Nitpick comments:
In `@backend/app/alembic/versions/074_add_judge_columns_to_evaluation_run.py`:
- Line 28: Missing type hints must be added to all listed functions: update
upgrade() and downgrade() in
backend/app/alembic/versions/074_add_judge_columns_to_evaluation_run.py at lines
28 and 55 to return None, and update the three test functions in
backend/app/tests/api/routes/test_evaluation_v2.py at lines 87-94, 120-127, and
150-157 to annotate _patch_dispatch as MagicMock and return None.
In `@backend/app/core/security.py`:
- Line 16: Replace typing.Tuple annotations in backend/app/core/security.py with
the built-in tuple syntax, update the affected annotations near the referenced
locations, and remove Tuple from the typing import while preserving the existing
type parameters and behavior.
In `@backend/app/crud/evaluations/judge.py`:
- Around line 263-265: Apply the mandatory typing rule across all listed judge
code: update _create_judge_response in
backend/app/crud/evaluations/judge.py:263-265 to return the concrete Responses
SDK type or a narrow protocol instead of Any; type helper parameters, callbacks,
and returns in backend/app/tests/crud/evaluations/test_fast_judge.py:114-247;
add -> None and fixture parameter annotations in
backend/app/tests/crud/evaluations/test_fast_judge.py:261-461; type usage and
the response helper return in
backend/app/tests/crud/evaluations/test_judge.py:37-49; and fully annotate
_capture in backend/app/tests/crud/evaluations/test_judge.py:198-218.
In `@backend/app/services/evaluations/dataset.py`:
- Around line 182-184: Revise the docstrings at
backend/app/services/evaluations/dataset.py:182-184,
backend/app/api/routes/evaluations/evaluation_v2.py:40-44,
backend/app/services/evaluations/judge.py:27-32, and
backend/app/crud/evaluations/cost.py:128 to describe the underlying purpose or
rationale rather than restating obvious operations; specifically explain why v2
bypasses Langfuse in dataset.py, remove the “Start a v2 evaluation run” recap in
evaluation_v2.py, replace judge.py’s dispatch steps with their intent, and
remove the self-evident operation recap in cost.py.
In `@backend/app/services/evaluations/judge.py`:
- Line 25: Extract the shared "N/A" fallback into a DEFAULT_TRACE_ID constant
and update the default trace_id in the judge definition at
backend/app/services/evaluations/judge.py:25-25 to reference it. Update the
evaluation route at backend/app/api/routes/evaluations/evaluation_v2.py:60-60 to
use the same imported constant, removing both duplicated string literals.
In `@backend/app/tests/services/evaluations/test_dataset_v2.py`:
- Around line 45-48: Extract the repeated S3 URL literal into a module-level
constant such as _MOCK_S3_URL, alongside _DATASET, and replace its occurrences
in the dataset evaluation tests, including
test_stores_original_rows_and_runtime_dup_metadata and the
upload_csv_to_object_store patch.
🪄 Autofix (Beta)
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
Run ID: 09a0286a-29ee-438d-9511-c611df7b75f8
📒 Files selected for processing (28)
backend/app/alembic/versions/074_add_judge_columns_to_evaluation_run.pybackend/app/api/docs/evaluation/create_evaluation_dataset_v2.mdbackend/app/api/docs/evaluation/create_evaluation_v2.mdbackend/app/api/main.pybackend/app/api/routes/evaluations/dataset_v2.pybackend/app/api/routes/evaluations/evaluation_v2.pybackend/app/core/config.pybackend/app/core/security.pybackend/app/crud/evaluations/cost.pybackend/app/crud/evaluations/dataset.pybackend/app/crud/evaluations/fast.pybackend/app/crud/evaluations/judge.pybackend/app/crud/evaluations/langfuse.pybackend/app/crud/evaluations/score.pybackend/app/main.pybackend/app/models/evaluation.pybackend/app/services/evaluations/__init__.pybackend/app/services/evaluations/dataset.pybackend/app/services/evaluations/fast.pybackend/app/services/evaluations/judge.pybackend/app/tests/api/routes/test_evaluation_dataset_v2.pybackend/app/tests/api/routes/test_evaluation_v2.pybackend/app/tests/crud/evaluations/test_fast_judge.pybackend/app/tests/crud/evaluations/test_judge.pybackend/app/tests/services/evaluations/test_dataset_v2.pybackend/app/tests/services/evaluations/test_load_run_dataset_items.pydocs/srd-three-metric-evaluation-verdict.mddocs/wiki/modules/evaluations.md
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
1 participant
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.
Issue
Closes #1050 #959
Summary
POST /api/v2/evaluations/datasets--> a Langfuse-free dataset upload. Same multipart shape as v1, but stores the CSV in S3 only (langfuse_dataset_idNULL), keeps original items only (no physical duplication), and recordsduplication_factor+ a run-time-duplication marker indataset_metadata.POST /api/v2/evaluations--> starts an evaluation run. Same request body as v1. v2 runs are always fast and always judged, norun_mode(batch is deferred).gpt-5-mini, a reasoning model, sent withreasoning.effort=minimaland no temperature + the built-in prompt.item ×duplication_factorat load time (unique id per copy). A v1-created (Langfuse-backed) dataset reads its S3 data as-is without re-multiplying.is_judge_runmarker onevaluation_runlets the shared chunked pipeline (which the aggregate runs byeval_run_idonly) skip cosine/embeddings/Langfuse for v2 while v1 stays identical: cosine + embeddings + Langfuse sync exactly as before, and v1 still requires a Langfuse-backed dataset.Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.