fix(inference): editing a connection's model sends it back for a download

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.

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(<InferenceScreen />));
+  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(<InferenceScreen />));

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``.

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

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"