From 18d81c9d96710fffa382052095ca7ca01ff2d49a Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Fri, 23 Oct 2026 13:03:01 -0700 Subject: [PATCH] fix(review): never queue an ambiguous file with nothing to choose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The maintainer's live queue held 39 items whose candidates all read "unresolved" ("No positive / No negative"). An ambiguous result is now reviewable only when at least two candidates are resolved with non-blank positive text. With none the file could not be traced; with exactly one, the untraced branches may still have made the file, so that text is not certain either. Both are reported unresolved, never linked and never queued (also the A1111 two-delimiter case, which has no candidates). A rescan removes the pending items of such files whose own evidence offers no choice, with the same safe delete as the linked settle (pending, revision 0, no decision row). A limited item (over 16 candidates) that holds at least two candidates is kept. The job scope gains review_untraced and the log "= N images left Needs review · nothing to choose". --- TESTING.md | 26 ++- database/review.py | 43 +++++ database/review_schema.py | 38 ++++ py/api/admin.py | 40 +++- py/api/review_recording.py | 68 ++++++- tests/test_api_admin.py | 4 +- tests/test_api_review.py | 19 +- tests/test_pair_scan.py | 6 +- tests/test_review.py | 21 ++- tests/test_review_choosable.py | 335 +++++++++++++++++++++++++++++++++ tests/test_review_outcome.py | 10 +- utils/review_capture.py | 52 ++++- 12 files changed, 607 insertions(+), 55 deletions(-) create mode 100644 tests/test_review_choosable.py diff --git a/TESTING.md b/TESTING.md index 72d83e7..36e2b5e 100644 --- a/TESTING.md +++ b/TESTING.md @@ -2308,15 +2308,26 @@ validation, annotation group included. The browser fixtures clear never the metadata row. Needs review items (#249, `utils/review_capture.py`, `database/review.py`, -`tests/test_review.py`, `tests/test_review_outcome.py`): a file is queued only -when its final outcome is ambiguous and unlinked. The output scan (Activity's +`tests/test_review.py`, `tests/test_review_outcome.py`, +`tests/test_review_choosable.py`): a file is queued only when its final outcome +is ambiguous and unlinked and there is something to choose. Only scans produce +items; the monitor links new outputs and clears pending items it resolves +(maintainer rulings, 2026-10-23). The output scan (Activity's Scan for metadata and the legacy `/scan`) hands `record_ambiguous_file` a file the ingestion resolver finds `ambiguous` only when the browser-parser fallback then links nothing (status `ambiguous`) and no saved row links the file already; an ambiguous file a saved row links is reported `unchanged` ("Already linked to its prompt"), not Needs review. A file the fallback links, an exact pair the ingester stores, and `conflict` and `unresolved` files are not -queued. The queue heals itself: after each batch, the scan removes the pending +queued. Neither is an ambiguous file with nothing to choose: fewer than two of +its candidates are `resolved` with non-blank positive text (`reviewable`). With +none the file could not be traced; with exactly one, the untraced branches may +still have made the file, so that text is not certain either. Both report +`unresolved` ("could not be traced"), are never linked, and their pending, +undecided items whose own evidence offers no choice (a limited item counts as +a choice when it holds at least two candidates) leave the queue on the next +scan or monitor pass (`settle_untraced_review_items`). The maintainer's +second live check found 39 items whose 11 candidates were all "unresolved". The queue heals itself: after each batch, the scan removes the pending items of every listed file (read or skipped as known) that a saved row now links, and the monitor does the same for each ambiguous output it links (`settle_linked_review_items`; the review index revision moves). Only an @@ -2333,20 +2344,21 @@ fenced by the capture's storage epoch. Evidence is the resolver reason plus one `{sampler_id, state, reason, positive, negative, hash}` per selectable candidate (nested ambiguous stages flattened, duplicates dropped; each side `{text, storage_text, provenance}`; `hash` = SHA-256 of the canonical candidate -without it). No candidate, more than 16, or evidence over 8 MiB is a `limited` +without it). More than 16 candidates, or evidence over 8 MiB, is a `limited` item without candidates. Recording is idempotent per `(byte_id, resolver_version, evidence_hash)` and never touches an existing item or its decisions; changed bytes add a new pending item and keep the old one. The capture is released before the write; no model is loaded and no pixel is decoded. Scan statuses, counters, outcome rows and the legacy SSE events are -unchanged apart from the already-linked case above; the job's `scope` gains +unchanged apart from the already-linked and nothing-to-choose cases above; the job's `scope` gains `review_items` (this scan's files that are in the pending queue after it, new or already there: the review group's ambiguous rows, less files a capture could not read or a decision already settled), `review_added` (new items), -`review_settled` (items removed because their file is linked now) and +`review_settled` (items removed because their file is linked now), +`review_untraced` (items removed because they offer nothing to choose) and `review_errors`. The log ends with `? N images added to Needs review`, `? N images already in Needs review`, `= N images left Needs review · now -linked` and `! N images could not be added to Needs review` (failures, e.g. +linked`, `= N images left Needs review · nothing to choose` and `! N images could not be added to Needs review` (failures, e.g. capture capacity), each only when N > 0. Recording captures in the background: it never takes the store's last worker or capture slot, which stay free for an interactive Tag dialog or Apply, and waits up to 2 s for room before counting the file as not added diff --git a/database/review.py b/database/review.py index 65af8a2..40780e8 100644 --- a/database/review.py +++ b/database/review.py @@ -39,6 +39,7 @@ from .review_schema import ( METADATA_TABLE, canonical_evidence, evidence_digest, + evidence_offers_choice, ) # Only this module's table constants are interpolated into the SQL below. @@ -95,6 +96,13 @@ _PENDING_OF_LOCATOR = ( f"JOIN {ITEMS_TABLE} i ON i.byte_id=b.id " "WHERE f.locator=? AND i.state='pending'" ) +# Pending items of one file with their evidence: an untraced file's settle. +_PENDING_EVIDENCE_OF_LOCATOR = ( + f"SELECT i.id,i.evidence FROM {FILES_TABLE} f " # nosec B608 + f"JOIN {BYTES_TABLE} b ON b.file_id=f.id " + f"JOIN {ITEMS_TABLE} i ON i.byte_id=b.id " + "WHERE f.locator=? AND i.state='pending'" +) # Only an undecided item goes: a decision row (RESTRICT) is never deleted. _SETTLE_ITEM = ( f"DELETE FROM {ITEMS_TABLE} WHERE id=? AND state='pending' " # nosec B608 @@ -417,6 +425,41 @@ class ReviewOperations: conn.execute(_BUMP_REVIEW_INDEX) return removed + def settle_untraced_review_items(self, locators) -> int: + """Remove the pending, undecided items that offer nothing to choose. + + ``locators`` are files the caller found ambiguous without a choice + (fewer than two candidates carry a prompt). Of their items, one leaves + the queue only when its own stored evidence offers no choice either + (``evidence_offers_choice``) and it is still ``pending`` at revision 0 + with no decision row, the same safe delete as + ``settle_linked_review_items``. Files without a pending item cost one + indexed read and open no write transaction. Returns how many items + were removed (the review index revision moves when any was). + """ + locators = list(locators) + for locator in locators: + _validate_locator(locator) + conn = self.model.get_connection() + pending = [ + locator + for locator in locators + if conn.execute(_PENDING_OF_LOCATOR, (locator,)).fetchone() is not None + ] + if not pending: + return 0 + removed = 0 + with self._pair_transaction() as conn: + for locator in pending: + rows = conn.execute(_PENDING_EVIDENCE_OF_LOCATOR, (locator,)).fetchall() + for item_id, evidence in rows: + if evidence_offers_choice(json.loads(evidence)): + continue + removed += conn.execute(_SETTLE_ITEM, (item_id,)).rowcount + if removed: + conn.execute(_BUMP_REVIEW_INDEX) + return removed + def review_queue_page(self, *, limit: int, after: tuple | None = None) -> dict: """One page of pending items, oldest first, from one snapshot. diff --git a/database/review_schema.py b/database/review_schema.py index 8c8aaf7..0370a5e 100644 --- a/database/review_schema.py +++ b/database/review_schema.py @@ -200,6 +200,44 @@ def canonical_evidence(value: Any) -> str: return text +def choosable(candidate: Any) -> bool: + """True when a candidate offers a prompt to choose: it is ``resolved`` and + its positive side carries non-blank text. The page disables any other + candidate, so it is never something to choose. + """ + if not isinstance(candidate, dict) or candidate.get("state") != "resolved": + return False + positive = candidate.get("positive") + if not isinstance(positive, dict): + return False + text = positive.get("text") + return isinstance(text, str) and bool(text.strip()) + + +# An item is worth a decision only when at least this many candidates offer +# a prompt; with fewer there is nothing to choose between (maintainer test of +# #284/#288: 39 items whose candidates were all "unresolved"). +MIN_CHOOSABLE = 2 + + +def evidence_offers_choice(evidence: Any) -> bool: + """False when stored evidence provably offers nothing to choose. + + Unlimited evidence must list ``MIN_CHOOSABLE`` choosable candidates. A + limited item stores none, only ``candidate_total``; it may offer a choice + (more than ``MAX_CANDIDATES`` prompts) unless that total is too small. + """ + if not isinstance(evidence, dict): + return False + if evidence.get("limited"): + total = evidence.get("candidate_total") + return type(total) is int and total >= MIN_CHOOSABLE + candidates = evidence.get("candidates") + if not isinstance(candidates, list): + return False + return sum(1 for c in candidates if choosable(c)) >= MIN_CHOOSABLE + + def evidence_digest(text: str) -> str: """SHA-256 of canonical evidence text (ASCII, hence UTF-8 identical).""" return hashlib.sha256(text.encode("utf-8")).hexdigest() diff --git a/py/api/admin.py b/py/api/admin.py index 740643a..8f8c5e4 100644 --- a/py/api/admin.py +++ b/py/api/admin.py @@ -44,6 +44,7 @@ try: parse_page_json, ) from ...utils.prompt_pair_resolver import resolve_prompt_pair + from ...utils.review_capture import reviewable except ImportError: from database.operations import PromptDatabase from database.pair_ingestion import ingest_resolved_pair @@ -54,6 +55,7 @@ except ImportError: parse_page_json, ) from utils.prompt_pair_resolver import resolve_prompt_pair + from utils.review_capture import reviewable try: @@ -214,8 +216,9 @@ def _time_left(done, total, batches, elapsed): def new_review_counts(): """The scan's Needs review tally: ``added`` new items, ``queued`` files in the queue after this scan (new or already there), ``settled`` items - removed because their file is linked now, and ``errors``.""" - return {"added": 0, "queued": 0, "settled": 0, "errors": 0} + removed because their file is linked now, ``untraced`` items removed + because they offer nothing to choose, and ``errors``.""" + return {"added": 0, "queued": 0, "settled": 0, "untraced": 0, "errors": 0} def _review_scope(review): @@ -229,6 +232,7 @@ def _review_scope(review): "review_items": review["queued"], "review_added": review["added"], "review_settled": review["settled"], + "review_untraced": review["untraced"], "review_errors": review["errors"], } @@ -1612,7 +1616,9 @@ class AdminRoutesMixin: whose final outcome is ambiguous and unlinked (the resolver found it ambiguous and the fallback did not link it) is recorded for Needs review (best effort, counted in ``review`` when given); one a saved - row already links is reported ``unchanged`` and never queued. + row already links is reported ``unchanged`` and never queued. An + ambiguous file with nothing to choose is reported ``unresolved`` and + its pending items without a choice leave the queue. """ counts = dict.fromkeys(SCAN_COUNT_KEYS, 0) @@ -1624,10 +1630,14 @@ class AdminRoutesMixin: if self._scan_job and self._scan_job.cancelled: break counts["processed"] += 1 - ambiguous = [] + ambiguous, untraced = [], [] try: outcome = self._ingest_scanned_file( - media_file, metadata, detailed=True, ambiguous=ambiguous + media_file, + metadata, + detailed=True, + ambiguous=ambiguous, + untraced=untraced, ) except Exception as e: self.logger.error(f"Error processing {media_file.name}: {e}") @@ -1639,6 +1649,8 @@ class AdminRoutesMixin: outcome = "unchanged" # already linked: not a review case else: self._record_scanned_review(media_file, roots, review) + elif untraced and outcome == "unresolved": + self._settle_untraced_reviews(untraced, roots, review) if outcome is None: note(media_file, None) continue @@ -1673,7 +1685,7 @@ class AdminRoutesMixin: return positive.strip() or None def _ingest_scanned_file( - self, media_file, metadata, detailed=False, ambiguous=None + self, media_file, metadata, detailed=False, ambiguous=None, untraced=None ): """Enrich exact pairs; retain legacy positive-only import for absent pairs. @@ -1683,7 +1695,10 @@ class AdminRoutesMixin: the pair counts. When the resolver finds the file ambiguous (whether or not the browser parser then links it) and ``ambiguous`` is a list, ``media_file`` is appended to it; only a file that then returns - ``"ambiguous"`` (nothing linked it) is a Needs review case. + ``"ambiguous"`` (nothing linked it) is a Needs review case. An + ambiguous result with nothing to choose (``reviewable`` is False: fewer + than two candidates carry a prompt) that nothing links returns + ``"unresolved"`` instead, and ``media_file`` goes to ``untraced``. """ if not metadata: return None @@ -1699,7 +1714,12 @@ class AdminRoutesMixin: return None _with_file_size(canonical, media_file) resolution = resolve_prompt_pair(canonical) - if resolution["state"] == "ambiguous" and ambiguous is not None: + # Ambiguous without a choice: unresolved unless something links it. + unchoosable = resolution["state"] == "ambiguous" and not reviewable(resolution) + if unchoosable: + if untraced is not None: + untraced.append(media_file) + elif resolution["state"] == "ambiguous" and ambiguous is not None: ambiguous.append(media_file) if resolution["state"] == "resolved": result = ingest_resolved_pair( @@ -1720,9 +1740,9 @@ class AdminRoutesMixin: # Graphs without a recognized sampler keep the legacy extractor below. prompt_text = self._graph_prompt_text(metadata) if not prompt_text: - return resolution["state"] + return "unresolved" if unchoosable else resolution["state"] elif resolution["state"] == "ambiguous": - return "ambiguous" + return "unresolved" if unchoosable else "ambiguous" elif resolution["state"] == "unresolved" and not isinstance( canonical.get("parameters"), str ): diff --git a/py/api/review_recording.py b/py/api/review_recording.py index 31de6d9..26c2de2 100644 --- a/py/api/review_recording.py +++ b/py/api/review_recording.py @@ -5,13 +5,18 @@ final outcome is ambiguous and unlinked. The strict resolver must find it ``ambiguous``, the scan's legacy fallback (the browser parser, ``_graph_prompt_text``) must not link it, and no saved row may link it already. A file the fallback links, an exact pair the ingester stored and an -already linked file are never queued. +already linked file are never queued. Neither is an ambiguous file with +nothing to choose: fewer than two of its candidates carry a prompt +(``utils.review_capture.reviewable``); the scan reports it unresolved ("could +not be traced"). The queue heals itself: a pending item whose file is now linked (by the fallback, a later resolution or the monitor) is removed by the next scan -that lists the file or by the monitor's next pass over it. Only an -undecided item goes (``pending``, revision 0, no decision row); a decision -is never deleted or changed (``settle_linked_review_items``). +that lists the file or by the monitor's next pass over it, and so is a +pending item without a choice whose file is still ambiguous without one +(``settle_untraced_review_items``). Only an undecided item goes +(``pending``, revision 0, no decision row); a decision is never deleted or +changed (``settle_linked_review_items``). The scan records through ``_record_scanned_review`` (best effort: a failure is logged and counted); the monitor calls ``record_review_file`` from its @@ -42,6 +47,9 @@ try: except ImportError: from utils.review_capture import record_ambiguous_file +# The scan log's reason for items that leave the queue with nothing to choose. +NOTHING_TO_CHOOSE = "nothing to choose" + class ReviewRecordingMixin: """Record the scan's and the monitor's ambiguous files for Needs review.""" @@ -49,7 +57,7 @@ class ReviewRecordingMixin: def _record_scanned_review(self, media_file, roots, review=None): """Record one ambiguous, unlinked scanned file (logged and counted).""" try: - outcome = self._record_review_sync(media_file, roots) + outcome = self._record_review_sync(media_file, roots, review) except Exception: self.logger.warning( f"{media_file.name} could not be added to Needs review", @@ -65,15 +73,22 @@ class ReviewRecordingMixin: if outcome in QUEUED_OUTCOMES: review["queued"] += 1 - def _record_review_sync(self, media_file, roots): - """``record_ambiguous_file`` for a file under one of ``roots`` (blocking).""" + def _record_review_sync(self, media_file, roots, review=None): + """``record_ambiguous_file`` for a file under one of ``roots`` (blocking). + + An ``untraced`` file (its captured bytes offer nothing to choose) also + loses its pending items that offer nothing either. + """ located = self._review_location(media_file, roots) if located is None: return "skipped" root, index, relative, _spelled = located - return record_ambiguous_file( + outcome = record_ambiguous_file( self.db, self._capture_store(), Path(root), index, relative ) + if outcome == "untraced": + self._settle_untraced_reviews([media_file], roots, review) + return outcome @staticmethod def _review_location(media_file, roots, real_roots=None): @@ -129,6 +144,33 @@ class ReviewRecordingMixin: review["settled"] += settled return settled + def _settle_untraced_reviews(self, media_files, roots, review=None): + """Remove the pending items without a choice of untraced ``media_files``. + + ``media_files`` are files found ambiguous with fewer than two + candidates that carry a prompt. Blocking; best effort like the linked + settle (a failure is logged and counted in ``review["errors"]``). + Returns how many items left. + """ + locators = [] + for media_file in media_files: + located = self._review_location(media_file, roots) + if located is not None: + locators.append(os.path.normcase(located[3])) + try: + removed = self.db.settle_untraced_review_items(locators) if locators else 0 + except Exception: + self.logger.warning( + "Needs review items without a choice could not be removed", + exc_info=True, + ) + if review is not None: + review["errors"] += 1 + return 0 + if review is not None: + review["untraced"] += removed + return removed + def _fallback_links(self, metadata): """True when the scan's legacy fallback would link these records.""" return bool(metadata.get("prompt")) and bool(self._graph_prompt_text(metadata)) @@ -140,7 +182,9 @@ class ReviewRecordingMixin: current root is ``skipped``. A linked file is never queued: its pending items are removed and the answer is ``linked``. An unlinked file the scan's fallback would link is ``skipped`` (the next scan - links it); any other file is recorded when it is ambiguous. While a + links it); any other file is recorded when it is ambiguous with a + choice, and is ``untraced`` (its pending items without a choice + removed) when it is ambiguous without one. While a job runs that a ``records`` job may not run beside, nothing is written and the answer is ``deferred``. Errors propagate: the monitor logs them and keeps its link. @@ -183,6 +227,12 @@ class ReviewRecordingMixin: f"{SCAN_LOG_PREFIXES['known']} " f"{_plural(review['settled'], 'image')} left Needs review · now linked" ) + if review["untraced"]: + job.log_line( + f"{SCAN_LOG_PREFIXES['known']} " + f"{_plural(review['untraced'], 'image')} left Needs review · " + f"{NOTHING_TO_CHOOSE}" + ) if review["errors"]: job.log_line( f"{SCAN_LOG_PREFIXES['errors']} " diff --git a/tests/test_api_admin.py b/tests/test_api_admin.py index 3ae7626..5db64b8 100644 --- a/tests/test_api_admin.py +++ b/tests/test_api_admin.py @@ -2223,7 +2223,9 @@ class TestActivityJobs(AdminAPITestCase): for name in names: (self.output_dir / name).write_bytes(b"not an image") - def fake_ingest(media_file, metadata, detailed=False, ambiguous=None): + def fake_ingest( + media_file, metadata, detailed=False, ambiguous=None, untraced=None + ): key = Path(media_file).relative_to(self.output_dir).as_posix() outcome = names[key] if isinstance(outcome, Exception): diff --git a/tests/test_api_review.py b/tests/test_api_review.py index 5e872f7..cb5e5a2 100644 --- a/tests/test_api_review.py +++ b/tests/test_api_review.py @@ -23,10 +23,17 @@ try: ReviewTestCase, ambiguous_graph, digest, + unlinkable_graph, write_png, ) except ImportError: # discovered with tests/ as the top-level directory - from test_review import ReviewTestCase, ambiguous_graph, digest, write_png + from test_review import ( + ReviewTestCase, + ambiguous_graph, + digest, + unlinkable_graph, + write_png, + ) from database.review_schema import canonical_evidence, evidence_digest from py.config import GalleryConfig @@ -284,13 +291,9 @@ class TestQueueReads(ReviewRoutesTestCase): self._require_routes() self._roots() limited = self.output_dir / "limited.png" - write_png( - limited, - parameters=( - "cat\nNegative prompt: blur\nNegative prompt: noise\n" - "Steps: 20, Sampler: Euler" - ), - ) + # More than 16 candidates with a prompt: limited, still a choice. + many = [f"prompt {n}" for n in range(17)] + write_png(limited, prompt=unlinkable_graph(*many)) self.assertEqual(self._record_raw(str(limited)), "limited") self._record(self._ambiguous("two.png")) items = (await self._list())["items"] diff --git a/tests/test_pair_scan.py b/tests/test_pair_scan.py index b8d914d..1597b1f 100644 --- a/tests/test_pair_scan.py +++ b/tests/test_pair_scan.py @@ -82,7 +82,8 @@ class TestPairScan(AdminAPITestCase): "class_type": "BasicGuider", "inputs": {"width": 2048, "height": 2048}, } - self.assertEqual(self._scanned("untraceable.png", g), "ambiguous") + # Ambiguous with nothing to choose: unresolved, never a review item. + self.assertEqual(self._scanned("untraceable.png", g), "unresolved") self.assertEqual(self.api.db.list_prompt_pairs()["total"], 0) self.assertEqual(self._texts(), []) @@ -159,9 +160,10 @@ class TestPairScan(AdminAPITestCase): "\nSteps: 20, Sampler: Euler" ) } + # No candidate to choose: reported unresolved, still not guessed. self.assertEqual( self.api._ingest_scanned_file(self.output_dir / "ambiguous.png", meta), - "ambiguous", + "unresolved", ) self.assertEqual(self.api.db.get_recent_prompts()["prompts"], []) diff --git a/tests/test_review.py b/tests/test_review.py index c118cea..70776c0 100644 --- a/tests/test_review.py +++ b/tests/test_review.py @@ -267,10 +267,12 @@ class TestScanRecording(ReviewTestCase): with self.assertLogs("prompt_manager.api", logging.WARNING) as logs: without = run(other, broken) self.assertEqual(recorded, without) - self.assertEqual(recorded[0]["ambiguous"], 2) + # delims.png offers nothing to choose: unresolved, not queued. + self.assertEqual(recorded[0]["ambiguous"], 1) + self.assertEqual(recorded[0]["unresolved"], 1) self.assertEqual(recorded[0]["added"], 1) self.assertTrue(any("Needs review" in line for line in logs.output)) - self.assertEqual(len(self._items()), 2) + self.assertEqual(len(self._items()), 1) other.close_annotation_captures() other_db.close_all() @@ -407,7 +409,9 @@ class TestScanRecording(ReviewTestCase): self.assertEqual(items[0]["locator"], items[1]["locator"]) self.assertEqual(job["scope"]["review_items"], 1) - async def test_parameters_delimiter_ambiguity_is_limited_without_candidates(self): + async def test_many_candidates_are_limited_without_candidates(self): + """Only ``many.png`` is queued: the A1111 delimiter ambiguity offers + no candidate to choose (``tests/test_review_choosable.py``).""" self._require_module() self._roots() write_png( @@ -420,7 +424,7 @@ class TestScanRecording(ReviewTestCase): job = await self._scan() items = {Path(item["locator"]).name: item for item in self._items()} - self.assertEqual(set(items), {"delims.png", "many.png"}) + self.assertEqual(set(items), {"many.png"}) for item in items.values(): self.assertEqual((item["limited"], item["candidate_count"]), (1, 0)) evidence = json.loads(item["evidence"]) @@ -431,7 +435,7 @@ class TestScanRecording(ReviewTestCase): json.loads(items["many.png"]["evidence"])["candidate_total"], 18, ) - self.assertEqual(job["scope"]["review_items"], 2) + self.assertEqual(job["scope"]["review_items"], 1) async def test_capacity_errors_are_logged_and_counted_not_fatal(self): self._require_module() @@ -533,8 +537,11 @@ class TestRecordAmbiguousFile(ReviewTestCase): self.assertEqual(self._record("two.png"), "duplicate") self.assertEqual(self._record("one.png"), "skipped") self.assertEqual(self._record("missing.png"), "skipped") - self.assertEqual(self._record("delims.png"), "limited") - self.assertEqual(self._record("delims.png"), "duplicate") + self.assertEqual(self._record("delims.png"), "untraced") + many = [f"prompt {n}" for n in range(17)] + write_png(self.output_dir / "many.png", prompt=unlinkable_graph(*many)) + self.assertEqual(self._record("many.png"), "limited") + self.assertEqual(self._record("many.png"), "duplicate") self.assertEqual(self._review_revision(), 2) async def test_restore_between_capture_and_write_skips(self): diff --git a/tests/test_review_choosable.py b/tests/test_review_choosable.py new file mode 100644 index 0000000..de86540 --- /dev/null +++ b/tests/test_review_choosable.py @@ -0,0 +1,335 @@ +"""Needs review queues a file only when there is something to choose. + +The maintainer's live queue (#284 / #288) held 39 items whose 11 candidates +all read "unresolved", "No positive / No negative": the ingestion resolver +found several sampler branches but could trace no prompt on any of them. +An ambiguous result is reviewable only when at least two candidates carry a +resolved prompt text. With none, the file could not be traced; with exactly +one, the other branches may still have made the file, so the one text is +not certain either (the resolver never flattens unknown semantics). Both +stay unresolved and are never queued, and a rescan removes the pending, +undecided items such files already have. +""" + +import os +import unittest +from unittest.mock import patch + +try: + from tests.test_review import DELIMITERS, write_png + from tests.test_review_outcome import OutcomeTestCase +except ImportError: # discovered with tests/ as the top-level directory + from test_review import DELIMITERS, write_png + from test_review_outcome import OutcomeTestCase + +from database.review_schema import evidence_offers_choice +from utils import review_capture + + +def opaque_graph(*opaque, traced=()): + """Several KSamplers: ``opaque`` positives come from an encoder the + ingestion resolver does not know, ``traced`` ones from CLIPTextEncode. + + The first sampler is opaque, so the scan's browser-parser fallback links + nothing either. + """ + graph = { + "1": {"class_type": "CheckpointLoaderSimple", "inputs": {"ckpt_name": "m"}}, + "2": {"class_type": "CLIPTextEncode", "inputs": {"text": "bad"}}, + } + sides = [("OpaqueTextEncode", "prompt", t) for t in opaque] + sides += [("CLIPTextEncode", "text", t) for t in traced] + for index, (encoder_class, key, text) in enumerate(sides): + encoder, sampler = str(10 + 2 * index), str(11 + 2 * index) + graph[encoder] = {"class_type": encoder_class, "inputs": {key: text}} + graph[sampler] = { + "class_type": "KSampler", + "inputs": { + "model": ["1", 0], + "positive": [encoder, 0], + "negative": ["2", 0], + }, + } + return graph + + +def _always_reviewable(_resolution): + return True + + +class ChoosableTestCase(OutcomeTestCase): + def _record_pre_fix(self, name): + """An item as the pre-fix recorder stored it (no choosable test).""" + with patch.object(review_capture, "reviewable", _always_reviewable): + self._record(name) + + def _write(self, name, graph): + write_png(self.output_dir / name, prompt=graph) + return self.output_dir / name + + +class TestChoosableCandidates(unittest.TestCase): + def _resolution(self, *states): + candidates = [] + for index, state in enumerate(states): + side = {"text": f"t{index}"} if state == "resolved" else None + candidates.append( + {"sampler_id": str(index), "state": state, "positive": side} + ) + return {"state": "ambiguous", "candidates": candidates} + + def test_two_resolved_candidates_are_reviewable(self): + resolution = self._resolution("resolved", "resolved", "unresolved") + self.assertTrue(review_capture.reviewable(resolution)) + + def test_none_or_one_resolved_candidate_is_not_reviewable(self): + for states in ((), ("unresolved",) * 11, ("resolved", "unresolved")): + with self.subTest(states=states): + resolution = self._resolution(*states) + self.assertFalse(review_capture.reviewable(resolution)) + + def test_blank_or_missing_text_does_not_count(self): + resolution = self._resolution("resolved", "resolved") + resolution["candidates"][0]["positive"] = {"text": " "} + self.assertFalse(review_capture.reviewable(resolution)) + resolution["candidates"][0]["positive"] = None + self.assertFalse(review_capture.reviewable(resolution)) + + def test_nested_ambiguous_stage_counts_its_own_candidates(self): + inner = self._resolution("resolved", "resolved")["candidates"] + resolution = { + "state": "ambiguous", + "candidates": [{"state": "ambiguous", "candidates": inner}], + } + self.assertTrue(review_capture.reviewable(resolution)) + + def test_a_result_that_is_not_ambiguous_is_not_reviewable(self): + resolution = self._resolution("resolved", "resolved") + resolution["state"] = "resolved" + self.assertFalse(review_capture.reviewable(resolution)) + + +class TestStoredEvidenceOffersChoice(unittest.TestCase): + def test_unlimited_evidence_needs_two_choosable_candidates(self): + side = {"text": "cat"} + two = [{"state": "resolved", "positive": side}] * 2 + self.assertTrue(evidence_offers_choice({"candidates": two})) + one = two[:1] + [{"state": "unresolved", "positive": None}] + self.assertFalse(evidence_offers_choice({"candidates": one})) + + def test_limited_evidence_offers_a_choice_only_with_two_candidates(self): + self.assertTrue( + evidence_offers_choice({"limited": True, "candidate_total": 18}) + ) + for total in (0, 1, None, "18", True): + with self.subTest(total=total): + evidence = {"limited": True, "candidate_total": total} + self.assertFalse(evidence_offers_choice(evidence)) + + def test_malformed_evidence_offers_nothing(self): + for evidence in (None, [], {"candidates": "x"}, {}): + with self.subTest(evidence=evidence): + self.assertFalse(evidence_offers_choice(evidence)) + + +class TestNothingToChooseIsNotQueued(ChoosableTestCase): + async def test_no_candidate_with_text_could_not_be_traced(self): + self._roots() + self._write("none.png", opaque_graph("cat", "dog")) + + job = await self._scan() + + self.assertEqual(self._items(), []) + self.assertEqual(self._pending(), 0) + self.assertEqual(job["scope"]["review_items"], 0) + (row,) = job["outcomes"]["review"] + self.assertEqual(row["path"], "none.png") + self.assertIn("could not be traced", row["detail"]) + self.assertEqual(job["counters"]["review"], 1, job["counters"]) + self.assertEqual(self._capture_files(), []) + + async def test_exactly_one_candidate_with_text_stays_unresolved(self): + """One traced branch beside untraced ones is not certain: no link.""" + self._roots() + self._write("one.png", opaque_graph("cat", traced=("dog",))) + + job = await self._scan() + + self.assertEqual(self._items(), []) + (row,) = job["outcomes"]["review"] + self.assertIn("could not be traced", row["detail"]) + self.assertEqual(job["counters"].get("added", 0), 0, job["counters"]) + self.assertEqual(job["counters"].get("known", 0), 0, job["counters"]) + + async def test_two_candidates_with_text_are_queued(self): + self._roots() + self._write("two.png", opaque_graph("x", traced=("cat", "dog"))) + + job = await self._scan() + + (item,) = self._items() + self.assertEqual(os.path.basename(item["locator"]), "two.png") + self.assertEqual(item["candidate_count"], 3) + self.assertEqual(job["scope"]["review_items"], 1) + (row,) = job["outcomes"]["review"] + self.assertIn("more than one prompt could match", row["detail"]) + + async def test_parameters_delimiter_ambiguity_is_not_queued(self): + """Two A1111 negative delimiters offer no candidate to choose.""" + self._roots() + write_png(self.output_dir / "delims.png", parameters=DELIMITERS) + + job = await self._scan() + + self.assertEqual(self._items(), []) + (row,) = job["outcomes"]["review"] + self.assertIn("could not be traced", row["detail"]) + + def test_recorder_reports_untraced_and_stores_nothing(self): + self._roots() + self._write("none.png", opaque_graph("cat", "dog")) + self._write("one.png", opaque_graph("cat", traced=("dog",))) + for name in ("none.png", "one.png"): + with self.subTest(name=name): + outcome = review_capture.record_ambiguous_file( + self.api.db, self.api._captures, self.output_dir, 0, name + ) + self.assertEqual(outcome, "untraced") + self.assertEqual(self._items(), []) + self.assertEqual(self._capture_files(), []) + + def test_monitor_does_not_queue_an_untraced_output(self): + self._roots() + path = self._write("none.png", opaque_graph("cat", "dog")) + + self.assertEqual(self.api.record_review_file(str(path)), "untraced") + self.assertEqual(self._items(), []) + + +class TestUntracedItemsLeaveTheQueue(ChoosableTestCase): + async def test_rescan_removes_pending_items_without_a_choice(self): + """The maintainer's 39 items: queued before the rule, gone on rescan.""" + self._roots() + self._write("none.png", opaque_graph("cat", "dog")) + self._write("one.png", opaque_graph("cat", traced=("dog",))) + self._write("two.png", opaque_graph("x", traced=("cat", "dog"))) + for name in ("none.png", "one.png", "two.png"): + self._record_pre_fix(name) + revision = self._review_revision() + self.assertEqual(self._pending(), 3) + + job = await self._scan("all") + + (item,) = self._items() + self.assertEqual(os.path.basename(item["locator"]), "two.png") + self.assertEqual(self._pending(), 1) + self.assertGreater(self._review_revision(), revision) + self.assertEqual(job["scope"]["review_untraced"], 2) + self.assertEqual(job["scope"]["review_items"], 1) + self.assertIn( + "= 2 images left Needs review · nothing to choose", "\n".join(job["log"]) + ) + + async def test_decided_items_of_untraced_files_are_never_touched(self): + self._roots() + for name in ("sel.png", "unres.png"): + self._write(name, opaque_graph("cat", traced=(name, name + "2"))) + self._record_pre_fix(name) + items = {os.path.basename(i["locator"]): i for i in self._items()} + self._decide(items["sel.png"], "selected") + self._decide(items["unres.png"], "unresolved") + before = (self._items(), self._decisions()) + # The files now carry graphs without a choice, queued pre-fix. + for name in ("sel.png", "unres.png", "pending.png"): + self._write(name, opaque_graph("a cow", "dog")) + self._record_pre_fix(name) + self.assertEqual(self._pending(), 3) + + job = await self._scan("all") + + self.assertEqual((self._items(), self._decisions()), before) + self.assertEqual(self._pending(), 0) + self.assertEqual(job["scope"]["review_untraced"], 3) + + async def test_an_item_with_a_choice_survives_beside_an_untraced_one(self): + """Removal judges each item by its own evidence: an item that offers + a choice is not removed although the file's new bytes offer none.""" + self._roots() + self._write("f.png", opaque_graph("x", traced=("cat", "dog"))) + self._record_pre_fix("f.png") + (choosable,) = self._items() + self._write("f.png", opaque_graph("a cow", "dog")) + self._record_pre_fix("f.png") + self.assertEqual(self._pending(), 2) + + job = await self._scan("all") + + self.assertEqual([i["id"] for i in self._items()], [choosable["id"]]) + self.assertEqual(job["scope"]["review_untraced"], 1) + + async def test_limited_items_go_only_without_enough_candidates(self): + """A pre-fix delimiter item (no candidate) goes; a limited item of 18 + candidates may hold a choice and stays, though the bytes now offer + none.""" + self._roots() + write_png(self.output_dir / "delims.png", parameters=DELIMITERS) + self._record_pre_fix("delims.png") + many = [f"prompt {n}" for n in range(17)] + self._write("many.png", opaque_graph("x", traced=many)) + self._record_pre_fix("many.png") + kept = [i["id"] for i in self._items() if "many" in i["locator"]] + self._write("many.png", opaque_graph("a cow", "dog")) + + job = await self._scan("all") + + self.assertEqual([i["id"] for i in self._items()], kept) + self.assertEqual(job["scope"]["review_untraced"], 1) + + async def test_choosable_pending_item_stays(self): + self._roots() + self._write("two.png", opaque_graph("x", traced=("cat", "dog"))) + await self._scan() + (item,) = self._items() + + job = await self._scan("all") + + self.assertEqual([i["id"] for i in self._items()], [item["id"]]) + self.assertEqual(job["scope"]["review_untraced"], 0) + + async def test_a_failed_removal_is_logged_and_counted_not_fatal(self): + import logging + import sqlite3 + + self._roots() + self._write("none.png", opaque_graph("cat", "dog")) + self._record_pre_fix("none.png") + with ( + patch.object( + self.api.db, + "settle_untraced_review_items", + side_effect=sqlite3.OperationalError("database is locked"), + ), + self.assertLogs("prompt_manager.api", logging.WARNING) as logs, + ): + job = await self._scan("all") + + self.assertEqual(job["state"], "finished") + self.assertEqual(job["scope"]["review_errors"], 1) + self.assertTrue(any("could not be removed" in line for line in logs.output)) + self.assertEqual(self._pending(), 1) + await self._scan("all") + self.assertEqual(self._pending(), 0) + + def test_monitor_removes_the_pending_item_of_an_untraced_output(self): + self._roots() + path = self._write("none.png", opaque_graph("cat", "dog")) + self._record_pre_fix("none.png") + # A pre-fix item of other bytes keeps the stat shortcut out of the way. + self._write("none.png", opaque_graph("a cow", "dog")) + + self.assertEqual(self.api.record_review_file(str(path)), "untraced") + self.assertEqual(self._items(), []) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_review_outcome.py b/tests/test_review_outcome.py index 3c36658..c4952c8 100644 --- a/tests/test_review_outcome.py +++ b/tests/test_review_outcome.py @@ -184,10 +184,12 @@ class TestOnlyUnlinkedAmbiguousFilesAreQueued(OutcomeTestCase): if "could not be traced" in row["detail"] ) queued = [name for name in review if name not in unresolved_rows] - self.assertEqual(queued, ["delims.png", "u1.png", "u2.png"], job) - self.assertEqual(job["scope"]["review_items"], 3) - self.assertEqual(self._pending(), 3) - self.assertIn("? 3 images already in Needs review", "\n".join(job["log"])) + # delims.png has nothing to choose: "could not be traced". + self.assertEqual(queued, ["u1.png", "u2.png"], job) + self.assertIn("delims.png", unresolved_rows) + self.assertEqual(job["scope"]["review_items"], 2) + self.assertEqual(self._pending(), 2) + self.assertIn("? 2 images already in Needs review", "\n".join(job["log"])) class TestQueueSelfHeals(OutcomeTestCase): diff --git a/utils/review_capture.py b/utils/review_capture.py index 31b9e2b..f26a46b 100644 --- a/utils/review_capture.py +++ b/utils/review_capture.py @@ -6,15 +6,21 @@ finds ``ambiguous`` to :func:`record_ambiguous_file`. It captures the file reads the metadata again from the capture, resolves it with the default (ingestion) profile and stores only an ``ambiguous`` result, bound to the capture's byte record. The evidence a reviewer chooses from is therefore the -evidence of exactly those bytes. Nothing here links a file, loads a model or -decodes pixels; the capture is released before the database write. +evidence of exactly those bytes. An ambiguous result is stored only when it +is ``reviewable``: at least two candidates carry a resolved prompt text. With +none the file could not be traced; with exactly one, the untraced branches +may still have made the file, so that text is not certain and the file stays +unresolved (the resolver never flattens unknown semantics into a guess). +Nothing here links a file, loads a model or decodes pixels; the capture is +released before the database write. Evidence is small on purpose (startup validates every row): the resolver's reason and, per candidate sampler, ``{sampler_id, state, reason, positive, negative}`` where each side is ``{text, storage_text, provenance}``. A candidate's ``hash`` is the SHA-256 of its canonical JSON without the hash. -More than ``MAX_CANDIDATES`` candidates, none at all, or evidence over the -8 MiB bound make a ``limited`` item without candidates. +More than ``MAX_CANDIDATES`` candidates or evidence over the 8 MiB bound make +a ``limited`` item without candidates (a result without any candidate is not +``reviewable`` and is never stored). """ from __future__ import annotations @@ -28,13 +34,23 @@ from typing import Any try: from ..database.library_commands import LibraryCommandError - from ..database.review_schema import MAX_CANDIDATES, canonical_evidence + from ..database.review_schema import ( + MAX_CANDIDATES, + MIN_CHOOSABLE, + canonical_evidence, + choosable, + ) from .media_capture import CaptureCapacity, CaptureError from .prompt_inspector import MetadataReadError, read_inspection_file from .prompt_pair_resolver import RESOLVER_VERSION, resolve_prompt_pair except ImportError: # imported with the repo root on sys.path from database.library_commands import LibraryCommandError - from database.review_schema import MAX_CANDIDATES, canonical_evidence + from database.review_schema import ( + MAX_CANDIDATES, + MIN_CHOOSABLE, + canonical_evidence, + choosable, + ) from utils.media_capture import CaptureCapacity, CaptureError from utils.prompt_inspector import MetadataReadError, read_inspection_file from utils.prompt_pair_resolver import RESOLVER_VERSION, resolve_prompt_pair @@ -55,7 +71,9 @@ def record_ambiguous_file(db, store, root, root_index: int, relative: str) -> st evidence, or the file's current stat is the verified fingerprint of bytes an item cites, so it is not even copied, and that item is still pending), ``decided`` (as ``duplicate``, but the item carries a decision, - so the file is not in the queue) or ``skipped`` (the file + so the file is not in the queue), ``untraced`` (ambiguous in its captured + bytes, but not ``reviewable``: nothing to choose, nothing stored) or + ``skipped`` (the file changed, vanished, is not ambiguous in its captured bytes, or storage was restored meanwhile). ``CaptureCapacity`` and database errors propagate to the caller, which @@ -87,6 +105,8 @@ def record_ambiguous_file(db, store, root, root_index: int, relative: str) -> st resolution = captured[1] if captured is not None else None if resolution is None or resolution.get("state") != "ambiguous": return "skipped" + if not reviewable(resolution): + return "untraced" evidence, count, limited = review_evidence(resolution) try: inserted = db.record_review_item( @@ -180,6 +200,24 @@ def captured_review(store, capture) -> tuple[dict, dict] | None: return canonical, resolve_prompt_pair(canonical) +def reviewable(resolution: Any) -> bool: + """True when an ambiguous result offers a choice: ``MIN_CHOOSABLE`` of its + selectable candidates (nested ambiguous stages flattened) are choosable. + + Counted before the evidence is bounded, so a ``limited`` item (more than + ``MAX_CANDIDATES`` candidates) with prompts to choose is still queued. + """ + if not isinstance(resolution, dict) or resolution.get("state") != "ambiguous": + return False + count = 0 + for candidate in _flattened(resolution.get("candidates"), 0): + if choosable(candidate): + count += 1 + if count >= MIN_CHOOSABLE: + return True + return False + + def review_evidence(resolution: dict) -> tuple[dict, int, bool]: """``(evidence, candidate_count, limited)`` for one ambiguous result.""" base = {