Refuse a Cluster Strength that DBSCAN cannot run with - #375
Open
lstein wants to merge 1 commit into
Open
Conversation
`/set_umap_eps/` constrains the number to nothing, so the API accepts two kinds of value the Cluster Strength control cannot produce and the map cannot use — and persists both to YAML: * **NaN or infinity** is fatal and permanent. `json` parses the bare `NaN` and `Infinity` literals on the way in but refuses to emit them, so `/get_umap_eps/` 500s serializing its own response, and DBSCAN rejects a NaN epsilon so `/umap_data/` 500s too. `yaml.safe_dump` writes `.nan` and it reloads faithfully, so restarting does not clear it. The album's semantic map is dead until the config is edited by hand. * **Zero or negative** is quieter. `resolve_cluster_eps` floors it to `MIN_CLUSTER_EPS`, so the map still draws — at a strength that is not the one `/get_umap_eps/` reports and the spinner displays. The control ends up lying about what the map is doing, which is the thing the derived-eps work went out of its way to avoid. Three parts: * The request model constrains a *number* to positive and finite, while leaving `None` alone — clearing the field is a deliberate signal meaning "derive one", not a value. The `cluster_eps` query parameter on both map endpoints gets the same bound, so a bad one is a 422 naming the field rather than a 500 out of sklearn. * An album whose stored value is already unusable is treated as not having chosen one, so it gets a derived strength — the same treatment an album that never had a value gets, and a better answer than any constant this code could pick. A config written before this change can contain one. * A validation-error handler that keeps the 422 serializable. FastAPI echoes the rejected input back in the error detail, so refusing a NaN produced a response body that could not be encoded — the refusal itself surfaced as a 500. Without this the first part looks like it does nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
force-pushed
the
lstein/fix/validate-cluster-strength-range
branch
from
August 20, 2026 00:13
7c962f4 to
e317e5a
Compare
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.
Rebased onto master now that #370 has landed; the findings below are re-measured against post-#370 master.
/set_umap_eps/constrains its number to nothing, so the API accepts two kinds of value the Cluster Strength control cannot produce and the map cannot use — and writes both to the config file.Measured on master
NaN or infinity is fatal and permanent.
jsonparses the bareNaN/Infinityliterals on the way in but refuses to emit them, so/get_umap_eps/500s serializing its own response; DBSCAN rejects a NaN epsilon, so/umap_data/500s too.yaml.safe_dumpwrites.nanand it reloads faithfully, so restarting does not help. The album's semantic map is dead until someone edits the YAML.Zero or negative is quieter but still wrong.
resolve_cluster_epsfloors it toMIN_CLUSTER_EPS, so the map draws — at a strength that is not the one/get_umap_eps/reports and the spinner shows. The control ends up lying about what the map is doing, which is exactly what the derived-eps work went out of its way to avoid ("the spinner must not silently display something other than what the map clustered with").Neither is reachable from the UI —
readSpinnerEpsguards NaN andJSON.stringify(Infinity)isnull. Both are reachable from the API.What changed
Refuse them at the boundary.
UmapEpsSetRequest.epsbecomesField(default=None, gt=0, allow_inf_nan=False). The constraint applies to a number;Noneis left alone, because clearing the field is a deliberate signal meaning "derive one", not a value. Thecluster_epsquery parameter on/umap_data/and/cluster_labels/gets the same bound — a 422 naming the field rather than a 500 out of sklearn.Treat an already-stored bad value as "not chosen". A field validator maps it to
None, so the album gets a derived strength — the same treatment an album that never had one gets, and a better answer than any constant this code could pick.Keep the 422 serializable. This is the half that is easy to miss: FastAPI's validation-error handler echoes the rejected input back in the body, so refusing a NaN produced a response that could not be encoded — the refusal surfaced to the client as a 500. A
RequestValidationErrorhandler now replaces any non-finite input with itsrepr. Without it, the first fix looks like it does nothing.Tests
12 tests in
tests/backend/test_umap_eps_validation.py: every bad literal refused and not persisted,/get_umap_eps/still serving afterwards, usable values still stored,eps: nullstill clearing to derived, both query parameters refused, and a config hand-written withumap_eps: 0/umap_eps: .nanloading and serving a derived strength rather than 500ing.739 backend tests pass; ruff clean.
Interaction with #374
#374 adds read-time tolerance (
resolve_umap_eps,eps > 0) for the same values. Complementary — that repairs at the point of use, this refuses at the point of entry so the stored number and the clustering cannot disagree in the first place. Worth reconciling whichever merges second; note #374 also introduces aDEFAULT_UMAP_EPSconstant, which this PR no longer needs since #370 made "no value" mean "derive one".🤖 Generated with Claude Code