Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@
)
from cardpicker.printing_consensus import (
NO_MATCH,
md5_group_cards,
md5_group_key,
identity_group_cards,
identity_group_key,
resolve_and_persist_printing,
resolve_printing,
)
Expand Down Expand Up @@ -139,10 +139,11 @@ def _would_be_printing_status(card: Card, group_card_ids: Sequence[int] | None =
file's own scope is meant to stay a self-contained management command) - see this module's
own docstring for why the dry-run path needs a non-writing prediction at all, unlike --apply.

`group_card_ids` is the md5 identity group `_recompute_printing` has already materialized
for this card (issue #473), passed through so the dry run doesn't re-derive the same group
per prediction. Omitting it (as `consensus_impact_report` does) is still correct, just one
query less efficient: `resolve_printing` derives the group itself when it isn't told.
`group_card_ids` is the combined identity group `_recompute_printing` has already
materialized for this card (issue #473, widened by #661), passed through so the dry run
doesn't re-derive the same group per prediction. Omitting it (as `consensus_impact_report`
does) is still correct, just less efficient: `resolve_printing` derives the group itself
when it isn't told.
"""
result = resolve_printing(card, group_card_ids=group_card_ids)
if result is None:
Expand Down Expand Up @@ -278,11 +279,11 @@ def _recompute_printing(report: dict[str, Any], apply: bool, batch_size: int, sa
with transaction.atomic():
cards = Card.objects.filter(pk__in=batch_ids).prefetch_related("printing_tags")
for card in cards:
group_key = md5_group_key(card)
group_key = identity_group_key(card)
if group_key in seen_group_keys:
continue
seen_group_keys.add(group_key)
members = md5_group_cards(card)
members = identity_group_cards(card)
before_by_member = [(member, member.printing_tag_status) for member in members]
if apply:
resolve_and_persist_printing(card, members=members)
Expand Down
309 changes: 266 additions & 43 deletions MPCAutofill/cardpicker/printing_consensus.py

Large diffs are not rendered by default.

32 changes: 17 additions & 15 deletions MPCAutofill/cardpicker/question_feed.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@
build_group_printing_vote_tuples,
get_contested_card_ids,
group_printing_votes,
md5_group_expanded_card_ids,
identity_group_expanded_card_ids,
)
from cardpicker.reason_tags import NOT_OFFICIAL_ART_REASON_TAGS
from cardpicker.schema_types import QuestionFeedCounts, QuestionFeedItem, TypeEnum
Expand Down Expand Up @@ -245,11 +245,12 @@ def _printing_vote_tuples(card: Card) -> list[VoteTuple]:

def _voter_answered_printing_card_ids(anonymous_id: str) -> set[int]:
"""
Every card this voter has already cast a printing vote on, WIDENED to those cards' full md5
identity groups (`printing_consensus.md5_group_expanded_card_ids`) - the exclusion set the
printing tiers below filter against, so a voter who answered one member of a group is never
asked the same byte-identical image again under a sibling's identifier (issue #473: the feed
serves one member per group, not N).
Every card this voter has already cast a printing vote on, WIDENED to those cards' full
combined identity groups (`printing_consensus.identity_group_expanded_card_ids` - md5 union
artbox-phash-d0, issue #661) - the exclusion set the printing tiers below filter against, so
a voter who answered one member of a group is never asked the same byte-identical or
phash-d0-identical image again under a sibling's identifier (issue #473: the feed serves one
member per group, not N).

This replaces the `.exclude(printing_tags__anonymous_id=anonymous_id)` clause those tiers
used before, and is exactly equivalent to it for a card whose group is itself alone (the
Expand All @@ -264,29 +265,30 @@ def _voter_answered_printing_card_ids(anonymous_id: str) -> set[int]:
direct caller - a test, a shell - can still ask for one tier by `anonymous_id` alone.
"""
voted_card_ids = CardPrintingTag.objects.filter(anonymous_id=anonymous_id).values_list("card_id", flat=True)
return md5_group_expanded_card_ids(voted_card_ids)
return identity_group_expanded_card_ids(voted_card_ids)


def _voter_answered_artist_card_ids(anonymous_id: str) -> set[int]:
"""
The artist-tier analogue of `_voter_answered_printing_card_ids` above: every card this voter
has already cast a `CardArtistVote` on, widened to those cards' full md5 identity groups, so
a voter who answered one member of a byte-identical group is not re-asked the same artist
question under a sibling's identifier (issue #473). Scoped to `_tier_2_contested` only
has already cast a `CardArtistVote` on, widened to those cards' full combined identity
groups, so a voter who answered one member of a byte-identical or phash-d0-identical group is
not re-asked the same artist question under a sibling's identifier (issue #473, widened by
#661). Scoped to `_tier_2_contested` only
(2026-08-04 gate on the phase-C/md5 routing brief) - `_tier_4_fresh`'s own artist exclusion
keeps its pre-existing, unwidened `.exclude(artist_votes__anonymous_id=...)` form.

COMPUTED ONCE PER FEED REQUEST, mirroring `_voter_answered_printing_card_ids`'s own
convention exactly.
"""
voted_card_ids = CardArtistVote.objects.filter(anonymous_id=anonymous_id).values_list("card_id", flat=True)
return md5_group_expanded_card_ids(voted_card_ids)
return identity_group_expanded_card_ids(voted_card_ids)


def _voter_answered_tag_card_ids_by_tag(anonymous_id: str) -> dict[str, set[int]]:
"""
For every tag name this voter has cast a `CardTagVote` on, the set of card ids - each widened
to its full md5 identity group - that count as "already answered" for THAT tag. Widening is
to its full combined identity group - that count as "already answered" for THAT tag. Widening is
on the CARD axis only, never the tag axis: `_tier_2_contested`'s own-vote exclusion is
deliberately scoped to (card, tag, anonymous_id), not (card, anonymous_id) - a card carries
~11 independent attribute-chip tags, and a card-level exclude would silently hide every other
Expand All @@ -304,7 +306,7 @@ def _voter_answered_tag_card_ids_by_tag(anonymous_id: str) -> dict[str, set[int]
card_ids_by_tag: dict[str, set[int]] = defaultdict(set)
for tag_name, card_id in rows:
card_ids_by_tag[tag_name].add(card_id)
return {tag_name: md5_group_expanded_card_ids(card_ids) for tag_name, card_ids in card_ids_by_tag.items()}
return {tag_name: identity_group_expanded_card_ids(card_ids) for tag_name, card_ids in card_ids_by_tag.items()}


def _not_official_art_card_ids() -> set[int]:
Expand All @@ -322,7 +324,7 @@ def _not_official_art_card_ids() -> set[int]:
caster from writing one in principle, and this routing signal is meant to represent an
actual human declaration that the artwork question is meaningless for this card - a future
machine-cast source earning the same trust would need its own explicit decision, not a
silent inclusion here.
silent inclusion here. Widened via the combined identity group, not md5 alone (issue #661).

Unlike `_voter_answered_printing_card_ids`/`_voter_answered_artist_card_ids` above, this is
NOT per-voter: it is a fact about the CARD, so it applies identically to every voter's feed.
Expand All @@ -332,7 +334,7 @@ def _not_official_art_card_ids() -> set[int]:
tag__name__in=NOT_OFFICIAL_ART_REASON_TAGS, polarity=VotePolarity.APPLY
).values_list("card_id", "source")
human_backed_card_ids = {card_id for card_id, source in rows if is_human_backed_source(source)}
return md5_group_expanded_card_ids(human_backed_card_ids)
return identity_group_expanded_card_ids(human_backed_card_ids)


def is_likely_resolve_printing(card: Card) -> bool:
Expand Down
15 changes: 13 additions & 2 deletions MPCAutofill/cardpicker/tests/test_md5_group_pooling.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,13 +246,24 @@ def test_singleton_votes_carry_no_pooling_key(self, db):
vote_tuples = build_group_printing_vote_tuples(votes, pool=is_group)
assert [vote.dedupe_key for vote in vote_tuples] == [None]

def test_singleton_read_honours_a_callers_prefetch_and_adds_no_query(self, db, django_assert_num_queries):
def test_singleton_read_honours_a_callers_prefetch_and_adds_one_phash_check_query(
self, db, django_assert_num_queries
):
"""
Zero EXTRA queries beyond the one issue #661 always adds: `group_printing_votes` now
derives `identity_group_card_ids`, not `md5_group_card_ids` alone, and the phash half of
that (`_card_artbox_phash`) is a query against `ImageEvidence` no matter what the md5
checksum lookup found - `artbox_phash` lives on a related model, not on `Card` itself
(see `_card_artbox_phash`'s own docstring). The checksum half stays free (a `getattr`),
and the caller's own `prefetch_related("printing_tags")` is still honoured for the votes
read itself - this is the one query the phash channel adds, not a second per-vote cost.
"""
card = CardFactory()
printing = CanonicalCardFactory()
human_vote(card, printing, "human-1")
prefetched = list(Card.objects.filter(pk=card.pk).prefetch_related("printing_tags"))[0]

with django_assert_num_queries(0):
with django_assert_num_queries(1):
votes, is_group = group_printing_votes(prefetched)

assert is_group is False
Expand Down
Loading
Loading