[NA] [SDK] fix: correct NLTK usage in METEOR and chrF metrics - #7925
[NA] [SDK] fix: correct NLTK usage in METEOR and chrF metrics#7925DivyaNarahari97 wants to merge 2 commits into
Conversation
METEOR's default backend passed raw strings to nltk meteor_score, which requires pre-tokenized input, so every call raised TypeError and the metric was unusable. chrF passed the whole reference list to sentence_chrf, which takes a single reference, so NLTK joined the references into one string and an exact match against one of them scored 0.42 instead of 1.0. - METEOR: tokenize references and hypothesis in the default NLTK scorer, keeping the public `meteor_fn` contract string-based. - chrF: score each reference separately and keep the best match. - Add regression tests exercising the real NLTK backend; the existing tests only covered the dependency-injected path, which is why both bugs were invisible to CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
python
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| except TypeError: | ||
| # Older NLTK versions expose the helper with fewer keyword arguments. | ||
| return float(nltk_chrf_score.sentence_chrf(references, candidate)) | ||
| return float(nltk_chrf_score.sentence_chrf(reference, candidate)) |
There was a problem hiding this comment.
_score_single retries sentence_chrf(reference, candidate) with all defaults on any TypeError, so configured max_len/beta such as ChrF(beta=2, char_order=1) are discarded and unrelated TypeErrors from bad input or implementation bugs are masked. Should we detect only the unsupported ignore_whitespace case, retry without that argument while preserving other options, and let other TypeErrors propagate?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/evaluation/metrics/heuristics/chrf.py around lines 99-101, refactor
the `_score_single` compatibility fallback so it does not blindly catch every
`TypeError` from `sentence_chrf` and does not drop all configured options. Detect
whether the installed NLTK callable lacks support for `ignore_whitespace` specifically
(via signature inspection or narrowly validated exception details), then retry with only
that unsupported option removed while still passing through `max_len` and `beta`; let
any other `TypeError` (from bad input or implementation errors) propagate with its
original diagnostics. Add regression tests covering non-default `beta` and `char_order`
against the older NLTK signature.
| # NLTK's meteor_score expects pre-tokenized input: an iterable of | ||
| # token lists for the references and a token list for the | ||
| # hypothesis. Handing it raw strings raises TypeError, so tokenize | ||
| # here (whitespace split, matching BLEU/GLEU) while keeping the | ||
| # public `meteor_fn` contract string-based. |
There was a problem hiding this comment.
Documented callback shape causes METEOR crashes
The meteor_fn docstring advertises NLTK’s tokenized meteor_score interface even though the adapter passes raw (Sequence[str], str) inputs, so callers using NLTK’s function directly get TypeError for every non-empty score — should we document meteor_fn as (Sequence[str], str) -> float and distinguish it from the tokenized adapter, or normalize custom callbacks consistently?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/evaluation/metrics/heuristics/meteor.py` around lines 85-89,
update the `METEOR.__init__` documentation and `_scorer` explanation to explicitly
define `meteor_fn` as `(Sequence[str], str) -> float` receiving raw string references
and hypothesis text. Clarify that the built-in adapter tokenizes these strings before
calling NLTK’s `meteor_score`, and that NLTK’s tokenized callback should not be
passed directly unless wrapped. Preserve the existing string-based `score()` and
injected callback contract.
| tokenized_references = [reference.split() for reference in references] | ||
| tokenized_hypothesis = hypothesis.split() | ||
| try: | ||
| return float( | ||
| nltk_meteor_score.meteor_score( | ||
| references, hypothesis, alpha=alpha, beta=beta, gamma=gamma | ||
| tokenized_references, | ||
| tokenized_hypothesis, |
There was a problem hiding this comment.
Older NLTK installs make METEOR fail
The adapter always passes list[list[str]] references and a list[str] hypothesis, but NLTK 3.6.4 and earlier expect list[str] references and a raw str hypothesis, so METEOR.score() raises a type error when those unconstrained versions are installed — should we require NLTK >=3.6.5 or branch for the legacy API?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/evaluation/metrics/heuristics/meteor.py` around lines 90-96,
update the nested `_scorer` adapter in `METEOR.__init__` because unconditional
tokenization only works with NLTK 3.6.5 and newer, while older installable versions
expect string references and a raw-string hypothesis. Prefer adding and enforcing an
NLTK minimum version wherever optional dependency requirements are declared, or, if
legacy support is required, branch the call based on the installed NLTK version and pass
the legacy argument shapes accordingly. Add or update tests to verify the
supported-version behavior.
| def _skip_without_wordnet() -> None: | ||
| """Skip when the optional `nltk` dependency or its WordNet corpus is missing.""" | ||
| pytest.importorskip("nltk") | ||
| from nltk.corpus import wordnet | ||
|
|
||
| try: | ||
| wordnet.ensure_loaded() | ||
| except LookupError: | ||
| pytest.skip("NLTK WordNet corpus is not available") |
There was a problem hiding this comment.
METEOR regression tests routinely become skips
Both new default-backend METEOR tests call _skip_without_wordnet(), so fresh CI runners without wordnet/omw-1.4 skip them and a reverted tokenizer fix can still pass — should we provision those corpora in unit-test setup, or replace the assertions with a non-skipped mocked NLTK-backend test while retaining an optional corpus integration test?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/evaluation/metrics/test_heuristics.py` around lines 414-422,
refactor `_skip_without_wordnet` and the default-backend METEOR tests so a missing NLTK
corpus cannot make the core tokenizer regression tests pass vacuously. Add a non-skipped
mocked NLTK backend test that verifies the METEOR implementation passes token lists, and
retain the current corpus-dependent behavior as a clearly optional integration test or
provision `wordnet` and `omw-1.4` in the Python SDK unit-test setup.
| def test_meteor_metric__default_nltk_backend__scores_without_error(): | ||
| # NLTK's meteor_score requires pre-tokenized input; before the fix the default | ||
| # backend passed raw strings and every call raised | ||
| # `TypeError: "hypothesis" expects pre-tokenized hypothesis`, so the metric was | ||
| # unusable outside of dependency-injected tests. Needs the WordNet corpus. |
There was a problem hiding this comment.
Third-party dependency contaminates unit suite
The unit tests invoke real NLTK backends—including METEOR cases gated by _skip_without_wordnet() and the chrF case—so optional packages and the WordNet corpus make unit runs slower and environment-dependent; should we move these regression tests to tests/library_integration/<library> and leave the unit file to cover injected scorer/tokenization behavior?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/evaluation/metrics/test_heuristics.py` around lines 447-481 and
576-588, move the real-NLTK METEOR and ChrF regression tests out of the unit suite into
the appropriate `tests/library_integration/<library>` suite. Remove
`_skip_without_wordnet()` and any resulting unused setup, and keep unit coverage limited
to injected scorer/tokenization behavior so tests remain fast, deterministic, and
independent of NLTK or the WordNet corpus.
- meteor: document `meteor_fn` as taking untokenized `(Sequence[str], str)`, explicitly distinguishing it from NLTK's tokenized `meteor_score` signature. - meteor: fail at construction with an actionable ImportError on nltk <= 3.6.4, which expects untokenized input and cannot work with the tokenizing adapter. Verified empirically that the API changed in nltk 3.6.5. - chrf: drop the `except TypeError` fallback. `sentence_chrf` has exposed the same signature since nltk 3.5, so the fallback was unreachable for any version installable on supported Pythons and could only mask genuine errors while silently discarding char_order/beta/ignore_whitespace. - tests: add stub-backed regression tests that run with no optional dependency installed, so the tokenization and per-reference contracts stay covered on a bare CI runner instead of being skipped when WordNet is absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| METEOR(track=False).score(output="the cat sat", reference="the cat ran") | ||
|
|
||
| assert recorded["hypothesis"] == ["the", "cat", "sat"] | ||
| assert recorded["references"] == [["the", "cat", "ran"]] | ||
|
|
There was a problem hiding this comment.
Incorrect METEOR scores go undetected
The test discards METEOR.score's result, so incorrect scores or metadata can pass unnoticed — should we assert result.value == pytest.approx(0.5) alongside the existing token assertions?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/evaluation/metrics/test_heuristics.py` around lines 467-471,
update `test_meteor_metric__default_backend__hands_nltk_pretokenized_input` so it
captures the result of `METEOR(track=False).score(...)` instead of discarding it. Add an
assertion that `result.value` equals `pytest.approx(0.5)`, while retaining the existing
tokenization assertions, so the test verifies both backend inputs and score propagation.
| # `sentence_chrf` has exposed this exact signature since NLTK 3.5, | ||
| # so every version installable on the Python versions this SDK | ||
| # supports accepts these keywords. Catching TypeError here would | ||
| # only mask genuine errors (and silently drop char_order/beta/ | ||
| # ignore_whitespace), so let it propagate. |
There was a problem hiding this comment.
Unverifiable NLTK compatibility claim
The compatibility comment claims sentence_chrf has supported this signature since NLTK 3.5 but gives no authoritative link, so maintainers can't verify why the unguarded keyword call is safe or where the compatibility boundary applies — should we add full URLs to the relevant NLTK fix and release?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/evaluation/metrics/heuristics/chrf.py` around lines 89-93, update
the compatibility comment in `ChrF.__init__` explaining the unguarded
`nltk_chrf_score.sentence_chrf` keyword call. Verify the claim that this signature is
supported since NLTK 3.5, then add full URLs to the authoritative NLTK API change,
release notes, or upstream source documenting that behavior and clearly state the
compatibility boundary.
| return float( | ||
| nltk_chrf_score.sentence_chrf( | ||
| reference, | ||
| candidate, | ||
| max_len=self._char_order, | ||
| beta=self._beta, |
There was a problem hiding this comment.
_compute now calls ChrF.score once per reference and takes the maximum, so multi-reference results, persisted reports, and pass/fail thresholds can change after an upgrade — should we document this best-reference contract and upgrade impact in the metric docs/changelog and add a regression test? score() accepts unbounded Sequence[str] inputs and text sizes, so one call can keep a worker busy indefinitely while futures.wait blocks — should we bound the inputs and add a per-score timeout or cancellation budget?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/evaluation/metrics/heuristics/chrf.py` around lines 94-112: 1)
Document that `ChrF` scores each reference independently and returns the maximum score
rather than passing the full reference sequence to NLTK, explain the impact on
multi-reference results, persisted reports, and thresholds, update the public metric
documentation and release changelog, and add/update a regression test covering the
best-reference behavior. 2) The `_compute`/`_score_single` logic invokes NLTK once per
reference without bounding reference count or total text size, letting a single score
consume a worker indefinitely. Add configurable maximum reference-count and
total-reference-text-size limits, validate them before entering the loop, fail with a
clear validation error when exceeded, and propagate a per-score timeout/cancellation
budget in the evaluation execution path so oversized or pathological NLTK work cannot
block a scoring worker.
| # NLTK 3.6.5 switched `meteor_score` to pre-tokenized input; 3.6.4 and earlier | ||
| # expect untokenized strings. Supporting both would mean branching on a release | ||
| # from 2021, so the default backend requires the modern API and says so clearly. | ||
| MINIMUM_NLTK_VERSION = "3.6.5" |
There was a problem hiding this comment.
Unverifiable NLTK API boundary
The version-specific NLTK compatibility comment omits authoritative full URLs for the upstream change and the release that introduced the pre-tokenized API, so the rationale for rejecting older releases is difficult to verify — should we add both links?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/evaluation/metrics/heuristics/meteor.py` around lines 14-17,
update the NLTK compatibility comment for the pre-tokenized `meteor_score` API boundary.
Add full, authoritative URLs to the upstream change/bug and the NLTK release
documentation for version 3.6.5, while preserving the explanation for rejecting older
releases.
1 participant
METEOR's default backend passed raw strings to nltk meteor_score, which requires pre-tokenized input, so every call raised TypeError and the metric was unusable. chrF passed the whole reference list to sentence_chrf, which takes a single reference, so NLTK joined the references into one string and an exact match against one of them scored 0.42 instead of 1.0.
meteor_fncontract string-based.Details
AI-WATERMARK
AI-WATERMARK: [yes|no]
Testing
Documentation