Skip to content

fix(typing): two real defects CI's type gate cannot see (#1791) - #1792

Merged
lmeyerov merged 1 commit into
masterfrom
fix/1791-typing-real-defects
Jul 27, 2026
Merged

fix(typing): two real defects CI's type gate cannot see (#1791)#1792
lmeyerov merged 1 commit into
masterfrom
fix/1791-typing-real-defects

Conversation

@lmeyerov

Copy link
Copy Markdown
Contributor

Two real defects surfaced by #1791, plus the evidence that CI's type gate cannot see them.

The gate is off, not mis-pinned

The premise in #1791 was that CI pins an older pandas-stubs than the container. The opposite is true, and neither pin is the cause.

mypy pandas-stubs result
CI, as it actually runs today 2.3.0 (uvx) none Success: no issues found in 324 source files
CI's own locked env (requirements/test-py3.12.lock) 2.1.0 3.0.3.260530 140 errors in 10 files
test-rapids-official:26.02-gfql-polars 1.19.1 3.0.0.260204 173 errors in 12 files

bin/typecheck.sh resolves MYPY_CMD to uvx mypy whenever uvx is on PATH — and CI puts it there (python -m pip install --upgrade pip uv). uvx runs mypy in an isolated ephemeral environment, so the --require-hashes lockfile install of pandas-stubs, types-requests, types-tqdm, types-defusedxml and the project's own deps is never visible to it. mypy.ini then has ignore_missing_imports = True for pandas.*, numpy.*, pyarrow.* etc., so every external symbol degrades to Any and the whole file set passes vacuously.

Two independent confirmations:

  • CI's log installs mypy==2.1.0 from the lock, then reports mypy 2.3.0 (compiled: yes) at run time — a different interpreter entirely.
  • mypy --no-site-packages reproduces CI's output byte for byte, including the file count: Success: no issues found in 324 source files.

bin/lint.sh has the same uvx ruff shape. Less harmful (ruff needs no imports and still reads pyproject.toml), but the locked ruff version is likewise bypassed, so a new ruff release can turn CI red with no repo change.

Full triage of all 140 in #1791.

What this PR fixes

Two bucket-B items — real code defects, worth fixing regardless of any stub decision.

1. graphviz: KeyError: None on a bound-but-unlabelled frame

g_to_pgv asserts _nodes/_edges are set, but not that the bindings are. g.nodes(df) with no node= leaves _node None while _nodes is set — and layout_graphviz/render_graphviz only call materialize_nodes() when _nodes is None, so that graph sails through and reaches row[None].

# master
g._node = None
RESULT: KeyError : None

# this PR
RESULT: ValueError : graphviz export requires a bound node id column;
        use g.nodes(df, node='id') or g.materialize_nodes()

Same for g.edges(df) with no source/destination. Bindings are now resolved once into non-Optional locals; validation moved ahead of the optional pygraphviz import so a caller error is not masked by a missing-backend error, and so it is testable without the optional dep. Two regression tests added — they fail on master, pass here, and are not skipped when pygraphviz is absent.

2. search_any: pat bound to both a module and a str in one scope

import pandas.api.types as pat     # line 95
...
    or bool(pat.is_bool_dtype(dt))  # line 100  -> module
...
pat, case = term, case_sensitive    # line 110  -> str
pred = Contains(pat, ...)           # line 118

Exactly the shadowed-name shape #1791 predicted. Ordering saves it today (module use strictly precedes the rebind), so this is a latent hazard rather than a live failure — one statement reorder from AttributeError: 'str' object has no attribute 'is_bool_dtype'. Module alias renamed to pd_types. No behaviour change, so no test; the mypy differential is the evidence.

Also corrects two dtype: object params to the engine-agnostic DType alias — they receive pandas/cuDF/polars dtypes, so object was both less accurate and on the repo's banned-annotation list.

No suppressions

Zero # type: ignore added, zero Any/cast/getattr/setattr/Dict[str, Any]/object. Every one of the 12 errors goes away because the code got more correct.

Gates

  • mypy differential, CI's own pin (mypy 2.1.0 + pandas-stubs 3.0.3.260530): 140 → 128, zero new errors, diffed error-set to error-set. search_any.py 9→0, graphviz.py 3→0. Under the container stubs: 173 → 161.
  • ruff (bin/ruff.sh, with pyproject.toml present): clean.
  • graphistry/tests/compute with --gpus all: 9 failed, 7030 passed, 90 skipped, 15 xfailed — identical to the master baseline; all 9 are the pre-existing igraph/cudf/dask failures.
  • graphistry/tests/plugins/test_graphviz.py: 2 passed, 9 skipped (pygraphviz absent); the 2 new tests run and pass.

All measured in graphistry/test-rapids-official:26.02-gfql-polars on dgx-spark.

Not in this PR

Turning the gate back on. It is a two-line change to bin/typecheck.sh, but it flips CI from green to 128 errors, so it needs its own staged PR — recommendation in #1791.

🤖 Generated with Claude Code

https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB

Both were found by running the repo's own mypy config against a real
pandas-stubs install. CI's `python-lint-types` cannot see them: `bin/typecheck.sh`
prefers `uvx mypy`, which runs mypy in an isolated ephemeral env with none of the
locked stubs, so every pandas symbol degrades to `Any` (see #1791 for the full
triage — CI's own pinned pandas-stubs 3.0.3.260530 already reports 140 errors).

1. graphviz: `KeyError: None` on a bound-but-unlabelled frame

   `g_to_pgv` asserts `_nodes`/`_edges` are set but not that the *bindings* are.
   `g.nodes(df)` with no `node=` (or `g.edges(df)` with no source/destination)
   leaves them None, and `layout_graphviz`/`render_graphviz` only call
   `materialize_nodes()` when `_nodes` is None — so those graphs reached
   `row[None]` and died with a bare `KeyError: None` from inside the row loop.
   Now resolved once, up front, into non-Optional locals with an actionable
   ValueError. Validation moved ahead of the optional `pygraphviz` import so a
   caller error is not masked by a missing-backend error (and so it is testable
   without the optional dep).

2. search_any: `pat` was bound to BOTH a module and a str in one scope

   `import pandas.api.types as pat` and, 10 lines later, `pat, case = term, ...`.
   Ordering saves it today (the module use strictly precedes the rebind), but it
   is one statement reorder from `AttributeError: 'str' object has no attribute
   'is_bool_dtype'`. Renamed the module alias to `pd_types`.

Also corrects two `dtype: object` params to the engine-agnostic `DType` alias —
these receive pandas/cuDF/polars dtypes, so `object` was both less accurate and
on the repo's banned-annotation list.

mypy differential under CI's own pin (mypy 2.1.0 + pandas-stubs 3.0.3.260530):
140 -> 128, zero new errors, `search_any.py` and `graphviz.py` both to zero.
Under the container stubs (pandas-stubs 3.0.0.260204 + mypy 1.19.1): 173 -> 161.
ruff clean. `graphistry/tests/compute` with `--gpus all`: 9 failed / 7030 passed,
identical to the master baseline (all 9 pre-existing igraph/cudf/dask).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB
@lmeyerov
lmeyerov merged commit c76f23a into master Jul 27, 2026
69 checks passed
lmeyerov added a commit that referenced this pull request Jul 28, 2026
Both landed without a CHANGELOG entry. #1793 is the one that matters: it fixes a
SILENT WRONG ANSWER that is user-visible on every engine.

Reproduced on the pre-fix tree while auditing the merge, so the entry states a
measured effect rather than a description: `MATCH (a {grp:1}) WITH a MATCH
(a)-[]->(b) RETURN count(*)` followed by a plain `MATCH (a)-[]->(b) RETURN
count(*)` on the SAME graph object returned 60 instead of 134, 69 instead of 136
and 61 instead of 143 across seeds. On polars the poisoned graph then raised
`NotImplementedError` for EVERY subsequent query — a failed query left the
user's object broken. Both are fixed on master; neither was written down.

#1792's graphviz half (`KeyError: None` -> an actionable `ValueError` on a
bound-but-unlabelled frame) is smaller but is still a user-facing error-message
change, so it gets an entry too.

DOCS ONLY: no code touched, so runtime delta is zero and no pyg-bench lane run
is required (CB5) — checkable from the diff, which is one file.

NOT INCLUDED, deliberately. The audit also flagged #1792's `dtype: object` ->
`dtype: DType` as a typing WIDENING (`DType = Any`, which is strictly weaker
than `object`, and mypy is indifferent between them here — verified). I am not
reverting it: `graphistry/compute/typing.py` declares `DType` as the repo's
engine-agnostic dtype alias with the comment "Honestly Any -- the concrete type
is engine-dependent", and these parameters do receive pandas/cuDF/polars dtypes,
so the named alias is the documented convention even though it checks as `Any`.
Reverting on the audit's reading would churn against a stated convention on a
judgement call that belongs to the owner. Reported, not silently decided.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant