Skip to content

Slow-path: exclude cards the illustration calculator resolved; pin the name-only face reading - #655

Merged
WilfordGrimley merged 1 commit into
masterfrom
fix/slow-path-illustration-exclusion
Jul 30, 2026
Merged

Slow-path: exclude cards the illustration calculator resolved; pin the name-only face reading#655
WilfordGrimley merged 1 commit into
masterfrom
fix/slow-path-illustration-exclusion

Conversation

@WilfordGrimley

Copy link
Copy Markdown

Item 3 of the "essential major blockers" brief (audit §1 Q2, first bullet), plus the coordinator's face_illustrations addition folded in as test-only.

1. The slow-path illustration exclusion

_slow_path_eligible_cards_queryset excluded already_routed and fallback_voted and nothing for the illustration calculator. management/commands/local_calculate_verdicts.py's own sequencing comment admitted it:

the slow-path queryset would need an additional exclusion for this identity's votes, similar to the fallback-voted-card exclusion it already carries

PR #604 did not close it.

Failure direction is wrong human work, not a silent no-op. Stage D sequences join-key → fallback → illustration → slow-path, so a card the illustration calculator resolves is routed to a reviewer moments later in the same invocation — a human asked to identify a card the pipeline just identified. Bounded so far only because stage-d-illustration-v2 has never run; the read-only replay in docs/pipeline-fidelity-gate.md projects ~3,233 printing votes, so it fires on the first -v2 run. This is a pre-fire fix, not a cleanup.

The fix is one .exclude(pk__in=illustration_voted_card_ids), built exactly like the fallback one:

  • is_no_match=False qualified. An illustration is_no_match vote is the calculator concluding it cannot identify the card — precisely a card a reviewer should see. Excluding those too would trade wrong human work for a silently emptied queue.
  • card_ids pushed into the subquery per PR perf: push card_ids into every Stage D dependency subquery (#533) #579 — an uncorrelated IN (SELECT ...) over a 167k-row table on every 25-card micro-batch otherwise.
  • Deliberately not run-scoped. "Illustration has a confident vote for this card" is a statement about the catalogue, not about a run. Scoping it would let slow-path in run B route a card resolved in run A.

The identity is a duplicated literal per this module's established "no hard import-time dependency between sibling engines" convention (the mirror of local_illustration._JOIN_KEY_NO_HIT_SKIP_REASONS). A test asserts it equals local_illustration.ILLUSTRATION_ANONYMOUS_ID, so a future -v3 bump fails there rather than silently reopening the defect.

2. face_illustrations name-only entries — audit result: nothing to fix

CanonicalPrintingMetadata.face_illustrations is about to be populated. 1,594 of 113,224 printings get a non-empty list; 1,534 carry a real illustration_id on every face and 60 are name-only ({name: ..., illustration_id: None}). Those 60 are non-empty lists containing no usable illustration, and they satisfy the partial index cpm_face_illustrations_present — so any consumer treating "non-empty list" as "has back-face art" is wrong for exactly those 60 the moment the importer runs.

Audited every reader of the field. Both real consumers are already correct:

consumer test it applies verdict
IllustrationIndex._build if not face_name or face_illustration_id is None: continue correct
printings_for_illustration JSONB __contains=[{"illustration_id": "<uuid>"}] — a None cannot satisfy it correct
_illustration_index_version_stamp .exclude(face_illustrations=[]).count() correct as a change detector — its job is cache invalidation, not "has usable art". A name-only entry appearing is a change; under-counting it would leave a stale index cached.

Nothing collapsed to [] and nothing filtered: a name-only entry is the record that we looked at that face and Scryfall publishes no illustration for it, and indices are positional so None must be retained to keep index N at face N. The data is correct; only a careless reading would be wrong.

So the readings are pinned with a name-only fixture instead — including a control proving the row is not merely invisible, and a mixed partly-name-only case asserting positions survive in storage.

Verification

mutation expected red result
remove the illustration exclusion resolved-not-routed, cross-run, end-to-end, subquery count 4 failed, 177 passed
drop the is_no_match=False qualifier (over-exclude) abstained-still-routed control 1 failed, 180 passed
bump the literal to -v3 version tripwire + the three behavioural tests 4 failed, 177 passed
index builder reads list truthiness name-only files no key; partly-name-only; the pre-existing None-string test 3 failed, 110 passed
printings_for_illustration reads list truthiness name-only never matches; the pre-existing narrowing test 2 failed, 111 passed
restored all green
  • pytest cardpicker/tests/ — 3470 passed, 11 skipped (full suite)
  • docs_lint.py --strict — clean
  • pre-commit (ruff/isort/black/mypy/prettier) — clean
  • No migration.

test_slow_path_scopes_every_dependency_subquery's expected_count went 4 → 5 as that test's own error message instructs, rather than being relaxed.

Docs

docs/identification-pipeline.md — new "Everything RESOLVED does not" paragraph stating the three deliberate properties of the exclusions (living wiki doc; no dated report).

🤖 Generated with Claude Code

https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

…e name-only face reading

`_slow_path_eligible_cards_queryset` excluded `already_routed` and
`fallback_voted` and had NOTHING for the illustration calculator.
`management/commands/local_calculate_verdicts.py`'s own sequencing comment
admitted it - "the slow-path queryset would need an additional exclusion for
this identity's votes". PR #604 did not close it.

Failure direction is WRONG HUMAN WORK, not a silent no-op: Stage D sequences
join-key -> fallback -> illustration -> slow-path, so a card the illustration
calculator resolves is routed to a reviewer moments later in the SAME
invocation, asking a human to identify a card the pipeline just identified.
Bounded so far only because `stage-d-illustration-v2` has never run; the
read-only replay in docs/pipeline-fidelity-gate.md projects ~3,233 printing
votes, so it fires on the first `-v2` run. This is a pre-fire fix.

One `.exclude(pk__in=illustration_voted_card_ids)`, built exactly like the
fallback one: `is_no_match=False` qualified (an illustration `is_no_match` vote
is the calculator CONCLUDING it cannot identify the card - precisely a card a
reviewer should see), `card_ids`-pushed-down per PR #579, and deliberately NOT
run-scoped, because "illustration has a confident vote for this card" is a
statement about the catalogue rather than about a run.

The identity is a duplicated literal per this module's established "no hard
import-time dependency between sibling engines" convention - with a test
asserting it equals `local_illustration.ILLUSTRATION_ANONYMOUS_ID`, so a future
`-v3` bump fails there instead of silently reopening the defect.

ALSO, test-only: `CanonicalPrintingMetadata.face_illustrations` is about to be
populated in production. 1,594 of 113,224 printings get a non-empty list, and
60 of those are NAME-ONLY (`{name: ..., illustration_id: None}` throughout) -
non-empty lists carrying no usable illustration, which satisfy the partial
index `cpm_face_illustrations_present`. Any consumer reading list truthiness as
"has back-face art" is wrong for exactly those 60 the moment the importer runs.

Audited every consumer. BOTH are already correct: `IllustrationIndex._build`
tests `illustration_id is None`, and `printings_for_illustration`'s JSONB
containment asks for a real uuid a `None` cannot satisfy. The version stamp's
`.exclude(face_illustrations=[])` is a CHANGE DETECTOR, where counting a
name-only row is the correct behaviour. Nothing to fix - so the readings are
pinned with a name-only fixture instead, proven by mutation to fail against the
truthiness reading in both consumers.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
@WilfordGrimley
WilfordGrimley force-pushed the fix/slow-path-illustration-exclusion branch from d7b2bc9 to 3e72593 Compare July 30, 2026 11:06
@WilfordGrimley
WilfordGrimley merged commit 1d5bf96 into master Jul 30, 2026
13 checks passed
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