Feature/dedup merger#42
Closed
Great-Frosty wants to merge 37 commits into
Closed
Conversation
MergerViewSession with deduplicate=true relied on _dedup_data, which keeps the first copy of a shared id by POSITION, ignoring dedup_priority. When the inner MergerPercentage interleaves a low-percentage high-priority source (recommended, prio 2) with a high-percentage low-priority source (regular, prio 1), the faster regular stream reaches shared ids first, so the recommended copy was dropped. Recommended collapsed far below its configured percentage (35% -> ~21% in prod, near 0 with heavy id overlap). Root cause: when a view_session runs as a positional owner, its ambient ctx.dedup is None, so the inner percentage merger ran without the priority-aware arbitration/refill that already exists in the engine. Fix: when the session owns deduplication (deduplicate=true + dedup_key) and there is no ambient dedup policy, build a request-local priority-aware DeduplicationPolicy + refill settings and pass them inward. This activates the existing arbitrate_owner_buffers (keeps higher-priority copy) and _refill_deficits (tops sources back to quota) machinery. No extra backend calls: dedup runs in-memory over already-fetched data (verified). Adds a regression test reproducing the collapse and asserting recommended keeps its share with overlapping ids. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fig) The regression test imported fakeredis, which is not a smartfeed dependency, so CI failed at collection (ModuleNotFoundError). Rewrite it to use the existing redis_client fixture (sync + async) like the other view_session tests. Also rebuild the test config to mirror the real feed shape (dedup -> positional -> view_session -> percentage). The previous standalone view_session->percentage config did not actually trigger the collapse, so the test passed even without the fix. The prod-shaped config reproduces it: recommended share is ~13% without the fix and ~34% with it.
The Test workflow path filter omitted the tests/ directory, so changes that touch only test files never ran CI. Add tests/** so test-only commits are validated too.
MergerPercentage interleaved sources proportionally to their ACTUAL lengths, which smeared a scarce source thinly across the whole feed (e.g. a 35% source with few items showed up at ~15% on every page instead of 35%). Replace the proportional block merge with a smooth weighted round-robin keyed on the configured percentages: each step emits the source most behind its target share that still has items. This front-loads the target ratio -- a scarce source keeps its target percentage on the early pages and then drops to zero once exhausted, instead of being diluted everywhere. Updates the existing percentage order assertion to the new (front-loaded) output and adds a front-load regression test.
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
2 participants
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.
No description provided.