Add nmslib indexer #2417

masa3141 · 2019-03-15T08:52:38Z

Hi, I added nmslib indexer.

Some research shows nmslib is better than annoy indexer.
https://erikbern.com/2018/06/17/new-approximate-nearest-neighbor-benchmarks.html
https://www.benfrederickson.com/approximate-nearest-neighbours-for-recommender-systems/

This is the first time to contribute to gensim. If I miss something, please let me know.

piskvorky · 2019-03-15T09:50:39Z

Thanks @masa3141 !

Ping @searchivarius – would you have capacity to review this PR? (choice of NMSLIB APIs, default parameters, etc). Cheers.

searchivarius · 2019-03-16T02:22:04Z

@piskvorky yes, will do soon. thanks letting me know!

masa3141 · 2019-03-25T12:45:22Z

Ping @searchivarius
Hi could you review the code? Thanks!

searchivarius · 2019-03-25T17:26:57Z

Please, bear with me. There was recently reported what seems to be a serious bug. I need to fix it first.

masa3141 · 2019-03-26T06:04:29Z

I see. Thank you!

masa3141 · 2019-04-26T07:19:24Z

@searchivarius
Sorry to bother you again.
Could you review the code?
Thanks!

searchivarius · 2019-04-26T09:57:23Z

@masa3141 sorry for the delay, I still haven't resolved the issue with the recently reported NMSLIB bug. In fact, knnQuery might not work, but I don't know yet why, knnQueryBatch seems to be working fine.
BTW, do I get it correctly that you use only the cosine distance?

masa3141 · 2019-04-26T15:56:32Z

Thank you for your support in your busy time. Please review at your convenience.
Yes, I only use cosine distance because annoy indexer in gensim only supports cosine distance.

searchivarius · 2019-05-16T04:19:23Z

Ok, I have started working on this. Should be ready soon!

searchivarius · 2019-05-17T01:56:54Z

Hi @masa3141 there seems to be, indeed, an issue with knnQuery. Could you slightly change your code so it uses knnQueryBatch instead? I am going to fix knnQuery very soon, but it will take time and the older NMSLIB versions will still be broken.

Please, make query matrix 2-dim numpy, example is here:
https://github.com/nmslib/nmslib/blob/master/python_bindings/notebooks/search_vector_dense_optim.ipynb

Thank you!

masa3141 · 2019-05-17T12:06:55Z

Hi @searchivarius, thank you for the review!
I changed the code to use knnQueryBatch instead of knnQuery.
Thanks!

masa3141 · 2019-05-17T13:11:25Z

Did the test fail due to Memory Error?
Oh, it seems to fail to install nmslib.

searchivarius · 2019-05-17T16:07:23Z

@masa3141 no it's related to the using wrong memory layout for numpy array data. I am quite surprised it wasn't caught before.

masa3141 · 2019-05-18T13:05:59Z

How do I fix it? Does it happen only in python2.7?

searchivarius · 2019-05-19T02:57:07Z

@masa3141 ohhh I missed that you changed to knnQueryBatch already. I will review shortly.

searchivarius · 2019-05-21T19:30:48Z

Hi @masa3141 and @piskvorky I have reviewed the querying part. It does look fine to me, many thanks! It would be nice to double check how all is working though when things are merged.

masa3141 · 2019-05-22T09:58:18Z

@searchivarius Thank you for the review!
@piskvorky It seems to fail to install nmslib in ci container in Python2.7. How do I fix this?

piskvorky · 2019-05-22T09:59:03Z

No idea. CC @mpenkov .

masa3141 · 2019-05-22T12:42:35Z

I changed to install nmslib into CI only when python version is over 3.0.
It seems to work.
(The test on python2.7 windows is failing. This failure happens in other pull requests.)

masa3141 · 2019-05-24T07:21:28Z

@piskvorky @mpenkov
Hi, do I need to do anything?
Thanks.

mpenkov · 2019-05-30T03:10:14Z

@masa3141 Not in the immediate future. Let us work out why Appveyor is failing. I'll get back to you once that's done.

masa3141 · 2019-06-06T22:30:43Z

@mpenkov Thanks for fixing Appveyor issue! When will this feature be merged?

mpenkov

Thank you for your patience. I finally got around to reviewing this. Please have a look at the comments.

gensim/similarities/nmslib.py

masa3141 · 2019-06-29T06:50:16Z

@mpenkov
py27-win test fails. Do you know the reason and solution?
Thanks.

mpenkov · 2019-06-29T07:25:37Z

Hmm, looks like a dependency problem:

ERROR:   py27-win: could not install deps [numpy==1.11.3, scipy==0.18.1, .[test-win]]; v = InvocationError(u"'C:\\projects\\gensim-431bq\\.tox\\py27-win\\Scripts\\pip.EXE' install --timeout=60 --trusted-host 28daf2247a33ed269873-7b1aad3fab3cc330e1fd9d109892382a.r6.cf2.rackcdn.com --find-links http://28daf2247a33ed269873-7b1aad3fab3cc330e1fd9d109892382a.r6.cf2.rackcdn.com/ numpy==1.11.3 scipy==0.18.1 '.[test-win]'", 2)

masa3141 · 2019-06-29T08:31:03Z

Interesting. It didn't happen before. From today this happens.
I have no idea to fix this. Could you look at this problem? I think this will happen in other pull requests.

mpenkov · 2019-06-29T12:48:27Z

OK, leave it with me. Once I fix this, I will ping you.

mpenkov

Picked up one more thing. While I'm messing with appveyor, please use underscores instead of camel cases in test method names.

I fixed a couple myself, leaving the rest up to you.

gensim/test/test_similarities.py

masa3141 · 2019-06-30T01:22:12Z

