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

bug: TypeError when converting a gpu model to cpu then to gpu #597

Closed
lfaucon opened this issue Jul 29, 2022 · 1 comment · Fixed by #598
Closed

bug: TypeError when converting a gpu model to cpu then to gpu #597

lfaucon opened this issue Jul 29, 2022 · 1 comment · Fixed by #598

Comments

@lfaucon
Copy link

lfaucon commented Jul 29, 2022

Steps to replicate

  1. Train a model implicit.gpu.als.AlternateLeastSquares
  2. call to_cpu then to_gpu on it
  3. calling recalculate_user now causes the error TypeError: __cinit__() takes exactly 1 positional argument (2 given)

Full example:

> als_model
<implicit.gpu.als.AlternatingLeastSquares object at 0x7fdcbc153460>
> als_model.recalculate_user(list(range(50)), u_i_matrix)
Matrix([[0. 0. 0. ... 0. 0. 0.]
 [0. 0. 0. ... 0. 0. 0.]
 [0. 0. 0. ... 0. 0. 0.]
 ...
 [0. 0. 0. ... 0. 0. 0.]
 [0. 0. 0. ... 0. 0. 0.]
 [0. 0. 0. ... 0. 0. 0.]])
> als_model = als_model.to_cpu().to_gpu()
> als_model.recalculate_user(list(range(50)), u_i_matrix)
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/storage/lofauco/recommender-systems/venv-recsys/lib64/python3.8/site-packages/implicit/gpu/als.py", line 176, in recalculate_user
    Cui, user_factors, self.YtY, self.item_factors, cg_steps=self.factors
  File "/storage/lofauco/recommender-systems/venv-recsys/lib64/python3.8/site-packages/implicit/gpu/als.py", line 261, in YtY
    self._YtY = implicit.gpu.Matrix(self.factors, self.factors)
  File "_cuda.pyx", line 87, in implicit.gpu._cuda.Matrix.__cinit__
TypeError: __cinit__() takes exactly 1 positional argument (2 given)

Note: The double conversion is not a common usecase. I mention it here only as the simplest way to replicate the error. The error occurs also when saving and loading the model.

Proposed fix

The class implicit.gpu.Matrix defined here
https://github.com/benfred/implicit/blob/main/implicit/gpu/_cuda.pyx#L87
is used here incorrectly
https://github.com/benfred/implicit/blob/main/implicit/gpu/als.py#L271

XtX and YtY should be initialized as self._XtX = implicit.gpu.Matrix.zeros(self.factors, self.factors)

benfred added a commit that referenced this issue Jul 30, 2022
The recalculate_user or recalculate_item functionality didn't work on the GPU
AlternatingLeastSquares model, if the model was created from a saved version
of converted from a CPU model. Fix and add a unittest that would have caught this

Fixes #597
benfred added a commit that referenced this issue Jul 30, 2022
The recalculate_user or recalculate_item functionality didn't work on the GPU
AlternatingLeastSquares model, if the model was created from a saved version
of converted from a CPU model. Fix and add a unittest that would have caught this

Fixes #597
@benfred
Copy link
Owner

benfred commented Jul 30, 2022

Thanks for the bug report! It looks like the existing test suite misses this - we have tests to_cpu/to_gpu conversion, as well as the save/load code, but there aren't any tests that test out the recalculate_user/item code after model.save or to_cpu . It seems like this bug has existed since I originally added the recalculate_user/item support to the GPU ALS model #515 =(

I have a fix in #598

benfred added a commit that referenced this issue Jul 30, 2022
The recalculate_user or recalculate_item functionality didn't work on the GPU
AlternatingLeastSquares model, if the model was created from a saved version
of converted from a CPU model. Fix and add a unittest that would have caught this

Fixes #597
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 a pull request may close this issue.

2 participants