Skip to content

Fix string-name normalization in SamplingMetric (inspect.getmembers returns a list, not a dict)#1292

Open
teddytennant wants to merge 1 commit into
huggingface:mainfrom
teddytennant:fix-sampling-metric-normalize-str
Open

Fix string-name normalization in SamplingMetric (inspect.getmembers returns a list, not a dict)#1292
teddytennant wants to merge 1 commit into
huggingface:mainfrom
teddytennant:fix-sampling-metric-normalize-str

Conversation

@teddytennant

Copy link
Copy Markdown

What

Wrap the inspect.getmembers(...) result in dict() inside SamplingMetric.__init__ so that a normalize passed as a string is correctly resolved to the named function in lighteval.metrics.normalizations.

Why

SamplingMetric.__init__ accepts normalize: Callable | str | None. When a string is passed, it is resolved via:

allowed_normalizations = inspect.getmembers(
    lighteval.metrics.normalizations, inspect.isfunction
)  # -> {name: fn}
if normalize in allowed_normalizations:
    self.normalize = allowed_normalizations[normalize]
else:
    raise ValueError(f"Unknown normalization function: {normalize}")

But inspect.getmembers returns a list of (name, fn) tuples, not a dict. So:

  • normalize in allowed_normalizations is a membership test of a plain string against a list of tuples — always False.
  • allowed_normalizations[normalize] would string-index a list — TypeError.

Net effect: passing any string for normalize always raises ValueError: Unknown normalization function: <name>, even for valid names such as "gsm8k_normalizer" or "math_normalizer". This affects the base class shared by AvgAtN, MajAtN, PassAtK, and GPassAtK. It stays latent because all built-in metrics pass callables / sample_scoring_function, never a string — only custom-metric users who pass a normalizer by name hit it.

Wrapping the result in dict() makes in check function names and indexing do a key lookup. Unknown names still fall through to the existing ValueError.

Testing

Added tests/test_unit_base_metrics.py::TestBaseMetrics::test_sampling_metric_normalize_by_name, a pure-CPU regression test that:

  • constructs AvgAtN(n=1, normalize="gsm8k_normalizer") and asserts it resolves to normalizations.gsm8k_normalizer,
  • asserts the resolved normalizer is applied (preprocess("The answer is #### 18") == "18"),
  • asserts an unknown name still raises ValueError.

The test fails before this change (ValueError: Unknown normalization function: gsm8k_normalizer) and passes after.

$ pytest tests/test_unit_base_metrics.py -k normalize_by_name -q
1 passed, 12 deselected

`inspect.getmembers` returns a list of (name, fn) tuples, but the code
treated it as a dict: the `normalize in allowed_normalizations` membership
test never matched a plain string and `allowed_normalizations[normalize]`
would raise TypeError. As a result, passing any string for `normalize`
(a supported public-API form, per the `Callable | str | None` type hint)
always raised `ValueError: Unknown normalization function: <name>`, even
for valid names like "gsm8k_normalizer".

Wrap the getmembers result in `dict()` so `in` checks function names and
indexing does a key lookup. Unknown names still fall through to the
existing ValueError.
Copilot AI review requested due to automatic review settings July 8, 2026 20:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the root cause directly: inspect.getmembers() returns a list[tuple[str, Callable]], not a dict, so normalize in allowed_normalizations was checking a string against a list of tuples (always False) and allowed_normalizations[normalize] would have raised TypeError on a list. Wrapping it in dict() is the correct fix, and it means every string-based normalize was silently broken before this, not just some edge case.

The regression test is solid: it checks the resolved function is the actual gsm8k_normalizer (not just that no exception is raised), applies it to confirm the normalization itself works end to end, and keeps the unknown-name path raising ValueError. Looks correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants