perf: push `card_ids` into every Stage D dependency subquery (#533) by WilfordGrimley · Pull Request #579 · ProxyPrints/ProxyPrints.github.io

WilfordGrimley · 2026-07-29T15:35:42Z

The last surviving instance of the defect class PR #541 was written to eliminate — and the one that mattered most. #541 fixed five calculators outside the dispatch loop and explicitly did not touch these, so the defect survived in three of the four calculators already in Stage E's hot path.

The defect

.filter(pk__in=<values_list qs>) compiles to an uncorrelated IN (SELECT ...). The outer .filter(pk__in=card_ids) therefore bounds the rows returned, not the work done: every micro-batch paid a full pass over CardPrintingTag (167,229 rows live) or CardScanLog (2,617,333 rows live, append-only, still growing) regardless of batch size.

function	dependency subqueries	scoped before
`_fallback_eligible_cards_queryset`	3	1 (the #469 own-exclusion)
`_slow_path_eligible_cards_queryset`	4	0
`local_illustration.run_illustration_calculator` → `_eligible_illustration_cards_queryset`	3	1 (the #526 own-exclusion)

The illustration case is the instructive one: that pair was already lazy, and a comment said so. Laziness only decides whether the rows land in Python memory — it says nothing about whether the database scans the table. _eligible_illustration_cards_queryset receives the pair as arguments and cannot scope what it is handed, so the fix has to be at the call site. Its docstring now carries that as an explicit caller contract.

_join_key_no_hit_subqueries is now the single place the join-key no-hit pair is built, for all three calculators, so a fourth cannot repeat the omission. The slow-path calculator's own two exclusions (already-routed CardScanLog, fallback-voted CardPrintingTag) are scoped inline alongside them.

Measurements

Live catalogue, eligibility query alone (no calculator work), median of 12 reps, read-only session:

calculator	batch 25	batch 250
`stage-d-fallback-v1`	1113.3 → 2.6 ms (434x)	1212.5 → 8.1 ms (149x)
`stage-d-slow-path-v1`	959.5 → 2.2 ms (445x)	991.0 → 7.5 ms (132x)
`stage-d-illustration-v2`	1117.2 → 2.9 ms (382x)	2101.8 → 10.7 ms (196x)

~3.2 s of fixed per-invocation cost removed at the production batch size of 25 — which at that batch size outweighed all nine calculators' actual compute, and is why Stage D cost 174–196 ms/card at batch 25 against 28–32 ms/card at batch 250.

Result sets are identical before and after: stage-d-illustration-v2 returned the same 25 / 250 rows either way. stage-d-fallback-v1 and stage-d-slow-path-v1 returned 0 rows for every batch — their production backlogs are genuinely drained right now (0 eligible cards across a 20,000-card sample of each branch of the join-key no-hit population), so 0 is the true live answer, not a scoping artefact. Set equivalence is pinned by tests instead.

`card_ids=None` is byte-identical

The compiled SQL of all three eligibility queries was diffed against origin/master's under a fixed PYTHONHASHSEED (the skip_reason IN (...) literal order is frozenset iteration order and varies per process, so the seed has to be pinned to compare at all). Identical to the byte — 7,548 bytes, md5 0fcecfb5eab427e051b33e384d9c61fd on both revisions.

Tests assert on the compiled SQL

Per #541's established rationale, and it is load-bearing here: the outer filter produces the same rows whether or not the push-down happened, so a result-set test is green either way and proves nothing. _dependency_subqueries slices every (SELECT U0."card_id" FROM ...) out of the compiled statement by balanced parentheses — necessary because the outer "cardpicker_card"."id" IN (<pks>) term carries the same pk literals in both shapes, and an assertion against the whole statement would be satisfied by it.

Each calculator gets: every dependency subquery scoped (with an exact expected count, so an added-but-unscoped subquery fails rather than slips through); the pre-fix shape reconstructed literally and shown to leave exactly the two join-key subqueries unscoped; BULK mode taking no scoping branch at all; and the result-set equivalence as the second half of the contract.

Mutation-proved. Reverting both push-downs turns exactly three tests red — test_fallback_scopes_every_dependency_subquery, test_slow_path_scopes_every_dependency_subquery, test_the_join_key_populations_are_scoped_to_card_ids_in_the_compiled_sql — while every result-set test in the same class stays green. That is the vacuous-green defect demonstrated rather than argued.

Verification

cardpicker/tests/: 3120 passed, 11 skipped, 0 failed.
black / isort / ruff / mypy clean (pre-commit).
No migrations touched.

⚠️ Running the suite locally required a throwaway merge migration, not committed: master currently has two 0096_* leaves (0096_card_scan_log_anon_skip_idx, 0096_freeze_deductive_backfill_zero_weight_cohort), so migrate fails with multiple leaf nodes and no test database can be built. CI will hit the same wall until that lands.

Refs #533, #541, #469, #526, #458, #460.

🤖 Generated with Claude Code

https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

WilfordGrimley · 2026-07-29T15:44:16Z

CI: Backend tests red for pre-existing reasons only — nothing here.

= 6 failed, 725 passed, 11 skipped, 2389 errors =

2,389 errors — all one cause: CommandError: Conflicting migrations detected; multiple leaf nodes in the migration graph: (0096_card_scan_log_anon_skip_idx, 0096_freeze_deductive_backfill_zero_weight_cohort in cardpicker). No test database can be built on current master. This PR touches no migrations.
6 failures — all ValueError: Missing staticfiles manifest entry for 'cardpicker/favicon.ico' in test_saved_deck_views.py / test_saved_deck_share_views.py. Unrelated to Stage D.

Same shape on a sibling branch that touches nothing related (fix/protected-core-skip-reason-constants, run 30466566482): = 6 failed, 768 passed, 11 skipped, 2376 errors = — identical 6 staticfiles failures, identical migration-graph errors. Every branch based on current master is red this way; fix/migration-graph-single-leaf (run 30464037241) is the only recent green one.

Locally, with a throwaway merge migration to make a test database buildable at all (not committed): cardpicker/tests/ = 3120 passed, 11 skipped, 0 failed.

Formatting and static type checking (ruff / isort / black / mypy) passes here.

Needs a rebase once the migration-graph fix lands; expected green then.

The last surviving instance of the defect class PR #541 was written to eliminate - and the one that mattered most, because #541 fixed five calculators OUTSIDE the dispatch loop and explicitly did not touch these, so it survived in three of the four calculators already in Stage E's hot path. `_fallback_eligible_cards_queryset` forwarded `card_ids` to `_eligible_cards_queryset` (the outer `Card` query and its own-exclusion) but built its two join-key no-hit DEPENDENCY SUBQUERIES unscoped. `_slow_path_eligible_cards_queryset` built four unscoped. And `local_illustration.run_illustration_calculator` built the same join-key pair unscoped at its call site, then handed it to `_eligible_illustration_cards_queryset`, which cannot scope what it receives as an argument. Django compiles `.filter(pk__in=<values_list qs>)` as an UNCORRELATED `IN (SELECT ...)`. The outer `.filter(pk__in=card_ids)` therefore bounds the ROWS RETURNED, not the WORK DONE: every micro-batch paid a full pass over `CardPrintingTag` (167,229 rows live) or `CardScanLog` (2,617,333 rows live, append-only, still growing) regardless of batch size. Keeping the subqueries LAZY - which `local_illustration` already did - does not help; laziness only decides whether the rows land in Python memory, not whether the database scans the table. `_join_key_no_hit_subqueries` is now the single place that pair is built, for all three calculators, so a fourth cannot repeat the omission. The slow-path calculator's own two exclusions (already-routed `CardScanLog`, fallback-voted `CardPrintingTag`) are scoped inline alongside them. Measured against the live catalogue, eligibility query alone, no calculator work, median of 12 reps, read-only: batch 25 batch 250 stage-d-fallback-v1 1113.3 -> 2.6 ms 1212.5 -> 8.1 ms stage-d-slow-path-v1 959.5 -> 2.2 ms 991.0 -> 7.5 ms stage-d-illustration-v2 1117.2 -> 2.9 ms 2101.8 -> 10.7 ms ~3.2 s of fixed per-invocation cost removed at the production batch size of 25 - which at that batch size outweighed all nine calculators' actual compute, and is why Stage D cost 174-196 ms/card at batch 25 against 28-32 ms/card at batch 250. BULK mode (`card_ids=None`, every management-command caller) is byte-identical: the compiled SQL of all three eligibility queries was diffed against `origin/master`'s under a fixed `PYTHONHASHSEED` and is unchanged to the byte. Tests assert on the COMPILED SQL, not the result set, per #541's established rationale: the outer filter produces the same rows whether or not the push-down happened, so a result-set test is green either way and proves nothing. The new SQL assertions were mutation-proved - reverting the push-down turns exactly those three red while every result-set test in the same class stays green, which is the vacuous-green defect demonstrated rather than argued. Refs #533, #541, #469, #526, #458, #460. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

Semantic (not textual) conflict surfaced by rebasing onto master after PR #567 landed. #567 renamed `SLOW_PATH_TO_REVIEW_REASON` -> `SLOW_PATH_TO_REVIEW_SKIP_REASON` in `cardpicker/local_calculate_verdicts.py`; this branch had concurrently ADDED a new test (`test_slow_path_scoped_and_unscoped_eligible_sets_agree`) referencing the OLD name. Git merged both cleanly - #567 touched only the declaration, this branch only added lines - so the breakage was invisible in the diff and would have shown up as a `NameError: SLOW_PATH_TO_REVIEW_REASON` at test run time (the import block at the top of the file already imports the NEW name, so collection succeeds and only this one test explodes). No behaviour change: the value behind both names is the same string "to-review". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

* Autoscale the Stage E micro-batch size from measured hardware (#533 follow-on) `settings.STAGE_E_MICRO_BATCH_SIZE = 25` was an explicit placeholder - the stage-e-streaming brief's §10(c) said the real number ships as a MEASURED output and nobody had measured it. Measured now, against the live catalog, and the answer is not a constant. New `cardpicker/stage_e_batch_sizing.py` decides the size from three terms, smallest wins, all three measured or ratified: * SATURATION - 250 cards. One dispatch costs `F + (m + c) * N`; only `F/N` improves with batch size, and `c` (Stage C's fetch-overlapped per-card floor, 561 ms measured) is set by `harvest_fetch_limiter.GOOGLE_IMAGE`, not by the box. 250 is the smallest tested size whose residual per-card fixed cost is under 1 ms. Deliberately NOT hardware-scaled: a bigger host buys more concurrent dispatch streams, not a bigger batch. * MEMORY - `(min(768, available/streams) - 320) / 0.002` cards, against the ratified per-worker RSS bar. Measured marginal is 1.0-1.9 KB/card, so this is worth ~224,000 cards on the production host and only ever bites on a host too small to hold the working set at all. * DURATION - `300s / (561ms * max(1, 7.0/usable_cores))`. The batch is the envelope's sampling interval AND the kill-loss bound; 300 s is `DEFAULT_PROGRESS_EVERY_SECONDS`. The inflation comes from HOST_LOAD_CEILING (flat 7.0 on every host) divided by discovered cores - the worst contention a dispatch may legally start under - never from a live getloadavg sample, which would make the size differ per dispatch. Measured effect on the production host: 25 -> 250 cards, per-card batch-fixed cost 8.70 -> 2.39 ms, peak RSS flat at ~298 MB (39% of the 768 MB bar) across N=10..2000. Two modes. MODE_BULK (`stream_full_catalog`, the whole-catalogue pass) gets the autoscaled size. MODE_INCREMENTAL (the event echo via `dispatch_for_card`, and `stream_backstop_sweep`, whose footprint is `--max-batches` x batch size with a default of 1000) stays on 25 - both want a small bounded unit of work, and both are behaviourally unchanged. Precedence is unchanged where it matters: `--batch-size N` wins outright, a non-None `settings.STAGE_E_MICRO_BATCH_SIZE` wins next, the rule last. That setting now defaults to None so "an operator chose 25" is distinguishable from "nobody chose anything". `--batch-size auto` names the rule explicitly, and every run prints the chosen size, its source and the binding term before batch 0, so `stream_full_catalog`'s "every tunable is a flag, changing one never costs a redeploy" property survives intact. Refs #533, #579, #458, #472. * Correct the false per-process fetch ceiling in Stage E batch sizing `discover_host` sized dispatch concurrency as min(STAGE_E_MAX_CONCURRENT_DISPATCHES, GOOGLE_IMAGE.max_concurrency, usable_cores) and two of those three terms are wrong for the question being asked. `GOOGLE_IMAGE.max_concurrency` was justified as "a stream past the limiter's sixth just blocks on its semaphore". It does not. That ceiling is `threading.Semaphore(config.max_concurrency)`, constructed once per PROCESS in `harvest_fetch_limiter._DestinationLimiter.__init__`, and concurrent dispatches are separate OS processes (`stage_e_concurrency`: django-q2 workers are "separate OS PROCESSES (multiprocessing, not threads)"). Each builds its own full-strength Semaphore(6) that cannot see the others. This is the same defect `run_image_evidence_cohort` documents having needed a per-worker "descaling hack" for, back when fetching lived inside N compute processes; the conveyor is still the multi-process case, so no cross-process mechanism enforces that 6. `usable_cores` bounds who is SCHEDULED, not who is RESIDENT. A descheduled dispatch process still holds its whole RSS, and this number is the divisor in `_memory_limit`. Both trims therefore UNDER-counted the resident process set, in the unsafe direction: with a cap of 12 the rule computed 6 and handed each process twice the memory budget it may really take. The only cross-process cap on dispatch count is `stage_e_concurrency`'s Postgres advisory-lock slots, i.e. the setting itself, so the setting is the whole answer. - `HostProfile.dispatch_streams` -> `concurrent_dispatches`, a PROCESS count, equal to `STAGE_E_MAX_CONCURRENT_DISPATCHES` with no `min()`. - New `FETCH_THREADS_PER_DISPATCH = 1` (the single `_stage_c_fetch_ahead_worker` thread), and `HostProfile.aggregate_fetch_threads` = the honest cross-process product, explicitly NOT clamped to 6, with the `threading.Semaphore` line and its multi-process consequence named at the term. - New `HostProfile.fetch_overcommitted` + a shouted `FETCH-OVERCOMMIT:` prefix on `describe()`. Sizing cannot fix an overcommit (it is a function of process count), so the rule reports it instead of hiding it. Production default of 2 is well inside the budget of 6, so this is a guard, not a live alarm. - Module docstring's "aggregate throughput across dispatches is capped by GOOGLE_IMAGE.max_concurrency = 6" removed as false. SATURATION_BATCH_SIZE=250 is unchanged and unaffected: it rests on the measured per-card `c` floor within ONE dispatch, a serial property of that single fetch thread, which needs no cross-process guarantee. Tests: three new assertions in `TestHostDiscovery`/`TestDecisionReporting` fail against the old expression with `assert 6 == 12` and `assert 6 == (12 * 1)`, plus a memory-guard test pinning the sizing consequence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

…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

…e name-only face reading (#655) `_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. Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

github-actions Bot assigned WilfordGrimley Jul 29, 2026

This was referenced Jul 29, 2026

Wire collector-line artist recovery into the pooled Stage C cohort runner #581

Merged

Backfill artist_ocr_name from already-stored collector/legal line text #582

Merged

WilfordGrimley and others added 2 commits July 29, 2026 17:14

WilfordGrimley force-pushed the perf/scope-stage-d-dependency-subqueries branch from 2991c23 to c62770e Compare July 29, 2026 17:31

WilfordGrimley mentioned this pull request Jul 29, 2026

Autoscale the Stage E micro-batch size from measured hardware #589

Merged

WilfordGrimley merged commit 1c1d6e6 into master Jul 29, 2026
9 checks passed

WilfordGrimley mentioned this pull request Jul 29, 2026

Run-scoped eligibility, the value-comparing split, and an archive for superseded votes #604

Merged

WilfordGrimley mentioned this pull request Jul 30, 2026

Monolith must accept an optional per-card / per-source scope, end to end #651

Open

WilfordGrimley mentioned this pull request Jul 30, 2026

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

Merged

Navigation Menu

Uh oh!

perf: push `card_ids` into every Stage D dependency subquery (#533) - #579

perf: push `card_ids` into every Stage D dependency subquery (#533)#579
WilfordGrimley merged 2 commits into
masterfrom
perf/scope-stage-d-dependency-subqueries

WilfordGrimley commented Jul 29, 2026

Uh oh!

WilfordGrimley commented Jul 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

WilfordGrimley commented Jul 29, 2026

The defect

Measurements

card_ids=None is byte-identical

Tests assert on the compiled SQL

Verification

Uh oh!

WilfordGrimley commented Jul 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

`card_ids=None` is byte-identical