Skip to content

fix(extract): distinguish a broken .sql grammar from a missing one (#2602) - #2686

Closed
ousamabenyounes wants to merge 1 commit into
Graphify-Labs:v8from
ousamabenyounes:fix/issue-2602
Closed

fix(extract): distinguish a broken .sql grammar from a missing one (#2602)#2686
ousamabenyounes wants to merge 1 commit into
Graphify-Labs:v8from
ousamabenyounes:fix/issue-2602

Conversation

@ousamabenyounes

Copy link
Copy Markdown
Contributor

Summary

Fix #2602

.sql files were being skipped with tree_sitter_sql not installed. Run: pip install "graphifyy[sql]" even when the grammar was installed — the suggested command is a no-op and the whole SQL layer silently vanished from the graph.

Root cause: an installed-but-broken grammar (e.g. a tree-sitter-sql wheel built for a different Python ABI — the reporter is on Python 3.14) raises ImportError at import time exactly like a genuinely-absent module. extract_sql's single except ImportError mapped both to "not installed", sending the user to a no-op pip install and hiding the real failure.

Fix:

No behaviour change for a genuinely-absent grammar: it still reports not installed with the graphifyy[sql] install hint.

Test verification (RED → GREEN)

Deterministically reproduced on Linux / Python 3.13 by forcing import tree_sitter_sql to raise a load-time ImportError while the module remains installed (find_spec sees it).

RED (on the unmodified v8, new tests applied):

FAILED tests/test_extract.py::test_extract_sql_reports_load_failure_not_missing
FAILED tests/test_extract.py::test_extract_warns_sql_grammar_failed_to_load
AssertionError: assert 'failed to load' in 'tree_sitter_sql not installed. Run: pip install tree-sitter-sql'

GREEN (with the fix):

5 passed, 168 deselected   # 2 new + the 3 existing #1745/sql tests

Full local CI replay (run-ci.sh, mirrors .github/workflows/ci.yml): skillgen checks OK, 4330 passed, 3 skipped. The only failures are 3 pre-existing test_ollama backend-detection tests that also fail on an unmodified v8 checkout (a local ollama daemon skews detection) and are unrelated to this diff.

Files changed

File Change
graphify/extractors/sql.py split the ImportError branch: absent vs installed-but-failed-to-load
graphify/extract.py #1745 warning surfaces load failures with the real cause, no reinstall hint
tests/test_extract.py 2 new regression tests (unit + aggregated warning)
CHANGELOG.md entry under 0.9.41

…raphify-Labs#2602)

An installed-but-broken tree-sitter-sql grammar (e.g. a wheel built for a
different Python ABI) raises ImportError at import time exactly like an absent
one, so extract_sql reported "tree_sitter_sql not installed. pip install ..." —
a no-op that hid the real failure and silently dropped every .sql file.

extract_sql now uses importlib.util.find_spec to tell an absent module from one
that failed to load, and surfaces the actual load exception in the latter case.
The Graphify-Labs#1745 aggregation warning is widened to surface these load failures too
(they carry no "not installed" marker) and omits the misleading reinstall hint.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).


Graphify review — findings

This PR changes how the SQL extractor and the aggregated #1745 dependency warning distinguish a genuinely-absent optional grammar from one that is installed but fails to load (e.g. an ABI-mismatched wheel). In extractors/sql.py, the ImportError handler now uses importlib.util.find_spec to tell the two cases apart, returning either the existing "not installed" message or a new "failed to load" error carrying the real exception. In extract.py, the warning aggregation logic is updated to match both markers, and it now selects the cause text and whether to show the pip install hint based on which case applies. New tests in test_extract.py cover the load-failure path at both the extractor and end-to-end warning levels, and a changelog entry is added.

No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 1838 functions depend on the 708 functions this change touches.

Health — this change adds coupling hotspots:

  • worse: extract() — 439 callers, 41 callees
  • worse: extract_sql() — 9 callers, 8 callees

Verification — 1838 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 1698 function(s) in the blast radius were not formally verified this run

· 2 more finding(s) on lines outside this diff (see the check run).

@safishamsi

Copy link
Copy Markdown
Collaborator

Shipped in v0.9.42 (graphifyy==0.9.42 on PyPI) (#2602). Distinguishes a broken tree-sitter-sql grammar from a missing one. Credited in the release notes. Thanks @ousamabenyounes!

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.

.sql files skipped as "tree_sitter_sql not installed" while the module loads and parses fine

2 participants