Conversation
Eval output scores had no way to declare which way is better. This adds direction on EvalOutputScore - higher_is_better (default), lower_is_better, or informational (context only, should never drive decisions) - so API consumers comparing results across runs can interpret score deltas without guessing. - Rating scales (five_star, pass_fail, pass_fail_critical) are higher-is-better by definition: they accept higher_is_better and informational, and reject lower_is_better. lower_is_better is reserved for custom scores (unbounded numeric metrics), which evaluators do not support yet. - Backward compatible: the field defaults to higher_is_better, so existing evals load unchanged and re-save with the field explicit; requests from clients that predate the field get the default. - The field flows through the create_evaluator and read APIs automatically via EvalOutputScore; regenerate the web client schema for the new field and enum.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
WalkthroughAdds ChangesEval score direction
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant EvaluatorCreationAPI
participant EvalOutputScore
participant EvalFile
Client->>EvaluatorCreationAPI: Submit output score direction
EvaluatorCreationAPI->>EvalOutputScore: Parse and validate direction
EvalOutputScore->>EvalFile: Persist default or explicit direction
EvalFile-->>EvaluatorCreationAPI: Saved evaluator
EvaluatorCreationAPI-->>Client: Return direction in response
Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
📊 Coverage ReportOverall Coverage: 92% Diff: origin/main...HEAD
Summary
Line-by-lineView line-by-line diff coveragelibs/core/kiln_ai/datamodel/eval.pyLines 119-131 119 if self.direction == ScoreDirection.lower_is_better:
120 raise ValueError(
121 f"Score '{self.name}' has type '{self.type.value}', which is higher-is-better by definition. 'lower_is_better' is only valid for custom scores."
122 )
! 123 case TaskOutputRatingType.custom:
124 # Any direction is valid for custom scores (unbounded numeric metrics).
! 125 pass
! 126 case _:
! 127 raise_exhaustive_enum_error(self.type)
128 return self
129
130
131 class EvalRun(KilnParentedModel):
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@app/web_ui/src/lib/api_schema.d.ts`:
- Around line 6047-6051: Update the evaluator-create input schema so
EvalOutputScore.direction is optional while retaining its server-side default
and required direction in response/output schemas. Split or override the
create-specific CreatedEvaluatorRequest/EvalOutputScore schema as needed, then
regenerate api_schema.d.ts using generate_schema.sh.
In `@libs/core/kiln_ai/datamodel/eval.py`:
- Around line 90-93: Update the ScoreDirection field description and the related
validate_type validation message to state that lower_is_better is reserved for
currently unsupported custom evaluator scores, rather than presenting it as a
usable custom-score option; keep the existing rejection behavior unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c2867f8-f448-40bf-b27e-d806c3aa69ae
📒 Files selected for processing (4)
app/desktop/studio_server/test_eval_api.pyapp/web_ui/src/lib/api_schema.d.tslibs/core/kiln_ai/datamodel/eval.pylibs/core/kiln_ai/datamodel/test_eval_model.py
| /** | ||
| * @description The direction of improvement for this score: 'higher_is_better', 'lower_is_better', or 'informational' (context only, no preferred direction). Rating scales ('five_star', 'pass_fail', 'pass_fail_critical') are higher-is-better by definition, so they allow 'higher_is_better' and 'informational' but not 'lower_is_better'. | ||
| * @default higher_is_better | ||
| */ | ||
| direction: components["schemas"]["ScoreDirection"]; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate schema/update scripts =="
fd -a 'check_schema\.sh|generate_schema\.sh' . || true
echo
echo "== relevant schema excerpt =="
if [ -f app/web_ui/src/lib/api_schema.d.ts ]; then
sed -n '5980,6075p' app/web_ui/src/lib/api_schema.d.ts | cat -n
fi
echo
echo "== search ScoreDirection usages =="
rg -n "ScoreDirection|ScoreUpdate|EvaluatorCreate|evaluator" app/web_ui/src/lib || true
echo
echo "== search schema scripts if present =="
if [ -f app/web_ui/check_schema.sh ]; then cat -n app/web_ui/check_schema.sh | sed -n '1,220p'; fi
if [ -f app/web_ui/generate_schema.sh ]; then cat -n app/web_ui/generate_schema.sh | sed -n '1,260p'; fiRepository: Kiln-AI/Kiln
Length of output: 14930
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== generate/check script excerpts =="
sed -n '1,280p' app/web_ui/src/lib/generate_schema.sh | cat -n
echo "----"
sed -n '1,240p' app/web_ui/src/lib/check_schema.sh | cat -n
echo
echo "== OpenAPI source for create evaluator relevant schemas =="
rg -n "create_evaluator|EvalOutputScore|ScoreDirection|output_scores" . -g '!app/web_ui/src/lib/api_schema.d.ts' --glob '!node_modules' --glob '!dist' --glob '!build' || true
echo
echo "== generated endpoint request/response schema block =="
sed -n '4720,4850p' app/web_ui/src/lib/api_schema.d.ts | cat -n
echo
echo "== all generated EvalOutputScore occurrences with line context =="
rg -n -C 5 'EvalOutputScore:|direction\?: components|direction: components' app/web_ui/src/lib/api_schema.d.ts || trueRepository: Kiln-AI/Kiln
Length of output: 42500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant datamodel/API schema definitions =="
sed -n '50,95p' libs/core/kiln_ai/datamodel/eval.py | cat -n
echo "---"
sed -n '150,175p' app/desktop/studio_server/eval_api.py | cat -n
echo "---"
sed -n '600,626p' app/desktop/studio_server/eval_api.py | cat -n
echo "---"
sed -n '280,310p' app/desktop/studio_server/test_eval_api.py | cat -n
echo "== schema file source metadata if available =="
fd -a 'openapi_schema\.sh|openapi.*schema|schema\.py|fastapi.*openapi|oas' . \
--exclude node_modules --exclude dist --exclude build \
| sed -n '1,120p'
echo
echo "== OpenAPI EvalOutputScore definition if generated source exists =="
sed -n '4290,4325p' app/web_ui/api_schema.d.ts 2>/dev/null | cat -n || trueRepository: Kiln-AI/Kiln
Length of output: 8085
Make direction optional for evaluator-create input schemas.
EvalOutputScore.direction has a server-side default, and requests without it are accepted, but the generated CreatedEvaluatorRequest/EvalOutputScore type has EvalOutputScore.direction as required, blocking legacy clients that omit it. Split or override the create-input schema and regenerate api_schema.d.ts with generate_schema.sh.
🤖 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 `@app/web_ui/src/lib/api_schema.d.ts` around lines 6047 - 6051, Update the
evaluator-create input schema so EvalOutputScore.direction is optional while
retaining its server-side default and required direction in response/output
schemas. Split or override the create-specific
CreatedEvaluatorRequest/EvalOutputScore schema as needed, then regenerate
api_schema.d.ts using generate_schema.sh.
Source: Coding guidelines
| direction: ScoreDirection = Field( | ||
| default=ScoreDirection.higher_is_better, | ||
| description="The direction of improvement for this score: 'higher_is_better', 'lower_is_better', or 'informational' (context only, no preferred direction). Rating scales ('five_star', 'pass_fail', 'pass_fail_critical') are higher-is-better by definition, so they allow 'higher_is_better' and 'informational' but not 'lower_is_better'.", | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clarify that lower_is_better is currently reserved, not usable.
The field description and validation error say it is valid for custom scores, but validate_type rejects every custom evaluator score first. State that it is reserved for unsupported custom scores to avoid advertising an API combination that always fails validation.
Proposed clarification
-... 'lower_is_better'. Rating scales ...
+... 'lower_is_better' (reserved for custom scores, which evaluators do not currently support). Rating scales ...Also applies to: 111-125
🤖 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 `@libs/core/kiln_ai/datamodel/eval.py` around lines 90 - 93, Update the
ScoreDirection field description and the related validate_type validation
message to state that lower_is_better is reserved for currently unsupported
custom evaluator scores, rather than presenting it as a usable custom-score
option; keep the existing rejection behavior unchanged.
| @model_validator(mode="after") | ||
| def validate_direction(self) -> Self: | ||
| match self.type: | ||
| case ( |
There was a problem hiding this comment.
This validator allows a score to be both pass_fail_critical and informational at the same time. Those two make opposite promises: critical means "this failure should dominate the outcome", informational means "never use this score to decide anything". A score carrying both is a contradiction, and any consumer reading it has no idea which promise to believe.
Suggest rejecting the combination in this PR rather than later. If it's forbidden now and a real use ever appears, removing the restriction is free and breaks nothing. If it ships allowed and turns out to be a mistake, it can't be tightened later without breaking loads of any file that used the combination in the meantime.
3 participants
What does this PR do?
Eval output scores had no way to declare which way is better. This adds
directiononEvalOutputScore—higher_is_better(default),lower_is_better, orinformational(context only, should never drive decisions) — so API consumers comparing results across runs can interpret score deltas without guessing.five_star,pass_fail,pass_fail_critical) are higher-is-better by definition: they accepthigher_is_betterandinformational, and rejectlower_is_better.lower_is_betteris reserved for custom scores (unbounded numeric metrics), which evaluators do not support yet.higher_is_better, so existing evals load unchanged; requests from clients that predate the field get the default.EvalOutputScore; web client schema regenerated.Related Issues
Companion to #1620 and #1621 (eval-API improvements).
Checklists
🤖 Generated with Claude Code
https://claude.ai/code/session_0135zWTBh8MRPwCXiWvuE597
Generated by Claude Code