[REVIEW] New Dataset API Clarifying Ownership - #1846
Conversation
|
/ok to test 5447a4c |
|
/ok to test 17ab09d |
|
NB: I updated the label to |
@achirkin The problem w/ using mdspan/mdarray for this is that it's not carrying along the proper information to either the algorithms nor the user (which is why we created this specialized class for this in the first place!). Two immediate reasons why this API is necessary:
This new API solves both of these problems while leaving the control over the memory ownership entirely in the user's hands. We've discussed this for a long time. We've known this is needed for a long time. it's time to prioritize this and get it done. I agree that an anstract class might make more sense, but ultimately we should not be moving any owneship over to the algorithm (the user should maintain ownership over the class and underlying memory the entire time). |
|
The doc that outlines some of the API design choices can be found in slack. Let me know if there are any parts of the design that can be altered to better suit our users' needs. The following files are test case files I've added and can be ignored for now. They will be removed before the final merge with upstream repo:
|
|
/ok to test 70b6b58 |
achirkin
left a comment
There was a problem hiding this comment.
Sorry for being so late for the second round of reviews! I have a few suggestions for the dataset type hierarchy.
Need to get this ABI breaking change in for this release (we can't push this to 26.10). It was mentioned that we need to verify benchmarks. I completely agree with this. We will evaluate further in 26.10.
The expectation is that these changes really shouldn't affect performance, and they don't affect the search path at all. We'll demonstrate this in the follow-up changes, though.
… had a type erased index input which supported both host and device input index for serialize_to_hnswlib. HNSW search happens only on host. Host path was accidentally removed when new Dataset API was introduced and type erased index was removed. Added host index path back. This fixes a test failure which originates from C++ and C API layer but manifests only in java.
| /** | ||
| * Internal wiring hook used by the Java wrapper implementation. | ||
| */ | ||
| public final void setDelegate(AutoCloseable delegate, long handleAddress) { |
There was a problem hiding this comment.
Exposing this as a part of external API is a bit scary considering that end user should probably never ever try to change that.
| PaddedDatasetView makePaddedDatasetView(CuVSMatrix dataset) throws Throwable; | ||
|
|
||
| /** Create a caller-owned standard dataset view handle from a matrix. */ | ||
| StandardDatasetView makeStandardDatasetView(CuVSMatrix dataset) throws Throwable; |
There was a problem hiding this comment.
@aamijar @tarang-jain this is stale right? If I remember correctly, we consolidated padded and standard factories and removed views at language wrappers. Can I get confirmation on this?
There was a problem hiding this comment.
@HowardHuang1, the padded and standard factories are still intentionally separate functions in the C API. We still have view functions in the C API. We can reassess whether we need views in the language wrappers (Python, Java, Go, Rust) in a follow up and can add that in the doc.
…originates on host. Merge() can no longer merge host data. Merge() only takes data on device
|
/ok to test 1618150 |
… OOM. However, we should re-evaluate this because I don't think this is good for the new DtasetAPI as the index return type can now be host index when GPU hits OOM but the user still expects device index. With the new templating on the datasets and indexes, this can cause user/caller confusion.
|
/ok to test 7011fe6 |
…earch. TODO: need to revisit this later. This is a temporary patch to fix failing java tests and avoid exposing a public tiered index update dataset function for users. Instead we use CAGRA's existing update dataset functions and keep it tiered index's responsibility to call this internally rather than leaving the responsibility to the users
|
/ok to test c727de2 |
Update C Tests to Use Getters
|
/ok to test e28a173 |
1 similar comment
|
/ok to test e28a173 |
… padded datasets onto host or standard indexes prior to search within Java tests.
|
/ok to test 6eebea0 |
…ndexImpl.java Co-authored-by: Igor Motov <igor@motovs.org>
…ndexImpl.java Co-authored-by: Igor Motov <igor@motovs.org>
|
/ok to test eb6e2db |
|
/ok to test ae4757a |
Overview
Addressing #1574 and #1571.
Replaced strided_dataset with padded_dataset class. Added support all the way up to CAGRA code.
Old class structure (Classes + Inheritance):
New Class Structure (ContainerType Tags + Composition):
Inheritance is removed entirely and all dataset types are on the same level of the inheritance tree.
3 Levels:
Ownership
The index and cagra::build / cagra::index do not own raw vector storage, they only take views.
The old code had a type-erased std::unique_ptr<dataset_view<...>>, i.e. non-owning view handles. The new code uses templates on the index type which determines the type of dataset_view the index holds.
ACE v.s. non-ACE paths on Host
ACE path copies datasets that can't entirely fit in CPU memory in chunks onto GPU memory by calling make_padded_dataset. This is 1x memory on CPU and 1x memory on GPU.
Return types:
Used mainly to maintain lifetime of dataset.
cuvs_cagra_c_api_lifetime_holder
It is a single C++ struct in cagra.cpp that groups the real cagra::index with any extra heap-owned things the C API had to create so the index’s non-owning views stay valid.
Miscellaneous: Extend Serialize Deserialize
Will fill in later
Factories:
Places where make_padded_dataset/view are called internally (not by user):
Host non-ACE path
Tiered CAGRA
Ownership in Downstream Functions:
Improvements:
Breaking Changes for Dataset API:
The following functions are removed since index no longer owns the dataset, index only takes views:
Removed old functions that took mdspan or derivatives of mdspan.
4 cases where index previously owned dataset [all deprecated paths]:
2 edge case build() paths when attach_dataset_on_build == true and a successful dense attach:
Compression Param:
Merge:
These paths have since been removed.
Attach Dataset
Compressed Dataset
Merged Dataset
Deserialize
Helpers
How to attach a compressed dataset onto an uncompressed index?
How to attach a searchable device dataset onto an index built with host build?
a. Utilizes map of host dataset type to device dataset type counterpart
TODOs:
Recent Updates:
Future PRs:
PR#2: Add Support for Compressed Datasets
PR#3: Migrate Rest of Algorithms to use Dataset API