Skip to content

fix(RHOAIENG-50189): Update nltk to 3.9.3#132

Merged
ruivieira merged 2 commits intoopendatahub-io:incubationfrom
AmberJBlue:update-nltk
Mar 18, 2026
Merged

fix(RHOAIENG-50189): Update nltk to 3.9.3#132
ruivieira merged 2 commits intoopendatahub-io:incubationfrom
AmberJBlue:update-nltk

Conversation

@AmberJBlue
Copy link
Copy Markdown

@AmberJBlue AmberJBlue commented Mar 18, 2026

Update nltk to 3.9.3+ to include nltk/nltk#3468

Summary by CodeRabbit

  • Chores
    • Raised NLTK requirement from 3.9.1 to 3.9.3 for IFEval tasks, enforcing a stricter runtime check.
    • Updated dependency listings: bumped optional NLTK, removed several packages, and added platform-specific constraints in requirements to improve compatibility.

@AmberJBlue AmberJBlue requested a review from ruivieira March 18, 2026 23:14
@AmberJBlue AmberJBlue self-assigned this Mar 18, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 18, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

The PR updates the minimum NLTK version requirement from 3.9.1 to 3.9.3 in two modules (lm_eval/tasks/ifeval/instructions_util.py and lm_eval/tasks/leaderboard/ifeval/instructions_util.py) and in the optional dependencies in pyproject.toml. requirements.txt was edited to remove multiple pinned packages (including cryptography, protobuf, rsa, opentelemetry packages, mlflow-skinny, uvicorn) and to add platform-conditional constraints for cffi and pycparser targeting PyPy.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Security considerations

Actionable items (security-first, prioritized):

  1. Verify NLTK bump correctness

    • Confirm NLTK 3.9.3 is compatible with code paths using NLTK resources.
    • Check NVD/security advisories for any fixes introduced between 3.9.1 → 3.9.3.
  2. Confirm intent and impact of removed pinned packages

    • Identify why cryptography, protobuf, rsa, mlflow-skinny, uvicorn, and opentelemetry pins were removed.
    • Ensure these libraries are not required transitively or at runtime; if still required, reintroduce appropriate version bounds.
    • Explicitly verify removal does not silently allow resolution to older vulnerable transitive versions.
  3. Assess protobuf/rsa removal risk

    • Protobuf and crypto-related library removals can introduce severe supply-chain and correctness risks. Verify that no code or third-party dependency relies on them.
  4. Validate platform-conditional constraints

    • The PyPy-only constraints for cffi/pycparser must be validated in PyPy CI runs to avoid breakage or missing packages in PyPy builds.
  5. CI and lockfile validation

    • Run dependency resolution (pip-tools/poetry/etc.) and CI to confirm reproducible installs and to detect any newly introduced vulnerable transitive versions.
    • If you rely on lockfiles, update and commit them; otherwise run a security scanner (Snyk/Dependabot/OSV/NVDC) against the resolved graph.
  6. Supply-chain mitigation

    • Ensure dependency changes do not enable dependency confusion or allow unpinned transitive libraries to regress to vulnerable versions.
    • Consider adding explicit version bounds for high-risk packages (cryptography, protobuf, rsa) or an allowlist/denylist in dependency scanning.

Only actionable flags above; do not merge until CI passes and the dependency removals and NLTK bump are explicitly validated.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately reflects the main change: updating nltk dependency from 3.9.1 to 3.9.3 across configuration and source files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can use your project's `ruff` configuration to improve the quality of Python code reviews.

Add a Ruff configuration file to your project to customize how CodeRabbit runs ruff.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
requirements.txt (1)

86-86: Remove the redundant implementation_name condition from the pycparser marker.

The condition implementation_name != "PyPy" uses case-mismatched logic. The canonical value for implementation_name on PyPy is "pypy" (lowercase), not "PyPy", making this condition always evaluate to true and adding no functional value. The platform_python_implementation == "PyPy" check alone sufficiently scopes the requirement.

Proposed fix
-pycparser==3.0 ; python_version >= "3.11" and python_version < "3.13" and platform_python_implementation == "PyPy" and implementation_name != "PyPy"
+pycparser==3.0 ; python_version >= "3.11" and python_version < "3.13" and platform_python_implementation == "PyPy"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@requirements.txt` at line 86, Remove the redundant implementation_name check
from the pycparser environment marker in the requirements entry for
"pycparser==3.0"; specifically, update the marker that currently reads
'python_version >= "3.11" and python_version < "3.13" and
platform_python_implementation == "PyPy" and implementation_name != "PyPy"' to
drop the 'implementation_name != "PyPy"' clause so the marker relies only on
python_version range and platform_python_implementation == "PyPy".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@requirements.txt`:
- Line 22: requirements.txt currently pins eval-hub-sdk==0.1.2 but
pyproject.toml and the code expect the adapter extra (evalhub.adapter is
imported in main.py around the imports block and later at use sites), so update
the requirements entry to include the adapter extra
(eval-hub-sdk[adapter]==0.1.2) while preserving the existing Python version
marker; this ensures the evalhub.adapter module is installed and prevents
ImportError when bootstrapping from requirements.txt.

---

Nitpick comments:
In `@requirements.txt`:
- Line 86: Remove the redundant implementation_name check from the pycparser
environment marker in the requirements entry for "pycparser==3.0"; specifically,
update the marker that currently reads 'python_version >= "3.11" and
python_version < "3.13" and platform_python_implementation == "PyPy" and
implementation_name != "PyPy"' to drop the 'implementation_name != "PyPy"'
clause so the marker relies only on python_version range and
platform_python_implementation == "PyPy".

ℹ️ Review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 8dd64824-31a2-482c-9e04-6afa2130fbd0

📥 Commits

Reviewing files that changed from the base of the PR and between 88aa9f3 and a8bd595.

⛔ Files ignored due to path filters (1)
  • poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • lm_eval/tasks/ifeval/instructions_util.py
  • lm_eval/tasks/leaderboard/ifeval/instructions_util.py
  • pyproject.toml
  • requirements.txt

@ruivieira ruivieira added the kind/bug Something isn't working label Mar 18, 2026
Copy link
Copy Markdown
Member

@ruivieira ruivieira left a comment

Choose a reason for hiding this comment

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

Is it ok to restore the eval-hub-sdk[adapter]? Thanks!

@ruivieira ruivieira merged commit 6b7f865 into opendatahub-io:incubation Mar 18, 2026
3 of 5 checks passed
@AmberJBlue AmberJBlue deleted the update-nltk branch March 18, 2026 23:27
ruivieira pushed a commit to ruivieira/lm-evaluation-harness that referenced this pull request Mar 26, 2026
* Update nltk to 3.9.3

* Revert eval-hub[adapter]
ruivieira added a commit to ruivieira/lm-evaluation-harness that referenced this pull request Mar 26, 2026
* Update nltk to 3.9.3

* Revert eval-hub[adapter]

Signed-off-by: Rui Vieira <ruidevieira@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants