fix(metrics): avg@n scores raw text instead of post-processed text, and skips preprocessing - #1335
Open
arthi-arumugam-git wants to merge 1 commit into
Conversation
…ocessing ModelResponse.__getitem__ rebuilt a single-generation response without text_post_processed, so final_text on the slice silently fell back to the raw generation. AvgAtN.compute is the only metric that scores through that slice, so with remove_reasoning_tags enabled (the pipeline default) avg@n was graded against text that still contained the model's reasoning block, while maj@n and pass@k on the identical generations were graded against the stripped answer. AvgAtN.compute also never called self.preprocess, so strip_strings and normalize were dropped even though Metrics.avg_at_n declares strip_strings=True. Neither raises. Both return a plausible, too-low number. AvgAtN.compute now mirrors PassAtK.compute: preprocess the choices, read model_response.final_text, preprocess each prediction. The explicit n-is-None guard keeps that failure loud, matching the wording MajAtN already uses. Adds tests that fail on main and pass here, including an all-wrong case that must keep returning 0.0 so the legitimate zero is preserved.
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
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.
The wrong number
On a reasoning model with the pipeline default
remove_reasoning_tags=True,avg@nreports 0.0 where the correct value is 1.0, whilemaj@nandpass@1on the identical generations both report 1. Nothing raises.A second, independent defect sits in the same three lines.
Metrics.avg_at_nis registered asAvgAtN(strip_strings=True), butcomputenever callsself.preprocess, sostrip_stringsandnormalizeare silently dropped. With ordinary leading and trailing whitespace and no reasoning tags at all:Both errors push the score down, and
avg@nis bounded [0, 1], so the first case is the maximum possible error on the scale.Cause
Both are the same shape: a contract that landed on every metric except one.
1.
ModelResponse.__getitem__dropstext_post_processed(src/lighteval/models/model_output.py:147). It copies nine fields and omits the one thatfinal_textdepends on, sofinal_texton a slice falls back toself.text.AvgAtN.computeis the only metric that scores throughmodel_response[i], which is why it is the only one affected.git log -Lshows__getitem__andtext_post_processedwere introduced in the same commit, d7beacb, "Added post processing (for reasoning tokens) to pipeline (#882)". The new field was simply missed in the new method.2.
AvgAtN.computenever preprocesses (src/lighteval/metrics/metrics_sample.py:1200).MajAtN,PassAtKandGPassAtKall build a preprocessedDocand readmodel_response.final_text.AvgAtNdid neither, while its docstring has claimed "It applies normalisation (if needed) to model prediction and gold" the whole time.This survived because
tests/unit/metrics/test_cases/avg_at_k.jsonnever executed: itsmetric_classdoes not match the registered enum member, so it falls into the silent-skip branch. That is already reported as #1304 / #1305, which is not mine.The change
AvgAtN.computenow reads exactly likePassAtK.compute: preprocess the choices, readmodel_response.final_text, preprocess each prediction.The explicit
n is Noneguard is deliberate. Moving fromrange(self.n)tofinal_text[: self.n]would have converted today's loudTypeErrorinto a silent "average whatever you were given", which is the same class of bug being fixed here. The wording matches whatMajAtNalready raises.Tests
tests/unit/metrics/test_avg_at_n_final_text.py, using only symbols that exist on main.On pristine main: 5 failed, 1 passed.
With this change: 6 passed.
The one that passes on main is the all-wrong case, which must keep returning
0.0. That is the legitimate zero, and the point of including it is to show this change does not turn every zero into a non-zero.Wider metric suite (
tests/unit/metrics/test_automated_metrics_pytest.pyandtests/test_unit_base_metrics.py), run before and after on the same machine:The failing set is byte-identical between the two runs, and none of the 12 touch
AvgAtNorModelResponse. They are environment-only on my Windows box and reproduce on unmodified main. There are no errored tests hiding behind the passed count.ruff checkandruff format --checkboth pass on all three files.Scope, and what I deliberately did not touch
MajAtN,PassAtKorGPassAtK. All three already readfinal_textand already preprocess. Their outputs are unchanged.MajAtNdoc.get_golds()behaviour atmetrics_sample.py:1244. I ran into it, but it is already open as Fix MajAtN IndexError when the gold is not the first choice #1274 and belongs to that author.SamplingMetric.__init__at1122, already open as Fix string-name normalization in SamplingMetric (inspect.getmembers returns a list, not a dict) #1292.avg_at_k.json/maj_at_k.jsonfixture naming, already open as avg_at_k / avg_at_k_math / maj_at_k metric regression fixtures silently no-op (metric_class name doesn't match registered Metrics enum members) #1304 / fix metric_class names in avg_at_n / avg_at_n_math / maj_at_n test cases #1305. I checked this change against those fixtures with fix metric_class names in avg_at_n / avg_at_n_math / maj_at_n test cases #1305's corrections applied and the expected values 0.5, 0.0 and 0.33 all reproduce, so the two do not collide.reasonings, which__getitem__also drops. No metric reads it today, so it is a latent schema inconsistency rather than a wrong number. Worth knowing about, but I did not want to widen this diff.remove_reasoning_tags, thefinal_textproperty, any task definition, or any corpus-level aggregation.One claim I am not making
Metrics.avg_at_n_mathis used byaime24,aime24_avg,aime25andaime25_avg, and it goes throughMultilingualExtractiveMatchMetric, which is far more robust to leading reasoning text. I built two realistic reasoning traces, one with an abandoned\boxed{}candidate and one with a self-correction, and could not make the extractor diverge; both returned 1.0 with and without the reasoning stripped.So I am claiming the confirmed wrong number on
Metrics.avg_at_nand on any customAvgAtNwith a string-comparison scoring function. On the AIME tasks I have confirmed only thatavg@nis fed different text from every other metric on the same run, not that the published AIME number moves. I would rather say that plainly than overstate it.