This repository was archived by the owner on Oct 4, 2026. It is now read-only.
Repository navigation
feat(api): background image embedding indexer service - #35
Merged
Merged
Conversation
lstein
marked this pull request as draft
August 2, 2026 22:23
lstein
force-pushed
the
feat/image-map-02-indexer
branch
from
August 2, 2026 22:38
a970db9 to
1bc499a
Compare
lstein
force-pushed
the
feat/image-map-02-indexer
branch
from
August 3, 2026 01:38
1bc499a to
c5cc29c
Compare
lstein
marked this pull request as ready for review
August 3, 2026 18:28
lstein
force-pushed
the
feat/image-map-02-indexer
branch
2 times, most recently
from
August 4, 2026 05:37
7ea889c to
5ca27a0
Compare
lstein
force-pushed
the
feat/image-map-01-schema
branch
from
August 4, 2026 05:37
b3f4618 to
712ed53
Compare
lstein
force-pushed
the
feat/image-map-02-indexer
branch
3 times, most recently
from
August 5, 2026 00:31
8412295 to
ad8d3f1
Compare
Adds the worker that keeps the image embedding index current: - New `ImageIndexService`: daemon worker thread fed by the image service's on_changed/on_deleted callbacks (enqueue-only on the caller's thread) plus a startup backfill pass over unembedded gallery images. Images that fail to load or encode are skipped and remembered so they cannot stall the backfill. - Embeds via the configured CLIP Vision or SigLIP model. On GPU the model loads through the shared model cache and indexing pauses while a generation is in progress; `image_index_device: cpu` keeps a service-local CPU copy instead, avoiding the cache and VRAM entirely. - New config: `image_index_enabled` (default off), `image_index_model` (default clip-vit-large-patch14), `image_index_device`, `image_index_batch_size`. - New broadcast-safe `image_index_status` socket event with aggregate counts. - Changing the configured model discards embeddings from the old model. Tests use an injected fake encoder; no models or GPU are touched in CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… batch PR #34 hardened the storage layer to reject embeddings that cannot be L2-normalized. The indexer's normalization predates that and deliberately let zero-norm rows through: norms[norms == 0] = 1.0 embeddings = embeddings / norms A zero vector survives that unchanged, so `upsert_embedding` now raises ValueError, the broad handler around the upsert loop catches it, and the *entire* batch is routed to `_record_failure` — retried `_MAX_ATTEMPTS` times and then marked permanently failed. One degenerate image cost every image it shared a batch with, and `pending` never reached zero. Degenerate rows (zero or non-finite norm) are now dropped before normalizing and only their own names are failed. `loaded_names` shrinks, so the batch correctly reports partial success and the backfill is re-armed to retry them. The norm is computed in float64. In float32 the sum of squares under/overflows well inside the range of the vector itself — components uniformly below ~1e-22 give norm 0.0 and above ~1.8e19 give inf — which would misclassify perfectly normalizable rows as unusable and permanently fail them. Deliberately *not* done: catching ValueError per-image around the upsert. That reads as the natural granular fix, but it strands the image — the batch still reports full success, the backfill is never re-armed, and the image sits at one attempt, unembedded and never retried, with `pending` stuck above zero. The existing retry contract only survives because every raise reaches the outer handler. Covered by a regression test using ValueError specifically, since the pre-existing test raises RuntimeError and so never exercised that path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
force-pushed
the
feat/image-map-02-indexer
branch
from
August 8, 2026 18:27
ad8d3f1 to
b2e74fc
Compare
…d model resolution
Adversarial review of the indexer service turned up four defects, all in code
this PR introduces. The feature is default-off, so none of them can reach a
user today; they are traps for whoever first enables it.
**Failure attribution (F2).** `_MAX_ATTEMPTS` exists so one bad file cannot
spin the backfill forever, but every failure was charged against it — including
failures of the machinery. A model outage lasting three sweeps therefore
retired every image it touched, and since nothing clears `_failed` but a
successful embed, reinstalling the model did not bring them back. Only a
restart did.
Repetition cannot tell the two apart: an outage and a poisonous image both fail
every time. So the encoder is asked directly. On a whole-batch failure,
`_encoder_is_healthy()` probes it with a trivial image:
- probe fails -> the machinery is down. No image is charged; they stay pending
and the sweep retries them once it recovers.
- probe succeeds -> the batch contents broke it. Charge the images, so a
poisonous one is quarantined and the backlog can advance past it.
That second half matters as much as the first: backfill is oldest-first, so
never charging images would trade this bug for a permanently wedged sweep.
Storage failures ("database is locked") are systemic outright — a database
being down is no more the images' fault than a missing model. Because an
outage no longer retires anything, systemic failures now back off
exponentially (1s to a 60s ceiling) instead of retrying at 1 Hz, resetting on
the first successful store.
`test_bad_encoder_output_marks_batch_failed` asserted the old conflation — that
a permanently broken encoder quarantines the image — and now asserts the
opposite.
**Device resolution (F1).** `_device()` built a `torch.device` from
`image_index_device`, a free-form config string with no validator. A near-miss
like `CPU` raised inside the worker loop before any batch was attempted, so
`_MAX_ATTEMPTS` never applied and the worker spun on the same exception
forever with pending stuck above zero. Both callers only ever asked whether
this is CPU mode — the GPU path follows the model cache — so the device object
was never needed. `_cpu_mode()` compares a normalized string instead, making
the failure unreachable rather than caught. A value that is not understood is
ignored, as the config documents, but now says so in the log.
**Model resolution (F4, F5).** `search_by_attr` drops its name predicate for a
falsy name, so an empty `image_index_model` adopted an arbitrary installed
model — and `start()` then discarded every embedding computed by the model the
user meant. Guarded, as `_model_not_installed_message` already did. Separately,
names are not unique and `search_by_attr` breaks ties by insertion order, so
reinstalling a duplicate moved the winner, changed the model hash, and wiped
the index; the pick is now ordered by key.
**Coverage.** `_make_invoker` hardcoded device/session_queue/model_manager,
which is why these paths had no tests. Parameterizing it showed
`delete_embeddings_for_other_models` — the only destructive operation here —
was untested, and `_wait_for_idle_generation` returned at its first statement
in 100% of runs, leaving the entire "don't thrash VRAM during generation"
contract dead under test. Both are covered now, along with each fix above;
every new test is verified to fail against the unfixed code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`pending` deliberately excludes images that were given up on, so that a permanently-failing image cannot wedge the index short of quiescence. That makes `pending` unable to express the difference between a finished index and one that quietly skipped images: a consumer sees `pending == 0` with `embedded < total` and has no way to explain the gap. A UI gating "indexing complete" on `pending == 0` would confidently render complete over a gallery with holes. `ImageIndexStatus` already tracked `failed`; it just never left the process. The service exposes no REST route, so the status event was the only path out, and it forwarded only total/embedded/pending. It now carries `failed` too, and the three numbers together make the state legible. `openapi.json` and `schema.ts` are updated to match. Both were hand-edited in place rather than regenerated: CI generates then runs prettier over the result, and without node_modules the raw generator output reformats the entire file (~6.5k lines changed) for a two-key addition. The edited openapi.json is verified to parse equal to the generator's output, and the schema.ts entry follows how openapi-typescript renders a required field carrying a default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
This was reviewed multiple times by Claude/Opus and I believe it to be ready for merge. |
CI failed on linux-cpu and windows-cpu (py3.11 and py3.12); macOS passed.
`test_worker_waits_for_generation_to_finish_when_not_on_cpu` passes no
`image_index_device`, so CPU mode is decided by autodetection. A host with an
accelerator reports cuda/mps, the worker waits, and the assertion holds. A
CPU-only host — every linux-cpu and windows-cpu runner — is in CPU mode, where
`_wait_for_idle_generation` returns immediately by design, so the image embeds
and `embedded == 0` fails. macOS reports mps and passed, which is why only two
of the three platforms broke.
The service is behaving correctly in both cases: falling through to
autodetection is what `image_index_device` documents ("any other value is
ignored"), and skipping the wait on a CPU-only host is the intended behavior —
there is no VRAM to protect. The test was wrong to assume the host has an
accelerator, so it now pins the device.
`test_generation_wait_does_not_block_shutdown` needed the same fixture. It was
passing on CPU-only runners, but only because it never entered the wait it
exists to test.
Verified both directions: with autodetection forced to CPU the suite reproduced
CI's failure exactly and now passes (71/71), and it still passes unmodified on
an accelerator host.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 8, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Stacked on #34 (PR 2 of the Image Map stack). Adds the worker that keeps the embedding index current:
ImageIndexService: daemon worker thread fed by the image service'son_changed/on_deletedcallbacks (enqueue-only — they fire synchronously on the generation thread) plus a startup backfill over unembedded gallery images.image_index_model, defaultclip-vit-large-patch14). On GPU, the model loads through the shared model cache and indexing pauses while a generation is in progress.image_index_device: cpukeeps a service-local CPU copy instead — zero VRAM contention.preprocessor_config.json(true of InvokeAI-published CLIP Vision models — same convention as IP-Adapter/FLUX); inputs are converted to the cached model's dtype (fp16 on GPU); transient failures are retried up to 3× with backoff before an image is marked failed; the dedup set is released in afinallyso no image can get stuck unindexable;stop()logs if the worker doesn't join; double-start()is guarded.image_index_enabled(default off),image_index_model,image_index_device,image_index_batch_size.image_index_status(aggregate counts) — routed to the admin room only, since the counts aggregate over all users' images.An adversarial review of this diff was run; both critical findings (broken processor loading on stock models, GPU dtype mismatch) plus the retry/poisoning,
_pending-leak, and event-privacy findings are fixed above. Remaining accepted trade-offs: CPU mode holds its model copy in RAM while the service lives;image_index_devicevalues other thancpudefer to the model cache (documented in the config description).Testing
10 service tests with an injected fake encoder (CI never loads models): backfill with eligibility filtering, callback-driven indexing, unloadable-image skip, transient-failure retry-to-success, bad-output batch failure, processor-fallback, status events, clean stop.
🤖 Generated with Claude Code