Merge lp://staging/~canonical-ca-hackers/ubuntu-recommender/933881-expire-app-recommendation into lp://staging/ubuntu-recommender

Proposed by Łukasz Czyżykowski
Status: Merged
Approved by: Łukasz Czyżykowski
Approved revision: 62
Merged at revision: 48
Proposed branch: lp://staging/~canonical-ca-hackers/ubuntu-recommender/933881-expire-app-recommendation
Merge into: lp://staging/ubuntu-recommender
Diff against target: 0 lines
To merge this branch: bzr merge lp://staging/~canonical-ca-hackers/ubuntu-recommender/933881-expire-app-recommendation
Reviewer Review Type Date Requested Status
Michael Nelson (community) Approve
Review via email: mp+95523@code.staging.launchpad.net

Commit message

Add recommendation_expire management command.

Description of the change

Overview
========
This branch adds new management command recommendation_expire which marks app recommendations without feedback as expired. Such app recommendations are ignored when another app recommendation is requested, so it could be generated from the latest slope one data.

UserRecommendations are alwasy created from the latest data, so there's no point of doing the same for them.

To post a comment you must log in.
Revision history for this message
Michael Nelson (michael.nelson) wrote :
Download full text (3.4 KiB)

Thanks for the overview of the app too!

10:03 < lukasz> noodles, wanna do a review?
10:07 < noodles> lukasz: sure, grabbing from email.
10:07 < noodles> lukasz: we can swap again? :)
10:07 < lukasz> noodles, sure
10:08 < noodles> lukasz: https://code.launchpad.net/~michael.nelson/software-center-a
gent/942664-part4-dry/+merge/95173
10:17 < noodles> lukasz: with your branch, my main question is why you're not adding
a date_created rather than an expired attribute?
10:18 < lukasz> noodles, how date_created would help with that?
10:20 < noodles> lukasz: the bug says that AppRecommendations should be expired after
 a certain time (if they have no feedback), whereas with the bool expired, you can't
(and don't) do that in your branch?
10:20 < noodles> Or is there already a date_created... I haven't checked.
10:20 < lukasz> noodles, I do, I mark them as expired which exclude them from being s
erved to users
10:21 < lukasz> noodles, no, there's no date_created (I'm going to add it in my next
branch)
10:21 < noodles> lukasz: you mark them as expired immediately - there's not time invo
lved is there? (As in, the app recommendation the command expires could have been cre
ated 1ms before)
10:21 < lukasz> noodles, for the bug which wants expired recommendations to be delete
d after a month
10:21 < lukasz> noodles, yes, idea is to run it daily
10:22 < noodles> lukasz: Oh, I was just reading the bug linked to your branch... and
assumed it mean that they should be deleted if they are one or two days old, rather t
han they should all be deleted every few days...
10:22 * noodles re-reads
10:23 < lukasz> noodles, last sentence in the description indicates that just deletin
g them is not a good idea
10:24 < lukasz> noodles, ironically my first stab at this bug had AppRecommendation.o
bjects(feedback__isnull=True).delete() :)
10:24 < noodles> lukasz: yes..., let me re-type the last part of my sentance: "rather
 than they should all be deleted every few days (the ones without feedback, that is)"
10:24 < noodles> heh :)
10:24 < noodles> lukasz: so I'm just thinking, do you even need an 'expired' attribute, if you add a date_created?
10:25 < lukasz> noodles, how would you decide that AppRecommendation which already exists should be ignored?
10:26 < noodles> lukasz: because you'd filter out apprecommendations older than X_DAYS (or optionally, only if they didn't have feedback)
10:28 < noodles> lukasz: another thought (sorry, I don't know the architecture well enough for recommender)... if a new recommendation comes in, and the only existing one is empty... why wouldn't you just re-use/update the existing one?
10:29 < lukasz> noodles, it's quicker to create new one than to update existing one
10:30 < noodles> lukasz: ok, but why not then *delete* the existing empty one when creating a new one, or is it useful data?
10:31 < lukasz> noodles, by new recommendation you mean new data which would change result of recommend_for_package?
10:32 < lukasz> noodles, if so, there's no link between AppRecommendation and any data which was used to create it. right now all of that data is processed offline
10:39 < noodles> lukasz: can we mumble in 10mins or so? I don't know the...

Read more...

57. By Łukasz Czyżykowski

Removed unnecessary migration.

58. By Łukasz Czyżykowski

Removed unnecessary management command.

59. By Łukasz Czyżykowski

Added migration which adds date_created field to *Recommendation models.

60. By Łukasz Czyżykowski

Modified code to take into account date_created instead of expired field.

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Looking great. As per IRC, up to you if you want to make the number of days before it's expired configurable.

review: Approve
61. By Łukasz Czyżykowski

Switched datetime.now() into datetime.utcnow()

62. By Łukasz Czyżykowski

Added APPRECOMMENDATION_LIFETIME setting.

Preview Diff

Empty

Subscribers

People subscribed via source and target branches