fix(review): operator-agnostic sanity check + invariant TODOs + dedupe
Code-review follow-ups on the pgvector spike: - self-similarity sanity check now compares to the seed's own self-distance (0 for cosine/L2, ~-1 for inner product <#>) instead of a hardcoded ~0, so it no longer misfires if Immich ever uses a vector_ip_ops index (correctness P3). - mark the deliberate raw-SQLite read in the probe as a spike-only exception and add an M1.5 TODO that the real pgvector reader belongs in shared/photoflow/immich and SQLite access in shared/photoflow/core (project-standards P2 x2). - document join_and_coverage's return shape; extract a _pct() helper to dedupe the coverage-percentage formatting (maintainability P3 x2). Findings doc refreshed from the latest live run (coverage now ~46%, re-run ongoing).
This commit is contained in:
@@ -20,6 +20,8 @@ class Config:
|
|||||||
# Postgres DSN for read-only access to Immich's pgvector embeddings.
|
# Postgres DSN for read-only access to Immich's pgvector embeddings.
|
||||||
# Optional: only the pgvector spike / M1.5 visual-similarity work needs it;
|
# Optional: only the pgvector spike / M1.5 visual-similarity work needs it;
|
||||||
# REST creds (immich_url/api_key) stay required.
|
# REST creds (immich_url/api_key) stay required.
|
||||||
|
# M1.5 TODO: the actual pgvector reader belongs in shared/photoflow/immich
|
||||||
|
# (the only Immich client, per CLAUDE.md) — this field only carries the DSN.
|
||||||
immich_db_url: Optional[str] = None
|
immich_db_url: Optional[str] = None
|
||||||
|
|
||||||
@property
|
@property
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
# pgvector embedding feasibility spike — findings
|
# pgvector embedding feasibility spike — findings
|
||||||
|
|
||||||
**Generated:** 2026-06-27 18:39 UTC by `scripts/pgvector_spike.py`
|
**Generated:** 2026-06-27 18:49 UTC by `scripts/pgvector_spike.py`
|
||||||
**Status:** machine-probed against the live Immich Postgres.
|
**Status:** machine-probed against the live Immich Postgres.
|
||||||
**This is M1.5's contract.** See the design spec:
|
**This is M1.5's contract.** See the design spec:
|
||||||
`docs/superpowers/specs/2026-06-27-pgvector-embedding-spike-design.md`.
|
`docs/superpowers/specs/2026-06-27-pgvector-embedding-spike-design.md`.
|
||||||
@@ -35,14 +35,14 @@ Immich's documented API → **read-only Postgres access (below) is the viable pa
|
|||||||
|
|
||||||
| # | Question | Answer |
|
| # | Question | Answer |
|
||||||
|---|----------|--------|
|
|---|----------|--------|
|
||||||
| 1 | Embeddings readable? | **yes — read 19607 distinct embedding(s)** |
|
| 1 | Embeddings readable? | **yes — read 20727 distinct embedding(s)** |
|
||||||
| 2 | Join to asset table? | **19607/19607 embeddings join to asset via assetId** |
|
| 2 | Join to asset table? | **20727/20727 embeddings join to asset via assetId** |
|
||||||
| 3 | Table / column | `public.smart_search` / `embedding` |
|
| 3 | Table / column | `public.smart_search` / `embedding` |
|
||||||
| 3 | Vector dimension | **1152** (live sample=1152, declared typmod=1152) |
|
| 3 | Vector dimension | **1152** (live sample=1152, declared typmod=1152) |
|
||||||
| 3 | Distance operator | **<=> (cosine) — from index opclass vector_cosine_ops** |
|
| 3 | Distance operator | **<=> (cosine) — from index opclass vector_cosine_ops** |
|
||||||
| 3 | Operator sanity check | OK — nearest neighbour (a duplicate (identical-vector) asset) at distance 0 |
|
| 3 | Operator sanity check | OK — nearest neighbour (a duplicate (identical-vector) asset) at distance 0 (seed self-distance 0) |
|
||||||
| 4 | Coverage (raw) | **42.8%** (19607/45854 assets) |
|
| 4 | Coverage (raw) | **45.2%** (20727/45854 assets) |
|
||||||
| 4 | Coverage (image-only) | **44.1%** (19221/43601 IMAGE assets) |
|
| 4 | Coverage (image-only) | **46.6%** (20323/43601 IMAGE assets) |
|
||||||
|
|
||||||
- **Asset-key join:** FK smart_search.assetId -> asset.id
|
- **Asset-key join:** FK smart_search.assetId -> asset.id
|
||||||
- **Orphan embeddings** (no matching asset): 0
|
- **Orphan embeddings** (no matching asset): 0
|
||||||
|
|||||||
+30
-13
@@ -176,20 +176,24 @@ def self_similarity_ok(conn, schema, table, column, key_col, op) -> str:
|
|||||||
sql.SQL(
|
sql.SQL(
|
||||||
"WITH seed AS (SELECT {key} AS k, {col} AS v FROM {tbl} "
|
"WITH seed AS (SELECT {key} AS k, {col} AS v FROM {tbl} "
|
||||||
" WHERE {col} IS NOT NULL LIMIT 1) "
|
" WHERE {col} IS NOT NULL LIMIT 1) "
|
||||||
"SELECT (e.{key} = seed.k) AS is_seed, (e.{col} {op} seed.v) AS dist "
|
"SELECT (e.{key} = seed.k) AS is_seed, (e.{col} {op} seed.v) AS dist, "
|
||||||
|
" (seed.v {op} seed.v) AS self_dist "
|
||||||
"FROM {tbl} e, seed WHERE e.{col} IS NOT NULL ORDER BY dist ASC LIMIT 5"
|
"FROM {tbl} e, seed WHERE e.{col} IS NOT NULL ORDER BY dist ASC LIMIT 5"
|
||||||
).format(key=key, col=col, tbl=tbl, op=sql.SQL(op))
|
).format(key=key, col=col, tbl=tbl, op=sql.SQL(op))
|
||||||
).fetchall()
|
).fetchall()
|
||||||
if not rows:
|
if not rows:
|
||||||
return "skipped — no embeddings present"
|
return "skipped — no embeddings present"
|
||||||
is_seed, top_dist = rows[0]
|
is_seed, top_dist, self_dist = rows[0]
|
||||||
# Operator works iff an identical vector reads back at distance ~0. The nearest
|
# Operator works iff an identical vector sorts first at the operator's own
|
||||||
# row may be a duplicate photo (also dist 0) rather than the seed itself — still a pass.
|
# self-distance. That value is operator-dependent (0 for cosine <=> / L2 <->,
|
||||||
good = abs(float(top_dist)) < 1e-6
|
# ~-1 for inner product <#>), so compare the nearest distance to the seed's
|
||||||
|
# self-distance rather than to a hardcoded 0. The nearest row may be a duplicate
|
||||||
|
# photo rather than the seed itself — still a pass.
|
||||||
|
good = abs(float(top_dist) - float(self_dist)) < 1e-6
|
||||||
who = "the seed itself" if is_seed else "a duplicate (identical-vector) asset"
|
who = "the seed itself" if is_seed else "a duplicate (identical-vector) asset"
|
||||||
return (
|
return (
|
||||||
f"{'OK' if good else 'UNEXPECTED'} — nearest neighbour ({who}) "
|
f"{'OK' if good else 'UNEXPECTED'} — nearest neighbour ({who}) "
|
||||||
f"at distance {float(top_dist):.6g}"
|
f"at distance {float(top_dist):.6g} (seed self-distance {float(self_dist):.6g})"
|
||||||
)
|
)
|
||||||
except Exception as exc: # noqa: BLE001
|
except Exception as exc: # noqa: BLE001
|
||||||
return f"failed: {exc}"
|
return f"failed: {exc}"
|
||||||
@@ -244,7 +248,13 @@ def detect_asset_key(conn, schema, table) -> tuple[Optional[str], Optional[str],
|
|||||||
|
|
||||||
|
|
||||||
def join_and_coverage(conn, schema, table, key_col, asset_table, assets_pk):
|
def join_and_coverage(conn, schema, table, key_col, asset_table, assets_pk):
|
||||||
"""Join validity + coverage (raw and image-only) inside Postgres."""
|
"""Join validity + coverage (raw and image-only) inside Postgres.
|
||||||
|
|
||||||
|
Returns a dict (the contract its three callers read by key):
|
||||||
|
n_embeddings_distinct, n_joined_to_assets, n_orphan, n_assets_total: int
|
||||||
|
n_assets_image, n_embedded_image: int | None (None when no asset 'type' column)
|
||||||
|
raw_ratio, image_ratio: float | None (None when the denominator is 0/None)
|
||||||
|
"""
|
||||||
tbl, key = sql.Identifier(schema, table), sql.Identifier(key_col)
|
tbl, key = sql.Identifier(schema, table), sql.Identifier(key_col)
|
||||||
atbl, apk = sql.Identifier(schema, asset_table), sql.Identifier(assets_pk)
|
atbl, apk = sql.Identifier(schema, asset_table), sql.Identifier(assets_pk)
|
||||||
|
|
||||||
@@ -300,6 +310,10 @@ def sqlite_crosscheck(conn, schema, table, key_col, sqlite_path: Path) -> str:
|
|||||||
|
|
||||||
Degrades gracefully: a fresh checkout has no `ingest` run, so an empty/absent
|
Degrades gracefully: a fresh checkout has no `ingest` run, so an empty/absent
|
||||||
store is not a failure of the join requirement.
|
store is not a failure of the join requirement.
|
||||||
|
|
||||||
|
NOTE: this opens SQLite directly, a deliberate exception for this throwaway probe.
|
||||||
|
Production M1.5 code must NOT — it must go through shared/photoflow/core/store.py,
|
||||||
|
the only SQLite owner (CLAUDE.md). This raw read-only sample check stays in the spike.
|
||||||
"""
|
"""
|
||||||
if not sqlite_path.is_file():
|
if not sqlite_path.is_file():
|
||||||
return f"skipped — no SQLite store at {sqlite_path} (fresh checkout; not a failure)"
|
return f"skipped — no SQLite store at {sqlite_path} (fresh checkout; not a failure)"
|
||||||
@@ -365,10 +379,15 @@ def environment_meta(conn) -> dict:
|
|||||||
|
|
||||||
|
|
||||||
# -------------------------------------------------------------------------- output
|
# -------------------------------------------------------------------------- output
|
||||||
|
def _pct(ratio: Optional[float]) -> str:
|
||||||
|
"""Format a coverage ratio as a percentage, or 'n/a' when it could not be computed."""
|
||||||
|
return "n/a" if ratio is None else f"{ratio * 100:.1f}%"
|
||||||
|
|
||||||
|
|
||||||
def render_findings(facts: dict) -> str:
|
def render_findings(facts: dict) -> str:
|
||||||
cov = facts["coverage"]
|
cov = facts["coverage"]
|
||||||
raw_pct = "n/a" if cov["raw_ratio"] is None else f"{cov['raw_ratio'] * 100:.1f}%"
|
raw_pct = _pct(cov["raw_ratio"])
|
||||||
img_pct = "n/a" if cov["image_ratio"] is None else f"{cov['image_ratio'] * 100:.1f}%"
|
img_pct = _pct(cov["image_ratio"])
|
||||||
meta = facts["meta"]
|
meta = facts["meta"]
|
||||||
return textwrap.dedent(
|
return textwrap.dedent(
|
||||||
f"""\
|
f"""\
|
||||||
@@ -469,10 +488,8 @@ def print_report(facts: dict) -> None:
|
|||||||
print(f" operator sanity : {facts['self_similarity']}")
|
print(f" operator sanity : {facts['self_similarity']}")
|
||||||
print(f" asset key : {facts['asset_key_note']}")
|
print(f" asset key : {facts['asset_key_note']}")
|
||||||
print(f" join : {facts['join_summary']} (orphans: {cov['n_orphan']})")
|
print(f" join : {facts['join_summary']} (orphans: {cov['n_orphan']})")
|
||||||
raw = "n/a" if cov["raw_ratio"] is None else f"{cov['raw_ratio'] * 100:.1f}%"
|
print(f" coverage (raw) : {_pct(cov['raw_ratio'])} ({cov['n_joined_to_assets']}/{cov['n_assets_total']})")
|
||||||
img = "n/a" if cov["image_ratio"] is None else f"{cov['image_ratio'] * 100:.1f}%"
|
print(f" coverage (image) : {_pct(cov['image_ratio'])} ({cov['n_embedded_image']}/{cov['n_assets_image']})")
|
||||||
print(f" coverage (raw) : {raw} ({cov['n_joined_to_assets']}/{cov['n_assets_total']})")
|
|
||||||
print(f" coverage (image) : {img} ({cov['n_embedded_image']}/{cov['n_assets_image']})")
|
|
||||||
print(f" sqlite x-check : {facts['sqlite']}")
|
print(f" sqlite x-check : {facts['sqlite']}")
|
||||||
print(f" postgres : {facts['meta']['postgres_version']}")
|
print(f" postgres : {facts['meta']['postgres_version']}")
|
||||||
print(f" pgvector ext : {facts['meta']['pgvector_extension']}")
|
print(f" pgvector ext : {facts['meta']['pgvector_extension']}")
|
||||||
|
|||||||
Reference in New Issue
Block a user