fix: resolve each index's own embedding model (serve queries, CLI index/stats/status) - #248
Conversation
|
A big thank-you for this PR, Don — truly excellent work. 🙇 One root cause (the per-index model contract in Two notes from the walkthrough, neither merge-blocking: 1. Needs evaluation — the new 2. Deferred by design (recorded, revisit at the next fastembed bump): the first cold-model load holds the pool's map mutex and briefly stalls all queries — the deliberate trade against doubling the ~2GB ONNX arena. Also noted, pre-existing: And finally: an invitation to join the repo as a collaborator is on its way — work of this quality is very welcome here. Thanks again! 🚀 |
OK I will take a look at the serve model when there is some index already. |
The multi-repo MCP service built its shared embedder from ModelType::default() (384-dim MiniLM) and ignored both --model and the model_short_name every index records. On indexes rebuilt with a 768-dim model (EmbeddingGemma) every semantic query failed with "Query embedding dimension mismatch: expected 768, got 384"; a same-dimension mismatch would have silently compared incomparable vector spaces. The single-repo stdio path already resolved the model from metadata.json (issue flupkede#118) — the serve path never did. - ModelType::from_index_metadata: reader counterpart to write_metadata_fields, used by every query path. - EmbeddingServicePool: lazily loads one EmbeddingService per model, shared across MCP sessions and REST handlers, so a hub can hold mixed-model indexes and each query uses its target's model. - CodesearchService::query_model / embedding_service_for replace the single shared Option<EmbeddingService>. - semantic_search and semantic_search_multi resolve the model per repo; the fan-out helper now passes the alias to its closure so each store is searched with its own model's query embedding. - CLI warns that serve --model is inert; README documents per-repo resolution. Tests: model_for_alias_* and serve_service_uses_repo_model_not_default (serve), test_model_type_round_trips_through_index_metadata (embed).
…ulting `codesearch index` used `model.unwrap_or_default()`, so re-indexing an existing index built with a non-default model (EmbeddingGemma, 768-dim) picked the 384-dim default: `FileMetaStore` reported "Model changed, full re-index required" and 384-dim vectors were embedded against a 768-dim store. It now resolves the recorded model through the same helper the serve/watcher paths use; an explicit `--model` that disagrees is rejected with a pointer to `--force`. `codesearch stats`, `get_db_stats` and the repo listing opened the vector store with a hardcoded 384, so `Dimensions:` always read 384 for every index — they now read the recorded dimensions. `status(kind="index")` reported the service's own model (the hardcoded default in serve mode) while `dimensions` came from live stats; it now resolves per repo, and reports the common model or "mixed" for a group. Tests: index_model_resolution_tests (recorded_dimensions and resolve_index_model precedence/mismatch/force/fresh), group_status_model_label_is_common_or_mixed.
`codesearch index add --model embeddinggemma-q4` delegates to `POST /repos` when serve is running. The handler opened the store with the default 384-dim dimension and applied the model override to `metadata.json` only afterwards, so the background reindex embedded 768-dim vectors into a 384-dim store and indexed nothing — the `.codesearch.db` directory was created, `stats` reported the new dimension, and zero files were indexed. `try_open_stores` now takes an optional dimension override, and `add_repo_handler` passes the request model's dimensions when one is given (the model parse moved before the store open). All other call sites pass `None` and keep reading the dimension from `metadata.json`. Test: `try_open_stores_honours_dimension_override_for_a_fresh_repo` (observes 384 vs 768 when the override is ignored).
…s built `status(kind="index")` derived readiness only from `total_chunks > 0`, so a repo mid-rebuild — chunks inserted but `build_index()` not yet run — reported `status: "ready"` / "Index is ready for searching." while every search failed with "Index not built. Call build_index() after inserting chunks." Observed live on a 27-repo hub partway through a model migration: `py-opa` had 403 chunks, `indexed: false`, and still read as ready. - Single-store path: new `single_index_status(total_chunks, indexed)`. - Group path: `index_status_summary` takes `all_indexed`; a store that failed to report stats is tracked separately so a failure is never misread as "not built" (it still surfaces as degraded-ready with `warnings`). Tests: `index_status_summary_reports_building_when_chunks_exist_but_graph_is_not_built` and `single_index_status_requires_a_built_graph_to_report_ready` (both fail `ready` vs `building` when the built-graph signal is ignored).
`codesearch serve --model X` now sets a serve-wide default embedding model for indexes created through serve. `POST /repos` (including `index add` delegated to a running serve) uses it when the request carries no explicit model and the index records none; an existing index keeps its recorded model, and an explicit `model` in the request still wins. Serve reports the default as `default_model` in `GET /status`, at startup, and as the fallback query model for a repo whose metadata records none. Adds resolve_add_repo_model() to pin the precedence, with tests covering the new-index default, the recorded-model-wins case, and the service fallback.
00b38af to
ee36102
Compare
`serve --model X` was used as the query fallback for a repo whose `metadata.json` records no model. That overrode a legacy index the moment the flag was set: a 384-dim index queried with a 768-dim model failed every search with "Query embedding dimension mismatch: expected 384, got 768", and a same-dimension model compared incomparable vector spaces without erroring. Resolve such an index to the built-in default instead: that is both the historical behaviour and the value every other reader assumes for metadata without a `model_short_name`. `serve --model` keeps doing what its name says, selecting the model for newly created indexes. Surface the assumption to the caller as a search warning naming the repo, the assumed model and the re-index command, and log it once per repo so a busy hub does not repeat the same line. The scope-free status summary still reports the serve default.
|
Thanks for the careful read, and for walking the whole thing through the audit. We addressed the first note in You were right about the fallback. When a repo's What we did:
The second note, where the first cold-model load holds the pool's map mutex, is deferred by design. We have not touched it here. We agree it belongs with the next fastembed bump. The pre-existing |
flupkede
left a comment
There was a problem hiding this comment.
Audited end-to-end (5 items walked, every criterion pinned by a regression test) and validated on the final head: fmt clean, clippy -D warnings clean, 716+712 tests green. The needs-evaluation point is settled by 654fd6d - the serve default no longer leaks into the query fallback for unrecorded indexes, and the caller-facing warning plus once-per-repo log are exactly right. Excellent work - thank you!
This PR fixes a family of bugs with one root cause: a code path using
ModelType::default()(384-dim MiniLM) — or a hardcoded384— instead of resolving the embedding model/dimensions each index records in itsmetadata.json. The indexing side already followed the per-index contract; the query, CLI, status, and served-add paths did not.1. Serve queries embedded with the default model
Symptom. On a
codesearch servehub whose indexes were rebuilt withembeddinggemma-q4(768-dim), every semantic query failed withQuery embedding dimension mismatch: expected 768, got 384.status(kind=index)reported the hub model asminilm-l6-q, and--modelon serve did nothing.Cause.
CodesearchService::new_for_servehardcodedmodel_type: ModelType::default(); its sharedembedding_servicewas built from that;run_serve(...)has no model parameter, so--modelwas silently inert. The single-repo stdio path already resolved the model frommetadata.json(issue #118) — serve never did.Fix. Resolve the query model per routed repo, with one embedder per model:
ModelType::from_index_metadata(db_path)— reader counterpart towrite_metadata_fields.EmbeddingServicePool— lazily loads oneEmbeddingServiceper model, shared across MCP sessions and REST handlers.ServeState::model_for_alias()+embedding_pool();CodesearchService::query_model(alias)/embedding_service_for(model).semantic_searchresolves the model from the routed alias;semantic_search_multiembeds once per distinct model and searches each store with its own embedding (with_vector_store_read_multinow passes the alias to its closure).serve --modelis inert; README documents per-repo resolution.2.
POST /reposwith--modelcreated the store at the wrong dimensionSymptom.
codesearch index add --model embeddinggemma-q4 <path>(which delegates toPOST /reposwhen serve is running): the.codesearch.dbdirectory is created,statsreports the new dimension, but no files are indexed.Cause.
add_repo_handleropened the store inline viatry_open_stores, which read the dimension frommetadata.json(absent → 384 default), and parsedbody.modelonly afterwards — applying the override to metadata but never to the store. The background reindex then embedded 768-dim vectors into a 384-dim store and indexed nothing.Fix.
try_open_storestakes an optional dimension override;add_repo_handlerparses the model first and passesmodel.dimensions()for a fresh/being-rebuilt store. All other call sites passNoneand keep readingmetadata.json.3. CLI
indexdowngraded non-default indexesSymptom.
codesearch index <path>on a repo indexed withembeddinggemma-q4embedded against it with the 384-dim default (FileMetaStorelogged "Model changed, full re-index required" and wiped its metadata);codesearch index addon an already-registered repo did nothing (it only registers).Cause.
index_with_optionsdidlet model_type = model.unwrap_or_default();and never calledresolve_embed_model(db_path)— the helper the serve/watcher paths use whose own doc says "Every embedding path resolves the model through this helper instead ofModelType::default()".Fix. New
resolve_index_model(db_path, force, requested): on an existing index the recorded model wins; an explicit--modelthat disagrees is rejected with a pointer to--force.resolve_embed_modelis nowpub(crate)so the CLI reuses it.4.
statshardcoded 384 dimscodesearch stats,get_db_statsand the repo listing opened the vector store asVectorStore::new(&db_path, 384), soDimensions:always read384and the store was opened with the wrong dimension. They now read the recorded dimensions viarecorded_dimensions(db_path); the delete-old-chunks path usesmodel_type.dimensions().5.
status(kind="index")reported the service defaultThe
modelfield wasself.model_type(the hardcoded default in serve mode) whiledimensionscame from live store stats, so every repo readminilm-l6-q. It now resolves per repo forproject=, and reports the common model — ormixed— for a group (group_model_label).6.
statusreportedreadyfor an index that cannot be searchedSymptom. On a 27-repo hub partway through a model migration,
status(kind="index", project="py-opa")reportedindexed: falsebutstatus: "ready"/ "Index is ready for searching." — whilesearch(project="py-opa")failed withIndex not built. Call build_index() after inserting chunks.Cause. Readiness keyed only off
total_chunks > 0; the HNSW graph state (stats.indexed) was ignored. Chunks-inserted-but-graph-not-built is exactly the mid-rebuild window an operator watches.Fix. Single-store path uses a new
single_index_status(total_chunks, indexed); the group path'sindex_status_summarytakesall_indexed. A store that failed to report stats is tracked separately, so a failure is never misread as "not built" (it still surfaces as degraded-ready withwarnings).Tests
model_for_alias_reads_the_index_metadata_model,..._is_none_without_index_metadata,..._is_per_repo_not_hub_wide,serve_service_uses_repo_model_not_default,group_status_model_label_is_common_or_mixed,try_open_stores_honours_dimension_override_for_a_fresh_repo.index::index_model_resolution_tests::{recorded_dimensions_reads_metadata_dimensions, recorded_dimensions_falls_back_to_model_then_default, resolve_index_model_prefers_recorded_model_on_existing_index, resolve_index_model_rejects_disagreeing_override_without_force, resolve_index_model_uses_requested_model_for_a_fresh_index}.test_model_type_round_trips_through_index_metadata.index_status_summary_reports_building_when_chunks_exist_but_graph_is_not_built,single_index_status_requires_a_built_graph_to_report_ready.Proof rule. Reintroducing each defect and watching the relevant test fail before reverting:
query_modelreturningself.model_type→serve_service_uses_repo_model_not_defaultfails (left: AllMiniLML6V2Q, right: EmbeddingGemma300MQ4).resolve_index_modelignoring the recorded model →resolve_index_model_prefers_recorded_model_on_existing_indexand..._rejects_disagreeing_override_without_forcefail.try_open_storesignoring the dimension override →try_open_stores_honours_dimension_override_for_a_fresh_repofails (left: 384, right: 768).left: ready, right: building).Validation
cargo check --all-targets,cargo clippy --all-targets -- -D warnings,cargo test --lib --bins. 702 passed, 13 ignored. The single failure,watch::tests::test_no_ignore_files_returns_none, is unrelated and pre-existing on this host (also fails on unmodifieddevelop). The pre-push QC hook could not run here becausepwshis not installed; the equivalentcargo fmt+ clippy + test gate was run manually.