diff --git a/docker/compose.yaml b/docker/compose.yaml index 3d4c50be..cd832bd1 100644 --- a/docker/compose.yaml +++ b/docker/compose.yaml @@ -185,6 +185,20 @@ services: # port open to every interface would hand the workspace to the LAN. The two # lines belong together and neither is safe alone. VISIONSET_UI_SESSION: always + # Forwarded from whoever ran `docker compose`, empty when they have none. + # Some published weights are served only to accounts that have been granted + # them, and the refusal for fetching one without that names `HF_TOKEN` as + # the remedy. Without this line the remedy is unreachable from this stack: + # the reader sets the variable on the host, the container never sees it, and + # the same refusal comes back with nothing about it changed. A refusal + # naming a remedy nobody in that position can take is the mistake + # `NotAWorkspace` is the standing example of. + # + # It carries no default and belongs to no mode: a stack without the + # inference runtime never fetches anything, and one with it fetches ungated + # weights exactly as before, because the hub client treats an empty value + # as no token at all. + HF_TOKEN: ${HF_TOKEN:-} # Raw uvicorn rather than `visionset server`, and deliberately so: that # command also serves the compiled bundle at /app, which in development is # vite's job on :5173 — and building that bundle is exactly what this stack diff --git a/docs/architecture/backend/inference.md b/docs/architecture/backend/inference.md index 139a0a07..ff7d8839 100644 --- a/docs/architecture/backend/inference.md +++ b/docs/architecture/backend/inference.md @@ -148,9 +148,21 @@ Third-party inference and labeling-ecosystem frameworks are not taken as runtime dependencies; external work in that space is design reference only. A candidate that exists solely inside one is excluded on that ground alone. -**Copyleft, business-source and custom-gated licenses are never bundled and never -curated as defaults.** At most they are reachable through the **Custom model...** -path a user types themselves, at that user's own compliance risk. +**Copyleft and business-source licenses are never bundled and never curated.** At +most they are reachable through the **Custom model...** path a user types +themselves, at that user's own compliance risk. + +**A gated model may be curated, and may never be the default** (2026-08-15, +superseding the narrower rule this replaces, which excluded gated licenses +alongside copyleft ones). Nothing is bundled either way: weights are fetched by +the user, from the publisher, after that user has accepted the publisher's terms +themselves, which is the same act whether the id came from a curated list or was +typed in. What curation adds is that the requirement is *stated* - the entry +carries the sentence and the page where access is requested, and the form shows +both before a download is offered. Leaving such a model out does not spare anybody +its terms; it means the people who want it find the id elsewhere and type it in, +having read nothing. The default entry must stay one that anybody can fetch, so +that opening the form is never itself a refusal. **`transformers`-native is strongly preferred**, because it is one integration surface. A candidate published only as a raw `.pt` or `.onnx` checkpoint needs its @@ -159,8 +171,10 @@ entry however small the model is. Curated entries live in [`inferenceCatalog.ts`](../../../frontend/ui-core/src/screens/inferenceCatalog.ts), -which records a model id, a pinned revision and a download size per entry and -asserts the license once, in the module's prose, for the whole list. Pinning the +which records a model id, a pinned revision and a download size per entry. The +module's prose asserts the license for the list and names its one exception by +hand; an entry that carries a requirement of its own carries it as data, in +`access`, because that is the half the interface has to render. Pinning the license per entry the way the revision is pinned is what would make a family-level assumption impossible to write by accident. @@ -169,6 +183,7 @@ assumption impossible to write by accident. | Family (`model_type`) | Capability | Curated checkpoints | License | | --- | --- | --- | --- | | `sam2`, `sam2_video` | `point_suggest` | `facebook/sam2.1-hiera-{tiny,small,base-plus,large}` | Apache-2.0 | +| `sam3_video` | `point_suggest` | `facebook/sam3` | SAM License, access granted by request | | `grounding-dino` | `text_detect` | `IDEA-Research/grounding-dino-{tiny,base}` | Apache-2.0 | | `mm-grounding-dino` | `text_detect` | none | - | @@ -254,15 +269,54 @@ question and not a family entry. ### The gate a gated model hits -`facebook/sam3` is a capability step-change - concept segmentation from text or -exemplar prompts - and it is **gated**: the hub reports `gated: manual` and a -`license: other`, and its `config.json` cannot be read without accepting terms. +**Both blockers recorded here are now cleared, and `facebook/sam3` ships as a +curated `point_suggest` entry.** What follows describes how, because the shape is +what any future gated candidate inherits. + +The hub reports `gated: manual` and a `license: other`, and the repository's +`config.json` cannot be read without being granted access. The license question is +answered by the gated-model rule above: nothing is bundled, the user fetches from +the publisher after accepting the publisher's terms, and the requirement is stated +in the form before a download is offered. + +The fetch question turned out to need no mechanism at all. The download job reaches +the hub through its standard client, which resolves a token from the environment +on its own, so an authenticated fetch is `HF_TOKEN` being set where the server or +the CLI runs and nothing in a workspace. What was missing was not the capability +but the *sentence*: an unauthenticated fetch raised an error whose text opened with +a status line and a request id, and re-raising it carried both to a reader. That is +now translated into the remedy. + +Two measurements worth keeping, because both contradict the assumption a reader +would otherwise make. **Reading a size needs no access** - the hub answers +`model_info` for a gated repository unauthenticated - which is what lets the size +line work before anybody has asked for anything. And **a gated repository answers +401 rather than 403**, the same status a repository that does not exist answers, so +the two are told apart by the client's exception class and never by the code. A +branch on the number would send somebody with a typo to request access to a model +nobody publishes. + +Point prompts run through the tracker half of the architecture, which agrees with +SAM 2 signature for signature; the concept segmentation the model is named for is +not integrated, and the entry declares `point_suggest` alone. + +**Which name a connection resolves on is the trap here, and it cost a revision.** +The repository publishes one artifact carrying the whole architecture, and the +family is read from the top of its config: -Two separate blockers, and only the second is this build's. The license is not one -the third rule above permits bundling or curating. And the download job has no -notion of an authenticated fetch - no hub token, no terms acceptance - so a gated -revision is unreachable regardless of its license. That is the gate any future -gated candidate would hit, recorded here rather than as current debt. +``` +Sam3VideoConfig model_type = sam3_video <- what resolves +|-- detector_config model_type = sam3 <- concepts and words ++-- tracker_config model_type = sam3_tracker_video <- the promptable half +``` + +Only `sam3_video` belongs in `SEGMENTER_FAMILIES`. `sam3` is the concept detector, +and admitting it would route a text model to the point adapter. A register built +by reasoning from the `transformers` class names produces `sam3` and +`sam3_tracker`, neither of which any published checkpoint declares, and the +capability is then unreachable while every test agrees with the guess that built +it. The rule this restates: a family entry is a string read out of a config, and +the test that holds it names the configs it was read from. ## Related diff --git a/docs/inference.md b/docs/inference.md index e5a78998..42329648 100644 --- a/docs/inference.md +++ b/docs/inference.md @@ -92,6 +92,11 @@ to the plain file - keeps projects, connections and downloaded weights; only the and `--build` is not optional on any of those switches. That stack is for development and changes nothing about the wheel above. +`HF_TOKEN` is forwarded from whoever runs `docker compose`, so fetching weights that have to be +asked for (below) works the same way inside the stack as outside it - export it in the shell you +bring the stack up in. It is empty when you have none, which is what every ungated model already +expects. + ## Knowing what a download costs, before agreeing to it A decision you cannot see the price of is not a decision, so the size is readable on its own, @@ -115,6 +120,10 @@ publishing two serialisations of the same tensors really does cost both, and a f of them would understate what lands on your disk. A revision the listing cannot fully size is refused rather than estimated. +`facebook/sam3` is the live example of that and the largest entry in the curated list: it publishes +its weights as both a checkpoint and safetensors, so about 6.9 GB arrives to install a model of +about 3.4 GB. The number shown is the one that describes your disk. + It needs the `local-inference` extra, because the size is read with the same client that would do the fetching. Without it you get `LOCAL_INFERENCE_UNAVAILABLE` and the install command. @@ -151,6 +160,26 @@ error on the job. There is no half-ready state to recover from because there is 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. +**Some models have to be asked for first.** A publisher can put its weights behind an access gate, +where the files are served only to accounts that have been granted them. `facebook/sam3` is one: +its weights are under Meta's SAM License and access is granted by request. Two steps, once per +machine rather than once per connection - ask for access on the model's own page, then put a token +from that account in the environment the server or the CLI runs in: + +```bash +export HF_TOKEN=hf_… +``` + +The token is read by the hub client itself, so there is nothing to configure in a workspace and +nothing stored in one. A download attempted without it fails with a sentence naming both halves of +the remedy rather than an HTTP status. Reading a model's **size** needs no token and no access: +that is what lets the connection form tell you what a download costs before you decide whether to +go and ask for it. + +The gate is on fetching, never on running. Once the files are in a workspace's `models/`, the model +loads and answers clicks with no token present at all - which is what makes a workspace you copy to +another machine still work there. + **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 **Check for missing files** and puts it in the row's overflow @@ -262,7 +291,7 @@ about to send? So a connection also declares what it can be asked for. | Capability | Means | Families | | --- | --- | --- | -| `point_suggest` | Give me the thing under these points | the SAM 2 family | +| `point_suggest` | Give me the thing under these points | the SAM 2 and SAM 3 families | | `text_detect` | Find everything these words name | the grounding-dino family | **Read from the model, never from its name.** The value comes from the `model_type` the @@ -492,15 +521,18 @@ steps, because the two kinds share almost no fields: first where the model runs, form. - **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 + grouped list - the SAM 2.1 ladder and SAM 3 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 - so picking `mps` leaves `fp32` as the only precision offered. 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 + deciding. An entry whose weights have to be asked for says so on the line above that one, with a + link to the page where access is requested, so the requirement is read while the model is being + chosen rather than met as a refused download later. + 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 diff --git a/frontend/app/e2e/inference.spec.ts b/frontend/app/e2e/inference.spec.ts index aa29f66a..99c4d3ed 100644 --- a/frontend/app/e2e/inference.spec.ts +++ b/frontend/app/e2e/inference.spec.ts @@ -365,3 +365,78 @@ test("a transfer and a re-read are two records on one row", async ({ page }) => await expect(page.getByTestId("integrity-progress-prose")).toHaveText("3 of 9 files · 33%"); await expect(page.getByTestId("download-progress")).toHaveCount(0); }); + +test("a model whose weights have to be asked for says so before it can be downloaded", async ({ + page, +}) => { + // Here rather than only in `inference.test.tsx` because the claim is about what + // a person meets on the way to a download: the requirement has to be legible in + // the real dialog, above the real size line, before the control that would + // fetch anything exists. jsdom proves the conditional; this proves the journey. + await serveApi(page, () => connection("ready", null)); + await page.route("**/api/inference/download-size*", (route) => + route.fulfill({ + status: 200, + json: { + model_id: "facebook/sam3", + model_revision: "3c879f39826c281e95690f02c7821c4de09afae7", + total_bytes: 6_895_093_624, + file_count: 12, + }, + }), + ); + await openInference(page); + + await page.getByTestId("new-connection").click(); + await page.getByTestId("choose-local").click(); + // The form opens on a model anybody can fetch, so there is nothing to say yet. + await expect(page.getByTestId("model-access")).toHaveCount(0); + + await page.getByTestId("connection-model").click(); + await page.getByRole("option", { name: /facebook\/sam3/ }).click(); + + const access = page.getByTestId("model-access"); + await expect(access).toBeVisible(); + await expect(access).toContainText("SAM License"); + await expect(access).toContainText("HF_TOKEN"); + await expect(access.getByRole("link", { name: "Request access" })).toHaveAttribute( + "href", + "https://huggingface.co/facebook/sam3", + ); +}); + +test("a model list taller than the window scrolls instead of running off it", async ({ page }) => { + // Layout under a real viewport, so it cannot live in `inference.test.tsx`: + // jsdom reports every height as zero and would pass against the implementation + // this replaced, which clipped the list and left the options past the bottom + // edge in the DOM, keyboard-reachable and unreachable with a pointer. + await serveApi(page, () => connection("ready", null)); + await page.route("**/api/inference/download-size*", (route) => + route.fulfill({ + status: 200, + json: { model_id: "m", model_revision: "r", total_bytes: 1_200_000_000, file_count: 3 }, + }), + ); + // Short enough that the curated list cannot fit under its trigger whatever the + // catalog holds — the condition being tested, rather than a window size that + // happens to provoke it today. + await page.setViewportSize({ width: 1280, height: 600 }); + await openInference(page); + await page.getByTestId("new-connection").click(); + await page.getByTestId("choose-local").click(); + await page.getByTestId("connection-model").click(); + + const listbox = page.getByRole("listbox"); + await expect(listbox).toBeVisible(); + const box = (await listbox.boundingBox())!; + expect(box.y + box.height).toBeLessThanOrEqual(600 + 1); + + // The last entry is the one a clipped list loses, and it is reachable: Radix + // scrolls it into view, and clicking it selects it rather than hitting a + // control drawn past the bottom of the window. + const last = page.getByRole("option", { name: /Custom model/ }); + await last.scrollIntoViewIfNeeded(); + await expect(last).toBeVisible(); + await last.click(); + await expect(page.getByTestId("connection-custom-model")).toBeVisible(); +}); diff --git a/frontend/ui-core/src/annotator/SuggestPanel.tsx b/frontend/ui-core/src/annotator/SuggestPanel.tsx index a934c28b..56eb1e11 100644 --- a/frontend/ui-core/src/annotator/SuggestPanel.tsx +++ b/frontend/ui-core/src/annotator/SuggestPanel.tsx @@ -179,9 +179,13 @@ const BLOCKER_COPY: Readonly< // Ranked *below* `not-ready` by `usableConnection`, and the copy relies on it: // this sentence is only ever read where something is downloaded and running, // so "the model you have" is a model that is genuinely here. + // The sentence names what the model has to *do* rather than which family does + // it. More than one architecture answers a point now, so a copy naming one of + // them was a second place the supported list lived — and the one that goes + // stale silently, because no build fails when prose falls behind a register. "not-capable": { title: "That model answers a different question", - body: "Suggesting a shape needs a model that takes points — the SAM 2 family. The connections that are ready here answer something else, so a click would come back refused.", + body: "Suggesting a shape needs a model that takes points and answers with a region. The connections that are ready here answer something else, so a click would come back refused.", action: "Set up a connection", tone: "warn", }, diff --git a/frontend/ui-core/src/primitives/Select.tsx b/frontend/ui-core/src/primitives/Select.tsx index 12c3a6a6..1501704f 100644 --- a/frontend/ui-core/src/primitives/Select.tsx +++ b/frontend/ui-core/src/primitives/Select.tsx @@ -27,10 +27,34 @@ * instead of a fixed `h-9`. A single-line option still measures exactly 36px, so * every select that shipped before this is unmoved. Nothing truncates and nothing * ellipsises — an identifier cut off in the middle is not an identifier. + * + * ## The list scrolls; it never runs off the screen + * + * Two-line options make a list tall quickly, and a list taller than the room under + * its trigger used to be **clipped** rather than scrolled: the options past the + * edge stayed in the DOM and stayed reachable by keyboard, while a pointer had no + * way to get to them and nothing on screen said they were there. A list that looks + * complete and is not is worse than either a scrollbar or a shorter list. Measured + * before the fix, on a 600px-tall window, the list's bottom edge sat at 836px. + * + * So the content is bounded by `--radix-select-content-available-height` — Radix's + * own measurement of the gap between the trigger and the viewport edge, which + * tracks a window resize and a trigger near the bottom of the screen without a + * constant here guessing at either — and the viewport scrolls inside it. + * + * The scroll buttons are the affordance, not decoration. macOS hides overlay + * scrollbars until a scroll is already under way, so on a stock Mac a truncated + * list and a whole one look the same until somebody gambles on a gesture; Radix + * mounts the buttons only while there is somewhere to scroll to, which is exactly + * when that ambiguity exists. + * + * It is a property of layout under a real viewport, so `inference.spec.ts` asserts + * it in chromium. jsdom reports every height as zero and would agree with any + * implementation, including the broken one this replaced. */ import * as SelectPrimitive from "@radix-ui/react-select"; -import { Check, ChevronDown } from "lucide-react"; +import { Check, ChevronDown, ChevronUp } from "lucide-react"; import { forwardRef, type ComponentPropsWithoutRef, @@ -84,13 +108,28 @@ export const SelectContent = forwardRef< ref={ref} position={position} className={cn( - "z-50 min-w-32 overflow-hidden rounded-lg border border-border bg-popover " + + "z-50 flex max-h-(--radix-select-content-available-height) min-w-32 flex-col " + + "overflow-hidden rounded-lg border border-border bg-popover " + "text-popover-foreground shadow-lg", className, )} {...props} > - {children} + {/* + `overflow-hidden` above is what rounds the corners, and on its own it + silently clipped anything past the bottom edge. The bound and the + scrolling viewport are what turn that into a scroll; the module note + above carries the argument and the measurement. + */} + + + + {children} + + + ); diff --git a/frontend/ui-core/src/screens/InferenceScreen.tsx b/frontend/ui-core/src/screens/InferenceScreen.tsx index 195ebbf3..071bff37 100644 --- a/frontend/ui-core/src/screens/InferenceScreen.tsx +++ b/frontend/ui-core/src/screens/InferenceScreen.tsx @@ -976,6 +976,7 @@ function ConnectionDialog({ + ) : ( @@ -1037,6 +1038,46 @@ function ConnectionDialog({ ); } +/** + * What has to be cleared before this model can be fetched, said before it is. + * + * Principle 9: no requirement is discovered by crashing into it. A gated model + * refuses its download with a sentence naming the remedy, which is the right + * refusal and still the wrong place to learn it — by then somebody has chosen a + * model, created a connection and pressed a button. This is the same fact, one + * step earlier, while the choice is still being made. + * + * **Looked up by model id alone, and deliberately not through + * {@link curatedEntry}.** That helper answers "is this row showing exactly this + * curated entry", which compares the revision too — the right question for the + * select, and the wrong one here. An access gate belongs to the *repository*: + * pinning some other commit of the same model does not exempt anybody from its + * terms, so a line that disappeared when the revision was edited would be hiding + * a requirement that still applies. + * + * A model id nobody curated gets nothing here, and that is honest rather than a + * gap — whether an arbitrary repository is gated is not something this build + * knows before asking, and the refusal is what answers it. + */ +function AccessLine({ modelId }: { readonly modelId: string }): JSX.Element { + const access = CURATED_BY_ID.get(modelId)?.access; + if (access === undefined) return <>; + return ( +

