fix(predict): stop majority() from dropping article-only answers (e.g. multiple-choice "A")#10066
fix(predict): stop majority() from dropping article-only answers (e.g. multiple-choice "A")#10066lntutor wants to merge 1 commit into
Conversation
majority() normalizes each candidate with default_normalize, which runs normalize_text (SQuAD-style: strips the articles a/an/the). An answer that is entirely an article -- e.g. the multiple-choice option 'A', or 'The' -- normalized to '' and, because majority treats a None/empty normalization as 'ignore this completion', was silently excluded from the vote. So majority(['A','A','A','B']) returned 'B'. Fall back to the lowercased/stripped answer when full normalization is empty, so article-only answers still count and group correctly (A and a together), while genuinely empty/whitespace answers still map to None and stay ignored. Non-empty-normalizing answers are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6RtoHuxrDqTUo9Mw9h4Cv
Greptile SummaryThis PR changes
Confidence Score: 4/5This PR has one contained issue that should be fixed before merging. The intended article-only behavior is covered, but the changed fallback broadens behavior to all non-whitespace strings that normalize to empty.
What T-Rex did
Important Files Changed
Reviews (2): Last reviewed commit: "fix(predict): stop majority() from dropp..." | Re-trigger Greptile |
| # solely of an article (e.g. the multiple-choice option "A", or "The") would normalize to | ||
| # an empty string and be discarded from the vote. Fall back to the lowercased, stripped | ||
| # answer so such answers still count, while genuinely empty answers still map to None. | ||
| return normalize_text(s) or (s.strip().lower() or None) |
There was a problem hiding this comment.
Preserve empty normalization semantics
This fallback now counts any non-whitespace string whose SQuAD normalization is empty, not just article-only answers. Because normalize_text also removes punctuation, completions like "!!!" or "..." now become vote keys and can beat a real answer, whereas majority previously ignored empty normalizations. Please restrict the fallback to values that become empty specifically because they are article-only, so punctuation-only non-answers still map to None.
Artifacts
Repro: Python harness exercising default_normalize and majority punctuation voting behavior
- Contains supporting evidence from the run (text/x-python; charset=utf-8).
Repro: command output showing normalized values and punctuation-only majority selection
- Keeps the command output available without making the summary code-heavy.
ErenAta16
left a comment
There was a problem hiding this comment.
The underlying bug is real and I reproduced it, but I think the fallback as written trades one wrong answer for another. Details, with output.
First, confirming the diagnosis. normalize_text is white_space_fix(remove_articles(remove_punc(lower(s)))), so an article-only answer collapses to "" and majority drops it:
3x 'A' vs 1x 'B' main -> 'B' this branch -> 'A'
And the two new tests are not vacuous. Reverting only dspy/predict/aggregation.py to main:
FAILED tests/predict/test_aggregation.py::test_majority_does_not_drop_article_answers
FAILED tests/predict/test_aggregation.py::test_majority_the_answer
2 failed, 6 passed
The problem is that s.strip().lower() is not the same normalizer minus article removal, it is no normalizer. It skips remove_punc and white_space_fix too, and that has two consequences.
1. Junk answers now win votes they used to lose. Anything that is only punctuation used to normalize to "" and be ignored. Now it survives the fallback and counts:
case main this branch
junk '...' x2 vs 'Paris' 'Paris' '...'
junk '-' x2 vs 'Paris' 'Paris' '-'
Two refusals or dangling dashes out of three samples now beat the one real answer. Whitespace-only is still fine because " ".strip() is empty, but "...", "-", "?" and friends are not.
2. It does not actually fix multiple choice once formatting varies. This is the use case the PR is aimed at, and models are not consistent about how they emit the letter:
normalize_text this branch
'A' -> '' -> 'a'
'A.' -> '' -> 'a.'
'(A)' -> '' -> '(a)'
'A)' -> '' -> 'a)'
Four buckets for one answer. So:
case main this branch
mixed MC formatting: A, A., (A) vs B, B 'B' 'B'
A got three of five votes and still loses, because the three A votes are split across three distinct keys. Self-consistency sampling is exactly where you would expect that spread.
What I would do instead. Fall back to the same pipeline with only remove_articles skipped, rather than to raw .strip().lower():
def _normalize_keep_articles(s):
s = unicodedata.normalize("NFD", s)
s = "".join(ch for ch in s.lower() if ch not in set(string.punctuation))
return " ".join(s.split())
def default_normalize(s):
return normalize_text(s) or (_normalize_keep_articles(s) or None)(Cleaner still would be a normalize_text(s, remove_articles=True) keyword in dspy/evaluate/metrics.py so the two paths cannot drift, but either shape works.)
I applied that on top of your branch and re-ran the same table:
case as-shipped punct-stripped fallback
3x 'A' vs 1x 'B' 'A' 'A'
mixed MC formatting: A, A., (A) vs B, B 'B' 'A'
junk '...' x2 vs 'Paris' '...' 'Paris'
junk '-' x2 vs 'Paris' '-' 'Paris'
blank x2 vs 'Paris' 'Paris' 'Paris'
'The Eiffel Tower' x2 vs 'x' ok ok
All four of your intended cases still behave, the junk regressions go away, and the MC formatting variants collapse into one bucket. Your two new tests pass unchanged:
tests/predict/test_aggregation.py tests/evaluate/test_metrics.py
11 passed
(identical to the 11 passed on your branch as-is, so this is a strict improvement, not a trade).
One small thing in your favor that is worth stating: default_normalize is only referenced as the default argument of majority and nowhere else in the tree, so the blast radius is contained to self-consistency voting. Answers that normalize to something non-empty are untouched by either version.
If you take the alternative, test_majority_does_not_drop_article_answers is worth extending with the A / A. / (A) mix, since that is the case the current test cannot distinguish.
What's broken
dspy.majoritynormalizes each candidate answer withdefault_normalize, which runsnormalize_text(SQuAD-style normalization that strips the English articlesa/an/the).majoritytreats aNone/empty normalization as "ignore this completion", so any answer that is entirely an article normalizes to""and is silently dropped from the vote:Multiple-choice QA with options
A/B/C/Dis an extremely common self-consistency use case, so this silently corrupts the majority winner. (normalize_text("A") == "", sodefault_normalize("A") is None.)Fix
Fall back to the lowercased/stripped answer when full normalization yields
"":Article-only answers now count and still group correctly (
"A"and"a"together), while genuinely empty/whitespace answers still map toNoneand stay ignored. Any answer that normalizes to non-empty is unchanged ("The Eiffel Tower"→"eiffel tower"as before).Tests
Adds
test_majority_does_not_drop_article_answersandtest_majority_the_answer. Existingtests/predict/test_aggregation.py(6) andtests/evaluate/test_metrics.py(3) still pass.