mirror of
https://github.com/EleutherAI/lm-evaluation-harness.git
synced 2026-10-11 21:59:53 +00:00
fix(bbh): guard find_match against an all-empty capture tuple (#4125)
* fix(bbh): guard find_match against an all-empty capture tuple
`ExtendedRegexFilter.find_match` narrowed a capture tuple with
`[m for m in match if m][0]`, which raises IndexError when every group in
the tuple is empty. A regex whose groups are all optional and whose matched
text lies outside them -- e.g. `x(foo)?(bar)?` against "x" -- makes
`findall` return `[("", "")]`, and the run aborts instead of falling
through to the filter's fallback.
`lm_eval/filters/extraction.py` already guards the same narrowing in
`MultiChoiceRegexFilter.find_match`; these two files are copies that
predate that fix. Adopt the upstream shape verbatim so an all-empty tuple
is treated as a non-match.
No shipped bbh task can currently reach this: every configured
`regex_pattern` has a single capture group, and the two-group regex
`MapRegexFilter` builds by joining its `regex_pattern_to_value` keys always
fills exactly one group when it matches. The crash is reachable from a
user-supplied filter config, so this is a latent fix -- no task's score
changes, and no `metadata.version` bump is warranted.
Co-Authored-By: Claude Opus 5 <[email protected]>
* chore(bbh): fix pre-existing ruff violations blocking Linters CI
The Linters job runs pre-commit with `--from-ref`/`--to-ref`, which scopes
to changed *files* rather than changed lines. Touching these two files
therefore surfaces every existing violation in them, so the preceding fix
cannot go green without clearing these first. Same situation, and same
remedy, as #3999.
Twelve violations, six mirrored in each file:
- `mutable-argument-default` (x4) -- `convert_dict={}` on `find_match` and
`regex_pattern_to_value={}` on `MapRegexFilter.__init__`. These are not
unrelated cleanup: `find_match` is the function the preceding commit
fixes, and `lm_eval/filters/extraction.py` already carries exactly this
change (`convert_dict: dict[str, str] | None = None` plus an in-body
None check). Adopting it closes the rest of the same divergence.
- `empty-docstring` (x2) -- a vestigial `""" """` on `WordSortFilter`.
Removed rather than filled; `D10` (undocumented-public-*) is ignored
repo-wide, so no docstring is the expected state.
- `zip-without-explicit-strict` (x6) -- made explicit with `strict=False`,
which is the pre-3.10 default and so preserves behaviour exactly.
`extraction.py` uses `strict=True` for the corresponding
`zip(resps, docs)`, but adopting that here would change behaviour on a
length mismatch (silent truncation becomes a raised error). That is a
real question and deserves its own change, not a smuggled one inside a
lint commit.
No behaviour changes. Both files remain byte-identical to each other.
Co-Authored-By: Claude Opus 5 <[email protected]>
* nits
---------
Co-authored-by: Claude Opus 5 <[email protected]>
Co-authored-by: Baber Abbasi <[email protected]>
This commit is contained in:
Vaibhav Hariram
co-authored by
Claude Opus 5
Baber Abbasi
Claude Opus 5
Baber Abbasi
parent
761c18cfd2
commit
e54687eac0
@@ -1,4 +1,3 @@
|
||||
import collections
|
||||
import re
|
||||
import sys
|
||||
import unicodedata
|
||||
@@ -38,12 +37,17 @@ class ExtendedRegexFilter(RegexFilter):
|
||||
st = st.translate(self.punct_tbl)
|
||||
return st
|
||||
|
||||
def find_match(self, regex, resp, convert_dict={}):
|
||||
def find_match(self, regex, resp, convert_dict: dict[str, str] | None = None):
|
||||
if convert_dict is None:
|
||||
convert_dict = {}
|
||||
match = regex.findall(resp)
|
||||
if match:
|
||||
match = match[self.group_select]
|
||||
if isinstance(match, tuple):
|
||||
match = [m for m in match if m][0]
|
||||
non_empty = [m for m in match if m]
|
||||
if not non_empty:
|
||||
return ""
|
||||
match = non_empty[0]
|
||||
match = match.strip()
|
||||
if match and match in convert_dict:
|
||||
match = convert_dict[match]
|
||||
@@ -53,20 +57,23 @@ class ExtendedRegexFilter(RegexFilter):
|
||||
class MapRegexFilter(ExtendedRegexFilter):
|
||||
def __init__(
|
||||
self,
|
||||
regex_pattern_to_value: dict = {},
|
||||
regex_pattern_to_value: dict | None = None,
|
||||
group_select=0,
|
||||
fallback: str = "[invalid]",
|
||||
ignore_case=False,
|
||||
ignore_punctuation=False,
|
||||
regexes_to_ignore=None,
|
||||
) -> None:
|
||||
"""
|
||||
"""Map regex matches to fixed values.
|
||||
|
||||
regex_pattern_to_value: Match the regex pattern and change the result into the value
|
||||
group_select: Selects the (group_select)th match from the findall result. We use the whole regex_patterns, concatenated by |
|
||||
ignore_case: Lowers the case of response before matching with the given regex
|
||||
ignore_punctuation: Remove the punctuation before matching with the given regex
|
||||
regexes_to_ignore: Remove these regexes before matching with the given regex
|
||||
"""
|
||||
if regex_pattern_to_value is None:
|
||||
regex_pattern_to_value = {}
|
||||
super().__init__(
|
||||
"|".join(list(regex_pattern_to_value.keys())),
|
||||
group_select,
|
||||
@@ -138,21 +145,16 @@ class NumberParseRegexFilter(ExtendedRegexFilter):
|
||||
|
||||
|
||||
class WordSortFilter(Filter):
|
||||
""" """
|
||||
|
||||
def apply(self, resps, docs):
|
||||
filtered_resps = []
|
||||
|
||||
for r, doc in zip(resps, docs):
|
||||
for r, doc in zip(resps, docs, strict=True):
|
||||
words = doc["input"].split("List:")[1].strip().split()
|
||||
regex = re.compile("|".join([f"\\b{w}\\b" for w in words]))
|
||||
filtered = []
|
||||
for resp in r:
|
||||
match = regex.findall(resp)
|
||||
match.reverse()
|
||||
ordered_words = reversed(
|
||||
collections.OrderedDict(zip(match, [None] * len(match)))
|
||||
)
|
||||
ordered_words = reversed(dict.fromkeys(reversed(match)))
|
||||
filtered.append(" ".join(ordered_words))
|
||||
filtered_resps.append(filtered)
|
||||
|
||||
@@ -161,7 +163,8 @@ class WordSortFilter(Filter):
|
||||
|
||||
class MultiChoiceRegexFilter(ExtendedRegexFilter):
|
||||
def __init__(self, *args, **kwargs):
|
||||
r"""
|
||||
r"""Extract a multiple-choice answer letter from a response.
|
||||
|
||||
regex_pattern: The basic regex pattern to use. If fails to match, we will use the customized match procedure
|
||||
- step 1 : We parse the choices between ([A-Z])s then try to find these choices in the response.
|
||||
- step 2 : We parse the choice with regex :[\s]*([A-?]), where ? varies by number of choices.
|
||||
@@ -180,7 +183,7 @@ class MultiChoiceRegexFilter(ExtendedRegexFilter):
|
||||
|
||||
filtered_resps = []
|
||||
|
||||
for r, doc in zip(resps, docs):
|
||||
for r, doc in zip(resps, docs, strict=False):
|
||||
fallback_regexes = []
|
||||
choice_to_alpha = {}
|
||||
next_alpha = "A"
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
import collections
|
||||
import re
|
||||
import sys
|
||||
import unicodedata
|
||||
@@ -38,12 +37,17 @@ class ExtendedRegexFilter(RegexFilter):
|
||||
st = st.translate(self.punct_tbl)
|
||||
return st
|
||||
|
||||
def find_match(self, regex, resp, convert_dict={}):
|
||||
def find_match(self, regex, resp, convert_dict: dict[str, str] | None = None):
|
||||
if convert_dict is None:
|
||||
convert_dict = {}
|
||||
match = regex.findall(resp)
|
||||
if match:
|
||||
match = match[self.group_select]
|
||||
if isinstance(match, tuple):
|
||||
match = [m for m in match if m][0]
|
||||
non_empty = [m for m in match if m]
|
||||
if not non_empty:
|
||||
return ""
|
||||
match = non_empty[0]
|
||||
match = match.strip()
|
||||
if match and match in convert_dict:
|
||||
match = convert_dict[match]
|
||||
@@ -53,20 +57,23 @@ class ExtendedRegexFilter(RegexFilter):
|
||||
class MapRegexFilter(ExtendedRegexFilter):
|
||||
def __init__(
|
||||
self,
|
||||
regex_pattern_to_value: dict = {},
|
||||
regex_pattern_to_value: dict | None = None,
|
||||
group_select=0,
|
||||
fallback: str = "[invalid]",
|
||||
ignore_case=False,
|
||||
ignore_punctuation=False,
|
||||
regexes_to_ignore=None,
|
||||
) -> None:
|
||||
"""
|
||||
"""Map regex matches to fixed values.
|
||||
|
||||
regex_pattern_to_value: Match the regex pattern and change the result into the value
|
||||
group_select: Selects the (group_select)th match from the findall result. We use the whole regex_patterns, concatenated by |
|
||||
ignore_case: Lowers the case of response before matching with the given regex
|
||||
ignore_punctuation: Remove the punctuation before matching with the given regex
|
||||
regexes_to_ignore: Remove these regexes before matching with the given regex
|
||||
"""
|
||||
if regex_pattern_to_value is None:
|
||||
regex_pattern_to_value = {}
|
||||
super().__init__(
|
||||
"|".join(list(regex_pattern_to_value.keys())),
|
||||
group_select,
|
||||
@@ -138,21 +145,16 @@ class NumberParseRegexFilter(ExtendedRegexFilter):
|
||||
|
||||
|
||||
class WordSortFilter(Filter):
|
||||
""" """
|
||||
|
||||
def apply(self, resps, docs):
|
||||
filtered_resps = []
|
||||
|
||||
for r, doc in zip(resps, docs):
|
||||
for r, doc in zip(resps, docs, strict=True):
|
||||
words = doc["input"].split("List:")[1].strip().split()
|
||||
regex = re.compile("|".join([f"\\b{w}\\b" for w in words]))
|
||||
filtered = []
|
||||
for resp in r:
|
||||
match = regex.findall(resp)
|
||||
match.reverse()
|
||||
ordered_words = reversed(
|
||||
collections.OrderedDict(zip(match, [None] * len(match)))
|
||||
)
|
||||
ordered_words = reversed(dict.fromkeys(reversed(match)))
|
||||
filtered.append(" ".join(ordered_words))
|
||||
filtered_resps.append(filtered)
|
||||
|
||||
@@ -161,7 +163,8 @@ class WordSortFilter(Filter):
|
||||
|
||||
class MultiChoiceRegexFilter(ExtendedRegexFilter):
|
||||
def __init__(self, *args, **kwargs):
|
||||
r"""
|
||||
r"""Extract a multiple-choice answer letter from a response.
|
||||
|
||||
regex_pattern: The basic regex pattern to use. If fails to match, we will use the customized match procedure
|
||||
- step 1 : We parse the choices between ([A-Z])s then try to find these choices in the response.
|
||||
- step 2 : We parse the choice with regex :[\s]*([A-?]), where ? varies by number of choices.
|
||||
@@ -180,7 +183,7 @@ class MultiChoiceRegexFilter(ExtendedRegexFilter):
|
||||
|
||||
filtered_resps = []
|
||||
|
||||
for r, doc in zip(resps, docs):
|
||||
for r, doc in zip(resps, docs, strict=True):
|
||||
fallback_regexes = []
|
||||
choice_to_alpha = {}
|
||||
next_alpha = "A"
|
||||
|
||||
@@ -0,0 +1,74 @@
|
||||
import re
|
||||
|
||||
import pytest
|
||||
|
||||
from lm_eval.tasks.bbh.cot_zeroshot.utils import (
|
||||
ExtendedRegexFilter as CotZeroshotRegexFilter,
|
||||
MapRegexFilter as CotZeroshotMapFilter,
|
||||
)
|
||||
from lm_eval.tasks.bbh.zeroshot.utils import (
|
||||
ExtendedRegexFilter as ZeroshotRegexFilter,
|
||||
MapRegexFilter as ZeroshotMapFilter,
|
||||
)
|
||||
|
||||
|
||||
# `bbh/zeroshot/utils.py` and `bbh/cot_zeroshot/utils.py` are byte-identical
|
||||
# copies of the same filters, so every case below runs against both.
|
||||
REGEX_FILTERS = [
|
||||
pytest.param(ZeroshotRegexFilter, id="zeroshot"),
|
||||
pytest.param(CotZeroshotRegexFilter, id="cot_zeroshot"),
|
||||
]
|
||||
MAP_FILTERS = [
|
||||
pytest.param(ZeroshotMapFilter, id="zeroshot"),
|
||||
pytest.param(CotZeroshotMapFilter, id="cot_zeroshot"),
|
||||
]
|
||||
|
||||
# Both groups are optional and the matched text ("x") lies outside them, so
|
||||
# `findall` yields a tuple whose every element is the empty string.
|
||||
OPTIONAL_GROUPS = re.compile(r"x(foo)?(bar)?")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("filter_cls", REGEX_FILTERS)
|
||||
def test_all_empty_tuple_returns_empty_string(filter_cls):
|
||||
"""An all-empty capture tuple is a non-match, not an IndexError.
|
||||
|
||||
`[m for m in match if m][0]` indexed an empty list here, so a regex with two
|
||||
optional groups crashed the run instead of falling through to the fallback.
|
||||
Mirrors the guard in lm_eval/filters/extraction.py.
|
||||
"""
|
||||
assert OPTIONAL_GROUPS.findall("x") == [("", "")]
|
||||
assert filter_cls().find_match(OPTIONAL_GROUPS, "x") == ""
|
||||
|
||||
|
||||
@pytest.mark.parametrize("filter_cls", REGEX_FILTERS)
|
||||
def test_partially_empty_tuple_takes_first_non_empty_group(filter_cls):
|
||||
"""The surviving group is still selected when only some groups are empty."""
|
||||
assert OPTIONAL_GROUPS.findall("xbar") == [("", "bar")]
|
||||
assert filter_cls().find_match(OPTIONAL_GROUPS, "xbar") == "bar"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("filter_cls", REGEX_FILTERS)
|
||||
def test_single_group_match_is_unchanged(filter_cls):
|
||||
"""The common path -- one capture group, so `findall` returns plain strings."""
|
||||
regex = re.compile(r"\(([A-Z])\)")
|
||||
assert filter_cls().find_match(regex, "The answer is (B)") == "B"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("filter_cls", REGEX_FILTERS)
|
||||
def test_convert_dict_still_applied(filter_cls):
|
||||
"""A matched value is still mapped through `convert_dict`."""
|
||||
regex = re.compile(r"\(([A-Z])\)")
|
||||
assert filter_cls().find_match(regex, "(B)", {"B": "(B)"}) == "(B)"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("filter_cls", MAP_FILTERS)
|
||||
def test_map_filter_falls_back_instead_of_crashing(filter_cls):
|
||||
"""End to end: an all-empty tuple yields the fallback, not a crash.
|
||||
|
||||
MapRegexFilter joins its configured patterns with "|", which is how a
|
||||
multi-group regex reaches `find_match` in the first place.
|
||||
"""
|
||||
filter_instance = filter_cls(
|
||||
regex_pattern_to_value={r"x(foo)?": "X", r"y(bar)?": "Y"}
|
||||
)
|
||||
assert filter_instance.apply([["x"]], [{}]) == [["[invalid]"]]
|
||||
Reference in New Issue
Block a user