Skip to content

feat(local_phash): local-only resumable backfill for CanonicalCard.image_hash - #694

Closed
WilfordGrimley wants to merge 1 commit into
masterfrom
feat/local-backfill-canonical-hash
Closed

feat(local_phash): local-only resumable backfill for CanonicalCard.image_hash#694
WilfordGrimley wants to merge 1 commit into
masterfrom
feat/local-backfill-canonical-hash

Conversation

@WilfordGrimley

Copy link
Copy Markdown

Summary

CanonicalCard.image_hash is the reference corpus every phash comparison
scores against (local_phash.find_best_match via
identify_land_printing/recover_frame_mismatch_printing_via_phash, and
any future illustration matching). It's populated lazily by
get_or_compute_canonical_hash, which caches forever on the premise a
printing's art crop never changes.

Measured against production (2026-08-05, read-only count, confirmed below):
27,151/113,224 canonical printings are still at the unset image_hash=0
sentinel.
CanonicalPrintingMetadata.art_crop_url (issue #339's local-first
fix) is populated on all 113,224/113,224 printings, so this backlog is
computable with zero Scryfall REST calls - only the image-CDN fetch that
produces the hash itself, which the same measurement below confirms fires
for all 27,151 rows (skipped_no_local_url would be 0).

This PR adds run_canonical_hash_backfill (local_phash.py) and its
management command, local_backfill_canonical_hash, following the existing
run_content_phash_backfill/local_backfill_content_phash sibling's shape:

  • Idempotent/resumable by construction: selects on image_hash=0, no
    separate --resume flag - a plain re-invocation after a kill just picks
    up what's left.
  • Pipelined: one long-lived thread pool for the whole run, a sliding
    submission window (batch_size * queue_depth_batches), checkpoint-flush
    per batch as fetches complete - identical shape/out-of-order-completion
    safety argument as the content-phash sibling.
  • Local-only by default: a printing with no local art_crop_url is
    counted in skipped_no_local_url and left alone, never silently routed
    to a live REST call. --allow-remote is an explicit opt-in escape hatch
    for deliberately closing a residual gap later (default off).
  • No separate rate-limit flag: unlike the content-phash sibling's
    --rate-limit-per-sec, _fetch_and_hash already goes through
    harvest_fetch_limiter.rate_limited_get(SCRYFALL_CDN, ...), which paces
    and bounds concurrency (10 req/s, max_concurrency=5) at the destination-
    limiter layer - a second uncoordinated pacer on top would only add a
    redundant ceiling.
  • --workers defaults to 5, matching SCRYFALL_CDN.max_concurrency (load
    discipline per the brief - a wider pool just queues behind that
    destination's own semaphore, it doesn't raise real throughput or host
    load).
  • --dry-run (off by default - must be passed explicitly) reports the
    selected/full-backlog counts, hashed/skipped/failed counts, and an
    elapsed-rate-based wall-clock estimate for the full backlog.

