From 7bbbc317098177335901870f1aace108d11430c3 Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya Date: Sun, 9 Aug 2026 22:32:10 -0700 Subject: [PATCH 1/2] fix(inference): editing a connection's model sends it back for a download MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `setup_state` answers *are the weights here*, and the weights on disk belong to the model reference the connection was pointing at. Editing `model_id` or `model_revision` left the row `ready` over files nothing had ever fetched. The wrong value did not stay put: `allowed_actions` is derived from it, so a connection with no snapshot went on offering `check_integrity`; and the family backfill is bounded on it, so an edited row was eligible to have its *new* model resolved against a cache that could not hold it, persisting "looked and found nothing" as a finding nobody was in a position to make. A model-reference edit now returns a weight-holding connection to `not_set_up` alongside the family it already forgot. Downloading again is the remedy and was already among the actions such a row offers, and the previous model's blobs stay in the cache — it is keyed by model, so pointing a connection back at something it used to name costs a cache hit rather than a transfer. Compared rather than merely supplied. The app's edit form PATCHes the whole shape, so a rename arrives carrying the model id it already had; reading that as a move would have sent a set-up connection back for a download of weights that never left. That also repairs the same over-reach in the family reset, which until now dropped the family on every edit the form made. An `http` connection holds no weights here and is unaffected. --- .../ui-core/src/screens/inference.test.tsx | 46 +++++++ .../services/inference_connection_service.py | 39 ++++-- tests/inference/test_weights.py | 43 ++++--- tests/kernel/test_inference_connections.py | 112 ++++++++++++++++++ tests/server/test_inference.py | 75 ++++++++++-- 5 files changed, 280 insertions(+), 35 deletions(-) diff --git a/frontend/ui-core/src/screens/inference.test.tsx b/frontend/ui-core/src/screens/inference.test.tsx index 0686b4e..09b0f8a 100644 --- a/frontend/ui-core/src/screens/inference.test.tsx +++ b/frontend/ui-core/src/screens/inference.test.tsx @@ -778,6 +778,52 @@ it("edits without offering to change the kind", async () => { expect(screen.queryByTestId("choose-type")).toBeNull(); }); +it("lands an edited row at Not set up without a reload", async () => { + // The declaration is a cached answer, and this edit changes it: repinning the + // connection to another revision sends it back for a download, so the row's + // whole meaning changes underneath a screen that is already showing it. The + // list invalidation on a successful PATCH is what carries that across. + let edited = false; + handlers.push((request) => { + if (request.method !== "GET" || !new URL(request.url).pathname.endsWith("/connections")) return; + const row = edited + ? connection({ + model_revision: "beefbeefbeefbeefbeefbeefbeefbeefbeefbeef", + setup_state: "not_set_up", + allowed_actions: ["download_weights", "update", "delete"], + }) + : connection({ + // Pinned to a revision the curated list does not name, so the form + // offers the revision as a field to edit rather than as a fixed pair. + model_revision: "0000000000000000000000000000000000000000", + setup_state: "ready", + capabilities: ["point_suggest"], + allowed_actions: READY_BOTH, + }); + return { status: 200, body: { items: [row], total: 1 } }; + }); + handlers.push((request) => { + if (request.method !== "PATCH") return; + edited = true; + return { status: 200, body: connection({ setup_state: "not_set_up" }) }; + }); + sizeIs(1_200_000_000); + + render(mount()); + expect((await screen.findByTestId("connection-status")).textContent).toContain("Ready"); + + await userEvent.click(await screen.findByTestId("actions-sam2-local")); + await userEvent.click(await screen.findByTestId("action-edit")); + const revision = await screen.findByTestId("connection-revision"); + await userEvent.clear(revision); + await userEvent.type(revision, "beefbeefbeefbeefbeefbeefbeefbeefbeefbeef"); + await userEvent.click(await screen.findByTestId("connection-submit")); + + await waitFor(() => + expect(screen.getByTestId("connection-status").textContent).toContain("Not set up"), + ); +}); + it("states the blast radius of a delete accurately", async () => { listing([connection()]); render(mount()); diff --git a/src/visionset/kernel/services/inference_connection_service.py b/src/visionset/kernel/services/inference_connection_service.py index 2a5211c..8083c31 100644 --- a/src/visionset/kernel/services/inference_connection_service.py +++ b/src/visionset/kernel/services/inference_connection_service.py @@ -33,6 +33,7 @@ from pydantic import ValidationError from visionset.kernel.domain import ( + WEIGHT_HOLDING_TYPES, ConnectionAction, ConnectionSetupState, ConnectionType, @@ -145,9 +146,13 @@ def update( """Edit a connection in place. Every argument is optional; ``None`` means *leave this alone*. - Pointing a connection at a different model or revision **forgets what - kind of model it was**, because that answer was read out of the old - model's config and nothing has read the new one. + Pointing a connection at a different model or revision **undoes its + setup**: it forgets what kind of model it was, and a connection whose + weights live on this machine goes back to ``not_set_up``. Both answers + were about the previous reference's files, and those files are still the + previous reference's. Fetching the weights again is the remedy, and it is + already among the actions such a connection offers. A field that arrives + holding the value it already had is not a move. The kind is deliberately not editable. Changing ``local`` to ``http`` would empty every parameter the row carries and keep only its name, which @@ -176,13 +181,29 @@ def update( ): if value is not None: changes[field] = value - # A different model is a different config, and nobody has read - # the new one. Keeping the old family would leave the row - # declaring what its *previous* weights could be asked for — - # a stale answer that reads exactly like a fresh one. Forgetting - # is what sends it back through the resolver. - if "model_id" in changes or "model_revision" in changes: + # Everything this row had learned was learned from the weights of + # the model it used to name, so moving the reference drops all of + # it: the family, because that answer was read out of the old + # model's config and nothing has read the new one, and — for a + # kind that keeps weights here — the setup state, because the + # files on disk are the *previous* reference's. A row left + # `ready` over weights nobody fetched is not a stale display; it + # is what `allowed_actions` and the family backfill are derived + # from. Downloading again is the remedy, and it is already + # offered. + # + # Compared rather than merely supplied, because the only client + # there is sends the whole shape on every edit: a rename arrives + # carrying the model id it already had, and reading that as a + # move would send a set-up connection back for a download of + # weights that never left. + if any( + field in changes and changes[field] != getattr(current, field) + for field in ("model_id", "model_revision") + ): changes["model_family"] = None + if current.connection_type in WEIGHT_HOLDING_TYPES: + changes["setup_state"] = ConnectionSetupState.NOT_SET_UP # Rebuilt rather than mutated, so the cross-field rule runs on the # result: ``model_copy`` does not validate, which is the whole # reason ``Source`` had to turn on ``validate_assignment``. diff --git a/tests/inference/test_weights.py b/tests/inference/test_weights.py index cebb088..b95bc25 100644 --- a/tests/inference/test_weights.py +++ b/tests/inference/test_weights.py @@ -461,8 +461,7 @@ def test_a_set_up_row_with_no_family_acquires_one_and_keeps_it( ) -> None: """The backfill itself, and the bound that makes it cost once.""" made = a_local(connections) - fetch_weights(workspace, made.id) - _forget_the_family(connections, made.id) + _set_up_without_looking(workspace, connections, made.id, monkeypatch) resolver = _Resolver("sam2") monkeypatch.setattr(weights_module, "family_of", resolver) @@ -488,8 +487,7 @@ def test_a_config_that_declared_nothing_is_recorded_and_not_asked_again( config that has already answered — for the life of the workspace. """ made = a_local(connections) - fetch_weights(workspace, made.id) - _forget_the_family(connections, made.id) + _set_up_without_looking(workspace, connections, made.id, monkeypatch) resolver = _Resolver("") monkeypatch.setattr(weights_module, "family_of", resolver) @@ -514,8 +512,7 @@ def test_a_build_that_cannot_look_records_nothing( is never asked again. """ made = a_local(connections) - fetch_weights(workspace, made.id) - _forget_the_family(connections, made.id) + _set_up_without_looking(workspace, connections, made.id, monkeypatch) monkeypatch.setattr( weights_module, "family_of", _Resolver(LocalInferenceUnavailable("no runtime")) @@ -544,13 +541,29 @@ def test_nothing_that_has_no_config_here_is_ever_asked( assert resolver.calls == 0 -def _forget_the_family(connections: InferenceConnectionService, connection_id: Any) -> None: - """Put a row back the way one written before the column looked. - - Through the service rather than by editing the row, so the state this starts - from is one the shipped code can actually produce: pointing a connection at a - different model forgets what kind of model it was. +def _set_up_without_looking( + workspace: WorkspaceService, + connections: InferenceConnectionService, + connection_id: Any, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Reach `ready` with nothing recorded about what kind of model it holds. + + The state the backfill exists for, produced the way a shipped build produces + it. Two things reach it: a migration, over a row that predates the column, + and a machine without the optional runtime, which downloads successfully and + records that it could not look. The second is the one a test can drive, and + it lands on the same row. + + Not an edit. Pointing a connection at a different model does clear the + family, but it now clears the setup state with it — the weights on disk + belong to the model it no longer names — so an edited row is `not_set_up` + and the backfill correctly never looks at it. """ - current = connections.get(connection_id) - connections.update(connection_id, model_id=current.model_id + "-again") - assert connections.get(connection_id).model_family is None + monkeypatch.setattr( + weights_module, "family_of", _Resolver(LocalInferenceUnavailable("no runtime")) + ) + fetch_weights(workspace, connection_id) + settled = connections.get(connection_id) + assert settled.setup_state is ConnectionSetupState.READY + assert settled.model_family is None diff --git a/tests/kernel/test_inference_connections.py b/tests/kernel/test_inference_connections.py index d4206bb..6d8a9cf 100644 --- a/tests/kernel/test_inference_connections.py +++ b/tests/kernel/test_inference_connections.py @@ -440,3 +440,115 @@ def test_editing_anything_else_keeps_the_family(connections) -> None: # noqa: A connections.record_weights_ready(made.id, model_family="sam2") assert connections.update(made.id, name="renamed").model_family == "sam2" assert connections.update(made.id, device="cpu", precision="fp32").model_family == "sam2" + + +def test_pointing_a_connection_at_another_model_sends_it_back_for_a_download( + connections, # noqa: ANN001 +) -> None: + """The weights on disk belong to the model this connection no longer names. + + `setup_state` answers *are the weights here*, so an edit that changes which + weights are meant makes the stored answer describe the wrong question. The + row would go on claiming to be set up over a reference nothing ever fetched. + """ + made = connections.create("local", **LOCAL) + connections.record_weights_ready(made.id, model_family="sam2") + + edited = connections.update(made.id, model_id="other/model") + assert edited.setup_state is ConnectionSetupState.NOT_SET_UP + assert edited.model_family is None + assert connections.get(made.id).setup_state is ConnectionSetupState.NOT_SET_UP + + +def test_pinning_a_connection_to_another_revision_sends_it_back_too( + connections, # noqa: ANN001 +) -> None: + """The reference is the pair, so either half of it moving means new weights. + + A revision is what makes provenance answerable; two revisions of one model id + are two different sets of files, and only one of them was downloaded. + """ + made = connections.create("local", **LOCAL) + connections.record_weights_ready(made.id, model_family="sam2") + + assert ( + connections.update(made.id, model_revision="beef1234").setup_state + is ConnectionSetupState.NOT_SET_UP + ) + + +def test_editing_anything_else_leaves_a_connection_set_up(connections) -> None: # noqa: ANN001 + """Renaming or moving devices changes nothing about which weights are meant. + + The companion of the two above, and the reason the reset is conditioned on + the model reference rather than on "something was edited": sending a renamed + connection back for a download it does not need would be its own defect. + """ + made = connections.create("local", **LOCAL) + connections.record_weights_ready(made.id, model_family="sam2") + + assert connections.update(made.id, name="renamed").setup_state is ConnectionSetupState.READY + assert ( + connections.update(made.id, device=CUDA, precision=Precision.FP16).setup_state + is ConnectionSetupState.READY + ) + + +def test_resupplying_the_same_model_reference_is_not_a_change(connections) -> None: # noqa: ANN001 + """A mention is not an edit, and the only client there is mentions every field. + + The app's edit form PATCHes the whole shape — a rename arrives carrying the + model id it already had. Reading "was this field supplied" as "did the model + move" would send a renamed connection back for a download of weights that + never left, so the reset compares values. + """ + made = connections.create("local", **LOCAL) + connections.record_weights_ready(made.id, model_family="sam2") + + renamed = connections.update( + made.id, + name="renamed", + model_id=LOCAL["model_id"], + model_revision=LOCAL["model_revision"], + ) + assert renamed.setup_state is ConnectionSetupState.READY + assert renamed.model_family == "sam2" + + +def test_an_http_connection_keeps_its_readiness_when_its_model_moves( + connections, # noqa: ANN001 +) -> None: + """There are no local weights to invalidate, so there is nothing to reset. + + An `http` connection is born `ready` because nothing has to be set up on this + machine at all — a fact about the kind, not about a download that happened. + Sending it to `not_set_up` would offer a remedy it cannot perform. + """ + made = connections.create("remote", **HTTP) + assert made.setup_state is ConnectionSetupState.READY + + edited = connections.update(made.id, model_id="other/model", model_revision="beef1234") + assert edited.setup_state is ConnectionSetupState.READY + + +def test_editing_back_and_downloading_again_restores_the_family( + connections, # noqa: ANN001 +) -> None: + """The round trip, and the reason the old blobs are left where they are. + + Pointing a connection back at a model it used to name is an ordinary edit + followed by an ordinary download; the cache is keyed by model, so the second + download finds what the first one fetched and the connection is set up again + cheaply. + """ + made = connections.create("local", **LOCAL) + connections.record_weights_ready(made.id, model_family="sam2") + connections.update(made.id, model_id="other/model") + + back = connections.update(made.id, model_id=LOCAL["model_id"]) + assert back.setup_state is ConnectionSetupState.NOT_SET_UP + assert back.model_family is None + + redownloaded = connections.record_weights_ready(made.id, model_family="sam2") + assert redownloaded.setup_state is ConnectionSetupState.READY + assert redownloaded.model_family == "sam2" diff --git a/tests/server/test_inference.py b/tests/server/test_inference.py index c3cd017..5ac3316 100644 --- a/tests/server/test_inference.py +++ b/tests/server/test_inference.py @@ -20,7 +20,7 @@ from visionset.inference.integrity import IntegrityReport from visionset.jobs import integrity as job_module from visionset.kernel.domain import BackgroundJobState -from visionset.kernel.errors import WeightsDamaged +from visionset.kernel.errors import LocalInferenceUnavailable, WeightsDamaged from visionset.kernel.services import InferenceConnectionService from visionset.server.routes import inference as inference_routes @@ -360,6 +360,56 @@ def test_a_finished_download_leaves_the_connection_ready( ] +def test_editing_the_model_takes_the_integrity_check_off_the_row( + tmp_path: Path, runtime_present: None, fetched: list[str] +) -> None: + """The declaration follows the reset, because it is derived from the state. + + `check_integrity` re-reads a snapshot. Pointing the connection at a model + whose snapshot was never fetched leaves nothing to read, so the action stops + being offered in the same response that performs the edit — a client never + sees a window in which it is declared over weights that are not there. + """ + with api_client(tmp_path / "ws", dispatcher=InlineDispatcher()) as client: + made = _made_ready(client) + assert "check_integrity" in made["allowed_actions"] + + edited = client.patch( + f"/inference/connections/{made['id']}", json={"model_id": "other/model"} + ).json() + assert edited["setup_state"] == "not_set_up" + assert edited["capabilities"] == [] + assert edited["allowed_actions"] == ["download_weights", "update", "delete"] + + +def test_renaming_a_ready_connection_leaves_it_ready( + tmp_path: Path, runtime_present: None, fetched: list[str] +) -> None: + """The whole shape arrives on every edit, and a mention is not a move. + + The app's form PATCHes each field, so a rename carries the model id the row + already had. Reading that as a change would send a set-up connection back for + a download of weights that never left — the reset's own failure mode, and the + reason it compares values rather than counting what was supplied. + """ + with api_client(tmp_path / "ws", dispatcher=InlineDispatcher()) as client: + made = _made_ready(client) + + renamed = client.patch( + f"/inference/connections/{made['id']}", + json={ + "name": "renamed", + "model_id": LOCAL["model_id"], + "model_revision": LOCAL["model_revision"], + "device": LOCAL["device"], + "precision": LOCAL["precision"], + }, + ).json() + assert renamed["setup_state"] == "ready" + assert renamed["capabilities"] == made["capabilities"] + assert "check_integrity" in renamed["allowed_actions"] + + def test_a_failed_download_leaves_the_connection_not_set_up( tmp_path: Path, runtime_present: None, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -761,15 +811,18 @@ def test_a_row_written_before_the_column_is_resolved_on_its_first_read( for the life of the workspace. """ with api_client(tmp_path / "ws", dispatcher=InlineDispatcher()) as client: + # A row the way one written before the column looked: `ready`, with + # nothing recorded about what kind of model it holds. Downloading on a + # machine whose optional runtime cannot read a config lands exactly + # there, and it is the one producer of that state a test can drive — an + # edited row is `not_set_up`, so the backfill never reaches it. + def _no_runtime(*_: Any, **__: Any) -> str: + raise LocalInferenceUnavailable("'transformers' is not installed here") + + monkeypatch.setattr(weights_module, "family_of", _no_runtime) made = _made_ready(client) - # A row the way one written before the column looked: still `ready`, - # with nothing recorded about what kind of model it holds. The edit's own - # answer is the honest empty one — it is a write, and nothing has read - # the new model's config. - patched = client.patch( - f"/inference/connections/{made['id']}", json={"model_id": "other/model"} - ) - assert patched.json()["capabilities"] == [] + assert made["setup_state"] == "ready" + assert made["capabilities"] == [] reads: list[str] = [] @@ -780,9 +833,9 @@ def _read(connection: Any, **_: Any) -> str: monkeypatch.setattr(weights_module, "family_of", _read) listed = client.get("/inference/connections").json()["items"] assert [one["capabilities"] for one in listed] == [["text_detect"]] - assert reads == ["other/model"] + assert reads == [LOCAL["model_id"]] # And the answer is now on the row, so no read of it looks again. client.get("/inference/connections") client.get(f"/inference/connections/{made['id']}") - assert reads == ["other/model"] + assert reads == [LOCAL["model_id"]] From 3fc88fe57f67ed98dbb3b85be8be7f83c15980bb Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya Date: Sun, 9 Aug 2026 22:32:15 -0700 Subject: [PATCH 2/2] docs(inference): a model edit sends a local connection back for a download The behaviour has a user-visible remedy and a rule about what does *not* trigger it, so it gets a section of its own rather than a clause: what resets, why the previous model's files are left in the cache, and that a name, a device or an unchanged reference resets nothing. The capability paragraph and the screen's editing paragraph point at it instead of restating it. --- docs/inference.md | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/docs/inference.md b/docs/inference.md index b6f1f56..22000f8 100644 --- a/docs/inference.md +++ b/docs/inference.md @@ -194,6 +194,22 @@ The two states stay the only two throughout. A check that cannot reach the hub repository that moved — changes nothing and removes nothing: there are no published digests to compare against, and that is an absence of evidence rather than a verdict in either direction. +## Pointing a connection somewhere else + +Editing a local connection's `model_id` or `model_revision` puts it back to `not_set_up`. The +weights on your disk are the weights of the model it used to name, and `setup_state` answers *are +the weights here* — so leaving it `ready` would have it claim to be set up over files nothing ever +fetched. It forgets what kind of model it holds at the same time, and for the same reason. + +**The remedy is the ordinary one.** The row offers **Download weights** again, and the cache is +keyed by model rather than by connection: the previous model's files are left where they are, so +pointing a connection back at something it used to name costs a cache hit instead of a second +transfer. Editing anything else — the name, the device, the precision — changes neither the state +nor the family, and neither does sending the same model reference back unchanged. + +An `http` connection keeps no weights here, so a model edit resets nothing for it. It stays +`ready`, which for that kind has always meant *there is nothing to set up on this machine*. + ## Running on the CPU A connection asking for `cuda` on a machine with no GPU falls back to the CPU, in full precision, @@ -243,9 +259,10 @@ connection. **It is recorded when the weights arrive**, because that is the first moment it is knowable without reaching a network. Editing a connection to point at another model or revision clears it -again: nothing has read the new one, and a stale answer reads exactly like a fresh one. A -connection created before this shipped acquires its answer the first time something reads it, -from files already on your disk. +again — nothing has read the new one, and a stale answer reads exactly like a fresh one — and +takes the setup state with it, for the same reason: see [Pointing a connection somewhere +else](#pointing-a-connection-somewhere-else). A connection created before this shipped acquires +its answer the first time something reads it, from files already on your disk. ## Suggesting a shape from a click @@ -381,9 +398,11 @@ what happened in the job's own words with what to do about it. There is no separ different reason — the damaged files were removed and the connection stood down before the row said so — and the retry is the same **Download weights**, which now has to fetch them again for real. -Editing does not offer to change the kind, because the kind is not editable. Deleting asks once -and says exactly what it destroys: *annotations keep their model provenance; only this -configuration is removed.* +Editing does not offer to change the kind, because the kind is not editable. Saving an edit that +moves a local row to another model or revision returns it to **Not set up** in place, with +**Download weights** as the next step — the files on the disk belong to the model it was pointing +at before. Deleting asks once and says exactly what it destroys: *annotations keep their model +provenance; only this configuration is removed.* Above twenty rows the list grows a filter, which matches a name substring and keeps saying how many it hid.