diff --git a/DESIGN.md b/DESIGN.md index 794a8c0..f8fd344 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -468,21 +468,37 @@ make the aside jump on every visit. ### Continue where you left off -The card names a project, a batch, `n / m annotated` in tabular figures, and a picture of -the frame it would open. It holds the view's one filled button. - -**It is ranked by progress, not by recency, and that is forced rather than chosen.** There -is no timestamp anywhere on a batch, an annotation, or an asset's progress, so *the batch -I touched last* has no source in the storage format and deriving one would mean a -migration. What the rows can answer is which batch is furthest through and not yet -finished, which is what the card offers. It degrades correctly — one open batch is offered -whatever its progress — and the accepted limit is that two batches part-way through -resolve to the further-along one rather than the more recent. - -**The label is load-bearing.** With a frame left to label the control reads **Continue -annotating** and opens the editor at that frame. With none left it reads **Open batch** and -goes to the gallery, because there is no frame to open and a button claiming otherwise -would land somewhere empty. Same card, same slot, different promise. +The card names a project, a batch, a count in tabular figures, and a picture of the frame +it would open. It holds the view's one filled button. + +**It is ranked by when somebody last worked the batch.** That is the only work-dating +timestamp in the storage format, and it exists for this. Batches nobody has worked since it +became recordable rank behind every batch that has one, ordered among themselves by how far +through they are — which is the whole population of a workspace created before the stamp +existed, since it was added without a backfill and a moment that was never recorded cannot +be invented. Such a workspace therefore behaves as it did before and converges to real +recency the moment anybody uses it. A batch still has no age of its own: nothing dates its +creation or its state changes, so *the oldest untouched batch* remains underivable. + +**The label is load-bearing, and it is the wire that picks it.** The card renders three +promises out of one slot: **Continue annotating** when the batch has a frame nobody has +labeled, **Review annotations** when the labeling is done and frames are waiting on a +reviewer, and **Open batch** when it needs neither and there is no frame to open. The first +two go to the editor and the third to the gallery, because a button claiming a frame that +does not exist would land somewhere empty. + +The two editor promises are the same destination, and that is the point rather than a +shortcut: a frame awaiting review opens read-only with **Accept** and **Return to +annotator** on it, so the reviewer and the annotator are one screen wearing what it is +looking at. Under *Review annotations* the count line changes with the label — `k waiting on +review` rather than `n / m annotated`, since how much of the batch is labeled is not the +number anybody came for. + +**Which of the three applies is decided by the kernel and carried on the response.** The +order between them is a judgment about what somebody should do next, not a fact the rest of +the response restates, and a judgment spelled once in the kernel and again in the browser is +one that drifts. Contrast the first-run state, which is deliberately *not* a field: that one +is a count the response already carries. ### Needs your attention, and activity @@ -500,13 +516,19 @@ project*, not one run finishing, because an ingest records no time at all; and a reports a version being **created**, because which version is active is derived — it is the highest — so there is no activation to date. -### The one filled button, in all three states +### The one filled button, in every state -Principle 8 is a count, so Home answers it in every state and there is never a fourth: -first run offers **Create project**, a workspace with somewhere to carry on offers -**Continue annotating**, and one with nothing open offers **New project** — because when -every batch is finished, starting the next piece of work genuinely is what comes next. -When the resume card renders, the header's own New project steps back to `secondary`. +Principle 8 is a count, so Home answers it in every state and there is never a second. +First run offers **Create project**. A workspace with somewhere to carry on offers whichever +of **Continue annotating**, **Review annotations** or **Open batch** the resume card +resolved to, and the header's own New project steps back to `secondary` behind it. Only a +workspace with nothing open at all offers **New project** as the filled one — when every +batch is finished and nothing is waiting on a reviewer, starting the next piece of work +genuinely is what comes next. + +That last state is the one to keep honest. It used to absorb a workspace whose only +outstanding work was a review, which meant the page answered "what do I do next?" by +suggesting more work rather than by naming the work already waiting. The first-run CTA opens the **same dialog** the project list's button opens, rather than navigating to the screen that carries it. A filled button labelled *Create project* that diff --git a/docs/api.md b/docs/api.md index c777e4c..12bb963 100644 --- a/docs/api.md +++ b/docs/api.md @@ -375,12 +375,24 @@ here to act on, only rows pointing at resources that declare their own capabilit because the page it answers asks four questions that each span every project, and answering them as separate resources would be a request per project per question. -Two of its fields are honest about limits the storage format imposes, and a client should render -them as they are described rather than as it might wish them. `resume` is ranked by **progress, -not recency** - nothing records when a batch was last worked on - and a null `next_asset_id` means -the batch has no unlabeled frame left, so the caller opens its gallery rather than the editor. An -`ingest` activity entry is the newest asset arrival in a project rather than one run finishing, -because an ingest job records no times. +**`resume` declares its own kind, and that is the field to read first.** `annotate` means +`next_asset_id` is a frame nobody has labeled, `review` means it is one awaiting a reviewer, and +`open` means the batch is settled throughout and `next_asset_id` is null - so the caller opens its +gallery rather than the editor. The three are in priority order and the order is resolved here: it +is a judgment about what somebody should do next rather than a fact the rest of the response +restates, and a client that worked it out again from the other fields would be keeping a second +copy of a rule that can drift. Contrast the first-run state, which is deliberately *not* a field +because `totals.projects` already answers it. + +Batches are ranked by when somebody last worked them - the one work-dating timestamp in the +storage format. Ones nobody has worked since that became recordable rank last, ordered among +themselves by how far through they are, which is every batch in a workspace created before the +stamp existed: it was added without a backfill, because a moment that was never recorded cannot be +reconstructed. Such a workspace behaves as it did before and converges as soon as anybody uses it. + +One further field is honest about a limit the storage format still imposes, and a client should +render it as described rather than as it might wish. An `ingest` activity entry is the newest +asset arrival in a project rather than one run finishing, because an ingest job records no times. ## Where the UI lives diff --git a/docs/persistence.md b/docs/persistence.md index 2fb2e1b..85f8e6f 100644 --- a/docs/persistence.md +++ b/docs/persistence.md @@ -159,11 +159,12 @@ MIGRATIONS: list[Migration] = [ Migration(version=5, name="schema_provenance", upgrade=_add_schema_provenance), Migration(version=6, name="inference_connections", upgrade=_add_inference_connections), Migration(version=7, name="model_family", upgrade=_add_model_family), + Migration(version=8, name="progress_touched", upgrade=_add_progress_touched), ] -FORMAT_VERSION: int = MIGRATIONS[-1].version # 7 +FORMAT_VERSION: int = MIGRATIONS[-1].version # 8 ``` -**Generation 1 is the baseline, and the six entries after it are ordinary migrations.** A long +**Generation 1 is the baseline, and the seven entries after it are ordinary migrations.** A long chain of generations got this schema to its present shape while VisionSet was unreleased. Every database they could have upgraded was disposable test data inside this repository, so what they actually bought was an idempotency argument and an undo line per generation, plus diff --git a/docs/ui.md b/docs/ui.md index 6962661..7ca56ac 100644 --- a/docs/ui.md +++ b/docs/ui.md @@ -138,14 +138,29 @@ mutation takes it as input; every row deep-links to a resource whose own wire sh says what may be done to it. A second copy of those declarations here would be the hand-mirrored table the capabilities contract forbids, one layer up. -Two things about it are consequences of the storage format rather than choices, and -both are stated on the endpoint as well as here. The resume target is ranked by -**progress, not recency** - no timestamp exists on a batch, an annotation or an -asset's progress row, so "most recently worked on" has no source - and when its -`next_asset_id` is null the batch has no unlabeled frame left, which is the client's -signal to open the gallery and to say *Open batch* rather than *Continue annotating*. -The activity feed's `ingest` entry is the newest `Asset.ingested_at` in a project -rather than a run finishing, because an ingest job records no times at all. +**The resume target declares its own kind, and the screen renders it rather than +working it out.** `annotate` means `next_asset_id` is a frame nobody has labeled, +`review` means it is one awaiting a reviewer, and `open` means the batch is settled +throughout and there is no frame at all. The card's label follows - *Continue +annotating*, *Review annotations*, *Open batch* - and so does its destination: the +first two open the editor, the third the gallery. The two editor cases are the same +route because a `review_pending` frame opens read-only with the review actions on it, +which is the position [the annotator section below](#review-is-a-flow-not-an-api-only-edge) +already takes. + +The order between the three is resolved on the server. It is a judgment about what +somebody should do next rather than a fact the rest of the response restates, so a +client deriving it again would be keeping a second copy of a rule that can drift - +the shape of defect the capabilities contract exists to prevent. Batches are ranked +by when somebody last worked them, with ones nobody has worked since that became +recordable ranked last and ordered among themselves by progress. That second group +is every batch in a workspace created before the stamp existed, since it was added +without a backfill. + +One thing about the page is still a consequence of the storage format rather than a +choice, and it is stated on the endpoint as well as here: the activity feed's +`ingest` entry is the newest `Asset.ingested_at` in a project rather than a run +finishing, because an ingest job records no times at all. A workspace with no projects reads zeros, nulls and empty lists. That is the first-run state, and `totals.projects` is how the screen recognises it - not a flag, diff --git a/docs/workspaces.md b/docs/workspaces.md index ad34b7c..c9607d7 100644 --- a/docs/workspaces.md +++ b/docs/workspaces.md @@ -162,7 +162,7 @@ A missing `blobs/` is repaired rather than rejected: zip archives and git both d directories, so its absence says nothing about the workspace's health. **Older workspaces are migrated, not refused** — that is what the migration list is for. It -holds seven entries today: the baseline, and six that have appended a column or a table since. +holds eight entries today: the baseline, and seven that have appended a column or a table since. A workspace stamped below `FORMAT_VERSION` runs whatever is pending and is restamped, in place and on the way in. See [persistence.md](persistence.md#migrations-and-format_version) for the list itself and for the rules a new entry has to satisfy. The honest cost of an in-place diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index 0bd6441..5c28267 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -709,12 +709,18 @@ export interface paths { * project list, not a copy of it, and `activity` is capped — both have a screen * that owns them in full. * - * `resume` is the batch to carry on with: the one furthest through that still - * has an unannotated frame. It is **derived on every call and never stored**, - * and it is ranked by progress rather than by recency because nothing in the - * workspace records when a batch was last worked on. When `next_asset_id` is - * null the batch has no unlabeled frame left — open its gallery rather than the - * editor. `resume` itself is null when no batch is open for annotation. + * `resume` is the batch to carry on with, **derived on every call and never + * stored**. Read its `kind` first: `annotate` means `next_asset_id` is a frame + * nobody has labeled, `review` means it is one awaiting a reviewer, and `open` + * means the batch is settled throughout and `next_asset_id` is null — open its + * gallery rather than the editor. The three are in priority order, decided + * here, and a client renders what it is told rather than working it out again. + * `resume` itself is null when no batch is open for annotation. + * + * Batches are ranked by when somebody last worked them. Ones nobody has worked + * since that became recordable rank last, ordered among themselves by how far + * through they are — which is every batch in a workspace created before the + * stamp existed, since it was added without a backfill. * * `attention` carries batches with frames awaiting review, and background jobs * that failed or are still running. A job row has no `project_id`: a job names @@ -3636,9 +3642,19 @@ export interface components { */ release_id: string; }; + /** + * ResumeKind + * @description What an open batch is being offered for, and so what `next_asset_id` is. + * + * `annotate` - a frame nobody has labeled, which is that frame. + * `review` - every frame is labeled or set aside and some await a reviewer, + * which is the first of those. `open` - neither, and `next_asset_id` is null. + * @enum {string} + */ + ResumeKind: "annotate" | "review" | "open"; /** * ResumeTargetOut - * @description The batch to carry on with, and where inside it to land. + * @description The batch to carry on with, what for, and where inside it to land. */ ResumeTargetOut: { /** Annotated */ @@ -3655,6 +3671,7 @@ export interface components { * Format: uuid */ job_id: string; + kind: components["schemas"]["ResumeKind"]; /** Next Asset Id */ next_asset_id: string | null; /** @@ -3664,6 +3681,8 @@ export interface components { project_id: string; /** Project Name */ project_name: string; + /** Review Pending */ + review_pending: number; /** Thumbnail Asset Id */ thumbnail_asset_id: string | null; /** Total */ diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index 348b43c..3b0acf5 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -188,8 +188,11 @@ export const checkAttentionItemOut: Check = export const checkProjectSummaryOut: Check = /*#__PURE__*/ object({ "annotated_fraction": [true, isNumber], "asset_count": [true, isInteger], "name": [true, isString], "project_id": [true, isString] } as const); +export const checkResumeKind: Check = + /*#__PURE__*/ oneOf(["annotate", "review", "open"] as const); + export const checkResumeTargetOut: Check = - /*#__PURE__*/ object({ "annotated": [true, isInteger], "batch_id": [true, isString], "batch_name": [true, isString], "job_id": [true, isString], "next_asset_id": [true, either([isString, isNull] as const)], "project_id": [true, isString], "project_name": [true, isString], "thumbnail_asset_id": [true, either([isString, isNull] as const)], "total": [true, isInteger] } as const); + /*#__PURE__*/ object({ "annotated": [true, isInteger], "batch_id": [true, isString], "batch_name": [true, isString], "job_id": [true, isString], "kind": [true, checkResumeKind], "next_asset_id": [true, either([isString, isNull] as const)], "project_id": [true, isString], "project_name": [true, isString], "review_pending": [true, isInteger], "thumbnail_asset_id": [true, either([isString, isNull] as const)], "total": [true, isInteger] } as const); export const checkWorkspaceTotalsOut: Check = /*#__PURE__*/ object({ "annotations": [true, isInteger], "assets": [true, isInteger], "projects": [true, isInteger], "releases": [true, isInteger] } as const); diff --git a/frontend/ui-core/src/screens/HomeScreen.tsx b/frontend/ui-core/src/screens/HomeScreen.tsx index f59f248..0283c5b 100644 --- a/frontend/ui-core/src/screens/HomeScreen.tsx +++ b/frontend/ui-core/src/screens/HomeScreen.tsx @@ -245,13 +245,26 @@ function Stage({ ); } +/** What the control promises, per kind. Presentation only — the order is the wire's. */ +const RESUME_LABEL: Record = { + annotate: "Continue annotating", + review: "Review annotations", + open: "Open batch", +}; + /** * Where to carry on, and the page's one filled button when it renders. * - * The label is not decoration. With a frame to open it says **Continue - * annotating** and goes into the editor; with none left it says **Open batch** - * and goes to the gallery, because the batch is still worth opening and there is - * no frame to open it at. + * The label is not decoration, and neither is the destination. Under `annotate` + * and `review` there is a frame, so the control opens the editor at it — the same + * screen either way, because a frame awaiting review opens read-only with the + * review actions on it and there is no second screen to send anybody to. Under + * `open` there is no frame at all, so it goes to the gallery rather than offering + * a link that would land somewhere empty. + * + * Which of the three applies is `resume.kind`, decided by the kernel. This + * component reads it; it does not work it out from the other fields, because the + * order between the three is a decision and a second copy of it here would drift. */ function Resume({ resume, @@ -262,7 +275,7 @@ function Resume({ readonly onContinue?: (jobId: string, assetId: string | null) => void; readonly onOpenBatch?: (projectId: string, batchId: string) => void; }): JSX.Element { - const hasFrame = resume.next_asset_id !== null; + const hasFrame = resume.kind !== "open"; const share = resume.total === 0 ? 0 : (resume.annotated / resume.total) * 100; const act = hasFrame ? onContinue === undefined @@ -276,7 +289,7 @@ function Resume({

