gsm8k: return None when flexible extraction finds no real number - #1
gsm8k: return None when flexible extraction finds no real number#1eeshsaxena wants to merge 1 commit into
Conversation
In extract_solution(method="flexible") the loop reused final_answer as its
loop variable:
for final_answer in reversed(answer):
if final_answer not in invalid_str:
break
When every regex match is an invalid token ("" or ".") the loop never breaks,
so final_answer is left bound to the last iterated (invalid) token instead of
the None it was initialized to. extract_solution("... .", "flexible") returned
".", and compute_score then treated the output as a real-but-wrong answer,
leaking format_score to responses that contain no number.
Iterate over a separate variable and only assign final_answer for a valid
candidate, so it stays None otherwise.
📝 WalkthroughWalkthroughGSM8K flexible extraction now stores the selected valid numeric answer before stopping reverse iteration. New CPU tests cover positive, negative, invalid-only, and digitless inputs, plus zero scoring without a valid answer. ChangesGSM8K answer extraction
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/utils/reward_score/test_gsm8k_on_cpu.py (1)
20-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover a valid number before a trailing invalid token.
Add
("The answer is 42 .", "42"). The current period case has no valid candidate, so it does not verify that reverse iteration skips"."and returns the previous valid number.Proposed test
("The result is just a period .", None), + ("The answer is 42 .", "42"), ("no digits at all", None),🤖 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 `@tests/utils/reward_score/test_gsm8k_on_cpu.py` around lines 20 - 33, Add the missing parametrized case to test_extract_solution_flexible: use input "The answer is 42 ." with expected result "42", preserving the existing invalid-period case and other coverage.
🤖 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 `@verl/utils/reward_score/gsm8k.py`:
- Around line 46-48: Update the candidate-selection loop around final_answer to
accept a candidate only when it represents a valid number, rather than checking
only invalid_str membership; preserve the reversed(answer) search and stop
assigning once the first valid numeric candidate is found, leaving final_answer
unset or at its existing default when none qualifies.
---
Nitpick comments:
In `@tests/utils/reward_score/test_gsm8k_on_cpu.py`:
- Around line 20-33: Add the missing parametrized case to
test_extract_solution_flexible: use input "The answer is 42 ." with expected
result "42", preserving the existing invalid-period case and other coverage.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b381facd-2a20-49b3-b80e-319b1d929971
📒 Files selected for processing (2)
tests/utils/reward_score/test_gsm8k_on_cpu.pyverl/utils/reward_score/gsm8k.py
| for candidate in reversed(answer): | ||
| if candidate not in invalid_str: | ||
| final_answer = candidate |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate candidates as numbers before assigning final_answer.
The regex at Line 38 also matches punctuation-only strings such as "..", ",", and "-.". The current condition rejects only exact "" and ".", so the loop can still return an invalid token when no real number exists. Validate each candidate before breaking.
Proposed fix
- invalid_str = ["", "."]
- # find the last number that is not '.'
+ # find the last parseable number
for candidate in reversed(answer):
- if candidate not in invalid_str:
- final_answer = candidate
- break
+ try:
+ float(candidate.replace(",", ""))
+ except ValueError:
+ continue
+ final_answer = candidate
+ break🤖 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 `@verl/utils/reward_score/gsm8k.py` around lines 46 - 48, Update the
candidate-selection loop around final_answer to accept a candidate only when it
represents a valid number, rather than checking only invalid_str membership;
preserve the reversed(answer) search and stop assigning once the first valid
numeric candidate is found, leaving final_answer unset or at its existing
default when none qualifies.
|
Hi! Gentle nudge on this one whenever you have some bandwidth. It's a small, self-contained fix ( |
1 participant
In
extract_solution(method="flexible")the loop reusesfinal_answeras the loop variable:When every regex match is an invalid token (
""or"."), the loop never breaks, sofinal_answerends up bound to the last iterated token instead of theNoneit was initialised to.compute_scorethen treats that as a real-but-wrong answer and returnsformat_scoreinstead of 0, so a non-zeroformat_scoreleaks to responses that contain no number at all.Fix iterates over a separate variable and only assigns
final_answerfor a valid candidate, leaving itNonewhen nothing valid is found. Valid inputs are unaffected (it still returns the last real number).Added
tests/utils/reward_score/test_gsm8k_on_cpu.pycovering the valid cases plus the no-real-number cases. The stray-period case returns'.'onmainandNonehere.Note: I couldn't run the full verl suite locally (no ray/torch/GPU on my machine), so I verified by exercising
verl/utils/reward_score/gsm8k.pydirectly: with the fix the new cases pass andcompute_score("just a period .", "42", method="flexible", format_score=0.1)returns0instead of0.1. The added test is plain CPU and should run in CI.Summary by CodeRabbit
Bug Fixes
Tests