I found there are many camel cases in test method names. (Ex, test_nmf.py, test_sklearn_api.py)
Should we change to use underscores in test_similarities.py?

mpenkov · 2019-06-30T05:10:23Z

Yes, we should, but those files are out of scope for this PR.

Let's handle the nmslib test cases only for now, and leave the rest for another PR.

Upgrade pip

I'll need to think of another way to do this. This reverts commit 32e6e51. This reverts commit e89b8e2. This reverts commit 85b8d28. This reverts commit a386f67. This reverts commit fbec445.

masa3141 · 2019-06-30T12:33:12Z

I see. I changed to use underscores.

mpenkov · 2019-06-30T15:33:11Z

Opened a new ticket with support: https://help.appveyor.com/discussions/problems/24208-unable-to-use-the-latest-version-of-pip-for-my-tox-builds

masa3141 · 2019-07-06T10:44:35Z

@mpenkov
Is there anything I should do?

mpenkov · 2019-07-07T09:03:21Z

@masa3141 Finally merged. Thank you for your contribution and your patience!

masa3141 · 2019-07-07T13:12:30Z

Thanks a lot!

searchivarius · 2019-07-07T13:38:01Z

Hurray, thanks everybody!

masa3141 · 2019-07-08T12:55:54Z

@mpenkov Hi, 3.8.0 release doesn't contain this nmslib feature? The release note doesn't mention this feature. Thanks

mpenkov · 2019-07-08T13:01:51Z

@masa3141 Thank you for pointing it out. Looks like our changelog generation script missed the nmslib PR. The indexer itself is definitely there: https://github.com/RaRe-Technologies/gensim/blob/develop/gensim/similarities/nmslib.py

I will update the change log.

Add nmslib indexer

fcca8f8

mpenkov added the feature Issue described a new feature label Apr 28, 2019

use knnQueryBatch instead of knnQuery

d8f8f85

install nmslib into CI only when python version is over 3.0

acff166

mpenkov self-assigned this May 30, 2019

Merge remote-tracking branch 'upstream/develop' into feature/nmslib

69a7057

mpenkov requested changes Jun 21, 2019

View reviewed changes

gensim/similarities/nmslib.py Outdated Show resolved Hide resolved

gensim/similarities/nmslib.py Outdated Show resolved Hide resolved

gensim/similarities/nmslib.py Outdated Show resolved Hide resolved

use smart_open's open

f8ea652

mpenkov reviewed Jun 29, 2019

View reviewed changes

gensim/similarities/nmslib.py Outdated Show resolved Hide resolved

improve docstring and initializer

9b4417a

mpenkov added 2 commits June 29, 2019 22:44

clarify comment about implementation detail in nmslib.py

47dd709

fix white space

ec7df96

mpenkov requested changes Jun 29, 2019

View reviewed changes

gensim/test/test_similarities.py Outdated Show resolved Hide resolved

gensim/test/test_similarities.py Outdated Show resolved Hide resolved

gensim/test/test_similarities.py Outdated Show resolved Hide resolved

gensim/test/test_similarities.py Outdated Show resolved Hide resolved

mpenkov and others added 8 commits June 30, 2019 15:12

Update install.ps1

7b21538

Upgrade pip

Update install.ps1

fbec445

Update install.ps1

a386f67

Create pip.sh

85b8d28

Update tox.ini

e89b8e2

Update pip.sh

32e6e51

Revert appveyor-related commits.

2227fcf

I'll need to think of another way to do this. This reverts commit 32e6e51. This reverts commit e89b8e2. This reverts commit 85b8d28. This reverts commit a386f67. This reverts commit fbec445.

change to use underscores instead of camel case

c5bc0df

mpenkov added 2 commits July 3, 2019 15:36

Merge remote-tracking branch 'upstream/develop' into feature/nmslib

09abb6d

clean up tox.ini

a7f26a9

mpenkov merged commit c9ec242 into piskvorky:develop Jul 7, 2019

Add nmslib indexer #2417

Add nmslib indexer #2417

Conversation

masa3141 commented Mar 15, 2019

piskvorky commented Mar 15, 2019 • edited Loading

searchivarius commented Mar 16, 2019

masa3141 commented Mar 25, 2019

searchivarius commented Mar 25, 2019

masa3141 commented Mar 26, 2019

masa3141 commented Apr 26, 2019

searchivarius commented Apr 26, 2019

masa3141 commented Apr 26, 2019

searchivarius commented May 16, 2019

searchivarius commented May 17, 2019

masa3141 commented May 17, 2019

masa3141 commented May 17, 2019 • edited Loading

searchivarius commented May 17, 2019

masa3141 commented May 18, 2019

searchivarius commented May 19, 2019

searchivarius commented May 21, 2019

masa3141 commented May 22, 2019

piskvorky commented May 22, 2019

masa3141 commented May 22, 2019

masa3141 commented May 24, 2019

mpenkov commented May 30, 2019

masa3141 commented Jun 6, 2019

mpenkov left a comment

Choose a reason for hiding this comment

masa3141 commented Jun 29, 2019

mpenkov commented Jun 29, 2019

masa3141 commented Jun 29, 2019

mpenkov commented Jun 29, 2019

mpenkov left a comment

Choose a reason for hiding this comment

masa3141 commented Jun 30, 2019

mpenkov commented Jun 30, 2019

masa3141 commented Jun 30, 2019

mpenkov commented Jun 30, 2019

masa3141 commented Jul 6, 2019

mpenkov commented Jul 7, 2019

masa3141 commented Jul 7, 2019

searchivarius commented Jul 7, 2019

masa3141 commented Jul 8, 2019

mpenkov commented Jul 8, 2019

piskvorky commented Mar 15, 2019 •

edited

Loading

masa3141 commented May 17, 2019 •

edited

Loading