Does not touch find_best_match, its 20/5 distance/margin calibration, or
any matching behaviour (issue #419's scope) - this populates the corpus, it
does not consume it. Does not run the backfill itself - writing 27,151 rows
is an owner-authorised operation, out of scope for this PR.

Deviations from the brief

  • Corrected number confirmed, not corrected: the brief's 27,151 figure
    was verified exactly via a read-only production count (see Verification).
  • --allow-remote was added (the brief called it a judgement call,
    default off either way): kept because it's a near-zero-cost wire-through
    of the REST fallback get_or_compute_canonical_hash already has, and the
    measurement below shows the local URL currently covers 100% of the
    backlog - so the flag is a deliberate, rarely-needed escape hatch rather
    than a routine one.
  • No PilotRunLedger integration (unlike backfill_md5_checksums's
    forced-dry-run-guard family): followed the local_backfill_content_phash
    sibling's simpler shape instead, since this command lives in the same
    module and the brief pointed at that command as a convention model for
    "CLI shape, --dry-run, and progress reporting."

Test plan

  • pytest cardpicker/tests/test_local_phash.py - 33 passed
    (individually)
  • pytest cardpicker/tests/test_local_phash.py cardpicker/tests/test_local_identify_printing_tags.py - 252 passed
    (together, per issue Test suite is order-dependent: leaked fetch-failure window trips the envelope across files (8 failures on master) #679's order-dependence concern)
  • Each new test class individually
    (TestCanonicalHashBackfill/TestCanonicalHashBackfillCommandCLI/
    TestCanonicalHashBackfillPipelineOutOfOrder) - all pass in isolation
  • pre-commit run on all three touched files (ruff, isort, black,
    mypy, prettier) - all green
  • Read-only --dry-run selection-count check executed directly against
    the live production DB (docker exec mpcautofill_django python manage.py shell, ORM .count() only, no writes, no mutating command run):
    CanonicalCard.objects.filter(image_hash=0).count() = 27,151, exactly
    matching the brief's figure. Also confirmed
    .filter(image_hash=0, printing_metadata__art_crop_url='').count() = 0
    and .filter(image_hash=0, printing_metadata__isnull=True).count() = 0 -
    every backlog row has a local art-crop URL, so skipped_no_local_url
    will be 0 on the real run and --allow-remote currently has nothing to
    do.
  • lsp_diagnostics - attempted once, timed out (known best-effort
    limitation on this box, per PR fix(image_evidence): wire the artist-crop fallback into live extraction #685's precedent); not retried - mypy +
    pytest substitute.
  • CI - not watched by this session per dispatch convention.

Task-end checks (CLAUDE.md)

  • Docs convention: no docs/ file documents this backfill's internals
    beyond the code's own docstrings (matching the content-phash sibling,
    which also has none); no blocker exceeded 15 minutes, so no
    docs/troubleshooting.md entry needed.
  • Policy text dates: not applicable.
  • Wiki maintenance: not applicable - an operator-only management command,
    not user- or admin-facing product behaviour.
  • Extractable-primitives ledger: not applicable - no new primitive
    independent of the vote/consensus system was created or removed.
  • Constant renames: none - all new constants
    (DEFAULT_CANONICAL_HASH_BACKFILL_BATCH_SIZE/_WORKERS) are newly added,
    nothing existing was renamed/extracted/retired.
  • Push policy: pushed to a dedicated branch
    (feat/local-backfill-canonical-hash, cut fresh from origin/master)
    and this PR opened per this task's explicit deliverable instructions, not
    the solo-work default. Not merging - merges are owner-only, and running
    the backfill itself is explicitly out of scope for this PR.

…age_hash

Closes the reference-corpus gap (27,151/113,224 canonical printings still
at the unset image_hash=0 sentinel, confirmed via a read-only dry-run
count against production 2026-08-05). CanonicalPrintingMetadata.art_crop_url
is populated on 113,224/113,224 printings (issue #339), so the entire
backlog is computable with zero Scryfall REST calls - only the image CDN
fetch that produces the hash itself.

- run_canonical_hash_backfill (local_phash.py): same pipelined,
  checkpoint-per-batch shape as run_content_phash_backfill. Selects on
  image_hash=0 (idempotent/resumable, no separate --resume flag). Local-only
  by default - a printing with no local art_crop_url is counted in
  skipped_no_local_url and left alone, not silently routed to a live REST
  call. --allow-remote is an explicit opt-in escape hatch, default off.
  No separate rate-limit parameter: _fetch_and_hash already goes through
  harvest_fetch_limiter's SCRYFALL_CDN destination limiter.
- local_backfill_canonical_hash management command: --dry-run (default
  off - explicit flag required to avoid writing), --batch-size, --workers
  (default 5, matching SCRYFALL_CDN.max_concurrency), --queue-depth-batches,
  --limit, --allow-remote, --nice. Dry-run reports selected/backlog,
  hashed/skipped/failed counts, and an elapsed-rate-based wall-clock
  estimate for the full backlog.
- Tests mirror TestContentPhashBackfill/TestBackfillCommandCLI/
  TestPipelinedBackfillOutOfOrder from test_local_identify_printing_tags.py,
  adapted for CanonicalCard + the missing-local-url skip path.

Does not run the backfill or touch find_best_match's matching behaviour -
populates the corpus, doesn't consume it.
@WilfordGrimley

Copy link
Copy Markdown
Author

Closing unmerged and deferring to #697, per owner decision 2026-08-05.

This builds the cross-source corpus (Scryfall art_crop fetched and hashed per printing), which contradicts #508's standing ruling that exemplars come from our own DB, never Scryfall images. That rationale is technical as well as bandwidth-related: cross-source crop framing makes Hamming distances unreliable, which is why find_best_match needs distance=20/margin=5 where #508's self-referential discipline is d=0 identity and 0<d<=2 narrowing.

Also corrected in the process: this backfill is not a local operation. _fetch_and_hash downloads image bytes via rate_limited_get(SCRYFALL_CDN, ...); the 2026-07-19 local-first change removed the per-printing REST metadata call, not the image fetch. Completing the corpus means 27,151 CDN downloads.

The branch is preserved. The batching, resumability, idempotence, --dry-run and tests are sound and are being reused directly for the self-referential exemplar corpus. This is a redirect, not a rejection of the work.

WilfordGrimley added a commit that referenced this pull request Aug 5, 2026
…ndex (issue #508 phase 1) (#701)

Builds ArtboxPhashExemplar, the second illustration-deduction path's
reference index, and its read-only backfill/retraction commands.
Reuses the batched/resumable/idempotent shape from PR #694
(feat/local-backfill-canonical-hash) - only the source changes, from
Scryfall CDN fetches to a scan of our own ImageEvidence; this backfill
needs none of that branch's threaded fetch pipeline since every input
is already a local DB read.

Seeds from two sources, per the owner's 2026-08-05 extension of #508's
original human-only design: human-backed printing resolutions
(printing_tag_status == RESOLVED, always human-backed by the
consensus gate) and unopposed join-key machine votes at 0.75/0.85
confidence. The artist-disagreement tier (0.65) and the no-match tier
(0.6) are both excluded from seeding. Every exemplar records its own
seed kind, confidence, and a seed_group_key that lets a bad seed
retract together with everything it seeded in one filtered delete.

Never fetches Scryfall images - phash comparability requires the same
crop geometry our own extractor guarantees. Phase 1 scope only: no
matching calculator, no vote, no change to consensus or the
human-backed resolution gate.

Documents the mechanism in docs/identification-pipeline.md's Parallel
detectors section, stating explicitly what it does not do.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant