[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 5 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.
There was a problem hiding this comment.
Fixed in 03e8bd6. The TypeError fallback is gone entirely: sentence_chrf is now always called with max_len/beta/ignore_whitespace, so a configuration like ChrF(beta=2, char_order=1) can no longer be silently discarded, and any TypeError propagates instead of being swallowed.
9029aa3 adds permalinks backing the "no fallback needed" reasoning: that keyword signature has been stable since NLTK 3.4 (2018), the release that added ignore_whitespace — https://github.com/nltk/nltk/blob/3.4/nltk/translate/chrf_score.py#L18 — and is unchanged in 3.9.1.
There was a problem hiding this comment.
Thanks for the clarification and permalinks. I’ll save this to memory once the PR is merged.
| # 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.
There was a problem hiding this comment.
Fixed in 03e8bd6. The meteor_fn docstring now documents the contract as (references, hypothesis) -> float over untokenized text (a sequence of reference strings, one hypothesis string), and explicitly warns that this differs from nltk.translate.meteor_score.meteor_score, so passing NLTK's function in directly will not work. The tokenizing adapter is internal to the default backend and is what normalizes strings into NLTK's pre-tokenized form.
| 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.
There was a problem hiding this comment.
Fixed in 03e8bd6. The default backend now refuses old NLTK at construction time rather than failing with a confusing TypeError at score time: MINIMUM_NLTK_VERSION = "3.6.5", and the constructor raises an ImportError naming the required version and pointing at pip install -U nltk or supplying meteor_fn. Covered by test_meteor_metric__legacy_nltk__raises_actionable_import_error.
9029aa3 adds the upstream citations behind that boundary: nltk/nltk#2822 and the 3.6.5 ChangeLog entry "METEOR evaluation now requires pre-tokenized input".
| 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.
There was a problem hiding this comment.
Addressed in 03e8bd6 by adding test_meteor_metric__default_backend__hands_nltk_pretokenized_input, which stubs the NLTK module and therefore never skips — it runs on a bare CI runner with neither nltk nor the WordNet corpus installed, and fails if the tokenization fix is reverted. test_meteor_metric__legacy_nltk__raises_actionable_import_error is stubbed the same way.
9029aa3 also makes that test assert the returned ScoreResult (value and reason), not just the shape of the NLTK call. The corpus-gated tests are kept as additional end-to-end coverage for environments that do have WordNet; they are no longer the only thing protecting the regression.
| 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.
There was a problem hiding this comment.
This follows the existing convention in that file rather than introducing it: test_chrf_metric__char_order_and_ignore_whitespace_vary__change_score already ran the real NLTK backend behind pytest.importorskip("nltk") before this PR.
In practice this does not slow CI or make it environment-dependent, because nltk is not in sdks/python/tests/test_requirements.txt — every one of these tests skips in the unit CI job. The regressions this PR fixes are locked by the stubbed tests added in 03e8bd6, which never skip. Making the corpus-gated ones run in CI would mean pulling WordNet over the network during unit tests, which seems worse than the current split.
Happy to move the importorskip tests to tests/library_integration/ if the maintainers prefer that boundary — just let me know and I'll include the pre-existing chrF one in the move.
There was a problem hiding this comment.
Thanks, that context makes the existing split appropriate. I’ll save this to memory once the PR is merged.
- 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.
There was a problem hiding this comment.
Fixed in 9029aa3 — the test now keeps the result and asserts both the value and the reason string:
result = METEOR(track=False).score(output="the cat sat", reference="the cat ran")
assert recorded["hypothesis"] == ["the", "cat", "sat"]
assert recorded["references"] == [["the", "cat", "ran"]]
assert result.value == pytest.approx(0.5)
assert result.reason == "METEOR score: 0.5000"| # `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.
There was a problem hiding this comment.
Fair — fixed in 9029aa3, and the claim was slightly off as well. The keyword signature has been stable since NLTK 3.4 (2018-11-17), the release that added ignore_whitespace, not 3.5. The comment now cites both ends of the range:
- https://github.com/nltk/nltk/blob/3.4/nltk/translate/chrf_score.py#L18
- https://github.com/nltk/nltk/blob/3.9.1/nltk/translate/chrf_score.py#L16
Both show sentence_chrf(reference, hypothesis, min_len=1, max_len=6, beta=3.0, ignore_whitespace=True), which is why an unguarded keyword call is safe here and why catching TypeError would only mask genuine errors.
There was a problem hiding this comment.
Thanks for correcting the version detail and updating it with supporting references. I’ll save this to memory once the PR is merged.
| 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.
There was a problem hiding this comment.
Documented in 9029aa3. The best-reference behaviour is now stated in the ChrF class docstring and in the user-facing metric docs (fern/docs/evaluation/metrics/heuristic_metrics.mdx and the docs-v2 copy): when several references are passed, the candidate is scored against each one separately and the highest score is returned. That is the standard multi-reference behaviour for chrF, and it is a fix rather than a behaviour change in the usual sense — the previous result came from NLTK joining the reference list into a single string, which scored an exact match against one reference at ~0.42.
Regression coverage already exists in this PR:
test_chrf_metric__default_backend__scores_each_reference_separately(stubbed NLTK, asserts one call per reference and that the best score wins)test_chrf_metric__multiple_references__scores_against_best_reference(real backend, exact match against one of two references scores 1.0)
On the timeout/bounded-input half: I'd rather not add that here. Every heuristic metric in this package takes caller-supplied text of arbitrary size (SentenceBLEU, ROUGE, GLEU included), chrF is linear in the input per reference, and a per-score budget belongs at the evaluation-framework level, not inside one metric — adding it to chrF alone would be inconsistent without fixing anything.
There was a problem hiding this comment.
Thanks for the detailed context. I can only save feedback to memory for specific code review findings, not general feedback or PR-level context.
| # 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.
There was a problem hiding this comment.
Fixed in 9029aa3. The comment above MINIMUM_NLTK_VERSION now links the upstream change and the release that shipped it:
- Accept pre-tokenized references & hypothesis for METEOR calculation nltk/nltk#2822 — "Accept pre-tokenized references & hypothesis for METEOR calculation"
- https://github.com/nltk/nltk/blob/3.6.5/ChangeLog — "Version 3.6.5 2021-10-11 ... METEOR evaluation now requires pre-tokenized input"
- the signatures either side of the boundary: https://github.com/nltk/nltk/blob/3.6.4/nltk/translate/meteor_score.py#L343 vs https://github.com/nltk/nltk/blob/3.6.5/nltk/translate/meteor_score.py#L347
|
Thanks for the contribution! When you have a chance, could you take a look at the automated review comments from Baz? For each comment, please either address it with a code change or reply with a brief explanation if you believe the current implementation is the right approach. Thanks! |
Follow-up on review feedback: - chrF: document the best-reference multi-reference behaviour in the class docstring and the heuristic metrics docs, and back the "no TypeError fallback" comment with permalinks to the NLTK sources that show the signature has been stable since 3.4. - chrF: type `chrf_fn`/the internal helpers with `str` for the candidate, matching what `score()` actually passes, and spell out the callable contract (single candidate, sequence of references). - METEOR: cite the upstream NLTK PR, ChangeLog entry and the 3.6.4 vs 3.6.5 signatures behind MINIMUM_NLTK_VERSION. - tests: assert the score value and reason returned through the stubbed NLTK backend, not just the tokenized call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
documentation
|
Thanks @andrescrz — I've gone through every Baz comment and replied to each one inline. Seven were addressed with code (03e8bd6 earlier, 9029aa3 just now):
Two I pushed back on, with reasoning in the threads: adding a per-score timeout / input bound to chrF (belongs at the evaluation-framework level, and would be inconsistent to add to one heuristic metric), and moving the Verified locally: the 10 METEOR/chrF tests pass with WordNet installed (none skipped), and |
| return max( | ||
| _score_single(candidate, reference) for reference in references | ||
| ) |
There was a problem hiding this comment.
Non-finite ChrF feedback reaches backend
max() can select a non-finite scorer result, while float(value) only changes its type, so it reaches ScoreResult.value, passes feedback validation, and is serialized by log_test_result_feedback_scores — should we apply math.isfinite before constructing ScoreResult and add a regression 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/src/opik/evaluation/metrics/heuristics/chrf.py` around lines 126-128,
update the `ChrF.__init__` default `_compute` multi-reference scoring so the score
selected by `max()` is validated with `math.isfinite` before it can become a
`ScoreResult`. Reject non-finite values such as NaN and positive or negative infinity
with the metric’s appropriate error behavior, and add a regression test covering a
scorer that returns a non-finite result.
There was a problem hiding this comment.
I looked into whether this is reachable, and I don't think it is for the default backend — NLTK guards the division itself. sentence_chrf returns an epsilon rather than dividing by zero, so degenerate inputs bottom out at 1e-16, not NaN:
sentence_chrf("a", "b", max_len=6, ignore_whitespace=True) -> 1.0000000000000001e-16
sentence_chrf("x", "y" * 20, max_len=1, ignore_whitespace=False) -> 1e-16
ChrF.score also rejects empty/whitespace candidates and references before anything reaches the scorer, so there is no input that makes max() select a non-finite value here.
That leaves a custom chrf_fn returning NaN, which is caller-supplied and identical in shape for every heuristic metric in this package (SentenceBLEU, ROUGE, GLEU, METEOR all do float(...) on an injected scorer's result). Guarding only chrF would be inconsistent without fixing anything, and the SDK already treats this as a framework-level concern — _is_valid_score_value in opik/evaluation/score_statistics.py filters non-finite values with math.isfinite during aggregation.
Happy to be overruled if the maintainers would rather have ScoreResult validate finiteness centrally; that seems like the right layer for it, but out of scope for an NLTK usage fix.
There was a problem hiding this comment.
Thanks for the detailed validation—you're right that this is not reachable for the default backend, and custom scorer finiteness should be handled centrally if needed. I'll save this context to memory once the PR is merged.
| if ( | ||
| nltk is not None | ||
| and semantic_version.SemanticVersion.parse(nltk.__version__) | ||
| < MINIMUM_NLTK_VERSION |
There was a problem hiding this comment.
METEOR leaks version-parser failures
The constructor feeds nltk.__version__ to strict SemanticVersion.parse without catching parser errors, so valid PEP 440 forms such as X.Y.5a and unknown (...) fallbacks raise raw ValueError instead of the documented compatibility ImportError; meanwhile, SemanticVersion.compare ignores prerelease components, so 3.6.5-rc1 is accepted as equal to 3.6.5. Should we use a compatible version parser/strategy and map parse failures, below-floor versions, and prereleases to the documented ImportError?
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 86-89,
update the `METEOR.__init__` NLTK version check to handle NLTK’s PEP 440-compatible
versions instead of passing `nltk.__version__` to strict `SemanticVersion.parse`. Use a
compatible parser/comparison strategy that rejects prereleases below `3.6.5`, and catch
invalid or fallback values such as `unknown (...)`, converting them into the documented
actionable `ImportError` rather than leaking parser exceptions.
There was a problem hiding this comment.
Good catch — this was a real bug, fixed in f4915fb.
SemanticVersion.parse is strict SemVer, and NLTK ships version strings it rejects. nltk.__version__ == "3.5" (a real 2020 release, and the same for "3.4" and "3.3") raised ValueError: '3.5' is not valid SemVer string — so the guard blew up with a parser error on exactly the old installs it exists to reject, instead of producing the documented ImportError.
The check now reads the leading numeric components and compares them as a tuple:
"3.5","3.4","3.3","3.2.5","3.0a3","3.6.4"→ the actionableImportError"3.6.5rc1"/"3.6.5-rc1"→ below the floor, as you noted, since a prerelease predates the API"3.6.5.post1","3.6.5+local","3.9.4","3.10.3"→ accepted (tuple comparison also orders 3.10 above 3.9, which a string comparison would get wrong)
On mapping parse failures to ImportError: I went the other way for genuinely unreadable strings such as the "unknown (...)" fallback. Refusing to construct the metric because a version string could not be read would break working installs over a cosmetic problem. Instead the guard is skipped and _scorer now maps NLTK's TypeError onto a MetricComputationError that names the version requirement while quoting NLTK's own message, so the failure stays actionable without being misreported.
Covered by test_meteor_metric__unsupported_nltk_versions__raise_import_error, test_meteor_metric__supported_nltk_versions__construct (both parametrized over the strings above) and test_meteor_metric__nltk_rejects_tokenized_input__raises_metric_error.
There was a problem hiding this comment.
Thanks for the detailed correction—this handling and test coverage make sense. I’ll save this to memory once the PR is merged.
`SemanticVersion.parse` rejects the two-component version strings NLTK
actually ships ("3.5", "3.4", "3.3") and the prerelease forms it used
historically ("3.0a3"), so the guard raised a raw ValueError on exactly
the old installs it exists to reject, instead of the documented
ImportError.
Read the leading numeric components instead and compare as a tuple, which
also orders 3.10 above 3.9 correctly. Prereleases of the minimum version
count as below it; post/local versions do not. Version strings that cannot
be read at all no longer block construction — an ancient NLTK now surfaces
through `_scorer`, which maps NLTK's TypeError onto an actionable
MetricComputationError naming the version requirement.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks again for the contribution! Could you please resolve the merge conflicts in the PR? Once that's done, we'll continue with the review. Thanks! |
# Conflicts: # apps/opik-documentation/documentation/fern/docs/evaluation/metrics/heuristic_metrics.mdx
|
Done — There was a single conflict, and it was a modify/delete: #7946 removed the v1 documentation tree, and this branch had edited Re-verified after the merge: the 24 METEOR/chrF unit tests pass, and |
2 participants
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