Skip to content

feat(deps): add rouge extra for ROUGE scoring deps - #506

Merged
arekay-nv merged 4 commits into
mainfrom
feat/rouge-scoring-deps
Sep 29, 2026
Merged

arekay-nv merged 4 commits into
mainfrom
feat/rouge-scoring-deps

Conversation

@anandhu-eng

Copy link
Copy Markdown
Contributor

Problem

RougeScorer (eval_method: "rouge") lazily imports nltk, evaluate and rouge_score and raises an ImportError telling users to install them — but no extra ever declared them. The three shipped Llama configs using eval_method: "rouge" fail at scorer construction on a stock install, and the only install instructions lived in examples/05_Llama_Examples/README.md.

Change

  • New rouge extra declaring nltk==3.10.3, evaluate==0.4.6, rouge-score==0.1.2.
  • test extra inherits it via inference-endpoint[rouge] — same self-reference pattern as inference-endpoint[sql] — so CI covers the import surface with no extra job.

Resolution is clean: 4 new packages, evaluate pulls the already-pinned datasets==5.0.1, and no existing pin moves. Unlike bfcl, no [tool.uv].conflicts fork is needed.

Verified

  • uv lock adds only absl-py, evaluate, nltk, rouge-score; no version changes to existing packages.
  • uv sync --frozen --extra rouge succeeds; all three import at the pinned versions.
  • The lazy-import guard in evaluation/scoring.py now resolves (_evaluate/_nltk non-None), so the ImportError path is no longer hit.

Note

Installing the packages is not sufficient for an offline image: nltk needs the punkt/punkt_tab corpora and evaluate.load("rouge") fetches its metric script from the HF Hub. Prefetching both is handled separately in Dockerfile.dev.

🤖 Generated with Claude Code

`RougeScorer` (eval_method: "rouge") lazily imports nltk, evaluate and
rouge_score and raises ImportError telling users to pip install them, but
no extra ever declared them. The three shipped Llama configs that use
eval_method: "rouge" therefore fail at scorer construction on a stock
install; the only instructions lived in examples/05_Llama_Examples/README.md.

Declare them in a new `rouge` extra and have the `test` extra inherit it
(same self-reference pattern as `inference-endpoint[sql]`), so CI covers
the import surface with no extra job. Resolution is clean: 4 new packages,
`evaluate` pulls the already-pinned datasets==5.0.1, and no existing pin
moves - so unlike bfcl this needs no [tool.uv].conflicts fork.

Not self-contained at runtime: nltk needs the punkt/punkt_tab corpora and
evaluate.load("rouge") fetches its metric script from the HF Hub. Offline
images must prefetch both - handled separately in Dockerfile.dev.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@anandhu-eng
anandhu-eng requested a review from a team September 15, 2026 11:41
@github-actions github-actions Bot added the size/normal PR Review Policy: <=500 non-test lines & <=20 files label Sep 15, 2026
@github-actions

Copy link
Copy Markdown

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@attafosu attafosu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@arekay-nv arekay-nv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

@anandhu-eng

Copy link
Copy Markdown
Contributor Author

With respect to the failing GitHub action , It seems there is no fixed version for that particular security issue (current nltk version is 3.10.3)

Scanning the nltk repo, seems they are actively working to fix this security issue:

nltk/nltk#3753

Last activity was yesterday. I have pinged them asking whether there is a timeline.

@attafosu , We could wait till folks from nltk fixes these issue or do we want to avoid using NLTK? Would replacing NLTK be a simpler fix?

@attafosu

attafosu commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

With respect to the failing GitHub action , It seems there is no fixed version for that particular security issue (current nltk version is 3.10.3)

Scanning the nltk repo, seems they are actively working to fix this security issue:

nltk/nltk#3753

Last activity was yesterday. I have pinged them asking whether there is a timeline.

@attafosu , We could wait till folks from nltk fixes these issue or do we want to avoid using NLTK? Would replacing NLTK be a simpler fix?

@anandhu-eng Yes I've looked at alternatives and pysbd's segmenter seems to lead to very similar rouge scores as nltk's sent_tokenize. However, the rouge-scorer package also requires nltk, so the audit will still flag it.
But the vulnerability in the nltk actually affects components that have no exposure in this repo: TransitionParser.train/parse, AveragedPerceptron.save/load . We only use the sentence tokenization and the stemming (in rouge-scorer).
Thus we can ignore this specific vulnerability check in the audit: --ignore-vuln PYSEC-2026-3740 (and its alias CVE-2026-81726).
@arekay-nv @anandhu-eng if agreed we can patch the test.yml.

arekay-nv and others added 2 commits September 29, 2026 16:22
nltk 3.10.3 (pulled in by the rouge extra) is flagged by PYSEC-2026-3740 /
CVE-2026-81726, a path traversal in model-artifact APIs (TransitionParser,
AveragedPerceptron, PerceptronTagger, maxent). No fixed release exists yet
(nltk/nltk#3753). This repo only uses sent_tokenize and rouge_score's Porter
stemmer, so the vulnerable code is unreachable.

Ignore the advisory in CI's pip-audit and document the flag for local runs.
Removal is tracked in #520.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
test_template_runs spawns 10 workers on 4-CPU CI runners, where start-up
takes long enough to straddle WorkerManager's 12s readiness slices (10% of
the 120s timeout). A ready signal received in one slice is discarded when
the slice times out, so init stalls until the full timeout. 300s gives 30s
slices, longer than worker start-up, which unblocks CI while the
underlying WorkerManager/ReadyCheckReceiver fix lands separately.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@arekay-nv
arekay-nv merged commit 0543413 into main Sep 29, 2026
14 of 15 checks passed
@arekay-nv
arekay-nv deleted the feat/rouge-scoring-deps branch October 8, 2026 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/normal PR Review Policy: <=500 non-test lines & <=20 files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants