Skip to content

Commit c76f23a

Browse files
lmeyerovclaude
andauthored
fix(typing): two real defects that CI's type gate cannot see (#1791) (#1792)
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). Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 4415116 commit c76f23a

3 files changed

Lines changed: 58 additions & 10 deletions

File tree

graphistry/compute/gfql/search_any.py

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
import re
1111
from typing import List, Optional
1212

13-
from graphistry.compute.typing import DataFrameT, SeriesT
13+
from graphistry.compute.typing import DataFrameT, DType, SeriesT
1414

1515
# inspector's numeric-term gate (streamgl-viz sortAndFilterRowsByQuery.js)
1616
_NUMERIC_TERM_RE = re.compile(r"^[0-9.\-]+$")
@@ -20,12 +20,12 @@ def is_numeric_term(term: str) -> bool:
2020
return bool(_NUMERIC_TERM_RE.match(term))
2121

2222

23-
def _is_searchable_string_dtype(dtype: object) -> bool:
23+
def _is_searchable_string_dtype(dtype: DType) -> bool:
2424
import pandas.api.types as pat # cuDF mirrors the pandas dtype API
2525
return bool(pat.is_string_dtype(dtype)) or bool(pat.is_object_dtype(dtype))
2626

2727

28-
def _is_int_dtype(dtype: object) -> bool:
28+
def _is_int_dtype(dtype: DType) -> bool:
2929
import pandas.api.types as pat
3030
return bool(pat.is_integer_dtype(dtype))
3131

@@ -93,11 +93,14 @@ def search_any_mask(
9393
# truncate) and temporal is unverified — decline honestly rather than
9494
# silently mismatch the pandas oracle (wave-3 W3-1). string/int/bool render
9595
# identically (bool parity is pinned) and stay native.
96-
import pandas.api.types as pat
96+
# NB: aliased ``pd_types``, not ``pat`` — ``pat`` is rebound below to the search
97+
# PATTERN string, and a module/str double-binding in one scope is one statement
98+
# reorder away from ``AttributeError: 'str' object has no attribute ...``.
99+
import pandas.api.types as pd_types
97100
for c in cols:
98101
dt = df[c].dtype
99102
if not (_is_searchable_string_dtype(dt) or _is_int_dtype(dt)
100-
or bool(pat.is_bool_dtype(dt))):
103+
or bool(pd_types.is_bool_dtype(dt))):
101104
raise NotImplementedError(
102105
"cuDF searchAny explicit columns support string/int/bool dtypes "
103106
"only (float/temporal stringification diverges from pandas); "

graphistry/plugins/graphviz.py

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,31 @@ def g_to_pgv(
2727
include_positions: bool = False
2828
) -> AGraph:
2929

30-
import pygraphviz as pgv
31-
3230
assert g._nodes is not None
3331
assert g._edges is not None
3432

33+
# The id/endpoint BINDINGS are Optional and are not implied by the frames being set:
34+
# ``g.nodes(df)`` (no ``node=``) / ``g.edges(df)`` (no source/destination) leave them
35+
# None, and ``layout_graphviz``/``render_graphviz`` only ``materialize_nodes()`` when
36+
# ``_nodes`` is None -- so an explicitly-bound-but-unlabelled nodes frame reached
37+
# ``row[None]`` below and died with a bare ``KeyError: None`` from deep inside the row
38+
# loop. Resolve them once, up front, into non-Optional locals.
39+
node_col = g._node
40+
src_col = g._source
41+
dst_col = g._destination
42+
if node_col is None:
43+
raise ValueError(
44+
"graphviz export requires a bound node id column; "
45+
"use g.nodes(df, node='id') or g.materialize_nodes()"
46+
)
47+
if src_col is None or dst_col is None:
48+
raise ValueError(
49+
"graphviz export requires bound edge endpoint columns; "
50+
"use g.edges(df, source='src', destination='dst')"
51+
)
52+
53+
import pygraphviz as pgv
54+
3555
graph = pgv.AGraph(directed=directed, strict=strict)
3656

3757
node_attr_cols: Set[NodeAttr] = {
@@ -58,7 +78,7 @@ def g_to_pgv(
5878
if pos_cols is not None:
5979
attrs['pos'] = f"{row[pos_cols[0]]},{row[pos_cols[1]]}"
6080
graph.add_node(
61-
row[g._node],
81+
row[node_col],
6282
**attrs
6383
)
6484

@@ -75,8 +95,8 @@ def g_to_pgv(
7595
edges_pdf = ensure_pandas(g._edges)
7696
for _, row in edges_pdf.iterrows():
7797
graph.add_edge(
78-
row[g._source],
79-
row[g._destination],
98+
row[src_col],
99+
row[dst_col],
80100
**{c: row[c] for c in edge_attr_cols if row[c] is not None}
81101
)
82102

graphistry/tests/plugins/test_graphviz.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,3 +156,28 @@ def test_render_graphviz_bytes(self, tree_g: Plottable) -> None:
156156

157157
svg_bytes = render_graphviz(tree_g, "dot", format="svg", max_nodes=100, max_edges=200)
158158
assert b'<svg' in svg_bytes
159+
160+
161+
class Test_graphviz_bindings:
162+
"""#1791: the id/endpoint bindings are Optional and are NOT implied by the frames
163+
being set. Before the fix these reached ``row[None]`` and raised a bare
164+
``KeyError: None`` from inside the row loop. No pygraphviz needed: the bindings are
165+
validated before the optional backend import."""
166+
167+
def test_unbound_node_column_raises_value_error(self) -> None:
168+
# .nodes(df) with no node= leaves _node None while _nodes is set, so
169+
# layout_graphviz/render_graphviz do NOT materialize_nodes() over it
170+
g = graphistry.edges(
171+
pd.DataFrame({'s': ['a'], 'd': ['b']}), 's', 'd'
172+
).nodes(pd.DataFrame({'id': ['a', 'b']}))
173+
assert g._node is None
174+
with pytest.raises(ValueError, match='bound node id column'):
175+
g_to_pgv(g)
176+
177+
def test_unbound_edge_endpoints_raise_value_error(self) -> None:
178+
g = graphistry.edges(
179+
pd.DataFrame({'s': ['a'], 'd': ['b']})
180+
).nodes(pd.DataFrame({'id': ['a', 'b']}), 'id')
181+
assert g._source is None and g._destination is None
182+
with pytest.raises(ValueError, match='bound edge endpoint columns'):
183+
g_to_pgv(g)

0 commit comments

Comments
 (0)