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.

Sign up for GitHub

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

Jump to bottom

IVF-PQ Python wrappers #970

Merged

rapids-bot merged 38 commits into rapidsai:branch-22.12 from tfeher:fea_ivf_pq_python_wrapper

Nov 15, 2022

Contributor

tfeher commented Nov 1, 2022 •

edited

Loading

This PR adds python wrappers to IVF-PQ.

github-actions bot added CMake cpp python labels

cjnolet assigned tfeher

tfeher force-pushed the fea_ivf_pq_python_wrapper branch from cf40569 to acdc3c4 Compare

November 3, 2022 01:38

tfeher marked this pull request as ready for review

November 3, 2022 01:46

tfeher requested review from a team as code owners

November 3, 2022 01:46

Contributor Author

tfeher commented Nov 3, 2022 •

edited

Loading

There are a few items that still need to be addressed, but otherwise it is ready for the first review @cjnolet

handle codebook_kind parameter
handle lut_dtype and internal_distance_dtype search params
Add more test parameters combinations
Add xfail tests

Contributor

achirkin commented Nov 7, 2022 •

edited

Loading

What would you think about re-exporting default values of C++ params structs? Like we did a while ago in cuml/linearSVM (rapidsai/cuml#4381).
This would require a bit of extra work to make it nice, e.g. partially port the WithReexportedParams metaclass definition to allow inserting default values into python docstrings. But I think it still makes sense in the long run. I suspect we may want to change the default parameter values in future (at least those that affect performance, but don't affect the data layout/search results).

tfeher added enhancement non-breaking labels

Contributor Author

tfeher commented Nov 7, 2022

@achirkin Thanks for the suggestion! yes it occurred to me to return to this idea. I would dedicate a follow up PR for this task.

Contributor Author

tfeher commented Nov 7, 2022

There is one issue pending: forwarding the C++ exceptions from build and search to Python. Using the standard method leads to the following error message:

pylibraft/neighbors/ivf_pq.cxx:7155:45: error: no matching function for call to 'raft::neighbors::ivf_pq::index<long unsigned int>::index()'
 7155 |   raft::neighbors::ivf_pq::index<uint64_t>  __pyx_t_20;

This happens because Cython tries define a variable for the return value, and the return value type (index) does not have a default constructor. I will look into a workaround.

cjnolet requested changes

View reviewed changes

Member

cjnolet left a comment

Thanks for adding this feature to pylibraft! I think the feature mostly looks ready to go, but I'd like to keep pylibraft's APIs consistent and make sure they all directly correspond to the c++ APIs they are wrapping.

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

Member

cjnolet commented Nov 8, 2022

Closes #998

cjnolet added improvement and removed enhancement labels

cjnolet requested changes

View reviewed changes

Member

cjnolet left a comment

This looks really good overall. I'm excited to have this in the python API!

cpp/CMakeLists.txt Outdated Show resolved Hide resolved

cpp/CMakeLists.txt Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

cjnolet and others added 10 commits

November 10, 2022 17:12


          renaming to device_ndarray

753a058


          Using pre-commit

2bd251a


          Making pairwise distance output optional

ac561ec


          Updating docs for device_ndarray

f5c4a00


          Updating style

3d0114b


          Add Python wrapper for IVF-PQ

5e9162d


          Added test, works so far<

571fb59


          Move build params to __init__

00da77a


          Extend method wrapped

bc4c66c


          Remove preferred_thread_block_size search param, since it will be dep…

52feeac

…recated in rapidsai#926


          Handle memory resource arg, add examples

1b5ff59

tfeher force-pushed the fea_ivf_pq_python_wrapper branch from 2d6da94 to 1b5ff59 Compare

November 11, 2022 15:52

tfeher commented

View reviewed changes

Contributor Author

tfeher left a comment

Thanks @cjnolet for the review! I have addressed the issues. There is a problem with the extend test, I am looking into it.

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

tfeher added 2 commits

November 11, 2022 22:10


          Fix extend specialization

d4a11c2


          Define string representation for the index

f76b438

Contributor Author

tfeher commented Nov 11, 2022

Fixed the problem with extend.

cjnolet reviewed

View reviewed changes

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

cjnolet requested changes

View reviewed changes

Member

cjnolet left a comment

Looking great! I definitely think this is going to make it into 22.12, just really one little thing- hiding the cuda data types behind np.dtype for API consistency

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

achirkin reviewed

View reviewed changes

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

cjnolet and others added 17 commits

November 14, 2022 13:06


          Adding device_ndarray.empty() staticmethod

a6c9d35


          Using pylibraft.device_ndarray in pairwise_distances

340300f


          Removing acciddentally checked-in file

9d8d2b9


          Update python/pylibraft/pylibraft/common/device_ndarray.py

45e5212

Co-authored-by: Ben Frederickson <github@benfrederickson.com>


          Updating usage examples in readme

3f8dcc4


          Adding example of converting device_ndarray to torch tensor

c4f9c7f


          Adding to pairwise distance pydocs

ec3cc6e


          Making in-place output optional for fused_l2_nn_argmin

f3d59f3


          Adding link to cuda array interface on README

ec93403


          Merge branch 'fea-2212-device_buffer' into fea-ivf-pq-python-wrapper-…

8014eb5

…device_ndarray


          Tests are passing. Committing.

c8fba7a


          Fixing style

09485bc


          Merge branch 'branch-22.12' into fea_ivf_pq_python_wrapper

2f40799


          Fixing typo for codespell

f0b3fee


          Use np dtypes, add string repr. for SearchParams

0fd422e


          Use cuda_interruptible context

14c3718


          Fix style

d45fd8f

tfeher commented

View reviewed changes

Contributor Author

tfeher left a comment

Thanks for the reviews, I have updated the PR!

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

python/pylibraft/pylibraft/neighbors/ivf_pq.pyx Outdated Show resolved Hide resolved

cjnolet approved these changes

View reviewed changes

Member

cjnolet left a comment

LGTM. Thanks again @tfeher!

Member

cjnolet commented Nov 15, 2022

@gpucibot merge

rapids-bot bot merged commit 355f693 into rapidsai:branch-22.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake cpp improvement non-breaking python