ci: switch test-postgres service to apache/age for KG tests - #216
Merged
Conversation
The test-postgres job ran on `pgvector/pgvector:pg16`, which ships pgvector but no AGE. `tests/test_knowledge_graph_age.py` (19 integration tests, all gated on `TEST_POSTGRES_DSN`) was therefore connecting to a postgres that lacked the AGE extension and silently producing zero coverage of `mempalace.knowledge_graph_age`. Production (`mempalace-db` on familiar) runs on `apache/age:release_PG16_1.6.0` with `postgresql-16-pgvector` apt-installed on top — same pattern this PR mirrors into CI. Changes: - Service image: `pgvector/pgvector:pg16` → `apache/age:release_PG16_1.6.0` - New step: `docker exec` into the running service container and apt-install `postgresql-16-pgvector` (the AGE image is just `postgres:16` + AGE, so the standard PG apt repo has the package). - Extension setup creates both `vector` and `age` extensions. - Pytest selector adds `tests/test_knowledge_graph_age.py` to the job. `tests/test_age_kg_units.py` is DSN-free and already runs in test-linux — not re-run here. Coverage of `mempalace.knowledge_graph_age` should rise once the AGE integration tests actually exercise. The combined coverage gate (78%, set by #126) won't break: this only adds covered lines.
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
This was referenced May 26, 2026
jphein
added a commit
that referenced
this pull request
May 26, 2026
* fix(age): omit NULL property-map keys in add_triple Cypher (#221) Cypher property maps reject bare NULL as a value — AGE raises SyntaxError: a name constant is expected and silently drops the write. The static templates in KnowledgeGraphAGE.add_triple and kg_triple_worker._add_triple_cypher emitted valid_from: NULL, valid_to: NULL, source: NULL whenever the LLM didn't supply those bounds, which during backfill meant every triple without an explicit temporal interval was lost at write time. Build the property map keys dynamically: only emit a key when its value is non-None. Reading the property back as r.valid_from after the row exists still returns NULL on the omitted side, which matches the open-interval semantics the rest of the API already assumes (query_triples as_of filter, stats current_facts counter). Five new tests: - test_age_kg_units: three monkeypatched fake-conn tests on the KnowledgeGraphAGE.add_triple path covering (None / partial / full). - test_kg_triple_worker: two pure-string tests on the worker's _add_triple_cypher helper covering (None / set). - test_knowledge_graph_age: two @Pgmark integration tests against the apache/age service container (#216), covering valid_from=None and source=None. Reverting either source file makes the unit tests fail with "property map should not contain NULL: source: NULL, valid_from: NULL, valid_to: NULL". * chore: fix lint + docs drift on age-null PR - scripts/kg-backfill-status.py: split E401 multi-import - README.md: bump test count 3234 → 3250 (new AGE NULL tests) - regenerate website/public/llms-full.txt and python-api/ Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(age): render graph name as literal in cypher() calls AGE's cypher(name, ...) first argument must be a literal *name constant*. psycopg3 binds %s as a server-side $1 parameter which AGE rejects with "a name constant is expected at character 22" — psycopg2 (the previous driver) client-side-substituted %s into the SQL text, so the literal made it through. After the psycopg3 cutover in #208, every AGE write silently failed: add_triple raised, the worker logged it, and zero triples were persisted. The triple-extraction backfill was extracting but writing nothing. Render the graph name into the SQL text with psycopg.sql.SQL + Literal. Concatenate the dollar-quoted Cypher body via SQL() so embedded { braces from MERGE/CREATE patterns aren't interpreted as format() placeholders (we never call .format()). Fixes the AGE-integration tests on test-postgres (test_age_add_triple_*, test_age_query_triples_*, test_age_clear_*, test_age_stats_*) and the silent triple-write failure on the live backfill workers. Also fix ruff format check on scripts/kg-backfill-status.py (missing blank line after module docstring). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(age): inline graph name as quoted literal, keep test-linux portable The previous attempt (psycopg.sql.SQL + Literal) worked against postgres but broke test-linux which mocks the psycopg driver and never installs the [postgres] extra — importing psycopg.sql at call-time raised ModuleNotFoundError, and the unit tests grep for substrings in the recorded SQL text which doesn't work against a Composed object. Switch to plain f-string interpolation. AGE_GRAPH_NAME is a controlled constant ("mempalace_kg") and the helper validates it against a tight identifier regex before substitution, so there's no injection surface. The output is the same shape psycopg2 used to produce client-side: SELECT * FROM cypher('mempalace_kg', $mp_age_q$ ... $mp_age_q$) AS (ok agtype) AGE accepts that as a name constant. psycopg3 sends it via the simple query path with no binds. Verified: 112 postgres + unit tests pass locally. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(kg-extract): hand-roll AsyncBarrier for python 3.10 compat asyncio.Barrier is 3.11+. The CI matrix runs 3.10/3.11/3.13, so the test failed on 3.10 with AttributeError. Replace with a counter+Event barrier that preserves the same "all N parties must arrive" semantic. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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.
Summary
The
test-postgresCI job ran onpgvector/pgvector:pg16— pgvector yes, AGE no.tests/test_knowledge_graph_age.py(19 integration tests, all gated onTEST_POSTGRES_DSN) was therefore connecting to a Postgres that lacked the AGE extension and silently producing zero coverage ofmempalace/knowledge_graph_age.py.This PR mirrors the production substrate (
mempalace-dbon familiar —apache/age:release_PG16_1.6.0withpostgresql-16-pgvectorapt-installed on top) into CI so the KG-AGE integration tests actually exercise.What changes
pgvector/pgvector:pg16→apache/age:release_PG16_1.6.0docker execinto the running service container and apt-installpostgresql-16-pgvector. The AGE image ispostgres:16+ AGE compiled in, so the standard PG apt repo has the package.vectorandageextensions.tests/test_knowledge_graph_age.py.tests/test_age_kg_units.pyis DSN-free and already runs intest-linux— not re-run here.Why now
Surfaced while investigating apparent absence of CI on recently-merged PRs (#207 / #213). Triage showed CI is in fact running on
pgvector/pgvector:pg16; the AGE integration suite was the actual gap.What runs after this lands
Out of scope
Test plan
Install pgvector into AGE service containerstep succeeds, andtest-postgresruns both test files with no skips beyond the existing per-test ones.check-coveragegate still passes (the threshold was set against a state where AGE integration tests didn't run — adding their coverage should only push it up).