feat(intake): count missing scores as 0 in evaluator rollups [ASE-616]#772
Merged
Conversation
…[ASE-616] Evaluator score rollups averaged only over scored runs (the score SQL INNER JOINs evaluator_results), so errored/unscored runs were dropped from the denominator — inflating score means and making them non-comparable across experiments. Switch scores to a fixed denominator: - A run that ERRORED with no score counts as 0 (a real failed attempt). - A run that SUCCEEDED with no score is excluded (the evaluator wasn't run on it; counting it 0 would wrongly penalize partial-coverage evaluators). - An errored run that DID record a score keeps it. Implemented in _score_rollups_sql via the session's root_status: scored sessions keep their value; errored-unscored sessions contribute 0 (UNION ALL + LEFT ANTI JOIN against the scored set); successful-unscored sessions are left out. Cost and latency (_metric_rollups_sql) are unchanged — a missing value there is unmeasured. Studio: the existing aggregate-metric tooltip on score cells now notes "Errored runs with no score count as 0." No new UI elements. Validated end-to-end against ClickHouse: new integration test seeds a scored, an errored-unscored, and a succeeded-unscored run — mean = avg(1.0, 0) = 0.5 over 2 test cases (the succeeded-unscored one excluded). Existing rollup tests unchanged. Signed-off-by: shanaiabuggy <[email protected]>
Contributor
|
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:
📝 WalkthroughWalkthroughEvaluation rollups now use all test-case sessions as the denominator and treat missing evaluator scores as zero. Tests cover scored, errored, and unscored runs. Evaluator tooltips explain this behavior. ChangesEvaluation score rollups
Sequence Diagram(s)sequenceDiagram
participant Sessions
participant RollupSQL
participant Studio
Sessions->>RollupSQL: Provide session and evaluator scores
RollupSQL->>RollupSQL: Build fixed denominators and zero-fill missing scores
RollupSQL-->>Studio: Return reward aggregates
Studio->>Studio: Display missing-as-zero tooltip text
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/packages/studio/src/components/dataViews/ExperimentGroupDataView/MeanValueTooltipCell.tsx (1)
31-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the new prop readonly.
Change it to
readonly countsMissingAsZero?: boolean.As per coding guidelines, “Use
readonlyfor immutable properties in TypeScript interfaces and types.”🤖 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 `@web/packages/studio/src/components/dataViews/ExperimentGroupDataView/MeanValueTooltipCell.tsx` around lines 31 - 36, Update the countsMissingAsZero property in the MeanValueTooltipCellProps interface to be readonly while preserving its optional boolean type and existing documentation.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 `@services/intake/tests/integration/spans/test_experiment_rollups.py`:
- Around line 326-330: Add an errored session with a non-null score to the seeds
in the experiment rollup integration test, then extend the expected assertions
to verify that its recorded score is retained rather than replaced or
supplemented with zero. Keep the existing unscored errored and successful
unscored cases unchanged.
---
Nitpick comments:
In
`@web/packages/studio/src/components/dataViews/ExperimentGroupDataView/MeanValueTooltipCell.tsx`:
- Around line 31-36: Update the countsMissingAsZero property in the
MeanValueTooltipCellProps interface to be readonly while preserving its optional
boolean type and existing documentation.
🪄 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: Enterprise
Run ID: 92f2b99f-8664-4f2d-b9f9-8a87a11b1f59
📒 Files selected for processing (4)
services/intake/src/nmp/intake/spans/evaluation_rollup_repository.pyservices/intake/tests/integration/spans/test_experiment_rollups.pyweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/MeanValueTooltipCell.tsxweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsx
Contributor
|
shanaiabuggy
marked this pull request as draft
July 17, 2026 22:18
…t in the rollup Addresses CodeRabbit on #772: the errored-missing-scores test only covered errored+unscored (->0) and successful+unscored (excluded); a regression that unioned 0 for every errored session would have slipped through. Adds an errored+scored seed and asserts its 0.8 is retained (mean 0.6). Also marks the MeanValueTooltipCell countsMissingAsZero prop readonly per the coding guideline. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
shanaiabuggy
marked this pull request as ready for review
July 20, 2026 17:05
Contributor
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 `@services/intake/src/nmp/intake/spans/evaluation_rollup_repository.py`:
- Around line 221-251: Update the test_case_scores calculation and its
supporting session-count logic so succeeded sessions without an evaluator score
are excluded from the denominator, while errored (session, evaluator) pairs
without scores contribute explicit zero rows. Preserve root_status through the
relevant CTEs, distinguish errored from succeeded sessions when generating
missing-score rows, and ensure the per-test-case evaluator averages use only
scored sessions plus those errored zero rows.
🪄 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: Enterprise
Run ID: c818cb03-e005-4a47-b3cd-eb3bf3805b7c
📒 Files selected for processing (3)
services/intake/src/nmp/intake/spans/evaluation_rollup_repository.pyservices/intake/tests/integration/spans/test_experiment_rollups.pyweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/MeanValueTooltipCell.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- services/intake/tests/integration/spans/test_experiment_rollups.py
- web/packages/studio/src/components/dataViews/ExperimentGroupDataView/MeanValueTooltipCell.tsx
…ator_results Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
shanaiabuggy
changed the title
Signed-off-by: shanaiabuggy <[email protected]>
BrianNewsom
approved these changes
Jul 21, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/intake/tests/test_experiment_rollup_repository.py (1)
181-194: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the denominator source and all score-join keys.
Line 182 only checks
count(DISTINCT session_id), so switching to scored rows would still pass while excluding unscored sessions. Lines 193-194 only check join keywords; removingtest_case_keyorevaluator_namepredicates could mix scores across groups without failing this test. AssertFROM scoped_sessionsand everyONpredicate, or execute the fully composed SQL.🤖 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 `@services/intake/tests/test_experiment_rollup_repository.py` around lines 181 - 194, Strengthen the assertions in the test covering _test_case_sessions_cte() and _test_case_scores_cte(): verify the denominator query reads from scoped_sessions, and assert every score JOIN key, including test_case_key and evaluator_name alongside the existing evaluation_id relationship. Keep the current fixed-denominator and zero-fill expectations while preventing joins that mix sessions or scores across groups.
🤖 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 `@services/intake/tests/test_experiment_rollup_repository.py`:
- Around line 181-194: Strengthen the assertions in the test covering
_test_case_sessions_cte() and _test_case_scores_cte(): verify the denominator
query reads from scoped_sessions, and assert every score JOIN key, including
test_case_key and evaluator_name alongside the existing evaluation_id
relationship. Keep the current fixed-denominator and zero-fill expectations
while preventing joins that mix sessions or scores across groups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 97a9a15c-2685-459f-aa4a-94456341f8e4
📒 Files selected for processing (2)
services/intake/src/nmp/intake/spans/evaluation_rollup_repository.pyservices/intake/tests/test_experiment_rollup_repository.py
🚧 Files skipped from review as they are similar to previous changes (1)
- services/intake/src/nmp/intake/spans/evaluation_rollup_repository.py
Signed-off-by: shanaiabuggy <[email protected]>
shanaiabuggy
enabled auto-merge
July 21, 2026 16:17
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.
Closes ASE-616
Why
Evaluator score rollups (the aggregate means in the Experiments table) averaged only over scored runs — the score SQL INNER JOINs
evaluator_results, so a run with no score was dropped from the denominator. That inflates score means and makes them non-comparable across experiments (the denominator varies per metric). Our standard — and Harbor's / switchyard-bench's — is a fixed denominator with no-score counted as 0.Cost/latency are different: a missing value there is unmeasured, not zero, so they keep excluding missing. They already compute on a separate SQL path, so this change stays on the score path.
Change
Backend —
evaluation_rollup_repository.py:_score_rollups_sqlnow uses a fixed denominator — a test case's recorded scores summed over its total session count, so any run with no score counts as 0._metric_rollups_sql(cost/latency) is unchanged.Studio — the existing score-cell tooltip now appends "Runs with no score count as 0." No new indicators.
Validation
Rollup integration tests pass against real ClickHouse (testcontainers). New test seeds a scored run, an unscored run, and an errored-but-scored run across four test cases →
mean = avg(1.0, 0, 0, 0.8) = 0.45. Existing rollup tests unchanged; ruff + ty clean; Studio typecheck + eslint clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
UI Improvements
Tests