From bf17be1b4d9fe00169738fe7a4dfc0f6b48d77fb Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:33:05 +0000 Subject: [PATCH 1/5] fix(image_evidence): wire the artist-crop fallback into live extraction artist_ocr_name stays blank on 87,371 evidence rows that carry non-blank artist_ocr_raw_text - the "Illus." anchor misses OCR-noisy reads (e.g. "Soot Itus." never matches _ILLUS_RE), and the 2026-07-29 collector-line recovery (recover_artist_from_card_text) only reads collector_line_raw_text/ legal_line_raw_text, so it is structurally blind to old-border proxies whose only credit is a centred "Illus. " line with no bottom print row at all. cardpicker.modern_artist_credit.recognize_artist_credit already exists for exactly this shape of input (re-reading artist_ocr_raw_text) but was only ever reachable through its own standalone backfill_modern_artist_names command, never during real extraction. This wires it into compute_card_evidence as a third fallback, tried only after both the anchor and the collector/legal-line recovery find nothing storable - strictly additive (new modern_artist_lexicon param, default None, no extractor_versions bump), and threaded through both live Stage C entry points (stage_e_dispatch.py, run_image_evidence_cohort.py). Verified against real production evidence rows (ids 221241/221268/221274): recognize_artist_credit recovers "Sebastian Giacobino"/"Aaron Miller"/ "Andrey Kuzinskiy" from their stored artist_ocr_raw_text at ratio 1.0 against the real ~2.5k-name lexicon. A fourth reproduction row (id 221289, "David Rapoza") turned out to be a correct abstention, not a bug: the real lexicon holds both "Dave Rapoza" and "David Rapoza" as distinct canonical artists, so the reading is genuinely ambiguous under the 2026-07-29 ruling (fuzzy matching yes, fuzzy storage no) - confirmed by direct measurement, not assumed. Full-population measurement of the 87,371-row split: 19,478 (22.3%) genuinely recoverable via the artist-crop fallback, 329 (0.4%) genuinely ambiguous (correctly abstained), 67,564 (77.3%) carry no artist credit findable by any means - consistent with this catalog's known proxy population rendered without standard bottom-row metadata. --- MPCAutofill/cardpicker/image_evidence.py | 65 +++++++++-- .../commands/run_image_evidence_cohort.py | 40 +++++-- MPCAutofill/cardpicker/stage_e_dispatch.py | 23 ++-- .../cardpicker/tests/test_image_evidence.py | 110 ++++++++++++++++++ .../tests/test_run_image_evidence_cohort.py | 9 +- .../cardpicker/tests/test_stage_e_dispatch.py | 4 + 6 files changed, 226 insertions(+), 25 deletions(-) diff --git a/MPCAutofill/cardpicker/image_evidence.py b/MPCAutofill/cardpicker/image_evidence.py index ec3d223dc..7f947df9a 100644 --- a/MPCAutofill/cardpicker/image_evidence.py +++ b/MPCAutofill/cardpicker/image_evidence.py @@ -278,6 +278,7 @@ ) from cardpicker.local_phash import ART_CROP_BOX from cardpicker.models import Card, CardScanLog, ImageEvidence +from cardpicker.modern_artist_credit import LexiconIndex, recognize_artist_credit from cardpicker.utils import twos_complement logger = logging.getLogger(__name__) @@ -651,6 +652,7 @@ def fetch_and_compute_card_evidence_for_tests( artist_lexicon: Optional[ArtistLexicon] = None, printing_artist_lookup: Optional[Callable[[Optional[str], Optional[str]], Optional[str]]] = None, name_artist_lookup: Optional[Callable[[str], tuple[str, ...]]] = None, + modern_artist_lexicon: Optional[LexiconIndex] = None, ) -> ExtractionResult: """ A TEST-ONLY CONVENIENCE WRAPPER. It has no production caller and has not had one since the @@ -720,6 +722,10 @@ def fetch_and_compute_card_evidence_for_tests( picklable `ProcessPoolExecutor` entrypoint, and a resolver holding a 113k-row index is not something to send across that boundary. `None` (the default) means no narrowing - every pre-2026-07-29 caller's behaviour, unchanged. + + `modern_artist_lexicon`, if given, is forwarded straight through to `compute_card_evidence` + below - see that function's own docstring for the ARTIST-CROP FALLBACK it controls. `None` + (the default) leaves it off, behavior identical to every caller that predates it. """ fetch_started_at = time.monotonic() @@ -743,6 +749,7 @@ def fetch_and_compute_card_evidence_for_tests( artist_lexicon=artist_lexicon, printing_artist_lookup=printing_artist_lookup, card_artist_names=() if name_artist_lookup is None else name_artist_lookup(card.name), + modern_artist_lexicon=modern_artist_lexicon, md5_checksum=card.md5_checksum, sha256_checksum=card.sha256_checksum, ) @@ -763,6 +770,7 @@ def compute_card_evidence( artist_lexicon: Optional[ArtistLexicon] = None, printing_artist_lookup: Optional[Callable[[Optional[str], Optional[str]], Optional[str]]] = None, card_artist_names: tuple[str, ...] = (), + modern_artist_lexicon: Optional[LexiconIndex] = None, md5_checksum: Optional[str] = None, sha256_checksum: Optional[str] = None, ) -> ExtractionResult: @@ -921,6 +929,39 @@ def compute_card_evidence( `local_identify_printing_tags.CandidateNameIndex.candidates_for` - the codebase's existing normaliser, not a new one. Empty (the default) means no narrowing. + `modern_artist_lexicon` (2026-08-04): the ARTIST-CROP FALLBACK - a third, independent source + for the `artist_ocr_name` storage fallback above, tried only after BOTH the "Illus." anchor + AND the collector/legal-line recovery have found nothing storable. + + THE GAP THIS CLOSES. `recover_artist_from_card_text` reads only `collector_line_raw_text` and + `legal_line_raw_text` - the bottom PRINT ROW. A real, measured population of cards (old-border + proxies with a centred "Illus. " credit and no collector line/set code/copyright row + printed at all) has NO text in either of those two fields, so that recovery is structurally + blind to them even when the anchor regex itself missed the credit to ordinary OCR noise + (`_ILLUS_RE` requires literal "llus", and a misread like "Soot Itus." - real production text, + card evidence id 221268 - never matches it). The ONE stored string that DOES carry the credit + on these cards is `artist_ocr_raw_text` - the artist-crop OCR text this same extractor already + computed a few lines above - and until now nothing in the live extraction path ever re-read it. + + `cardpicker.modern_artist_credit.recognize_artist_credit` already exists for exactly this + shape of input (its own module docstring: "a wholly independent, parse-only re-reader of the + SAME already-stored `artist_ocr_raw_text` strings") and was previously reachable only through + its own standalone `backfill_modern_artist_names` command, never during real extraction - so a + freshly-extracted or re-extracted row could go on carrying the same gap forever. Verified + against three real evidence rows this way blank in production (ids 221241, 221268, 221274): + `recognize_artist_credit` recovers "Sebastian Giacobino"/"Aaron Miller"/"Andrey Kuzinskiy" from + their stored `artist_ocr_raw_text` at ratio 1.0 with a comfortable margin over the runner-up, + using the real ~2.5k-name production lexicon - not a toy fixture. + + ORDER: tried strictly AFTER `recover_artist_from_card_text` returns nothing storable, never + instead of it or in competition with it - the print-row read is format-anchored (a fixed + layout, narrowed by `card_artist_names`) and stays authoritative when it succeeds; this is + purely an ADDITIONAL fallback for the population it can never reach. `None` (the default, and + every pre-2026-08-04 caller) leaves this off entirely - byte-identical to before. No + extractor_versions key is added and no version bumped, deliberately, for the same reason + `artist_lexicon` above isn't: either would invalidate every existing row and force a full + catalog re-extraction to recover text this repository already has. + `md5_checksum`/`sha256_checksum` (2026-07-25, issue #473 PR-2, folded with issue #472): the calling card's own live `Card.md5_checksum`/`Card.sha256_checksum` at the moment of THIS real extraction pass, stamped verbatim onto the result's `fields` (so @@ -1251,18 +1292,28 @@ def compute_card_evidence( # key is added and no version bumped, deliberately - either would invalidate every # existing row against `run_image_evidence_cohort.MANIFEST_EXTRACTOR_CURRENT_VERSIONS` and # force a full 220k-card Stage C re-extraction. - if artist_name is None and artist_lexicon is not None: - recovered_artist = recover_artist_from_card_text( - fields["collector_line_raw_text"], - legal_line_raw_text, - artist_lexicon, - allowed_artist_names=card_artist_names, - ) + if artist_name is None: + recovered_artist = None + if artist_lexicon is not None: + recovered_artist = recover_artist_from_card_text( + fields["collector_line_raw_text"], + legal_line_raw_text, + artist_lexicon, + allowed_artist_names=card_artist_names, + ) if recovered_artist is not None and recovered_artist.canonical_name is not None: # `canonical_name` is a verbatim `CanonicalArtist.name` and is `None` unless the # reading is compatible with exactly one of them - fuzzy MATCHING is permitted, # fuzzy STORAGE is not (owner ruling, 2026-07-29). fields["artist_ocr_name"] = recovered_artist.canonical_name + elif modern_artist_lexicon is not None: + # ARTIST-CROP FALLBACK (2026-08-04) - see this function's own `modern_artist_ + # lexicon` docstring paragraph. Re-reads `artist_raw_text` (the artist-crop OCR + # text this extractor already computed above), the one stored string the + # collector/legal-line recovery above structurally cannot reach. + recognized_artist = recognize_artist_credit(artist_raw_text, modern_artist_lexicon) + if recognized_artist is not None: + fields["artist_ocr_name"] = recognized_artist.matched_name extractor_versions["collector_line_ocr"] = COLLECTOR_LINE_OCR_EXTRACTOR_VERSION extractor_versions["artist_ocr"] = ARTIST_OCR_EXTRACTOR_VERSION diff --git a/MPCAutofill/cardpicker/management/commands/run_image_evidence_cohort.py b/MPCAutofill/cardpicker/management/commands/run_image_evidence_cohort.py index 53753bb0c..691be3503 100644 --- a/MPCAutofill/cardpicker/management/commands/run_image_evidence_cohort.py +++ b/MPCAutofill/cardpicker/management/commands/run_image_evidence_cohort.py @@ -277,6 +277,7 @@ from cardpicker.harvest_fetch_limiter import GoogleFetchLockoutError from cardpicker.local_calculate_verdicts import known_set_codes from cardpicker.models import CanonicalCard, Card, ImageEvidence, PilotRunLedger +from cardpicker.modern_artist_credit import LexiconIndex, load_lexicon_index from cardpicker.pilot_run_lifecycle import ( add_dry_run_guard_arguments, enforce_dry_run_precondition, @@ -410,9 +411,13 @@ def already_extracted_card_ids(run_id: str, only_never_extracted: bool = False) # only production constructor) always passes it. _WORKER_ARTIST_LEXICON: Optional[ArtistLexicon] = None _WORKER_PRINTING_ARTIST_LOOKUP: Optional[PrintingArtistLookup] = None +_WORKER_MODERN_ARTIST_LEXICON: Optional[LexiconIndex] = None -def _init_worker(artist_lexicon: Optional[ArtistLexicon] = None) -> None: +def _init_worker( + artist_lexicon: Optional[ArtistLexicon] = None, + modern_artist_lexicon: Optional[LexiconIndex] = None, +) -> None: """Compute pool `initializer=` - runs once per worker PROCESS, immediately after it starts (fork on Linux), before that worker executes its first task. Three jobs (the rate-limiter descaling job is gone entirely under the decoupled design - see module docstring): @@ -423,6 +428,10 @@ def _init_worker(artist_lexicon: Optional[ArtistLexicon] = None) -> None: ARTIST WIRING section for why this one travels by initializer while `known_set_codes` travels per submit, and why `printing_artist_lookup` is built here rather than shipped from the parent. + + `modern_artist_lexicon` (2026-08-04) is `handle()`'s own `load_lexicon_index()` result, + delivered the same way for the same reason - see `compute_card_evidence`'s own + `modern_artist_lexicon` docstring paragraph for what it feeds, the ARTIST-CROP FALLBACK. """ # 1. tesseract's LSTM engine can multi-thread itself internally via OpenMP - without this, N # worker PROCESSES (not just N threads within one process) would each ALSO spread across @@ -442,9 +451,10 @@ def _init_worker(artist_lexicon: Optional[ArtistLexicon] = None) -> None: # can only ever open a connection this worker owns. Skipped entirely when no lexicon was # supplied - a `PrintingArtistLookup` with no lexicon alongside it can never be consulted # (`_parse_artist_is_contradicted` requires both), so building one would be pure waste. - global _WORKER_ARTIST_LEXICON, _WORKER_PRINTING_ARTIST_LOOKUP + global _WORKER_ARTIST_LEXICON, _WORKER_PRINTING_ARTIST_LOOKUP, _WORKER_MODERN_ARTIST_LEXICON _WORKER_ARTIST_LEXICON = artist_lexicon _WORKER_PRINTING_ARTIST_LOOKUP = None if artist_lexicon is None else build_printing_artist_lookup() + _WORKER_MODERN_ARTIST_LEXICON = modern_artist_lexicon def _get_rss_mb() -> Optional[float]: @@ -645,13 +655,14 @@ def _compute_one_card( list stays picklable exactly as `compute_card_evidence`'s own docstring requires. `()` (the default) means "don't narrow", byte-identical to the pre-2026-07-29 behaviour. - THE OTHER TWO ARTIST INPUTS ARE PROCESS STATE, NOT ARGUMENTS. `artist_lexicon` and - `printing_artist_lookup` are read off `_WORKER_ARTIST_LEXICON`/`_WORKER_PRINTING_ARTIST_LOOKUP`, + THE OTHER ARTIST INPUTS ARE PROCESS STATE, NOT ARGUMENTS. `artist_lexicon`, + `printing_artist_lookup`, and (2026-08-04) `modern_artist_lexicon` are read off + `_WORKER_ARTIST_LEXICON`/`_WORKER_PRINTING_ARTIST_LOOKUP`/`_WORKER_MODERN_ARTIST_LEXICON`, which `_init_worker` set once when this worker process started - see the module docstring for - why each travels the way it does. Both being `None` (an un-initialised process: a direct unit - call, a pool built without the initializer) disables the escalation gate and the - `artist_ocr_name` storage fallback entirely, which is exactly what this command did before - 2026-07-29 - never an error, and never a partially-wired read.""" + why each travels the way it does. All being `None` (an un-initialised process: a direct unit + call, a pool built without the initializer) disables the escalation gate and both + `artist_ocr_name` storage fallbacks entirely, which is exactly what this command did before + 2026-07-29/2026-08-04 - never an error, and never a partially-wired read.""" from cardpicker.image_evidence import compute_card_evidence, persist_evidence wall_started_at = time.monotonic() if profile else None @@ -676,6 +687,7 @@ def _compute_one_card( artist_lexicon=_WORKER_ARTIST_LEXICON, printing_artist_lookup=_WORKER_PRINTING_ARTIST_LOOKUP, card_artist_names=card_artist_names, + modern_artist_lexicon=_WORKER_MODERN_ARTIST_LEXICON, md5_checksum=md5_checksum, sha256_checksum=sha256_checksum, ) @@ -801,6 +813,7 @@ def _run_cohort( known_set_codes: Optional[frozenset[str]] = None, artist_lexicon: Optional[ArtistLexicon] = None, name_artist_lookup: Optional[NameArtistLookup] = None, + modern_artist_lexicon: Optional[LexiconIndex] = None, ) -> tuple[int, int, bool, int, bool, Optional[float]]: """ The decoupled fetch/compute driver itself. Two concurrent executors: @@ -854,6 +867,10 @@ def _run_cohort( `None` (the default, and every test that doesn't thread them) leaves the artist gate and the `artist_ocr_name` storage fallback off, exactly as before 2026-07-29. + `modern_artist_lexicon` (2026-08-04): built ONCE by `handle()` below and handed to the compute + pool's `initializer=` alongside `artist_lexicon` - same lifetime, same route. `None` (the + default) leaves the ARTIST-CROP FALLBACK off - see `compute_card_evidence`'s own docstring. + Returns `(completed, fetch_failures, lockout_hit, short_circuited, rss_limit_hit, peak_rss_mb)` - the same three figures the old single-loop design printed in its final summary line, plus the short-circuit counter (item 1's own "count it during the real run" ask), the RSS-limit flag, and @@ -865,7 +882,7 @@ def _run_cohort( stats = _CohortStats(total=len(cohort_ids), stdout_write=stdout_write, stop_event=stop_event, max_rss_mb=max_rss_mb) with ThreadPoolExecutor(max_workers=fetch_threads) as fetch_pool, ProcessPoolExecutor( - max_workers=workers, initializer=_init_worker, initargs=(artist_lexicon,) + max_workers=workers, initializer=_init_worker, initargs=(artist_lexicon, modern_artist_lexicon) ) as compute_pool: cohort_iter = iter(cohort_ids) outstanding_fetch: "set[Future[Any]]" = set() @@ -1280,6 +1297,10 @@ def priority_key(pair: tuple[int, str]) -> tuple[float, int]: # `run_join_key_calculator` already practise. artist_lexicon = load_artist_lexicon() name_artist_lookup = build_name_artist_lookup() + # ARTIST-CROP FALLBACK (2026-08-04): same "query once, pass through explicitly" + # convention as the two lookups immediately above - see `compute_card_evidence`'s own + # `modern_artist_lexicon` docstring paragraph. + modern_artist_lexicon = load_lexicon_index() # Close the parent's own DB connection(s) before forking the compute pool - belt-and- # braces alongside each compute worker's own _init_worker close_all() call, so the @@ -1307,6 +1328,7 @@ def priority_key(pair: tuple[int, str]) -> tuple[float, int]: known_set_codes=lexicon, artist_lexicon=artist_lexicon, name_artist_lookup=name_artist_lookup, + modern_artist_lexicon=modern_artist_lexicon, ) finally: if profile_file is not None: diff --git a/MPCAutofill/cardpicker/stage_e_dispatch.py b/MPCAutofill/cardpicker/stage_e_dispatch.py index a4abe88b6..1dc261cdf 100644 --- a/MPCAutofill/cardpicker/stage_e_dispatch.py +++ b/MPCAutofill/cardpicker/stage_e_dispatch.py @@ -654,6 +654,7 @@ def _verify_stage_c_chunk(chunk: list[int]) -> Iterable[int]: _compute_pool_artist_lexicon: Any = None _compute_pool_printing_artist_lookup: Any = None _compute_pool_name_artist_lookup: Any = None +_compute_pool_modern_artist_lexicon: Any = None def _stage_c_compute_worker_init( @@ -685,12 +686,14 @@ def _stage_c_compute_worker_init( sufficient for a fresh, independent socket on this worker's own first query. 3. Lookup singletons: ``known_set_codes``, ``load_artist_lexicon``, - ``build_printing_artist_lookup``, and ``build_name_artist_lookup`` are all built once per - worker process (matching the batch-scoped ``_run_stage_c`` convention exactly — the - call sites are identical, just hoisted from per-batch in the parent to per-worker-process - in the pool), and the last two return stateful resolvers with internal caches backed by - DB queries — they MUST be rebuilt in the child rather than passed across the fork boundary, - which would carry a stale cache pointing at the (now-closed) parent's own DB handles. + ``build_printing_artist_lookup``, ``build_name_artist_lookup``, and (2026-08-04) + ``modern_artist_credit.load_lexicon_index`` — the ARTIST-CROP FALLBACK's own lexicon, see + ``compute_card_evidence``'s ``modern_artist_lexicon`` docstring paragraph — are all built + once per worker process (matching the batch-scoped ``_run_stage_c`` convention exactly — + the call sites are identical, just hoisted from per-batch in the parent to per-worker-process + in the pool), and the stateful resolvers among them return caches backed by DB queries — + they MUST be rebuilt in the child rather than passed across the fork boundary, which would + carry a stale cache pointing at the (now-closed) parent's own DB handles. """ import os as _os @@ -702,6 +705,9 @@ def _stage_c_compute_worker_init( ) from cardpicker.collector_line_artist import load_artist_lexicon as _load_artist from cardpicker.local_calculate_verdicts import known_set_codes as _known_set_codes + from cardpicker.modern_artist_credit import ( + load_lexicon_index as _load_modern_artist, + ) # 1. Pin OpenMP thread count — MUST happen before any PIL/tesseract import reaches OpenMP init. _os.environ["OMP_THREAD_LIMIT"] = "1" @@ -718,12 +724,14 @@ def _stage_c_compute_worker_init( global _compute_pool_short_circuit global _compute_pool_lexicon, _compute_pool_artist_lexicon global _compute_pool_printing_artist_lookup, _compute_pool_name_artist_lookup + global _compute_pool_modern_artist_lexicon _compute_pool_short_circuit = short_circuit _compute_pool_lexicon = _known_set_codes() _compute_pool_artist_lexicon = _load_artist() _compute_pool_printing_artist_lookup = _build_printing() _compute_pool_name_artist_lookup = _build_name() + _compute_pool_modern_artist_lexicon = _load_modern_artist() def _stage_c_compute_one_card( @@ -746,7 +754,7 @@ def _stage_c_compute_one_card( completed card from a compute crash and handle each appropriately. All lookup singletons (lexicon, artist_lexicon, printing_artist_lookup, name_artist_lookup, - short_circuit) are read from process-global module variables set by + modern_artist_lexicon, short_circuit) are read from process-global module variables set by _stage_c_compute_worker_init — no second build, no imports at call time beyond PIL and the two evidence functions themselves. """ @@ -769,6 +777,7 @@ def _stage_c_compute_one_card( artist_lexicon=_compute_pool_artist_lexicon, printing_artist_lookup=_compute_pool_printing_artist_lookup, card_artist_names=_compute_pool_name_artist_lookup(card_name), + modern_artist_lexicon=_compute_pool_modern_artist_lexicon, md5_checksum=md5_checksum, sha256_checksum=sha256_checksum, ) diff --git a/MPCAutofill/cardpicker/tests/test_image_evidence.py b/MPCAutofill/cardpicker/tests/test_image_evidence.py index 73ae38bd4..19acf02f4 100644 --- a/MPCAutofill/cardpicker/tests/test_image_evidence.py +++ b/MPCAutofill/cardpicker/tests/test_image_evidence.py @@ -113,6 +113,7 @@ from cardpicker.local_ocr import DEFAULT_CROP_BOX, LEGAL_LINE_CROP_BOX from cardpicker.local_phash import ART_CROP_BOX from cardpicker.models import CardScanLog, CardTagVote, ImageEvidence +from cardpicker.modern_artist_credit import build_lexicon_index from cardpicker.tests.factories import CardFactory, ImageEvidenceFactory, TagFactory @@ -1789,6 +1790,115 @@ def test_a_failed_fetch_still_reports_the_legal_line_as_skipped(self, db, monkey assert "legal_line_raw_text" not in result.fields +class TestExtractCardEvidenceArtistCropFallback: + """2026-08-04, the ARTIST-CROP FALLBACK (see `compute_card_evidence`'s own + `modern_artist_lexicon` docstring paragraph). Real production rows (evidence ids 221241, + 221268, 221274) carry NO collector line and NO legal line at all - an old-border proxy's only + on-card credit is a centred "Illus. " line, OCR'd into `artist_ocr_raw_text` but missed + by the anchor regex to ordinary noise (`Soot Itus.` never matches `_ILLUS_RE`). Neither the + anchor nor `recover_artist_from_card_text` (which reads only the two bottom-print-row fields) + can ever reach that population; `modern_artist_credit.recognize_artist_credit`, re-reading + `artist_ocr_raw_text` itself, can. + + `run_tesseract` backs both the legal-line crop and the artist-crop fallback pass; the two + crops have very different pixel heights (`LEGAL_LINE_CROP_BOX` is a thin strip, + `ARTIST_CROP_BOX` a much taller band), so the stub below distinguishes them by `variant.size` + rather than by call order. + """ + + LEXICON = build_artist_lexicon(["Aaron Miller"]) + MODERN_LEXICON = build_lexicon_index(["Aaron Miller"]) + + @staticmethod + def _lookup(set_code, collector_number): + return None # no printing resolution - these tests are about the READING, not the gate + + @staticmethod + def _run_tesseract_by_crop_height(variant, **kwargs): + if variant.size[1] < 400: + return "" # the legal-line crop: blank, matching "no bottom print row at all" + return "Soot Itus. Aaron Miller ~ *" # the artist crop: the real card-30-shaped OCR text + + def test_recovers_a_name_the_anchor_and_print_row_recovery_both_missed(self, db, monkeypatch): + card = CardFactory(content_phash=1) + image = _build_card_image([(DEFAULT_CROP_BOX, "")]) + monkeypatch.setattr(module, "fetch_card_image", lambda card, dpi=None: image) + monkeypatch.setattr(module, "run_tesseract_text_and_words", lambda image_arg, config: ("", [])) + monkeypatch.setattr(module, "run_tesseract", self._run_tesseract_by_crop_height) + + result = fetch_and_compute_card_evidence_for_tests( + card, + artist_lexicon=self.LEXICON, + printing_artist_lookup=self._lookup, + modern_artist_lexicon=self.MODERN_LEXICON, + ) + + assert result.fields["artist_ocr_name"] == "Aaron Miller" + assert result.fields["illus_anchor_fired"] is False # the anchor genuinely never fired + assert result.fields["collector_line_raw_text"] == "" + assert result.fields["legal_line_raw_text"] == "" + + def test_without_the_lexicon_the_gap_stays_open(self, db, monkeypatch): + """Control: byte-identical inputs with `modern_artist_lexicon` left at its `None` default + - every pre-2026-08-04 caller's behaviour, unchanged.""" + card = CardFactory(content_phash=1) + image = _build_card_image([(DEFAULT_CROP_BOX, "")]) + monkeypatch.setattr(module, "fetch_card_image", lambda card, dpi=None: image) + monkeypatch.setattr(module, "run_tesseract_text_and_words", lambda image_arg, config: ("", [])) + monkeypatch.setattr(module, "run_tesseract", self._run_tesseract_by_crop_height) + + result = fetch_and_compute_card_evidence_for_tests( + card, artist_lexicon=self.LEXICON, printing_artist_lookup=self._lookup + ) + + assert result.fields["artist_ocr_name"] == "" + + def test_the_illus_anchor_still_wins_when_it_fires(self, db, monkeypatch): + """The anchor's own reading always wins - this fallback only ever fills a BLANK value, + same invariant `TestExtractCardEvidenceCollectorLineArtistGate` already pins for the + print-row recovery.""" + card = CardFactory(content_phash=1) + image = _build_card_image([(DEFAULT_CROP_BOX, "")]) + monkeypatch.setattr(module, "fetch_card_image", lambda card, dpi=None: image) + monkeypatch.setattr(module, "run_tesseract_text_and_words", lambda image_arg, config: ("", [])) + monkeypatch.setattr(module, "run_tesseract", lambda variant, **kwargs: "Illus. Ron Spears") + + lexicon = build_artist_lexicon(["Ron Spears", "Aaron Miller"]) + modern_lexicon = build_lexicon_index(["Ron Spears", "Aaron Miller"]) + result = fetch_and_compute_card_evidence_for_tests( + card, artist_lexicon=lexicon, printing_artist_lookup=self._lookup, modern_artist_lexicon=modern_lexicon + ) + + assert result.fields["artist_ocr_name"] == "Ron Spears" + assert result.fields["illus_anchor_fired"] is True + + def test_print_row_recovery_still_wins_over_the_artist_crop_fallback(self, db, monkeypatch): + """Precedence: when the collector/legal-line recovery finds something storable, it is + never second-guessed by the artist-crop fallback, even though the artist crop's own OCR + text names a DIFFERENT, equally real, lexicon artist. The legal-line crop is left BLANK + here (unlike `test_recovers_a_name_...` above) so the print-row recovery's own answer + comes from the collector line alone, not the wider legal-line read winning on a tie-break.""" + card = CardFactory(content_phash=1) + image = _build_card_image([(DEFAULT_CROP_BOX, "")]) + monkeypatch.setattr(module, "fetch_card_image", lambda card, dpi=None: image) + monkeypatch.setattr( + module, "run_tesseract_text_and_words", lambda image_arg, config: ("059/274R\nDMR ¢ EN RON SPEARS", []) + ) + + def _run_tesseract(variant, **kwargs): + return "" if variant.size[1] < 400 else "Aaron Miller" + + monkeypatch.setattr(module, "run_tesseract", _run_tesseract) + + lexicon = build_artist_lexicon(["Ron Spears", "Aaron Miller"]) + modern_lexicon = build_lexicon_index(["Ron Spears", "Aaron Miller"]) + result = fetch_and_compute_card_evidence_for_tests( + card, artist_lexicon=lexicon, printing_artist_lookup=self._lookup, modern_artist_lexicon=modern_lexicon + ) + + assert result.fields["artist_ocr_name"] == "Ron Spears" # the print row, not the art crop + + class TestCollectorLineOcrAttempts: """Direct tests of `_collector_line_ocr_attempts` (issue #259) - the lazy, ordered (image, tesseract_config, tier) generator `collector_line_ocr`'s own loop consumes. The `tier` diff --git a/MPCAutofill/cardpicker/tests/test_run_image_evidence_cohort.py b/MPCAutofill/cardpicker/tests/test_run_image_evidence_cohort.py index 7164c63df..2493b60b1 100644 --- a/MPCAutofill/cardpicker/tests/test_run_image_evidence_cohort.py +++ b/MPCAutofill/cardpicker/tests/test_run_image_evidence_cohort.py @@ -840,6 +840,7 @@ def _stub_compute_card_evidence( artist_lexicon: Any = None, printing_artist_lookup: Any = None, card_artist_names: tuple[str, ...] = (), + modern_artist_lexicon: Any = None, md5_checksum: Optional[str] = None, sha256_checksum: Optional[str] = None, ) -> Any: @@ -887,6 +888,7 @@ def _stub_compute_card_evidence( artist_lexicon: Any = None, printing_artist_lookup: Any = None, card_artist_names: tuple[str, ...] = (), + modern_artist_lexicon: Any = None, md5_checksum: Optional[str] = None, sha256_checksum: Optional[str] = None, ) -> Any: @@ -954,6 +956,7 @@ def _stub_compute_card_evidence( artist_lexicon: Any = None, printing_artist_lookup: Any = None, card_artist_names: tuple[str, ...] = (), + modern_artist_lexicon: Any = None, md5_checksum: Optional[str] = None, sha256_checksum: Optional[str] = None, ) -> Any: @@ -2073,10 +2076,12 @@ def submit(self, fn: Any, *args: Any) -> "Future[Any]": name_artist_lookup=lambda card_name: (f"artist-for-{card_name}",), ) - # The compute pool - and ONLY the compute pool - is initialised with the lexicon. + # The compute pool - and ONLY the compute pool - is initialised with the lexicon. The + # second initargs slot is `modern_artist_lexicon` (2026-08-04) - `None` here since this + # test doesn't thread one through, matching `_run_cohort`'s own default. compute_pools = [pool for pool in constructed if pool["initializer"] is cohort_command._init_worker] assert len(compute_pools) == 1 - assert compute_pools[0]["initargs"] == (lexicon,) + assert compute_pools[0]["initargs"] == (lexicon, None) # `card_artist_names` is the last positional argument of every submission, resolved from # that card's own name in the parent process. diff --git a/MPCAutofill/cardpicker/tests/test_stage_e_dispatch.py b/MPCAutofill/cardpicker/tests/test_stage_e_dispatch.py index 56203ef88..273463a0c 100644 --- a/MPCAutofill/cardpicker/tests/test_stage_e_dispatch.py +++ b/MPCAutofill/cardpicker/tests/test_stage_e_dispatch.py @@ -178,6 +178,7 @@ def _stub( artist_lexicon=None, printing_artist_lookup=None, card_artist_names=(), + modern_artist_lexicon=None, md5_checksum=None, sha256_checksum=None, ): @@ -735,6 +736,7 @@ def _recording_stub( artist_lexicon=None, printing_artist_lookup=None, card_artist_names=(), + modern_artist_lexicon=None, md5_checksum=None, sha256_checksum=None, ): @@ -1968,6 +1970,7 @@ def fake_compute( artist_lexicon: Any = None, printing_artist_lookup: Any = None, card_artist_names: Any = (), + modern_artist_lexicon: Any = None, md5_checksum: Any = None, sha256_checksum: Any = None, ) -> Any: @@ -2086,6 +2089,7 @@ def fake_compute( artist_lexicon: Any = None, printing_artist_lookup: Any = None, card_artist_names: Any = (), + modern_artist_lexicon: Any = None, md5_checksum: Any = None, sha256_checksum: Any = None, ) -> Any: From 408996509381739499da79cad76785fd309cd6ac Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Tue, 4 Aug 2026 18:37:11 +0000 Subject: [PATCH 2/5] fix(tests): thread modern_artist_lexicon through remaining compute_card_evidence stubs test_stage_e_shakedown.py and test_stream_full_catalog.py's _install_ok_stage_c_stub helpers mirrored compute_card_evidence's pre-modern_artist_lexicon signature, missed by the sibling repair in test_stage_e_dispatch.py and test_run_image_evidence_cohort.py (bf17be1b) because neither module was run locally. Same fix, same shape: accept and ignore the new keyword-only param. --- MPCAutofill/cardpicker/tests/test_stage_e_shakedown.py | 1 + MPCAutofill/cardpicker/tests/test_stream_full_catalog.py | 1 + 2 files changed, 2 insertions(+) diff --git a/MPCAutofill/cardpicker/tests/test_stage_e_shakedown.py b/MPCAutofill/cardpicker/tests/test_stage_e_shakedown.py index d9398388e..d8a27f414 100644 --- a/MPCAutofill/cardpicker/tests/test_stage_e_shakedown.py +++ b/MPCAutofill/cardpicker/tests/test_stage_e_shakedown.py @@ -207,6 +207,7 @@ def _stub_compute( artist_lexicon=None, printing_artist_lookup=None, card_artist_names=(), + modern_artist_lexicon=None, md5_checksum=None, sha256_checksum=None, ): diff --git a/MPCAutofill/cardpicker/tests/test_stream_full_catalog.py b/MPCAutofill/cardpicker/tests/test_stream_full_catalog.py index a2e180f57..40e41849f 100644 --- a/MPCAutofill/cardpicker/tests/test_stream_full_catalog.py +++ b/MPCAutofill/cardpicker/tests/test_stream_full_catalog.py @@ -177,6 +177,7 @@ def _stub_compute( artist_lexicon: Any = None, printing_artist_lookup: Any = None, card_artist_names: Any = (), + modern_artist_lexicon: Any = None, md5_checksum: Any = None, sha256_checksum: Any = None, ) -> Any: From e2b65c8e6ff5ec8633ede3e772c11d98686560df Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Tue, 4 Aug 2026 19:32:56 +0000 Subject: [PATCH 3/5] feat(question-feed): expose per-candidate art-crop URL on PrintingCandidate Adds PrintingCandidate.artCropUrl, sourced from the existing CanonicalPrintingMetadata.art_crop_url sidecar field (no new fetch or harvest - the data is already on disk), following the illustrationId precedent in shape and style: - CanonicalCard.serialise_as_printing_candidate populates it, null- tolerant for both the no-sidecar and empty-art_crop_url cases. - PrintingCandidate in both schema_types.py and schema_types.ts gains the optional field (backend from_dict/to_dict, frontend interface + typeMap entry). - docs/features/printing-tags.md's Known gaps section notes the field now exists but nothing consumes it yet - rendering it is future frontend work. Query efficiency: every path that reaches serialise_as_printing_candidate (get_ranked_printing_candidates's two branches, and the ai_vote lookup in question_feed._confirm_suggestion_item) already select_related's printing_metadata for illustrationId's sake, so no new prefetch wiring was needed. A new test proves the candidate-grid serialisation issues zero additional queries regardless of candidate count. This closes the backend half of the gap PR #687 (question-feed-frontend- fixes branch, not yet merged) identified: illustration-clustered candidate tiles have no shared art crop to render instead of each candidate's own full-card scan. Consuming this field in the frontend grid remains out of scope here. --- MPCAutofill/cardpicker/models.py | 7 ++++ MPCAutofill/cardpicker/schema_types.py | 5 +++ .../tests/test_printing_candidates.py | 20 ++++++++++++ .../test_serialise_as_printing_candidate.py | 32 +++++++++++++++++++ docs/features/printing-tags.md | 7 ++++ frontend/src/common/schema_types.ts | 6 ++++ 6 files changed, 77 insertions(+) diff --git a/MPCAutofill/cardpicker/models.py b/MPCAutofill/cardpicker/models.py index d7413bbe6..623e3ad90 100755 --- a/MPCAutofill/cardpicker/models.py +++ b/MPCAutofill/cardpicker/models.py @@ -173,6 +173,13 @@ def serialise_as_printing_candidate(self) -> PrintingCandidate: illustrationId=( str(metadata.illustration_id) if metadata is not None and metadata.illustration_id else None ), + # Scryfall's art-crop image URL for this printing - see + # CanonicalPrintingMetadata.art_crop_url's own docstring for provenance. Null-tolerant + # for the same two reasons illustrationId is above: metadata can be absent entirely, + # and art_crop_url is `blank=True` so a printing can legitimately carry an empty + # value - both collapse to None here so the frontend can fall back to + # mediumThumbnailUrl rather than render a broken image. + artCropUrl=metadata.art_crop_url if metadata is not None and metadata.art_crop_url else None, ) diff --git a/MPCAutofill/cardpicker/schema_types.py b/MPCAutofill/cardpicker/schema_types.py index 302199b3b..9f2848b27 100644 --- a/MPCAutofill/cardpicker/schema_types.py +++ b/MPCAutofill/cardpicker/schema_types.py @@ -94,6 +94,7 @@ class PrintingCandidate(BaseModel): smallThumbnailUrl: str releasedAt: Optional[str] = None illustrationId: Optional[str] = None + artCropUrl: Optional[str] = None @staticmethod def from_dict(obj: Any) -> "PrintingCandidate": @@ -115,6 +116,7 @@ def from_dict(obj: Any) -> "PrintingCandidate": smallThumbnailUrl = from_str(obj.get("smallThumbnailUrl")) releasedAt = from_union([from_none, from_str], obj.get("releasedAt")) illustrationId = from_union([from_none, from_str], obj.get("illustrationId")) + artCropUrl = from_union([from_none, from_str], obj.get("artCropUrl")) return PrintingCandidate( artist, borderColor, @@ -133,6 +135,7 @@ def from_dict(obj: Any) -> "PrintingCandidate": smallThumbnailUrl, releasedAt, illustrationId, + artCropUrl, ) def to_dict(self) -> dict: @@ -156,6 +159,8 @@ def to_dict(self) -> dict: result["releasedAt"] = from_union([from_none, from_str], self.releasedAt) if self.illustrationId is not None: result["illustrationId"] = from_union([from_none, from_str], self.illustrationId) + if self.artCropUrl is not None: + result["artCropUrl"] = from_union([from_none, from_str], self.artCropUrl) return result diff --git a/MPCAutofill/cardpicker/tests/test_printing_candidates.py b/MPCAutofill/cardpicker/tests/test_printing_candidates.py index 241f70596..468c93061 100644 --- a/MPCAutofill/cardpicker/tests/test_printing_candidates.py +++ b/MPCAutofill/cardpicker/tests/test_printing_candidates.py @@ -6,6 +6,7 @@ rank_candidates_by_confidence, ) from cardpicker.tests.factories import ( + CanonicalArtistFactory, CanonicalCardFactory, CanonicalExpansionFactory, CanonicalPrintingMetadataFactory, @@ -108,3 +109,22 @@ def test_explicit_query_overrides_linked_card_browse_mode(self, db): results = get_ranked_printing_candidates(card, "Something Else Entirely") assert results == [other] + + def test_serialising_the_candidate_grid_issues_no_extra_queries_per_candidate(self, db, django_assert_num_queries): + # `PrintingCandidate.artCropUrl`/`illustrationId` both read `CanonicalPrintingMetadata` + # off each candidate - the sidecar must already be `select_related`, or this scales with + # grid size instead of staying flat. `CANDIDATE_RESULT_LIMIT` bounds real grid size at + # 50; this uses 10 to keep the test fast while still exercising more than one row. + card = CardFactory(name="Mountain", searchq="mountain") + artist = CanonicalArtistFactory() + for i in range(10): + printing = CanonicalCardFactory(name="Mountain", artist=artist, collector_number=f"{i:03}") + CanonicalPrintingMetadataFactory(canonical_card=printing, art_crop_url=f"https://example.com/{i}.jpg") + + candidates = get_ranked_printing_candidates(card, None) + assert len(candidates) == 10 + + with django_assert_num_queries(0): + serialised = [candidate.serialise_as_printing_candidate() for candidate in candidates] + + assert all(candidate.artCropUrl is not None for candidate in serialised) diff --git a/MPCAutofill/cardpicker/tests/test_serialise_as_printing_candidate.py b/MPCAutofill/cardpicker/tests/test_serialise_as_printing_candidate.py index 130c2f28e..9aaae704b 100644 --- a/MPCAutofill/cardpicker/tests/test_serialise_as_printing_candidate.py +++ b/MPCAutofill/cardpicker/tests/test_serialise_as_printing_candidate.py @@ -39,3 +39,35 @@ def test_emits_none_when_metadata_exists_but_illustration_id_is_null(self, db): candidate = card.serialise_as_printing_candidate() assert candidate.illustrationId is None + + +class TestSerialiseAsPrintingCandidateArtCropUrl: + def test_emits_art_crop_url_when_metadata_has_one(self, db): + card = CanonicalCardFactory() + CanonicalPrintingMetadataFactory( + canonical_card=card, art_crop_url="https://cards.scryfall.io/art_crop/example.jpg" + ) + + candidate = card.serialise_as_printing_candidate() + + assert candidate.artCropUrl == "https://cards.scryfall.io/art_crop/example.jpg" + + def test_emits_none_when_metadata_sidecar_is_missing_entirely(self, db): + # deliberately no `CanonicalPrintingMetadataFactory` row for this card at all - same + # no-sidecar shape `illustrationId` covers above. + card = CanonicalCardFactory() + + candidate = card.serialise_as_printing_candidate() + + assert candidate.artCropUrl is None + + def test_emits_none_when_metadata_exists_but_art_crop_url_is_empty(self, db): + # `art_crop_url` is `blank=True, default=""`, not nullable - an empty string is the + # legitimate "no crop on file" value, and must collapse to the same optional-absent + # shape as the no-sidecar case above rather than serialising as an empty-string URL. + card = CanonicalCardFactory() + CanonicalPrintingMetadataFactory(canonical_card=card, art_crop_url="") + + candidate = card.serialise_as_printing_candidate() + + assert candidate.artCropUrl is None diff --git a/docs/features/printing-tags.md b/docs/features/printing-tags.md index e47803a4e..4c78e5688 100644 --- a/docs/features/printing-tags.md +++ b/docs/features/printing-tags.md @@ -1944,6 +1944,13 @@ for history (this doc's own established convention — see the `cardPanel.tsx` b ## Known gaps +- **Illustration-grouped tiles still show printing scans, not an art crop**: + `PrintingCandidate.artCropUrl` (`schema_types.py`/`schema_types.ts`, + sourced from the existing `CanonicalPrintingMetadata.art_crop_url` + sidecar field, same provenance as `illustrationId`) now exists on the + questionFeed payload for exactly this purpose — but nothing consumes it + yet. Rendering it in place of `mediumThumbnailUrl` for illustration- + clustered candidate tiles is frontend work, not yet done. - Client-side (local-folder/Google Drive) search gets no re-rank/filter/ match-indicator parity — no ES/DB access on that path. - The starburst/card/chip-ring layout was hand-tuned via iterative diff --git a/frontend/src/common/schema_types.ts b/frontend/src/common/schema_types.ts index 32b1430fd..924a3640d 100644 --- a/frontend/src/common/schema_types.ts +++ b/frontend/src/common/schema_types.ts @@ -154,6 +154,7 @@ export interface QuestionFeedItem { } export interface PrintingCandidate { + artCropUrl?: null | string; artist: string; borderColor: string; canonicalId: string; @@ -2894,6 +2895,11 @@ const typeMap: any = { ), PrintingCandidate: o( [ + { + json: "artCropUrl", + js: "artCropUrl", + typ: u(undefined, u(null, "")), + }, { json: "artist", js: "artist", typ: "" }, { json: "borderColor", js: "borderColor", typ: "" }, { json: "canonicalId", js: "canonicalId", typ: "" }, From 6e35e8c27ceb8f1655c67e1ab6e87c7919082c04 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Tue, 4 Aug 2026 23:04:28 +0000 Subject: [PATCH 4/5] feat(question-feed): render artCropUrl on illustration-clustered tiles; reconcile docs Item 1: merged origin/master (PR #685 was CONFLICTING) - the reconciled Known-gaps bullet takes the branch side (artCropUrl now exists, nothing consumed it) since it is the informed successor to master's #687 bullet. Item 2: illustration-clustered candidate tiles now render PrintingCandidate.artCropUrl in place of mediumThumbnailUrl, falling back to the printing scan when a candidate's metadata sidecar has none. Ungrouped/identify_printing tiles are unaffected. Item 4: the Known-gaps bullet this closes is removed; the shipped behaviour is documented on the existing 'Illustration grouping' architecture bullet instead. --- docs/features/printing-tags.md | 18 ++++--- .../questionFeed/QuestionFeed.test.tsx | 52 +++++++++++++++++++ .../features/questionFeed/QuestionFeed.tsx | 11 ++-- frontend/src/mocks/handlers.ts | 4 ++ frontend/tests/QuestionFeed.spec.ts | 32 ++++++++++++ 5 files changed, 106 insertions(+), 11 deletions(-) diff --git a/docs/features/printing-tags.md b/docs/features/printing-tags.md index 4c78e5688..085204b63 100644 --- a/docs/features/printing-tags.md +++ b/docs/features/printing-tags.md @@ -894,7 +894,16 @@ for history (this doc's own established convention — see the `cardPanel.tsx` b `CandidateGrid` below the clusters, visually identical to Level 2's pre-grouping grid. Ungrouped candidates still call `selectCandidate` with that exact `PrintingCandidate` through the unchanged - `/2/submitPrintingTag/` path. + `/2/submitPrintingTag/` path. Clustered tiles render the candidate's + `artCropUrl` (`PrintingCandidate.artCropUrl`, added for this purpose — + `schema_types.py`/`schema_types.ts`, sourced from + `CanonicalPrintingMetadata.art_crop_url`) in place of + `mediumThumbnailUrl`, falling back to the printing scan when a + candidate's metadata sidecar has no crop — since the vote this cluster + casts is illustration-level, the border/frame/language a full scan + shows is information the vote itself doesn't record. Ungrouped tiles + keep the full scan unconditionally: identifying one specific printing + (the vote `selectCandidate` casts) does need that detail. - **Illustration voting** (issue #503, WTC phase C2; `CardIllustrationVote` itself is #524/#531): tapping a tile inside an illustration cluster now calls `selectIllustrationGroup` instead of `selectCandidate` — it sends @@ -1944,13 +1953,6 @@ for history (this doc's own established convention — see the `cardPanel.tsx` b ## Known gaps -- **Illustration-grouped tiles still show printing scans, not an art crop**: - `PrintingCandidate.artCropUrl` (`schema_types.py`/`schema_types.ts`, - sourced from the existing `CanonicalPrintingMetadata.art_crop_url` - sidecar field, same provenance as `illustrationId`) now exists on the - questionFeed payload for exactly this purpose — but nothing consumes it - yet. Rendering it in place of `mediumThumbnailUrl` for illustration- - clustered candidate tiles is frontend work, not yet done. - Client-side (local-folder/Google Drive) search gets no re-rank/filter/ match-indicator parity — no ES/DB access on that path. - The starburst/card/chip-ring layout was hand-tuned via iterative diff --git a/frontend/src/features/questionFeed/QuestionFeed.test.tsx b/frontend/src/features/questionFeed/QuestionFeed.test.tsx index 8a7fb1ebe..9cd937cc3 100644 --- a/frontend/src/features/questionFeed/QuestionFeed.test.tsx +++ b/frontend/src/features/questionFeed/QuestionFeed.test.tsx @@ -891,5 +891,57 @@ describe("QuestionFeed", () => { }); expect(illustrationVoteCalled).toBe(false); }); + + it("clustered tiles render the candidate's art crop, falling back to the printing scan when absent; ungrouped tiles are unaffected", async () => { + const withArtCrop = { + ...groupedItem, + candidates: [ + { + ...groupedItem.candidates[0], + artCropUrl: "https://example.com/art-crop-1.png", + }, + { ...groupedItem.candidates[1], artCropUrl: null }, + groupedItem.candidates[2], + ], + }; + server.use( + http.get(buildRoute("2/questionFeed/"), () => + HttpResponse.json( + { + item: withArtCrop, + remainingEstimate: { + total: 1, + confirmable: 0, + contested: 0, + fresh: 1, + }, + }, + { status: 200 } + ) + ) + ); + renderFeed(); + await revealCard(); + + const group = await screen.findByTestId( + "question-feed-illustration-group" + ); + expect(within(group).getByAltText("abc 1")).toHaveAttribute( + "src", + "https://example.com/art-crop-1.png" + ); + expect(within(group).getByAltText("xyz 42")).toHaveAttribute( + "src", + groupedItem.candidates[1].mediumThumbnailUrl + ); + + const ungroupedGrid = await screen.findByTestId( + "question-feed-candidate-grid-ungrouped" + ); + expect(within(ungroupedGrid).getByAltText("def 3")).toHaveAttribute( + "src", + groupedItem.candidates[2].mediumThumbnailUrl + ); + }); }); }); diff --git a/frontend/src/features/questionFeed/QuestionFeed.tsx b/frontend/src/features/questionFeed/QuestionFeed.tsx index fc19a6a84..8e3a37340 100644 --- a/frontend/src/features/questionFeed/QuestionFeed.tsx +++ b/frontend/src/features/questionFeed/QuestionFeed.tsx @@ -1456,7 +1456,11 @@ export function QuestionFeed() { // inside it is redundant. Ungrouped tiles have no cluster-level credit, so they keep // this caption at its default (true) - the only place a candidate's artist is still // shown at all for that grid. - showArtistCaption: boolean = true + showArtistCaption: boolean = true, + // Illustration clusters (below) pass candidate.artCropUrl, falling back to the full + // scan when a candidate's metadata sidecar has none - see this function's own comment + // on showArtistCaption for why grouped tiles diverge from the ungrouped default here. + imageUrl: string = candidate.mediumThumbnailUrl ) => ( {`${candidate.expansionCode} @@ -1643,7 +1647,8 @@ export function QuestionFeed() { candidate.illustrationId as string, candidate ), - false + false, + candidate.artCropUrl || candidate.mediumThumbnailUrl ) )} diff --git a/frontend/src/mocks/handlers.ts b/frontend/src/mocks/handlers.ts index 744fd598f..eeafb3c5b 100644 --- a/frontend/src/mocks/handlers.ts +++ b/frontend/src/mocks/handlers.ts @@ -1617,17 +1617,21 @@ export const questionFeedIdentifyPrinting = http.get( // (CanonicalPrintingMetadata.illustration_id is nullable and frequently absent - see // local_illustration.py:137). Built by spreading the existing printingCandidate1/2 fixtures // rather than editing test-constants.ts, which is out of this change's scope. +// candidateA carries an art crop (the common case); candidateB shares its illustration but +// has none (a metadata sidecar gap) - together they cover both the swap and its fallback. export const illustrationGroupCandidateA: PrintingCandidate = { ...printingCandidate1, identifier: "illustration-group-candidate-a", collectorNumber: "101", illustrationId: "illustration-shared", + artCropUrl: "https://example.com/art-crop-a.png", }; export const illustrationGroupCandidateB: PrintingCandidate = { ...printingCandidate2, identifier: "illustration-group-candidate-b", collectorNumber: "102", illustrationId: "illustration-shared", + artCropUrl: null, }; export const illustrationGroupCandidateC: PrintingCandidate = { ...printingCandidate1, diff --git a/frontend/tests/QuestionFeed.spec.ts b/frontend/tests/QuestionFeed.spec.ts index ac8b98c5b..709941dd5 100644 --- a/frontend/tests/QuestionFeed.spec.ts +++ b/frontend/tests/QuestionFeed.spec.ts @@ -298,6 +298,38 @@ test.describe("question feed - Level 2 illustration grouping", () => { ).toHaveCount(0); }); + test("clustered tiles render each candidate's art crop, falling back to the printing scan when absent; ungrouped tiles keep the printing scan regardless", async ({ + page, + network, + }) => { + network.use( + questionFeedIdentifyPrintingGroupedByIllustration, + ...defaultHandlers + ); + await loadPageWithDefaultBackend(page, "whatsthat"); + + const group = page.getByTestId("question-feed-illustration-group"); + await expect( + group.locator( + `[data-card-identifier="${illustrationGroupCandidateA.identifier}"] img` + ) + ).toHaveAttribute("src", illustrationGroupCandidateA.artCropUrl as string); + await expect( + group.locator( + `[data-card-identifier="${illustrationGroupCandidateB.identifier}"] img` + ) + ).toHaveAttribute("src", illustrationGroupCandidateB.mediumThumbnailUrl); + + const ungroupedGrid = page.getByTestId( + "question-feed-candidate-grid-ungrouped" + ); + await expect( + ungroupedGrid.locator( + `[data-card-identifier="${illustrationGroupCandidateC.identifier}"] img` + ) + ).toHaveAttribute("src", illustrationGroupCandidateC.mediumThumbnailUrl); + }); + // Issue #503 (WTC phase C2) / #524 - supersedes this describe block's former "selecting a // grouped candidate submits the identical payload to the identical endpoint as an ungrouped // one" test (see .github/coverage-acks.txt for the rename ack). That title asserted phase From 735bb679e39e2359d0878928641dab56ede2da43 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Tue, 4 Aug 2026 23:59:05 +0000 Subject: [PATCH 5/5] feat(image-evidence): bump artist_ocr extractor to v3 (PR #685) modern_artist_credit.recognize_artist_credit lands in this PR as a third artist fallback inside compute_card_evidence, but without a version bump the fix reaches no existing row: Stage C's resume filter (MANIFEST_EXTRACTOR_CURRENT_VERSIONS, issue #509) compares stored extractor_versions against the manifest and skips any row whose value already matches, and all 230,378 production rows currently carry artist_ocr: artist-ocr-v2. Bumps ARTIST_OCR_EXTRACTOR_VERSION and its MANIFEST_EXTRACTOR_CURRENT_VERSIONS counterpart to artist-ocr-v3, with a dedicated version-history comment on the constant (the shared v1->v2 OCR-engine-swap comment above it now covers only collector_line_ocr/collector_line_tsv, which are not bumped here). Updates the three test fixtures asserting the literal "artist-ocr-v2" string to match. --- MPCAutofill/cardpicker/image_evidence.py | 24 ++++++++++++------- .../commands/run_image_evidence_cohort.py | 2 +- .../tests/test_local_attribute_chip_cast.py | 2 +- .../tests/test_local_calculate_verdicts.py | 4 ++-- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/MPCAutofill/cardpicker/image_evidence.py b/MPCAutofill/cardpicker/image_evidence.py index 7f947df9a..77c2b9964 100644 --- a/MPCAutofill/cardpicker/image_evidence.py +++ b/MPCAutofill/cardpicker/image_evidence.py @@ -290,16 +290,22 @@ LAYOUT_CLASS_EXTRACTOR_VERSION = "layout-class-v1" CROP_COORDINATES_EXTRACTOR_VERSION = "crop-coordinates-v1" # v1 -> v2 (issue #480's combined pass, THE FLIP: settings.OCR_ENGINE default -> "tesserocr" - -# see settings.py's own comment). These three extractors are the ones whose STORED VALUE actually -# depends on which OCR engine produced it (they all read through `local_ocr. -# run_tesseract_text_and_words`, which now runs on tesserocr's differently-compiled tesseract/ -# leptonica build by default - issue #423's spike already found byte-identical output is -# structurally unreachable given that vendored-build mismatch). Bumping the version is what makes -# every existing row under the OLD tag stale, so the next pass re-extracts it under the new -# engine rather than silently mixing two engines' output under one provenance label (issue #480's -# correction comment: "Engine swap WITHOUT a version bump is forbidden"). +# see settings.py's own comment). These extractors' STORED VALUE actually depends on which OCR +# engine produced it (they read through `local_ocr.run_tesseract_text_and_words`, which now runs +# on tesserocr's differently-compiled tesseract/leptonica build by default - issue #423's spike +# already found byte-identical output is structurally unreachable given that vendored-build +# mismatch). Bumping the version is what makes every existing row under the OLD tag stale, so the +# next pass re-extracts it under the new engine rather than silently mixing two engines' output +# under one provenance label (issue #480's correction comment: "Engine swap WITHOUT a version +# bump is forbidden"). COLLECTOR_LINE_OCR_EXTRACTOR_VERSION = "collector-line-ocr-v2" -ARTIST_OCR_EXTRACTOR_VERSION = "artist-ocr-v2" +# v2 -> v3 (PR #685): wires modern_artist_credit.recognize_artist_credit in as a third artist +# fallback inside compute_card_evidence, recovering a name for ~19,478 cards that previously +# stored none. The extractor's own OCR pass is unchanged from v2 (still issue #480's tesserocr +# engine) - this bump exists solely so Stage C's version-aware resume filter +# (MANIFEST_EXTRACTOR_CURRENT_VERSIONS) selects the whole catalogue for one re-extraction pass +# under the new fallback, per issue #509's stale-value comparison. +ARTIST_OCR_EXTRACTOR_VERSION = "artist-ocr-v3" COLLECTOR_LINE_TSV_EXTRACTOR_VERSION = "collector-line-tsv-v2" # NOT bumped: symbol_region is a raw phash of a crop region (imagehash, no tesseract call at all) # - engine-independent by construction. diff --git a/MPCAutofill/cardpicker/management/commands/run_image_evidence_cohort.py b/MPCAutofill/cardpicker/management/commands/run_image_evidence_cohort.py index 691be3503..903ffbb69 100644 --- a/MPCAutofill/cardpicker/management/commands/run_image_evidence_cohort.py +++ b/MPCAutofill/cardpicker/management/commands/run_image_evidence_cohort.py @@ -330,7 +330,7 @@ "layout_class": "layout-class-v1", "crop_coordinates": "crop-coordinates-v1", "collector_line_ocr": "collector-line-ocr-v2", - "artist_ocr": "artist-ocr-v2", + "artist_ocr": "artist-ocr-v3", "collector_line_tsv": "collector-line-tsv-v2", "artbox_phash": "artbox-phash-v1", "symbol_region": "symbol-region-v1", diff --git a/MPCAutofill/cardpicker/tests/test_local_attribute_chip_cast.py b/MPCAutofill/cardpicker/tests/test_local_attribute_chip_cast.py index 1ee8e4809..237ffc7f6 100644 --- a/MPCAutofill/cardpicker/tests/test_local_attribute_chip_cast.py +++ b/MPCAutofill/cardpicker/tests/test_local_attribute_chip_cast.py @@ -42,7 +42,7 @@ _COMPLETE_EXTRACTOR_VERSIONS = { "collector_line_ocr": "collector-line-ocr-v2", - "artist_ocr": "artist-ocr-v2", + "artist_ocr": "artist-ocr-v3", "geometry_bleed": "geometry-bleed-v1", } diff --git a/MPCAutofill/cardpicker/tests/test_local_calculate_verdicts.py b/MPCAutofill/cardpicker/tests/test_local_calculate_verdicts.py index 0d2031343..d23329ee0 100644 --- a/MPCAutofill/cardpicker/tests/test_local_calculate_verdicts.py +++ b/MPCAutofill/cardpicker/tests/test_local_calculate_verdicts.py @@ -132,7 +132,7 @@ def _evidence(card, **overrides): # (`FRAME_CHECK_REQUIRED_EXTRACTOR_KEYS`), and a default that silently omitted it would # turn every frame-veto test into a no-op. `TestFrameVetoRequiresArtistOcr` overrides this # to exercise the absent case deliberately. - extractor_versions={"collector_line_ocr": "collector-line-ocr-v1", "artist_ocr": "artist-ocr-v2"}, + extractor_versions={"collector_line_ocr": "collector-line-ocr-v1", "artist_ocr": "artist-ocr-v3"}, collector_line_raw_text="", collector_line_set_code="", collector_line_collector_number="", @@ -3500,7 +3500,7 @@ def test_the_same_card_is_still_vetoed_once_artist_ocr_has_run(self, db): anchor - a real negative - so "modern" is a genuine reading and the veto fires exactly as it always did. Same card, same evidence, one extra manifest key.""" printing, card, candidates, evidence = self._old_frame_setup( - {"collector_line_ocr": "collector-line-ocr-v2", "artist_ocr": "artist-ocr-v2"} + {"collector_line_ocr": "collector-line-ocr-v2", "artist_ocr": "artist-ocr-v3"} ) verdict = calculate_join_key_verdict(card.pk, evidence, candidates)