feat(evaluation): v2 native judge metrics + Langfuse-free#1055
Open
Ayush8923 wants to merge 16 commits into
Open
feat(evaluation): v2 native judge metrics + Langfuse-free#1055Ayush8923 wants to merge 16 commits into
Ayush8923 wants to merge 16 commits into
Conversation
github-actions
Bot
changed the title
📝 WalkthroughWalkthroughAdds ChangesEvaluation v2
Estimated code review effort: 5 (Critical) | ~120 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 🟢 5 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 |
| 🟢 | POST |
/api/v1/evaluations |
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]/is_judge_run to the response with the 200 status |
| 🟢 | POST |
/api/v2/evaluations |
endpoint added |
| 🟢 | POST |
/api/v2/evaluations/datasets |
endpoint added |
main ↔ 06bb9305 · 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
2 tasks
2 tasks
Co-authored-by: AkhileshNegi <akhileshnegi.an3@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/app/tests/crud/evaluations/test_fast_judge.py (2)
1178-1180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate assertion. Line 1180 repeats line 1178 verbatim. Either drop it or replace it with the intended distinct check (e.g. asserting
base_params["tools"] == []).♻️ Remove the redundant assert
assert "include" not in base_params assert "tool_choice" not in base_params - assert "include" not in base_params🤖 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/crud/evaluations/test_fast_judge.py` around lines 1178 - 1180, Remove the duplicated “include” assertion in the base_params assertions, or replace it with the intended distinct validation such as confirming base_params["tools"] equals an empty list. Keep the existing checks for “include” and “tool_choice” unchanged.
169-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd missing type hints to
_both_metrics_response.
usagelacks a type hint and the function has no return annotation, unlike the sibling helpers.♻️ Proposed types
def _both_metrics_response( *, ground_truth: tuple[float, str] = (0.8, "conveys the same facts"), prompt: tuple[float, str] = (0.6, "answered in the wrong language"), - usage=(12, 6, 18), -): + usage: tuple[int, int, int] = (12, 6, 18), +) -> ChatCompletion:Adjust the return type to match
_raw_judge_response's actual return type.As per coding guidelines: "Use type hints on every function parameter and return value;
-> Anyis not acceptable unless the type can 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/tests/crud/evaluations/test_fast_judge.py` around lines 169 - 174, Add a concrete type hint for the usage parameter and a return annotation to _both_metrics_response, matching the corresponding annotations and actual return type used by _raw_judge_response. Ensure every parameter, including usage, and the function return value are explicitly typed without using Any.Source: Coding guidelines
backend/app/tests/crud/evaluations/test_judge.py (1)
274-441: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an end-to-end
judge_rowtest for thePROMPTmetric.Coverage exists for
ground_truth/knowledge_basethroughjudge_row, but the newPROMPTmetric is only exercised via_compose_judge_input/enabled_metric_specs, never through the fulljudge_row→_create_judge_response→_parse_judge_outputpath withCONFIG_PROMPTsupplied. Adding that case would catch regressions in the newest metric's row-level scoring.🤖 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/crud/evaluations/test_judge.py` around lines 274 - 441, Add a TestJudgeRow test covering the PROMPT metric through judge_row with CONFIG_PROMPT supplied in the inputs. Mock _create_judge_response with a prompt score and reasoning, then assert the returned PROMPT MetricScore and that the prompt configuration reaches the judge request, exercising the full response parsing path.
🤖 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.
Nitpick comments:
In `@backend/app/tests/crud/evaluations/test_fast_judge.py`:
- Around line 1178-1180: Remove the duplicated “include” assertion in the
base_params assertions, or replace it with the intended distinct validation such
as confirming base_params["tools"] equals an empty list. Keep the existing
checks for “include” and “tool_choice” unchanged.
- Around line 169-174: Add a concrete type hint for the usage parameter and a
return annotation to _both_metrics_response, matching the corresponding
annotations and actual return type used by _raw_judge_response. Ensure every
parameter, including usage, and the function return value are explicitly typed
without using Any.
In `@backend/app/tests/crud/evaluations/test_judge.py`:
- Around line 274-441: Add a TestJudgeRow test covering the PROMPT metric
through judge_row with CONFIG_PROMPT supplied in the inputs. Mock
_create_judge_response with a prompt score and reasoning, then assert the
returned PROMPT MetricScore and that the prompt configuration reaches the judge
request, exercising the full response parsing path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a599002-97dd-4465-adc9-9ba22adcdad5
📒 Files selected for processing (18)
backend/app/alembic/versions/075_add_judge_column_to_evaluation_run.pybackend/app/api/docs/evaluation/create_evaluation_v2.mdbackend/app/api/routes/evaluations/dataset_v2.pybackend/app/api/routes/evaluations/evaluation_v2.pybackend/app/core/config.pybackend/app/crud/evaluations/cost.pybackend/app/crud/evaluations/fast.pybackend/app/crud/evaluations/judge.pybackend/app/crud/evaluations/score.pybackend/app/models/evaluation.pybackend/app/models/response.pybackend/app/services/evaluations/__init__.pybackend/app/services/response/response.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/response/response/test_generate_response.pydocs/wiki/modules/evaluations.md
💤 Files with no reviewable changes (2)
- backend/app/services/evaluations/init.py
- backend/app/models/evaluation.py
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/app/api/docs/evaluation/create_evaluation_v2.md
- backend/app/tests/api/routes/test_evaluation_v2.py
- backend/app/api/routes/evaluations/dataset_v2.py
- docs/wiki/modules/evaluations.md
- backend/app/crud/evaluations/fast.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/app/crud/evaluations/response_parsing.py (2)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the helper docstrings explain why, not what.
These docstrings restate the following statements instead of explaining the shared SDK/dict normalization requirement. Replace them with rationale-focused documentation, or omit them where the module docstring already provides sufficient context.
Also applies to: 21-21
🤖 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/response_parsing.py` at line 12, Update the helper docstrings in the response-parsing module, including the field-reading helper, to describe the rationale for normalizing SDK objects and test dictionaries rather than restating their behavior. If the module-level documentation already explains this shared requirement, remove the redundant helper docstrings instead.Source: Coding guidelines
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract response-shape literals into constants.
Names such as
"output_text","output","type","message", and"content"are protocol values embedded throughout the parser. Define UPPER_SNAKE_CASE constants (with separate names for the field and discriminator meanings) to prevent drift and satisfy the no-magic-values guideline. Verify these constants against the Responses payload contract.Also applies to: 26-35
🤖 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/response_parsing.py` at line 22, Replace the response-shape string literals in the parser around field_value and the related logic with module-level UPPER_SNAKE_CASE constants, using distinct names for field keys versus discriminator values. Cover “output_text”, “output”, “type”, “message”, and “content”, and update all affected references while preserving the Responses payload contract.Source: Coding guidelines
🤖 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/crud/evaluations/response_parsing.py`:
- Around line 11-17: Update field_value to use a precise generic return type
instead of Any, introducing a TypeVar and appropriate cast or overloads for
values retrieved from dictionaries and SDK objects. Preserve the existing
default and None handling while allowing callers to infer concrete types such as
str, sequences, and discriminator values.
---
Nitpick comments:
In `@backend/app/crud/evaluations/response_parsing.py`:
- Line 12: Update the helper docstrings in the response-parsing module,
including the field-reading helper, to describe the rationale for normalizing
SDK objects and test dictionaries rather than restating their behavior. If the
module-level documentation already explains this shared requirement, remove the
redundant helper docstrings instead.
- Line 22: Replace the response-shape string literals in the parser around
field_value and the related logic with module-level UPPER_SNAKE_CASE constants,
using distinct names for field keys versus discriminator values. Cover
“output_text”, “output”, “type”, “message”, and “content”, and update all
affected references while preserving the Responses payload contract.
🪄 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: 60ec331d-2648-4591-8a68-c9ebcd00473d
📒 Files selected for processing (4)
backend/app/core/config.pybackend/app/crud/evaluations/fast.pybackend/app/crud/evaluations/judge.pybackend/app/crud/evaluations/response_parsing.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/core/config.py
- backend/app/crud/evaluations/judge.py
- backend/app/crud/evaluations/fast.py
Ayush8923
changed the title
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.
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.Summary by CodeRabbit
New Features
Bug Fixes
Documentation