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)