From 5281117eb153a4e3091e6f8b5140f9c038ef678f Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Tue, 18 Aug 2026 23:13:48 -0400 Subject: [PATCH] fix: tolerate an album with no chosen Cluster Strength MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``umap_eps`` — the semantic map's DBSCAN epsilon, shown in the UI as Cluster Strength — was a required float. A newer PhotoMapAI writes ``umap_eps: null`` for albums that leave the value to the app, and parsing that into a non-nullable field failed the *whole* config load: RuntimeError: Failed to load configuration from config.yaml: 1 validation error for Album umap_eps Input should be a valid number One optional per-album map setting stopped the app from starting, with no way out but hand-editing YAML. Nothing about a config file should be able to do that, and the two builds share the file whenever a user moves between releases. The field is nullable now, meaning "nobody chose one". An unset value is written as an *absent* key rather than an explicit null, so a config this build writes still loads on one that predates the change. Everything that consumes the value resolves it through ``resolve_umap_eps``, which substitutes ``DEFAULT_UMAP_EPS`` — the map and the label endpoints have to agree here, or the cluster ids they return describe different clusterings and the hover labels attach to the wrong blobs. That helper treats zero and negatives as "not chosen" too, because DBSCAN raises on them: ``/set_umap_eps/`` accepted a negative epsilon, stored it, and left the map answering 500 until someone edited the file. It is rejected at the request model now, so the stored value can always be used. Also names the constant. Two places spelled the fallback out and disagreed — the field default said 0.2, the YAML reader said 0.07. Tests: an explicit null, an absent key and a chosen value all load; unset round-trips as an absent key; ``/get_umap_eps`` reports what the map will actually use; ``/umap_data`` and ``/cluster_labels`` cluster an album that never chose one; and a non-positive strength is refused. Backend 675 passed, frontend 578 passed, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- photomap/backend/config.py | 44 +++++- photomap/backend/routers/album.py | 23 ++- photomap/backend/routers/cluster_labels.py | 5 +- photomap/backend/routers/umap.py | 8 +- tests/backend/test_unset_umap_eps.py | 171 +++++++++++++++++++++ 5 files changed, 238 insertions(+), 13 deletions(-) create mode 100644 tests/backend/test_unset_umap_eps.py diff --git a/photomap/backend/config.py b/photomap/backend/config.py index cb297700..e1bc31ac 100644 --- a/photomap/backend/config.py +++ b/photomap/backend/config.py @@ -21,6 +21,25 @@ logger = logging.getLogger(__name__) +# The DBSCAN epsilon the semantic map falls back to when an album has not +# chosen one. Named because three places used to spell it out and two of them +# disagreed (the field default said 0.2, the YAML reader said 0.07). +DEFAULT_UMAP_EPS = 0.07 + + +def resolve_umap_eps(eps: float | None) -> float: + """``eps`` if an album actually chose one, else :data:`DEFAULT_UMAP_EPS`. + + DBSCAN requires a positive epsilon and raises on anything else, so a + stored zero or negative — which older builds accepted through + ``/set_umap_eps/`` — is treated as "not chosen" rather than passed on to + fail the request. Every consumer resolves through here: the map and the + label endpoints must agree, or their cluster ids describe different + clusterings and the hover labels attach to the wrong blobs. + """ + return eps if eps is not None and eps > 0 else DEFAULT_UMAP_EPS + + def default_board_index_path(album_key: str) -> Path: """Index location for albums that have no image directory of their own. @@ -85,7 +104,15 @@ class Album(BaseModel): "id 'none' is InvokeAI's Uncategorized bucket." ), ) - umap_eps: float = Field(default=0.2, description="UMAP epsilon parameter") + umap_eps: float | None = Field( + default=None, + description=( + "DBSCAN epsilon for the semantic map's clustering. None means " + "'not chosen' — the map falls back to DEFAULT_UMAP_EPS — and is " + "how a config written by a newer PhotoMapAI records an album " + "whose Cluster Strength is left to the app." + ), + ) description: str = Field(default="", description="Album description") encoder_spec: str = Field( # Resolved per-host: OpenCLIP ViT-L-14 on CUDA/macOS, lighter OpenAI CLIP @@ -207,7 +234,6 @@ def to_dict(self) -> dict[str, Any]: "source_type": self.source_type, "image_paths": self.image_paths, "index": self.index, - "umap_eps": self.umap_eps, "description": self.description, "encoder_spec": self.encoder_spec, "min_search_score": self.min_search_score, @@ -216,6 +242,12 @@ def to_dict(self) -> dict[str, Any]: "min_image_dimension": self.min_image_dimension, "min_image_bytes": self.min_image_bytes, } + # An unset Cluster Strength is written as an *absent* key rather than + # an explicit null: both mean the same thing here, but a null is what + # older PhotoMapAI versions choke on, and this file is shared with + # them whenever a user moves between releases. + if self.umap_eps is not None: + data["umap_eps"] = self.umap_eps # Keep directory-album YAML free of irrelevant InvokeAI keys. if self.source_type == "invokeai_board": data["invokeai_url"] = self.invokeai_url @@ -235,7 +267,11 @@ def from_dict(cls, key: str, data: dict[str, Any]) -> "Album": source_type=data.get("source_type", "directory"), image_paths=data.get("image_paths", []), index=data["index"], - umap_eps=data.get("umap_eps", 0.07), + # Absent — or an explicit null, which a newer PhotoMapAI wrote + # for a while — means nobody chose one. Refusing to load the whole + # config over that field left users unable to start the app at + # all, which no single album setting should ever be able to do. + umap_eps=data.get("umap_eps"), description=data.get("description", ""), # Legacy YAML albums predate the encoder_spec field; their indexes # were built with the original CLIP, so fall back to that to stay @@ -669,7 +705,7 @@ def create_album( name: str, image_paths: list[str] | None, index: str | None, - umap_eps: float, + umap_eps: float | None = None, description: str = "", encoder_spec: str | None = None, min_search_score: float | None = None, diff --git a/photomap/backend/routers/album.py b/photomap/backend/routers/album.py index cfb23290..2f4aaf26 100644 --- a/photomap/backend/routers/album.py +++ b/photomap/backend/routers/album.py @@ -6,9 +6,15 @@ from fastapi import APIRouter, Depends, HTTPException from fastapi.responses import JSONResponse -from pydantic import BaseModel - -from ..config import Album, create_album, default_board_index_path, get_config_manager +from pydantic import BaseModel, Field + +from ..config import ( + Album, + create_album, + default_board_index_path, + get_config_manager, + resolve_umap_eps, +) from ..embeddings import Embeddings from ..encoders import default_encoder_spec from ..video_cache import VideoFrameCache @@ -16,7 +22,10 @@ class UmapEpsSetRequest(BaseModel): album: str - eps: float + # DBSCAN raises on a non-positive epsilon, so storing one only defers the + # failure to the next time the map is opened — and the stored value is + # then invisible, since every reader resolves it away. + eps: float = Field(gt=0) class UmapEpsGetRequest(BaseModel): @@ -320,7 +329,7 @@ async def update_album(album_data: dict) -> JSONResponse: name=album_data["name"], image_paths=album_data.get("image_paths"), index=index, - umap_eps=album_data.get("umap_eps", 0.07), + umap_eps=album_data.get("umap_eps"), description=album_data.get("description", ""), encoder_spec=album_data.get("encoder_spec"), min_search_score=album_data.get("min_search_score"), @@ -434,6 +443,8 @@ async def get_umap_eps(request: UmapEpsGetRequest): album_config = config_manager.get_album(request.album) if not album_config: raise HTTPException(status_code=404, detail="Album not found") - return {"success": True, "eps": album_config.umap_eps} + # An album that never set one reports the value the map will actually + # use, so the slider opens where the clustering really is. + return {"success": True, "eps": resolve_umap_eps(album_config.umap_eps)} diff --git a/photomap/backend/routers/cluster_labels.py b/photomap/backend/routers/cluster_labels.py index 26017c80..2a5cb62c 100644 --- a/photomap/backend/routers/cluster_labels.py +++ b/photomap/backend/routers/cluster_labels.py @@ -15,6 +15,7 @@ from fastapi.responses import JSONResponse from ..cluster_labels import compute_image_label, get_or_build_cluster_labels +from ..config import resolve_umap_eps from .album import AlbumDep, EmbeddingsDep cluster_labels_router = APIRouter() @@ -49,7 +50,9 @@ async def get_cluster_labels( # ``/umap_data`` resolve to the same value for the same request. # If they disagree, the cluster IDs returned by the two endpoints # diverge and the hover-label feature breaks. - cluster_eps = cluster_eps if cluster_eps is not None else album_config.umap_eps + cluster_eps = ( + cluster_eps if cluster_eps is not None else resolve_umap_eps(album_config.umap_eps) + ) labels = await asyncio.to_thread( get_or_build_cluster_labels, embeddings, diff --git a/photomap/backend/routers/umap.py b/photomap/backend/routers/umap.py index 88f1b1f0..db1d1b74 100644 --- a/photomap/backend/routers/umap.py +++ b/photomap/backend/routers/umap.py @@ -7,7 +7,7 @@ from fastapi.responses import JSONResponse from sklearn.cluster import DBSCAN -from ..config import get_config_manager +from ..config import get_config_manager, resolve_umap_eps from ..media_types import media_type_for from .album import AlbumDep, EmbeddingsDep @@ -43,7 +43,11 @@ async def get_umap_data( # so the two endpoints resolve identical eps values for the same # request — otherwise the cluster IDs they return would disagree # and the hover-label feature would break. - cluster_eps = cluster_eps if cluster_eps is not None else album_config.umap_eps + # ``album_config.umap_eps`` is None for an album whose Cluster Strength + # was never set, so the album's own fallback has a fallback. + cluster_eps = ( + cluster_eps if cluster_eps is not None else resolve_umap_eps(album_config.umap_eps) + ) # Load cached UMAP embeddings (will compute/cache if missing) umap_embeddings = embeddings.umap_embeddings diff --git a/tests/backend/test_unset_umap_eps.py b/tests/backend/test_unset_umap_eps.py new file mode 100644 index 00000000..0bdd14b5 --- /dev/null +++ b/tests/backend/test_unset_umap_eps.py @@ -0,0 +1,171 @@ +"""An album may have no Cluster Strength of its own. + +Newer PhotoMapAI versions derive the semantic map's DBSCAN epsilon when the +user has not set one, and for a while they recorded that as an explicit +``umap_eps: null`` in config.yaml. Parsing that into a non-nullable float made +the *whole* config fail to load — one album's optional map setting stopping +the app from starting at all. These tests pin the tolerant behaviour and the +fallback that replaces it. +""" + +import pytest +import yaml +from fixtures import build_index + +from photomap.backend.config import ( + DEFAULT_UMAP_EPS, + Album, + ConfigManager, + get_config_manager, +) + + +def _config_with(tmp_path, album_extra): + album = { + "name": "Album", + "description": "", + "image_paths": [str(tmp_path)], + "index": str(tmp_path / "i.npz"), + "encoder_spec": "openai-clip:ViT-B/32", + **album_extra, + } + path = tmp_path / "config.yaml" + path.write_text( + yaml.safe_dump({"config_version": "1.0.0", "albums": {"a": album}}, indent=2) + ) + return path + + +def test_explicit_null_cluster_strength_loads(tmp_path): + """The failure this fixes: a config carrying a null refused to load, and + the app could not start until the user hand-edited YAML.""" + config_path = _config_with(tmp_path, {"umap_eps": None}) + + album = ConfigManager(config_path=config_path).get_album("a") + + assert album is not None + assert album.umap_eps is None + + +def test_absent_cluster_strength_loads(tmp_path): + config_path = _config_with(tmp_path, {}) + + assert ConfigManager(config_path=config_path).get_album("a").umap_eps is None + + +def test_chosen_cluster_strength_survives(tmp_path): + config_path = _config_with(tmp_path, {"umap_eps": 0.35}) + + assert ConfigManager(config_path=config_path).get_album("a").umap_eps == 0.35 + + +def test_unset_strength_is_written_as_an_absent_key(tmp_path): + """Not as a null: that is exactly what older versions choke on, and the + config file is shared with them whenever a user moves between releases.""" + album = Album( + key="a", + name="A", + image_paths=[str(tmp_path)], + index=str(tmp_path / "i.npz"), + ) + + assert "umap_eps" not in album.to_dict() + + album.umap_eps = 0.3 + assert album.to_dict()["umap_eps"] == 0.3 + + +def test_get_umap_eps_reports_the_value_the_map_will_use(client, tmp_path): + """The slider has to open where the clustering actually is, so an album + that never chose a strength reports the fallback rather than null.""" + key = "unset_eps_album" + response = client.post( + "/add_album/", + json={ + "key": key, + "name": "Unset Eps", + "image_paths": [str(tmp_path)], + "index": str(tmp_path / "i.npz"), + }, + ) + assert response.status_code == 201, response.text + try: + response = client.post("/get_umap_eps/", json={"album": key}) + assert response.status_code == 200, response.text + assert response.json()["eps"] == pytest.approx(DEFAULT_UMAP_EPS) + + # Setting one stores it, and it is what comes back afterwards. + response = client.post("/set_umap_eps/", json={"album": key, "eps": 0.42}) + assert response.status_code == 200, response.text + assert client.post("/get_umap_eps/", json={"album": key}).json()[ + "eps" + ] == pytest.approx(0.42) + finally: + client.delete(f"/delete_album/{key}") + + +def test_umap_data_clusters_an_album_with_no_chosen_strength(client, new_album): + """The map endpoint is the one that cannot work without the fallback: + handing DBSCAN a None raises, and the map is the first thing the user + opens after indexing.""" + build_index(client, new_album) + + manager = get_config_manager() + album = manager.get_album(new_album["key"]) + album.umap_eps = None + manager.update_album(album) + + response = client.get(f"/umap_data/{new_album['key']}") + assert response.status_code == 200, response.text + + +def test_a_strength_dbscan_cannot_use_is_refused(client, tmp_path): + """Storing a non-positive epsilon only defers the failure to the next time + the map opens, and the value is invisible in the meantime because every + reader resolves it away.""" + key = "bad_eps_album" + client.post( + "/add_album/", + json={ + "key": key, + "name": "Bad Eps", + "image_paths": [str(tmp_path)], + "index": str(tmp_path / "i.npz"), + }, + ) + try: + for bad in (0, -0.5): + response = client.post("/set_umap_eps/", json={"album": key, "eps": bad}) + assert response.status_code == 422, response.text + finally: + client.delete(f"/delete_album/{key}") + + +def test_cluster_labels_uses_the_fallback_when_nothing_is_chosen( + client, new_album, monkeypatch +): + """The endpoints resolve their own fallback rather than handing None to + DBSCAN, which would raise on the first request the map makes.""" + manager = get_config_manager() + album = manager.get_album(new_album["key"]) + album.umap_eps = None + manager.update_album(album) + + captured = {} + + def fake_get_or_build(embeddings, *, cluster_eps, cluster_min_samples, top_k): + captured["eps"] = cluster_eps + return {} + + monkeypatch.setattr( + "photomap.backend.routers.cluster_labels.get_or_build_cluster_labels", + fake_get_or_build, + ) + + response = client.get(f"/cluster_labels/{new_album['key']}") + assert response.status_code == 200, response.text + assert captured["eps"] == pytest.approx(DEFAULT_UMAP_EPS) + + # An explicit query parameter still wins over both. + client.get(f"/cluster_labels/{new_album['key']}?cluster_eps=0.25") + assert captured["eps"] == pytest.approx(0.25)