feat(image-evidence): make evidence-affecting helpers version-visible + CI totality - #690
Merged
Merged
Conversation
… + CI totality Inventory + declare EXTRACTOR_OWNERSHIP for every module-private helper in image_evidence.py and every externally-imported callable it actually calls from collector_line_artist.py/local_ocr.py/local_image_quality.py that helps determine a stored ImageEvidence field but carries no *_EXTRACTOR_VERSION of its own (issue #509's own follow-up). All 17 entries are components of an already-versioned extractor - none are new extractors, so no manifest key, golden-set expectation, or stored value changes. - .github/scripts/check_extractor_ownership_totality.py: AST-derived totality check, sibling to check_extractor_manifest_sync.py. Derives the set of module-private helpers + called scoped-external imports reachable from compute_card_evidence's own call graph and fails CI if it disagrees with EXTRACTOR_OWNERSHIP in either direction. Cross-checks every declared owning key against the real MANIFEST_EXTRACTOR_KEYS (imported directly from check_extractor_manifest_sync.py). Excludes local_fallback.py and _collector_line_ocr_attempts (the OCR tier ladder) - both out of scope, worked on in parallel per this PR's own brief. - .github/scripts/tests/test_check_extractor_ownership_totality.py: fixture + real-repo tests, including a case proving a newly-added undeclared contributor fails CI. - .github/workflows/docs-lint.yml: wires the new tether in as its own job, mirroring extractor-manifest-sync exactly. - docs/features/catalog-completion-plan.md: "Stage C extractor ownership" section - the rule, why the two rosters stay in sync, the two explicit exclusions, and a correction to this PR's own brief (local_art_edge.py's extended-art classifier, cited as an example, is not actually reachable from compute_card_evidence today - it's a declared-not-yet-live Stage D calculator, not a Stage C extractor). - image_evidence.py: pointer paragraph in the module docstring; no behavioral change.
WilfordGrimley
added a commit
that referenced
this pull request
Aug 5, 2026
…s to 2 (issue #677) (#698) * perf(image-evidence): collapse Stage C OCR attempt ladder from 3 tiers to 2 (issue #677) _collector_line_ocr_attempts drops tier 3 (a PSM-11 re-try of tier 1's own variants) - measured, not guessed. Two fresh real-production probes (450 forced-escalation cards combined) walked the old 3-tier ladder to full completion and recorded, per card, the first tier at which a candidate-validated genuine match appeared: tier 2 produced 2 genuine matches across the sample, tier 3 produced zero - only more lexicon-valid -but-uncorroborated noise, consistent with the 2026-07-23 preprocessing probes' own ~99% hopeless-art-noise finding for this population. Cross-checked against all 30 golden_set.GOLDEN_CARD_IDS cards: none ever resolved uniquely at tier 3, so this collapse changes zero golden-set expectations. Worst case drops from 8 tesseract calls to 6 (down from #259-#677's own 4x pre-#259 cost to 3x). collector_line_ocr/collector_line_tsv/artist_ocr extractor versions bumped v2->v3 (all three read through the same escalation loop, per EXTRACTOR_OWNERSHIP's own 'bump every listed key together' convention); legal_line unchanged. Investigated and explicitly did NOT ship the companion 'unify the collector-line/legal-line OCR reads into one full-width pass' idea named in this issue's own brief: a real-image accuracy probe found it regresses 46/150 (30.7%) of currently-correct collector-number reads (mostly proxy/ watermark text getting picked up by the set-code regex in place of the real code, or the wider crop causing tesseract to drop the collector- number line outright), with zero improvements. Reported as a legitimate negative finding rather than shipped anyway. Lifts the EXTRACTOR_OWNERSHIP EXCLUDED_HELPERS entry for the OCR ladder (check_extractor_ownership_totality.py) that PR #690 left in place while this branch was in flight - _collector_line_ocr_attempts and preprocess_fallback_variants now carry real ownership entries. Verification: full backend suite (3598 passed, 8 skipped, 0 failed), ownership-totality + manifest-sync scripts clean with their own unit tests green (40/40), docs_lint --strict clean, pre-commit clean on all tracked files. * fix(image-evidence): bump artist_ocr to v4 to avoid colliding with #685's v3 ARTIST_OCR_EXTRACTOR_VERSION was set to "artist-ocr-v3" on this branch for issue #677's ladder-collapse dedup, but PR #685 (merged to master separately) already claims "artist-ocr-v3" for an unrelated change (wiring modern_artist_credit's crop fallback into live extraction). Two different behaviours under one version string would defeat the staleness filter MANIFEST_EXTRACTOR_CURRENT_VERSIONS exists to enforce. Bumps artist_ocr straight to v4, updates the matching manifest entry in run_image_evidence_cohort.py, and documents the collision in both the version constant's own history comment and catalog-completion-plan.md's #677 section. collector_line_ocr/collector_line_tsv stay at v3 (master still has both at v2, no collision). Also fixes golden_set.py's #677 re-verification comment, which cited MPCAutofill/scripts/experiments/ocr_ladder_tier_attribution.py - a worktree-only analysis script never committed to this repo, per the established convention (see docs/reports/2026-07-23-ocr-preprocessing- probe.md's own LIVE STATE) of not committing one-off probes. Points at catalog-completion-plan.md's already-committed #677 section instead, which documents the same 450-card probe's methodology and results. * fix(docs): cite OCR ladder probe scripts as prose, not backticked paths Both scripts are deliberately uncommitted (worktree-only analysis tooling, same convention as the 2026-07-23 probe reports). Backticked citations to their paths tripped docs_lint.py's path-reference rule since MPCAutofill/scripts/ doesn't exist in the repo. Rephrase as prose so the provenance stays legible without a machine-checkable path reference to a file that was never meant to be committed.
8 tasks
WilfordGrimley
added a commit
that referenced
this pull request
Aug 5, 2026
…s to 2 (issue #677) (#698) (#702) * perf(image-evidence): collapse Stage C OCR attempt ladder from 3 tiers to 2 (issue #677) _collector_line_ocr_attempts drops tier 3 (a PSM-11 re-try of tier 1's own variants) - measured, not guessed. Two fresh real-production probes (450 forced-escalation cards combined) walked the old 3-tier ladder to full completion and recorded, per card, the first tier at which a candidate-validated genuine match appeared: tier 2 produced 2 genuine matches across the sample, tier 3 produced zero - only more lexicon-valid -but-uncorroborated noise, consistent with the 2026-07-23 preprocessing probes' own ~99% hopeless-art-noise finding for this population. Cross-checked against all 30 golden_set.GOLDEN_CARD_IDS cards: none ever resolved uniquely at tier 3, so this collapse changes zero golden-set expectations. Worst case drops from 8 tesseract calls to 6 (down from #259-#677's own 4x pre-#259 cost to 3x). collector_line_ocr/collector_line_tsv/artist_ocr extractor versions bumped v2->v3 (all three read through the same escalation loop, per EXTRACTOR_OWNERSHIP's own 'bump every listed key together' convention); legal_line unchanged. Investigated and explicitly did NOT ship the companion 'unify the collector-line/legal-line OCR reads into one full-width pass' idea named in this issue's own brief: a real-image accuracy probe found it regresses 46/150 (30.7%) of currently-correct collector-number reads (mostly proxy/ watermark text getting picked up by the set-code regex in place of the real code, or the wider crop causing tesseract to drop the collector- number line outright), with zero improvements. Reported as a legitimate negative finding rather than shipped anyway. Lifts the EXTRACTOR_OWNERSHIP EXCLUDED_HELPERS entry for the OCR ladder (check_extractor_ownership_totality.py) that PR #690 left in place while this branch was in flight - _collector_line_ocr_attempts and preprocess_fallback_variants now carry real ownership entries. Verification: full backend suite (3598 passed, 8 skipped, 0 failed), ownership-totality + manifest-sync scripts clean with their own unit tests green (40/40), docs_lint --strict clean, pre-commit clean on all tracked files. * fix(image-evidence): bump artist_ocr to v4 to avoid colliding with #685's v3 ARTIST_OCR_EXTRACTOR_VERSION was set to "artist-ocr-v3" on this branch for issue #677's ladder-collapse dedup, but PR #685 (merged to master separately) already claims "artist-ocr-v3" for an unrelated change (wiring modern_artist_credit's crop fallback into live extraction). Two different behaviours under one version string would defeat the staleness filter MANIFEST_EXTRACTOR_CURRENT_VERSIONS exists to enforce. Bumps artist_ocr straight to v4, updates the matching manifest entry in run_image_evidence_cohort.py, and documents the collision in both the version constant's own history comment and catalog-completion-plan.md's still has both at v2, no collision). Also fixes golden_set.py's #677 re-verification comment, which cited MPCAutofill/scripts/experiments/ocr_ladder_tier_attribution.py - a worktree-only analysis script never committed to this repo, per the established convention (see docs/reports/2026-07-23-ocr-preprocessing- probe.md's own LIVE STATE) of not committing one-off probes. Points at catalog-completion-plan.md's already-committed #677 section instead, which documents the same 450-card probe's methodology and results. * fix(docs): cite OCR ladder probe scripts as prose, not backticked paths Both scripts are deliberately uncommitted (worktree-only analysis tooling, same convention as the 2026-07-23 probe reports). Backticked citations to their paths tripped docs_lint.py's path-reference rule since MPCAutofill/scripts/ doesn't exist in the repo. Rephrase as prose so the provenance stays legible without a machine-checkable path reference to a file that was never meant to be committed. (cherry picked from commit c5eee1c)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ImageEvidencefield but carries no*_EXTRACTOR_VERSIONof its own (issue Add illustration_id import and Stage D illustration deduction calculator #509's own follow-up): 8 module-private helpers inimage_evidence.py(_crop_box_to_pixels,_compute_region_phash,_short_circuit_enabled_by_env,_contains_digit,_parse_is_lexicon_valid,_extract_legal_line,_parse_artist_is_contradicted,_confidently_digit_free) plus 9 externally-imported callables it actually calls fromcollector_line_artist.py/local_ocr.py/local_image_quality.py(recover_artist_from_card_text,compute_blur_variance,compute_entropy,is_image_truncated,parse_collector_line,parse_legal_line,preprocess_variants,run_tesseract,run_tesseract_text_and_words) - 17 entries total, each declared in a newEXTRACTOR_OWNERSHIPmap naming everyMANIFEST_EXTRACTOR_KEYSmember whose stored field it helps determine.MANIFEST_EXTRACTOR_KEYSmember, noMANIFEST_EXTRACTOR_CURRENT_VERSIONSentry, noGOLDEN_EXPECTATIONSaddition, and no storedImageEvidencevalue changes as a result of this PR - all 17 were already feeding their currently-versioned extractor(s) before this landed. This is a declaration + CI-enforcement PR, not a re-extraction trigger..github/scripts/check_extractor_ownership_totality.py, a sibling to the pre-existingcheck_extractor_manifest_sync.py: AST-derives the set ofimage_evidence.py's own module-private helpers plus every scoped-external import it actually calls, and fails CI if that derived set disagrees withEXTRACTOR_OWNERSHIPin either direction (undeclared new contributor, or a stale entry for a renamed/removed one). Also cross-checks every declared owning key against the realMANIFEST_EXTRACTOR_KEYS, imported directly fromcheck_extractor_manifest_sync.pyso the two scripts can never independently disagree about what counts as a real key.local_fallback.py's own helpers and the OCR attempt-tier ladder (_collector_line_ocr_attempts, issue Stage D no-text bucket: OCR preprocessing/crop recovery (88.8% of skips are fixable, not a coverage ceiling) #259) - both being worked on in parallel by other branches per this PR's own brief. Each is already the direct mechanism of an existing versioned extractor, so the omission is a sequencing decision, not a coverage gap.docs/features/catalog-completion-plan.mdstates the rule (own extractor vs. declared component, never left implicit), how the two rosters (manifest + ownership) stay in sync, and the two exclusions above. Pointer paragraph added toimage_evidence.py's own module docstring; no behavioral change to that file.local_art_edge.classify_art_edge_continuity(issue Extended art: the owner's two-sample-point art-edge classifier (evidence-only) #617's extended-art classifier) was named as a candidate for this sweep but is not actually reachable fromcompute_card_evidencetoday -local_art_edge.pyis a declared-but-not-yet-live Stage D calculator (reads already-persistedImageEvidence.art_crop_px, isn't called from Stage C's own extraction pass), confirmed by grep against the wholecardpickerpackage finding zero call sites outside its own tests. Documented in the new doc section rather than silently dropped.Test plan
.github/scripts/tests/test_check_extractor_manifest_sync.py+.github/scripts/tests/test_check_extractor_ownership_totality.py, individually and together (python3 -m pytestboth files in one invocation) - 39 passed, no cross-file state leakage between the two scripts'REPO_ROOTmonkeypatches.test_undeclared_new_contributor_fails) proving a newly-added, newly-called module-private helper with noEXTRACTOR_OWNERSHIPentry fails CI, plus a matching case for an undeclared external call, a stale-entry case, a fake-manifest-key case, and the excluded-ladder boundary (a name called only from inside_collector_line_ocr_attemptsis correctly NOT required).check_extractor_ownership_totality.pyandcheck_extractor_manifest_sync.pyboth run clean against the real repo (17 declared contributors / 11 manifest keys).python3 .github/scripts/docs_lint.py --strictclean.cardpicker/tests/test_image_evidence.py(111 tests) andcardpicker/tests/test_golden_set.py(29 tests), individually and together against real testcontainers Postgres/ES (issue Test suite is order-dependent: leaked fetch-failure window trips the envelope across files (8 failures on master) #679's order-dependence concern) - 111 / 29 / 140 all passed, confirming the docstring-onlyimage_evidence.pychange introduced no regression.pre-commit run(ruff, isort, black, mypy, prettier, docs-lint's readme parity) green on every touched file.CLAUDE.md task-end checks
docs/features/catalog-completion-plan.mdin place (new dated section, matching that file's own established chronological-log convention) rather than a separate changelog file.