fix(g-eval): locate the score token by content instead of a fixed offset - #8152
fix(g-eval): locate the score token by content instead of a fixed offset#8152feiiiiii5 wants to merge 3 commits into
Conversation
The logprob scorer assumed the score digit is always the fourth token
and that scores are single digits. Both assumptions break in the wild:
a two-digit score ("score": 10) tokenizes across two tokens, so only
the first digit is averaged and a confident 10 scores ~0.1; and
tokenizer variants that fold the whitespace after the colon into the
score token (" 0") make every candidate non-decimal, so a perfectly
parseable response raises MetricComputationError instead.
Locate the score digits by reconstructing the token stream and
matching the "score" key, strip candidate tokens before the decimal
check, and for a two-token span combine the two positions' candidates
("1" + "0" -> 10) while counting a first-position candidate only
when it covers the whole span. Falls back to the legacy fixed offset
when the key cannot be found in the reconstructed text.
Related: comet-ml#8134 (defect 3 of 4)
github-actions
Bot
added
python
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| if entry_indices is None: | ||
| entry_indices = [3] |
There was a problem hiding this comment.
Silent incorrect scores on locator failure
Score localization falls back to index 3 without diagnostics, so malformed model output can select an unrelated token and produce incorrect metrics that are hard to diagnose — should we log the reconstructed content before using the legacy offset, or raise an explicit parsing error when localization fails?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/evaluation/metrics/llm_judges/g_eval/parser.py` around lines
71-72, update `parse_litellm_model_output` so failure of `_locate_score_entries` is not
silently hidden by falling back to index `3`. Emit a warning containing the
reconstructed token content and the fallback decision before using the legacy offset, or
raise an explicit parsing error if that fallback is unsafe; centralize this handling in
a helper if appropriate and ensure it remains covered by the existing error path.
There was a problem hiding this comment.
Addressed in ecb2385 — before falling back to the fixed offset we now log the reconstructed response at debug level, so a malformed response that lands on an unrelated token is diagnosable.
| _SCORE_KEY_RE = re.compile(r'"score"\s*:\s*(\d+)') | ||
|
|
||
|
|
||
| def _locate_score_entries(entries: list) -> list[int] | None: | ||
| """Find the entry indices whose tokens carry the score digits. |
There was a problem hiding this comment.
Rationale text can override actual score
The score locator scans the entire reconstructed response and takes the first "score": <digits> occurrence, so rationale text can supply _locate_score_entries's indices and cause _weighted_score_sums to compute from rationale tokens instead of the actual score — should we parse the structured object and target its top-level score field?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/evaluation/metrics/llm_judges/g_eval/parser.py` around lines
100-104, fix `_SCORE_KEY_RE` and `_locate_score_entries` so a `
There was a problem hiding this comment.
Addressed in 137faf9, with one correction to the premise: inside valid JSON an in-string echo can never match the key regex, because the quotes would be escaped (\"score\": 5 defeats \"score\"), so every matchable occurrence IS a key position. The real residual vector is a duplicated "score" key — json.loads resolves duplicates to the LAST one (which is also what the no-logprob text path reads via dict_content["score"]), while the locator took the first. The locator now takes the last match so both paths agree.
There was a problem hiding this comment.
Thanks for the correction—taking the last duplicate key to match json.loads is the right alignment. I’ll save this to memory once the PR is merged.
| """ | ||
| token_texts = [str(_to_dict(entry).get("token", "")) for entry in entries] | ||
| full_text = "".join(token_texts) | ||
| match = _SCORE_KEY_RE.search(full_text) |
There was a problem hiding this comment.
Reason text overrides score property
_SCORE_KEY_RE.search(full_text) finds the first raw "score": <digits> text, so an escaped score-like fragment in a preceding reason string can replace the parsed score — should we locate the numeric span from the parsed score property/token structure instead?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/evaluation/metrics/llm_judges/g_eval/parser.py` around lines
113-113, fix `_locate_score_entries` so it does not use the first raw-text regex match,
which can select an embedded `"score": ...` fragment inside the `reason` string. Locate
the numeric span belonging to the actual parsed JSON `score` property, while preserving
the correct token indices for weighted log-probability calculation. Add a regression
test where `reason` appears before `score` and contains an embedded score-like fragment.
There was a problem hiding this comment.
Same root as the previous finding, addressed in 137faf9: for valid JSON the reason text cannot contain a matchable "score": n (its quotes are escaped), so the only matchable occurrences are key positions; duplicate keys now resolve to the last match, matching json.loads and the text path.
There was a problem hiding this comment.
Thanks for clarifying—valid JSON escaping and the updated last-match behavior address this concern. I’ll save this to memory once the PR is merged.
| first_candidates = _decimal_candidates(entries[entry_indices[0]]) | ||
| second_candidates = _decimal_candidates(entries[entry_indices[1]]) | ||
|
|
||
| for token_a, prob_a in first_candidates: | ||
| if token_a == digits and 0 <= int(token_a) <= 10: | ||
| # one token covering the whole span (alternative tokenization) | ||
| linear_probs_sum += prob_a | ||
| weighted_score_sum += prob_a * int(token_a) | ||
| for token_b, prob_b in second_candidates: | ||
| combined = token_a + token_b | ||
| if not combined.isdecimal() or not 0 <= int(combined) <= 10: | ||
| continue |
There was a problem hiding this comment.
Split scores mix incompatible probabilities
second_candidates is computed once for the observed second token but reused across alternate token_a prefixes, so multiplying prob_a * prob_b creates joint candidates whose second-token probabilities were never measured for that prefix and changes the weighted score. Should we keep only candidates with a same-prefix distribution, or document this as an explicit approximation rather than candidate-space joint mass?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/evaluation/metrics/llm_judges/g_eval/parser.py` around lines
168-179, refactor `_weighted_score_sums` so it does not cross-product every
`first_candidates` entry with `second_candidates` from only the observed second-token
prefix. Only combine probabilities when the second-token distribution was measured for
the corresponding first-token candidate; otherwise fall back to the observed score path
or use an explicitly documented approximation that is not presented as true joint
candidate mass. Add or update tests to ensure alternate first-token candidates cannot
invent unsupported probability mass.
There was a problem hiding this comment.
True limitation, kept as documented: per-position top_logprobs carry no cross-position conditionals, so P(a)·P(b|observed_a) is the only combination the data supports; linear_probs_sum normalization bounds its effect, and the combination rule (first candidate counted only when it covers the whole span) is stated in the PR description. This PR fixes the score-token location and deliberately keeps the weighted-average design unchanged — happy to revisit the weighting in a follow-up if you'd like.
There was a problem hiding this comment.
Thanks for clarifying the data constraint and intentional scope. I’ll save this context to memory once the PR is merged.
| def test_two_digit_score_split_across_tokens_scores_near_one(): | ||
| # {"score":10} tokenizes as {" score ": "1" "0" ...}; the score is 10 but | ||
| # position 3 only sees the first digit "1", so the weighted average of | ||
| # digit candidates lands near 0.1 instead of 1.0. | ||
| entries = [ | ||
| _entry('{"', -0.01), | ||
| _entry("score", -0.01), | ||
| _entry('":', -0.01), | ||
| _entry( | ||
| "1", |
There was a problem hiding this comment.
test_two_digit_score_split_across_tokens_scores_near_one always puts the score at entries[3], so a regression that reads the legacy offset instead of using _locate_score_entries would still pass — should we prepend a token that shifts the score and assert the located numeric value?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/evaluation/metrics/llm_judges/g_eval/test_parser.py` around
lines 48-57, extend or update `test_two_digit_score_split_across_tokens_scores_near_one`
so a valid prefix token appears before the score, moving the score away from
`entries[3]`. Make the shifted fixture leave `entries[3]` as a non-score token whose
fallback interpretation would produce a materially different value, then assert that
`_locate_score_entries` returns the expected two-digit score near 1.0 — ensuring the
test fails if score extraction still reads only entry 3.
There was a problem hiding this comment.
Kept: the stub's second-token candidates mirror the first-token distribution of the legacy single-token path, which is what makes the two-digit assertion (≈0.09 before the fix) comparable against the pre-fix behavior it documents.
There was a problem hiding this comment.
Thanks for clarifying—the mirrored distribution is intentional to preserve comparability with the legacy path. I’ll save this context to memory once the PR is merged.
There was a problem hiding this comment.
Correction to my previous reply (which answered a different question): on the offset-discrimination point itself, this test does fail a legacy-offset regression — the legacy path reads only entries[3] (the "1" token), whose candidates average to ~0.099, and the assertion requires >0.9 (that exact value is the pre-fix failure documented in the PR). The shifted-position variant is additionally covered by the two newer tests added in 137faf9, where the located digits are not at entries[3]: the duplicate-key fixture locates the score at the second key and the escaped-echo fixture locates it after a long reason. So the suite pins the locator both by value and by position.
There was a problem hiding this comment.
Thanks for the correction—I understand that the fixture fails the legacy entries[3] path and the newer cases cover position shifts. I’ll save this to memory once the PR is merged.
|
Hi @feiiiiii5 Thanks for the contribution! When you have a chance, could you take a look at the automated review comments from Baz? For each comment, please either address it with a code change or reply with a brief explanation if you believe the current implementation is the right approach. Thanks! |
…loads The token-stream locator matched the first "score": <digits> in the reconstructed text, but json.loads (and therefore the no-logprob text path) resolves duplicate keys to the LAST one. A model that repeats the score key would be scored from the stale first key in the logprob path while the text path reports the final value. Take the last match so both paths agree.
When the score key cannot be located, the legacy fixed offset is used with no diagnostics; a malformed response that lands on an unrelated token is then hard to diagnose. Log the reconstructed text at debug level before falling back.
|
Thanks @andrescrz — all Baz comments on this PR are now either addressed or answered inline: locator-failure diagnostics in ecb2385, duplicate-"score"-key resolution (last-wins, mirroring json.loads and the text path) in 137faf9 with regression tests, and the two-token probability note answered inline as a documented approximation of the existing weighted-average design. |
| result = parser.parse_litellm_model_output( | ||
| _response(content, entries), | ||
| name="g_eval", | ||
| log_probs_supported=True, | ||
| ) |
There was a problem hiding this comment.
Missing provider-boundary regression coverage
parse_litellm_model_output is covered only with hand-built token dictionaries, while the existing integration test uses LangChain and generic score bounds/type/reason checks, so provider-shaped responses through the public GEval path can pass with incorrect bounded results. Could we add a GEval integration test with a provider-shaped raw LiteLLM response/token stream that exercises the direct parser branch and asserts split-score and folded-whitespace behavior?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/evaluation/metrics/llm_judges/g_eval/test_parser.py` around
lines 176-180, extend the regression coverage to exercise the public `GEval` metric path
with a provider-shaped raw LiteLLM response and token stream instead of only calling
`parse_litellm_model_output` with hand-built dictionaries. Add precise assertions that
the split-score case selects the final score and that folded whitespace is handled
correctly, so an incorrectly bounded result cannot pass. Keep the existing direct parser
tests as focused unit coverage, but add the integration setup through the actual LiteLLM
branch rather than LangChain.
| name="g_eval", | ||
| log_probs_supported=True, | ||
| ) | ||
| assert 0.55 < result.value < 0.68, f"scored from the stale key: {result.value}" |
There was a problem hiding this comment.
Incorrect score calculation passes tests
The duplicate-key regression test accepts any result.value between 0.55 and 0.68, so an incorrect candidate distribution can still pass — should we assert the calculated weighted score for the second (7) value with an appropriate floating-point tolerance?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/evaluation/metrics/llm_judges/g_eval/test_parser.py` around
lines 181-181, strengthen `test_duplicate_score_keys_last_wins_like_the_text_path` so it
verifies the exact weighted score for the final `"score": 7` token and its `7`/`1`
logprob candidates. Calculate the expected normalized weighted result from those
candidates (approximately 0.6219) and replace the broad range assertion with a
`pytest.approx` assertion using an appropriate tolerance, ensuring an incorrect
candidate selection cannot pass.
|
|
||
| Reconstructs the decoded text from the token stream and locates the | ||
| digits after `"score":`; returns the one or two indices covering them, | ||
| or None when the key cannot be found in the reconstructed text (the | ||
| caller then falls back to the legacy fixed offset). | ||
|
|
||
| Uses the LAST match so duplicate `"score"` keys resolve the same way | ||
| json.loads does — to the final one, which is also what the no-logprob | ||
| text path reads via `dict_content["score"]`. | ||
| """ |
There was a problem hiding this comment.
Update stale score-token documentation
The parse_litellm_model_output docstring still says the score is always the fourth token, so it misdocuments the content-based "score" lookup and index-3 compatibility fallback — should we update it to describe the intent rather than the mechanics?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/evaluation/metrics/llm_judges/g_eval/parser.py around lines 67-72,
update the parse_litellm_model_output docstring to document content-based score
localization, including that the last matching "score" key is used and token index 3 is
only a compatibility fallback. Align the wording with the logic around lines 111-120 and
describe the parser’s intent rather than its old fixed-token mechanics.
2 participants
Related: #8134 (defect 3 of 4 —
Resolvesintentionally avoided so the four-defect issue stays open for defects 1 and 2)Summary
"score": 10) tokenizes across two tokens, so only the first digit is averaged: a confident 10 parses as ~0.099 (deterministic stub in tests). And when a tokenizer folds the whitespace after the colon into the score token (" 0"), every candidate fails theisdecimal()check and a perfectly parseable response raisesMetricComputationError"score"key; strip candidate tokens before the decimal check; for a two-token span combine both positions' candidates ("1" + "0"→ 10), counting a first-position candidate only when it covers the whole spanIn scope
sdks/python/src/opik/evaluation/metrics/llm_judges/g_eval/parser.py: rewrittenparse_litellm_model_outputlogprob block + two helpers (_locate_score_entries,_weighted_score_sums)sdks/python/tests/unit/evaluation/metrics/llm_judges/g_eval/test_parser.py: two defect-reproduction stubs + two parity controlsOut of scope
Validation
PYTHONPATH=sdks/python/src python -m pytest sdks/python/tests/unit/evaluation/metrics/llm_judges/g_eval/ -q| sample size:N=4| key metrics: pre-fix 2 failed (score 10 parsed as 0.09916469633471672;MetricComputationErroron" 0") → post-fix4 passed| result:passsdks/python/tests/unit/evaluation/shows the same failure count on the clean tree (91 failed / 927 passed) and with this branch (91 failed / 930 passed) — the 91 are pre-existing environment failures, unrelated to this diffruff@0.14.14(pre-commit pin)check+format --checkon both files | result:passRisk / Compatibility
strip()is a no-op on space-free candidatesType of Change