Skip to content
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

Feature/video relevance #120

Merged
merged 10 commits into from
Apr 27, 2023
Merged

Conversation

WRadoslaw
Copy link
Contributor

Copy link

@ignazio-bovo ignazio-bovo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a question about the weights used

Copy link

@ignazio-bovo ignazio-bovo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

em
)
await em.query(`
UPDATE "video"
Copy link

@ignazio-bovo ignazio-bovo Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query looks correct, I have double checked it using chatgpt too

Copy link
Contributor

@Lezek123 Lezek123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a brief look at the PR and here I some general remarks I have:

  1. The PR is targetting orion-v2, but this is a stale branch. Since the production release of Orion v2, all the latest work is already on master, so please retarget your PR to master. I think we can even delete orion-v2 at this point to prevent confusion.
  2. I don't think that storing the relevance score and trying to recalculate it every time some action can affect it (even if there's a "delay", like "only recalculate if views_count changes by at least X") is a good approach as opposed to extending queries using custom resolvers. It should be pretty cheap to calculate the relevance score since there are already comments_count, reactions_count and views_num columns inside video table which are kept up to date, so you don't even need to do subqueries/joins with reactions/comments/views tables in order to do that, it would just be a pretty simple formula using columns already available in video. So to clarify what I mean exactly: Instead of storing relevance score as a separate column, you could augment/create a custom query like mostViewedVideosConnection that would support ordering by relevance score and the formula for calculating the relevance score could just be part of the SELECT or ORDER BY statement, ie. SELECT (video.views_num * views_weight + video.comments_count * comments_weight ...) as relevance, what do you think? Do you see any clear downside of this approach taking into account your desired use-case?

@WRadoslaw
Copy link
Contributor Author

How inefficient is it to keep this solution over a custom query that calculates it on the fly? When we first talked about this videoRelevance, it was supposed to be just a temporary solution until the User Accounts arrive, so then we would switch to user-targeted recommendations. Since UA development is around the corner and for now on any gateway there are not that many reactions, maybe we can keep this PR till UA-based recommendations. I hope I didn't overlook smth important here, WDYT?

@Lezek123
Copy link
Contributor

Lezek123 commented Apr 26, 2023

How inefficient is it to keep this solution over a custom query that calculates it on the fly?

I don't think it's very inefficient, my main issue is that it's complex and more prone to errors I think.
But if it's just temporary and you're sure that it works as expected then I guess that's fine.
One suggestion I have then would be to just simplify the update query to rely on already exisiting comments_count / reactions_count counters in order not to duplicate the logic of counting those, I think that should be a pretty simple change.

@attemka attemka changed the base branch from orion-v2 to master April 27, 2023 08:28
@WRadoslaw WRadoslaw requested a review from Lezek123 April 27, 2023 08:28
src/mappings/content/video.ts Outdated Show resolved Hide resolved
src/server-extension/resolvers/AdminResolver/index.ts Outdated Show resolved Hide resolved
@WRadoslaw WRadoslaw requested a review from Lezek123 April 27, 2023 10:23
Copy link
Contributor

@Lezek123 Lezek123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

src/mappings/content/video.ts Outdated Show resolved Hide resolved
src/mappings/content/video.ts Outdated Show resolved Hide resolved
@Lezek123 Lezek123 self-requested a review April 27, 2023 12:33
@Lezek123 Lezek123 merged commit c39505e into Joystream:master Apr 27, 2023
ignazio-bovo pushed a commit to ignazio-bovo/orion that referenced this pull request Jul 3, 2023
* Introduce new video property and scheduler to update it

* Adjust equation and change score to float

* Schedule updates in required places

* Add mutation to change video weights

* Adjust env weights

* Fix for newness weight on creation

* CR fixes

* CR fixes v2

* Update src/mappings/content/video.ts

* Update src/mappings/content/video.ts

---------

Co-authored-by: Leszek Wiesner <leszek@jsgenesis.com>
malchililj pushed a commit to malchililj/orion that referenced this pull request Sep 3, 2024
* Introduce new video property and scheduler to update it

* Adjust equation and change score to float

* Schedule updates in required places

* Add mutation to change video weights

* Adjust env weights

* Fix for newness weight on creation

* CR fixes

* CR fixes v2

* Update src/mappings/content/video.ts

* Update src/mappings/content/video.ts

---------

Co-authored-by: Leszek Wiesner <leszek@jsgenesis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants