Tolerate an album with no chosen Cluster Strength - #374
Closed
lstein wants to merge 1 commit into
Closed
Conversation
``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) <noreply@anthropic.com>
Owner
Author
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
umap_eps— the semantic map's DBSCAN epsilon, shown in the UI as Cluster Strength — was a required float. A newer PhotoMapAI writesumap_eps: nullfor albums that leave the value to the app, and parsing that into a non-nullable field failed the whole config load:One optional per-album map setting stopped the app from starting, with no way out but hand-editing YAML. Nothing in a config file should be able to do that — and the two builds share the file whenever a user moves between releases. (This is not hypothetical: it is how the bug was found, on a real config with two such albums.)
What changed
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 a single
resolve_umap_epshelper, which substitutesDEFAULT_UMAP_EPS. Routing both map endpoints through one helper is the point:/umap_dataand/cluster_labelsmust resolve the same epsilon for the same request, or the cluster ids they return describe different clusterings and the hover labels attach to the wrong blobs.The helper treats zero and negatives as "not chosen" too, because DBSCAN raises on them.
/set_umap_eps/accepted a negative epsilon, stored it happily, and left the map answering 500 until someone edited the file by hand; the request model rejects it now, so a stored value can always be used.It 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
Eight cases: an explicit null, an absent key and a chosen value all load; an unset value round-trips as an absent key;
/get_umap_epsreports what the map will actually use;/umap_dataand/cluster_labelsboth cluster an album that never chose one; and a non-positive strength is refused. Each was checked by reverting its production hunk and confirming it fails — including the/umap_datafallback, which an adversarial review caught as load-bearing but untested.Backend 675 passed, frontend 578 passed, ruff clean.
Relationship to the adaptive Cluster Strength work
lstein/feature/adaptive-cluster-epsmakes the same field nullable and means something richer by None ("derive an epsilon from the album's own coordinates"). The two are compatible: identical YAML shape, neither writes an explicit null, and configs written by either load on the other. Only the resolution differs — a fixed default here, a derived value there — so they will conflict textually on a few comment lines and nowhere semantically. This branch is the robustness half, and it stands alone: the crash it fixes affects anyone whose config was ever touched by the newer build.🤖 Generated with Claude Code