-
Notifications
You must be signed in to change notification settings - Fork 50
GPU support for ImplicitRanker #201
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #201 +/- ##
===========================================
Coverage 100.00% 100.00%
===========================================
Files 45 57 +12
Lines 2242 3787 +1545
===========================================
+ Hits 2242 3787 +1545 ☔ View full report in Codecov by Sentry. |
|
Force-pushed with linter fix. |
blondered
left a comment
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.
Thank you for the PR! I'm suggesting some changes to the test inputs so that we don't have to handle different results from cpu and gpu ranking.
8b99388 to
71b029d
Compare
62ce57c to
23e7da4
Compare
Implement use_gpu argument to ImplicitRanker.rank method. Adapt tests to it.
blondered
left a comment
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.
Thank you for contributing! We are preparing for the upcoming release now.
You can enable use_gpu for iALS in the next PR if you like. The logic for this model is very simple: use_gpu should be set to True if wrapped model is an instance of GPUAlternatingLeastSquares.
For other models we need a bit of thinking. I'm not sure if we are going to use gpu by default. The alternative is to make it an option for user to decide. Anyway, it's gonna take some time to decide and implement (and fix tests).
|
Nice! Thank you for RecTools! |
Type of change
Description
Implement use_gpu argument to ImplicitRanker.rank method. Adapt tests to it.
This PR closes #200 and #199 .
The GPU support works well but it does not pass all the test normally. The reason is behind implicit's implementation of topk on GPU and is not a bug: if predicted scores are equal, the order of elements is undefined. On CPU it is sorted in an ascending order but on GPU the order seems to be reverse. This limitation fails a lot of tests. Im not sure how to handle this because here are too much test cases to be rewritten without this thing.
Also I should mention changes in _get_neginf_score. On GPU the filtration score is slightly different than on CPU: https://github.com/benfred/implicit/blob/main/implicit/gpu/knn.cu#L36
I changed the implementation and its tests.
So, questions like interface for user, GPU by default, tests for ranker on GPU keep open and feedback from mainterers will be fine.
PS.
For testers: make sure implicit has GPU support on specific environment(
python -c "from implicit.gpu import HAS_CUDA; print(HAS_CUDA)"). It is really unstable thing and I was able to run it normally only in the custom environment. On one created withmake installit was falling back to disabled GPU.