Repository navigation
fix(outliers): make the optional-import fallback actually work - #2094
Merged
Merged
Conversation
graphistry/outliers.py guards matplotlib/sklearn/numpy behind try/except and binds each name to None on failure, so the module is meant to import without them. It did not. `import matplotlib.font_manager` binds the name `matplotlib`, and that name was missing from the except block, so outliers.py:162 would have raised NameError. Worse, `get_outliers` annotates `Union[np.ndarray, pd.DataFrame]`, which is evaluated at def time: with `np = None` the module raised AttributeError before defining anything. matplotlib is not a dependency, so `import graphistry.outliers` failed on a default install. Nothing inside graphistry/ imports it, so only direct callers were affected. Annotations are now lazy and the fallback list is complete. A test imports the module with matplotlib blocked, and a second test pins the defect CLASS: no name may be bound on only one path of that try/except. One more instance of the same shape, a no-op at runtime: `if polars: import polars as pl` in gfql_unified.py makes `pl` a function-local, so the pandas path left it unbound. Both arms are guarded by the same flag and both are exercised directly in verification. The six skrub instances in feature_utils.py are deliberately NOT here. Their `else` branch is unreachable in every lane the changed-line-coverage gate consumes -- it needs skrub absent, while the enclosing function needs sklearn, which only the umap/ai lanes have, and those do not feed that gate. That file is already the subject of a later PR in this series; it gets fixed there, with its test lanes considered as a whole, rather than carried here behind a pragma. Pyright ratchet 273 -> 271 (-2). Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_017ropeBMLJUuy6ViYwy15ud
This was referenced Sep 18, 2026
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
1 participant
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.
First of the pyright-ratchet cleanup series (after #2092, #2093). Ratchet 273 → 271.
The real bug
graphistry/outliers.pyguards matplotlib/sklearn/numpy behindtry/exceptand binds each name toNoneon failure, so the module is meant to import without them. It did not — and matplotlib is not a dependency, so that is the default install:Two separate defects:
import matplotlib.font_managerbinds the namematplotlib, and that name was missing from the except block — every other name in thetryhad a fallback.outliers.py:162usesmatplotlib.font_manager.FontPropertiesdirectly, so that path raisedNameError.get_outliersannotatesUnion[np.ndarray, pd.DataFrame], which is evaluated atdeftime. Withnp = Nonethe module raisedAttributeErrorbefore defining anything — so thetry/exceptcould never have helped.Fixed by completing the fallback list and making annotations lazy (
from __future__ import annotations). The module now imports and degrades as written; calls still fail without matplotlib, which is the documented intent of theNonebindings.Nothing inside
graphistry/imports this module, so only direct callers ofgraphistry.outlierswere affected. That is why it went unnoticed.Tests
test_outliers_imports_when_matplotlib_is_missingblocks matplotlib viasys.modulesand re-imports. It fails on master with theAttributeErrorabove.test_every_name_the_optional_import_binds_has_a_fallbackpins the defect class rather than the instance: it walks the module's AST and asserts no name is bound on only one path of thattry/except. That is the check that was missing.Also here
if polars: import polars as plingfql_unified.pymakespla function-local, so the pandas path left it unbound. Both arms are guarded by the same flag, so nothing changes at runtime; the binding is made explicit. Verified by exercising both arms directly (polars:pl.col(...)mirrors the prefixed column; pandas:plstays untouched), plus 7091 polars tests.Deliberately not here
The six skrub instances in
feature_utils.pyare the same shape, but theirelsebranch is unreachable in every lane the changed-line-coverage gate consumes — it needs skrub absent, while the enclosing function needs sklearn, which only the umap/ai lanes have, and those do not feed that gate. Carrying it here would mean a# pragma: no coveron a line I can't justify skipping.feature_utils.pyis already the subject of a later PR in this series (it holds 12 more findings); it gets fixed there, with its test lanes considered as a whole.Verification
bin/lint.sh(ruff, type-hygiene 4448, comment-density 1151) · pyright guard 271 · mypy 351 files · changed-line-coverage 3/3 = 100% · 7091 polars tests ingraphistry/tests/compute/.🤖 Generated with Claude Code
https://claude.ai/code/session_017ropeBMLJUuy6ViYwy15ud