questionFeed: exclude N>1 illustration-vote answers; dedupe contested_card_ids across the view - #738
Merged
Merged
Conversation
…_card_ids across the view - _voter_answered_printing_card_ids now also reads CardIllustrationVote, not just CardPrintingTag. The illustration-cluster answer path (cast_illustration_vote, N>1 candidates sharing an illustration) writes only CardIllustrationVote, which the printing-tier exclusion could not see - the card stayed eligible and was immediately re-served to the same voter. - get_next_question_feed_item and get_remaining_estimate both accept an optional contested_card_ids parameter (extending PR #729's convention across the view boundary); views.get_question_feed resolves get_contested_card_ids() once per request and threads it into both, instead of each independently paying the ~520-627ms query cost.
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
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
Two related backend fixes in the What's That Card question feed, sharing files.
1. Printing exclusion now sees N>1 illustration-cluster answers.
identify_printinghas twoanswer paths: a single/unclustered candidate posts to
/2/submitPrintingTag/and writesCardPrintingTag; a shared-illustration cluster of two or more candidates posts to/2/submitIllustrationVote/instead, andcast_illustration_votewritesCardPrintingTagonlywhen the illustration resolves to exactly one live printing. At N>1 - the exact premise of the
cluster UI that fired - nothing lands on the printing channel, only on
CardIllustrationVote._voter_answered_printing_card_idsread exclusively fromCardPrintingTag, so a voter whoanswered via the cluster path cast a real, persisted vote the exclusion couldn't see: the card
stayed
UNRESOLVEDand was re-served immediately. Production evidence: both of the only twohuman illustration votes on record were each followed, within seconds, by an
is_no_matchprinting vote on the same card - a voter escaping the loop, manufacturing false no-match data.
Fix:
_voter_answered_printing_card_idsnow unions card ids fromCardPrintingTagwith card idsfrom
CardIllustrationVotebefore the existing md5/phash identity-group widening, so an N>1answer excludes the card from every printing tier exactly like a direct
CardPrintingTagvotedoes. No new model or migration -
CardIllustrationVotealready exists and already carries theright (card, anonymous_id) grain.
Rejected alternatives (both considered, both wrong for this case):
CardQuestionAbstention(PR Differentiate 'Not sure' from 'Skip': record a human abstention (issue #712) #731's "Not sure" record): that model represents ahuman non-answer ("I looked and couldn't tell"); an N>1 illustration vote is a real, weighted
answer. Conflating the two would corrupt the exact distinction Differentiate 'Not sure' from 'Skip': record a human abstention (issue #712) #731 was built to preserve.
CardPrintingTag):cast_illustration_vote's own docstring explains why this is deliberatelynever done - the endpoint has no way to express "one of these N printings" without falsely
asserting an unsupported outcome, corrupting printing consensus. Extending the exclusion query
instead of the write path leaves that constraint untouched.
2.
get_contested_card_ids()deduped across the view boundary.get_next_question_feed_itemand
get_remaining_estimateeach independently called it - measured at 513-627ms per callagainst live production data (single-gunicorn-worker deployment, so this is site-wide latency on
every affected request, not one user's). Following the "compute once, thread as an optional
parameter" convention PR #729 established inside this file, both functions now accept an
optional
contested_card_idsparameter (Nonedefault - every existing direct caller, includingcatalog_stats.py, is unaffected).views.get_question_feedresolves it once and passes it toboth. No cross-request cache introduced.
Test plan
cd MPCAutofill && pytest cardpicker/tests/test_question_feed.py cardpicker/tests/test_illustration_vote.py cardpicker/tests/test_md5_group_pooling.py-150 passed (includes two new regression tests under
TestIllustrationVoteAnsweredExclusionthat fail against pre-fix code: one asserts
_voter_answered_printing_card_idsincludes thecard after an N>1 illustration vote, the other asserts a voter is not re-served the card via
get_next_question_feed_itemafter casting one)cd MPCAutofill && pytest cardpicker/tests/(excluding the pre-existing, unrelatedTestSharedCacheNotConfigured/TestGetCatalogStatsViewfailures intest_catalog_stats.py,confirmed identical on unmodified
origin/master, and the container-onlytest_federation_hash_tool_parity.py) - 3676 passed, 8 skipped, 0 failedpre-commit run(ruff, isort, black, mypy, prettier) on all changed files - clean