Continue where you left off

@@ -299,14 +312,17 @@ function Resume({ {resume.project_name} {resume.batch_name} - {formatCount(resume.annotated)} / {formatCount(resume.total)} annotated ·{" "} - {formatPercent(share)} + {/* Under `review` the labeling is finished, so how much of it is + labeled is not the number anybody is here for. */} + {resume.kind === "review" + ? `${formatCount(resume.review_pending)} waiting on review` + : `${formatCount(resume.annotated)} / ${formatCount(resume.total)} annotated · ${formatPercent(share)}`}
{act !== undefined && ( )} diff --git a/frontend/ui-core/src/screens/home.test.tsx b/frontend/ui-core/src/screens/home.test.tsx index 5c3a2d2..2eba893 100644 --- a/frontend/ui-core/src/screens/home.test.tsx +++ b/frontend/ui-core/src/screens/home.test.tsx @@ -76,6 +76,7 @@ function homeBody(overrides: Record = {}): Record = {}): Record { return { + kind: "annotate", project_id: PROJECT, project_name: "Highway pilot", batch_id: BATCH, @@ -84,6 +85,7 @@ function resume(overrides: Record = {}): Record { render(mount( {}} />)); const card = await screen.findByTestId("home-resume"); - expect(card.getAttribute("data-has-frame")).toBe("true"); + expect(card.getAttribute("data-kind")).toBe("annotate"); expect(card.textContent).toContain("Highway pilot"); expect(card.textContent).toContain("Batch 3"); expect(card.textContent).toContain("148 / 200 annotated"); expect(screen.getByTestId("home-resume-cta").textContent).toContain("Continue annotating"); }); +it("sends the reviewer to the frame awaiting review, on the same route", async () => { + const opened: [string, string | null][] = []; + on("GET", /\/home$/, { + status: 200, + body: homeBody({ + resume: resume({ kind: "review", annotated: 200, total: 200, review_pending: 12 }), + }), + }); + render( + mount( + opened.push([job, asset])} + onOpenBatch={() => expect.unreachable("review opens the editor, not the gallery")} + />, + ), + ); + + const card = await screen.findByTestId("home-resume"); + expect(card.getAttribute("data-kind")).toBe("review"); + // The count line follows the label: how much of the batch is labeled is not + // the number anybody is here for once the labeling is done. + expect(card.textContent).toContain("12 waiting on review"); + expect(card.textContent).not.toContain("annotated"); + + const cta = screen.getByTestId("home-resume-cta"); + expect(cta.textContent).toContain("Review annotations"); + await userEvent.click(cta); + expect(opened).toEqual([[JOB, ASSET]]); +}); + +it("renders what the wire declares rather than deriving it from the other fields", async () => { + // A batch with a frame to label *and* frames awaiting review. The kernel + // resolved this to `annotate`; a screen that worked the priority out again + // would be keeping a second copy of a rule that can drift, and this fixture is + // where the two spellings would disagree. + on("GET", /\/home$/, { + status: 200, + body: homeBody({ resume: resume({ kind: "annotate", review_pending: 7 }) }), + }); + render(mount( {}} />)); + + const card = await screen.findByTestId("home-resume"); + expect(screen.getByTestId("home-resume-cta").textContent).toContain("Continue annotating"); + expect(card.textContent).toContain("148 / 200 annotated"); +}); + it("hands the annotator the frame the card named", async () => { const opened: [string, string | null][] = []; on("GET", /\/home$/, { status: 200, body: homeBody({ resume: resume() }) }); @@ -185,7 +233,9 @@ it("falls back to opening the batch when no frame is left to label", async () => const opened: [string, string][] = []; on("GET", /\/home$/, { status: 200, - body: homeBody({ resume: resume({ next_asset_id: null, annotated: 200 }) }), + body: homeBody({ + resume: resume({ kind: "open", next_asset_id: null, annotated: 200 }), + }), }); render( mount( @@ -359,15 +409,29 @@ it("shows exactly one filled button in the first-run state", async () => { expect(filledButtons(container)).toEqual(["Create project"]); }); -it("shows exactly one filled button when there is somewhere to carry on", async () => { - on("GET", /\/home$/, { status: 200, body: homeBody({ resume: resume() }) }); - const { container } = render(mount( {}} />)); - - await screen.findByTestId("home-resume"); - // "New project" is on screen too and must have stepped back to `secondary`; - // asserting the whole set is what catches it not having. - expect(filledButtons(container)).toEqual(["Continue annotating"]); -}); +it.each([ + ["annotate", "Continue annotating"], + ["review", "Review annotations"], + ["open", "Open batch"], +] as const)( + "shows exactly one filled button when the card is offering %s", + async (kind, label) => { + on("GET", /\/home$/, { + status: 200, + body: homeBody({ + resume: resume({ kind, next_asset_id: kind === "open" ? null : ASSET }), + }), + }); + const { container } = render( + mount( {}} onOpenBatch={() => {}} />), + ); + + await screen.findByTestId("home-resume"); + // "New project" is on screen too and must have stepped back to `secondary`; + // asserting the whole set is what catches it not having. + expect(filledButtons(container)).toEqual([label]); + }, +); it("still shows exactly one filled button when nothing is open for annotation", async () => { on("GET", /\/home$/, { status: 200, body: homeBody({ resume: null }) }); diff --git a/openapi.json b/openapi.json index 3e9db92..85c6202 100644 --- a/openapi.json +++ b/openapi.json @@ -3366,8 +3366,18 @@ "title": "ReleaseVerificationOut", "type": "object" }, + "ResumeKind": { + "description": "What an open batch is being offered for, and so what `next_asset_id` is.\n\n`annotate` - a frame nobody has labeled, which is that frame.\n`review` - every frame is labeled or set aside and some await a reviewer,\nwhich is the first of those. `open` - neither, and `next_asset_id` is null.", + "enum": [ + "annotate", + "review", + "open" + ], + "title": "ResumeKind", + "type": "string" + }, "ResumeTargetOut": { - "description": "The batch to carry on with, and where inside it to land.", + "description": "The batch to carry on with, what for, and where inside it to land.", "properties": { "annotated": { "title": "Annotated", @@ -3387,6 +3397,9 @@ "title": "Job Id", "type": "string" }, + "kind": { + "$ref": "#/components/schemas/ResumeKind" + }, "next_asset_id": { "anyOf": [ { @@ -3408,6 +3421,10 @@ "title": "Project Name", "type": "string" }, + "review_pending": { + "title": "Review Pending", + "type": "integer" + }, "thumbnail_asset_id": { "anyOf": [ { @@ -3426,6 +3443,7 @@ } }, "required": [ + "kind", "project_id", "project_name", "batch_id", @@ -3434,6 +3452,7 @@ "next_asset_id", "annotated", "total", + "review_pending", "thumbnail_asset_id" ], "title": "ResumeTargetOut", @@ -6536,7 +6555,7 @@ }, "/home": { "get": { - "description": "Everything the workspace's front page shows, in one response.\n\n`totals` counts the whole workspace. `projects` is a short shortcut into the\nproject list, not a copy of it, and `activity` is capped \u2014 both have a screen\nthat owns them in full.\n\n`resume` is the batch to carry on with: the one furthest through that still\nhas an unannotated frame. It is **derived on every call and never stored**,\nand it is ranked by progress rather than by recency because nothing in the\nworkspace records when a batch was last worked on. When `next_asset_id` is\nnull the batch has no unlabeled frame left \u2014 open its gallery rather than the\neditor. `resume` itself is null when no batch is open for annotation.\n\n`attention` carries batches with frames awaiting review, and background jobs\nthat failed or are still running. A job row has no `project_id`: a job names\nan ingest run or a release, never a project.\n\nAn empty workspace answers zeros, nulls and empty lists. That is the\nfirst-run state, and `totals.projects` is how a client recognises it.", + "description": "Everything the workspace's front page shows, in one response.\n\n`totals` counts the whole workspace. `projects` is a short shortcut into the\nproject list, not a copy of it, and `activity` is capped \u2014 both have a screen\nthat owns them in full.\n\n`resume` is the batch to carry on with, **derived on every call and never\nstored**. Read its `kind` first: `annotate` means `next_asset_id` is a frame\nnobody has labeled, `review` means it is one awaiting a reviewer, and `open`\nmeans the batch is settled throughout and `next_asset_id` is null \u2014 open its\ngallery rather than the editor. The three are in priority order, decided\nhere, and a client renders what it is told rather than working it out again.\n`resume` itself is null when no batch is open for annotation.\n\nBatches are ranked by when somebody last worked them. Ones nobody has worked\nsince that became recordable rank last, ordered among themselves by how far\nthrough they are \u2014 which is every batch in a workspace created before the\nstamp existed, since it was added without a backfill.\n\n`attention` carries batches with frames awaiting review, and background jobs\nthat failed or are still running. A job row has no `project_id`: a job names\nan ingest run or a release, never a project.\n\nAn empty workspace answers zeros, nulls and empty lists. That is the\nfirst-run state, and `totals.projects` is how a client recognises it.", "operationId": "get_home", "responses": { "200": { diff --git a/src/visionset/kernel/adapters/_tables.py b/src/visionset/kernel/adapters/_tables.py index fb0eecd..d0f3044 100644 --- a/src/visionset/kernel/adapters/_tables.py +++ b/src/visionset/kernel/adapters/_tables.py @@ -403,6 +403,21 @@ class AnnotationJobAssetRow(Base): #: NULL`` without a default. That is the price of every future ``NOT NULL`` #: column too, so it stays rather than being tidied away. position: Mapped[int] = mapped_column(Integer, nullable=False, server_default=text("0")) + #: When somebody last moved this asset's progress, or NULL if nobody has + #: since the column existed. The only timestamp in the schema that dates a + #: person's work rather than a record's creation, which is what lets the + #: workspace summary rank open batches by recency instead of by progress. + #: + #: Written exclusively inside ``UnitOfWork.set_asset_progress``'s guarded + #: ``UPDATE``, so it is stamped in the same statement as the transition it + #: records and cannot drift from it. + #: + #: Declared last, after ``position``, because it arrives by ``ALTER TABLE`` + #: — see the column-order rule at the top of this module. Nullable, so + #: unlike ``position`` it needs no ``server_default``: NULL is the honest + #: value for a row nobody has touched, and it is what the ranking's fallback + #: population is keyed on. + touched_at: Mapped[str | None] = mapped_column(String, nullable=True) class AnnotationRow(Base): diff --git a/src/visionset/kernel/adapters/migrations.py b/src/visionset/kernel/adapters/migrations.py index 6e7cbf0..d6e8d7d 100644 --- a/src/visionset/kernel/adapters/migrations.py +++ b/src/visionset/kernel/adapters/migrations.py @@ -248,6 +248,25 @@ def _add_model_family(connection: Connection) -> None: _add_column(connection, "inference_connection", "model_family") +def _add_progress_touched(connection: Connection) -> None: + """``annotation_job_asset.touched_at``: when somebody last worked this frame. + + **Nothing to backfill, and nothing that could be.** The value this column + holds is a moment that was never recorded, so there is no other row, no + derived field and no file timestamp to reconstruct it from — a workspace's + entire history of who-worked-when begins at this migration. Every existing + row therefore stays NULL, which reads as *nobody has touched this frame since + the column existed* rather than as *nobody ever has*, and the summary's + ranking is written to say exactly that: an untouched batch is ordered by the + progress rule that came before, behind every batch that has a stamp. + + Migration 5's posture on ``annotation_schema.provenance``, for the same + reason and with the same consequence — a NULL that converges to a real value + as soon as somebody uses the workspace, and never lies in the meantime. + """ + _add_column(connection, "annotation_job_asset", "touched_at") + + MIGRATIONS: list[Migration] = [ Migration(version=1, name="baseline_schema", upgrade=_create_baseline_schema), Migration(version=2, name="batch_lineage", upgrade=_add_batch_lineage), @@ -256,6 +275,7 @@ def _add_model_family(connection: Connection) -> None: Migration(version=5, name="schema_provenance", upgrade=_add_schema_provenance), Migration(version=6, name="inference_connections", upgrade=_add_inference_connections), Migration(version=7, name="model_family", upgrade=_add_model_family), + Migration(version=8, name="progress_touched", upgrade=_add_progress_touched), ] FORMAT_VERSION: int = MIGRATIONS[-1].version diff --git a/src/visionset/kernel/adapters/sqlite_metadata_store.py b/src/visionset/kernel/adapters/sqlite_metadata_store.py index e4820b2..9e2de66 100644 --- a/src/visionset/kernel/adapters/sqlite_metadata_store.py +++ b/src/visionset/kernel/adapters/sqlite_metadata_store.py @@ -351,6 +351,7 @@ def set_asset_progress( *, expected: AssetProgress, progress: AssetProgress, + touched_at: datetime, ) -> AssetProgress | None: """One guarded ``UPDATE`` — see the port's docstring for why it exists. @@ -367,6 +368,10 @@ def set_asset_progress( rather than to decide whether the write happened. That second read runs inside this transaction, so a third writer cannot change what it reports before this one commits. + + ``touched_at`` rides in the same ``values``, so it is stamped under the + same guard: a write that loses the race writes neither column, and there + is no ordering in which the timestamp records work that did not happen. """ # `Session.execute` is typed as returning `Result`, which has no # `rowcount`; a DML statement always yields the `CursorResult` that does. @@ -377,7 +382,7 @@ def set_asset_progress( .where(t.AnnotationJobAssetRow.job_id == job_id) .where(t.AnnotationJobAssetRow.asset_id == asset_id) .where(t.AnnotationJobAssetRow.progress == expected) - .values(progress=progress) + .values(progress=progress, touched_at=touched_at.isoformat()) ), ) if result.rowcount == 1: @@ -392,6 +397,26 @@ def set_asset_progress( raise EntityNotFound(f"job {job_id} does not carry asset {asset_id}") return AssetProgress(stored) + def last_touched(self, job_id: UUID) -> datetime | None: + """One ``max()`` over the job's rows — see the port's docstring. + + The aggregate runs on the stored **strings**, and that is sound rather + than lucky: every timestamp in this schema is written by + ``datetime.isoformat()`` at a fixed UTC offset, and ISO-8601 in that form + orders lexicographically exactly as it orders chronologically. A column + holding two different offsets would not, which is one more reason the + write side stamps in one place. + + ``max()`` over no rows, or over rows that are all NULL, is NULL — so a + job whose assets nobody has touched needs no separate branch here. + """ + stamped = self._session.scalar( + select(func.max(t.AnnotationJobAssetRow.touched_at)).where( + t.AnnotationJobAssetRow.job_id == job_id + ) + ) + return None if stamped is None else datetime.fromisoformat(stamped) + def add_batch_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> list[UUID]: """One ``INSERT ... SELECT`` per asset — see the port's docstring for why. diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index cede098..a815dba 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -196,6 +196,7 @@ AttentionItem, AttentionKind, ProjectSummary, + ResumeKind, ResumeTarget, WorkspaceSummary, WorkspaceTotals, @@ -378,6 +379,7 @@ "AttentionItem", "AttentionKind", "ProjectSummary", + "ResumeKind", "ResumeTarget", "WorkspaceSummary", "WorkspaceTotals", diff --git a/src/visionset/kernel/domain/summary.py b/src/visionset/kernel/domain/summary.py index eeeb278..a83f9d3 100644 --- a/src/visionset/kernel/domain/summary.py +++ b/src/visionset/kernel/domain/summary.py @@ -11,12 +11,14 @@ train on"; neither can answer "which of my projects is waiting on me", because neither is allowed to look at more than one. -**No timestamp here dates a person's work, and that is a fact about the storage -format rather than an omission.** There is no timestamp column on ``batch``, on -``annotation``, or on ``annotation_job_asset`` — nothing records when a batch was -created, when it changed state, when a label was drawn, or when an asset's -progress last moved. Every consequence that follows in this module, chiefly -:class:`ResumeTarget`'s ranking, comes from that one absence. +**One timestamp here dates a person's work, and it is the only one.** +``annotation_job_asset.touched_at`` records when somebody last moved a frame's +progress, and :class:`ResumeTarget` is ranked on it. Nothing else in the storage +format does: a ``batch`` records neither when it was created nor when it changed +state, and an ``annotation`` records nothing at all. So a batch nobody has worked +still has no age, which is what the ranking's second population is about, and the +activity feed is still derived from timestamps that were put there for other +reasons. """ from __future__ import annotations @@ -45,6 +47,29 @@ class AttentionKind(StrEnum): JOB_RUNNING = "job_running" +# This docstring ships verbatim into `openapi.json`, so it says what a client +# needs and no more. The reasoning it used to carry belongs here instead. +# +# **The order is a decision this module owns, not a fact the response restates.** +# Labeling first, then review, then a batch that needs neither, is a judgment +# about what somebody should do next — and a judgment spelled once in Python and +# again in whatever renders it is one that drifts. That is what distinguishes it +# from the summary's first-run state, which is deliberately *not* a field: +# "this workspace has no projects" is a count the response already carries, so a +# flag beside it would be a second spelling of the same number. +class ResumeKind(StrEnum): + """What an open batch is being offered for, and so what `next_asset_id` is. + + `annotate` - a frame nobody has labeled, which is that frame. + `review` - every frame is labeled or set aside and some await a reviewer, + which is the first of those. `open` - neither, and `next_asset_id` is null. + """ + + ANNOTATE = "annotate" + REVIEW = "review" + OPEN = "open" + + class ActivityKind(StrEnum): """What sort of thing a row of the activity feed is. @@ -76,33 +101,35 @@ class WorkspaceTotals(BaseModel): class ResumeTarget(BaseModel): - """The batch to carry on with, and where inside it to land. - - **Ranked by progress, not by recency, and the reason is structural.** No - timestamp exists anywhere on a batch, an annotation, or an asset's progress - row, so "the batch I touched last" has no source in the schema and deriving - one would mean a migration. What the data *can* answer is which batch is - furthest along and not yet finished — the batch you are part-way through — - and that is what this is. - - The substitution is acceptable because of two properties. It degrades - correctly: a workspace with one open batch is offered that batch whatever its - progress. And it claims only what is true by construction, where a recency - ordering would have claimed something the rows cannot support. - - The accepted limit, stated so nobody reports it as a defect: two batches - part-way through resolve to the further-along one rather than to the one - somebody touched most recently. - - ``next_asset_id`` is NULL when nothing in the batch is ``unannotated`` any - more — every frame is settled or waiting on review. That is not an error and - not an empty resume: the batch is still the one to open, so the caller sends - somebody to its gallery instead of into the editor. A surface rendering this - changes its own label accordingly. + """The batch to carry on with, where inside it to land, and what for. + + **Ranked by recency, and by progress only where recency has nothing to say.** + ``annotation_job_asset.touched_at`` is stamped whenever somebody moves a + frame, so a batch that has been worked outranks every batch that has not, and + the latest touch wins among those that have. Batches nobody has touched are + ranked among themselves the way the whole card used to be ranked: furthest + through first, ties to the later-created batch, which is the closest thing to + recency insertion order can offer. + + That second population is not a leftover. ``touched_at`` was added rather + than backfilled, because the moments it holds were never recorded anywhere — + so every row in a workspace that predates it is NULL, and the fallback is + what makes such a workspace behave exactly as it did before while converging + to real recency as soon as anybody uses it. + + ``kind`` decides the rest, and it is the field to read first. It says whether + ``next_asset_id`` is a frame to label, a frame to review, or absent — see + :class:`ResumeKind`. A batch is offered as ``open`` when it is settled + throughout: not an error and not an empty resume, just a batch worth opening + with no frame to open it at. """ model_config = ConfigDict(frozen=True, extra="forbid") + #: What this batch is being offered for, and therefore what a surface should + #: promise on its control. Resolved here rather than by whoever renders it, + #: because the order between the three is a decision rather than a fact. + kind: ResumeKind project_id: UUID project_name: str batch_id: UUID @@ -115,13 +142,20 @@ class ResumeTarget(BaseModel): #: ``in_annotation`` has at least one job. A batch with none is not offered #: at all rather than offered with nothing to open. job_id: UUID - #: The first ``unannotated`` asset in batch order, or NULL — see above. + #: Where to land, in batch order: the first ``unannotated`` frame under + #: ``annotate``, the first ``review_pending`` one under ``review``, NULL + #: under ``open``. Which of those it is comes off ``kind``. next_asset_id: UUID | None = None #: Settled assets, i.e. those not blocking the job from completing. Counted #: against ``SETTLED_PROGRESS`` rather than against ``annotated`` alone, so a #: skipped frame reads as dealt with rather than as outstanding. annotated: int = Field(ge=0) total: int = Field(ge=0) + #: Frames in this batch waiting on a reviewer. Always populated, not only + #: under ``review``: a batch can hold frames for review and unlabeled frames + #: at once, and a surface showing the count only in the state where it is the + #: headline would hide the more interesting case. + review_pending: int = Field(ge=0) #: A frame to show beside the card, or NULL when the batch holds none that #: records a cached preview. The caller reaches the bytes by asset id, the #: way every other thumbnail in the product is addressed. diff --git a/src/visionset/kernel/ports/metadata_store.py b/src/visionset/kernel/ports/metadata_store.py index 09bd884..4e023da 100644 --- a/src/visionset/kernel/ports/metadata_store.py +++ b/src/visionset/kernel/ports/metadata_store.py @@ -200,6 +200,7 @@ def set_asset_progress( *, expected: AssetProgress, progress: AssetProgress, + touched_at: datetime, ) -> AssetProgress | None: """Move one asset's progress, and only if it is still where it was read. @@ -224,11 +225,42 @@ def set_asset_progress( the write was refused and where the asset actually is. A caller finding its own target there has nothing left to do. + ``touched_at`` is stamped on the row **in the same statement**, so the + record of when somebody worked this frame is as atomic as the move it + records and there is no window in which one landed without the other. It + is passed in rather than read here, on ``claim_job``'s terms: an adapter + that reaches for a clock is one a test cannot place in time. + + A refused write stamps nothing, which is the behaviour worth having: + losing a race is not work, and a caller whose move was rejected did not + touch the frame. + Raises ``EntityNotFound`` if the job does not carry that asset at all, matching ``Repository.update`` on an id that is not stored. """ ... + def last_touched(self, job_id: UUID) -> datetime | None: + """When somebody last moved any of this job's assets, or NULL if nobody has. + + The read side of :meth:`set_asset_progress`'s stamp, and a named + aggregate rather than a repository scan for ``annotation_totals``' + reason: ``Repository[AnnotationJob]`` answers whole jobs, and a caller + ranking every open batch in a workspace by recency wants one number per + job rather than every asset's progress map. + + NULL means the whole job predates the column or nobody has worked it + since — deliberately one answer rather than two, because a caller ranking + by recency treats both the same way and telling them apart would need a + second timestamp nobody records. + + A caller ranking many jobs pays one query each. That is the same N+1 the + workspace summary already accepts elsewhere, and if it starts to cost the + fix is a form of this method taking several job ids — never a SQLAlchemy + import in a service. + """ + ... + def add_batch_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> list[UUID]: """Append assets to a batch's membership, skipping any it already holds. diff --git a/src/visionset/kernel/services/annotation_service.py b/src/visionset/kernel/services/annotation_service.py index b353e6d..3809670 100644 --- a/src/visionset/kernel/services/annotation_service.py +++ b/src/visionset/kernel/services/annotation_service.py @@ -56,6 +56,7 @@ class is not in the version, whose geometry is not the one that class declares, from collections.abc import Iterable, Iterator, Sequence from contextlib import contextmanager +from datetime import UTC, datetime from uuid import UUID from visionset.kernel.domain import ( @@ -555,14 +556,29 @@ def _refresh_progress(uow: UnitOfWork, job: AnnotationJob, asset_ids: Iterable[U the whole call, and that is the right outcome rather than a harsh one: this service is all-or-nothing, so the labels roll back with it, and a caller that reads again derives its progress from a state that is actually there. + + **The write happens even when the progress does not move**, which is the one + place this function does something for a reason other than progress. Most + labeling leaves an asset exactly where it was — the second box on a frame + that was already ``annotated`` — and that is still somebody working in this + batch. Writing the value it already holds is what stamps ``touched_at``, and + it costs nothing else: the guard is ``progress = current``, which is + satisfied by construction unless somebody moved the asset underneath this + call — in which case the refusal below is exactly as welcome as it is for a + move that changes something. + + One timestamp for the whole call rather than one per asset, because a caller + that labeled six frames in one request did that at one moment. """ + touched_at = datetime.now(UTC) for asset_id in dict.fromkeys(asset_ids): remaining = uow.annotations.list(asset_id) current = job.progress[asset_id] - target = progress_after_annotating(current, has_annotations=bool(remaining)) - if target is None: - continue - stored = uow.set_asset_progress(job.id, asset_id, expected=current, progress=target) + moved = progress_after_annotating(current, has_annotations=bool(remaining)) + target = current if moved is None else moved + stored = uow.set_asset_progress( + job.id, asset_id, expected=current, progress=target, touched_at=touched_at + ) if stored is not None and stored is not target: raise StaleWrite( f"asset {asset_id} in job {job.id} was {current.value!r} when these labels were " diff --git a/src/visionset/kernel/services/job_service.py b/src/visionset/kernel/services/job_service.py index c869331..5ededc1 100644 --- a/src/visionset/kernel/services/job_service.py +++ b/src/visionset/kernel/services/job_service.py @@ -34,6 +34,7 @@ from __future__ import annotations +from datetime import UTC, datetime from uuid import UUID from visionset.kernel.domain import ( @@ -223,7 +224,10 @@ def mark(self, job_id: UUID, asset_id: UUID, progress: AssetProgress) -> Annotat Marking a state the asset is already in is a **no-op**, not a refusal: progress is a marker driven by what annotators do, and re-stating it is not a move. That is deliberately unlike ``BatchService.approve``, where a - second call would re-partition the batch. + second call would re-partition the batch. It is also why such a call + leaves ``touched_at`` alone: it returns above the write, and a client + re-sending a value the asset already holds is a retry rather than + somebody working the frame. One method rather than five intent-named ones, because ``ASSET_PROGRESS_TRANSITIONS`` is the whole of what is legal and a second @@ -262,7 +266,13 @@ def mark(self, job_id: UUID, asset_id: UUID, progress: AssetProgress) -> Annotat # job and would put back every *other* asset as it was read. A # returning value means the guard failed, i.e. somebody moved this # asset while the lines above were deciding. - stored = uow.set_asset_progress(job.id, asset_id, expected=current, progress=progress) + stored = uow.set_asset_progress( + job.id, + asset_id, + expected=current, + progress=progress, + touched_at=datetime.now(UTC), + ) if stored is not None: if stored is progress: # Somebody else made the same move first. The caller's intent diff --git a/src/visionset/kernel/services/summary_service.py b/src/visionset/kernel/services/summary_service.py index 72683c0..0b3317a 100644 --- a/src/visionset/kernel/services/summary_service.py +++ b/src/visionset/kernel/services/summary_service.py @@ -20,10 +20,13 @@ module-level helper the owning service exposes: ``jobs_of`` is ``BatchService``'s, borrowed rather than rewritten, the way ``JobService`` borrows it. -- **Nothing here is ordered by a clock this build controls.** Every ordering - decision below follows from one fact about the storage format — there is no - timestamp on ``batch``, on ``annotation``, or on ``annotation_job_asset``. The - models in ``domain/summary.py`` state it where a reader of them will find it. +- **One ordering here is a clock and the rest are not.** The resume card is + ranked on ``annotation_job_asset.touched_at``, the schema's only record of when + somebody worked; a ``batch`` still dates neither its creation nor its state + changes, and an ``annotation`` dates nothing at all, so every other ordering + below follows from a timestamp that was stored for a different purpose or from + insertion order. The models in ``domain/summary.py`` state which is which where + a reader of them will find it. Composition follows ``docs/workspaces.md``: this service takes an open :class:`WorkspaceService` and nothing else, and never names an adapter. @@ -33,6 +36,7 @@ from dataclasses import dataclass from datetime import datetime +from typing import Final from uuid import UUID from visionset.kernel.domain import ( @@ -50,6 +54,7 @@ DatasetOperation, Project, ProjectSummary, + ResumeKind, ResumeTarget, WorkspaceSummary, WorkspaceTotals, @@ -128,7 +133,7 @@ def summary(self) -> WorkspaceSummary: count=waiting, ) ) - candidate = _candidate(project, batch, jobs) + candidate = _candidate(project, batch, jobs, _touched(uow, jobs)) if candidate is not None: best = _preferred(best, candidate) @@ -164,14 +169,15 @@ def frozen(self, projects: int) -> WorkspaceTotals: class _Candidate: """One batch considered for the resume card, with what it is ranked on. - ``settled`` is the rank and ``has_work`` is the tier: a batch with labeling - left always beats one without, however far along the other is. Carrying both + Three things, consulted in that order by :func:`_preferred`: the ``kind`` on + the target is the tier, ``touched_at`` is the rank, and ``settled`` is the + tie-break among batches that have never been touched. Carrying all of them beside the finished :class:`ResumeTarget` is what lets :func:`_preferred` be a comparison rather than a re-derivation. """ + touched_at: datetime | None settled: int - has_work: bool target: ResumeTarget @@ -329,14 +335,34 @@ def _in_state(jobs: list[AnnotationJob], progress: AssetProgress) -> int: return sum(1 for job in jobs for value in job.progress.values() if value is progress) -def _candidate(project: Project, batch: Batch, jobs: list[AnnotationJob]) -> _Candidate | None: - """Rank one open batch, and work out where inside it to land. +def _touched(uow: UnitOfWork, jobs: list[AnnotationJob]) -> datetime | None: + """When this batch was last worked, across every job cut out of it. - The landing place is the first ``unannotated`` asset **in batch order**, and - batch order is ``Batch.asset_ids`` rather than any one job's own sequence: a - partition cuts a batch into several jobs, so no single job's ordering is the - batch's. The per-asset states are merged across the jobs first, which is the - same projection the batch asset listing builds for the same reason. + A partition splits a batch into several jobs and somebody works whichever + one holds the frame in front of them, so the batch's recency is the newest of + its jobs' — never any single job's. + """ + stamps = [stamp for job in jobs if (stamp := uow.last_touched(job.id)) is not None] + return max(stamps) if stamps else None + + +def _candidate( + project: Project, batch: Batch, jobs: list[AnnotationJob], touched_at: datetime | None +) -> _Candidate | None: + """Rank one open batch, work out what it is for, and where inside it to land. + + The landing place is a frame **in batch order**, and batch order is + ``Batch.asset_ids`` rather than any one job's own sequence: a partition cuts + a batch into several jobs, so no single job's ordering is the batch's. The + per-asset states are merged across the jobs first, which is the same + projection the batch asset listing builds for the same reason. + + Which frame depends on what the batch still needs, and the order of the two + searches *is* the priority: an unannotated frame if there is one, otherwise + the first waiting on a reviewer, otherwise nothing. Labeling comes first + because it is the work that cannot be done by anybody else later — a frame + nobody has drawn on blocks the batch outright, where one awaiting review is + already done and waiting on a second opinion. A batch with no jobs is not a candidate. Nothing can open it — the editor is keyed on a job — and the kernel already makes it unreachable by refusing to @@ -349,28 +375,28 @@ def _candidate(project: Project, batch: Batch, jobs: list[AnnotationJob]) -> _Ca asset_id: (job.id, value) for job in jobs for asset_id, value in job.progress.items() } settled = sum(1 for _, value in holders.values() if value in SETTLED_PROGRESS) - landing = next( - ( - (asset_id, holders[asset_id][0]) - for asset_id in batch.asset_ids - if holders.get(asset_id, (None, None))[1] is AssetProgress.UNANNOTATED - ), - None, - ) + waiting = sum(1 for _, value in holders.values() if value is AssetProgress.REVIEW_PENDING) + landing = _landing(batch, holders, AssetProgress.UNANNOTATED) + kind = ResumeKind.ANNOTATE + if landing is None: + landing = _landing(batch, holders, AssetProgress.REVIEW_PENDING) + kind = ResumeKind.OPEN if landing is None else ResumeKind.REVIEW return _Candidate( + touched_at=touched_at, settled=settled, - has_work=landing is not None, target=ResumeTarget( + kind=kind, project_id=project.id, project_name=project.name, batch_id=batch.id, batch_name=batch.name, - # The job holding the landing frame, or — with nothing left to - # label — the batch's first, so the gallery still has a way in. + # The job holding the landing frame, or — with nowhere to land — + # the batch's first, so the gallery still has a way in. job_id=landing[1] if landing else jobs[0].id, next_asset_id=landing[0] if landing else None, annotated=settled, total=len(holders), + review_pending=waiting, # The frame somebody is about to open, or failing that the batch's # first — a picture for the card, not a claim about progress. A # missing preview renders as a placeholder, which every thumbnail in @@ -382,23 +408,56 @@ def _candidate(project: Project, batch: Batch, jobs: list[AnnotationJob]) -> _Ca ) +def _landing( + batch: Batch, holders: dict[UUID, tuple[UUID, AssetProgress]], progress: AssetProgress +) -> tuple[UUID, UUID] | None: + """The first asset of this batch in one state, with the job that holds it.""" + return next( + ( + (asset_id, holders[asset_id][0]) + for asset_id in batch.asset_ids + if holders.get(asset_id, (None, None))[1] is progress + ), + None, + ) + + +#: The order the resume card offers its three kinds in, lowest first. A table +#: rather than a chain of comparisons, so the priority is one thing to read and +#: adding a fourth kind is one line rather than a rewrite of :func:`_preferred`. +_KIND_RANK: Final = {ResumeKind.ANNOTATE: 0, ResumeKind.REVIEW: 1, ResumeKind.OPEN: 2} + + def _preferred(best: _Candidate | None, other: _Candidate) -> _Candidate: """Which of two open batches the card should offer. - Two tiers, and the tiers are the point. A batch with labeling left always - beats one without, because "continue" means there is something to continue; - only when *nothing* in the workspace has an unannotated frame does the card - fall to the furthest-along batch, which it offers as somewhere to open rather - than as somewhere to type. - - Within a tier the rank is settled assets — the batch you are furthest through - — and a tie goes to the **later** one, since ``Repository.list`` answers in - insertion order and the most recently created batch is the closest thing to - recency the rows can offer. That is also why the comparison is ``>=``: a - strict ``>`` would keep the first of equals and quietly mean the opposite. + Three questions in order, and the order is the whole of it. + + **What the batch is for** comes first, by ``_KIND_RANK``: a batch with + labeling left beats one that only needs review, which beats one that needs + neither — however far ahead, and however recently touched, the other is. + "Continue" has to mean there is something to continue, and only when nothing + in the workspace has an unannotated frame does the card start offering review + instead. + + **Recency** decides within a kind. A batch somebody has worked beats one + nobody has, and the later touch wins between two that have been. This is what + ``annotation_job_asset.touched_at`` was added for. + + **Progress** decides between two batches that have *never* been touched — + every row NULL, which is every row in a workspace that predates the column. + Furthest through first, and a tie goes to the **later** batch, since + ``Repository.list`` answers in insertion order and the most recently created + batch is the closest thing to recency those rows can offer. That is why the + last comparison is ``>=``: a strict ``>`` would keep the first of equals and + quietly mean the opposite. """ if best is None: return other - if best.has_work != other.has_work: - return other if other.has_work else best + if _KIND_RANK[best.target.kind] != _KIND_RANK[other.target.kind]: + return other if _KIND_RANK[other.target.kind] < _KIND_RANK[best.target.kind] else best + if (best.touched_at is None) != (other.touched_at is None): + return other if best.touched_at is None else best + if best.touched_at is not None and other.touched_at is not None: + return other if other.touched_at > best.touched_at else best return other if other.settled >= best.settled else best diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index 611d99c..054f37c 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -108,6 +108,7 @@ ProjectSummary, Release, ReleaseVerification, + ResumeKind, ResumeTarget, SchemaChange, SchemaDiff, @@ -2029,24 +2030,33 @@ def of(cls, totals: WorkspaceTotals) -> Self: class ResumeTargetOut(BaseModel): - """The batch to carry on with, and where inside it to land.""" + """The batch to carry on with, what for, and where inside it to land.""" + # What the batch is being offered for. Resolved by the kernel because the + # order between the three is a decision rather than a fact the rest of this + # response restates, and a decision spelled twice is one that drifts. + kind: ResumeKind project_id: UUID project_name: str batch_id: UUID batch_name: str job_id: UUID - # NULL when nothing in the batch is unannotated. The batch is still the one - # to open; a client sends somebody to its gallery instead of into the editor - # and says so on the control, rather than offering a link to no frame. + # The frame to land on, whose meaning comes off `kind`: unlabeled under + # `annotate`, awaiting review under `review`, and NULL under `open` — where + # the batch is still the one to open, so a client sends somebody to its + # gallery and says so on the control rather than offering a link to no frame. next_asset_id: UUID | None annotated: int total: int + # Always populated, not only under `review`: a batch can hold frames for + # review and unlabeled frames at once. + review_pending: int thumbnail_asset_id: UUID | None @classmethod def of(cls, resume: ResumeTarget) -> Self: return cls( + kind=resume.kind, project_id=resume.project_id, project_name=resume.project_name, batch_id=resume.batch_id, @@ -2055,6 +2065,7 @@ def of(cls, resume: ResumeTarget) -> Self: next_asset_id=resume.next_asset_id, annotated=resume.annotated, total=resume.total, + review_pending=resume.review_pending, thumbnail_asset_id=resume.thumbnail_asset_id, ) diff --git a/src/visionset/server/routes/home.py b/src/visionset/server/routes/home.py index 7d9c562..22c7ea8 100644 --- a/src/visionset/server/routes/home.py +++ b/src/visionset/server/routes/home.py @@ -39,12 +39,18 @@ def get_home(workspace: WorkspaceDep) -> HomeOut: project list, not a copy of it, and `activity` is capped — both have a screen that owns them in full. - `resume` is the batch to carry on with: the one furthest through that still - has an unannotated frame. It is **derived on every call and never stored**, - and it is ranked by progress rather than by recency because nothing in the - workspace records when a batch was last worked on. When `next_asset_id` is - null the batch has no unlabeled frame left — open its gallery rather than the - editor. `resume` itself is null when no batch is open for annotation. + `resume` is the batch to carry on with, **derived on every call and never + stored**. Read its `kind` first: `annotate` means `next_asset_id` is a frame + nobody has labeled, `review` means it is one awaiting a reviewer, and `open` + means the batch is settled throughout and `next_asset_id` is null — open its + gallery rather than the editor. The three are in priority order, decided + here, and a client renders what it is told rather than working it out again. + `resume` itself is null when no batch is open for annotation. + + Batches are ranked by when somebody last worked them. Ones nobody has worked + since that became recordable rank last, ordered among themselves by how far + through they are — which is every batch in a workspace created before the + stamp existed, since it was added without a backfill. `attention` carries batches with frames awaiting review, and background jobs that failed or are still running. A job row has no `project_id`: a job names diff --git a/tests/kernel/test_job_service.py b/tests/kernel/test_job_service.py index d444ee7..f99eb9a 100644 --- a/tests/kernel/test_job_service.py +++ b/tests/kernel/test_job_service.py @@ -7,6 +7,7 @@ from __future__ import annotations from collections.abc import Callable +from datetime import UTC, datetime from io import BytesIO from pathlib import Path from uuid import UUID, uuid4 @@ -338,9 +339,17 @@ def test_a_stale_write_names_where_the_asset_actually_is(tmp_path: Path) -> None with fixture.workspace.unit_of_work() as uow: # What another writer's commit leaves behind, from this caller's side: # the row moved and nothing told it. - assert uow.set_asset_progress(job.id, asset, expected=UNANNOTATED, progress=SKIPPED) is None + now = datetime.now(UTC) assert ( - uow.set_asset_progress(job.id, asset, expected=UNANNOTATED, progress=ANNOTATED) + uow.set_asset_progress( + job.id, asset, expected=UNANNOTATED, progress=SKIPPED, touched_at=now + ) + is None + ) + assert ( + uow.set_asset_progress( + job.id, asset, expected=UNANNOTATED, progress=ANNOTATED, touched_at=now + ) is SKIPPED ) fixture.close() diff --git a/tests/kernel/test_migrations.py b/tests/kernel/test_migrations.py index ca8b484..662f02b 100644 --- a/tests/kernel/test_migrations.py +++ b/tests/kernel/test_migrations.py @@ -179,6 +179,7 @@ def _at_generation_one(path: Path) -> None: connection.execute(text("ALTER TABLE annotation DROP COLUMN job_id")) connection.execute(text("ALTER TABLE annotation_schema DROP COLUMN provenance")) connection.execute(text("ALTER TABLE inference_connection DROP COLUMN model_family")) + connection.execute(text("ALTER TABLE annotation_job_asset DROP COLUMN touched_at")) connection.execute(text(f"UPDATE {META_TABLE} SET format_version = 1")) store.close() @@ -243,6 +244,7 @@ def test_running_every_migration_again_changes_nothing(tmp_path: Path) -> None: # docstring below says this exists to catch. "batch": ["parent_batch_id"], "annotation": ["job_id"], + "annotation_job_asset": ["touched_at"], } diff --git a/tests/kernel/test_summary_service.py b/tests/kernel/test_summary_service.py index 3ae3121..4093a54 100644 --- a/tests/kernel/test_summary_service.py +++ b/tests/kernel/test_summary_service.py @@ -1,9 +1,10 @@ """SummaryService: the workspace read across every project at once. -The subject is the resume derivation. It is the one rule here that had to be -*chosen* rather than read off the rows — no timestamp exists on a batch, an -annotation or an asset's progress — so most of this file is the two-tier -comparison and the ways it can be got backwards. +The subject is the resume derivation, which is three rules stacked: what a batch +is being offered *for*, then when it was last worked, then — for batches nobody +has worked since ``annotation_job_asset.touched_at`` existed — how far through it +is. Most of this file is those three and the ways each can be got backwards, +because each is invisible to tests of the other two. Everything walks the real services. A planted batch state or a hand-written progress map would let the tests agree with a fixture rather than with the @@ -17,6 +18,8 @@ from pathlib import Path from uuid import UUID, uuid4 +from sqlalchemy import text + from visionset.kernel.domain import ( ActivityKind, Annotation, @@ -26,10 +29,13 @@ BackgroundJob, BackgroundJobState, BboxGeometry, + BySize, GeometryType, LabelClass, + ResumeKind, ) from visionset.kernel.services import ( + AnnotationService, BatchService, DatasetService, JobService, @@ -49,6 +55,7 @@ class Fixture: def __init__(self, tmp_path: Path) -> None: self.workspace = WorkspaceService.init(tmp_path / "ws") self.projects = ProjectService(self.workspace) + self.annotations = AnnotationService(self.workspace) self.batches = BatchService(self.workspace) self.jobs = JobService(self.workspace) self.schemas = SchemaService(self.workspace) @@ -87,10 +94,51 @@ def open_batch(self, project_id: UUID, name: str, assets: list[UUID]) -> tuple[U self.jobs.start(job.id) return batch.id, job.id + def split_batch(self, project_id: UUID, name: str, assets: list[UUID]) -> list[UUID]: + """A batch cut into one job per asset. Returns the job ids, in batch order.""" + batch = self.batches.create(project_id, name, assets) + self.batches.approve(batch.id, BySize(size=1)) + self.batches.start(batch.id) + jobs = self.batches.jobs(batch.id) + # The caller indexes these against `assets`, so the pairing is asserted + # here rather than assumed from the partition's own ordering. + assert [next(iter(job.progress)) for job in jobs] == assets + for job in jobs: + self.jobs.start(job.id) + return [job.id for job in jobs] + def annotate(self, job_id: UUID, asset_ids: list[UUID]) -> None: for asset_id in asset_ids: self.jobs.mark(job_id, asset_id, AssetProgress.ANNOTATED) + def forget_touches(self) -> None: + """Make every frame look like one nobody has worked since the column existed. + + The only way to reach the ranking's second population, and it is not a + contrivance: it is exactly the state of a workspace created before + migration 8, where ``touched_at`` was added and deliberately not + backfilled. Raw SQL rather than a service call, because no service can + put a workspace back into that state and none should be able to. + """ + store = self.workspace.metadata_store + with store.engine.begin() as connection: # type: ignore[attr-defined] + connection.execute(text("update annotation_job_asset set touched_at = null")) + + def touched(self, job_id: UUID, asset_id: UUID) -> None: + """Work a frame without changing where it is — the ordinary annotation edit.""" + self.annotations.add( + job_id, + [ + Annotation( + asset_id=asset_id, + label_class="sign", + geometry=BboxGeometry(x=1.0, y=1.0, width=2.0, height=2.0), + schema_version=1, + provenance="human", + ) + ], + ) + def summary(self): # noqa: ANN201 - the domain model, named at each call site return SummaryService(self.workspace).summary() @@ -289,27 +337,92 @@ def test_a_skipped_frame_counts_as_dealt_with(tmp_path: Path) -> None: fixture.close() -def test_the_batch_you_are_furthest_through_wins(tmp_path: Path) -> None: - """The rank, in the tier where both batches still have labeling left.""" +def test_the_batch_you_worked_last_wins(tmp_path: Path) -> None: + """The rank, and the whole reason ``touched_at`` exists. + + The further-along batch is worked first and then left; the one behind it is + worked afterwards. Under the ordering this replaced, the further-along one + would win — which is the substitution the column removes. + """ fixture = Fixture(tmp_path) try: project = fixture.project("p") - behind = fixture.assets(project, 5) ahead = fixture.assets(project, 5) - fixture.open_batch(project, "behind", behind) - further, ahead_job = fixture.open_batch(project, "ahead", ahead) + behind = fixture.assets(project, 5) + _, ahead_job = fixture.open_batch(project, "ahead", ahead) fixture.annotate(ahead_job, ahead[:3]) + recent, behind_job = fixture.open_batch(project, "behind", behind) + fixture.annotate(behind_job, behind[:1]) resume = fixture.summary().resume assert resume is not None - assert resume.batch_id == further - assert resume.batch_name == "ahead" + assert resume.batch_id == recent + assert resume.batch_name == "behind" + finally: + fixture.close() + + +def test_a_split_batch_is_as_recent_as_its_newest_job(tmp_path: Path) -> None: + """A partition cuts a batch into several jobs, and somebody works one at a time. + + The batch's recency is the newest of its jobs', never any single job's — and + a batch with only one job, which is every other fixture in this file, cannot + tell the two apart. + """ + fixture = Fixture(tmp_path) + try: + project = fixture.project("p") + # Three assets, so one is left unlabeled and both batches below stay in + # the same kind — otherwise the priority decides this before recency is + # ever consulted, and the test would pass without exercising anything. + split = fixture.assets(project, 3) + other = fixture.assets(project, 2) + jobs = fixture.split_batch(project, "split", split) + fixture.annotate(jobs[0], split[:1]) + + _, plain_job = fixture.open_batch(project, "plain", other) + fixture.annotate(plain_job, other[:1]) + + # Back to the split batch, in a *different* job. Taking the oldest of its + # jobs would leave the plain batch looking like the more recent one. + fixture.annotate(jobs[1], split[1:2]) + + resume = fixture.summary().resume + + assert resume is not None + assert resume.batch_name == "split" finally: fixture.close() -def test_the_further_along_batch_wins_across_projects_too(tmp_path: Path) -> None: +def test_a_batch_somebody_has_worked_beats_one_nobody_has(tmp_path: Path) -> None: + """The two populations, and which way round they go. + + The untouched batch is *further through*, so under the fallback ranking alone + it would win. A stamp outranks no stamp whatever the progress says. + """ + fixture = Fixture(tmp_path) + try: + project = fixture.project("p") + stale = fixture.assets(project, 5) + fresh = fixture.assets(project, 5) + _, stale_job = fixture.open_batch(project, "further-through", stale) + fixture.annotate(stale_job, stale[:4]) + fixture.forget_touches() + + recent, fresh_job = fixture.open_batch(project, "barely-begun", fresh) + fixture.annotate(fresh_job, fresh[:1]) + + resume = fixture.summary().resume + + assert resume is not None + assert resume.batch_id == recent + finally: + fixture.close() + + +def test_the_batch_you_worked_last_wins_across_projects_too(tmp_path: Path) -> None: """The walk is workspace-wide, so the comparison must survive the project loop.""" fixture = Fixture(tmp_path) try: @@ -317,9 +430,10 @@ def test_the_further_along_batch_wins_across_projects_too(tmp_path: Path) -> Non second = fixture.project("second") one = fixture.assets(first, 5) two = fixture.assets(second, 5) + _, second_job = fixture.open_batch(second, "in-second", two) + fixture.annotate(second_job, two[:4]) _, first_job = fixture.open_batch(first, "in-first", one) - fixture.annotate(first_job, one[:4]) - fixture.open_batch(second, "in-second", two) + fixture.annotate(first_job, one[:1]) resume = fixture.summary().resume @@ -330,6 +444,66 @@ def test_the_further_along_batch_wins_across_projects_too(tmp_path: Path) -> Non fixture.close() +def test_an_edit_that_moves_no_progress_still_counts_as_working_the_batch( + tmp_path: Path, +) -> None: + """Drawing a second box on a labeled frame leaves progress alone and is still work. + + The case a stamp written only on a *transition* would miss, and the one an + annotator spends most of their time in: the frame was already ``annotated``, + so nothing about it changes except that somebody was there. + """ + fixture = Fixture(tmp_path) + try: + project = fixture.project("p") + one = fixture.assets(project, 4) + two = fixture.assets(project, 4) + first, first_job = fixture.open_batch(project, "first", one) + fixture.annotate(first_job, one[:2]) + _, second_job = fixture.open_batch(project, "second", two) + fixture.annotate(second_job, two[:2]) + before = fixture.summary().resume + assert before is not None + assert before.batch_name == "second" + + fixture.touched(first_job, one[0]) + + resume = fixture.summary().resume + + assert resume is not None + assert resume.batch_id == first + finally: + fixture.close() + + +def test_the_batch_you_are_furthest_through_wins_where_nobody_has_been( + tmp_path: Path, +) -> None: + """The fallback rank, reachable only in a workspace that predates the column. + + Every stamp is cleared, which is what such a workspace looks like: the + ordering falls back to progress, and the further-along batch wins as it did + before ``touched_at`` existed. + """ + fixture = Fixture(tmp_path) + try: + project = fixture.project("p") + behind = fixture.assets(project, 5) + ahead = fixture.assets(project, 5) + fixture.open_batch(project, "behind", behind) + further, ahead_job = fixture.open_batch(project, "ahead", ahead) + fixture.annotate(ahead_job, ahead[:3]) + fixture.forget_touches() + + resume = fixture.summary().resume + + assert resume is not None + assert resume.batch_id == further + assert resume.batch_name == "ahead" + finally: + fixture.close() + + def test_a_batch_with_labeling_left_beats_a_finished_one_however_far_ahead( tmp_path: Path, ) -> None: @@ -355,7 +529,7 @@ def test_a_batch_with_labeling_left_beats_a_finished_one_however_far_ahead( def test_a_batch_with_nothing_left_is_still_offered_as_somewhere_to_open( tmp_path: Path, ) -> None: - """The fallback tier: no unannotated frame anywhere, so the card opens a gallery.""" + """The last kind: nothing to label, nothing to review, so the card opens a gallery.""" fixture = Fixture(tmp_path) try: project = fixture.project("p") @@ -366,6 +540,7 @@ def test_a_batch_with_nothing_left_is_still_offered_as_somewhere_to_open( resume = fixture.summary().resume assert resume is not None + assert resume.kind is ResumeKind.OPEN assert resume.batch_id == batch assert resume.next_asset_id is None assert (resume.annotated, resume.total) == (3, 3) @@ -373,27 +548,103 @@ def test_a_batch_with_nothing_left_is_still_offered_as_somewhere_to_open( fixture.close() -def test_a_frame_waiting_on_review_is_not_something_to_carry_on_with( - tmp_path: Path, -) -> None: - """``review_pending`` is neither settled nor unannotated, and blocks the tier.""" +def test_a_frame_waiting_on_review_is_something_to_carry_on_with(tmp_path: Path) -> None: + """``review_pending`` is neither settled nor unannotated, and it is the second kind.""" fixture = Fixture(tmp_path) try: project = fixture.project("p") assets = fixture.assets(project, 2) _, job = fixture.open_batch(project, "b", assets) fixture.annotate(job, assets) - fixture.jobs.mark(job, assets[0], AssetProgress.REVIEW_PENDING) + fixture.jobs.mark(job, assets[1], AssetProgress.REVIEW_PENDING) resume = fixture.summary().resume assert resume is not None - assert resume.next_asset_id is None + assert resume.kind is ResumeKind.REVIEW + # The frame awaiting review, not the batch's first — the search that + # found it runs in batch order over one state, like the labeling one. + assert resume.next_asset_id == assets[1] + assert resume.review_pending == 1 assert resume.annotated == 1 finally: fixture.close() +def test_labeling_outranks_review_in_the_same_batch(tmp_path: Path) -> None: + """Priority 1 beats priority 2 where a batch offers both, which is the ordinary case. + + A reviewer sends one frame back while others have never been drawn on. The + card must not send somebody to review while labeling is outstanding: an + unlabeled frame blocks the batch outright, where one awaiting review is + already done and waiting on a second opinion. + """ + fixture = Fixture(tmp_path) + try: + project = fixture.project("p") + assets = fixture.assets(project, 3) + _, job = fixture.open_batch(project, "b", assets) + fixture.annotate(job, assets[:1]) + fixture.jobs.mark(job, assets[0], AssetProgress.REVIEW_PENDING) + + resume = fixture.summary().resume + + assert resume is not None + assert resume.kind is ResumeKind.ANNOTATE + assert resume.next_asset_id == assets[1] + # Reported all the same, so a surface can say what else is waiting. + assert resume.review_pending == 1 + finally: + fixture.close() + + +def test_labeling_in_one_batch_outranks_review_in_a_more_recent_one(tmp_path: Path) -> None: + """The kind is consulted before recency, which is what makes it a tier at all.""" + fixture = Fixture(tmp_path) + try: + project = fixture.project("p") + stale = fixture.assets(project, 2) + fresh = fixture.assets(project, 2) + older, older_job = fixture.open_batch(project, "older", stale) + fixture.annotate(older_job, stale[:1]) + + _, newer_job = fixture.open_batch(project, "newer", fresh) + fixture.annotate(newer_job, fresh) + fixture.jobs.mark(newer_job, fresh[0], AssetProgress.REVIEW_PENDING) + + resume = fixture.summary().resume + + assert resume is not None + assert resume.kind is ResumeKind.ANNOTATE + assert resume.batch_id == older + finally: + fixture.close() + + +def test_review_outranks_a_batch_with_neither(tmp_path: Path) -> None: + """The second rung, which without a test would be indistinguishable from the third.""" + fixture = Fixture(tmp_path) + try: + project = fixture.project("p") + waiting = fixture.assets(project, 2) + settled = fixture.assets(project, 2) + review, review_job = fixture.open_batch(project, "in-review", waiting) + fixture.annotate(review_job, waiting) + fixture.jobs.mark(review_job, waiting[0], AssetProgress.REVIEW_PENDING) + + # Later, and further through, and still not what the card offers. + _, done_job = fixture.open_batch(project, "done", settled) + fixture.annotate(done_job, settled) + + resume = fixture.summary().resume + + assert resume is not None + assert resume.kind is ResumeKind.REVIEW + assert resume.batch_id == review + finally: + fixture.close() + + def test_a_tie_goes_to_the_batch_created_later(tmp_path: Path) -> None: """Insertion order is the only recency the rows can offer, so it breaks ties.""" fixture = Fixture(tmp_path)