From 7c51d4c32f33f67e6909f29dfc068a645ecd108f Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Thu, 16 Jul 2026 15:59:20 -0400 Subject: [PATCH] feat: batch image deletion with a single embeddings-index rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting N starred images from the favorites menu previously issued N serial delete_image requests, each of which loaded and atomically rewrote the entire .npz index — O(N x index size) I/O that made multi-select deletes crawl on large albums. - New POST /delete_images/{album_key} endpoint deletes the files (or routes each through InvokeAI for board albums), reports per-item errors, and drops all rows in one rewrite. - Embeddings.remove_images_from_embeddings() maps all sorted-order indices against one snapshot of the (modtime, filename) lexsort, so callers need no reverse-order shifting; the singular method now delegates to it. The rewrite carries over non-per-image keys (model_id etc.) so the encoder stamp survives. - deleteBookmarkedImages() now sends one request and dispatches the albumChanged deletion event with the indices actually removed. Co-Authored-By: Claude Fable 5 --- photomap/backend/embeddings.py | 46 ++++--- photomap/backend/routers/index.py | 98 +++++++++++++++ .../frontend/static/javascript/bookmarks.js | 24 ++-- photomap/frontend/static/javascript/index.js | 14 +++ tests/backend/test_index.py | 113 ++++++++++++++++++ tests/backend/test_invokeai_board_index.py | 62 ++++++++++ tests/frontend/index-api.test.js | 26 +++- 7 files changed, 354 insertions(+), 29 deletions(-) diff --git a/photomap/backend/embeddings.py b/photomap/backend/embeddings.py index beff42ab..457ba471 100644 --- a/photomap/backend/embeddings.py +++ b/photomap/backend/embeddings.py @@ -1926,6 +1926,22 @@ def remove_image_from_embeddings(self, index: int) -> None: """ Remove an image from the embeddings file. """ + self.remove_images_from_embeddings([index]) + + def remove_images_from_embeddings(self, indices: list[int]) -> None: + """ + Remove a batch of images from the embeddings file in one rewrite. + + Args: + indices: Sorted-order indices (the ones the API exposes) of the + images to remove. All indices refer to the sort order BEFORE + any removal — callers must not pre-shift them. + + The whole batch costs one .npz load and one atomic rewrite, instead + of one full load/rewrite per image as repeated single deletions do. + """ + if not indices: + return try: # 1. Load data explicitly without using the cache wrapper # This ensures we get a fresh copy to work on @@ -1935,22 +1951,24 @@ def remove_image_from_embeddings(self, index: int) -> None: modtimes = data["modification_times"].copy() metadata = data["metadata"].copy() extras = _copy_non_per_image_keys(data) - # Reconstruct sorting locally to find correct index. Must match - # the (modtime, filename) lexsort used in ``_open_npz_file`` or - # we'd find the wrong file to delete. + # Reconstruct sorting locally to find correct indices. Must + # match the (modtime, filename) lexsort used in + # ``_open_npz_file`` or we'd find the wrong files to delete. sorted_indices = np.lexsort((filenames, modtimes)) - sorted_filenames = filenames[sorted_indices] - current_filename = sorted_filenames[index] - - # 2. Find index in the arrays - original_idx = np.where(filenames == current_filename)[0][0] + # 2. Map sorted-order indices to positions in the raw arrays. + # All lookups happen against the same snapshot, so the batch + # needs no reverse-order index-shifting dance. + for index in indices: + if index < 0 or index >= len(sorted_indices): + raise IndexError(f"Index {index} out of bounds for embeddings file.") + original_indices = sorted_indices[np.asarray(indices, dtype=np.intp)] - # 3. Remove from all arrays - filenames = np.delete(filenames, original_idx) - embeddings = np.delete(embeddings, original_idx, axis=0) - modtimes = np.delete(modtimes, original_idx) - metadata = np.delete(metadata, original_idx) + # 3. Remove from all arrays in one pass + filenames = np.delete(filenames, original_indices) + embeddings = np.delete(embeddings, original_indices, axis=0) + modtimes = np.delete(modtimes, original_indices) + metadata = np.delete(metadata, original_indices) # 4. Clear Cache immediately (Before touching disk) _open_npz_file.cache_clear() @@ -1972,7 +1990,7 @@ def remove_image_from_embeddings(self, index: int) -> None: _open_npz_file(self.embeddings_path) except Exception as e: - logger.error(f"Error removing image: {e}") + logger.error(f"Error removing images: {e}") raise def update_image_path(self, index: int, new_path: Path) -> None: diff --git a/photomap/backend/routers/index.py b/photomap/backend/routers/index.py index 923bb38e..3ecb56c5 100644 --- a/photomap/backend/routers/index.py +++ b/photomap/backend/routers/index.py @@ -49,6 +49,11 @@ class UpdateIndexRequest(BaseModel): album_key: str +class DeleteImagesRequest(BaseModel): + indices: list[int] + move_to_trash: bool = True + + class MoveImagesRequest(BaseModel): indices: list[int] target_directory: str @@ -340,6 +345,99 @@ async def delete_image( raise HTTPException(status_code=500, detail=f"Failed to delete file: {str(e)}") from e +@index_router.post( + "/delete_images/{album_key}", + tags=["Index"], + dependencies=[Depends(require_no_lock)], +) +async def delete_images( + album_key: str, + req: DeleteImagesRequest, + album_config: AlbumDep, + embeddings: EmbeddingsDep, +) -> JSONResponse: + """Delete a batch of images with a single embeddings-index rewrite. + + Repeated calls to ``delete_image`` rewrite the entire .npz once per + image; this endpoint removes the files first and then drops all of + their rows in one rewrite, so a multi-select delete costs the same + index I/O as a single deletion. + """ + if not req.indices: + raise HTTPException(status_code=400, detail="No indices provided") + + try: + indices = sorted(set(req.indices)) + + # Resolve every index against one snapshot of the sort order before + # anything is deleted — the indices all refer to the pre-delete + # ordering, so nothing here needs reverse-order shifting. + image_paths: dict[int, Path] = {} + errors: list[str] = [] + for index in indices: + try: + image_paths[index] = embeddings.get_image_path(index) + except IndexError: + errors.append(f"Index {index}: out of range") + + deleted_indices: list[int] = [] + deleted_files: list[str] = [] + for index, image_path in image_paths.items(): + try: + if not validate_image_access(album_config, image_path): + errors.append(f"Index {index}: Access denied") + continue + + if album_config.source_type == "invokeai_board": + # Board images belong to InvokeAI: deleting the file + # directly would leave a dangling row in InvokeAI's + # database, so route each deletion through its API + # (which also removes the file). ``move_to_trash`` has + # no meaning here and is ignored. + await invokeai_client.delete_image( + album_config.invokeai_url, + image_path.name, + album_config.invokeai_username, + album_config.invokeai_password, + ) + else: + if not image_path.exists() or not image_path.is_file(): + errors.append(f"Index {index}: File not found") + continue + if req.move_to_trash: + send2trash(str(image_path)) + else: + image_path.unlink() + + deleted_indices.append(index) + deleted_files.append(image_path.name) + except Exception as e: + logger.error(f"Error deleting image at index {index}: {e}") + errors.append(f"Index {index}: {str(e)}") + + # One rewrite for the whole batch — this is the entire speedup. + if deleted_indices: + embeddings.remove_images_from_embeddings(deleted_indices) + + response_data = { + "success": len(deleted_indices) > 0 or len(errors) == 0, + "deleted_count": len(deleted_indices), + "deleted_indices": deleted_indices, + "deleted_files": deleted_files, + } + if errors: + response_data["errors"] = errors + response_data["error_count"] = len(errors) + + return JSONResponse(content=response_data, status_code=200) + + except HTTPException: + raise + except Exception as e: + logger.error(f"Failed to delete images: {e}") + raise HTTPException(status_code=500, detail=f"Failed to delete images: {str(e)}") from e + + @index_router.post( "/move_images/{album_key}", tags=["Index"], diff --git a/photomap/frontend/static/javascript/bookmarks.js b/photomap/frontend/static/javascript/bookmarks.js index db4d2b43..3c02f454 100644 --- a/photomap/frontend/static/javascript/bookmarks.js +++ b/photomap/frontend/static/javascript/bookmarks.js @@ -4,6 +4,7 @@ import { showDeleteConfirmModal } from "./control-panel.js"; import { createSimpleDirectoryPicker } from "./filetree.js"; +import { deleteImages } from "./index.js"; import { showConfirmModal } from "./modal-utils.js"; import { setSearchResults } from "./search.js"; import { slideState } from "./slide-state.js"; @@ -575,18 +576,13 @@ class BookmarkManager { showSpinner(); try { - // Delete images in reverse order to maintain index consistency - const sortedIndices = [...indices].sort((a, b) => b - a); - - for (const globalIndex of sortedIndices) { - try { - await fetchJson( - `delete_image/${encodeURIComponent(state.album)}/${globalIndex}?move_to_trash=${state.moveToTrash}`, - { method: "DELETE" } - ); - } catch { - console.warn(`Failed to delete image at index ${globalIndex}`); - } + // One request for the whole batch: the server deletes the files and + // rewrites the embeddings index once, instead of once per image as + // the old per-image loop forced it to. + const result = await deleteImages(state.album, indices, state.moveToTrash); + const deletedIndices = result.deleted_indices ?? []; + if (result.errors?.length) { + console.warn("Some images could not be deleted:", result.errors); } // Trigger album refresh BEFORE clearing bookmarks @@ -596,9 +592,9 @@ class BookmarkManager { new CustomEvent("albumChanged", { detail: { album: state.album, - totalImages: slideState.totalAlbumImages - indices.length, + totalImages: slideState.totalAlbumImages - deletedIndices.length, changeType: "deletion", - deletedIndices: indices, + deletedIndices, }, }) ); diff --git a/photomap/frontend/static/javascript/index.js b/photomap/frontend/static/javascript/index.js index 58ba6d8c..005b0dbe 100644 --- a/photomap/frontend/static/javascript/index.js +++ b/photomap/frontend/static/javascript/index.js @@ -40,6 +40,20 @@ export async function deleteImage(albumKey, index, moveToTrash = true) { } } +// Batch delete: one request and a single embeddings rewrite on the server, +// instead of one full .npz rewrite per image. Returns the server's summary, +// including ``deleted_indices`` (the subset that actually got removed). +export async function deleteImages(albumKey, indices, moveToTrash = true) { + try { + return await fetchJson(`delete_images/${encodeURIComponent(albumKey)}`, { + json: { indices, move_to_trash: moveToTrash }, + }); + } catch (e) { + console.warn("Failed to delete images."); + throw e; + } +} + // Given an album key, returns metadata about the index, including number of images. // On any failure, dispatches ``albumIndexError`` so the album manager can // react (e.g. start auto-indexing on 404). The errorType distinguishes diff --git a/tests/backend/test_index.py b/tests/backend/test_index.py index 17a083f1..70dc3497 100644 --- a/tests/backend/test_index.py +++ b/tests/backend/test_index.py @@ -168,6 +168,119 @@ def test_npz_rewrites_preserve_non_per_image_keys(tmp_path: Path): _open_npz_file.cache_clear() +def test_delete_images_batch( + client: TestClient, new_album: dict, monkeypatch: pytest.MonkeyPatch +): + """The batch endpoint deletes several images in one request, drops all of + their index rows in a single rewrite, and keeps the encoder stamp.""" + build_index(client, new_album) + album_key = new_album["key"] + + # Resolve which files the sorted indices refer to BEFORE deleting. + targets = {} + for idx in (0, 2, 4): + targets[idx] = client.get(f"/retrieve_image/{album_key}/{idx}").json()["filename"] + survivor = client.get(f"/retrieve_image/{album_key}/1").json()["filename"] + + # move_to_trash=False for the same determinism reason as test_delete_image. + response = client.post( + f"/delete_images/{album_key}", + json={"indices": [0, 2, 4], "move_to_trash": False}, + ) + assert response.status_code == 200, response.text + result = response.json() + assert result["success"] is True + assert result["deleted_count"] == 3 + assert sorted(result["deleted_indices"]) == [0, 2, 4] + assert "errors" not in result + + metadata = client.get(f"/index_metadata/{album_key}").json() + assert metadata["filename_count"] == TEST_IMAGE_COUNT - 3 + + directory = Path(new_album["image_paths"][0]) + for filename in targets.values(): + assert not (directory / filename).exists(), f"{filename} should be deleted" + assert (directory / survivor).exists(), "non-selected image must survive" + + # The single batch rewrite must carry the encoder stamp over, exactly + # like the per-image path — losing it breaks every subsequent search. + config = get_config_manager().get_album(album_key) + with np.load(Path(config.index), allow_pickle=True) as data: + assert str(data["model_id"]) == new_album["encoder_spec"] + remaining = {Path(str(f)).name for f in data["filenames"]} + assert survivor in remaining + assert remaining.isdisjoint(targets.values()) + + +def test_delete_images_batch_reports_bad_indices( + client: TestClient, new_album: dict, monkeypatch: pytest.MonkeyPatch +): + """Out-of-range indices are reported per-item; the valid ones still + delete and the index shrinks only by the successful count.""" + build_index(client, new_album) + album_key = new_album["key"] + + response = client.post( + f"/delete_images/{album_key}", + json={"indices": [0, 9999], "move_to_trash": False}, + ) + assert response.status_code == 200, response.text + result = response.json() + assert result["success"] is True + assert result["deleted_count"] == 1 + assert result["deleted_indices"] == [0] + assert result["error_count"] == 1 + assert "9999" in result["errors"][0] + + metadata = client.get(f"/index_metadata/{album_key}").json() + assert metadata["filename_count"] == TEST_IMAGE_COUNT - 1 + + +def test_delete_images_batch_rejects_empty_list( + client: TestClient, new_album: dict, monkeypatch: pytest.MonkeyPatch +): + build_index(client, new_album) + response = client.post( + f"/delete_images/{new_album['key']}", + json={"indices": []}, + ) + assert response.status_code == 400 + + +def test_remove_images_batch_maps_sorted_indices_and_keeps_extras(tmp_path: Path): + """``remove_images_from_embeddings`` receives sorted-order indices (the + ones the API exposes) that all refer to the pre-delete ordering. It must + map them through the (modtime, filename) lexsort against ONE snapshot — + no reverse-order shifting — and its single rewrite must carry over every + non-per-image key (model_id and anything added later).""" + encoder_spec = "openai-clip:ViT-B/32" + npz_path = tmp_path / "embeddings.npz" + # Raw storage order deliberately differs from sorted order: lexsorting by + # (modtime, filename) yields [c, a, b, d]. + np.savez( + npz_path, + embeddings=np.arange(8, dtype=np.float32).reshape(4, 2), + filenames=np.array([str(tmp_path / f"{name}.jpg") for name in "abcd"]), + modification_times=np.array([2.0, 3.0, 1.0, 4.0]), + metadata=np.array([{}, {}, {}, {}], dtype=object), + model_id=np.array(encoder_spec), + future_key=np.array("still here"), + ) + emb = Embeddings(embeddings_path=npz_path, encoder_spec=encoder_spec) + + # Sorted indices 0 and 2 are c.jpg and b.jpg. + emb.remove_images_from_embeddings([0, 2]) + + with np.load(npz_path, allow_pickle=True) as data: + names = {Path(str(f)).name for f in data["filenames"]} + assert names == {"a.jpg", "d.jpg"} + assert len(data["embeddings"]) == 2 + assert str(data["model_id"]) == encoder_spec + assert str(data["future_key"]) == "still here" + + _open_npz_file.cache_clear() + + # test that we can move images def test_move_images( client: TestClient, new_album: dict, monkeypatch: pytest.MonkeyPatch, tmp_path: Path diff --git a/tests/backend/test_invokeai_board_index.py b/tests/backend/test_invokeai_board_index.py index 358fc2dd..312d0275 100644 --- a/tests/backend/test_invokeai_board_index.py +++ b/tests/backend/test_invokeai_board_index.py @@ -229,6 +229,68 @@ async def broken_delete(base_url, image_name, username, password): assert metadata["filename_count"] == 4 +def test_delete_images_batch_routes_through_invokeai(client, board_album, monkeypatch): + """The batch endpoint deletes each board image through InvokeAI's API + (never touching the files directly) and drops all their index rows in + one rewrite.""" + _build_index(client) + + deleted_names = [] + + async def fake_delete(base_url, image_name, username, password): + deleted_names.append(image_name) + + monkeypatch.setattr(invokeai_client, "delete_image", fake_delete) + + targets = { + idx: Path(client.get(f"/retrieve_image/{ALBUM_KEY}/{idx}").json()["filename"]).name + for idx in (0, 1) + } + + response = client.post(f"/delete_images/{ALBUM_KEY}", json={"indices": [0, 1]}) + assert response.status_code == 200, response.text + result = response.json() + assert result["deleted_count"] == 2 + assert set(deleted_names) == set(targets.values()) + + # The local files are NOT touched directly — InvokeAI owns them. + for name in targets.values(): + assert (board_album["images_dir"] / name).exists() + # But the index rows are gone. + metadata = client.get(f"/index_metadata/{ALBUM_KEY}").json() + assert metadata["filename_count"] == 2 + assert set(targets.values()).isdisjoint( + _index_filenames(board_album["index_path"]) + ) + + +def test_delete_images_batch_invokeai_failure_keeps_failed_row( + client, board_album, monkeypatch +): + """A per-image InvokeAI failure is reported as an error and its index row + survives; the rest of the batch still deletes.""" + _build_index(client) + + calls = [] + + async def flaky_delete(base_url, image_name, username, password): + calls.append(image_name) + if len(calls) == 1: + raise HTTPException(status_code=502, detail="backend down") + + monkeypatch.setattr(invokeai_client, "delete_image", flaky_delete) + + response = client.post(f"/delete_images/{ALBUM_KEY}", json={"indices": [0, 1]}) + assert response.status_code == 200, response.text + result = response.json() + assert result["deleted_count"] == 1 + assert result["error_count"] == 1 + assert len(result["deleted_indices"]) == 1 + + metadata = client.get(f"/index_metadata/{ALBUM_KEY}").json() + assert metadata["filename_count"] == 3 + + def test_move_images_rejected_for_board_albums(client, board_album, tmp_path): _build_index(client) diff --git a/tests/frontend/index-api.test.js b/tests/frontend/index-api.test.js index baceb6b0..f52aa993 100644 --- a/tests/frontend/index-api.test.js +++ b/tests/frontend/index-api.test.js @@ -13,7 +13,7 @@ jest.unstable_mockModule(`${M}/utils.js`, () => ({ })); const { fetchJson } = await import(`${M}/utils.js`); -const { updateIndex } = await import(`${M}/index.js`); +const { deleteImages, updateIndex } = await import(`${M}/index.js`); beforeEach(() => { fetchJson.mockReset(); @@ -51,3 +51,27 @@ describe("updateIndex", () => { } }); }); + +describe("deleteImages", () => { + test("posts the whole batch in one request and returns the summary", async () => { + const response = { success: true, deleted_count: 3, deleted_indices: [1, 5, 9] }; + fetchJson.mockResolvedValue(response); + + const result = await deleteImages("my album", [1, 5, 9], false); + + expect(result).toBe(response); + expect(fetchJson).toHaveBeenCalledTimes(1); + expect(fetchJson).toHaveBeenCalledWith("delete_images/my%20album", { + json: { indices: [1, 5, 9], move_to_trash: false }, + }); + }); + + test("defaults move_to_trash to true and rethrows failures", async () => { + fetchJson.mockRejectedValue(new Error("boom")); + + await expect(deleteImages("alb", [0])).rejects.toThrow("boom"); + expect(fetchJson).toHaveBeenCalledWith("delete_images/alb", { + json: { indices: [0], move_to_trash: true }, + }); + }); +});