fix(extract): skip Python primitive/library type-annotation nodes#1332
fix(extract): skip Python primitive/library type-annotation nodes#1332MasterKinjalk wants to merge 1 commit into
Conversation
|
Thanks @MasterKinjalk — the idea (skip primitive/library type names so they don't become noise nodes) is sound, but a few things need addressing before this can merge:
Also please rebase onto current |
Type annotations (return_type, parameter_type, field, generic_arg, base class) materialize language primitives and common library types -- str, int, Tensor, ndarray, Path, Module, np, torch -- as graph nodes. They carry no domain meaning and become spurious high-degree hubs that pollute community clustering and clutter the report's node list. _LANGUAGE_BUILTIN_GLOBALS already filtered these at call sites but not at type-reference sites, which route through ensure_named_node. What this does - Add _PYTHON_TYPE_REF_PRIMITIVES (builtin globals + stdlib/typing/NumPy/ PyTorch annotation types) and gate it per language via _TYPE_REF_PRIMITIVES_BY_LANG / _type_ref_primitives(ts_module). - _extract_generic skips node creation for these names only when the language is Python; the id is still returned so reference edges resolve (dangling-to-external, already tolerated by build.py). - Add a regression test per affected extractor (Python/Rust/Julia/Go): primitive dropped, a real same-named user type kept, edge still resolves. Why it is scoped to Python (review Graphify-Labs#1332) The first cut applied one flat primitive set in every extractor. That over-filters: the same bare names are ordinary user-defined types in other languages -- a Rust `Response`, a Julia `Module`, a Go `Set` -- and a name-only membership test silently dropped them. Same-file definitions were never at risk (ensure_named_node returns the stem-qualified id for in-file symbols; only the bare cross-file/external id is gated), but the foreign-name application was still wrong, so it is removed. Why Go is not guarded, and the hand-written walkers are reverted - Go already filters its predeclared types at the collector (_GO_PREDECLARED_TYPES in _go_collect_type_refs), so it never materializes int/string/error as nodes; adding the flat guard would only over-filter Go user types. Rust likewise skips `primitive_type` nodes and Python scalars are dropped by _PYTHON_ANNOTATION_NOISE (Graphify-Labs#1147). Predeclared-type filtering belongs at the per-language collector, not a global membership test. - The Julia/Fortran/Rust/PowerShell/Obj-C walkers therefore revert to a plain `if nid not in seen_ids`. Their extractor tests intentionally assert primitive type refs resolve as nodes (PowerShell `string`/`void`, Julia `Float64`, Obj-C `NSString`), so filtering them there would both regress those tests and contradict established behaviour. Net effect: the noise-hub fix lands for Python (the motivating case) without mis-dropping real types in any other language. Rebased onto v8. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pQL3KWPSPtYQZj22wK95m
6cd2781 to
557d4df
Compare
|
Thanks for the detailed review. Rebased onto current Scope reworked to Python-only. The root issue you flagged — a name-only membership test with no language/origin check — drove the redesign. The filter is no longer one flat set applied in every walker; it's keyed by language ( 1. Go / consistency. Go is intentionally excluded, with a reason rather than a guard: 2. Over-filtering / shadow nodes. Two findings:
3. Scope / typed languages. 4. Tests. Added 5. Rebase. Done — rebased onto Full suite shows no new failures vs the v8 base. Happy to adjust scope if you'd prefer the filter extended to other languages' own primitives (that would mean updating those extractors' existing assertions, so I left them as-is). |
Synvoya
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the problem is real and correctly diagnosed. Library/annotation type names (Tensor, ndarray, np, torch, Module, Path, datetime) leaking into the graph as high-degree noise hubs is worth fixing, and the per-language scoping rationale in the description is exactly the right instinct (a flat global list would mis-drop a Rust Response or a Go Set). Two things before it can land, though — the patch as written can't apply to v8, and its mechanism collides with a fix that went in after this branch was cut.
1. The patch no longer applies to v8
git apply --check fails at extract.py:43. This was written against a pre-refactor extract.py: the hunk anchors on an ensure_named_node whose body is
nid = _make_id(name)
if nid not in seen_ids:
add_node(nid, name, line)but on current v8 (extract.py:3504) that function is now nid = _make_id(stem, ".".join(namespace_stack), name) and its body is a sourceless-stub append, not add_node. Needs a rebase before it can be assessed for merge.
2. Suppressing the node here would regress the #1402 phantom-duplicate fix
This is the blocking concern. On v8, ensure_named_node deliberately materializes a sourceless stub for cross-file type references (extract.py:3510-3525):
# The name isn't defined in this file, so this is a cross-file reference ...
# Emit a SOURCELESS stub ... so the corpus-level rewire can collapse it onto
# the real definition. A sourced stub here ... blocks the rewire, which is
# the phantom-duplicate-node bug (#1402).The PR's change is "don't create the node at all, just return the id." For an imported real type (from .models import Dataset, used only in an annotation), that removes the very stub the corpus rewire keys on — so instead of collapsing onto the real Dataset definition, the reference edge now points at an id with no node and no rewire target. That reintroduces #1402 for real imported types. The same sourceless-stub pattern is load-bearing at seven sites (3516, 7690, 8007, 8270, 8625, 8989, 12274), so a suppression at ensure_named_node fights all of them.
3. The scalar names in _PYTHON_TYPE_REF_PRIMITIVES are already filtered upstream
Python parameter/return/generic annotations don't reach ensure_named_node raw — they go through _python_collect_type_refs (extract.py:651), which already drops _PYTHON_ANNOTATION_NOISE (str, int, float, bool, bytes, bytearray, complex, object, True, False, at 641) and _PYTHON_TYPE_CONTAINERS before the emit sites at 4384-4401. So int, str, bytearray, complex, object in the new set can never fire — they're filtered a layer earlier. The genuinely-new coverage is only the library names (Tensor, ndarray, np, torch, Module, Dataset, NDArray, Path, datetime, …).
Suggested direction: drop the ensure_named_node change and instead extend the existing collector filter — add the library/annotation names to _PYTHON_ANNOTATION_NOISE (or a sibling _PYTHON_LIBRARY_ANNOTATION_NOISE frozenset unioned into the same guards at 651-698). That filters the noise at the same point Python scalars are already filtered, keeps the fix Python-scoped without the _TYPE_REF_PRIMITIVES_BY_LANG indirection, and never suppresses a stub the #1402 rewire depends on — a name dropped at the collector never creates an edge in the first place, so there's no dangling target.
One caveat on that path: a name like Path or Dataset can legitimately be a user's own class. Filtering it at the collector means a real project-internal class Dataset referenced in an annotation would also lose that annotation edge. _PYTHON_ANNOTATION_NOISE already accepts that trade for scalars; worth a sentence on why it's acceptable for these library names too (they're overwhelmingly third-party), or gating on whether the name resolves to a local definition.
4. Tests assert behavior this diff doesn't change
tests/test_type_ref_primitives.py asserts Rust/Julia/Go outcomes, but the diff only adds a Python key to _TYPE_REF_PRIMITIVES_BY_LANG — every other language resolves to frozenset() and is untouched, so those cases test pre-existing collector behavior, not this change. The one test that exercises the new path (test_python_primitive_type_refs_not_materialized) is the one to keep; consider folding it into tests/test_languages.py to match the per-language convention the rest of the suite (and the merge-onto-v8 flow) uses.
Net: correct problem, right scoping instinct, but (2) is a real regression risk and (1) blocks the merge. Moving the filter to the collector resolves both and shrinks the diff. Happy to re-review after a rebase.
Problem
Type annotations (
return_type,parameter_type,field,generic_arg, base class) materialize Python language primitives and common library types —str,int,Tensor,ndarray,Path,Module,np,torch— as graph nodes. They carry no domain meaning and become spurious high-degree hubs that pollute community clustering and clutter the report's node list.Cause
_LANGUAGE_BUILTIN_GLOBALSalready filtered these names at call sites, but not at type-reference sites, which route throughensure_named_node.Fix (scoped to Python — see review thread)
_PYTHON_TYPE_REF_PRIMITIVES=_LANGUAGE_BUILTIN_GLOBALS∪ {stdlib / typing / NumPy / PyTorch annotation types}, gated per language via_TYPE_REF_PRIMITIVES_BY_LANG/_type_ref_primitives(ts_module)._extract_genericskipsadd_nodefor these names only when the language is Python. The node id is still returned, so reference edges still resolve (dangling-to-external, tolerated bybuild.py).Why Python-only
The same bare names are ordinary user-defined types in other languages (a Rust
Response, a JuliaModule, a GoSet). A single flat membership test applied in every extractor silently dropped them. Languages that need to suppress their own primitives already do so at the collector, scoped to the language — Go via_GO_PREDECLARED_TYPES, Rust by skippingprimitive_typenodes, Python scalars via_PYTHON_ANNOTATION_NOISE(#1147). This PR layers the library/annotation names on top, for Python only.Tests
tests/test_type_ref_primitives.py— per affected extractor (Python / Rust / Julia / Go): primitive dropped, a real same-named user type kept, edge still resolves, plus the cross-language over-filter regression.Verification
Extracting a sample with
Tensor/ndarray/intannotations:Tensor,ndarray,int,np,torchappear as nodes.Net,.forward(),.to_np(),count()); no primitive leakage.Rebased onto
v8(9e0b876). Full suite: no new failures vs the v8 base.🤖 Generated with Claude Code