-
-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: Add suggestions based on watch history #5328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, this is great!
Unfortunately it's also a really costly feature with the way its currently implemented. On each request to the new suggested videos feed Invidious will be making up to 28 requests (get_video
queries two endpoints: /player and /next) to YouTube in even the best case scenario.
For a single person instance this translates to a significant delay in the response time of that page, and a greater chance of being ratelimited by Youtube.
But for a large instance this is completely unbearable. If just a hundred users access the suggested feed at once then that'll mean a concurrent request load of at least 2800 (100 * 14 * 2) requests to either YouTube or the database.
Not only is this bandwidth and computationally expensive it will also lead to an instant block by YouTube and it is already extremely difficult for large instances to workaround these blocks.
.DS_Store
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the .DS_Store
MacOS file. You can edit .git/info/exclude
or set a global gitignore via git config --global core.excludesFile '[LOCATION]'
to exclude it in future commits.
suggested_videos = fetch_suggested_video_ids(env, region, locale) | ||
if suggested_videos.size > 0 | ||
return suggested_videos | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a new feed rather than overriding the /trending
feed... a user shouldn't just randomly lose access to a YouTube feature just because they made an account.
user = env.get("user").as(User) | ||
if user.preferences.watch_history | ||
Invidious::Database::Users.mark_watched(user, id) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't include unrelated changes in a PR.
I personally don't think the API should have any bearing on user data outside of the authenticated endpoints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I solely use the API to watch videos using a third party Apple TV application (Yattee). Videos that are served through their platform did not trigger any views, which is why I have added the if user, track view part.
If you say there are separate authenticated API endpoints, I should probably check out the Yattee source code first.
@@ -40,3 +44,82 @@ def fetch_trending(trending_type, region, locale) | |||
# Deduplicate items before returning results | |||
return extracted.select(SearchVideo | ProblematicTimelineItem).uniq!(&.id), plid | |||
end | |||
|
|||
def fetch_suggested_video_ids(env, region, locale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately there needs to be a way to disable this feature entirely on the instance level
@@ -40,3 +44,82 @@ def fetch_trending(trending_type, region, locale) | |||
# Deduplicate items before returning results | |||
return extracted.select(SearchVideo | ProblematicTimelineItem).uniq!(&.id), plid | |||
end | |||
|
|||
def fetch_suggested_video_ids(env, region, locale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about converting this into a background job?
A background job will allow the feeds to be created over longer periods of times and as such minimize the number of requests to YouTube.
But this will mean needing new tables within the database and accordingly also an automatic migration solution which we currently do not have...
Hi @syeopite, really appreciate the time and effort you put into this in-depth review. It's clear you've given it serious thought, and I've learned quite a bit just from reading through your comments, thanks. I fully agree with your observations, especially around the fetching logic which is due for a proper rethink. A background job, thread, or scheduled task would definitely be better. The question then, of course, is when to trigger that fetch, and I agree that this needs to be carefully considered and probably discussed further to avoid hitting limits. That said, I have to admit my experience with Crystal is pretty much limited to this PR. I’d love to help more, but I’m probably not the best person to implement these changes. Really appreciate you pointing things out so clearly! |
This PR adds suggestions based on the users subscriptions and watch history.
The Trending → Default page is overwritten with data based on the users watch history when:
The 'algorithm' works as follows:
For all those (14) video's, related videos are fetched. Those related videos are then shuffled and shown.
As you can see, I like airplanes and magic tricks. Hope this helps.