+ {access.note}{" "} + + Request access + + . +

+ ); +} + /** * What this revision would cost to fetch, beside the control that confirms it. * diff --git a/frontend/ui-core/src/screens/inference.test.tsx b/frontend/ui-core/src/screens/inference.test.tsx index b073edf2..d3474109 100644 --- a/frontend/ui-core/src/screens/inference.test.tsx +++ b/frontend/ui-core/src/screens/inference.test.tsx @@ -705,6 +705,82 @@ it("stacks each option: the id on one line, what it costs on the next (#472)", a expect(custom.querySelector(".text-muted-foreground")).not.toBeNull(); }); +it("states a model's access requirement while it is being chosen, not when it is downloaded", async () => { + // Principle 9, on the one entry that has a requirement: a gated model refuses + // its own download with a sentence naming the remedy, and by the time that + // arrives somebody has chosen a model, created a connection and pressed a + // button. This is the same fact while the choice is still open. + const gated = CURATED_MODELS.flatMap((group) => group.models).find( + (model) => model.access !== undefined, + )!; + listing([]); + sizeIs(1_200_000_000); + render(mount()); + await userEvent.click(await screen.findByTestId("new-connection")); + await userEvent.click(await screen.findByTestId("choose-local")); + + // The form opens on an entry anybody can fetch, so there is nothing to say. + expect(screen.queryByTestId("model-access")).toBeNull(); + + await userEvent.click(await screen.findByTestId("connection-model")); + await userEvent.click(screen.getByRole("option", { name: new RegExp(gated.modelId) })); + + const line = await screen.findByTestId("model-access"); + expect(line.textContent).toContain(gated.access!.note); + expect(line.querySelector("a")?.getAttribute("href")).toBe(gated.access!.href); + // Nothing has been created and nothing has been fetched: the requirement is on + // screen strictly before either becomes possible. + expect(sent.some((one) => one.method === "POST")).toBe(false); +}); + +it("drops the access line again when the choice moves back to an open model", async () => { + // The line describes the current choice rather than the dialog's history. One + // left behind would tell somebody a model they are not using needs approval, + // which is the same defect as never showing it, pointed the other way. + const gated = CURATED_MODELS.flatMap((group) => group.models).find( + (model) => model.access !== undefined, + )!; + 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")); + await userEvent.click(screen.getByRole("option", { name: new RegExp(gated.modelId) })); + expect(await screen.findByTestId("model-access")).not.toBeNull(); + + await userEvent.click(await screen.findByTestId("connection-model")); + await userEvent.click(screen.getByRole("option", { name: new RegExp(DEFAULT_MODEL.modelId) })); + expect(screen.queryByTestId("model-access")).toBeNull(); +}); + +it("keeps saying a model needs access when it is pinned to another commit", async () => { + // An access gate belongs to the repository, not to the revision: choosing a + // different commit of the same model does not exempt anybody from its terms. + // The line is therefore looked up by model id alone, where the select's own + // "is this the curated entry" test compares both halves — a distinction no + // other test in this file could see, and the reason this one exists. + const gated = CURATED_MODELS.flatMap((group) => group.models).find( + (model) => model.access !== undefined, + )!; + listing([ + connection({ + name: "pinned-elsewhere", + model_id: gated.modelId, + model_revision: "0000000000000000000000000000000000000000", + allowed_actions: ["update", "delete"], + }), + ]); + sizeIs(1_200_000_000); + render(mount()); + await userEvent.click(await screen.findByTestId("actions-pinned-elsewhere")); + await userEvent.click(await screen.findByTestId("action-edit")); + + const line = await screen.findByTestId("model-access"); + expect(line.textContent).toContain(gated.access!.note); +}); + it("curates without restricting: Custom reveals the free model and revision", async () => { listing([]); sizeIs(1_200_000_000); diff --git a/frontend/ui-core/src/screens/inferenceCatalog.test.ts b/frontend/ui-core/src/screens/inferenceCatalog.test.ts index 93c5b71c..778b2ef0 100644 --- a/frontend/ui-core/src/screens/inferenceCatalog.test.ts +++ b/frontend/ui-core/src/screens/inferenceCatalog.test.ts @@ -88,3 +88,25 @@ it("offers the devices every machine can be asked about", () => { // escape, because a Mac has exactly one. expect([...DEVICES]).toEqual(["cpu", "cuda", "mps"]); }); + +it("carries an access note on the one entry that cannot simply be downloaded", () => { + // The list is mostly checkpoints anybody can fetch, and one that is not. The + // note is the whole reason that entry may be curated at all: leaving it out + // would not spare anybody the terms, it would only mean the people who want it + // type the id in from somewhere else having read nothing. + const gated = EVERY_MODEL.filter((model) => model.access !== undefined); + expect(gated.map((model) => model.modelId)).toEqual(["facebook/sam3"]); + for (const model of gated) { + expect(model.access!.note.trim()).not.toBe(""); + // The note says a requirement exists; the link is where it is cleared. A + // note with nowhere to go is the dead-end this pair exists to avoid. + expect(model.access!.href).toMatch(/^https:\/\/huggingface\.co\/.+/); + } +}); + +it("keeps the gated entry off the default, so nobody meets a gate they did not choose", () => { + // Opening the form pre-fills a model. Pre-filling one that refuses its own + // download until somebody has been granted access would make the first + // experience of this screen a refusal. + expect(DEFAULT_MODEL.access).toBeUndefined(); +}); diff --git a/frontend/ui-core/src/screens/inferenceCatalog.ts b/frontend/ui-core/src/screens/inferenceCatalog.ts index b177cf41..1ece91f5 100644 --- a/frontend/ui-core/src/screens/inferenceCatalog.ts +++ b/frontend/ui-core/src/screens/inferenceCatalog.ts @@ -57,8 +57,40 @@ export interface CuratedModel { 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. */ + /** + * One line saying what this entry needs, in its own terms. + * + * **No word whose referent is somewhere else.** "Newer", "different", "other", + * "another", "improved", "latest" all point at something the reader is expected + * to already have in mind, and here there is nothing for them to point at: this + * product has no released history to be newer than, and {@link CURATED_MODELS} + * is grouped by the question a model answers rather than ranked, so there is no + * position in a list for an entry to be different *from*. A reader meets one of + * those words as a comparison whose first half is missing. + * + * Hardware and speed are the axes that survive being read alone. A ladder of + * rungs from one family may compare within itself, because the rungs are on + * screen together and the ordering is real; an entry that is nobody's rung says + * only what it is. + * + * The size and the access requirement are already rendered beside this line, so + * a hint restating either spends its one line saying nothing new. + */ readonly hint: string; + /** + * What has to be cleared before this entry can be downloaded at all. + * + * Absent on an entry anybody can fetch, which is most of them. Where it is + * present the form states it **before** the download is offered, because a + * requirement discovered by pressing a button and reading a refusal is a + * requirement the interface knew about and did not say. + */ + readonly access?: { + /** One sentence: what must be accepted, and under whose terms. */ + readonly note: string; + /** Where that is done. */ + readonly href: string; + }; } /** A family of curated models, named by the question its models answer. */ @@ -70,9 +102,20 @@ export interface CuratedGroup { /** * 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. + * Every entry is published by the people who trained it, 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. + * + * **Most of them are Apache-2.0 and one is not.** `facebook/sam3` is published + * under its trainer's own licence and behind an + * access gate, and it is offered anyway because the alternative is worse: leaving + * it out does not spare anybody the terms, it only means the people who want it + * have to find the model id somewhere else and type it in, having read nothing. + * Curating it is what puts {@link CuratedModel.access} on screen before a + * download is offered. Nothing about the licence reaches this product's own: the + * adapter code is ours and stays Apache-2.0, and the weights are fetched by the + * person using it, from the publisher, after they have accepted the terms + * themselves — this list never redistributes anything. * * 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" @@ -106,6 +149,20 @@ export const CURATED_MODELS: readonly CuratedGroup[] = [ totalBytes: 1_796_013_511, hint: "large — the most accurate, wants a GPU", }, + { + modelId: "facebook/sam3", + revision: "3c879f39826c281e95690f02c7821c4de09afae7", + // The repository publishes its weights twice, once as a checkpoint and + // once as safetensors, and fetching a revision fetches all of it. So this + // is about twice the size of the model it installs, and it is the figure + // that belongs here: what lands on the disk, not what gets loaded. + totalBytes: 6_895_093_624, + hint: "wants a GPU", + access: { + note: "Meta publishes these weights under the SAM License and grants access by request. Ask for it, then set HF_TOKEN before downloading.", + href: "https://huggingface.co/facebook/sam3", + }, + }, ], }, { diff --git a/pyproject.toml b/pyproject.toml index 41dafa38..32f620fc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -85,7 +85,15 @@ dependencies = [ local-inference = [ "torch>=2.4", "torchvision>=0.19", - "transformers>=4.44", + # 5.0 is where `sam3` and `sam3_tracker` arrive, measured against the wheels + # rather than inferred from a changelog: 4.57.6 ships neither module and 5.0.0 + # ships both, exporting `Sam3TrackerProcessor` and `Sam3TrackerModel` at the + # top level, which is what `sam_provider` names. It is a major bump for + # everybody, including the SAM 2 and Grounding DINO users who gain nothing + # from it — and it makes the floor honest rather than raising it. `uv.lock` + # has resolved 5.x for as long as this extra has existed, so `>=4.44` was a + # compatibility claim no run has ever tested. + "transformers>=5.0", "accelerate>=0.33", "huggingface-hub>=0.24", ] diff --git a/src/visionset/inference/families.py b/src/visionset/inference/families.py index 77e2d5f4..2b19faa9 100644 --- a/src/visionset/inference/families.py +++ b/src/visionset/inference/families.py @@ -30,22 +30,45 @@ from visionset.inference._extra import imported from visionset.kernel.domain import InferenceConnection, ModelCapability -SEGMENTER_FAMILIES: Final[frozenset[str]] = frozenset({"sam2", "sam2_video"}) +SEGMENTER_FAMILIES: Final[frozenset[str]] = frozenset({"sam2", "sam2_video", "sam3_video"}) """``model_type`` values this build serves with the point-prompted adapter. -**Two spellings of one architecture, and the second is not a door held open for -later.** The published SAM 2 checkpoints — including the one the connection form -suggests — declare ``sam2_video``, and ``transformers`` loads such a checkpoint -into the image model deliberately, saying so as it does: *"loading a -``sam2_video`` checkpoint into ``Sam2Model``"*. Naming only ``sam2`` sends the -commonest point-prompt model in the product to the detector adapter, which then -refuses a click with a sentence about text prompts. - -Whole models only. The locked ``transformers`` also registers -``sam2_vision_model`` and ``sam2_hiera_det_model``, which are the encoder halves -a full config nests rather than checkpoints anything can prompt. A connection -naming one of those is refused by ``provider_for``, not handed to an adapter that -would look for a mask decoder and find none. +**Every entry is a string some published checkpoint actually declares, read out of +its config — never a name reasoned about from a class.** The published SAM 2 +checkpoints, including the one the connection form suggests, declare +``sam2_video``, and ``transformers`` loads such a checkpoint into the image model +deliberately, saying so as it does: *"loading a ``sam2_video`` checkpoint into +``Sam2Model``"*. Naming only ``sam2`` sends the commonest point-prompt model in +the product to the detector adapter, which then refuses a click with a sentence +about text prompts. + +``sam3_video`` is the same shape and was very nearly got wrong the same way. +``facebook/sam3`` publishes one artifact carrying the whole architecture, and its +config declares **``sam3_video``** — not ``sam3``, which is what the *detector* +half declares one level down, and not ``sam3_tracker``. The nesting is worth +knowing, because two of those three names are traps: + +.. code-block:: text + + Sam3VideoConfig model_type = sam3_video <- what a connection resolves on + |-- detector_config model_type = sam3 <- concepts and words + +-- tracker_config model_type = sam3_tracker_video <- the promptable half + +So **``sam3`` is deliberately absent**: it names the concept detector, and +admitting it here would hand a text model to the point adapter — a confident wrong +answer of exactly the kind this module's opening warns about. ``sam3_tracker`` is +absent for the opposite reason: it is a real config class, no checkpoint is known +to declare it, and a set entry nobody has measured is a guess wearing a +frozenset's clothes. Either becomes one word to add the day a checkpoint declares +it. + +Whole models only. The locked ``transformers`` registers a dozen further +``sam3_*`` types — ``sam3_vision_model``, ``sam3_detr_decoder``, +``sam3_mask_decoder`` and the rest, beside SAM 2's ``sam2_vision_model`` and +``sam2_hiera_det_model``. Those are the halves a full config nests rather than +checkpoints anything can prompt. A connection naming one of them is refused by +``provider_for``, not handed to an adapter that would look for a mask decoder and +find none. """ DETECTOR_FAMILIES: Final[frozenset[str]] = frozenset({"grounding-dino", "mm-grounding-dino"}) diff --git a/src/visionset/inference/providers.py b/src/visionset/inference/providers.py index 4cee0cb7..cf14e465 100644 --- a/src/visionset/inference/providers.py +++ b/src/visionset/inference/providers.py @@ -202,7 +202,13 @@ def _local(connection: InferenceConnection, *, workspace_root: Path) -> Runner: } family = family_of(connection, cache_dir=cache_dir) if family in SEGMENTER_FAMILIES: - return LocalSamProvider(connection.model_id, connection.model_revision, **common) + # The family travels into the segmenter because more than one architecture + # answers a point now, and they do not load through the same + # ``transformers`` classes. It is not on ``common``: the detector resolves + # its own classes through one ``Auto`` model and has nothing to vary. + return LocalSamProvider( + connection.model_id, connection.model_revision, family=family, **common + ) if family in DETECTOR_FAMILIES: return LocalTransformersProvider(connection.model_id, connection.model_revision, **common) raise InferenceConnectionNotRunnable(_no_adapter_for(connection, family)) diff --git a/src/visionset/inference/sam_provider.py b/src/visionset/inference/sam_provider.py index e5656959..8d1c052c 100644 --- a/src/visionset/inference/sam_provider.py +++ b/src/visionset/inference/sam_provider.py @@ -37,7 +37,7 @@ from __future__ import annotations -from collections.abc import Iterator +from collections.abc import Iterator, Mapping from io import BytesIO from pathlib import Path from typing import Any, Final @@ -57,6 +57,54 @@ ) from visionset.kernel.errors import UnsupportedPrompt +_CLASSES: Final[Mapping[str, tuple[str, str]]] = { + "sam2": ("AutoProcessor", "Sam2Model"), + "sam2_video": ("AutoProcessor", "Sam2Model"), + "sam3_video": ("Sam3TrackerProcessor", "Sam3TrackerModel"), +} +"""Which ``transformers`` pair loads each family this adapter serves. + +Keyed by exactly the members of :data:`~visionset.inference.families.SEGMENTER_FAMILIES` +— ``test_the_class_table_covers_every_segmenter_family`` holds the two together, +because a family added to the register without an entry here would resolve to this +adapter and then fail inside a load with a ``KeyError`` rather than in a refusal. + +**SAM 3's entry names its processor and SAM 2's does not, and the asymmetry is the +point rather than an oversight.** ``AutoProcessor`` resolves against the +repository's own declared ``processor_class``, which is the right answer for SAM 2: +``facebook/sam2.1-hiera-base-plus`` declares ``Sam2VideoProcessor``, and that is +what the shipped adapter has always loaded. Asked the same question about +``facebook/sam3`` it answers **``Sam3VideoProcessor``** — measured, not predicted — +which is the video path and not a thing a single-image click can be expressed to. +Naming ``Sam3TrackerProcessor`` is what keeps the point prompt reaching a processor +that takes points. + +The model class is named for the same reason and SAM 2's has been since this +adapter shipped: a config declaring the video variant loads into the promptable +image model deliberately. ``transformers`` says so itself when it happens, and +names the older case while doing it — *"You are using a model of type +``sam3_video`` to instantiate a model of type ``sam3_tracker``. This may be +expected if you are loading a checkpoint that shares a subset of the architecture +(e.g., loading a ``sam2_video`` checkpoint into ``Sam2Model``)"*. That warning is +left where a reader can see it, on the shipped adapter's own precedent. + +**Both rows are measured against a real load, not reasoned about.** Asking +``from_pretrained(..., output_loading_info=True)`` for ``facebook/sam3`` on CPU +reports ``missing_keys: 0``, ``unexpected_keys: 0``, ``mismatched_keys: 0`` — the +same clean result recorded below for SAM 2, so every parameter the tracker needs +came out of the checkpoint and no weight is left randomly initialised. A point +prompt then runs the whole way through: ``get_image_embeddings`` returns an +embedding, the forward answers ``pred_masks`` of ``(1, 1, 3, 288, 288)`` beside +``iou_scores`` of ``(1, 1, 3)``, and ``post_process_masks`` lifts them to +``(1, 3, 240, 320)`` — three masks at the asset's own size, which is exactly the +shape ``_segments`` below already reads. + +``Sam3TrackerModel`` and ``Sam2Model`` agree signature for signature on everything +below this line — ``get_image_embeddings(pixel_values, **kwargs)``, ``forward``'s +full keyword list, and ``post_process_masks(masks, original_sizes, …)`` — which is +why one adapter serves both and only these two names move. +""" + POSITIVE: Final = 1 NEGATIVE: Final = 0 """What this family calls a point that says *this* and one that says *not that*. @@ -122,6 +170,7 @@ def __init__( model_id: str, model_revision: str, *, + family: str, device: str, precision: str | None, cache_dir: Path, @@ -130,6 +179,7 @@ def __init__( ) -> None: self._model_id = model_id self._model_revision = model_revision + self._family = family self._device = device self._precision = precision self._cache_dir = cache_dir @@ -295,6 +345,12 @@ def _ready(self) -> tuple[Any, Any, str, bool]: def _load(self) -> tuple[Any, Any, str, bool]: """Processor and model, once per provider. + **Which pair is loaded is the connection's family's answer, not this + method's.** :data:`_CLASSES` holds it, one row per family, and the argument + for each row is written there. What stays here is everything that does not + vary: the device resolution, the dtype, and ``local_files_only``, because + nothing on a load path may reach the network. + **``transformers`` warns here on every load, and the warning is expected.** The published SAM 2 checkpoints declare ``model_type: sam2_video``, so loading one into ``Sam2Model`` prints *"You are using a model of type @@ -329,8 +385,9 @@ def _load(self) -> tuple[Any, Any, str, bool]: "cache_dir": str(self._cache_dir), "local_files_only": True, } - processor = transformers.AutoProcessor.from_pretrained(self._model_id, **common) - model = transformers.Sam2Model.from_pretrained( + processor_class, model_class = _CLASSES[self._family] + processor = getattr(transformers, processor_class).from_pretrained(self._model_id, **common) + model = getattr(transformers, model_class).from_pretrained( self._model_id, dtype=torch.float16 if half else torch.float32, **common, diff --git a/src/visionset/inference/weights.py b/src/visionset/inference/weights.py index 6667529f..a421b35f 100644 --- a/src/visionset/inference/weights.py +++ b/src/visionset/inference/weights.py @@ -55,7 +55,7 @@ from collections.abc import Callable, Iterator, Sequence from contextlib import contextmanager from pathlib import Path -from typing import Final +from typing import Any, Final from uuid import UUID from visionset.inference._extra import imported @@ -298,6 +298,49 @@ def _awaiting_a_family(connection: InferenceConnection) -> bool: ) +def _needs_approved_access(hub: Any, exc: BaseException) -> bool: + """Whether the hub declined because nobody has cleared that model's terms. + + **Told by the exception's class and never by its status code.** A gated + repository answers **401** to an unauthenticated caller — not the 403 the word + "gated" suggests — and so does a repository that is private, and so does one + whose id was mistyped and does not exist. Branching on the number would report + a typo as a licence somebody has to go and accept, sending a reader to a page + that is not there. ``huggingface_hub`` separates the cases itself, and + ``GatedRepoError`` is the one that means what this asks. + + The lookup is defensive because the class is being read off a runtime this + build declares a floor for rather than a pin: an installation old enough not to + publish the name answers ``False`` and takes the general message, which is the + behaviour every release had before this one. + + Order matters at the call site and is the reason this is a function rather than + an ``except`` clause. ``GatedRepoError`` **inherits from** + ``RepositoryNotFoundError``, so a handler naming the parent first swallows the + gated case and reports a missing model — the trap the media adapter records for + ``UnidentifiedImageError`` under ``OSError``, and the persistence adapter for + ``IntegrityError`` under ``DatabaseError``. + """ + gated = getattr(getattr(hub, "errors", None), "GatedRepoError", None) + return gated is not None and isinstance(exc, gated) + + +def _access_remedy(model_id: str) -> str: + """What happened and what to do about it, for a model behind an access gate. + + Two sentences, on the boundary's own error contract: one naming the state, one + naming the action that changes it. The hub's own text is deliberately dropped + rather than appended — it opens with a status line and a request id, and a + reader who has not been granted access needs neither. + """ + return ( + f"the weights for {model_id} are published under terms that have to be accepted before " + f"they can be downloaded, and this machine has not been authorised for them; request " + f"access at https://huggingface.co/{model_id}, then set HF_TOKEN to a token belonging to " + "an account that has been granted it and run the download again" + ) + + def download( connection: InferenceConnection, *, @@ -362,6 +405,12 @@ def download( # kernel's vocabulary is the same translation ``_built`` does for # pydantic, and it is what keeps a failed job carrying a sentence # instead of a library traceback nobody can act on. + # + # One of those ways has a remedy a person can act on, so it gets its own + # sentence rather than arriving inside the general one carrying an HTTP + # status line and a request id. + if _needs_approved_access(hub, exc): + raise LocalInferenceUnavailable(_access_remedy(connection.model_id)) from exc raise LocalInferenceUnavailable( f"could not fetch {connection.model_id} at {connection.model_revision}: {exc}" ) from exc @@ -491,6 +540,13 @@ def measure(model_id: str, model_revision: str) -> DownloadSize: try: info = hub.model_info(model_id, revision=model_revision, files_metadata=True) except Exception as exc: # noqa: BLE001 — ``download``'s reason, one call earlier + # A gate is on the *files*, not on the listing: the hub sizes a gated + # repository for anybody, which is what lets somebody see what a download + # costs before deciding whether to go and ask for it. So this branch is + # unreachable for a merely gated model and is kept for the private one, + # where the same sentence is the same remedy. + if _needs_approved_access(hub, exc): + raise LocalInferenceUnavailable(_access_remedy(model_id)) from exc raise LocalInferenceUnavailable( f"could not read the size of {model_id} at {model_revision}: {exc}" ) from exc diff --git a/tests/inference/stubs.py b/tests/inference/stubs.py index 08b9e9da..3f1dc290 100644 --- a/tests/inference/stubs.py +++ b/tests/inference/stubs.py @@ -166,6 +166,14 @@ class StubTorch: """ float16 = "float16" + float32 = "float32" + """Both dtypes, because a load picks between them and only one had a stand-in. + + ``float16`` was enough while every test entered below the loader with + ``_ready`` already stubbed. The tests that drive ``_load`` itself take the + other branch — full precision is what a CPU resolves to — and would otherwise + fail on the stub rather than on the code. + """ def __init__(self, *, cuda: bool = False, mps: bool = False, mps_usable: bool = True) -> None: self.nn = SimpleNamespace(functional=Functional()) diff --git a/tests/inference/test_families.py b/tests/inference/test_families.py index 0ecfcfd4..6d2d30ec 100644 --- a/tests/inference/test_families.py +++ b/tests/inference/test_families.py @@ -57,9 +57,63 @@ def a_local(connections: InferenceConnectionService) -> Any: # --- the sets themselves ------------------------------------------------------ -def test_both_spellings_of_the_one_architecture_are_named() -> None: - """A set rather than a string, and both members are load-bearing today.""" - assert {"sam2", "sam2_video"} <= SEGMENTER_FAMILIES +def test_every_published_checkpoint_this_build_curates_is_named() -> None: + """The strings real checkpoints declare, read out of their configs. + + A checkpoint declares the variant it was *published* as rather than the half + this build asks for, so these are the spellings a resolver actually meets: + the SAM 2.1 ladder says ``sam2_video`` and ``facebook/sam3`` says + ``sam3_video``. Missing one sends that model to the detector adapter, which + then refuses a click with a sentence about text prompts. + """ + assert {"sam2", "sam2_video", "sam3_video"} <= SEGMENTER_FAMILIES + + +@pytest.mark.parametrize( + ("family", "what_it_is"), + [ + ("sam3", "the concept detector nested at detector_config — it answers words"), + ("sam3_tracker", "a config class no known checkpoint declares"), + ("sam3_tracker_video", "the video tracker nested at tracker_config"), + ], +) +def test_the_names_around_the_published_one_are_not_mistaken_for_it( + family: str, what_it_is: str +) -> None: + """The three near-misses, and why each is absent. This is a regression test. + + An earlier revision of this register carried ``sam3`` and ``sam3_tracker``, + derived from the names of the ``transformers`` classes rather than read from a + config, and the whole feature was unreachable: ``facebook/sam3`` declares + ``sam3_video`` at the top level and neither of those anywhere a resolver + looks. Every test agreed, because they had all been written against the same + guess the implementation made. + + ``sam3`` is the worse of the two to admit. It is what the *detector* half + declares, so serving it here would hand a text-prompt model to the point + adapter — the confident wrong answer this module's opening warns about, rather + than a gap somebody notices. + """ + assert family not in SEGMENTER_FAMILIES, what_it_is + + +def test_the_nested_halves_of_a_config_are_not_offered_as_models() -> None: + """A register of whole models, so an encoder half is refused rather than loaded. + + The runtime registers a ``model_type`` for every nested piece of these + architectures as well as for the wholes. A connection naming one of those is + something the resolver must decline; admitting it here would hand the adapter + a config with no mask decoder in it and turn a refusal into a failure inside a + forward pass. + """ + halves = { + "sam2_vision_model", + "sam2_hiera_det_model", + "sam3_vision_model", + "sam3_mask_decoder", + "sam3_detr_decoder", + } + assert not halves & SUPPORTED_FAMILIES def test_the_two_families_are_disjoint_and_are_the_whole_of_what_is_supported() -> None: diff --git a/tests/inference/test_provider_concurrency.py b/tests/inference/test_provider_concurrency.py index 5e45efc0..4a3cbf17 100644 --- a/tests/inference/test_provider_concurrency.py +++ b/tests/inference/test_provider_concurrency.py @@ -89,6 +89,7 @@ def gated(monkeypatch: pytest.MonkeyPatch, parties: int) -> tuple[LocalSamProvid provider = LocalSamProvider( "some/segmenter", "abc123", + family="sam2", device="cpu", precision=None, cache_dir=Path("/nowhere"), diff --git a/tests/inference/test_sam_provider.py b/tests/inference/test_sam_provider.py index 8fa76129..d02a653e 100644 --- a/tests/inference/test_sam_provider.py +++ b/tests/inference/test_sam_provider.py @@ -23,6 +23,7 @@ from tests.inference.stubs import StubModel, StubProcessor, StubTorch, blank, disc from visionset.inference import sam_provider +from visionset.inference.families import SEGMENTER_FAMILIES from visionset.inference.sam_provider import ( NEGATIVE, POSITIVE, @@ -51,6 +52,7 @@ def built( *, masks: list[list[list[bool]]] | None = None, scores: list[float] | None = None, + family: str = "sam2", ) -> tuple[LocalSamProvider, StubProcessor, StubModel]: """A provider whose model is a script, with everything else as shipped.""" processor = StubProcessor(masks or [disc(20)], scores or [0.9]) @@ -58,6 +60,7 @@ def built( provider = LocalSamProvider( "some/segmenter", "abc123", + family=family, device="cpu", precision=None, cache_dir=Path("/nowhere"), @@ -220,3 +223,117 @@ def test_the_cache_is_bounded_and_evicts_the_least_recently_used( list(provider.segment(asked(one_click(), one))) assert provider.encodes == 4, "the first asset was evicted by the third and re-encoded" + + +# --- which classes a family loads through ------------------------------------- + + +class _Loaded: + """Whatever ``from_pretrained`` handed back, remembering how it was asked for.""" + + def __init__(self, model_id: str, options: dict[str, Any]) -> None: + self.model_id = model_id + self.options = options + + def to(self, _: str) -> _Loaded: + return self + + def eval(self) -> _Loaded: + return self + + +class _Recorder: + """A stand-in ``transformers`` that records which classes a load reached for. + + Deliberately answers *every* attribute rather than only the expected ones, so + a row naming a class this build does not ship fails on the assertion about + which names were asked for rather than on an ``AttributeError`` that would + read as a broken stub. + """ + + def __init__(self) -> None: + self.asked: list[str] = [] + self.loaded: list[_Loaded] = [] + + def __getattr__(self, name: str) -> Any: + self.asked.append(name) + recorder = self + + class _Class: + @staticmethod + def from_pretrained(model_id: str, **options: Any) -> _Loaded: + made = _Loaded(model_id, options) + recorder.loaded.append(made) + return made + + return _Class + + def load(self, provider: LocalSamProvider, monkeypatch: pytest.MonkeyPatch) -> None: + torch = StubTorch() + monkeypatch.setattr( + sam_provider, "imported", lambda name: torch if name == "torch" else self + ) + provider._load() + + +def a_provider(family: str) -> LocalSamProvider: + return LocalSamProvider( + "some/segmenter", + "abc123", + family=family, + device="cpu", + precision=None, + cache_dir=Path("/nowhere"), + connection_name="local", + ) + + +def test_the_class_table_covers_every_segmenter_family() -> None: + """The two registers agree, and nothing else makes them. + + The resolver sends a connection here on the strength of its family being in + ``SEGMENTER_FAMILIES``; the loader then looks that family up in ``_CLASSES``. + A family added to the first and forgotten in the second resolves to this + adapter and dies on a ``KeyError`` inside a load — past every refusal, in a + place whose message names a dictionary rather than a model. + """ + assert set(sam_provider._CLASSES) == SEGMENTER_FAMILIES + + +@pytest.mark.parametrize( + ("family", "expected"), + [ + ("sam2", ["AutoProcessor", "Sam2Model"]), + ("sam2_video", ["AutoProcessor", "Sam2Model"]), + ("sam3_video", ["Sam3TrackerProcessor", "Sam3TrackerModel"]), + ], +) +def test_each_family_loads_through_its_own_classes( + family: str, expected: list[str], monkeypatch: pytest.MonkeyPatch +) -> None: + """Named rather than resolved, for the half that cannot be resolved correctly. + + SAM 2 keeps ``AutoProcessor`` because the repositories it is pointed at + declare a processor that takes points. Asked about ``facebook/sam3`` the same + call answers ``Sam3VideoProcessor`` — measured against the real repository — + which is the video path, so resolving there would hand this adapter something + no single-image click can be expressed to and the failure would surface inside + a call rather than in a refusal. + """ + recorder = _Recorder() + recorder.load(a_provider(family), monkeypatch) + assert recorder.asked == expected + + +def test_a_load_never_reaches_the_network(monkeypatch: pytest.MonkeyPatch) -> None: + """Weights are fetched when somebody asks for them and at no other time. + + Both halves of the load carry it, which is what the assertion checks: a + processor quietly downloading its own config on first use would defeat the + rule as thoroughly as the model doing it, and it is the easier of the two to + leave out when a row is added. + """ + recorder = _Recorder() + recorder.load(a_provider("sam3_video"), monkeypatch) + assert [one.options["local_files_only"] for one in recorder.loaded] == [True, True] + assert {one.options["revision"] for one in recorder.loaded} == {"abc123"} diff --git a/tests/inference/test_weights.py b/tests/inference/test_weights.py index ff3e829f..c29d98b3 100644 --- a/tests/inference/test_weights.py +++ b/tests/inference/test_weights.py @@ -442,6 +442,144 @@ def snapshot_download(**_: object) -> str: assert "Repository Not Found" in str(raised.value) +class _HubErrors: + """The hub client's exception tree, in the shape the real one has. + + The inheritance is the point rather than scenery. ``GatedRepoError`` really + does derive from ``RepositoryNotFoundError``, so a handler naming the parent + first swallows the gated case; a flat pair of unrelated classes here would let + that mistake pass every test in this file. + """ + + class RepositoryNotFoundError(OSError): + pass + + class GatedRepoError(RepositoryNotFoundError): + pass + + +def _refusing(raised: BaseException) -> type: + """A hub whose every call fails that way, with its error tree attached.""" + + class FakeHub: + errors = _HubErrors + + @staticmethod + def snapshot_download(**_: object) -> str: + raise raised + + @staticmethod + def model_info(*_: object, **__: object) -> object: + raise raised + + return FakeHub + + +def _a_connection() -> InferenceConnection: + return InferenceConnection( + name="local-gated", + connection_type=ConnectionType.LOCAL, + model_id="somebody/gated-model", + model_revision="abc123", + device="cpu", + precision="fp32", + ) + + +GATED = _HubErrors.GatedRepoError( + "401 Client Error. (Request ID: Root=1-6a8074a4-3bc3584f53a6fc7333db2091)\n\n" + "Cannot access gated repo for url https://huggingface.co/somebody/gated-model/resolve/" + "main/config.json.\nAccess to model somebody/gated-model is restricted. You must have " + "access to it and be authenticated to access it. Please log in." +) +"""The real thing, kept verbatim: this is what an unauthenticated fetch raises.""" + + +def test_a_model_behind_an_access_gate_says_so_and_names_the_remedy( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """What happened and what to do, with none of the transport in between. + + The library's own text opens with a status line and a request id, and the + general translation would carry both into the sentence a person reads on a + failed job. Neither is actionable, and a status code in particular invites the + reader to go and look up a number instead of clearing the gate. + """ + monkeypatch.setattr(weights_module, "imported", lambda name: _refusing(GATED)) + with pytest.raises(LocalInferenceUnavailable) as raised: + download(_a_connection(), into=tmp_path / MODELS_DIRNAME) + + message = str(raised.value) + assert "have to be accepted" in message + assert "https://huggingface.co/somebody/gated-model" in message + assert "HF_TOKEN" in message + assert "401" not in message, "no status code reaches a reader" + assert "Request ID" not in message + assert "Client Error" not in message + + +def test_reading_a_size_behind_the_gate_gives_the_same_remedy( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The other call that reaches the hub, and the same answer. + + A gate is on the files rather than on the listing, so this is unreachable for + a merely gated repository and is the private-repository path. It is asserted + because the two call sites translate separately, and one of them having the + sentence is what makes the other's absence invisible. + """ + monkeypatch.setattr(weights_module, "imported", lambda name: _refusing(GATED)) + with pytest.raises(LocalInferenceUnavailable) as raised: + weights_module.measure("somebody/gated-model", "abc123") + + assert "have to be accepted" in str(raised.value) + assert "Request ID" not in str(raised.value) + + +def test_a_repository_that_is_not_there_is_not_reported_as_a_licence_to_accept( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """The discrimination the whole translation turns on. + + A gated repository and a mistyped one both answer **401** to an + unauthenticated caller, so a branch on the status code would send somebody + whose model id has a typo to a page that does not exist, to ask for access to + a model nobody publishes. Only the exception's class separates them, and this + is the case that proves the class is what is being read. + """ + missing = _HubErrors.RepositoryNotFoundError("401 Client Error. Repository Not Found") + monkeypatch.setattr(weights_module, "imported", lambda name: _refusing(missing)) + with pytest.raises(LocalInferenceUnavailable) as raised: + download(_a_connection(), into=tmp_path / MODELS_DIRNAME) + + message = str(raised.value) + assert "could not fetch somebody/gated-model at abc123" in message + assert "have to be accepted" not in message + assert "HF_TOKEN" not in message + + +def test_a_client_too_old_to_name_the_error_still_reports_the_failure( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """The floor is a floor rather than a pin, so the class may not be there. + + An installation predating the name answers with the general translation, which + is what every release did before this one. Looking the class up defensively is + what keeps that a degraded message rather than an ``AttributeError`` raised + while handling the original failure. + """ + + class OldHub: + @staticmethod + def snapshot_download(**_: object) -> str: + raise OSError("something went wrong") + + monkeypatch.setattr(weights_module, "imported", lambda name: OldHub) + with pytest.raises(LocalInferenceUnavailable) as raised: + download(_a_connection(), into=tmp_path / MODELS_DIRNAME) + assert "could not fetch somebody/gated-model at abc123" in str(raised.value) + + def test_the_cache_directory_is_created_by_the_download( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: diff --git a/tests/scripts/compose_token.test.mjs b/tests/scripts/compose_token.test.mjs new file mode 100644 index 00000000..e136d806 --- /dev/null +++ b/tests/scripts/compose_token.test.mjs @@ -0,0 +1,67 @@ +// Run with: pnpm test:scripts (also part of the root `pnpm test`) +// +// The dev stack has to hand the hub credential to the container that fetches weights, +// because some published models are served only to accounts that have been granted +// them. Without it the failure is the bad kind rather than a loud one: the refusal +// names `HF_TOKEN`, the reader exports it on the host, the container never sees it, +// and the identical refusal comes back with nothing about it changed. A refusal +// naming a remedy nobody in that position can take is the mistake `NotAWorkspace` is +// the standing example of. +// +// Asserted by reading the files rather than by rendering them, deliberately: `docker +// compose config` needs a Docker daemon, and CI's `frontend` job has none. What that +// costs is the *rendering*, which is why the second test below pins the one property +// a text read could otherwise miss — that neither overlay replaces the block the +// variable lives in. +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { dirname, join } from "node:path"; +import { test } from "node:test"; + +const ROOT = join(dirname(fileURLToPath(import.meta.url)), "..", ".."); +const read = (name) => readFileSync(join(ROOT, "docker", name), "utf8"); + +const BASE = "compose.yaml"; +/** The two ways to get a runtime that downloads weights at all. */ +const OVERLAYS = ["compose.cpu-inference.yaml", "compose.gpu.yaml"]; + +test("the api service forwards the hub credential from whoever ran compose", () => { + // `${HF_TOKEN:-}` rather than a bare value: it is the host's to supply, and the + // empty default is what keeps a machine with no token running every ungated model + // exactly as before instead of failing to start. + assert.match( + read(BASE), + /^\s*HF_TOKEN:\s*\$\{HF_TOKEN:-\}\s*$/m, + "docker/compose.yaml must forward HF_TOKEN to the api service", + ); +}); + +test("neither inference overlay replaces the block that carries it", () => { + // This is the half a grep for the variable cannot see. Both overlays amend the same + // `api` service the base defines, and Compose merges mappings key by key — so an + // overlay is free to add to `environment` and would be overwriting nothing. What + // would silently drop the credential from one stack and not the other is an overlay + // that grew its own `environment:` and a reader assuming it merged. + for (const overlay of OVERLAYS) { + const text = read(overlay); + assert.ok( + !/^\s*environment:/m.test(text), + `docker/${overlay} declares its own environment: — re-check that HF_TOKEN survives ` + + "the merge, and say so here, because the base is no longer the only place it lives", + ); + } +}); + +test("every stack that can fetch weights is built on the base that carries the token", () => { + // The overlays are `-f` amendments rather than standalone stacks, which is the only + // reason the base's environment reaches them at all. An overlay that stopped naming + // the same service would start a second one beside `api` instead of amending it. + for (const overlay of OVERLAYS) { + assert.match( + read(overlay), + /^services:\s*$\n^\s+api:\s*$/m, + `docker/${overlay} must amend the base's own api service`, + ); + } +}); diff --git a/uv.lock b/uv.lock index de69485e..1eb17d8d 100644 --- a/uv.lock +++ b/uv.lock @@ -2643,7 +2643,7 @@ requires-dist = [ { name = "sqlalchemy", specifier = ">=2.0" }, { name = "torch", marker = "extra == 'local-inference'", specifier = ">=2.4" }, { name = "torchvision", marker = "extra == 'local-inference'", specifier = ">=0.19" }, - { name = "transformers", marker = "extra == 'local-inference'", specifier = ">=4.44" }, + { name = "transformers", marker = "extra == 'local-inference'", specifier = ">=5.0" }, { name = "typer", specifier = ">=0.12" }, { name = "uvicorn", specifier = ">=0.52.0" }, ]