perf(kg-extract): migrate KG triple worker to psycopg3 AsyncConnectionPool - #208
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the KG triple-extraction worker’s psycopg2 + global asyncio.Lock bottleneck by migrating the hot path to psycopg_pool.AsyncConnectionPool, enabling concurrent DB writes per coroutine and avoiding asyncio.to_thread overhead.
Changes:
- Refactors
mempalace/kg_triple_worker.pyto use an async connection pool for queue ops + AGE writes, and introduces async variants of queue helper functions. - Mechanically swaps imports across the codebase to use
psycopg(psycopg3) while preserving legacy_load_psycopg2naming for test seams. - Updates extras in
pyproject.tomlto depend onpsycopg[binary]andpsycopg-pool, and updates worker tests/fakes to support dual sync/async call patterns.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
mempalace/kg_triple_worker.py |
Migrates worker DB hot path to AsyncConnectionPool, adds async queue helpers, and implements direct async AGE triple writes. |
tests/test_kg_triple_worker.py |
Updates fakes to be usable from both sync and async code paths; adapts tests to new async worker helpers. |
pyproject.toml |
Switches postgres/kg-extract extras from psycopg2-binary to psycopg[binary] + psycopg-pool. |
mempalace/backends/postgres.py |
Updates lazy driver import helper to return psycopg3 + psycopg3 sql. |
mempalace/knowledge_graph_age.py |
Updates lazy driver import to psycopg3 and documents legacy naming compatibility. |
mempalace/searcher.py |
Replaces psycopg2 imports with psycopg as psycopg2 aliasing and updates error text. |
mempalace/migrate_to_postgres.py |
Replaces psycopg2 imports with psycopg as psycopg2 aliasing and updates fatal install guidance. |
mempalace/cli.py |
Updates rooms command dependency check/import from psycopg2 to psycopg3. |
mempalace/kg_writethrough.py |
Updates documentation comment to reflect psycopg3 under legacy helper name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+441
to
+445
| # Defense in depth: reject any value carrying the AGE outer | ||
| # dollar-quote tag before the inlining step. ``_cypher_literal`` | ||
| # raises ValueError on hit; we let it bubble up so callers see | ||
| # the offending triple. | ||
| _cypher_literal(subject) |
Comment on lines
+57
to
+63
| AGE_GRAPH_NAME = "mempalace_kg" | ||
| # Same dollar-quote tag KnowledgeGraphAGE uses for its synchronous writes. | ||
| # Kept in sync so adversarial-value checks in ``_cypher_literal`` apply | ||
| # uniformly across both paths. | ||
| _AGE_DQ_TAG = "mp_age_q" | ||
| _AGE_DQ_OPEN = f"${_AGE_DQ_TAG}$" | ||
| _AGE_DQ_CLOSE = f"${_AGE_DQ_TAG}$" |
jphein
added a commit
that referenced
this pull request
May 26, 2026
CI on PR #208 failed two checks: 1. `ruff format --check` flagged three migration-touched files (mempalace/cli.py, mempalace/kg_triple_worker.py, mempalace/migrate_to_postgres.py). All three are cosmetic line-wrap diffs from string literals that exceeded the line limit after the driver-rename swap. Ran `ruff format` to fix. 2. .github/workflows/ci.yml's test-postgres job had an inline `python - <<'PY' ... import os, psycopg2 ...` step that creates the pgvector extension before pytest runs. Since psycopg2-binary is no longer in [postgres], this failed with ModuleNotFoundError. Swapped to `import os, psycopg` + the matching `psycopg.connect(...)`. Sync API is identical (autocommit attribute + cursor().execute()) so no other changes needed. Real connection is still exercised — the step continues to create the vector extension against the live pgvector/pgvector:pg16 service container.
jphein
added a commit
that referenced
this pull request
May 26, 2026
Two CI failures on PR #208: 1. test-postgres: tests/test_backends_postgres.py:129 still did a literal `import psycopg2`. With psycopg2 removed from [postgres], the import raises ModuleNotFoundError before any assertion runs. Swap to `import psycopg as psycopg2` — the rest of the test body (connect, autocommit, cursor, execute, fetchone) is psycopg3-compatible. 2. check-docs: website/reference/python-api/{kg_triple_worker,kg_writethrough}.md were stale after the worker rewrite (714a930) and the writethrough docstring update. Regenerated via scripts/render-api-docs.py. Worker unit tests still 14/14 green. The other test files that import psycopg2 (test_migrate_to_postgres.py, test_palace_graph.py) are not exercised by PR #208's test-postgres job — keeping this commit scoped to the actually-failing surface.
|
Warning Gemini encountered an error creating the summary. You can try again by commenting |
3 tasks
…nPool Replaces psycopg2 + asyncio.Lock with psycopg_pool.AsyncConnectionPool on the KG triple-extraction hot path. Each coroutine now claims its own connection so writes can overlap with the N LLM calls feeding them; the previous single-shared-connection design serialised every postgres write after PR #206 raised LLM throughput ~10x. Worker changes (mempalace/kg_triple_worker.py): - _SyncConnPool retains its name but wraps psycopg_pool.AsyncConnectionPool internally; `configure=` callback runs LOAD 'age' + SET search_path once per fresh connection (was per-cursor before). - _KGHandle.add_triple is async; no asyncio.Lock anywhere. - Pool sized min=max(4, max_concurrency // 2), max=max_concurrency + 2 so steady-state idle capacity is modest but bursts can keep all LLM writers concurrent with the queue claim loop. - Async variants of _claim_batch, _fetch_drawer_text, _mark_completed, _mark_error, _seed_backfill live alongside the sync versions which the CLI --status flag still uses. Driver swap elsewhere is mechanical (`import psycopg as psycopg2`) so the existing test monkeypatch surface keeps working: - backends/postgres.py, knowledge_graph_age.py: _load_psycopg2 still returns the driver+sql modules (now psycopg3) under the legacy name. - searcher.py, migrate_to_postgres.py, cli.py, kg_writethrough.py: aliased imports + error-message updates. Tests: - tests/test_kg_triple_worker.py fakes upgraded to dual-mode sync/async via _NoopAwaitable + _SyncResult helpers — same fakes back both the AsyncConnectionPool hot path and the kept-sync CLI status path. - _FakeKG.add_triple is async with the worker's predicate-positional shape; kg_factory test seam takes the pool, not the dsn. - All 14 kg_triple_worker tests pass; full suite: 3157 passed (up from 3151), 38 pre-existing failures unrelated to psycopg (CLI search output, daemon routing, readme parity, source-adapter entry-point ordering). pyproject.toml: psycopg lines only — psycopg[binary]>=3.2,<4 and psycopg-pool>=3.2,<4 in both [postgres] and [kg-extract] extras. References: #206 (LLM-blob ReDoS fix that made the lock binding).
CI on PR #208 failed two checks: 1. `ruff format --check` flagged three migration-touched files (mempalace/cli.py, mempalace/kg_triple_worker.py, mempalace/migrate_to_postgres.py). All three are cosmetic line-wrap diffs from string literals that exceeded the line limit after the driver-rename swap. Ran `ruff format` to fix. 2. .github/workflows/ci.yml's test-postgres job had an inline `python - <<'PY' ... import os, psycopg2 ...` step that creates the pgvector extension before pytest runs. Since psycopg2-binary is no longer in [postgres], this failed with ModuleNotFoundError. Swapped to `import os, psycopg` + the matching `psycopg.connect(...)`. Sync API is identical (autocommit attribute + cursor().execute()) so no other changes needed. Real connection is still exercised — the step continues to create the vector extension against the live pgvector/pgvector:pg16 service container.
Two CI failures on PR #208: 1. test-postgres: tests/test_backends_postgres.py:129 still did a literal `import psycopg2`. With psycopg2 removed from [postgres], the import raises ModuleNotFoundError before any assertion runs. Swap to `import psycopg as psycopg2` — the rest of the test body (connect, autocommit, cursor, execute, fetchone) is psycopg3-compatible. 2. check-docs: website/reference/python-api/{kg_triple_worker,kg_writethrough}.md were stale after the worker rewrite (714a930) and the writethrough docstring update. Regenerated via scripts/render-api-docs.py. Worker unit tests still 14/14 green. The other test files that import psycopg2 (test_migrate_to_postgres.py, test_palace_graph.py) are not exercised by PR #208's test-postgres job — keeping this commit scoped to the actually-failing surface.
jphein
force-pushed
the
chore/psycopg3-migration
branch
from
May 26, 2026 03:58
55ee44d to
442a73a
Compare
After rebasing onto d2ef152 (PR #209), check-docs flagged two drifts — both downstream of #209's content, picked up here so #208's CI goes green on its own: - website/reference/python-api/sources/registry.md: documents the new reset_discovery() function added in #209's entry-point-pollution fix. - website/public/llms-full.txt: one-line shield bump from #209's README version-badge update (3.3.5 -> 3.3.6 to match version.py). Re-ran `scripts/render-api-docs.py` (81 files, 1 changed) and `scripts/render-llms-full.py` (73417 bytes, 1 line changed). No mempalace/ source touched — pure rendered-artifact sync.
This was referenced May 26, 2026
jphein
added a commit
that referenced
this pull request
May 26, 2026
Five tightly-coupled changes that together unstick KG backfill throughput after PR #208 removed the asyncio.Lock bottleneck: 1. Semaphore narrowed to the LLM call only. Extracted ``_extract_under_sem`` so the ``asyncio.Semaphore`` slot is held during ``extract_triples`` and released before any AGE writes begin. Previously a slow DB write held an LLM slot, starving llama server's ``--parallel`` capacity. 2. Streaming task pool replaces the gather-over-batch barrier. The old loop did claim → ``asyncio.gather(*batch)`` → next claim, so the slowest drawer in each batch stalled the next claim. New model: one producer task tops up an ``asyncio.Queue`` whenever it dips below ``batch_size // 2``; ``max_concurrency`` persistent consumer tasks pull and run ``_process_one``. No batch boundaries. 3. Default knobs bumped for the new floor: - ``DEFAULT_CONCURRENCY`` 8 → 24 (matches llama-server --parallel) - ``_SyncConnPool.max_size`` 10 → 32 (covers 24 LLM slots + slack) 4. New ``--db-pool-size`` CLI flag (env: ``MEMPALACE_KG_DB_POOL_SIZE``). Validated ``db_pool_size >= max_concurrency`` so every in-flight LLM call is guaranteed a write conn. Default is ``max_concurrency + 8``. 5. ``DEFAULT_ENDPOINT`` switched from ``http://familiar.jphe.in:11436`` to ``http://familiar:11436``. The FQDN was stale; bare ``familiar`` resolves via Tailscale for inter-host calls. Tests: 23/23 in tests/test_kg_triple_worker.py — including new coverage for streaming-pool no-barrier behaviour, consumer count = max_concurrency, sem-release-before-DB-write, db-pool-size validation, and the bare-host endpoint default.
jphein
added a commit
that referenced
this pull request
May 26, 2026
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>
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
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
Migrates the KG triple-extraction worker from
psycopg2 + asyncio.Locktopsycopg_pool.AsyncConnectionPool. After PR #206 raised LLM throughput ~10x by killing the JSON-blob ReDoS, the worker's single-shared-connection serialisation lock became the new throughput ceiling. This PR removes it.What changed
Worker hot path (
mempalace/kg_triple_worker.py)_SyncConnPoolkeeps its name (so test factories don't need to be renamed) but its internals are now apsycopg_pool.AsyncConnectionPool. The pool'sconfigure=callback runsLOAD 'age'andSET search_path = ag_catalog, "$user", publiconce per fresh connection, paid once instead of per-cursor._KGHandle.add_tripleis async. Noasyncio.Lockanywhere. Each coroutine grabs its own connection from the pool and writes concurrently.min=max(4, max_concurrency // 2),max=max_concurrency + 2. Modest steady-state, enough headroom for the LLM-writer fleet plus the claim loop._claim_batch,_fetch_drawer_text,_mark_completed,_mark_error,_seed_backfill. The sync versions are kept for the CLI--statuspath (a single short-lived connection — async pool would be overkill).Driver swap elsewhere is mechanical
import psycopg as psycopg2aliasing so existingmonkeypatch.setattr(..., "_load_psycopg2", ...)andpsycopg2.connecttest seams keep working:mempalace/backends/postgres.pymempalace/knowledge_graph_age.pymempalace/searcher.pymempalace/migrate_to_postgres.pymempalace/cli.pymempalace/kg_writethrough.pypyproject.toml— only psycopg lines touched:Pool sizing rationale
min_sizemax(4, max_concurrency // 2)max_sizemax_concurrency + 2max_concurrencyis the LLM concurrency cap. Production runs with 24 → pool min 12, max 26. AGE per-connection setup pays once at first checkout, never again on that physical connection's lifetime.Test plan
tests/test_kg_triple_worker.py— fakes upgraded to dual-mode sync/async via small_NoopAwaitableand_SyncResulthelpers. All 14 tests pass.tests/test_palace_graph.py,tests/test_age_kg_units.py,tests/test_kg_extraction_queue.py,tests/test_backfill_kg_triples.py,tests/test_backends_postgres.py,tests/test_migrate_to_postgres.py— all 120 pass (22 skipped, postgres-required).kg-extract-aurora).References