Fix: explain the nltk 3.10.1 CWD import guard instead of leaking it - #151
Open
AmaadMartin wants to merge 2 commits into
Open
Fix: explain the nltk 3.10.1 CWD import guard instead of leaking it#151AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
…g it nltk 3.10.1 installs a meta-path finder that refuses any import whose file resolves under the current working directory while nltk is on the stack. With the virtualenv inside the project - the common '.venv/' layout - site-packages is under the CWD, so nltk blocks its own dependency and rouge_score, which ADK imports through google.adk.dependencies.rouge_scorer for the ROUGE eval metric, fails to import. The error the user sees names 'regex', never nltk and never ADK, and the '-P' / PYTHONSAFEPATH remedy it suggests does not work: the finder tests where the file lives, not what is on sys.path. pyproject.toml already excludes 3.10.1, and nltk reverted the hook in 3.10.2 (nltk/nltk#3732), but neither helps an environment that already has 3.10.1 installed or pinned in a lockfile. Translate that one failure at the dependency shim into a message naming the cause and the fix, chaining the original error; every other import failure propagates unchanged.
…ests Both module constants had exactly one reader each, and the module comment repeated what the user-facing message already says. Move the comment into the except block, keep only the facts the message does not carry, and inline both strings at their single use site. The tests set attributes on a fake module through setattr(). mypy excludes tests/ (pyproject.toml), so there was no checker to route around. Use plain attribute assignment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Link to an existing issue (if applicable):
No existing issue. I searched
google/adk-pythonfornltkand forBlocked importand found none.Or, if no issue exists, describe the change:
Problem: nltk 3.10.1 installs a meta-path finder that blocks any import whose file resolves under the current working directory while nltk is on the call stack. site-packages resolves under the current working directory when the virtual environment lives inside the project, so nltk blocks its own dependency and
rouge_scorefails to import. The user seesBlocked import of regex from current working directory for security reasons, which names neither nltk nor ADK. The-PandPYTHONSAFEPATHremedy that message suggests does not work, because the finder tests where the file lives, not what is onsys.path.Solution:
google.adk.dependencies.rouge_scoreris the only place ADK importsrouge_score, so I translate that one failure there. The newImportErrornames nltk 3.10.1 and the fixpip install --upgrade "nltk!=3.10.1", and it chains the original nltk error as__cause__. Every other import failure re-raises unchanged, so an absentrouge_scorestill raisesModuleNotFoundErrorand the existingexcept ModuleNotFoundErrorhandlers incli_eval.py,dev_server.pyandagent_evaluator.pykeep working.pyproject.tomlalready excludes 3.10.1, but that does not help an environment that already installed 3.10.1 or pinned it in a lockfile.I chose the diagnostic over setting
NLTK_DISABLE_IMPORT_SECURITY=1. A library must not switch off a dependency's security control on the user's behalf, and that variable is read only at nltk's first import, so the assignment is a silent no-op if anything imported nltk earlier.Grounding: I inspected both wheels.
nltk/inisec.pyexists in the 3.10.1 wheel andnltk/__init__.py:22imports it. Both are absent in 3.10.2. The marker string this change matches is the verbatim textinisec.NLTKSafeImportFinder.find_specraises.Collision check: I listed all 150 open pull requests on the fork and inspected their file lists. None touches
src/google/adk/dependencies/,tests/unittests/test_optional_dependencies.py, orfinal_response_match_v1.py. PR #114 shares the nltk root cause but changes onlytests/unittests/conftest.py, which this pull request does not touch. This branch is one commit ahead ofmainand zero behind.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
Four tests in
tests/unittests/test_optional_dependencies.pycover the guard-matched branch, the cause chain, the re-raise branch, and the success path.Mutation results. I ran each new test against broken code and confirmed it fails.
from rouge_score import ...lines..._explains_nltk_cwd_guardassert 'nltk' in "Blocked import of regex from current working directory ..."from rouge_score import ...lines..._chains_original_nltk_errorassert None is ImportError("Blocked import of regex ...")if ... not in str(e): raiseguard..._reraises_unrelated_import_errorassert 'nltk' not in 'Failed to import rouge_score because nltk 3.10.1 ...'tokenizersre-export..._reexports_when_import_succeedsAttributeError: module '_test_adk_rouge_scorer_shim' has no attribute 'tokenizers'Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
I ran this against real nltk 3.10.1, with the packages installed under the working directory.
I also checked the no-false-positive path: with
rouge_scoremapped toNoneinsys.modules, the import still raisesModuleNotFoundError: import of rouge_score halted; None in sys.modules.CI: every test job passes. Unit Tests on Python 3.10, 3.11, 3.12, 3.13 and 3.14, Mypy Check on 3.10 to 3.13, and A2A v0.3 Tests on 3.10 to 3.14 are all green.
The Pre-commit Linter job fails, and it does not fail because of this change. The
update-constraintshook fails the same way onmainat commita95b008f, the parent of this branch. Both logs reporthook id: update-constraintsandResolution failed even without constraints. I left it alone because it is unrelated breakage.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.