[Brave News]: FeedV2Builder refactor #23492

fallaciousreasoning · 2024-05-08T02:05:42Z

This PR refactors the FeedV2Builder to split out a FeedGenerationInfo struct and keep track of the number of articles available in each content group. This allows us to avoid attempting to generate from a ContentGroup with no available articles (which terminates the feed generation).

Additionally, this logic is reused for generating Channel blocks in the feed.

@LorenzoMinto to you remember what the issue for this was?

TODO

Add test
Rename ArticleWeight to ArticleMeta

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

LorenzoMinto · 2024-05-08T10:37:31Z

This would be help fix brave/brave-browser#36137. We can filter out articles older than X days and confidently sample if a content group runs out of eligible content. Wdyt? I guess we could fix it here or as a follow up

fallaciousreasoning · 2024-05-09T02:52:16Z

Lets do it as a followup 😄 I'd like to try and get this merged

fallaciousreasoning · 2024-05-09T02:55:43Z

@LorenzoMinto mind taking a look?

LorenzoMinto

Great refactor. What do you think about renaming FeedGenerationInfo to FeedGenerator as otherwise it reads more like a data class, but it's doing quite a bit that it's not just representational.

And also, what do you think about moving the generation functions GenerateBlock, GenerateCluster/Special/ChannelBlock etc. to the the generator and passing in the relevant arguments? Right now these are loose functions in feed_v2_builder

components/brave_news/browser/feed_sampling.h

components/brave_news/browser/feed_sampling.cc

components/brave_news/browser/feed_generation_info.cc

simonhong · 2024-05-13T01:48:26Z

components/brave_news/browser/test/BUILD.gn

