-
Notifications
You must be signed in to change notification settings - Fork 5
Add recommend endpoint #158
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: main
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.
This is the right general idea, and a great start. However, the recommend
function from make_recommendation
doesn't properly make recommendations for an arbitrary selection of user provided movies. You'll notice that it doesn't take any arguments. How does it know which movies the user already likes? It doesn't!
That whole module was just intended, I believe, to exercise the LightFM system with random movies. It picks a random user that is already in the dataset and withholds some of the movies when training, to see if we "reocmmend" them properly.
I think this PR could work as a type of scaffolding, so that once we have an actual recommend function (that takes in movies!) we could drop that in.
However, we still have the problem that when @JamesKohlsRepo and I discussed this, we anticipated that it would deal with movie ids only, not titles. So you would pass a list of ids to the API, and get back a list of IDs It would then be the responsibility of the frontend to use another method (that hasn't been written yet) to retrieve movie title and poster based on one or more ids.
Still this is good work!
Thx! Yeah that makes sense, I noticed that the recommendations weren't actually real, but wasn't sure if there was another function somewhere or something. I assumed this would be something of a placeholder until the rest was implemented (assuming it didn't exist somewhere that I didn't see). In the meantime, maybe I (and @JamesKohlsRepo ) can at least clean up the styling then for when we get the real data? And perhaps implement the other function to take in the appropriate data types, etc, even if it doesn't work yet? Is anyone working on the recommend function itself, or should we focus on that instead? |
As far as the styling goes, can't you just reuse |
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.
Great to see your commit! I tired to keep the style similar to how you initially designed it. It would be great to have your help to clean up the styling and less then optimal design choices I've made.
For the Ruff error, I think you can fix line 26 by removing the double quotations:
return f"{movie.get('netflix_id')}"
The ruff error is fixed in #159. However, the mystery was why it linted cleanly on the command line, but not in CI. I couldn't solve that mystery so I just updated the code as you suggested and bumped the python version to 3.12. |
try: | ||
rec_ids = recommend() | ||
rec_titles = [get_title(int(mid)) for mid in rec_ids] | ||
except Exception as e: |
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.
It's considered bad practice to except Exception
, because if there are logical errors in your code (NameError
, IndexError
, etc), they will also get caught here. Prefer to catch the precise exception you're worried about.
Updated the PR to use movie IDs instead of titles and use the MovieList component, let me know what yall think |
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.
Resolved some comments and added another one.
return jsonify({"error": "Movie not found"}), 404 | ||
return jsonify(dict(movie._mapping)), 200 | ||
|
||
@app.route("/api/v1/movie/recommend", methods=["GET"]) # type: ignore |
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.
nit: methods=["GET"]
is not required, that's the default.
movies_list = [row._asdict() for row in movies] | ||
return jsonify(movies_list), 200 | ||
|
||
@app.route("/api/v1/movie/<movie_id>") |
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.
nit: Technically for REST, this would be /api/v1/movies/<movie_id>
I'm not certain, but I think so-called "smart quotes" are causing a merge conflict. Note the four bold e2 80 9x sequences in the attached PDF. We should prefer US-ASCII in our source code. The lockfile conflicts are pretty vanilla. Just "accept theirs", or regenerate with |
Add recommend button
Adds recommendation endpoint to the backend & calls it when clicking the "Get Recommendations button" on the frontend. Functionality only, shows a list of the titles of the retrieved recommendations (no styling yet). Will refine further, just putting this up for review to ensure it's on the right track & using the correct recommendation function.
Screenshots