diff --git a/docs/cli.md b/docs/cli.md index 7a4317df..4f9361e0 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -38,7 +38,8 @@ visionset token create --name NAME visionset token list visionset token revoke NAME [--yes] visionset inference create NAME --type local|http --model ID --revision REV - [--device D] [--precision P] [--endpoint URL] + [--device cpu|cuda|cuda:N] [--precision fp16|fp32] + [--endpoint URL] visionset inference list visionset inference show|update|delete NAME_OR_ID visionset inference download NAME_OR_ID diff --git a/docs/inference.md b/docs/inference.md index 4a23513c..1a7fca56 100644 --- a/docs/inference.md +++ b/docs/inference.md @@ -36,7 +36,7 @@ with WorkspaceService.open("./road-signs") as workspace: model_id="some/model", model_revision="abc123", device="cuda", - precision="fp16", + precision="fp16", # fp16 needs a cuda device; a cpu connection is fp32 ) for one in connections.list(): print(one.name, one.connection_type.value, one.setup_state.value) @@ -149,12 +149,16 @@ misdescribes. **A failure changes nothing.** The connection is marked ready as the last step, after every file is present — so a download that dies partway leaves it exactly as it was, at `not_set_up`, with the error on the job. There is no half-ready state to recover from because there is no moment at which -one could be written. Ask again; a partial cache is verified and resumed rather than restarted. +one could be written. Ask again: an interrupted transfer resumes from what it had, and each file +that arrives is checked against the size the hub published for it before it is put in place. -**Asking twice is refused, not repeated.** Once a connection is ready there is nothing left to -fetch, so `download_weights` stops being offered and the request is answered with -`INFERENCE_CONNECTION_NOT_DOWNLOADABLE`. An `http` connection is refused with the same code for the -other reason: its model runs elsewhere, so it has no weights of its own in any state. +**Asking again checks rather than repeats.** `download_weights` stays available once a connection +is `ready`, where the same call re-checks that the snapshot is still complete and fetches only what +is missing — the browser labels it **Verify weights** and puts it in the row's overflow menu. That +is worth doing on a machine where a disk filled or a cache was pruned; it is *completeness* and not +integrity, because a file already in the cache under this revision is found rather than re-read. An +`http` connection is refused with `INFERENCE_CONNECTION_NOT_DOWNLOADABLE` in any state, for the +other reason: its model runs elsewhere, so it has no weights of its own. ## Running on the CPU @@ -163,9 +167,17 @@ with a warning in the log. It is a fallback rather than a preference — a works workstation should still open on a laptop — but it is slower by a large factor, which is why it is said out loud rather than silently done. -Half precision (`fp16`, `float16`, `half` — the spelling is yours) applies on CUDA only. On a CPU -it is not the conservative choice it looks like: `float16` arithmetic outside CUDA's autocast is -slower than the `float32` it was avoiding. +Half precision applies on CUDA only, and the kernel now says so rather than absorbing it: a `cpu` +connection asking for `fp16` is refused at creation. On a CPU it was never the conservative choice +it looks like — `float16` arithmetic outside CUDA's autocast is slower than the `float32` it was +avoiding — and a setting the adapters drop is one the row would otherwise go on displaying as +though it had an effect. + +**Both fields are closed vocabularies.** `device` is `cpu`, `cuda`, or `cuda:N` for the second GPU +on a machine that has one; `precision` is `fp16` or `fp32`, and `float16`, `half`, `float32` and +`full` are accepted as spellings of those two. Anything else is refused with a sentence naming the +members. What this closes is a gap rather than a freedom: `gpu` used to be accepted and then +resolved onto the CPU, so the connection described a run that never happened. ## Suggesting a shape from a click @@ -268,22 +280,31 @@ A workspace with none says so and offers one thing — **Add connection**. Creat steps, because the two kinds share almost no fields: first where the model runs, then that kind's form. -- **Local** opens pre-filled with the suggested model, `facebook/sam2-hiera-base-plus` at `main`, - a `cpu` device and `fp16` precision. Every one of those is a starting point you can type over. - Underneath the fields is what fetching that revision would cost — the size described above, read - while you are still deciding. If this machine has no `local-inference` extra the size cannot be - read, and the form says so, in the server's own words, with the install command. **It stays - usable**: creating a connection downloads nothing, so not knowing the size is information rather - than a barrier. +- **Local** opens on a curated model, a `cpu` device and `fp32` precision. The model field is a + grouped list — the SAM 2.1 ladder under *Interactive segmentation*, Grounding DINO under + *Text-prompt detection* — showing each entry's download size and a line on what it is for, and + each one is pinned to a revision this build was checked against. **Custom model…** is the last + entry and reveals the free model id and revision fields: the list guides, it does not restrict, + and any model this build has an adapter for remains typeable. Device and precision are lists too, + and the precision list follows the device, because half precision applies on CUDA only. Underneath + is what fetching that revision would cost — the size described above, read while you are still + deciding. If this machine has no `local-inference` extra the size cannot be read, and the form + says so, in the server's own words, with the install command. **It stays usable**: creating a + connection downloads nothing, so not knowing the size is information rather than a barrier. - **HTTP** asks for the endpoint URL. There is no credential field; where a secret would live is still open (`cf. #421`), and a field added ahead of that answer would be answering it. Each row shows its name, its kind, `model @ revision`, and its status as a word — **Ready** or **Not set up** — beside a colour, never as a colour alone. A local row that is not set up carries **Download weights**, which launches the background job described above and reports its progress -in place; the row becomes **Ready** when the job finishes. A machine without the extra still shows -the control, and pressing it answers with the install command — a control that vanished would take -the remedy with it. +in place; the row becomes **Ready** when the job finishes, without a reload. A row that is already +ready carries **Verify weights** in its overflow menu instead — the same request, re-checking the +snapshot. A machine without the extra still shows the control, and pressing it answers with the +install command — a control that vanished would take the remedy with it. + +A failed download leaves the row at **Not set up**, because weights arrive or they do not, and says +what happened in the job's own words with what to do about it. There is no separate retry button: +**Download weights** is the retry. 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 @@ -304,6 +325,7 @@ control. visionset inference size some/model --revision abc123 visionset inference create local-detector \ --type local --model some/model --revision abc123 --device cuda --precision fp16 +# --device takes cpu, cuda or cuda:N; --precision takes fp16 or fp32, and fp16 needs a cuda device visionset inference list visionset inference show local-detector --json visionset inference update local-detector --revision def456 diff --git a/frontend/ui-core/src/data/inferenceQueries.ts b/frontend/ui-core/src/data/inferenceQueries.ts index 7a258a18..24c6f507 100644 --- a/frontend/ui-core/src/data/inferenceQueries.ts +++ b/frontend/ui-core/src/data/inferenceQueries.ts @@ -95,6 +95,7 @@ export interface SuggestionOut { export type ConnectionType = components["schemas"]["ConnectionType"]; export type ConnectionSetupState = components["schemas"]["ConnectionSetupState"]; +export type Precision = components["schemas"]["Precision"]; export type DownloadSizeOut = components["schemas"]["DownloadSizeOut"]; export const inferenceKeys = { @@ -134,7 +135,7 @@ export interface ConnectionInput { readonly modelId: string; readonly modelRevision: string; readonly device?: string | null; - readonly precision?: string | null; + readonly precision?: Precision | null; readonly endpointUrl?: string | null; } @@ -209,6 +210,27 @@ export function useDownloadWeights() { }); } +/** + * Re-read every connection, because something that changes one has finished. + * + * The mutations above invalidate the list themselves; this is for the change + * that does not arrive as a mutation's response. A weights download answers + * `202` and finishes later, in a background job — and when it finishes it has + * moved `setup_state` and, with it, what the row may be asked to do. Nothing + * re-reads the list at that moment unless somebody says so, which is why the row + * used to sit at `Not set up` until the page was reloaded. + * + * Lives here rather than beside the screen for this module's stated reason: the + * list, its key and its invalidation are one fact, and a second spelling of the + * key under a screen is how two callers come to disagree about what is stale. + */ +export function useRefreshConnections(): () => void { + const queries = useQueryClient(); + return () => { + void queries.invalidateQueries({ queryKey: inferenceKeys.connections() }); + }; +} + /** * What fetching that revision would cost, read before anybody agrees to it. * diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index 47090150..0f85d820 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -2593,8 +2593,7 @@ export interface components { model_revision: string; /** Name */ name: string; - /** Precision */ - precision?: string | null; + precision?: components["schemas"]["Precision"] | null; }; /** * ConnectionOut @@ -2624,8 +2623,7 @@ export interface components { model_revision: string; /** Name */ name: string; - /** Precision */ - precision: string | null; + precision: components["schemas"]["Precision"] | null; setup_state: components["schemas"]["ConnectionSetupState"]; /** * Updated At @@ -2692,8 +2690,7 @@ export interface components { model_revision?: string | null; /** Name */ name?: string | null; - /** Precision */ - precision?: string | null; + precision?: components["schemas"]["Precision"] | null; }; /** * DatasetChangeOut @@ -3144,6 +3141,23 @@ export interface components { */ type: "polyline"; }; + /** + * Precision + * @description The numeric precision a local connection asks its weights to be loaded in. + * + * A closed vocabulary rather than the free text this field started as, on + * ``ConnectionType``'s test: the set is small, the kernel is what decides + * whether a member is usable on a given device, and it grows only by a + * deliberate kernel change — bf16 arriving later is exactly that change. + * + * Free text here was not neutrality but a gap. ``fp32x`` was accepted and then + * ignored; so was ``fp16`` beside ``cpu``, which the adapters silently drop + * (see :func:`precisions_for`). A field whose wrong values are absorbed rather + * than refused is a field that cannot tell somebody they are configuring a run + * that will not happen. + * @enum {string} + */ + Precision: "fp16" | "fp32"; /** * ProgressCounts * @description How many assets sit in each annotation state. diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index 0dded6e7..104c4849 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -119,8 +119,11 @@ export const checkConnectionSetupState: Check = export const checkConnectionType: Check = /*#__PURE__*/ oneOf(["local", "http"] as const); +export const checkPrecision: Check = + /*#__PURE__*/ oneOf(["fp16", "fp32"] as const); + export const checkConnectionOut: Check = - /*#__PURE__*/ object({ "allowed_actions": [true, arrayOf(checkConnectionAction)], "connection_type": [true, checkConnectionType], "created_at": [true, isString], "device": [true, either([isString, isNull] as const)], "endpoint_url": [true, either([isString, isNull] as const)], "id": [true, isString], "model_id": [true, isString], "model_revision": [true, isString], "name": [true, isString], "precision": [true, either([isString, isNull] as const)], "setup_state": [true, checkConnectionSetupState], "updated_at": [true, isString] } as const); + /*#__PURE__*/ object({ "allowed_actions": [true, arrayOf(checkConnectionAction)], "connection_type": [true, checkConnectionType], "created_at": [true, isString], "device": [true, either([isString, isNull] as const)], "endpoint_url": [true, either([isString, isNull] as const)], "id": [true, isString], "model_id": [true, isString], "model_revision": [true, isString], "name": [true, isString], "precision": [true, either([checkPrecision, isNull] as const)], "setup_state": [true, checkConnectionSetupState], "updated_at": [true, isString] } as const); export const checkConnectionPage: Check = /*#__PURE__*/ object({ "items": [true, arrayOf(checkConnectionOut)], "total": [true, isInteger] } as const); diff --git a/frontend/ui-core/src/screens/InferenceScreen.tsx b/frontend/ui-core/src/screens/InferenceScreen.tsx index c9d984a9..6e7a4cd6 100644 --- a/frontend/ui-core/src/screens/InferenceScreen.tsx +++ b/frontend/ui-core/src/screens/InferenceScreen.tsx @@ -27,6 +27,29 @@ * (`cf. #421`); until then there is no third value to render and no control that * would produce one. * + * ## What the form offers, and what it refuses to compute + * + * The model, the device and the precision are all chosen from lists rather than + * typed, and every one of those lists lives in `inferenceCatalog.ts` — one + * module, so extending the curated set is one entry and no other edit. Curation + * guides without restricting: **Custom model…** reveals the same free model id + * and revision fields the form had before. + * + * The device and precision lists are the kernel's vocabularies, offering-side. + * The kernel is what refuses a pair outside them — including `cpu` with `fp16`, + * which both local adapters silently drop — and its refusal renders here as + * prose like any other. This is not the hand-mirror `ui-capabilities` bans: that + * rule is about `allowed_actions`, and no field-level shape can carry which + * precision a device honours. + * + * ## One declared action, two things to call it + * + * `download_weights` is declared for a local connection in either state (#469). + * Below `Ready` it is the row's **Download weights** button; at `Ready` it is + * **Verify weights** in the overflow, where it re-checks that the snapshot is + * still complete. The row picks the label from `setup_state` — a field the wire + * states — and never from a table of its own. + * * ## The size is asked for before the connection exists * * D1 on #424 requires the local form to show what a download would cost *before* @@ -37,7 +60,7 @@ * instead of disabling itself. */ -import { Download, Filter, MoreHorizontal, Pencil, Plug, Trash2 } from "lucide-react"; +import { Download, Filter, MoreHorizontal, Pencil, Plug, ShieldCheck, Trash2 } from "lucide-react"; import { useEffect, useState, type FormEvent, type JSX } from "react"; import { Async } from "../data/Async"; @@ -48,6 +71,7 @@ import { useDeleteConnection, useDownloadSize, useDownloadWeights, + useRefreshConnections, useUpdateConnection, type Connection, type ConnectionType, @@ -68,19 +92,29 @@ import { DropdownMenuTrigger, } from "../primitives/Menu"; import { FieldError, FieldHint, Input, Label } from "../primitives/Input"; +import { + Select, + SelectContent, + SelectGroup, + SelectItem, + SelectLabel, + SelectTrigger, + SelectValue, +} from "../primitives/Select"; import { Table, TableBody, TableCell, TableHead, TableHeader, TableRow } from "../primitives/Table"; +import { + CURATED_BY_ID, + CURATED_MODELS, + CUSTOM_MODEL, + DEFAULT_MODEL, + DEVICES, + curatedEntry, + precisionOn, + precisionsFor, + type Precision, +} from "./inferenceCatalog"; import { useBackgroundJob } from "./queries"; -/** - * The model D1 suggests, and the revision it is pinned at. - * - * "Suggested default" means exactly that: it fills the form in, and anybody may - * type over it. Nothing is bundled and nothing is fetched until somebody presses - * the action that fetches it. - */ -export const SUGGESTED_MODEL = "facebook/sam2-hiera-base-plus"; -export const SUGGESTED_REVISION = "main"; - /** Above this many rows a list carries a filter input (`DESIGN.md`). */ const FILTER_ABOVE = 20; @@ -203,6 +237,7 @@ function ConnectionRow({ }): JSX.Element { const can = new Set(connection.allowed_actions); const ready = connection.setup_state === "ready"; + const weights = useWeightsRun(connection); return ( {connection.name} @@ -230,35 +265,76 @@ function ConnectionRow({ -
- {can.has("download_weights") && } - {(can.has("update") || can.has("delete")) && ( - - - - - - {can.has("update") && ( - - - )} - {can.has("delete") && ( - - - )} - - +
+
+ {/* + One declared action, two readings of it, and the row picks the + reading from `setup_state` — a field the wire states. Whether the + control may exist is still `allowed_actions` and nothing else. + */} + {can.has("download_weights") && !ready && ( + + )} + {(can.has("update") || can.has("delete") || (can.has("download_weights") && ready)) && ( + + + + + + {can.has("download_weights") && ready && ( + + + )} + {can.has("update") && ( + + + )} + {can.has("delete") && ( + + + )} + + + )} +
+ {weights.running && weights.progress !== null && ( + + {weights.progress} + + )} + {weights.failure !== null && ( + + {weights.failure.code} {weights.failure.message}{" "} + {ready + ? "The connection is still Ready — nothing was changed. Verify weights again to re-check the cache." + : "The connection is still Not set up: weights arrive or they do not, so there is nothing half-installed to clear up. Download weights again — an interrupted transfer resumes from what it had."} + )}
@@ -267,58 +343,59 @@ function ConnectionRow({ } /** - * The `download_weights` action, and the job it launches. + * The `download_weights` action, wherever the row renders it, and the job it runs. + * + * 202 and poll, the contract the export route uses. It lives on the *row* rather + * than inside a control because one of the two controls is a menu item, and a + * menu closes when it is chosen — a job whose progress and refusal lived inside + * the item would take both with it on the way out. * - * 202 and poll, the contract the export route uses: the button hands off to - * `useBackgroundJob` and reports the phase the job is in. A failed run leaves the - * connection exactly as it was — the state flip is the download's last statement - * — so there is nothing to undo and the button simply comes back. + * **The list is re-read when the job settles, and that is the fix for a bug** + * (#469). What the `202` changed was the declaration; what the *completion* + * changes is `setup_state` and, with it, the row's whole meaning. Nothing was + * re-reading at that moment, so a finished download left `Not set up` on screen + * until somebody reloaded the page. A settled job is a mutation like any other, + * so it invalidates what it touched. */ -function DownloadWeights({ connection }: { readonly connection: Connection }): JSX.Element { +function useWeightsRun(connection: Connection): { + readonly start: () => void; + readonly running: boolean; + readonly progress: string | null; + readonly failure: { readonly code: string; readonly message: string } | null; +} { const download = useDownloadWeights(); + const refresh = useRefreshConnections(); const [jobId, setJobId] = useState(null); const job = useBackgroundJob(jobId); const state = job.data?.state; const running = download.isPending || state === "queued" || state === "running"; - // Stop polling once the work settles, and let the invalidated list carry the - // outcome — the row itself is what says `Ready`, so a second announcement here - // would be the same fact twice. useEffect(() => { - if (state === "succeeded" || state === "cancelled") setJobId(null); + if (state !== "succeeded" && state !== "failed" && state !== "cancelled") return; + refresh(); + // A failure keeps its job id, because the job is where the reason is; the + // poll has already stopped on its own — `useBackgroundJob` settles — so this + // holds a finished row, not an open request. + if (state !== "failed") setJobId(null); + // `refresh` is stable per render under the compiler and re-running this on a + // list re-read would invalidate in a loop; the transition is what it watches. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [state]); - const failure = download.isError - ? asApiError(download.error) - : state === "failed" - ? { code: "DOWNLOAD_FAILED", message: job.data?.error ?? "The download did not finish." } - : null; - - return ( -
- - {running && job.data !== undefined && ( - - {job.data.processed} - {job.data.total === null ? "" : ` of ${job.data.total}`} - - )} - {failure !== null && ( - - {failure.code} {failure.message} - - )} -
- ); + return { + start: () => + download.mutate(connection.id, { onSuccess: (queued) => setJobId(queued.id) }), + running, + progress: + job.data === undefined + ? null + : `${job.data.processed}${job.data.total === null ? "" : ` of ${job.data.total}`}`, + failure: download.isError + ? asApiError(download.error) + : state === "failed" + ? { code: "DOWNLOAD_FAILED", message: job.data?.error ?? "The download did not finish." } + : null, + }; } /** @@ -344,13 +421,18 @@ function ConnectionDialog({ const [name, setName] = useState(""); const [modelId, setModelId] = useState(""); const [revision, setRevision] = useState(""); - const [device, setDevice] = useState(""); - const [precision, setPrecision] = useState(""); + // Which entry the model select is showing: a curated model id, or the sentinel + // that reveals the free fields. Kept beside `modelId` rather than derived from + // it, because a custom connection may name a curated model at another revision + // and the select must go on showing "Custom" while it does. + const [choice, setChoice] = useState(CUSTOM_MODEL); + const [device, setDevice] = useState("cpu"); + const [precision, setPrecision] = useState("fp32"); const [endpoint, setEndpoint] = useState(""); // Fill the form from whatever the dialog was opened for. An edit arrives with a - // row; a create arrives with nothing and, once a local kind is chosen, with - // D1's suggestion already in it. + // row; a create arrives with nothing and, once a local kind is chosen, with the + // default curated model already in it. useEffect(() => { if (!open) return; if (editing !== undefined) { @@ -358,8 +440,14 @@ function ConnectionDialog({ setName(editing.name); setModelId(editing.model_id); setRevision(editing.model_revision); - setDevice(editing.device ?? ""); - setPrecision(editing.precision ?? ""); + setChoice( + curatedEntry(editing.model_id, editing.model_revision)?.modelId ?? CUSTOM_MODEL, + ); + // A device outside what a form offers — `cuda:1`, or a row from a build + // before the vocabulary closed — is shown as it is rather than rewritten + // to the nearest offered member behind somebody's back. + setDevice(editing.device ?? "cpu"); + setPrecision(editing.precision ?? "fp32"); setEndpoint(editing.endpoint_url ?? ""); return; } @@ -367,26 +455,47 @@ function ConnectionDialog({ setName(""); setModelId(""); setRevision(""); + setChoice(CUSTOM_MODEL); setDevice("cpu"); - setPrecision("fp16"); + setPrecision("fp32"); setEndpoint(""); }, [open, editing]); function choose(next: ConnectionType): void { setKind(next); - if (next === "local") { - setModelId(SUGGESTED_MODEL); - setRevision(SUGGESTED_REVISION); - } + if (next === "local") pickModel(DEFAULT_MODEL.modelId); + } + + /** Pick a curated entry — which sets both halves of the pair — or reveal the fields. */ + function pickModel(next: string): void { + setChoice(next); + const entry = next === CUSTOM_MODEL ? undefined : CURATED_BY_ID.get(next); + if (entry === undefined) return; + setModelId(entry.modelId); + setRevision(entry.revision); + } + + /** + * Moving the device can strand the precision, so the precision moves with it. + * + * The kernel refuses `cpu` + `fp16`, so a form that left `fp16` selected while + * the device went to `cpu` would be offering a pair it knows will be refused — + * which is worse than either offering it and rendering the refusal or not + * offering it at all. + */ + function pickDevice(next: string): void { + setDevice(next); + setPrecision(precisionOn(next, precision)); } const local = kind === "local"; + const custom = choice === CUSTOM_MODEL; const pending = create.isPending || update.isPending; const complete = name.trim() !== "" && modelId.trim() !== "" && revision.trim() !== "" && - (local ? device.trim() !== "" && precision.trim() !== "" : endpoint.trim() !== ""); + (local ? device.trim() !== "" : endpoint.trim() !== ""); function submit(event: FormEvent): void { event.preventDefault(); @@ -397,7 +506,7 @@ function ConnectionDialog({ modelId: modelId.trim(), modelRevision: revision.trim(), device: device.trim(), - precision: precision.trim(), + precision, endpointUrl: endpoint.trim(), }; // Only on success: a refusal leaves the dialog open with what was typed @@ -445,59 +554,151 @@ function ConnectionDialog({ /> Unique in this workspace, ignoring case.
-
- - setModelId(event.target.value)} - /> -
-
- - setRevision(event.target.value)} - /> - Pinned. A moving pointer is not a provenance. -
+ {/* + The curated list is the *local* form's, and only its. A curated + entry is a checkpoint this build has an adapter for and would + download; an HTTP connection names whatever the endpoint on the + other end runs, which this build never loads and cannot vouch + for. Offering the same list there would be recommending models + for somebody else's server. + */} {local ? ( <> +
+ + + + {custom + ? "Any model this build has an adapter for. The list above is a starting point, not a limit." + : "Pinned to the revision this list was checked against."} + +
+ {custom && ( + <> +
+ + setModelId(event.target.value)} + /> +
+
+ + setRevision(event.target.value)} + /> + Pinned. A moving pointer is not a provenance. +
+ + )}
- setDevice(event.target.value)} - /> +
- setPrecision(event.target.value)} - /> + onValueChange={(next) => setPrecision(next as Precision)} + > + + + + + {precisionsFor(device).map((one) => ( + + {one} + + ))} + + + + {precisionsFor(device).length === 1 + ? "Half precision applies on CUDA only — on a CPU it has no effect." + : "fp16 halves the memory and runs faster on CUDA."} +
) : ( -
- - setEndpoint(event.target.value)} - /> -
+ <> +
+ + setModelId(event.target.value)} + /> +
+
+ + setRevision(event.target.value)} + /> + Pinned. A moving pointer is not a provenance. +
+
+ + setEndpoint(event.target.value)} + /> +
+ )} {failure !== null && ( diff --git a/frontend/ui-core/src/screens/inference.test.tsx b/frontend/ui-core/src/screens/inference.test.tsx index efabc1d4..a0b02c28 100644 --- a/frontend/ui-core/src/screens/inference.test.tsx +++ b/frontend/ui-core/src/screens/inference.test.tsx @@ -26,7 +26,8 @@ import { afterEach, beforeEach, expect, it, vi } from "vitest"; import type { JSX, ReactNode } from "react"; import { ApiProvider } from "../data/ApiProvider"; -import { InferenceScreen, SUGGESTED_MODEL, SUGGESTED_REVISION, bytes } from "./InferenceScreen"; +import { InferenceScreen, bytes } from "./InferenceScreen"; +import { CURATED_MODELS, DEFAULT_MODEL } from "./inferenceCatalog"; import type { Connection } from "../data/inferenceQueries"; const API = "http://visionset.test"; @@ -89,8 +90,8 @@ function connection(overrides: Partial = {}): Connection { id: "11111111-1111-4111-8111-111111111111", name: "sam2-local", connection_type: "local", - model_id: SUGGESTED_MODEL, - model_revision: SUGGESTED_REVISION, + model_id: DEFAULT_MODEL.modelId, + model_revision: DEFAULT_MODEL.revision, device: "cuda", precision: "fp16", endpoint_url: null, @@ -131,8 +132,8 @@ function sizeIs(totalBytes: number, fileCount = 3): void { on("GET", /^\/inference\/download-size$/, { status: 200, body: { - model_id: SUGGESTED_MODEL, - model_revision: SUGGESTED_REVISION, + model_id: DEFAULT_MODEL.modelId, + model_revision: DEFAULT_MODEL.revision, total_bytes: totalBytes, file_count: fileCount, }, @@ -174,7 +175,7 @@ it("shows the model and its revision the way a person reads them", async () => { listing([connection()]); render(mount()); expect( - await screen.findByText(`${SUGGESTED_MODEL} @ ${SUGGESTED_REVISION}`), + await screen.findByText(`${DEFAULT_MODEL.modelId} @ ${DEFAULT_MODEL.revision}`), ).not.toBeNull(); }); @@ -257,14 +258,20 @@ it("asks where the model runs before asking anything else", async () => { expect(screen.queryByTestId("connection-name")).toBeNull(); }); -it("pre-fills the local form with the suggested model", async () => { +it("pre-fills the local form with the default curated model, pinned", async () => { listing([]); sizeIs(1_200_000_000); render(mount()); await userEvent.click(await screen.findByTestId("new-connection")); await userEvent.click(await screen.findByTestId("choose-local")); - expect(value(await screen.findByTestId("connection-model"))).toBe(SUGGESTED_MODEL); - expect(value(screen.getByTestId("connection-revision"))).toBe(SUGGESTED_REVISION); + expect((await screen.findByTestId("connection-model")).textContent).toContain( + DEFAULT_MODEL.modelId, + ); + // A curated entry carries its own revision, so there is nothing to type and + // nothing left showing a branch name. + expect(screen.queryByTestId("connection-revision")).toBeNull(); + const asked = sent.find((one) => one.url.includes("download-size")); + expect(asked!.url).toContain(encodeURIComponent(DEFAULT_MODEL.revision)); }); it("shows the download size before anything is confirmed", async () => { @@ -321,7 +328,7 @@ it("sends only the fields the chosen kind carries", async () => { await userEvent.click(await screen.findByTestId("new-connection")); await userEvent.click(await screen.findByTestId("choose-http")); await userEvent.type(await screen.findByTestId("connection-name"), "remote"); - await userEvent.type(screen.getByTestId("connection-model"), "some/model"); + await userEvent.type(screen.getByTestId("connection-custom-model"), "some/model"); await userEvent.type(screen.getByTestId("connection-revision"), "abc123"); await userEvent.type(screen.getByTestId("connection-endpoint"), "https://example.invalid"); await userEvent.click(screen.getByTestId("connection-submit")); @@ -348,7 +355,7 @@ it("keeps what was typed when a create is refused", async () => { await userEvent.click(await screen.findByTestId("new-connection")); await userEvent.click(await screen.findByTestId("choose-http")); await userEvent.type(await screen.findByTestId("connection-name"), "remote"); - await userEvent.type(screen.getByTestId("connection-model"), "some/model"); + await userEvent.type(screen.getByTestId("connection-custom-model"), "some/model"); await userEvent.type(screen.getByTestId("connection-revision"), "abc123"); await userEvent.type(screen.getByTestId("connection-endpoint"), "https://example.invalid"); await userEvent.click(screen.getByTestId("connection-submit")); @@ -365,6 +372,253 @@ it("has no credential field, because where a secret lives is still open", async expect(screen.queryByLabelText(/credential|token|api key|secret/i)).toBeNull(); }); +// --- the curated list, and the fields that are closed sets ---------------------- + +it("offers every curated model, grouped, from the one module that holds them", async () => { + listing([]); + sizeIs(1_200_000_000); + render(mount()); + await userEvent.click(await screen.findByTestId("new-connection")); + await userEvent.click(await screen.findByTestId("choose-local")); + await userEvent.click(await screen.findByTestId("connection-model")); + + // Derived from the catalog rather than listed here: a model id spelled out in + // this file would be a second source, which is exactly what the module exists + // to prevent. + for (const group of CURATED_MODELS) { + expect(screen.getByText(group.label)).not.toBeNull(); + for (const model of group.models) { + const option = screen.getByRole("option", { name: new RegExp(model.modelId) }); + expect(option.textContent).toContain(bytes(model.totalBytes)); + expect(option.textContent).toContain(model.hint); + } + } +}); + +it("curates without restricting: Custom reveals the free model and revision", async () => { + listing([]); + sizeIs(1_200_000_000); + render(mount()); + await userEvent.click(await screen.findByTestId("new-connection")); + await userEvent.click(await screen.findByTestId("choose-local")); + expect(screen.queryByTestId("connection-custom-model")).toBeNull(); + + await userEvent.click(await screen.findByTestId("connection-model")); + await userEvent.click(screen.getByRole("option", { name: /Custom model/ })); + + const model = await screen.findByTestId("connection-custom-model"); + await userEvent.clear(model); + await userEvent.type(model, "someone/else"); + await userEvent.clear(screen.getByTestId("connection-revision")); + await userEvent.type(screen.getByTestId("connection-revision"), "deadbeef"); + expect(value(model)).toBe("someone/else"); + expect(value(screen.getByTestId("connection-revision"))).toBe("deadbeef"); +}); + +it("offers half precision only where an adapter would honour it", async () => { + listing([]); + sizeIs(1_200_000_000); + render(mount()); + await userEvent.click(await screen.findByTestId("new-connection")); + await userEvent.click(await screen.findByTestId("choose-local")); + + // A CPU connection: `fp16` is dropped by both adapters, so it is not on offer + // and the field says why rather than leaving the absence to be guessed at. + await userEvent.click(await screen.findByTestId("connection-precision")); + expect(screen.queryByRole("option", { name: "fp16" })).toBeNull(); + expect(screen.getByRole("option", { name: "fp32" })).not.toBeNull(); + await userEvent.keyboard("{Escape}"); + expect(screen.getByTestId("precision-hint").textContent).toContain("CUDA only"); + + await userEvent.click(screen.getByTestId("connection-device")); + await userEvent.click(screen.getByRole("option", { name: "cuda" })); + await userEvent.click(screen.getByTestId("connection-precision")); + expect(screen.getByRole("option", { name: "fp16" })).not.toBeNull(); +}); + +it("moves the precision with the device rather than leaving a refused pair", async () => { + listing([]); + sizeIs(1_200_000_000); + const posted: Record[] = []; + handlers.push((request) => { + if (request.method !== "POST" || !request.url.endsWith("/inference/connections")) return; + return { status: 201, body: connection() }; + }); + render(mount()); + await userEvent.click(await screen.findByTestId("new-connection")); + await userEvent.click(await screen.findByTestId("choose-local")); + await userEvent.type(await screen.findByTestId("connection-name"), "sam2"); + + await userEvent.click(screen.getByTestId("connection-device")); + await userEvent.click(screen.getByRole("option", { name: "cuda" })); + await userEvent.click(screen.getByTestId("connection-precision")); + await userEvent.click(screen.getByRole("option", { name: "fp16" })); + // Back to the CPU, where `fp16` is not a thing the kernel accepts. + await userEvent.click(screen.getByTestId("connection-device")); + await userEvent.click(screen.getByRole("option", { name: "cpu" })); + + await userEvent.click(screen.getByTestId("connection-submit")); + await waitFor(() => expect(sent.some((one) => one.method === "POST")).toBe(true)); + const body = JSON.parse(await sent.find((one) => one.method === "POST")!.clone().text()); + posted.push(body as Record); + expect(body.device).toBe("cpu"); + expect(body.precision).toBe("fp32"); +}); + +it("renders the kernel's refusal of a pair it disagrees with, as prose", async () => { + // The form offers only what works; this is the other half of the same rule — + // the kernel is the authority, and whatever it refuses reaches a person in the + // words the kernel wrote. Nothing here is computed client-side. + listing([]); + sizeIs(1_200_000_000); + on("POST", /^\/inference\/connections$/, { + status: 422, + body: { + code: "INFERENCE_CONNECTION_INVALID", + message: "fp16 is not available on cpu; cpu runs in fp32", + }, + }); + render(mount()); + await userEvent.click(await screen.findByTestId("new-connection")); + await userEvent.click(await screen.findByTestId("choose-local")); + await userEvent.type(await screen.findByTestId("connection-name"), "sam2"); + await userEvent.click(screen.getByTestId("connection-submit")); + expect((await screen.findByTestId("connection-error")).textContent).toContain( + "fp16 is not available on cpu", + ); +}); + +it("shows a stored device the form does not offer instead of rewriting it", async () => { + // `cuda:1` is a device the kernel accepts and a form cannot enumerate — how + // many GPUs this machine has is not something the list can know. Opening the + // edit form must not quietly reassign the row to `cuda`. + listing([ + connection({ setup_state: "ready", device: "cuda:1", allowed_actions: ["update", "delete"] }), + ]); + sizeIs(1_200_000_000); + render(mount()); + await userEvent.click(await screen.findByTestId("actions-sam2-local")); + await userEvent.click(await screen.findByTestId("action-edit")); + expect((await screen.findByTestId("connection-device")).textContent).toContain("cuda:1"); +}); + +it("shows a curated model at another revision as a custom connection", async () => { + // The pair is the identity. A row naming a curated model at a revision the + // list does not pin is not that entry, and showing it as one would misreport + // which weights it runs. + listing([ + connection({ + model_revision: "0000000000000000000000000000000000000000", + setup_state: "ready", + allowed_actions: ["update", "delete"], + }), + ]); + sizeIs(1_200_000_000); + render(mount()); + await userEvent.click(await screen.findByTestId("actions-sam2-local")); + await userEvent.click(await screen.findByTestId("action-edit")); + expect(value(await screen.findByTestId("connection-revision"))).toBe( + "0000000000000000000000000000000000000000", + ); + expect((screen.getByTestId("connection-model")).textContent).toContain("Custom"); +}); + +// --- the download's whole life -------------------------------------------------- + +it("refreshes the row when the job finishes, with no reload", async () => { + // The bug this closes: the `202` invalidated the list, and nothing invalidated + // it again when the work actually finished — so the row sat at `Not set up` + // until the page was reloaded. + let ready = false; + handlers.push((request) => { + if (request.method !== "GET" || !new URL(request.url).pathname.endsWith("/connections")) return; + const row = ready + ? connection({ setup_state: "ready", allowed_actions: ["download_weights", "update", "delete"] }) + : connection(); + return { status: 200, body: { items: [row], total: 1 } }; + }); + on("POST", /\/download$/, { status: 202, body: job("queued") }); + handlers.push((request) => { + if (!request.url.includes("/background-jobs/")) return; + // The job settles, and the row it moved is what the next listing answers. + ready = true; + return { status: 200, body: job("succeeded", 1, 1) }; + }); + + render(mount()); + await userEvent.click(await screen.findByTestId("download-weights")); + await waitFor(() => + expect(screen.getByTestId("connection-status").textContent).toContain("Ready"), + ); + // And the row's control follows the state it is now in. + expect(screen.queryByTestId("download-weights")).toBeNull(); +}); + +it("surfaces a failed download as prose, and leaves the same action as the retry", async () => { + listing([connection()]); + on("POST", /\/download$/, { status: 202, body: job("queued") }); + on("GET", /^\/background-jobs\/job-1$/, { + status: 200, + body: { + ...(job("failed") as Record), + error: "could not fetch facebook/sam2.1-hiera-base-plus at b73207: the connection was lost", + }, + }); + render(mount()); + await userEvent.click(await screen.findByTestId("download-weights")); + + const shown = await screen.findByTestId("download-error"); + expect(shown.textContent).toContain("the connection was lost"); + // What happened, and what to do about it — including that nothing is half done. + expect(shown.textContent).toContain("still Not set up"); + expect(shown.textContent).toContain("resumes"); + // The never-half-ready invariant, at the layer a person reads it. + expect(screen.getByTestId("connection-status").textContent).toContain("Not set up"); + // The retry *is* the action: no second control appeared, and this one is live. + const retry = screen.getByTestId("download-weights") as HTMLButtonElement; + expect(retry.disabled).toBe(false); + expect(screen.queryByText(/retry/i)).toBeNull(); +}); + +it("offers Verify weights in the overflow once a connection is ready", async () => { + listing([ + connection({ setup_state: "ready", allowed_actions: ["download_weights", "update", "delete"] }), + ]); + render(mount()); + await screen.findByTestId("connections-table"); + // Not the prominent control — there is nothing to fetch, only something to check. + expect(screen.queryByTestId("download-weights")).toBeNull(); + + await userEvent.click(screen.getByTestId("actions-sam2-local")); + expect(await screen.findByTestId("action-verify-weights")).not.toBeNull(); +}); + +it("does not offer Verify weights when the wire withholds the action", async () => { + // The same `setup_state`, so a screen deriving the item from the row's state + // would still render it. Only `allowed_actions` gets this right. + listing([connection({ setup_state: "ready", allowed_actions: ["update", "delete"] })]); + render(mount()); + await userEvent.click(await screen.findByTestId("actions-sam2-local")); + await screen.findByTestId("action-edit"); + expect(screen.queryByTestId("action-verify-weights")).toBeNull(); +}); + +it("runs the same request for Verify weights as for Download weights", async () => { + listing([ + connection({ setup_state: "ready", allowed_actions: ["download_weights", "update", "delete"] }), + ]); + on("POST", /\/download$/, { status: 202, body: job("queued") }); + on("GET", /^\/background-jobs\/job-1$/, { status: 200, body: job("succeeded", 1, 1) }); + render(mount()); + await userEvent.click(await screen.findByTestId("actions-sam2-local")); + await userEvent.click(await screen.findByTestId("action-verify-weights")); + await waitFor(() => + expect( + sent.some((one) => one.method === "POST" && one.url.endsWith("/download")), + ).toBe(true), + ); +}); + // --- editing and deleting ------------------------------------------------------ it("edits without offering to change the kind", async () => { diff --git a/frontend/ui-core/src/screens/inferenceCatalog.test.ts b/frontend/ui-core/src/screens/inferenceCatalog.test.ts new file mode 100644 index 00000000..32ba7ef2 --- /dev/null +++ b/frontend/ui-core/src/screens/inferenceCatalog.test.ts @@ -0,0 +1,83 @@ +/** + * The curated list's own rules, which no screen test can see. + * + * A form test can prove the list is rendered from this module. It cannot prove + * that an entry added later carries a commit rather than a branch, or that the + * default is one of the entries at all — and those are exactly the mistakes an + * addition makes, because the type accepts a plausible-looking string for both. + */ + +import { expect, it } from "vitest"; + +import { + CURATED_BY_ID, + CURATED_MODELS, + CUSTOM_MODEL, + DEFAULT_MODEL, + DEVICES, + curatedEntry, + precisionOn, + precisionsFor, +} from "./inferenceCatalog"; + +const EVERY_MODEL = CURATED_MODELS.flatMap((group) => group.models); + +it("pins every curated entry to a commit, never to a moving pointer", () => { + // The form's own helper text says a moving pointer is not a provenance. A + // curated list that pinned `main` would be saying it while doing the opposite, + // and the size beside the entry would describe whatever the branch last was. + for (const model of EVERY_MODEL) { + expect(model.revision).toMatch(/^[0-9a-f]{40}$/); + } +}); + +it("says what each entry costs and what it is for", () => { + for (const model of EVERY_MODEL) { + expect(model.totalBytes).toBeGreaterThan(0); + expect(model.hint.trim()).not.toBe(""); + } +}); + +it("names every entry once, so a lookup cannot be ambiguous", () => { + expect(CURATED_BY_ID.size).toBe(EVERY_MODEL.length); +}); + +it("defaults to an entry the list actually holds", () => { + // `DEFAULT_MODEL` is resolved by id, so a rename that missed it would leave + // the form opening on `undefined` — a blank select and no revision. + expect(DEFAULT_MODEL).toBeDefined(); + expect(EVERY_MODEL).toContain(DEFAULT_MODEL); +}); + +it("keeps the custom sentinel out of the model ids it could collide with", () => { + expect(CURATED_BY_ID.has(CUSTOM_MODEL)).toBe(false); +}); + +it("treats the pair as the identity of a curated entry", () => { + expect(curatedEntry(DEFAULT_MODEL.modelId, DEFAULT_MODEL.revision)).toBe(DEFAULT_MODEL); + // The same model at another revision is a custom connection wearing a + // familiar name, and calling it the curated entry would misreport its weights. + expect(curatedEntry(DEFAULT_MODEL.modelId, "deadbeef")).toBeUndefined(); + expect(curatedEntry("someone/else", DEFAULT_MODEL.revision)).toBeUndefined(); +}); + +it("offers half precision on CUDA and on every address of it", () => { + expect(precisionsFor("cpu")).toEqual(["fp32"]); + expect(precisionsFor("cuda")).toEqual(["fp16", "fp32"]); + // A second GPU is still a GPU. This is the kernel's `precisions_for`, and the + // two answer the same way or the form offers what the kernel refuses. + expect(precisionsFor("cuda:1")).toEqual(["fp16", "fp32"]); +}); + +it("keeps a precision that survives a device change and replaces one that does not", () => { + expect(precisionOn("cuda", "fp32")).toBe("fp32"); + expect(precisionOn("cpu", "fp32")).toBe("fp32"); + expect(precisionOn("cpu", "fp16")).toBe("fp32"); +}); + +it("offers the two devices every machine can be asked about", () => { + // `cuda:N` is deliberately absent: how many GPUs this machine has is not + // something a static list can know, so it is typed by the kernel's pattern and + // shown by the form only when a row already carries one. + expect([...DEVICES]).toEqual(["cpu", "cuda"]); +}); diff --git a/frontend/ui-core/src/screens/inferenceCatalog.ts b/frontend/ui-core/src/screens/inferenceCatalog.ts new file mode 100644 index 00000000..f045b57e --- /dev/null +++ b/frontend/ui-core/src/screens/inferenceCatalog.ts @@ -0,0 +1,192 @@ +/** + * What the connection form offers: the curated models, the devices, the precisions. + * + * **One module, because a second one is how a list starts disagreeing with + * itself.** The form reads this and nothing else, so adding a model is an entry + * here and no other edit — and a model id hardcoded anywhere in `screens/` is a + * bug this file exists to make visible. + * + * ## Curation guides, it never restricts + * + * Every entry below is a checkpoint this build has an adapter for, and + * {@link CUSTOM_MODEL} is beside them: any model id remains typeable, with its + * own revision, exactly as before. What curation buys is that the six obvious + * choices are one click away and each one is known to work, rather than being a + * name somebody has to already know how to spell. + * + * ## Each entry is verified rather than hoped for + * + * Before an entry lands here, three things are established against the locked + * `transformers` and the publishing hub: the `model_type` its config declares + * lands in the resolver's supported family sets (`inference/providers.py`), the + * revision is a real commit, and the download size is the hub's own figure for + * that revision. A candidate that fails any of them is dropped rather than + * shipped with a hopeful comment. + * + * **The revision is a commit hash and never `main`.** The form's own helper text + * says a moving pointer is not a provenance; a curated list that pinned a branch + * would be saying it while doing the opposite, and the size beside the entry + * would describe whatever the branch pointed at last week. + * + * **The size is safe to hold as a constant** for the same reason: a pinned + * revision is an immutable set of files, so the number cannot go stale. It is + * what the list shows while somebody is still choosing; the line under the field + * reads the same fact live from the size endpoint, and that is the one a person + * confirms a download against. + * + * ## The two closed vocabularies + * + * `device` and `precision` are the kernel's vocabularies + * (`kernel/domain/inference.py`), and the kernel is what refuses a pair outside + * them — including `cpu` with `fp16`, which both local adapters silently drop. + * What lives here is the *offering*: which members a form puts on screen and in + * what order. That is not the hand-mirror `ui-capabilities` bans — that rule is + * about `allowed_actions`, which no field-level shape can carry — and the + * refusal still arrives from the server and still renders as prose, which + * `inference.test.tsx` holds. + */ + +import type { Precision } from "../data/inferenceQueries"; + +export type { Precision }; + +/** One curated checkpoint: what it is, what it costs, and why you would pick it. */ +export interface CuratedModel { + readonly modelId: string; + /** The commit verified at curation time. Never a branch — see the module note. */ + readonly revision: string; + /** The hub's figure for that revision, every file included. */ + readonly totalBytes: number; + /** One line, the difference between this rung and its neighbours. */ + readonly hint: string; +} + +/** A family of curated models, named by the question its models answer. */ +export interface CuratedGroup { + readonly label: string; + readonly models: readonly CuratedModel[]; +} + +/** + * The models this build has an adapter for, grouped by what you ask them. + * + * Both groups are Apache-2.0 checkpoints published by the people who trained + * them, which is the neutral-sources rule this product configures itself under: + * a curated list points at originals, never at a re-publisher or a mirror. + * + * The ladders are complete on purpose. Offering only a middle rung would make + * the choice between "runs on this laptop" and "as accurate as this build gets" + * something a person has to leave the form to discover. + */ +export const CURATED_MODELS: readonly CuratedGroup[] = [ + { + label: "Interactive segmentation (point prompts)", + models: [ + { + modelId: "facebook/sam2.1-hiera-tiny", + revision: "de431c4043854a71d8101e17995dfe596bf101a5", + totalBytes: 311_949_047, + hint: "tiny — fastest, comfortable on a CPU", + }, + { + modelId: "facebook/sam2.1-hiera-small", + revision: "ee5bba1d82bb8749febdf90f45e84b687142ba03", + totalBytes: 368_754_000, + hint: "small — a little more accurate, still light", + }, + { + modelId: "facebook/sam2.1-hiera-base-plus", + revision: "b7320756a13354e7530a63935656d35b2f91a290", + totalBytes: 647_115_465, + hint: "base-plus — the balanced default", + }, + { + modelId: "facebook/sam2.1-hiera-large", + revision: "665f8e2ad61cf5f53d65644ff27c8ee525124610", + totalBytes: 1_796_013_511, + hint: "large — the most accurate, wants a GPU", + }, + ], + }, + { + label: "Text-prompt detection", + models: [ + { + modelId: "IDEA-Research/grounding-dino-tiny", + revision: "a2bb814dd30d776dcf7e30523b00659f4f141c71", + totalBytes: 1_382_224_246, + hint: "tiny — fastest, comfortable on a CPU", + }, + { + modelId: "IDEA-Research/grounding-dino-base", + revision: "12bdfa3120f3e7ec7b434d90674b3396eccf88eb", + totalBytes: 1_870_353_436, + hint: "base — more accurate, wants a GPU", + }, + ], + }, +]; + +/** + * The select's value for "none of these — let me type one". + * + * A sentinel rather than an empty string, because empty is what the field holds + * before anything is chosen and the two mean different things. + */ +export const CUSTOM_MODEL = "custom"; + +/** Every curated entry, flat, for a lookup by model id. */ +export const CURATED_BY_ID: ReadonlyMap = new Map( + CURATED_MODELS.flatMap((group) => group.models).map((model) => [model.modelId, model]), +); + +/** + * What a new local connection starts on. + * + * The balanced rung of the point-prompt ladder, and the pinned successor of the + * single model this form suggested before there was a list — so the default a + * person meets does not move under them, it only stops being a branch. + */ +export const DEFAULT_MODEL: CuratedModel = CURATED_BY_ID.get("facebook/sam2.1-hiera-base-plus")!; + +/** + * The entry a stored connection is showing, or `undefined` if it is a custom one. + * + * Both halves are compared. A row naming a curated model at a *different* + * revision is a custom connection wearing a familiar name, and showing it as the + * curated entry would misreport which weights it runs. + */ +export function curatedEntry(modelId: string, revision: string): CuratedModel | undefined { + const found = CURATED_BY_ID.get(modelId); + return found !== undefined && found.revision === revision ? found : undefined; +} + +/** The devices a form offers, in the order it offers them. */ +export const DEVICES = ["cpu", "cuda"] as const; + +/** + * The precisions that are honoured on that device — the kernel's + * `precisions_for`, offering-side. + * + * Half precision is CUDA-only in both local adapters, so `cpu` + `fp16` is not a + * slower run but a setting with no effect that the row would go on displaying as + * though it had one. A machine addressing a second GPU writes `cuda:1`, which is + * not a member of {@link DEVICES} and is still a CUDA device — hence the prefix + * test rather than an equality against `"cuda"`. + */ +export function precisionsFor(device: string): readonly Precision[] { + return device.startsWith("cuda") ? ["fp16", "fp32"] : ["fp32"]; +} + +/** + * The precision to select when the device changes under an existing choice. + * + * Keeps what was chosen when it survives the move, so switching to `cuda` and + * back does not quietly rewrite somebody's `fp32`. Falls to the first offered + * member when it does not — which is the whole of "a curated model on `cpu` + * defaults to `fp32`". + */ +export function precisionOn(device: string, current: Precision): Precision { + const offered = precisionsFor(device); + return offered.includes(current) ? current : offered[0]!; +} diff --git a/openapi.json b/openapi.json index ffce7be9..d57f0669 100644 --- a/openapi.json +++ b/openapi.json @@ -1545,13 +1545,12 @@ "precision": { "anyOf": [ { - "type": "string" + "$ref": "#/components/schemas/Precision" }, { "type": "null" } - ], - "title": "Precision" + ] } }, "required": [ @@ -1623,13 +1622,12 @@ "precision": { "anyOf": [ { - "type": "string" + "$ref": "#/components/schemas/Precision" }, { "type": "null" } - ], - "title": "Precision" + ] }, "setup_state": { "$ref": "#/components/schemas/ConnectionSetupState" @@ -1758,13 +1756,12 @@ "precision": { "anyOf": [ { - "type": "string" + "$ref": "#/components/schemas/Precision" }, { "type": "null" } - ], - "title": "Precision" + ] } }, "title": "ConnectionUpdate", @@ -2604,6 +2601,15 @@ "title": "PolylineGeometry", "type": "object" }, + "Precision": { + "description": "The numeric precision a local connection asks its weights to be loaded in.\n\nA closed vocabulary rather than the free text this field started as, on\n``ConnectionType``'s test: the set is small, the kernel is what decides\nwhether a member is usable on a given device, and it grows only by a\ndeliberate kernel change \u2014 bf16 arriving later is exactly that change.\n\nFree text here was not neutrality but a gap. ``fp32x`` was accepted and then\nignored; so was ``fp16`` beside ``cpu``, which the adapters silently drop\n(see :func:`precisions_for`). A field whose wrong values are absorbed rather\nthan refused is a field that cannot tell somebody they are configuring a run\nthat will not happen.", + "enum": [ + "fp16", + "fp32" + ], + "title": "Precision", + "type": "string" + }, "ProgressCounts": { "description": "How many assets sit in each annotation state.", "properties": { diff --git a/src/visionset/cli/inference.py b/src/visionset/cli/inference.py index 41cdff8e..6b4fa82a 100644 --- a/src/visionset/cli/inference.py +++ b/src/visionset/cli/inference.py @@ -31,7 +31,7 @@ from visionset.cli._output import JsonOption, document, note, table from visionset.cli._workspace import WorkspaceOption, opened_workspace from visionset.inference import download_size, fetch_weights -from visionset.kernel.domain import ConnectionType, InferenceConnection +from visionset.kernel.domain import ConnectionType, InferenceConnection, Precision from visionset.kernel.services import InferenceConnectionService inference_app = typer.Typer( @@ -71,10 +71,12 @@ def inference_create( str, typer.Option("--revision", help="Pinned. A moving pointer is not a provenance.") ], device: Annotated[ - str | None, typer.Option("--device", help="Local only. For example cuda or cpu.") + str | None, + typer.Option("--device", help="Local only. cpu, cuda, or cuda:N for a second GPU."), ] = None, precision: Annotated[ - str | None, typer.Option("--precision", help="Local only. For example fp16.") + Precision | None, + typer.Option("--precision", help="Local only. fp16 needs a cuda device."), ] = None, endpoint_url: Annotated[ str | None, typer.Option("--endpoint", help="HTTP only. Where to send predictions.") @@ -122,8 +124,14 @@ def inference_update( name: Annotated[str | None, typer.Option("--name", help="Rename it.")] = None, model_id: Annotated[str | None, typer.Option("--model", help="Point at another model.")] = None, model_revision: Annotated[str | None, typer.Option("--revision", help="Move the pin.")] = None, - device: Annotated[str | None, typer.Option("--device", help="Local only.")] = None, - precision: Annotated[str | None, typer.Option("--precision", help="Local only.")] = None, + device: Annotated[ + str | None, + typer.Option("--device", help="Local only. cpu, cuda, or cuda:N for a second GPU."), + ] = None, + precision: Annotated[ + Precision | None, + typer.Option("--precision", help="Local only. fp16 needs a cuda device."), + ] = None, endpoint_url: Annotated[str | None, typer.Option("--endpoint", help="HTTP only.")] = None, json_out: JsonOption = False, workspace: WorkspaceOption = None, diff --git a/src/visionset/inference/_fp16.py b/src/visionset/inference/_fp16.py index 756865e7..5d6c64e3 100644 --- a/src/visionset/inference/_fp16.py +++ b/src/visionset/inference/_fp16.py @@ -39,9 +39,12 @@ HALF_PRECISION_NAMES = frozenset({"fp16", "float16", "half"}) """What a connection's ``precision`` may say to mean half. -A set rather than one spelling because the field is free text by design — -``InferenceConnection.precision`` documents that — and refusing ``float16`` from -somebody who wrote what torch calls it would be a trap rather than a rule. +Wider than the vocabulary that can now reach it. ``Precision`` closed the field +around ``fp16``/``fp32`` (#469) and the domain normalizes the other two spellings +onto it, so this set's extra members are what stops a *direct* caller — a test +holding a stand-in, a future adapter reading a value from somewhere else — from +having to know which spelling won. Keeping them costs a frozenset lookup and +removes a way for two modules to disagree about what ``float16`` means. """ diff --git a/src/visionset/inference/weights.py b/src/visionset/inference/weights.py index 21641d97..3e6aa1af 100644 --- a/src/visionset/inference/weights.py +++ b/src/visionset/inference/weights.py @@ -22,11 +22,22 @@ state meaning "some of it arrived". That is an ordering rather than a guard, which is why nothing in the domain has to encode it. -**Idempotent, and it is the handler's idempotency that needs it.** A connection -already ``ready`` is verified — the files are looked for — and left alone. That -is not a convenience for people typing twice: the download job is registered -idempotent, and an orphan re-queued after a crash arrives at a connection a -previous attempt already finished. +**Idempotent, and two callers need it to be.** A connection already ``ready`` is +re-checked — every file the revision names is looked for, and anything missing is +fetched — and then left alone. That is not only a convenience for people typing +twice: the download job is registered idempotent, an orphan re-queued after a +crash arrives at a connection a previous attempt already finished, and since #469 +so does somebody asking a set-up connection to check itself. + +**What that check is, precisely.** ``huggingface_hub`` addresses its cache by the +revision's commit hash and each file's etag, so a re-run at a pinned revision +finds the files it already has and returns them without reading their bytes. +What a re-run therefore proves is that the snapshot is **complete** — the common +failure, since a download interrupted by a network or a full disk leaves it +incomplete — and what it does not prove is that a file already on disk still +holds the bytes it was written with. Saying the difference out loud is the point: +a control labelled as a check must not imply an integrity guarantee the library +underneath it does not make. """ from __future__ import annotations @@ -72,7 +83,6 @@ def fetch_weights( workspace: WorkspaceService, connection_id: UUID, *, - retrying: bool = False, on_progress: Callable[[str], None] | None = None, ) -> InferenceConnection: """Fetch the weights this connection names, then mark it ready. @@ -83,13 +93,14 @@ def fetch_weights( this sequence is how the CLI and the API would come to disagree about what "set up" means. - ``retrying`` is passed by the job handler and by nobody else, and it is what - makes a re-run safe rather than merely cheap: an orphan re-enqueued after a - crash may arrive at a connection a previous attempt already finished, and - ``require_downloadable`` explains why refusing that would be wrong. The work - it then does is a *verification* — the snapshot download checks a cache it - already filled against its hashes rather than re-fetching it — and the write - below is a no-op on a connection that is already ready. + **A run against a connection that is already ``ready`` is a re-check, and it + needs no flag to be one (#469).** The snapshot download finds what the cache + already holds and fetches only what is missing, and the write below is a + no-op on a connection that is already ready — so the orphan the queue + re-enqueues after a crash and the person asking a set-up connection to check + itself take the identical path. What it establishes is completeness rather + than integrity; see this module's docstring for why the distinction is worth + a paragraph. ``on_progress`` is a plain callable rather than a ``ProgressReporter``, because what this can honestly report is a *phase* and not a count: a @@ -100,12 +111,12 @@ def fetch_weights( Raises: InferenceConnectionNotFound: no such connection in this workspace. - InferenceConnectionNotDownloadable: it is already set up, or it is a kind - with no weights of its own. + InferenceConnectionNotDownloadable: it is a kind with no weights of its + own. LocalInferenceUnavailable: the optional runtime is not installed. """ connections = InferenceConnectionService(workspace) - connection = connections.require_downloadable(connection_id, retrying=retrying) + connection = connections.require_downloadable(connection_id) say = on_progress or (lambda _: None) say(f"fetching {connection.model_id} at {connection.model_revision}") @@ -128,9 +139,13 @@ def download(connection: InferenceConnection, *, into: Path) -> Path: whose identity the row now misdescribes — which is the provenance failure the pin exists to prevent. - Idempotent by the library's own design: a snapshot already in the cache is - verified against its hashes and not re-fetched, which is what makes a - re-run of the job cheap rather than merely safe. + Idempotent by the library's own design, and idempotent in a specific way. A + file already in the cache under this revision's commit hash is returned + without being re-read, so a re-run costs a metadata call per file and fetches + only what is absent; a file that arrives is checked against the size the hub + published for it before it is moved into place, and a transfer interrupted + part-way resumes from what it had. So a re-run repairs a snapshot that is + incomplete and cannot detect one that is complete but damaged. Raises: LocalInferenceUnavailable: ``huggingface_hub`` is not installed, or the diff --git a/src/visionset/jobs/weights.py b/src/visionset/jobs/weights.py index f88e2e29..57d43b62 100644 --- a/src/visionset/jobs/weights.py +++ b/src/visionset/jobs/weights.py @@ -81,11 +81,10 @@ def run( # Never a ``with``: the handle belongs to the worker and outlives this task. # See ``jobs/context.py``. workspace = workspace_for(workspace_root) - # ``retrying``: this handler is only ever entered for work a surface - # already gated, and a crash between the state flip and the row settling - # re-enqueues it against a connection that is now ``ready``. See - # ``require_downloadable``. - ready = fetch_weights(workspace, connection_id, retrying=True) + # No flag for the re-run: ``download_weights`` is legal at ``ready`` too + # (#469), so an orphan re-enqueued after a crash and a person asking a + # set-up connection to check itself are the same idempotent call. + ready = fetch_weights(workspace, connection_id) reporter.report(processed=1, total=1) return { "connection_id": str(ready.id), diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index 93ba50ba..23983c63 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -66,12 +66,18 @@ PolylineGeometry, ) from visionset.kernel.domain.inference import ( + CPU, + CUDA, + DEVICE_PATTERN, EVERY_CONNECTION_TYPE, EVERY_SETUP_STATE, + OFFERED_DEVICES, ConnectionSetupState, ConnectionType, DownloadSize, InferenceConnection, + Precision, + precisions_for, ) from visionset.kernel.domain.ingest import ( INGEST_TRANSITIONS, @@ -201,6 +207,11 @@ "CONNECTION_KINDS", "EVERY_CONNECTION_TYPE", "EVERY_SETUP_STATE", + "CPU", + "CUDA", + "DEVICE_PATTERN", + "OFFERED_DEVICES", + "precisions_for", "ASSET_MOVES", "ASSET_PROGRESS_TRANSITIONS", "BACKGROUND_JOB_TRANSITIONS", @@ -259,6 +270,7 @@ "DatasetStats", "DomainEvent", "DownloadSize", + "Precision", "ClassCompatibility", "ClassExportStatus", "ExportCompatibility", diff --git a/src/visionset/kernel/domain/capabilities.py b/src/visionset/kernel/domain/capabilities.py index 6529df45..d70aa4da 100644 --- a/src/visionset/kernel/domain/capabilities.py +++ b/src/visionset/kernel/domain/capabilities.py @@ -259,14 +259,31 @@ def offered_from(self, current: S, transitions: Mapping[S, frozenset[S]]) -> boo CONNECTION_GATES: Final[Mapping[ConnectionAction, frozenset[ConnectionSetupState]]] = { - ConnectionAction.DOWNLOAD_WEIGHTS: frozenset({ConnectionSetupState.NOT_SET_UP}), + ConnectionAction.DOWNLOAD_WEIGHTS: EVERY_SETUP_STATE, ConnectionAction.UPDATE: EVERY_SETUP_STATE, ConnectionAction.DELETE: EVERY_SETUP_STATE, } """Which setup states each connection action is legal in. +**``download_weights`` is legal in both, and that is a decision rather than a +widening for convenience (#469).** The work behind it is idempotent by the +download library's own design: files already in the cache under this revision +are found rather than re-fetched, and ``record_weights_ready`` returns a +``ready`` connection unchanged. So the same request against a ``ready`` +connection answers "is this snapshot still complete?" — a real question on a +machine where a disk filled or a cache was pruned mid-download — and it had no +action at all before. It is completeness rather than integrity, and +``visionset.inference.weights`` says why that distinction is worth keeping. A +client renders it under its own label; the wire keeps one name, because it is +one call doing one thing. + +That leaves this table unconditional in every row, and the conditionality +entirely in :data:`CONNECTION_KINDS`. The table stays rather than folding away: +two maps read together is what makes a later narrowing a one-line edit here +instead of a new dimension, and ``connection_actions`` requires both. + There is still no ``CONNECTION_MOVES`` and no transition table beside this, -even though ``download_weights`` now moves ``setup_state``. A transition table +even though ``download_weights`` can move ``setup_state``. A transition table earns its place when a state has more than one way out and the edges need naming — ``BATCH_TRANSITIONS`` has eight. Here there is exactly one edge, ``not_set_up -> ready``, and it is not a move somebody *performs*: it is what a finished @@ -275,11 +292,9 @@ def offered_from(self, current: S, transitions: Mapping[S, frozenset[S]]) -> boo the question a client actually asks — may this be downloaded — and the single edge lives in the service that writes it. -``update`` and ``delete`` name +Every entry names :data:`~visionset.kernel.domain.inference.EVERY_SETUP_STATE` itself rather than a frozenset spelled out here, the discipline ``DELETABLE_STATES`` gets above. -``download_weights`` spells its one member out because that *is* the rule, and -there is no existing set for "the state weights are missing in" to name instead. """ @@ -311,15 +326,19 @@ def connection_actions( ) -> list[ConnectionAction]: """Everything this connection does not refuse, in declaration order. - Both dimensions, and neither is optional. A local connection that has not - fetched its weights declares ``download_weights``; the same connection once - ready does not, because there is nothing left to fetch; and an ``http`` - connection never does in any state, because it has no weights of its own. - That is the whole of :data:`CONNECTION_GATES` and :data:`CONNECTION_KINDS` - read together, and `InferenceConnectionService.require_downloadable` gates on - this same function rather than restating either table — the hand-mirror this - module exists to prevent, and the antipattern this repository has paid for - twice. + Both dimensions, and neither is optional. A local connection declares + ``download_weights`` in either state — fetching what is missing, verifying + what is there — and an ``http`` connection never does in any state, because + it has no weights of its own. That is the whole of :data:`CONNECTION_GATES` + and :data:`CONNECTION_KINDS` read together, and + `InferenceConnectionService.require_downloadable` gates on this same function + rather than restating either table — the hand-mirror this module exists to + prevent, and the antipattern this repository has paid for twice. + + A client that wants to *label* the two readings differently reads + ``setup_state``, which is a field of the resource and not a second capability + table. Deriving the word on a control from a state the wire states is not the + banned mirror; computing whether the control may exist would be. ``download_weights`` being declared says the *connection* is ready to be asked, never that this installation can carry it out: whether the local diff --git a/src/visionset/kernel/domain/inference.py b/src/visionset/kernel/domain/inference.py index e0e0f6ed..2dae209d 100644 --- a/src/visionset/kernel/domain/inference.py +++ b/src/visionset/kernel/domain/inference.py @@ -29,6 +29,8 @@ from __future__ import annotations +import re +from collections.abc import Mapping from datetime import UTC, datetime from enum import StrEnum from typing import Final @@ -69,6 +71,92 @@ class ConnectionSetupState(StrEnum): READY = "ready" +class Precision(StrEnum): + """The numeric precision a local connection asks its weights to be loaded in. + + A closed vocabulary rather than the free text this field started as, on + ``ConnectionType``'s test: the set is small, the kernel is what decides + whether a member is usable on a given device, and it grows only by a + deliberate kernel change — bf16 arriving later is exactly that change. + + Free text here was not neutrality but a gap. ``fp32x`` was accepted and then + ignored; so was ``fp16`` beside ``cpu``, which the adapters silently drop + (see :func:`precisions_for`). A field whose wrong values are absorbed rather + than refused is a field that cannot tell somebody they are configuring a run + that will not happen. + """ + + FP16 = "fp16" + FP32 = "fp32" + + +_PRECISION_ALIASES: Final[Mapping[str, Precision]] = { + "float16": Precision.FP16, + "half": Precision.FP16, + "float32": Precision.FP32, + "full": Precision.FP32, +} +"""Spellings this build has honoured, mapped onto the vocabulary rather than refused. + +``visionset.inference._fp16.HALF_PRECISION_NAMES`` has accepted ``float16`` and +``half`` for as long as the field has existed, so a row already carrying one was +written by somebody following the product rather than by somebody guessing. The +vocabulary closing around it must not make that row unreadable — a value the +domain refuses is refused on the way *out* of the store as well as into it — so +the alias is normalized at the boundary and the closed set is what everything +downstream sees. The full-precision pair is here for symmetry: honouring one +spelling of half and none of full is the kind of asymmetry nobody can remember. + +Anything outside this map and the vocabulary is refused, which is the point. +""" + + +CPU: Final = "cpu" +"""The device every machine has, and the only one that needs no vocabulary escape.""" + +CUDA: Final = "cuda" +"""The default GPU. A machine with several addresses the rest as ``cuda:1``, ``cuda:2``…""" + +OFFERED_DEVICES: Final[tuple[str, ...]] = (CPU, CUDA) +"""The devices a form offers, in the order it offers them. + +Not the whole of what :data:`DEVICE_PATTERN` accepts, and the difference is +deliberate: ``cuda:N`` is an escape for the machine with more than one GPU, which +is a fact about *that* machine and not a choice a form can enumerate. A client +holding a connection whose device is outside this tuple shows it as it is rather +than silently rewriting it to the nearest member. +""" + +DEVICE_PATTERN: Final = re.compile(r"^(?:cpu|cuda(?::\d+)?)$") +"""Every device string this build can honestly run on. + +A pattern rather than an enum because of the one member that is not a fixed +word. What is *not* here is the point: ``gpu``, ``mps``, ``auto`` and every +typo were accepted before and then quietly fell back to the CPU in full +precision — a connection that names a runtime it never gets. The adapters still +fall back when a *valid* device turns out to be absent at run time, which is a +fact about the machine at the moment of the call and belongs there; a device +nothing could ever address is a fact about the configuration and belongs here. +""" + + +def precisions_for(device: str) -> tuple[Precision, ...]: + """The precisions that are honoured on that device, in offering order. + + The conditioning rule, stated once, where the validator below and every + surface that offers a choice can read the same answer. Half precision is + CUDA-only: both local adapters resolve ``half`` as *this device is CUDA and + the connection asked for fp16*, so ``cpu`` + ``fp16`` is not a slow run but a + setting that has no effect at all — and one the row would go on displaying as + though it did. + + Takes the string rather than a member because ``cuda:1`` is a device and not + an enum, and returns a tuple rather than a set because a caller offering a + choice needs an order and a caller checking membership does not care. + """ + return (Precision.FP32,) if device == CPU else (Precision.FP16, Precision.FP32) + + EVERY_CONNECTION_TYPE: Final[frozenset[ConnectionType]] = frozenset(ConnectionType) """The kinds that refuse nothing — the type half of an unconditional capability. @@ -154,11 +242,14 @@ class InferenceConnection(BaseModel): #: model produced this label" is unanswerable if the answer is a name that #: means something different next month. model_revision: str - #: ``local`` only. Free text — ``cuda``, ``cuda:1``, ``cpu`` — because what is - #: addressable is a property of the machine at run time, not of this domain. + #: ``local`` only. ``cpu``, ``cuda``, or ``cuda:N`` on a machine with more + #: than one — :data:`DEVICE_PATTERN` is the whole of it. Whether the named + #: device is *present* is a property of the machine at run time and stays + #: there; whether it is a device at all is a property of the configuration + #: and is settled here. device: str | None = None - #: ``local`` only. Free text — ``fp16``, ``fp32`` — for the same reason. - precision: str | None = None + #: ``local`` only, and conditioned on the device — see :func:`precisions_for`. + precision: Precision | None = None #: ``http`` only. endpoint_url: str | None = None setup_state: ConnectionSetupState = ConnectionSetupState.NOT_SET_UP @@ -179,6 +270,39 @@ def _is_not_blank(cls, value: str, info: object) -> str: raise ValueError(f"{field} must contain at least one non-blank character") return value + @field_validator("device", mode="before") + @classmethod + def _is_a_device_this_build_can_address(cls, value: object) -> object: + """Case and surrounding space are forgiven; the vocabulary is not. + + ``before`` because the normalization has to happen for the pattern to + judge the same string the adapters will read — `` CUDA `` and ``cuda`` + are one device written two ways, while ``gpu`` is not a device. + """ + if not isinstance(value, str): + return value + device = value.strip().casefold() + if not DEVICE_PATTERN.match(device): + raise ValueError( + f"{value!r} is not a device this build can run on; use " + f"{', '.join(OFFERED_DEVICES)}, or cuda:N for a second GPU" + ) + return device + + @field_validator("precision", mode="before") + @classmethod + def _is_a_precision_this_build_offers(cls, value: object) -> object: + """The vocabulary, plus the spellings of it this build already honoured. + + Returns the raw string when it is neither, so that the enum itself + writes the refusal and there is one sentence listing the members rather + than two that could disagree. + """ + if not isinstance(value, str): + return value + precision = value.strip().casefold() + return _PRECISION_ALIASES.get(precision, precision) + @field_validator("created_at", "updated_at") @classmethod def _is_timezone_aware(cls, value: datetime) -> datetime: @@ -194,6 +318,12 @@ def _parameters_match_the_type(self) -> InferenceConnection: connection holding an ``endpoint_url`` is the shape that makes a later reader ask which field the adapter should believe, and ``Source`` pays for the same rule with ``validate_assignment``. + + The device and the precision are then checked *against each other*, + which no field validator can do: each is a member of its own vocabulary + and the pair is what is legal or not. :func:`precisions_for` owns that + rule, so a surface offering a choice and the kernel refusing one are + reading the same function rather than two copies of one sentence. """ local = self.connection_type is ConnectionType.LOCAL required = ("device", "precision") if local else ("endpoint_url",) @@ -205,4 +335,12 @@ def _parameters_match_the_type(self) -> InferenceConnection: for field in forbidden: if getattr(self, field) is not None: raise ValueError(f"a {self.connection_type.value} connection cannot carry {field}") + if local: + assert self.device is not None and self.precision is not None # the loop above + offered = precisions_for(self.device) + if self.precision not in offered: + raise ValueError( + f"{self.precision.value} is not available on {self.device}; " + f"{self.device} runs in {', '.join(one.value for one in offered)}" + ) return self diff --git a/src/visionset/kernel/services/inference_connection_service.py b/src/visionset/kernel/services/inference_connection_service.py index 9c692b60..354ed0ab 100644 --- a/src/visionset/kernel/services/inference_connection_service.py +++ b/src/visionset/kernel/services/inference_connection_service.py @@ -33,7 +33,6 @@ from pydantic import ValidationError from visionset.kernel.domain import ( - CONNECTION_KINDS, ConnectionAction, ConnectionSetupState, ConnectionType, @@ -110,7 +109,8 @@ def create( Raises: InvalidName: the name is blank once stripped. InferenceConnectionNameTaken: another connection holds that name. - InferenceConnectionInvalid: the parameters do not match the kind. + InferenceConnectionInvalid: the parameters do not match the kind, or + the device or precision is outside what this build offers. """ try: with self._workspace.unit_of_work() as uow: @@ -153,7 +153,8 @@ def update( InferenceConnectionNotFound: no such connection in this workspace. InvalidName: a supplied name is blank once stripped. InferenceConnectionNameTaken: another connection holds that name. - InferenceConnectionInvalid: the result would not match the kind. + InferenceConnectionInvalid: the result would not match the kind, or + the device or precision is outside what this build offers. """ try: with self._workspace.unit_of_work() as uow: @@ -177,9 +178,7 @@ def update( except ConstraintViolated as exc: raise self._as_name_collision(exc, name or "") from exc - def require_downloadable( - self, connection_id: UUID, *, retrying: bool = False - ) -> InferenceConnection: + def require_downloadable(self, connection_id: UUID) -> InferenceConnection: """The connection, if fetching weights for it is something to do. The gate every download surface calls before it commits to anything — @@ -195,21 +194,19 @@ def require_downloadable( refusal the same answer: a client that saw ``download_weights`` in the declaration can call, and one that did not will be told why. - **``retrying`` is what a re-run of already-accepted work passes**, and it - is the state half of the gate and only that half. The question this - answers is normally "may this be *started*?", and a retry was started - already: ``sweep_orphans`` re-enqueues an idempotent orphan as a new job, - so a crash between the state flip committing and the row settling - produces a second run against a connection that is now ``ready``. - Refusing that would fail a job whose work is done. The **kind** half is - not relaxed by it — a connection with no weights of its own has none on - the second attempt either — so this is narrower than a flag that skips - the gate. + **A ``ready`` connection passes, and there is no ``retrying`` flag any + more (#469).** The flag existed because the gate refused ``ready``, while + two callers legitimately arrive there: ``sweep_orphans`` re-enqueues an + idempotent orphan whose previous attempt may have finished, and somebody + asking a set-up connection to check its own weights. Both are the same + call doing the same idempotent work, so the table says so and the + exception disappears — a parameter that relaxes a rule is worse than a + rule that was drawn correctly. Raises: InferenceConnectionNotFound: no such connection in this workspace. - InferenceConnectionNotDownloadable: it is already set up, or it is a - kind with no weights of its own. + InferenceConnectionNotDownloadable: it is a kind with no weights of + its own. """ with self._workspace.unit_of_work() as uow: connection = self.require_connection(uow, connection_id) @@ -217,9 +214,6 @@ def require_downloadable( connection.setup_state, connection_type=connection.connection_type ): return connection - kinds = CONNECTION_KINDS[ConnectionAction.DOWNLOAD_WEIGHTS] - if retrying and connection.connection_type in kinds: - return connection raise InferenceConnectionNotDownloadable(_why_not_downloadable(connection)) def record_weights_ready(self, connection_id: UUID) -> InferenceConnection: @@ -379,21 +373,16 @@ def _first_reason(exc: ValidationError) -> str: def _why_not_downloadable(connection: InferenceConnection) -> str: - """Which of the two refusals this is, in a sentence somebody can act on. + """The one remaining refusal, in a sentence somebody can act on. - The *message* distinguishes them and the code does not, deliberately: a - client branches on ``INFERENCE_CONNECTION_NOT_DOWNLOADABLE`` to decide - whether to stop asking, and both readings say stop. Splitting the code would - publish a distinction no caller behaves differently on. + There were two until ``download_weights`` became legal at ``ready`` (#469), + where it verifies rather than refuses. What is left is a fact about the kind + and never about where a connection has got to, which is why the sentence + names the kind. """ - if connection.connection_type is not ConnectionType.LOCAL: - return ( - f"connection {connection.name!r} is an {connection.connection_type.value} connection; " - "its model runs elsewhere, so there are no weights here to fetch" - ) return ( - f"connection {connection.name!r} is already set up; " - "its weights are present and there is nothing to fetch" + f"connection {connection.name!r} is an {connection.connection_type.value} connection; " + "its model runs elsewhere, so there are no weights here to fetch" ) diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index 9f846857..39828a37 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -90,6 +90,7 @@ Partition, PolygonGeometry, PolylineGeometry, + Precision, Project, ProjectStats, Release, @@ -1624,8 +1625,14 @@ class ConnectionOut(BaseModel): connection_type: ConnectionType model_id: str model_revision: str + #: Two closed vocabularies published two ways, because one of them has a + #: member that is not a fixed word: ``cuda:N`` addresses the second GPU on a + #: machine that has one, so ``device`` travels as a string the kernel refuses + #: when it is outside ``DEVICE_PATTERN``, while ``precision`` is an enum a + #: client can enumerate. Which precisions a device honours is the kernel's + #: cross-field rule, not a shape either type can carry. device: str | None - precision: str | None + precision: Precision | None endpoint_url: str | None setup_state: ConnectionSetupState allowed_actions: list[ConnectionAction] @@ -1670,7 +1677,7 @@ class ConnectionCreate(BaseModel): model_id: str model_revision: str device: str | None = None - precision: str | None = None + precision: Precision | None = None endpoint_url: str | None = None @@ -1688,7 +1695,7 @@ class ConnectionUpdate(BaseModel): model_id: str | None = None model_revision: str | None = None device: str | None = None - precision: str | None = None + precision: Precision | None = None endpoint_url: str | None = None diff --git a/tests/cli/test_inference_commands.py b/tests/cli/test_inference_commands.py index 0feffa0b..ba4d6d96 100644 --- a/tests/cli/test_inference_commands.py +++ b/tests/cli/test_inference_commands.py @@ -38,7 +38,7 @@ "--device", "cpu", "--precision", - "fp16", + "fp32", ) HTTP = ( @@ -303,18 +303,23 @@ def test_download_prints_the_connection_as_json(root: Path, fetched: list[str]) ok(root, *LOCAL) document = payload(root, "inference", "download", "local-gd") assert document["setup_state"] == "ready" - assert document["allowed_actions"] == ["update", "delete"] + assert document["allowed_actions"] == ["download_weights", "update", "delete"] -def test_downloading_twice_exits_one_with_a_sentence(root: Path, fetched: list[str]) -> None: - """The second one has nothing to do and says so, rather than fetching again.""" +def test_downloading_twice_verifies_rather_than_refusing(root: Path, fetched: list[str]) -> None: + """The second run checks the cache it already filled, and says so (#469). + + The command that fetches is the command that checks, because the work is the + same work: a snapshot already on disk is found rather than transferred + again, and only what is missing moves. + """ ok(root, *LOCAL) ok(root, "inference", "download", "local-gd") result = run(root, "inference", "download", "local-gd") - assert result.exit_code == 1, result.output - assert "already set up" in result.stderr - assert fetched == ["some/model@abc123"] + assert result.exit_code == 0, result.output + assert "is ready" in result.stderr + assert fetched == ["some/model@abc123", "some/model@abc123"] def test_downloading_an_http_connection_exits_one(root: Path, fetched: list[str]) -> None: diff --git a/tests/inference/test_weights.py b/tests/inference/test_weights.py index 11b7eaa2..b4b074bb 100644 --- a/tests/inference/test_weights.py +++ b/tests/inference/test_weights.py @@ -189,15 +189,25 @@ def test_reporting_is_optional( # --- the gate ----------------------------------------------------------------- -def test_a_connection_that_is_already_set_up_is_refused( +def test_a_connection_that_is_already_set_up_is_verified_rather_than_refused( connections: InferenceConnectionService, workspace: WorkspaceService, fetched: list ) -> None: + """The second run is the repair action, not a mistake to catch (#469). + + `download_weights` is legal at `ready`, so this reaches the download again + — and the download against a full cache is a hash check rather than a + transfer, which is what makes running it the way to answer "are the weights + still there?" on a machine where a disk filled or a cache was pruned. + """ made = a_local(connections) - fetch_weights(workspace, made.id) + ready = fetch_weights(workspace, made.id) - with pytest.raises(InferenceConnectionNotDownloadable, match="already set up"): - fetch_weights(workspace, made.id) - assert len(fetched) == 1 + verified = fetch_weights(workspace, made.id) + assert verified.setup_state is ConnectionSetupState.READY + # Nothing moved: the record is a no-op on a row that is already ready, so a + # verification does not age the connection it verified. + assert verified.updated_at == ready.updated_at + assert len(fetched) == 2 def test_an_http_connection_is_refused_before_anything_is_fetched( diff --git a/tests/jobs/test_weights_job.py b/tests/jobs/test_weights_job.py index 128c1f13..189d4954 100644 --- a/tests/jobs/test_weights_job.py +++ b/tests/jobs/test_weights_job.py @@ -162,10 +162,10 @@ def test_a_second_run_verifies_and_settles_rather_than_failing( cannot protect it — a retry does not go through a route — and refusing here would fail a job whose work is done. - So it verifies: the download is entered again (a snapshot download checks a - cache it already filled against its hashes rather than re-fetching it) and - the write below is a no-op. The result is the same result, which is what the - caller polling the row needs it to be. + So it re-checks: the download is entered again (a snapshot download finds + what the cache already holds and fetches only what is missing) and the write + below is a no-op. The result is the same result, which is what the caller + polling the row needs it to be. """ calls: list[str] = [] monkeypatch.setattr( @@ -184,11 +184,13 @@ def test_a_second_run_verifies_and_settles_rather_than_failing( def test_a_retry_is_still_refused_for_a_kind_with_no_weights( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """`retrying` relaxes the *state* half of the gate and only that half. + """The kind half of the gate is the half that never relaxed. - A connection with no weights of its own has none on the second attempt - either, so the kind stays a refusal — which is what makes the flag narrower - than one that skips the gate. + #469 made the state half unconditional — a `ready` connection can be asked + to re-check its own cache — and left this exactly where it was: a connection + with no weights of its own has none on the second attempt either, so the + handler refuses rather than reaching a download that would have nothing to + do. """ from visionset.kernel.errors import InferenceConnectionNotDownloadable diff --git a/tests/kernel/test_capabilities.py b/tests/kernel/test_capabilities.py index 867afb77..4893406b 100644 --- a/tests/kernel/test_capabilities.py +++ b/tests/kernel/test_capabilities.py @@ -806,7 +806,7 @@ def _connection_in( model_id="some/model", model_revision="abc123", device="cpu", - precision="fp16", + precision="fp32", ) if setup_state is ConnectionSetupState.READY: made = connections.record_weights_ready(made.id) @@ -914,18 +914,19 @@ def test_a_connections_declaration_is_read_from_the_kernels_own_gate() -> None: the same edit. Spelling the answers out here instead would be the hand-mirror this module exists to remove. - `download_weights` is the one entry that names its own members, because - "the state weights are missing in" and "the kind that has weights" are not - sets the domain had already. So the check on it is the *rule*: local and - not-set-up, and nothing else — which is what turns red if either table stops - gating it. + Every gate is unconditional in *state* since #469 — `download_weights` at + `ready` is the verification of a cache the download already filled — so all + three name the set itself. The one conditional entry left is the kind half, + and the check on it is the rule: `download_weights` is local and nothing + else, which is what turns red if that table is widened or dropped. """ + for action in ConnectionAction: + assert CONNECTION_GATES[action] is EVERY_SETUP_STATE + unconditional = set(ConnectionAction) - {ConnectionAction.DOWNLOAD_WEIGHTS} for action in unconditional: - assert CONNECTION_GATES[action] is EVERY_SETUP_STATE assert CONNECTION_KINDS[action] is EVERY_CONNECTION_TYPE - assert CONNECTION_GATES[ConnectionAction.DOWNLOAD_WEIGHTS] == {ConnectionSetupState.NOT_SET_UP} assert CONNECTION_KINDS[ConnectionAction.DOWNLOAD_WEIGHTS] == {ConnectionType.LOCAL} for kind in ConnectionType: @@ -936,13 +937,16 @@ def test_a_connections_declaration_is_read_from_the_kernels_own_gate() -> None: ) -def test_download_weights_is_declared_on_exactly_one_square() -> None: - """Local and not set up, and nowhere else. +def test_download_weights_is_declared_on_both_local_squares_and_no_others() -> None: + """Local in either state, and no `http` square in any state. - The mutation gate for the download capability, stated as the one sentence a - reader can check: this fails if either table is widened, if either is - dropped from `connection_actions`, or if the action is quietly gated on - something else instead. + The pinned action set for the download capability, stated as the one + sentence a reader can check, and **deliberately moved** by #469: it used to + read `{(local, not_set_up)}` and now reads both local squares, because a + `ready` connection can be asked to verify the cache it already has. This + file is where that becomes a decision rather than a drift — widening the + kind table, or gating the action on something other than these two, still + turns it red. """ declared = { (kind, state) @@ -950,7 +954,10 @@ def test_download_weights_is_declared_on_exactly_one_square() -> None: for state in ConnectionSetupState if ConnectionAction.DOWNLOAD_WEIGHTS in connection_actions(state, connection_type=kind) } - assert declared == {(ConnectionType.LOCAL, ConnectionSetupState.NOT_SET_UP)} + assert declared == { + (ConnectionType.LOCAL, ConnectionSetupState.NOT_SET_UP), + (ConnectionType.LOCAL, ConnectionSetupState.READY), + } def test_the_actions_a_connection_cannot_yet_be_asked_for_are_not_declared() -> None: diff --git a/tests/kernel/test_inference_connections.py b/tests/kernel/test_inference_connections.py index d7bcf901..13ed3e0a 100644 --- a/tests/kernel/test_inference_connections.py +++ b/tests/kernel/test_inference_connections.py @@ -26,9 +26,13 @@ from visionset.kernel.adapters import _mappers as m from visionset.kernel.adapters import _tables as t from visionset.kernel.domain import ( + CPU, + CUDA, ConnectionSetupState, ConnectionType, InferenceConnection, + Precision, + precisions_for, ) from visionset.kernel.services import InferenceConnectionService, WorkspaceService @@ -37,7 +41,7 @@ model_id="some/model", model_revision="abc123", device="cpu", - precision="fp16", + precision="fp32", ) HTTP = dict( @@ -124,6 +128,119 @@ def test_the_service_refuses_in_the_kernels_own_vocabulary(connections) -> None: assert str(refusal.value) == "a local connection needs device" +# --- the two closed vocabularies (#469) --------------------------------------- + + +@pytest.mark.parametrize( + ("written", "stored"), + [("cpu", "cpu"), ("cuda", "cuda"), ("cuda:1", "cuda:1"), (" CUDA ", "cuda")], +) +def test_a_device_this_build_can_address_is_kept_and_normalized(written: str, stored: str) -> None: + """Case and surrounding space are forgiven; `cuda:N` is a device, not a typo.""" + made = InferenceConnection(name="x", **(dict(LOCAL) | {"device": written, "precision": "fp32"})) + assert made.device == stored + + +@pytest.mark.parametrize("written", ["gpu", "mps", "cuda:", "cuda:x", "cuda 1", "", "cpu0"]) +def test_a_device_nothing_here_could_address_is_refused(written: str) -> None: + """The gap this closes: every one of these was accepted and then ignored. + + The adapters resolve anything that is not CUDA onto the CPU in full + precision, so a connection saying `gpu` used to describe a run that never + happened and went on displaying `gpu` while it did not happen. + """ + with pytest.raises(ValidationError, match="not a device this build can run on"): + InferenceConnection(name="x", **(dict(LOCAL) | {"device": written})) + + +@pytest.mark.parametrize( + ("written", "stored"), + [ + ("fp16", Precision.FP16), + ("FP16", Precision.FP16), + (" float16 ", Precision.FP16), + ("half", Precision.FP16), + ("fp32", Precision.FP32), + ("float32", Precision.FP32), + ("full", Precision.FP32), + ], +) +def test_the_spellings_this_build_has_honoured_normalize_onto_the_vocabulary( + written: str, stored: Precision +) -> None: + """A row written under the free-text field stays readable. + + `_fp16.HALF_PRECISION_NAMES` has accepted `float16` and `half` for as long as + the field has existed, so those rows were written by somebody following the + product. A vocabulary that closed around them by refusing them would refuse + them on the way *out* of the store, which is a workspace that will not list. + """ + made = InferenceConnection(name="x", **(dict(LOCAL) | {"device": "cuda", "precision": written})) + assert made.precision is stored + + +@pytest.mark.parametrize("written", ["fp8", "bf16", "float64", "int8", "auto", ""]) +def test_a_precision_outside_the_vocabulary_is_refused(written: str) -> None: + with pytest.raises(ValidationError): + InferenceConnection(name="x", **(dict(LOCAL) | {"precision": written})) + + +def test_half_precision_is_refused_on_a_cpu_and_offered_on_a_gpu() -> None: + """The cross-field rule, which is the one neither vocabulary can state alone. + + Both local adapters resolve half precision as *this device is CUDA and the + connection asked for fp16*, so `cpu` + `fp16` is not a slow run: it is a + setting with no effect that the row would go on displaying as though it had + one. + """ + with pytest.raises(ValidationError, match="fp16 is not available on cpu"): + InferenceConnection(name="x", **(dict(LOCAL) | {"device": "cpu", "precision": "fp16"})) + + for device in ("cuda", "cuda:1"): + made = InferenceConnection( + name="x", **(dict(LOCAL) | {"device": device, "precision": "fp16"}) + ) + assert made.precision is Precision.FP16 + + +def test_the_conditioning_rule_has_one_owner() -> None: + """The validator above and every form that offers a choice read this function. + + Two copies of "fp16 needs CUDA" is how a form comes to offer what the kernel + refuses, which is the shape `ui-capabilities` bans one layer up. + """ + assert precisions_for(CPU) == (Precision.FP32,) + assert precisions_for(CUDA) == (Precision.FP16, Precision.FP32) + assert precisions_for("cuda:3") == (Precision.FP16, Precision.FP32) + + +def test_the_service_refuses_a_bad_vocabulary_in_the_kernels_own_words(connections) -> None: # noqa: ANN001 + """Not a `ValidationError`, and not a 500 on a request whose fault is a typo.""" + with pytest.raises(InferenceConnectionInvalid) as refusal: + connections.create("x", **(dict(LOCAL) | {"device": "gpu"})) + assert not isinstance(refusal.value, ValidationError) + assert "not a device this build can run on" in str(refusal.value) + + made = connections.create("y", **LOCAL) + with pytest.raises(InferenceConnectionInvalid, match="fp16 is not available on cpu"): + connections.update(made.id, precision="fp16") + + +def test_editing_a_device_alone_can_leave_a_pair_the_kernel_refuses(connections) -> None: # noqa: ANN001 + """The half of the rule an edit could otherwise walk around. + + `update` rebuilds and revalidates rather than patching a column, so moving a + `cuda` + `fp16` connection onto the CPU is refused as the pair it would + produce — a partial edit cannot leave a row the create path would not accept. + """ + made = connections.create("z", **(dict(LOCAL) | {"device": "cuda", "precision": "fp16"})) + with pytest.raises(InferenceConnectionInvalid, match="fp16 is not available on cpu"): + connections.update(made.id, device="cpu") + + moved = connections.update(made.id, device="cpu", precision="fp32") + assert (moved.device, moved.precision) == ("cpu", Precision.FP32) + + # --- workspace scoping -------------------------------------------------------- diff --git a/tests/kernel/test_metadata_store.py b/tests/kernel/test_metadata_store.py index eb85caa8..5e33af1f 100644 --- a/tests/kernel/test_metadata_store.py +++ b/tests/kernel/test_metadata_store.py @@ -194,7 +194,7 @@ def _seed(uow: UnitOfWork) -> list[tuple[str, UUID]]: model_id="some/model", model_revision="abc123", device="cpu", - precision="fp16", + precision="fp32", created_at=datetime(2026, 8, 7, 12, 0, tzinfo=UTC), updated_at=datetime(2026, 8, 7, 12, 0, tzinfo=UTC), ) diff --git a/tests/server/test_inference.py b/tests/server/test_inference.py index 17a1b1ff..589971d2 100644 --- a/tests/server/test_inference.py +++ b/tests/server/test_inference.py @@ -27,7 +27,7 @@ "model_id": "some/model", "model_revision": "abc123", "device": "cpu", - "precision": "fp16", + "precision": "fp32", } HTTP: dict[str, Any] = { @@ -219,6 +219,45 @@ def test_an_edit_into_a_shape_the_kind_refuses_is_a_422(client: TestClient) -> N assert response.status_code == 422, response.text +def test_a_device_this_build_cannot_address_is_refused_with_the_reason( + client: TestClient, +) -> None: + """The vocabulary reaches the wire as a sentence, not as a silent fallback (#469). + + What the form does with the two fields is the form's business; that a caller + who bypasses it is *told* is the kernel's, and this is where a client can see + it. The message is what a control renders, so it names the members rather + than saying the value was rejected. + """ + response = client.post("/inference/connections", json=LOCAL | {"device": "gpu"}) + assert response.status_code == 422, response.text + body = response.json() + assert body["code"] == "INFERENCE_CONNECTION_INVALID" + assert "not a device this build can run on" in body["message"] + assert "cuda:N" in body["message"] + + +def test_half_precision_on_a_cpu_is_refused_with_the_reason(client: TestClient) -> None: + """The cross-field rule at the wire, which is the one a form cannot own alone.""" + response = client.post("/inference/connections", json=LOCAL | {"precision": "fp16"}) + assert response.status_code == 422, response.text + assert "fp16 is not available on cpu" in response.json()["message"] + + +def test_a_precision_outside_the_vocabulary_never_reaches_the_kernel( + client: TestClient, +) -> None: + """An enum on the wire, so the contract states the members rather than implying them. + + `device` cannot be one — `cuda:N` is a member that is not a fixed word — which + is why the two closed vocabularies are published two different ways and why + the kernel, not the schema, is what both refusals have in common. + """ + response = client.post("/inference/connections", json=LOCAL | {"precision": "bf16"}) + assert response.status_code == 422, response.text + assert "fp16" in response.text and "fp32" in response.text + + # --- deleting ----------------------------------------------------------------- @@ -293,8 +332,10 @@ def test_a_finished_download_leaves_the_connection_ready( after = client.get(f"/inference/connections/{made['id']}").json() assert after["setup_state"] == "ready" - # And the declaration follows the state: there is nothing left to fetch. - assert after["allowed_actions"] == ["update", "delete"] + # The declaration survives the flip: what the action means changes from + # "fetch these" to "check these are still here" (#469), and the name of + # a capability does not change with the state it is read in. + assert after["allowed_actions"] == ["download_weights", "update", "delete"] def test_a_failed_download_leaves_the_connection_not_set_up( @@ -329,10 +370,12 @@ def test_running_the_download_twice_is_a_verified_no_op( ) -> None: """The idempotency the registry claims for this handler, exercised. - The route refuses a second *request* — the connection is ready, so - `download_weights` is no longer declared — but a re-queued orphan does not go - through the route. This drives the handler directly, which is the path a - crash recovery actually takes. + Both the re-queued orphan and the person pressing the action a second time + take this path since #469: the route accepts, the handler runs, the download + verifies a cache it already filled, and the row ends where it started. What + the test holds is that *nothing moved* — a second run that reported a state + change would mean the write is not the no-op the handler's registration + promises. """ from visionset.jobs.weights import run as download_run @@ -345,13 +388,19 @@ def test_running_the_download_twice_is_a_verified_no_op( with api_client(tmp_path / "ws", dispatcher=InlineDispatcher()) as client: made = created(client, LOCAL) client.post(f"/inference/connections/{made['id']}/download") - assert client.get(f"/inference/connections/{made['id']}").json()["setup_state"] == "ready" + ready = client.get(f"/inference/connections/{made['id']}").json() + assert ready["setup_state"] == "ready" response = client.post(f"/inference/connections/{made['id']}/download") - assert response.status_code == 409, response.text - assert response.json()["code"] == "INFERENCE_CONNECTION_NOT_DOWNLOADABLE" + assert response.status_code == 202, response.text + job = client.get(f"/background-jobs/{response.json()['id']}").json() + assert job["state"] == BackgroundJobState.SUCCEEDED.value, job + + again = client.get(f"/inference/connections/{made['id']}").json() + assert again["setup_state"] == "ready" + assert again["updated_at"] == ready["updated_at"] - assert calls == ["some/model"] + assert calls == ["some/model", "some/model"] assert callable(download_run)