@@ -16,6 +16,7 @@ source_set("brave_news_unit_tests") {
 "//brave/components/brave_news/browser/direct_feed_controller_unittest.cc",
 "//brave/components/brave_news/browser/direct_feed_fetcher_unittest.cc",
 "//brave/components/brave_news/browser/feed_building_unittest.cc",
+ "//brave/components/brave_news/browser/feed_generation_info_unittest.cc",


nit: How about moving brave_news_unit_tests target to above components/brave_news_browser/BUILD.gn?
I think target and its source files should be in the same folder.

For some reason I thought we were meant to have separate test targets - I have a vague recollection of a Slack thread about it (but my memory is hazy).

Happy to change it because this has always seemed a bit weird to me but maybe I'll get @petemill & @goodov to weigh in.

components/brave_news/browser/feed_generation_info.h

components/brave_news/browser/feed_generation_info.cc

github-actions · 2024-05-19T23:27:56Z

[puLL-Merge] - brave/brave-core@23492

Here is my review of the PR:

Description

This PR adds a new FeedGenerationInfo class that encapsulates all the data needed to generate a personalized news feed. It moves some feed generation logic out of FeedV2Builder into this new class. The main motivation seems to be to simplify the feed generation code by centralizing the data and logic in one place.

Changes

Changes

feed_generation_info.h, feed_generation_info.cc:

Added new FeedGenerationInfo class that stores locale, feed items, publishers, channels, signals, suggested publisher IDs and topics
Provides methods to get article infos, eligible content groups, pick and consume articles based on a picking strategy
Lazily initializes article infos and content groups when first accessed
Reduces publisher/channel counts when articles are consumed to maintain content group eligibility

feed_sampling.h, feed_sampling.cc:

Renamed ArticleWeight struct to ArticleMetadata
Moved some article metadata generation logic from free functions into FeedGenerationInfo
Added PickChannelRoulette function to pick articles from a specific channel

feed_v2_builder.cc:

Refactored feed generation methods to use FeedGenerationInfo instead of separate parameters
Removed some duplicate logic that is now handled in FeedGenerationInfo

feed_generation_info_unittest.cc, feed_sampling_unittest.cc:

Added unit tests for the new FeedGenerationInfo class
Moved some unit tests from feed_sampling_unittest.cc to feed_generation_info_unittest.cc

Overall, the changes look good and well-structured. Encapsulating the feed generation data and logic in the FeedGenerationInfo class seems to simplify and clean up the code nicely. The unit tests provide decent coverage of the new functionality.

Let me know if you have any other questions!

fallaciousreasoning · 2024-05-19T23:29:28Z

@LorenzoMinto & @simonhong thanks for the review

What do you think about renaming FeedGenerationInfo to FeedGenerator as otherwise it reads more like a data class, but it's doing quite a bit that it's not just representational

Yeah, I agree the name isn't quite right, but I think FeedGenerator is equally misleading, as it doesn't actually generate the feed. I might stick with the current name for now.

And also, what do you think about moving the generation functions GenerateBlock, GenerateCluster/Special/ChannelBlock etc. to the the generator and passing in the relevant arguments?

It's on my TODO list, but it's not quite that simple yet, as I'm not 100% sure the best way to set up the generation method from the FeedV2Builder. I'm keen to leave it as is for now until I get the chance to think some more.

Mind both having another look?

simonhong

++ 👍🏼

LorenzoMinto

++

fallaciousreasoning requested a review from LorenzoMinto May 8, 2024 02:05

github-actions bot assigned fallaciousreasoning May 8, 2024

fallaciousreasoning force-pushed the bn-gen-2 branch from 0cf9143 to dbb857e Compare May 8, 2024 03:36

fallaciousreasoning marked this pull request as ready for review May 9, 2024 02:55

fallaciousreasoning requested a review from simonhong May 9, 2024 02:55

LorenzoMinto reviewed May 9, 2024

View reviewed changes

simonhong reviewed May 13, 2024

View reviewed changes

fallaciousreasoning added 11 commits May 20, 2024 10:45

Move FeedGenerationInfo into a separate file

4c095fd

Stash

55b9e5a

Compiling again

338f15e

Better channel picking

bcb89a6

Generate available content groups

10da80d

More logging

b0836c7

Cleanup

ffeec69

Convert suggested_publisher_ids to span

a4ef659

Rename ArticleWeight ==> ArticleMeta

de12ab8

Add test

eb7cce7

Changes for PR

05fe089

fallaciousreasoning force-pushed the bn-gen-2 branch from dbb857e to ca7fb15 Compare May 19, 2024 23:27

github-actions bot added the puLL-Merge label May 19, 2024

fallaciousreasoning force-pushed the bn-gen-2 branch from ca7fb15 to e1d6e07 Compare May 19, 2024 23:35

Move GetArticleWeight into FeedGenerationInfo

897ce7a

fallaciousreasoning force-pushed the bn-gen-2 branch from e1d6e07 to 897ce7a Compare May 20, 2024 00:30

simonhong approved these changes May 20, 2024

View reviewed changes

LorenzoMinto approved these changes May 20, 2024

View reviewed changes

fallaciousreasoning merged commit 7486b0d into master May 20, 2024
19 checks passed

fallaciousreasoning deleted the bn-gen-2 branch May 20, 2024 23:57

github-actions bot added this to the 1.68.x - Nightly milestone May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Brave News]: FeedV2Builder refactor #23492

[Brave News]: FeedV2Builder refactor #23492

fallaciousreasoning commented May 8, 2024 •

edited

LorenzoMinto commented May 8, 2024

fallaciousreasoning commented May 9, 2024 •

edited

fallaciousreasoning commented May 9, 2024

LorenzoMinto left a comment •

edited

simonhong May 13, 2024

fallaciousreasoning May 19, 2024

github-actions bot commented May 19, 2024

Changes

fallaciousreasoning commented May 19, 2024

simonhong left a comment

LorenzoMinto left a comment

[Brave News]: FeedV2Builder refactor #23492

[Brave News]: FeedV2Builder refactor #23492

Conversation

fallaciousreasoning commented May 8, 2024 • edited

TODO

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

LorenzoMinto commented May 8, 2024

fallaciousreasoning commented May 9, 2024 • edited

fallaciousreasoning commented May 9, 2024

LorenzoMinto left a comment • edited

Choose a reason for hiding this comment

simonhong May 13, 2024

Choose a reason for hiding this comment

fallaciousreasoning May 19, 2024

Choose a reason for hiding this comment

github-actions bot commented May 19, 2024

Description

Changes

fallaciousreasoning commented May 19, 2024

simonhong left a comment

Choose a reason for hiding this comment

LorenzoMinto left a comment

Choose a reason for hiding this comment

fallaciousreasoning commented May 8, 2024 •

edited

fallaciousreasoning commented May 9, 2024 •

edited

LorenzoMinto left a comment •

edited