Repository navigation
[SC-19101] Fix list_tests crash on installs without optional extras - #573
Conversation
list_tests() loads every test before filtering and only skips tests that raise MissingDependencyError. StabilityAnalysisTranslation (transformers), the ragas tests (datasets) and ragas/utils.py (langchain_core) imported optional packages outside a guard, so a plain ModuleNotFoundError aborted the whole listing on installs without the nlp/llm extras. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
juanmleng
left a comment
There was a problem hiding this comment.
Review: #573 at 36f5d6d (merge-base e4b744b)
Verdict: Merge with recommendations
Computed by the calibrated-review skill, run by @juanmleng.
Summary
- What changed: Moves the optional
transformers,datasetsandlangchain_coreimports in 12 test modules inside the usualMissingDependencyErrorguard, solist_tests()skips those tests instead of crashing. - Result: The new guards are correct, but on a clean
validmind[xgboost]installlist_tests()still crashes, on 18 other tests. - Main thing to know: The PR's "none crash" result depends on
statsmodelsandnltkalready being installed. Colab preinstalls them, but a fresh local venv does not have them. - Next step: Add one fallback in
_load_tests(FN1) and alist_tests()smoke check to the existingcoreCI leg (FN2).
Fix now
FN1
- What: On a fresh local venv with
pip install "validmind[xgboost]",vm.tests.list_tests(filter="classification")still raisesLoadTestError, the symptom this PR sets out to fix. 16 tests importstatsmodels(in thestatsextra) and 2 importnltk(in thenlpextra) at module top level with no guard. - Why: It is the PR's stated bug, so it is worth closing here. It is not blocking because it fails loudly and no worse than on
main. - Where:
validmind/tests/load.py:255-259(_load_testsre-raises anything that is notMissingDependencyError). Examples of the unguarded imports:validmind/tests/data_validation/ADF.py:8validmind/tests/model_validation/embeddings/StabilityAnalysisSynonyms.py:8
- Suggested fix: In
_load_tests, also skip whene.original_erroris aModuleNotFoundError, and log it atwarningrather thandebug, so a genuinely broken import in a core test is still visible. That one guard covers all 18 tests and the next unguarded import someone adds. Keep the per-module guards this PR adds, because they give the better install message.
FN2
- What: No CI job calls
list_tests()without extras installed, so nothing caught this bug and nothing will catch the next one. - Why: The
coreleg ofdependency-testing.yamlalready installs the bare wheel (plusjinja2), so this is a one-line addition. - Where:
.github/workflows/dependency-testing.yaml,corebranch of the "Install and test via pip artifacts" step. - Suggested fix:
python -c 'import validmind as vm; vm.tests.list_tests(pretty=False)'. It fails on this PR's head today and passes once FN1 lands.
Fix later
FL1. A plain pip install validmind cannot be imported outside Jupyter. validmind/tests/load.py:24 imports pandas' Styler, which requires jinja2, and jinja2 is not a declared dependency. The CI core leg already works around this with a comment. Anyone using the library from a script or a fresh venv hits ImportError: Missing optional dependency 'Jinja2' on import validmind. It is not blocking because it predates this PR. Fix: declare jinja2 in core dependencies, or import Styler lazily. Suggested owner: library maintainers.
Accepted risks
-
What I checked: whether the
"datasets" in str(e)match is too loose or too narrow. -
Why it is acceptable: It matches
No module named 'datasets'. Any otherImportErroris re-raised, the same as the existingragasbranch. -
What I checked: the ragas tests import
.utilsabove their owntry, so thelangchain_coreguard fires before theragas/datasetsone. -
Why it is acceptable: It still raises
MissingDependencyErrorwithextra="llm". Only the package named in the message changes.
Verification performed
- Clean venv from the PR head, base package +
jinja2, loading every ID from_list_test_ids()→total 230 ok 165 skip 47 crash 18- all 18 crashes are
No module named 'statsmodels'or'nltk'
- all 18 crashes are
- Clean venv from the PR head with the
[xgboost]extra +jinja2, same loop →crash 18, the same 18 tests - Same
[xgboost]venv afterpip install statsmodels nltk→ok 187 skip 43 crash 0, andlist_tests(filter="classification")returns 106 tests - Clean venv without
jinja2→import validmindfails (FL1) - PR body claims:
- "37 skipped, none crash, classification returns 109" → reproduced only with
statsmodelsandnltkpresent; my counts differ (43 / 106), which is consistent with a different environment - "Full-extras env: all 11 touched tests still load" → NOT CHECKED
- "
tests.test_validmind_tests_module12 OK, ragastest_utils.py2 passed, black/isort/flake8 clean" → NOT CHECKED
- "37 skipped, none crash, classification returns 109" → reproduced only with
A fresh `validmind[xgboost]` install still crashed in list_tests() on 18 tests that import statsmodels or nltk without a guard. _load_tests now skips any test whose import raises ModuleNotFoundError and logs one warning naming the missing packages, so a broken import stays visible. Add a list_tests smoke check to the core CI leg, using a filter so every test is actually imported, plus a unit test for the fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`import validmind` needs jinja2: vm_models/result/utils.py imports it directly and pandas' Styler (format_dataframe, Styler test outputs) requires it. It was only installed transitively through optional extras, so a plain `pip install validmind` failed to import outside Jupyter. Drop the core CI leg's manual jinja2 install so that leg exercises the bare wheel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@juanmleng thanks for the review, all of it is addressed:
Verified in fresh venvs:
CI is running on the new commits. |
|
Thanks @cachafla, checked all three in fresh venvs: bare and |
Pull Request Description
What and why?
vm.tests.list_tests(filter=..., pretty=True)crashed on any install without thenlp/llmextras (e.g.pip install "validmind[xgboost]", which the quickstart notebooks use):list_testsloads every test before filtering, and_load_testsonly skips tests that raiseMissingDependencyError. Three spots imported optional packages outside a guard, so they raised a plainModuleNotFoundErrorand aborted the whole listing, whatever the filter:embeddings/StabilityAnalysisTranslation.py:transformers, with no guardragas/*tests:datasetswas imported above the existingragasguardragas/utils.py:langchain_core. This was hidden behind thedatasetsfailure and surfaced once that one was fixedEach import now sits in the usual
try/except ImportErrorblock and raisesMissingDependencyErrorwith the right extra, so these tests are skipped the same way the other optional-dependency tests are.Per review, there's also a fallback for the next unguarded import:
_load_testsnow skips any test whose import raisesModuleNotFoundErrorand logs one warning naming the missing packages (e.g.Skipping tests that import packages that aren't installed: `nltk` (2 tests); `statsmodels` (16 tests)). On a fresh[xgboost]install that covers 16statsmodelstests and 2nltktests that have no guard. ThecoreCI leg now runs alist_testssmoke check, and a unit test covers the fallback.Also from review:
jinja2is now a declared core dependency.import validmindneeds it (vm_models/result/utils.pyimports it directly, and pandas'Stylerused byformat_dataframerequires it), but it was only installed transitively through extras, so a plainpip install validmindcouldn't be imported outside Jupyter. ThecoreCI leg no longer installsjinja2by hand, so it exercises the bare wheel.How to test
In an env with only the base package (+
xgboost):Ran locally:
validmind[xgboost]+jinja2. Before the fallback: 165 load, 47 skipped, 18 crash (allstatsmodels/nltk). After:list_tests(filter="classification")returns 98 tests with one warning, andpretty=Trueworks.jinja2) in a fresh venv:jinja2comes in as a dependency,import validmindworks,list_tests(filter="classification")returns 98 tests, andpretty=Trueworks.tests.test_validmind_tests_module: 13 OK, including the new fallback test, which fails without theload.pychange.tests/unit_tests/model_validation/ragas/test_utils.py: 2 passed. black/isort/flake8 clean.CI smoke check: the
coreleg useslist_tests(filter="classification", pretty=False). It needs a filter: with no filter/task/tags andpretty=False,list_testsreturns the IDs without importing any test, so it would never catch this.What needs special review?
The ragas guard now matches
"ragas"or"datasets"in the error message and lists both inrequired_dependencies.The
ModuleNotFoundErrorfallback logs one aggregated warning perlist_testscall rather than one per test, to keep notebooks on minimal installs readable. Per-test details are logged at debug.Dependencies, breaking changes, and deployment notes
Adds
jinja2 (>=3.1.2)to coredependencies(pandas 2.x's minimum forStyler).uv.lockonly gains thejinja2edges.Release notes
Fixed
vm.tests.list_tests()failing with aLoadTestErrorwhen optional dependencies such astransformersordatasetsweren't installed. Tests that need those extras are now skipped from the listing instead.jinja2is now installed with the core package, soimport validmindworks after a plainpip install validmind.Checklist
🤖 Generated with Claude Code