IVF-Flat reconstruction

[viclafargue]

Answers #1205

[achirkin]

NB: ivf-pq reconstruction: #1298

[tfeher]

Thanks @viclafargue for this PR! The details of the reconstruction function needs to updated after #1271 is merged, therefore I did not review that part yet. Below please find a few comments for the rest.

Could you update the PR destription to mention that it also adds the mdspan interface for ivf-pq?

[cjnolet]

@viclafargue is this PR ready for another review yet?

[viclafargue]

@viclafargue is this PR ready for another review yet?

The PR is ready for review.
cc @achirkin @tfeher @cjnolet

[achirkin]

Thanks for the update, here's couple comments on the api.

[achirkin]

In ivf_pq, list and query dimensions are limited to uint32_t rather than IdxT (type of DB indices).

Here in the ivf_flat API you use device_matrix_view<T, IdxT, row_major> rather than device_matrix_view<T, uint32_t, row_major> , but you also set both label and offset arguments to uint32_t. Could you please check here, in the overload above, and down the detail namespace what are the appropriate indexing types and make it consistent?

[viclafargue]

We are using IdxT for the IVF-Flat API. I corrected the code so that it uses the template type.

[achirkin]

Oh, interesting! Out of curiosity, could you add a test case for ivf-flat with 64-bit indexing type and try to call something with an offset slightly outside the 32-bit range? :)

[achirkin]

A quick browsing confirms that 64-bit indexing/extents are actually not supported

raft/cpp/include/raft/neighbors/ivf_flat-inl.cuh

Lines 455 to 464 in cad88b3

	return search(handle,
	params,
	index,
	queries.data_handle(),
	static_cast<std::uint32_t>(queries.extent(0)),
	static_cast<std::uint32_t>(neighbors.extent(1)),
	neighbors.data_handle(),
	distances.data_handle(),
	nullptr);

[viclafargue]

The portion of code you are mentioning is indeed probably a mistake. At the moment the IVF-Flat API functions are templated with IdxT everywhere, and it is set to use uint64_t during compilation. Do you recommend that we update the IVF-Flat API to systematically use uint32_t instead? If so, this is an extensive change, and that would maybe require a separate PR, right?

[achirkin]

If ivf-flat can actually work with extents and input sizes IdxT larger than 32-bit, I don't see any problem using IdxT in the api. But I suspect this is not the case. Hence we should fix it either by limiting extent types to 32-bit or making sure it works with all possible IdxT (not only those we instantiate explicitly btw). I agree this work is worth a separate PR.
However, for the new public functions added in this PR, I think it makes sense to fix the api from the beginning, keeping in mind the future changes in the other parts of the code.

[achirkin]

I thought the consensus was to opt for a "inverted-index" module (hashmap/array/whatever) shared among ivf methods to convert user indices to (label, in-cluster-offset) pairs?
This code could break if the user adds non-contiguous range of large indices to the DB.

[viclafargue]

Thanks for the review! Yes absolutely, this is a temporary solution that would be largely improved by the use of a hashmap in a follow-up PR. But, I thought that letting this version as it is would set the API and allow people to run a reconstruction if their use case allows it (smaller index). But, can still remove it if that's the better path forward. What do you think?

[achirkin]

I was actually thinking of decoupling the ivf-list hashmap struct api from the ivf-pq and ivf-flat methods. Hence a user would need to construct the hashmap explicitly once (costly operation) and then either:
a) search-by-user-ids in two calls (e.g. hashmap::get_lists_offsets + index::get_vectors)
b) pass the hashmap as an argument to the ivf index methods (index::get_vectors)

Not sure what we will decide in the end, but point is the api may change as we progress with the hashmap. Then, maybe we just can keep this function in the detail namespace for now? Hence the work won't be lost and if someone needs the functionality urgently, they can use the detail function, and we don't need to break the public api in the follow-up PR.