feat: Centralised configuration utility and urgent bug fixes#28
Conversation
… with full test suite checked
There was a problem hiding this comment.
_build_filter_positions reads metadata.npz from disk on every call
effectively the same cache-miss problem this PR just fixed for the FAISS index
At 1.8M concepts the .npz file is >100 MB - re-reading per query will dominate latency for filtered searches
Consider adding _metadata_cache: Optional[Tuple[np.ndarray, ...]] attribute alongside _index_cache (holding concept_ids, domain_ids, vocabulary_ids, is_standard, is_valid arrays in memory once loaded), invalidated on the same freshness check that governs _index_cache
freshness check should verify the .npz file mtime, not just .json sidecar.
There was a problem hiding this comment.
This no longer applies. _build_filter_positions and the metadata.npz-based filtering path are gone. Concept filters are now resolved live against the backend (get_concept_ids_matching_filter), and the per-index .faiss/.json freshness check already covers staleness correctly (tested in test_is_fresh_false_after_later_upsert). metadata.npz is kept only for one-time migration of pre-existing legacy caches.
| faiss_index_config=faiss_index_config, | ||
| ) | ||
|
|
||
| def get_embeddings_by_concept_ids( |
There was a problem hiding this comment.
Now that _load_index caches the index in memory, get_embeddings_by_concept_ids can bypass the DB using faiss.IndexIDMap.reconstruct_batch(ids)
current implementation always hits self._backend regardless of FAISS config
for bulk lookups this is a significant unnecessary DB round trip.
Suggested resolution:
add FAISSCache.get_embeddings_by_concept_ids(concept_ids) using index.reconstruct_batch(np.array(concept_ids, dtype=np.int64)), then call from EmbeddingReaderInterface.get_embeddings_by_concept_ids when self._faiss_cache is available / fresh
Prerequisite for the centroid/joint-embedding capability
There was a problem hiding this comment.
Eager-loading at construction vs. lazy-loading on first search() doesn't reduce total work. faiss.read_index runs exactly once either way, this only moves when. That only pays off for a long-lived process that constructs the interface once and serves many requests afterward, and ONLY for the first request as subsequent request are solved through the cache. If the index would change between requests, this pattern could also actively "waste" work: faiss_index_config is supplied per-call to get_nearest_concepts, independent of construction time, so eagerly loading one config on the chance it's the one used later can load an index that's never queried.
| # FAISS fast path activated at construction, not mid-search | ||
| _faiss_dir = faiss_cache_dir or os.getenv(ENV_OMOP_EMB_FAISS_CACHE_DIR) | ||
| _faiss_dir = faiss_cache_dir or OmopEmbConfig.get_config().faiss_cache_dir | ||
| self._faiss_cache: Optional["FAISSCache"] = None |
There was a problem hiding this comment.
FAISSCache._index_cache is initialised to None
first search() call after server startup pays full disk-read cost even though the cache object was just created
this update fixes repeated-call overhead but not cold-start latency, which is user-facing on the first request
Consider adding a FAISSCache.warm(metric_type, index_config) method that calls _load_index eagerly, and calling from EmbeddingReaderInterface.__init__ when self._faiss_cache.is_fresh(model_record, ...) returns true
--> moves load cost to server startup where it is expected and invisible to end users
There was a problem hiding this comment.
Looked into this in depth, and there isn't a version of it that's worth doing due to the following reasons:
COSINE-metric indices storeL2-normalizedvectors, not the raw values the DB returns, so this would be a silent behavior change forCOSINEmodels.- the index wrapper this code actually uses (
IndexIDMap) doesn't supportreconstruct/reconstruct_batchat all in the installed faiss-cpu- only
IndexIDMap2does, meaning a breaking on-disk cache format change and a rebuild of every existing .faiss file - a single missing key fails the whole batch call, so we'd need to reimplement the DB path's per-ID-missing error contract ourselves.
- only
reconstruct_batchalso has no concept of filtering, so anywhere we actually combine filters with ID lookups we're hitting the DB regardless.- The one real caller of
get_embeddings_by_concept_ids(stream_embedding_batches, used by both export and byFAISSCache.build_from_backenditself) calls the backend method directly, never through the interface layer this comment targets- implementing it there wouldn't speed up the one place it's actually used
- Doing it where it would help means pulling FAISS-awareness into the backend classes themselves, which currently have zero knowledge of FAISS by design.
Don't think it is worth it...
| configure_logging_level(verbosity) | ||
| load_dotenv() | ||
|
|
||
| cfg = OmopEmbConfig.get_config() |
There was a problem hiding this comment.
get_config() calls load_stack_config(), which raises FileNotFoundError when ~/.config/omop/config.toml does not exist
tool is completely non-functional on a fresh machine before omop-config configure has been run
even when the user supplies every required parameter on the command line
There was a problem hiding this comment.
resolve_backend() and resolve_omop_cdm_engine() both need config internally and have no CLI override for backend selection or CDM engine connection, so the same FileNotFoundError would just resurface a few lines later regardless of which flags are supplied. Making config genuinely optional isn't something this command structure supports today without also exposing backend/DB overrides on the CLI, which is a bigger change than this fix.
What's actually missing: oa-configurator already validates required resources with an actionable error once a config file exists (points the user at omop-config configure); the only gap is the bare FileNotFoundError when the file is completely absent, which has no such guidance. Wrapping that specific case to point at omop-config configure too, rather than trying to make any of this configuration-free.
gkennos
left a comment
There was a problem hiding this comment.
only these ones to block deployment (either easy or significant)
- silent wrong results every time a user omits --model with a non-default configured model. Easy to hit, silent, wrong output.
- SQLite :memory: fallback - this is just a warning
- concept_filter.limit ignored in sqlite-vec: confirmed behavioural divergence between backends. Callers depending on the limit get wrong counts silently.
- OmopEmbConfig.get_config() unconditional: tool is completely broken on fresh install if omop-config configure hasn't been run - blocks onboarding
Summary
Adapts
omop-embto theoa-configuratorconfiguration layer, replacing environment-variable-based setup with a typed TOML-backed config and aomop-config configure omop_embsubcommand.Notes
Due to the urgency, this PR also absorbed four bug fixes uncovered while doing the migration, plus two more found auditing those fixes:
Includes also a registry staleness gap and a FAISS cache round-tripping gap, neither tracked by an existing issue (see "Additional fixes" below)
Changes
Configuration (#23)
OmopEmbConfigsubclassesPackageConfigBase, exposing all package settings as typed Pydantic fields backed by[tools.omop_emb]in~/.config/omop/config.toml.omop.configsoomop-config configure omop_embprompts interactively or via named flags (--host,--database-name,--backend,--provider-type, …).OmopEmbConfig.get_config()/Resolver.from_active_config()/OmopEmbConfig.configure_logging(verbosity=…)replace the old standalone env-var-driven equivalentsEmbeddingReaderInterfaceno longer reaches into global config itself forfaiss_cache_dir. Instead, it's a plain, fully-explicit constructor argument now (Nonemeans no FAISS, unambiguously). Resolution against the active config moves to the one place that owns it: the CLI (cli_embeddings.py'ssearchcommand) andomop-graph'sKnowledgeGraph. This also means the test suite no longer has an incidental, unrelated dependency on a config file existing on disk.release.yml) now runsomop-config configure omop_embnon-interactively beforepytest, provisioning a real test_emb_db resourcepgvectorintegration tests run for real in CI instead of silently skipping.docker-compose.yamlupdated from--resource-set/--setflags to the current named flags.FAISS index caching + efSearch (#24)
FAISSCache._load_index()re-read the index from disk on every search call, andHNSWIndexConfig.ef_searchwas never applied at query time.(metric_type, index_config)so alternating between multiple metric/index combinations on one instance doesn't thrash a single slotDistance-metric inconsistency, FAISS vs DB backends (#27)
IndexFlatIP(COSINE) returned raw inner product∈ [-1, 1]but was fed directly into a conversion formula expecting cosine distance∈ [0, 2], compressing and sometimes inverting similarity scores relative topgvector/sqlite-vec. -IndexFlatL2similarly returned squared Euclidean distance where the shared formula expected true (non-squared) distance.1 - IPfor cosine,sqrt()for L2), verified against the DB backends with parity tests.OOM on export + silent COSINE corruption (#30)
FAISSCache.export()accumulated every batch into a Python list before a singlevstack, peaking at ~2x dataset size in memory and getting OOM-killed before any disk write on large exports..faissfile itself, which for COSINE holds L2-normalized vectors. Reimporting from it silently discarded original magnitudes.Fix
storage/embedding_bundle.py) as the single source of truthreconstruct_to_bundle()escape hatch covers migrating standalone legacy.faissfiles with a clear data-loss warning.build-faiss-cache.concept_filter) moved from a frozenmetadata.npzsnapshot to a live query against the backend (get_concept_ids_matching_filter), so filtering can never drift from what the database currently says.Additional fixes found during this work
Registry staleness gap: embedding upserts never bumped
model_registry.updated_at, soFAISSCache.is_fresh()could report a cache fresh indefinitely even after new embeddings were added.refresh_model_updated_at_timestamp(), called after every upsert path throughEmbeddingWriterInterface.import_bundle()force-overwrite staleness gap: re-importing into an already-registered model withforce=Truereplaced its content outright but never bumpedupdated_at.updated_atduring this operationRegistry timestamp precision:
refresh_model_updated_at_timestampswitched from DB-sidefunc.now()to a Python-side timestamp, since SQLite'sCURRENT_TIMESTAMPonly has whole-second resolutionUnified how both embedding backends' define and access their table schema, replacing independent hardcoded/string-based column handling with one shared, typed source of truth.