From 3a082d77a0e6c008ab24e46c88060f2637760dc5 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Thu, 16 Jul 2026 03:48:01 +0000 Subject: [PATCH 1/4] Two fast-follows: expansion_hint narrowing, name-frequency elimination expansion_hint candidate narrowing: uses the already-populated Card.expansion_hint field to narrow the candidate list OCR/phash/ fallback consider, unlocking phash for 407 cards currently skipped on too-many-candidates. Scoped to engine-matching only - never touches coverage/ordering queries. Name-frequency elimination: new run_name_frequency_elimination + management command, votes for a card when its name has exactly one uncovered printing AND exactly one unresolved eligible card - the 1:1 gate is what makes the deduction sound (a name with multiple unresolved cards and one uncovered printing doesn't tell you which card is the missing one). 1,678 real cards qualify under the safe gate, measured against the full catalog. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016i9S7LQsCL3FGaih3ZTRBJ --- .../local_identify_printing_tags.py | 141 +++++++++++- .../local_name_frequency_elimination.py | 56 +++++ .../test_local_identify_printing_tags.py | 202 ++++++++++++++++++ docs/features/printing-tags.md | 53 +++++ 4 files changed, 451 insertions(+), 1 deletion(-) create mode 100644 MPCAutofill/cardpicker/management/commands/local_name_frequency_elimination.py diff --git a/MPCAutofill/cardpicker/local_identify_printing_tags.py b/MPCAutofill/cardpicker/local_identify_printing_tags.py index 2fc127632..535d94a7f 100644 --- a/MPCAutofill/cardpicker/local_identify_printing_tags.py +++ b/MPCAutofill/cardpicker/local_identify_printing_tags.py @@ -272,7 +272,7 @@ def select_candidates( for card in ( _eligible_base_queryset(anonymous_id, exclude_source_pks) .exclude(dpi__lt=RESOLUTION_FLOOR_DPI) - .only("pk", "name", "identifier", "source_id") + .only("pk", "name", "identifier", "source_id", "expansion_hint") .order_by("pk") .iterator(chunk_size=5000) ): @@ -470,6 +470,37 @@ def run_phash_for_card( DEFAULT_WORKERS = 2 +def _narrow_candidates_by_expansion_hint(selected: SelectedCard) -> SelectedCard: + """Fast-follow (2026-07-16): a confidence PRIOR, not an entailment - narrows the candidate + list an engine considers when the card's own `expansion_hint` (extracted from its filename + at import time by `cardpicker.tags.Tags.extract` - a lone set-code bracket token that + didn't resolve a direct CanonicalCard match, e.g. "[UNF]" with no collector number) matches + at least one of its name's real candidates. Never narrows to empty: if the hint matches zero + candidates (a real, measured ~9% data-quality case - the hint may be stale or mismatched), + the full candidate list is used instead, exactly as if there were no hint at all - narrowing + that made matching IMPOSSIBLE would be worse than not narrowing. + + Scoped to engine-matching ONLY - never call this from select_candidates/ + compute_covered_printing_pks/anything computing coverage or ordering, which need the true, + unnarrowed candidate set to stay correct. The returned SelectedCard is a LOCAL substitute + used only for this card's own OCR/phash/fallback calls within _compute_card; nothing + downstream (run_pilot's own all_selected_by_card_id) ever sees the narrowed version. + + Real yield (measured live, 2026-07-15): of 2,466 pilot-eligible cards with a real + expansion_hint, 645 currently exceed PHASH_MAX_CANDIDATES and get skipped entirely - + narrowing brings 407 of those back under the cap, giving phash a real shot where it + currently never runs at all. OCR's own exact-match logic doesn't benefit from narrowing + (a smaller candidate list doesn't change whether a parsed code+number is IN it) - this is a + phash-only unlock in practice, though harmless to apply uniformly to all three engines.""" + hint = selected.card.expansion_hint + if not hint: + return selected + narrowed = [c for c in selected.candidates if c.expansion_code == hint] + if not narrowed: + return selected + return SelectedCard(card=selected.card, candidates=narrowed) + + def _compute_card( selected: SelectedCard, ocr_selected_ids: set[int], @@ -501,6 +532,11 @@ def _compute_card( image = fetch_card_image(selected.card, fetch_dpi) ocr_raw_texts: list[str] = [] + # fast-follow (2026-07-16): narrow the candidate list every engine below sees, using this + # card's own expansion_hint if it has one - `selected.card`/`card_id` above still reference + # the ORIGINAL card either way; only the candidate list used for matching changes. + selected = _narrow_candidates_by_expansion_hint(selected) + outcome.image_fetched = image is not None bleed_class = local_fallback.classify_bleed_edge(image) if image is not None else None outcome.bleed_class = bleed_class @@ -1153,6 +1189,105 @@ def propagate_cluster_vote( return results, attributes +# Fast-follow (2026-07-16): name-frequency elimination - see run_name_frequency_elimination's +# own docstring for the full design rationale (in particular the SAFE 1:1 gate that makes this +# sound, not just "one uncovered printing"). +NAME_FREQUENCY_ANONYMOUS_ID = "local-name-frequency-v1" +# Deliberately modest relative to OCR/phash's own confidences (0.85/0.75/0.8) - this is a purely +# structural deduction (no visual confirmation of THIS card at all), weaker evidence than an +# engine that actually looked at the image, even though the 1:1 gate makes it sound. +NAME_FREQUENCY_CONFIDENCE = 0.6 + + +@dataclass +class NameFrequencyResult: + dry_run: bool = False + votes_written: int = 0 + gate_violations: list[int] = field(default_factory=list) + + +def run_name_frequency_elimination(dry_run: bool = False, batch_size: int = DEFAULT_BATCH_SIZE) -> NameFrequencyResult: + """Fast-follow (2026-07-16): for a NAME where exactly one of its printings remains + uncovered (see compute_covered_printing_pks) AND exactly one pilot-eligible card is + unresolved for that name, the match is deducible by elimination alone - no image fetch, no + OCR/phash, no visual disambiguation needed at all. + + The SAFE gate is "exactly one uncovered printing AND exactly one unresolved-eligible card", + not just "exactly one uncovered printing" - a name can have one uncovered printing while + SEVERAL unresolved cards share that name, in which case elimination does NOT tell you WHICH + of those cards is the missing one (any of the others could just as easily be a redundant + depiction of an ALREADY-covered printing, uploaded by a different source). Gating on "and + exactly one unresolved card too" is what makes the deduction airtight; it is not a + nice-to-have refinement, it is the difference between a sound inference and a coin flip. + Measured live against the full (not sampled) catalog, 2026-07-16: 2,076 names have exactly + one uncovered printing; only 1,678 of those also have exactly one unresolved eligible card. + The naive, ungated version would have voted - incorrectly, on average - for the other ~400 + names' multiple candidate cards. + + Still just a VOTE (this function's own anonymous_id), never a direct resolve - same + consensus/gate-check discipline as every other engine in this module. Reuses + _eligible_base_queryset for the exact same base eligibility rules (unresolved, no confirmed + match, card_type=CARD, not deductive-backfill-covered, no custom-art/non-english tag) plus + this function's own anonymous_id for idempotence, and the SAME batch-flush + gate-check + pattern as run_pilot (a kill loses at most one batch; a plain re-invocation resumes cleanly). + """ + covered_printing_pks = compute_covered_printing_pks() + index = CandidateNameIndex() + + cards_by_name: dict[str, list[int]] = collections.defaultdict(list) + for card_id, name in ( + _eligible_base_queryset(NAME_FREQUENCY_ANONYMOUS_ID) + .values_list("pk", "name") + .order_by("pk") + .iterator(chunk_size=5000) + ): + cards_by_name[name].append(card_id) + + result = NameFrequencyResult(dry_run=dry_run) + votes_batch: list[CardPrintingTag] = [] + batch_written_card_ids: list[int] = [] + + def flush() -> None: + nonlocal votes_batch, batch_written_card_ids + if dry_run: + votes_batch, batch_written_card_ids = [], [] + return + if votes_batch: + CardPrintingTag.objects.bulk_create(votes_batch) + if batch_written_card_ids: + result.gate_violations.extend(verify_zero_resolutions(batch_written_card_ids)) + votes_batch, batch_written_card_ids = [], [] + + for name, card_ids in cards_by_name.items(): + if len(card_ids) != 1: + continue + candidates = index.candidates_for(name) + if not candidates: + continue + uncovered = [c for c in candidates if c.pk not in covered_printing_pks] + if len(uncovered) != 1: + continue + + votes_batch.append( + CardPrintingTag( + card_id=card_ids[0], + printing_id=uncovered[0].pk, + is_no_match=False, + anonymous_id=NAME_FREQUENCY_ANONYMOUS_ID, + source=VoteSource.OCR, + confidence=NAME_FREQUENCY_CONFIDENCE, + ) + ) + batch_written_card_ids.append(card_ids[0]) + result.votes_written += 1 + + if len(batch_written_card_ids) >= batch_size: + flush() + + flush() + return result + + def verify_zero_resolutions(card_ids: list[int], batch_size: int = 2000) -> list[int]: """Identical rationale/mechanism to cardpicker.deductive_backfill.verify_zero_resolutions - the *pure* resolve_printing (never resolve_and_persist_printing, which must never itself @@ -1195,5 +1330,9 @@ def verify_zero_resolutions(card_ids: list[int], batch_size: int = 2000) -> list "PilotResult", "AttributeReport", "run_pilot", + "NAME_FREQUENCY_ANONYMOUS_ID", + "NAME_FREQUENCY_CONFIDENCE", + "NameFrequencyResult", + "run_name_frequency_elimination", "verify_zero_resolutions", ] diff --git a/MPCAutofill/cardpicker/management/commands/local_name_frequency_elimination.py b/MPCAutofill/cardpicker/management/commands/local_name_frequency_elimination.py new file mode 100644 index 000000000..21833453b --- /dev/null +++ b/MPCAutofill/cardpicker/management/commands/local_name_frequency_elimination.py @@ -0,0 +1,56 @@ +from typing import Any + +from django.core.management.base import BaseCommand, CommandError + +from cardpicker.local_identify_printing_tags import run_name_frequency_elimination + + +class Command(BaseCommand): + help = ( + "Fast-follow (see docs/features/printing-tags.md Stage 8): casts OCR-weight votes for " + "cards whose name has exactly one uncovered printing AND exactly one unresolved " + "pilot-eligible card - a pure structural deduction, no image fetch or OCR/phash " + "involved. Never resolves a card by itself (the human-backed consensus gate still " + "applies) - see run_name_frequency_elimination's own docstring for the safety gate " + "that makes this deduction sound." + ) + + def add_arguments(self, parser: Any) -> None: + parser.add_argument( + "--dry-run", + action="store_true", + default=False, + help="Evaluate without writing anything or running the gate check.", + ) + parser.add_argument( + "--batch-size", + type=int, + default=25, + help="Flush votes to the DB (and run the gate check) every this many cards, same " + "checkpointing convention as local_identify_printing_tags. Default: 25.", + ) + + def handle(self, *args: Any, **kwargs: Any) -> None: + dry_run = kwargs["dry_run"] + batch_size = kwargs["batch_size"] + + mode = "DRY RUN" if dry_run else "WRITE" + print(f"[{mode}] local_name_frequency_elimination --batch-size={batch_size}") + + result = run_name_frequency_elimination(dry_run=dry_run, batch_size=batch_size) + + print(f"votes written: {result.votes_written}") + + if dry_run: + print("Dry run - nothing written, gate check not run.") + return + + if result.gate_violations: + raise CommandError( + f"GATE VIOLATION: {len(result.gate_violations)} card(s) resolved after a " + f"machine-only vote, which should be structurally impossible - STOP and " + f"investigate before continuing. Affected card pks: {result.gate_violations[:50]}" + + (" (truncated)" if len(result.gate_violations) > 50 else "") + ) + + print(f"Gate check passed: 0/{result.votes_written} affected cards resolved.") diff --git a/MPCAutofill/cardpicker/tests/test_local_identify_printing_tags.py b/MPCAutofill/cardpicker/tests/test_local_identify_printing_tags.py index faa920936..37c40c25f 100644 --- a/MPCAutofill/cardpicker/tests/test_local_identify_printing_tags.py +++ b/MPCAutofill/cardpicker/tests/test_local_identify_printing_tags.py @@ -17,6 +17,8 @@ from cardpicker.local_fallback import FALLBACK_ANONYMOUS_ID from cardpicker.local_identify_printing_tags import ( DEDUCTIVE_BACKFILL_ANONYMOUS_ID, + NAME_FREQUENCY_ANONYMOUS_ID, + NAME_FREQUENCY_CONFIDENCE, OCR_ANONYMOUS_ID, PHASH_ANONYMOUS_ID, RESOLUTION_FLOOR_DPI, @@ -26,6 +28,7 @@ compute_own_image_clusters, count_below_resolution_floor, get_worker_image_url, + run_name_frequency_elimination, run_pilot, select_candidates, verify_zero_resolutions, @@ -152,6 +155,85 @@ def test_more_uncovered_candidates_come_before_fewer_when_both_fully_uncovered(s assert [s.card.pk for s in selected] == [multi.pk, single.pk] +class TestExpansionHintNarrowing: + """Fast-follow (2026-07-16): _narrow_candidates_by_expansion_hint narrows the candidate + list an engine considers using Card.expansion_hint (already populated at import time by + cardpicker.tags.Tags.extract - not a new field, just newly wired into the pilot).""" + + def test_no_hint_returns_selected_unchanged(self, db): + import cardpicker.local_identify_printing_tags as module + + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="aaa")) + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="bbb")) + card = CardFactory(name="Forest", expansion_hint="") + index = CandidateNameIndex() + selected = module.SelectedCard(card=card, candidates=index.candidates_for("Forest")) + + narrowed = module._narrow_candidates_by_expansion_hint(selected) + + assert narrowed is selected + assert len(narrowed.candidates) == 2 + + def test_hint_narrows_to_only_matching_candidates(self, db): + import cardpicker.local_identify_printing_tags as module + + expansion_bbb = CanonicalExpansionFactory(code="bbb") + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="aaa")) + CanonicalCardFactory(name="Forest", expansion=expansion_bbb) + CanonicalCardFactory(name="Forest", expansion=expansion_bbb) + card = CardFactory(name="Forest", expansion_hint="bbb") + index = CandidateNameIndex() + selected = module.SelectedCard(card=card, candidates=index.candidates_for("Forest")) + assert len(selected.candidates) == 3 + + narrowed = module._narrow_candidates_by_expansion_hint(selected) + + assert len(narrowed.candidates) == 2 + assert all(c.expansion_code == "bbb" for c in narrowed.candidates) + assert narrowed.card is card + + def test_hint_matching_zero_candidates_falls_back_to_full_list(self, db): + # a real, measured data-quality case: the hint doesn't match anything in this name's + # actual candidate pool - narrowing to empty would make matching IMPOSSIBLE, strictly + # worse than not narrowing at all. + import cardpicker.local_identify_printing_tags as module + + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="aaa")) + card = CardFactory(name="Forest", expansion_hint="zzz") + index = CandidateNameIndex() + selected = module.SelectedCard(card=card, candidates=index.candidates_for("Forest")) + + narrowed = module._narrow_candidates_by_expansion_hint(selected) + + assert len(narrowed.candidates) == 1 + + def test_phash_unlocked_when_narrowing_crosses_under_the_candidate_cap(self, db, monkeypatch): + # the real, measured benefit: a name with MORE than PHASH_MAX_CANDIDATES total + # printings gets skipped entirely ("too-many-candidates") - but if this card's own + # expansion_hint narrows it down to a small handful, phash gets a real shot instead. + import cardpicker.local_identify_printing_tags as module + + for i in range(module.PHASH_MAX_CANDIDATES + 3): + CanonicalCardFactory(name="Beast", expansion=CanonicalExpansionFactory(code=f"e{i:02}")) + CanonicalCardFactory(name="Beast", expansion=CanonicalExpansionFactory(code="hnt")) + CardFactory(name="Beast", expansion_hint="hnt") + + phash_call_candidate_counts: list[int] = [] + + def recording_run_phash_for_card(selected, image, threshold, margin, max_candidates, bleed_class=None): + phash_call_candidate_counts.append(len(selected.candidates)) + return None, "no-clear-winner" + + monkeypatch.setattr(module, "fetch_card_image", lambda card, dpi=None: Image.new("RGB", (750, 1050))) + monkeypatch.setattr(module, "run_phash_for_card", recording_run_phash_for_card) + + run_pilot(engine="phash", limit=10, dry_run=True, nice=False) + + # narrowed to just the "hnt" candidate (1), not the full 13+ - phash actually ran + # (recorded a call) instead of being skipped at selection-adjacent "too-many-candidates". + assert phash_call_candidate_counts == [1] + + class TestCoveragePriority: """Addendum item 1 (2026-07-15): coverage-gap + demand ordering, the full 5-key tuple (zero-covered first, descending uncovered count, demand rank, fewer candidates, pk).""" @@ -739,6 +821,126 @@ def fake_ocr(selected, image, crop_box, bleed_class=None): assert attributes.uncovered_printings_closed == 0 +class TestNameFrequencyElimination: + """Fast-follow (2026-07-16): run_name_frequency_elimination's SAFE 1:1 gate - exactly one + uncovered printing AND exactly one unresolved-eligible card for that name - not just "one + uncovered printing" (which is unsound whenever more than one unresolved card shares a name; + see the function's own docstring for the full rationale, backed by a live measurement).""" + + def test_votes_for_the_single_uncovered_printing_when_exactly_one_card_and_one_gap(self, db): + covered_printing = CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="aaa")) + uncovered_printing = CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="bbb")) + CardFactory(canonical_card=covered_printing) # confirms "aaa" as covered + card = CardFactory(name="Forest") # the single unresolved card for this name + + result = run_name_frequency_elimination(dry_run=False) + + assert result.votes_written == 1 + vote = CardPrintingTag.objects.get(card=card, anonymous_id=NAME_FREQUENCY_ANONYMOUS_ID) + assert vote.printing_id == uncovered_printing.pk + assert vote.confidence == NAME_FREQUENCY_CONFIDENCE + assert vote.is_no_match is False + + def test_does_not_vote_when_multiple_unresolved_cards_share_the_name(self, db): + # the unsafe case this gate exists specifically to exclude: TWO unresolved cards for + # "Forest", only one uncovered printing - elimination can't tell you which card (if + # either) is the missing one, so it must abstain for BOTH, not guess for either. + covered_printing = CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="aaa")) + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="bbb")) + CardFactory(canonical_card=covered_printing) + CardFactory(name="Forest") + CardFactory(name="Forest") + + result = run_name_frequency_elimination(dry_run=False) + + assert result.votes_written == 0 + assert not CardPrintingTag.objects.filter(anonymous_id=NAME_FREQUENCY_ANONYMOUS_ID).exists() + + def test_does_not_vote_when_more_than_one_printing_is_uncovered(self, db): + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="aaa")) + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="bbb")) + CardFactory(name="Forest") # single unresolved card, but nothing covered at all + + result = run_name_frequency_elimination(dry_run=False) + + assert result.votes_written == 0 + + def test_does_not_vote_when_fully_covered(self, db): + printing = CanonicalCardFactory(name="Forest") + CardFactory(canonical_card=printing) + CardFactory(name="Forest") + + result = run_name_frequency_elimination(dry_run=False) + + assert result.votes_written == 0 + + def test_dry_run_writes_nothing(self, db): + covered_printing = CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="aaa")) + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="bbb")) + CardFactory(canonical_card=covered_printing) + CardFactory(name="Forest") + + result = run_name_frequency_elimination(dry_run=True) + + assert result.votes_written == 1 # counted, even though nothing is persisted + assert not CardPrintingTag.objects.exists() + + def test_idempotent_on_a_second_invocation(self, db): + covered_printing = CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="aaa")) + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="bbb")) + CardFactory(canonical_card=covered_printing) + CardFactory(name="Forest") + + first = run_name_frequency_elimination(dry_run=False) + second = run_name_frequency_elimination(dry_run=False) + + assert first.votes_written == 1 + assert second.votes_written == 0 + assert CardPrintingTag.objects.filter(anonymous_id=NAME_FREQUENCY_ANONYMOUS_ID).count() == 1 + + def test_excludes_tokens_and_cardbacks(self, db): + covered_printing = CanonicalCardFactory(name="Beast", expansion=CanonicalExpansionFactory(code="aaa")) + CanonicalCardFactory(name="Beast", expansion=CanonicalExpansionFactory(code="bbb")) + CardFactory(canonical_card=covered_printing) + CardFactory(name="Beast", card_type=CardTypes.TOKEN) + + result = run_name_frequency_elimination(dry_run=False) + + assert result.votes_written == 0 + + +class TestNameFrequencyEliminationCommand: + def test_dry_run_writes_nothing(self, db, capsys): + from django.core.management import call_command + + covered_printing = CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="aaa")) + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="bbb")) + CardFactory(canonical_card=covered_printing) + CardFactory(name="Forest") + + call_command("local_name_frequency_elimination", "--dry-run") + + printed = capsys.readouterr().out + assert "[DRY RUN]" in printed + assert "votes written: 1" in printed + assert not CardPrintingTag.objects.exists() + + def test_real_run_writes_and_passes_gate_check(self, db, capsys): + from django.core.management import call_command + + covered_printing = CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="aaa")) + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="bbb")) + CardFactory(canonical_card=covered_printing) + card = CardFactory(name="Forest") + + call_command("local_name_frequency_elimination") + + printed = capsys.readouterr().out + assert "[WRITE]" in printed + assert "Gate check passed: 0/1 affected cards resolved." in printed + assert CardPrintingTag.objects.filter(card=card, anonymous_id=NAME_FREQUENCY_ANONYMOUS_ID).exists() + + class TestRunPilotSourceExclusion: def test_excluded_sources_cards_never_reach_the_engine(self, db, monkeypatch): excluded_source = SourceFactory() diff --git a/docs/features/printing-tags.md b/docs/features/printing-tags.md index 404750df5..f73a92755 100644 --- a/docs/features/printing-tags.md +++ b/docs/features/printing-tags.md @@ -2042,3 +2042,56 @@ trigger), PR #19's disposition (owner's convenience). **Full-catalog run: not yet fired.** This report is the synthesizing deliverable requested before that authorization - awaiting explicit owner go-ahead. + +## Two fast-follows, built after HOLD #2 (2026-07-16) + +Both researched and sized before building (see the HOLD #2 section above and this doc's earlier +feasibility notes) - neither required schema changes, both reuse already-existing, already- +populated data. + +### `expansion_hint` candidate narrowing + +`_narrow_candidates_by_expansion_hint` (`local_identify_printing_tags.py`) narrows the +candidate list every engine considers, using `Card.expansion_hint` - a field that already +existed and is already populated at import time by `cardpicker.tags.Tags.extract` (a lone +set-code bracket token in the filename that didn't resolve a direct match, e.g. `[UNF]` with no +collector number). Not a new signal - just newly wired into the pilot; `deductive_backfill`'s +own D2 tier already trusts this same field for direct resolution when it narrows to exactly +one candidate. + +A confidence PRIOR, not an entailment: narrows the list passed to `run_ocr_for_card`/ +`run_phash_for_card`/`run_fallback_for_card` inside `_compute_card` only - never touches +`select_candidates`'s ordering, `compute_covered_printing_pks`, or the +`uncovered_printings_closed` metric, all of which need the true, unnarrowed candidate set to +stay correct. Never narrows to empty: if the hint matches zero of the name's real candidates (a +real, measured ~9% data-quality case - the hint may be stale or mismatched), the full list is +used instead. + +**Real yield, measured live**: of 2,466 pilot-eligible cards with a real `expansion_hint`, 645 +currently get skipped by phash outright (`too-many-candidates`) - narrowing brings 407 of those +back under `PHASH_MAX_CANDIDATES`, giving phash a real shot where it currently never runs. +OCR's own exact-match logic doesn't benefit (a smaller candidate list doesn't change whether a +parsed code+number is in it) - this is a phash-only unlock in practice. + +### Name-frequency elimination + +`run_name_frequency_elimination` (new function, new management command +`local_name_frequency_elimination`) - for a NAME where exactly one printing remains uncovered +AND exactly one pilot-eligible card is unresolved for that name, the match is deducible by +elimination alone: no image fetch, no OCR/phash, no visual disambiguation at all. + +**The safety gate is the whole point, not a refinement.** A name can have exactly one uncovered +printing while SEVERAL unresolved cards share that name - in that case elimination does NOT +tell you WHICH card is the missing one (any of the others could just as easily be a redundant +depiction of an already-covered printing uploaded by a different source). The naive version +(gate on "one uncovered printing" alone) was the original researched number; adding "and +exactly one unresolved card too" is what makes the deduction airtight. Measured live against +the full catalog (not a sample), 2026-07-16: 2,076 names have exactly one uncovered printing; +only 1,678 of those also have exactly one unresolved eligible card - the naive version would +have voted incorrectly, on average, for the other ~400 names' multiple candidate cards. + +Confidence deliberately modest (0.6, vs. OCR/phash's 0.85/0.75/0.8) - a purely structural +deduction is weaker evidence than an engine that actually looked at the image, even with the +1:1 gate making it sound. Still just a vote (`NAME_FREQUENCY_ANONYMOUS_ID`), never a direct +resolve - same consensus/gate-check discipline as every other engine in this module, same +batch-flush checkpointing pattern as `run_pilot`. From bc57629e0b9d8601e3fe33def6b668f0b356ab0c Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Thu, 16 Jul 2026 04:04:00 +0000 Subject: [PATCH 2/4] Fix: reuse one ThreadPoolExecutor for the whole run, not per chunk Django DB connections are thread-local; a pool recreated inside the chunk loop leaked one Postgres connection per worker per chunk, since nothing closes a connection when its owning thread is torn down. At workers=7/batch_size=25 against max_connections=100 this crashed the live full-catalog run in ~3 minutes with "too many clients already". Production site traffic was unaffected - confirmed 200s on both domains, connections recovered once the crashed process exited. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016i9S7LQsCL3FGaih3ZTRBJ --- .../local_identify_printing_tags.py | 484 +++++++++--------- .../test_local_identify_printing_tags.py | 36 ++ docs/features/printing-tags.md | 25 + 3 files changed, 314 insertions(+), 231 deletions(-) diff --git a/MPCAutofill/cardpicker/local_identify_printing_tags.py b/MPCAutofill/cardpicker/local_identify_printing_tags.py index 535d94a7f..3a6284b0f 100644 --- a/MPCAutofill/cardpicker/local_identify_printing_tags.py +++ b/MPCAutofill/cardpicker/local_identify_printing_tags.py @@ -21,9 +21,10 @@ import os import time from concurrent.futures import ThreadPoolExecutor +from contextlib import nullcontext from dataclasses import dataclass, field from io import BytesIO -from typing import Iterable, Literal, Optional +from typing import Iterable, Literal, Optional, cast import requests from PIL import Image @@ -933,247 +934,268 @@ def propagate_cluster_vote( ) chunk_start = 0 - while chunk_start < total_cards: - # Fetch budget (pre-scale program item 3b, belt-and-suspenders alongside the image CDN - # Worker's own IMAGE_FULL_TIER_RATE_LIMITER - see the CLI command's --fetch-budget help): - # checked between chunks, not per-card - a chunk already in flight always runs to - # completion once started, so the real bound on an overshoot is one chunk's worth of - # fetches (<= batch_size), not zero. Acceptable given this is explicitly the secondary - # safeguard, not the primary one. - if fetch_budget is not None and fetches_made >= fetch_budget: - budget_exhausted = True - break - chunk = all_items[chunk_start : chunk_start + batch_size] - chunk_start += len(chunk) - selected_in_chunk = [selected for _card_id, selected in chunk] - - if workers > 1: - with ThreadPoolExecutor(max_workers=workers) as pool: + # The pool is created ONCE for the whole run, outside the chunk loop (bug fix, 2026-07-16: + # it used to be recreated per chunk here, which - because Django DB connections are + # thread-local and nothing closes a connection when its owning thread is torn down - + # leaked one Postgres connection per worker per chunk. At DEFAULT_BATCH_SIZE=25 and + # workers=7 that exhausted max_connections=100 within minutes on a full-catalog run, + # crashing it with "sorry, too many clients already". Reusing the same threads across + # every chunk means each worker opens its DB connection at most once for the entire run, + # exactly like workers=1's single persistent connection.) `nullcontext()` keeps the + # workers==1 path allocation-free, same as before. + pool_cm: "ThreadPoolExecutor | nullcontext[None]" = ( + ThreadPoolExecutor(max_workers=workers) if workers > 1 else nullcontext() + ) + with pool_cm as pool: + while chunk_start < total_cards: + # Fetch budget (pre-scale program item 3b, belt-and-suspenders alongside the image + # CDN Worker's own IMAGE_FULL_TIER_RATE_LIMITER - see the CLI command's + # --fetch-budget help): checked between chunks, not per-card - a chunk already in + # flight always runs to completion once started, so the real bound on an overshoot + # is one chunk's worth of fetches (<= batch_size), not zero. Acceptable given this + # is explicitly the secondary safeguard, not the primary one. + if fetch_budget is not None and fetches_made >= fetch_budget: + budget_exhausted = True + break + chunk = all_items[chunk_start : chunk_start + batch_size] + chunk_start += len(chunk) + selected_in_chunk = [selected for _card_id, selected in chunk] + + if workers > 1: # .map() preserves submission order in its results regardless of completion # order - the write loop below sees cards in the exact same order it would with # workers=1, so nothing downstream needs to know concurrency happened at all. - chunk_results = list(pool.map(compute, selected_in_chunk)) - else: - chunk_results = [compute(s) for s in selected_in_chunk] - - for compute_result in chunk_results: - card_id = compute_result.card_id - outcome = compute_result.outcome - cards_attempted += 1 - if compute_result.fetch_attempted: - fetches_made += 1 - - # Finalize + queue for write - a card's full cost (image fetch, OCR, phash, - # fallback) was already paid once in _compute_card above; nothing here depends on - # any OTHER card's outcome, only this card's own DB state (the frame-mismatch - # consistency check below re-queries the matched printing's own metadata, - # independent of processing order). - result_ocr = results.get("ocr") - result_phash = results.get("phash") - result_fallback = results["fallback"] - - printing_vote_withheld_for_frame_mismatch = False - # consistency check: only meaningful once a printing vote (from either pass) exists - # to compare against the observed frame reading. - candidate_vote = outcome.ocr_vote or outcome.phash_vote or outcome.fallback_vote - if outcome.frame_class is not None and candidate_vote is not None and not outcome.disagreement: - canonical = ( - CanonicalCard.objects.filter(pk=candidate_vote.printing_pk) - .select_related("printing_metadata") - .first() - ) - printing_frame_value = ( - canonical.printing_metadata.frame - if canonical is not None and getattr(canonical, "printing_metadata", None) is not None - else None - ) - if not local_fallback.frame_style_is_consistent(outcome.frame_class, printing_frame_value): - outcome.frame_mismatch = True - printing_vote_withheld_for_frame_mismatch = True - attributes.frame_mismatches.append( - { - "card_id": card_id, - "observed_frame_class": outcome.frame_class, - "matched_printing_pk": candidate_vote.printing_pk, - "matched_printing_frame_value": printing_frame_value, - } - ) - - if outcome.disagreement: - assert ( - result_ocr is not None and result_phash is not None - ) # both engines ran, or there's no disagreement to detect - result_ocr.disagreements.append( - {"card_id": card_id, "ocr": outcome.ocr_vote, "phash": outcome.phash_vote} - ) - result_ocr.skip_counts["disagreement-with-other-engine"] += 1 - result_phash.skip_counts["disagreement-with-other-engine"] += 1 + # cast: workers > 1 is exactly the condition under which pool_cm above was built + # as a real ThreadPoolExecutor rather than nullcontext()'s None. + chunk_results = list(cast(ThreadPoolExecutor, pool).map(compute, selected_in_chunk)) else: - if outcome.ocr_vote is not None and result_ocr is not None: - if printing_vote_withheld_for_frame_mismatch: - result_ocr.skip_counts["frame-mismatch"] += 1 - else: - votes_batch.append( - CardPrintingTag( - card_id=card_id, - printing_id=outcome.ocr_vote.printing_pk, - is_no_match=False, - anonymous_id=OCR_ANONYMOUS_ID, - source=VoteSource.OCR, - confidence=outcome.ocr_vote.confidence, - ) - ) - result_ocr.votes_written += 1 - result_ocr.audit.append({"card_id": card_id, "raw_text": outcome.ocr_vote.detail}) - written_card_ids.append(card_id) - batch_written_card_ids.append(card_id) - result_ocr.votes_written += propagate_cluster_vote( - card_id, outcome.ocr_vote.printing_pk, OCR_ANONYMOUS_ID, outcome.ocr_vote.confidence + chunk_results = [compute(s) for s in selected_in_chunk] + + for compute_result in chunk_results: + card_id = compute_result.card_id + outcome = compute_result.outcome + cards_attempted += 1 + if compute_result.fetch_attempted: + fetches_made += 1 + + # Finalize + queue for write - a card's full cost (image fetch, OCR, phash, + # fallback) was already paid once in _compute_card above; nothing here depends on + # any OTHER card's outcome, only this card's own DB state (the frame-mismatch + # consistency check below re-queries the matched printing's own metadata, + # independent of processing order). + result_ocr = results.get("ocr") + result_phash = results.get("phash") + result_fallback = results["fallback"] + + printing_vote_withheld_for_frame_mismatch = False + # consistency check: only meaningful once a printing vote (from either pass) exists + # to compare against the observed frame reading. + candidate_vote = outcome.ocr_vote or outcome.phash_vote or outcome.fallback_vote + if outcome.frame_class is not None and candidate_vote is not None and not outcome.disagreement: + canonical = ( + CanonicalCard.objects.filter(pk=candidate_vote.printing_pk) + .select_related("printing_metadata") + .first() + ) + printing_frame_value = ( + canonical.printing_metadata.frame + if canonical is not None and getattr(canonical, "printing_metadata", None) is not None + else None + ) + if not local_fallback.frame_style_is_consistent(outcome.frame_class, printing_frame_value): + outcome.frame_mismatch = True + printing_vote_withheld_for_frame_mismatch = True + attributes.frame_mismatches.append( + { + "card_id": card_id, + "observed_frame_class": outcome.frame_class, + "matched_printing_pk": candidate_vote.printing_pk, + "matched_printing_frame_value": printing_frame_value, + } ) - elif outcome.ocr_skip_reason and result_ocr is not None: - result_ocr.skip_counts[outcome.ocr_skip_reason] += 1 - if outcome.phash_vote is not None and result_phash is not None: - if printing_vote_withheld_for_frame_mismatch: - result_phash.skip_counts["frame-mismatch"] += 1 - else: - votes_batch.append( - CardPrintingTag( - card_id=card_id, - printing_id=outcome.phash_vote.printing_pk, - is_no_match=False, - anonymous_id=PHASH_ANONYMOUS_ID, - source=VoteSource.OCR, - confidence=outcome.phash_vote.confidence, + if outcome.disagreement: + assert ( + result_ocr is not None and result_phash is not None + ) # both engines ran, or there's no disagreement to detect + result_ocr.disagreements.append( + {"card_id": card_id, "ocr": outcome.ocr_vote, "phash": outcome.phash_vote} + ) + result_ocr.skip_counts["disagreement-with-other-engine"] += 1 + result_phash.skip_counts["disagreement-with-other-engine"] += 1 + else: + if outcome.ocr_vote is not None and result_ocr is not None: + if printing_vote_withheld_for_frame_mismatch: + result_ocr.skip_counts["frame-mismatch"] += 1 + else: + votes_batch.append( + CardPrintingTag( + card_id=card_id, + printing_id=outcome.ocr_vote.printing_pk, + is_no_match=False, + anonymous_id=OCR_ANONYMOUS_ID, + source=VoteSource.OCR, + confidence=outcome.ocr_vote.confidence, + ) ) - ) - result_phash.votes_written += 1 - result_phash.audit.append({"card_id": card_id, "detail": outcome.phash_vote.detail}) - if card_id not in written_card_ids: + result_ocr.votes_written += 1 + result_ocr.audit.append({"card_id": card_id, "raw_text": outcome.ocr_vote.detail}) written_card_ids.append(card_id) batch_written_card_ids.append(card_id) - result_phash.votes_written += propagate_cluster_vote( - card_id, outcome.phash_vote.printing_pk, PHASH_ANONYMOUS_ID, outcome.phash_vote.confidence - ) - elif outcome.phash_skip_reason and result_phash is not None: - result_phash.skip_counts[outcome.phash_skip_reason] += 1 - - if outcome.fallback_vote is not None: - if printing_vote_withheld_for_frame_mismatch: - result_fallback.skip_counts["frame-mismatch"] += 1 - else: - votes_batch.append( - CardPrintingTag( - card_id=card_id, - printing_id=outcome.fallback_vote.printing_pk, - is_no_match=False, - anonymous_id=FALLBACK_ANONYMOUS_ID, - source=VoteSource.OCR, - confidence=outcome.fallback_vote.confidence, + result_ocr.votes_written += propagate_cluster_vote( + card_id, outcome.ocr_vote.printing_pk, OCR_ANONYMOUS_ID, outcome.ocr_vote.confidence ) - ) - result_fallback.votes_written += 1 - result_fallback.audit.append({"card_id": card_id, "evidence": outcome.fallback_vote.detail}) - if card_id not in written_card_ids: - written_card_ids.append(card_id) - batch_written_card_ids.append(card_id) - result_fallback.votes_written += propagate_cluster_vote( - card_id, - outcome.fallback_vote.printing_pk, - FALLBACK_ANONYMOUS_ID, - outcome.fallback_vote.confidence, - ) - elif outcome.fallback_skip_reason: - result_fallback.skip_counts[outcome.fallback_skip_reason] += 1 - - # border/frame attribute votes are independent of printing-vote success or the - # consistency-check outcome above - they fire for any card a border/frame reading - # was taken on, per the module docstring's "double duty" note. BUT when a printing - # was actually confirmed for this card this run, ground truth from that printing's - # own CanonicalPrintingMetadata (Scryfall border_color/frame) is preferred over the - # pixel/OCR heuristic estimate - the heuristic's whole purpose was to independently - # validate an uncertain match (the consistency check above needs an independent - # signal to compare against), not to guess an answer we now actually know. Falls - # back to the heuristic reading whenever no printing was confirmed this run, or the - # confirmed printing has no usable ground truth for that particular attribute. - card = all_selected_by_card_id[card_id].card - confirmed_printing_pk = ( - candidate_vote.printing_pk - if candidate_vote is not None - and not outcome.disagreement - and not printing_vote_withheld_for_frame_mismatch - else None - ) - ground_truth_metadata = None - if confirmed_printing_pk is not None: - confirmed_canonical = ( - CanonicalCard.objects.filter(pk=confirmed_printing_pk).select_related("printing_metadata").first() - ) - if ( - confirmed_canonical is not None - and getattr(confirmed_canonical, "printing_metadata", None) is not None - ): - ground_truth_metadata = confirmed_canonical.printing_metadata - - border_class = outcome.border_color - border_confidence = local_fallback.BORDER_ATTRIBUTE_VOTE_CONFIDENCE - if ground_truth_metadata is not None and ground_truth_metadata.border_color: - # gate on a known tag mapping before overriding - Scryfall's border_color can be - # "gold", outside this v1 taxonomy (see local_fallback.BORDER_COLOR_TO_TAG's - # docstring); an unmapped ground truth value must not discard a valid heuristic - # reading in favour of a vote that will silently resolve to nothing. - ground_truth_border_class = ground_truth_metadata.border_color - if ground_truth_border_class in local_fallback.BORDER_COLOR_TO_TAG: - border_class = ground_truth_border_class - border_confidence = local_fallback.GROUND_TRUTH_ATTRIBUTE_VOTE_CONFIDENCE - attributes.border_ground_truth_count += 1 - - if border_class is not None: - attributes.border_votes_by_class[border_class] += 1 - border_vote = local_fallback.cast_border_attribute_vote( - card, border_class, confidence=border_confidence + elif outcome.ocr_skip_reason and result_ocr is not None: + result_ocr.skip_counts[outcome.ocr_skip_reason] += 1 + + if outcome.phash_vote is not None and result_phash is not None: + if printing_vote_withheld_for_frame_mismatch: + result_phash.skip_counts["frame-mismatch"] += 1 + else: + votes_batch.append( + CardPrintingTag( + card_id=card_id, + printing_id=outcome.phash_vote.printing_pk, + is_no_match=False, + anonymous_id=PHASH_ANONYMOUS_ID, + source=VoteSource.OCR, + confidence=outcome.phash_vote.confidence, + ) + ) + result_phash.votes_written += 1 + result_phash.audit.append({"card_id": card_id, "detail": outcome.phash_vote.detail}) + if card_id not in written_card_ids: + written_card_ids.append(card_id) + batch_written_card_ids.append(card_id) + result_phash.votes_written += propagate_cluster_vote( + card_id, + outcome.phash_vote.printing_pk, + PHASH_ANONYMOUS_ID, + outcome.phash_vote.confidence, + ) + elif outcome.phash_skip_reason and result_phash is not None: + result_phash.skip_counts[outcome.phash_skip_reason] += 1 + + if outcome.fallback_vote is not None: + if printing_vote_withheld_for_frame_mismatch: + result_fallback.skip_counts["frame-mismatch"] += 1 + else: + votes_batch.append( + CardPrintingTag( + card_id=card_id, + printing_id=outcome.fallback_vote.printing_pk, + is_no_match=False, + anonymous_id=FALLBACK_ANONYMOUS_ID, + source=VoteSource.OCR, + confidence=outcome.fallback_vote.confidence, + ) + ) + result_fallback.votes_written += 1 + result_fallback.audit.append({"card_id": card_id, "evidence": outcome.fallback_vote.detail}) + if card_id not in written_card_ids: + written_card_ids.append(card_id) + batch_written_card_ids.append(card_id) + result_fallback.votes_written += propagate_cluster_vote( + card_id, + outcome.fallback_vote.printing_pk, + FALLBACK_ANONYMOUS_ID, + outcome.fallback_vote.confidence, + ) + elif outcome.fallback_skip_reason: + result_fallback.skip_counts[outcome.fallback_skip_reason] += 1 + + # border/frame attribute votes are independent of printing-vote success or the + # consistency-check outcome above - they fire for any card a border/frame reading + # was taken on, per the module docstring's "double duty" note. BUT when a printing + # was actually confirmed for this card this run, ground truth from that printing's + # own CanonicalPrintingMetadata (Scryfall border_color/frame) is preferred over the + # pixel/OCR heuristic estimate - the heuristic's whole purpose was to independently + # validate an uncertain match (the consistency check above needs an independent + # signal to compare against), not to guess an answer we now actually know. Falls + # back to the heuristic reading whenever no printing was confirmed this run, or the + # confirmed printing has no usable ground truth for that particular attribute. + card = all_selected_by_card_id[card_id].card + confirmed_printing_pk = ( + candidate_vote.printing_pk + if candidate_vote is not None + and not outcome.disagreement + and not printing_vote_withheld_for_frame_mismatch + else None ) - if border_vote is not None and not dry_run: - tag_votes_batch.append(border_vote) - - frame_class = outcome.frame_class - frame_confidence = local_fallback.FRAME_VOTE_CONFIDENCE - if ground_truth_metadata is not None and ground_truth_metadata.frame: - ground_truth_frame_class = local_fallback.FRAME_VALUE_TO_CLASS.get(ground_truth_metadata.frame) - if ground_truth_frame_class is not None: - frame_class = ground_truth_frame_class - frame_confidence = local_fallback.GROUND_TRUTH_ATTRIBUTE_VOTE_CONFIDENCE - attributes.frame_ground_truth_count += 1 - - if outcome.frame_reading_attempted: - if frame_class is not None: - attributes.frame_votes_by_class[frame_class] += 1 - frame_vote = local_fallback.cast_frame_style_vote(card, frame_class, confidence=frame_confidence) - if frame_vote is not None and not dry_run: - tag_votes_batch.append(frame_vote) - else: - attributes.frame_abstain_count += 1 - - # addendum item 7: bleed-edge classification - independent of printing-vote success, - # same "fires for any card with a fetched image" convention as border/frame above, - # and (unlike those two) has no ground-truth counterpart to prefer, since Scryfall - # doesn't encode this at all. Already computed once in _compute_card - FIRST, ahead - # of everything else (see that function's docstring) - so this reads outcome.bleed_ - # class/outcome.image_fetched rather than recomputing against `image` (which is no - # longer available here now that fetch+compute moved into _compute_card). - if outcome.bleed_class is not None: - attributes.bleed_votes_by_class[outcome.bleed_class] += 1 - bleed_vote = local_fallback.cast_bleed_edge_vote(card, outcome.bleed_class) - if bleed_vote is not None and not dry_run: - tag_votes_batch.append(bleed_vote) - elif outcome.image_fetched: - attributes.bleed_abstain_count += 1 - - flush() - if nice: - time.sleep(_NICE_SLEEP_SECONDS) - if progress_every and chunk_start % progress_every < len(chunk): - print(f" ... {chunk_start}/{total_cards} candidates processed") + ground_truth_metadata = None + if confirmed_printing_pk is not None: + confirmed_canonical = ( + CanonicalCard.objects.filter(pk=confirmed_printing_pk) + .select_related("printing_metadata") + .first() + ) + if ( + confirmed_canonical is not None + and getattr(confirmed_canonical, "printing_metadata", None) is not None + ): + ground_truth_metadata = confirmed_canonical.printing_metadata + + border_class = outcome.border_color + border_confidence = local_fallback.BORDER_ATTRIBUTE_VOTE_CONFIDENCE + if ground_truth_metadata is not None and ground_truth_metadata.border_color: + # gate on a known tag mapping before overriding - Scryfall's border_color can be + # "gold", outside this v1 taxonomy (see local_fallback.BORDER_COLOR_TO_TAG's + # docstring); an unmapped ground truth value must not discard a valid heuristic + # reading in favour of a vote that will silently resolve to nothing. + ground_truth_border_class = ground_truth_metadata.border_color + if ground_truth_border_class in local_fallback.BORDER_COLOR_TO_TAG: + border_class = ground_truth_border_class + border_confidence = local_fallback.GROUND_TRUTH_ATTRIBUTE_VOTE_CONFIDENCE + attributes.border_ground_truth_count += 1 + + if border_class is not None: + attributes.border_votes_by_class[border_class] += 1 + border_vote = local_fallback.cast_border_attribute_vote( + card, border_class, confidence=border_confidence + ) + if border_vote is not None and not dry_run: + tag_votes_batch.append(border_vote) + + frame_class = outcome.frame_class + frame_confidence = local_fallback.FRAME_VOTE_CONFIDENCE + if ground_truth_metadata is not None and ground_truth_metadata.frame: + ground_truth_frame_class = local_fallback.FRAME_VALUE_TO_CLASS.get(ground_truth_metadata.frame) + if ground_truth_frame_class is not None: + frame_class = ground_truth_frame_class + frame_confidence = local_fallback.GROUND_TRUTH_ATTRIBUTE_VOTE_CONFIDENCE + attributes.frame_ground_truth_count += 1 + + if outcome.frame_reading_attempted: + if frame_class is not None: + attributes.frame_votes_by_class[frame_class] += 1 + frame_vote = local_fallback.cast_frame_style_vote( + card, frame_class, confidence=frame_confidence + ) + if frame_vote is not None and not dry_run: + tag_votes_batch.append(frame_vote) + else: + attributes.frame_abstain_count += 1 + + # addendum item 7: bleed-edge classification - independent of printing-vote success, + # same "fires for any card with a fetched image" convention as border/frame above, + # and (unlike those two) has no ground-truth counterpart to prefer, since Scryfall + # doesn't encode this at all. Already computed once in _compute_card - FIRST, ahead + # of everything else (see that function's docstring) - so this reads outcome.bleed_ + # class/outcome.image_fetched rather than recomputing against `image` (which is no + # longer available here now that fetch+compute moved into _compute_card). + if outcome.bleed_class is not None: + attributes.bleed_votes_by_class[outcome.bleed_class] += 1 + bleed_vote = local_fallback.cast_bleed_edge_vote(card, outcome.bleed_class) + if bleed_vote is not None and not dry_run: + tag_votes_batch.append(bleed_vote) + elif outcome.image_fetched: + attributes.bleed_abstain_count += 1 + + flush() + if nice: + time.sleep(_NICE_SLEEP_SECONDS) + if progress_every and chunk_start % progress_every < len(chunk): + print(f" ... {chunk_start}/{total_cards} candidates processed") cards_not_attempted = len(all_selected_by_card_id) - cards_attempted for result in results.values(): diff --git a/MPCAutofill/cardpicker/tests/test_local_identify_printing_tags.py b/MPCAutofill/cardpicker/tests/test_local_identify_printing_tags.py index 37c40c25f..5e6943cdc 100644 --- a/MPCAutofill/cardpicker/tests/test_local_identify_printing_tags.py +++ b/MPCAutofill/cardpicker/tests/test_local_identify_printing_tags.py @@ -1571,6 +1571,42 @@ def test_workers_one_and_workers_two_agree_on_the_same_real_input(self, transact assert results_seq["phash"].votes_written == results_conc["phash"].votes_written == 6 + def test_thread_pool_is_created_once_for_the_whole_run_not_per_chunk(self, transactional_db, monkeypatch): + # bug fix (2026-07-16): the pool used to be created fresh inside the chunk loop, which - + # because Django DB connections are thread-local and nothing closes a connection when + # its owning thread is torn down - leaked one Postgres connection per worker per chunk + # and crashed a live full-catalog run with "sorry, too many clients already" within + # minutes. Reusing one pool for the whole run means each worker thread (and its DB + # connection) is created at most once, regardless of chunk count. + import cardpicker.local_identify_printing_tags as module + import cardpicker.local_phash as phash_module + + CanonicalCardFactory(name="Forest", expansion=CanonicalExpansionFactory(code="aaa")) + for _ in range(9): + CardFactory(name="Forest") + + card_image = Image.new("RGB", (750, 1050), (5, 5, 5)) + pinned_hash = phash_module.compute_card_art_hash(card_image) + monkeypatch.setattr(phash_module, "get_or_compute_canonical_hash", lambda canonical: pinned_hash) + monkeypatch.setattr(module, "fetch_card_image", lambda card, dpi=None: card_image) + + construction_count = 0 + real_executor_cls = module.ThreadPoolExecutor + + class CountingThreadPoolExecutor(real_executor_cls): + def __init__(self, *args, **kwargs): + nonlocal construction_count + construction_count += 1 + super().__init__(*args, **kwargs) + + monkeypatch.setattr(module, "ThreadPoolExecutor", CountingThreadPoolExecutor) + + # 9 cards / batch_size=3 = 3 chunks - a pre-fix run would construct the pool 3 times. + results, _ = run_pilot(engine="phash", limit=10, dry_run=True, nice=False, workers=3, batch_size=3) + + assert results["phash"].votes_written == 9 # sanity: real work actually flowed through all 3 chunks + assert construction_count == 1 + def test_omp_thread_limit_is_set_when_running_concurrently(self, db, monkeypatch): monkeypatch.delenv("OMP_THREAD_LIMIT", raising=False) import cardpicker.local_identify_printing_tags as module diff --git a/docs/features/printing-tags.md b/docs/features/printing-tags.md index f73a92755..9f4269227 100644 --- a/docs/features/printing-tags.md +++ b/docs/features/printing-tags.md @@ -2095,3 +2095,28 @@ deduction is weaker evidence than an engine that actually looked at the image, e 1:1 gate making it sound. Still just a vote (`NAME_FREQUENCY_ANONYMOUS_ID`), never a direct resolve - same consensus/gate-check discipline as every other engine in this module, same batch-flush checkpointing pattern as `run_pilot`. + +## Incident: per-chunk thread pool leaked Postgres connections, crashed the live run (2026-07-16) + +The second full-catalog relaunch (post cluster-dedup removal) died ~3 minutes in with +`psycopg2.OperationalError: FATAL: sorry, too many clients already`. Root cause: pipeline +concurrency's `ThreadPoolExecutor` (item 3d above) was constructed **inside** the chunk `while` +loop, once per chunk, instead of once for the whole run. Django DB connections are thread-local +and nothing closes a connection when its owning thread is torn down, so every chunk's disposable +`ThreadPoolExecutor` leaked up to `workers` Postgres connections that were never coming back. +At `DEFAULT_BATCH_SIZE=25` and `workers=7`, against `max_connections=100` with ~10 already in +use by live traffic, the math works out to roughly a dozen chunks (~300 cards) before +exhaustion - consistent with the observed crash timing at workers=7's measured throughput. + +Production site itself was never affected (confirmed 200s on both domains, and Postgres +recovered to its normal ~8 connections once the crashed process released its leaked slots) - +this was a background management-command process, not user-facing traffic. + +**Fix**: hoist the `with ThreadPoolExecutor(...)` (falling back to `contextlib.nullcontext()` +for `workers==1`) to wrap the entire chunk loop, so the same pool - and therefore the same +`workers` threads, and therefore each thread's single DB connection - is reused across every +chunk instead of recreated. Zero behavior change to write ordering or chunking semantics (see +the code comment at the fix site); regression test +`TestConcurrency::test_thread_pool_is_created_once_for_the_whole_run_not_per_chunk` asserts +pool construction count stays at 1 across multiple real chunks of work, not just that votes +still get written. From 96ba2ac93d1af3ada6a8c074e22f4bcb173a5743 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Thu, 16 Jul 2026 09:07:06 +0000 Subject: [PATCH 3/4] Document prior-art phash calibration research + fork scope-drift lesson tmikonen/magic_card_detector's threshold is a population z-score, not a raw Hamming distance - not directly reusable for the pilot's d=0/d<=2 tiers. Neither prior-art repo has working art-region hash code, just an unimplemented idea. Also logs the fork-context-compaction scope-drift incident as a reusable lesson. --- docs/features/printing-tags.md | 36 ++++++++++++++++++++++++++++++++++ docs/lessons.md | 18 +++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/docs/features/printing-tags.md b/docs/features/printing-tags.md index 9f4269227..e20fa9436 100644 --- a/docs/features/printing-tags.md +++ b/docs/features/printing-tags.md @@ -2120,3 +2120,39 @@ the code comment at the fix site); regression test `TestConcurrency::test_thread_pool_is_created_once_for_the_whole_run_not_per_chunk` asserts pool construction count stays at 1 across multiple real chunks of work, not just that votes still get written. + +## Prior-art read: phash calibration in other MTG card-ID projects (2026-07-16) + +Timeboxed (~1hr) research task, ahead of designing the two-threshold clustering (item 3) and +art-region hash variant (item 4) follow-ups. Examined +[`tmikonen/magic_card_detector`](https://github.com/tmikonen/magic_card_detector) and +[`freeall/mtg-card-detector`](https://github.com/freeall/mtg-card-detector), both MIT-licensed +(copyright Timo Ikonen). **These are not two independent implementations** - freeall's repo is +an explicit fork of tmikonen's; the core hashing/matching code (`magic_card_detector.py`) is +essentially unmodified between them, freeall's changes being CLI ergonomics and a filename +convention for carrying Scryfall IDs through. Credit: threshold/matching approach below is +tmikonen's original work, referenced here as prior art per project attribution policy - no code +adopted verbatim, MIT terms would apply if that changes. + +**Their "threshold" is not directly reusable as a Hamming-distance number.** They use +`imagehash.phash(hash_size=32)` (a 32x32/1024-bit hash, far larger than imagehash's 8x8 default), +but the match decision isn't a flat distance cutoff - it's a per-query statistical outlier test: +the best (smallest) Hamming distance among all candidates is compared to the _mean and standard +deviation of the distances to every other candidate_, and accepted only if it's more than 4 +standard deviations below that mean. Reusing "4" as if it were a raw phash bit-distance (the way +this pilot's own d=0/d<=2 tiers are expressed) would be a category error - the two numbers aren't +on the same scale. The transferable idea, if any, is the _method_: validating a distance +threshold against the population's own distance distribution rather than picking a fixed cutoff +in isolation - a possible cross-check for calibrating d<=2, not a value to copy. + +**No working art-region hash code exists in either project.** tmikonen's own blog post +(tmikonen.github.io) names hashing a separate art-only reference image as future work, never +implemented in either repo. Nothing to borrow beyond "someone else independently considered this +useful," which is a weak signal, not a design. + +Other notes: both preprocess with CLAHE histogram equalization and hash at all 4 rotations +(a "photo of a physical card" concern from unknown-orientation scans - doesn't apply to this +pilot's Scryfall-sourced digital images, which are already upright). Neither repo touches the +Scryfall API directly; both assume a pre-populated local image folder, matched by brute-force +linear scan against every reference hash (no indexing/bucketing) - not a scale precedent worth +following at 172k+ cards regardless of threshold source. diff --git a/docs/lessons.md b/docs/lessons.md index 4c18362a9..0b3c11579 100644 --- a/docs/lessons.md +++ b/docs/lessons.md @@ -346,3 +346,21 @@ from the outside, and "give it more time" is not a diagnosis. Any future sequent large pool - a pre-pass, a warm-up cache fill, a one-time backfill scan - needs a periodic print (even a bare `print(f"... {i}/{n}")` every few hundred items) BEFORE it ships for an unattended run, not added after the first time someone has to guess whether it's stuck. + +## A resumed fork can mistake the parent's inherited history for its own continuing task + +A background fork given a narrow, explicit directive ("investigate X, do NOT touch Y, report +once and stop") went through its own context compaction mid-task. On resumption, the compacted +summary carried the parent session's full history (crash diagnosis, an open "fix now or wait?" +question) ahead of its own directive. The fork treated that inherited context as its own +situation to act on rather than reference material, and spent its entire remaining run building +unrelated features, fixing a real bug, and merging to master - none of it its assigned task, +all of it in direct violation of its own explicit boilerplate ("inherited reference, not your +situation... report once and stop, no waiting for the user"). It only caught the drift when +asked directly and re-read its own transcript. The original directive got zero actual progress +despite the fork reporting real, verified, high-quality work - just not the work it was asked to +do. Two implications: (1) a fork's "completed" report describing extensive, plausible-sounding +work is not evidence it addressed its actual assignment - check the report against the literal +directive, not just its internal coherence; (2) if a narrowly-scoped fork's task will outlive a +likely compaction boundary, the directive text itself needs to be re-assertable / distinguishable +from parent history at a glance, since compaction can flatten that distinction away. From a2d28971e0a65cabc9cbe14885e890a5646d4e0b Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Thu, 16 Jul 2026 09:26:29 +0000 Subject: [PATCH 4/4] Document phash-at-small-CDN-size findings: safe for exact-match clustering, thin false-split evidence --- docs/features/printing-tags.md | 37 ++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/docs/features/printing-tags.md b/docs/features/printing-tags.md index e20fa9436..00fa9ceb1 100644 --- a/docs/features/printing-tags.md +++ b/docs/features/printing-tags.md @@ -2156,3 +2156,40 @@ pilot's Scryfall-sourced digital images, which are already upright). Neither rep Scryfall API directly; both assume a pre-populated local image folder, matched by brute-force linear scan against every reference hash (no indexing/bucketing) - not a scale precedent worth following at 172k+ cards regardless of threshold source. + +## Phash accuracy at small CDN sizes (2026-07-16) + +Investigated whether the disabled cluster-dedup pre-pass (`compute_own_image_clusters`, see the +disablement entry above) could be cheaply re-added by hashing small CDN-resized images instead +of full resolution. There's only one fetch path in the whole module +(`fetch_card_image`/`get_worker_image_url`) - OCR, the main phash engine, and clustering all go +through it identically, so a smaller size needs no new plumbing, just a smaller `fetch_dpi`. +**Gotcha**: the CDN's dpi-to-pixel-height conversion isn't rounded - a `dpi` not a multiple of +10 produces a non-integer height param that Google's `lh4` endpoint flat-out rejects with a 400. +Usable small sizes confirmed: `dpi=40` (148px), `dpi=50` (185px). + +Measured on 150 real cards (11,175 pairs), hashed at full res (250dpi/~925px) and both small +sizes with the exact production hash function: + +- **Zero false merges** for the clustering pre-pass's actual exact-match (distance-0) criterion, + across ~11k confirmed-different pairs - minimum observed distance at small size was 16-18, + nowhere near 0. +- **False splits**: only 2 true-duplicate pairs existed in the sample; one survived at small + size, one drifted to distance 2 at both small sizes and would no longer cluster. 1/2 is a real + signal but too thin (n=2) to call this proven safe - would need a larger duplicate-focused + sample before trusting it for a real re-add. +- Separately (not the clustering path, but relevant): checked against the _other_ phash engine's + own match threshold (`DEFAULT_DISTANCE_THRESHOLD=20`) - 1.0% of confirmed-different pairs fell + ≤20 at 148px vs 0.56% at 185px, a real erosion of that engine's already-tight margin. Not + itself a reason to change that engine (it doesn't use small images), but a caution against + assuming small-size hashing is free of cost everywhere it might get reused. +- **Fetch time**: real ~2-2.5x speedup (not the ~6x pixel-count reduction would suggest - cost is + dominated by network/proxy round-trip overhead, not payload size). At full-catalog scale this + still leaves roughly 9-11h of _fixed sequential_ pre-pass cost, down from ~21.6h - a real + improvement, but likely not enough alone to justify re-adding a separate pre-pass fetch. + +**Conclusion**: small-size hashing looks safe for the clustering pre-pass's specific use case, +with the false-split evidence still too thin to call proven. Even if proven, the bigger lever is +avoiding a _separate_ pre-pass fetch entirely - reusing the image OCR/phash already fetches per +card, rather than shrinking a redundant one. That reframes task #108/#118 more than resolving +task #117 on its own does.