You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
@tarang-jain mentioned in slack:
We should remove the explicit "view" type above and add a member to the dataset_t to designate whether it's owning (this isn't unlike Python ndarray)
We should make sure the c++ impl of the C apis is properly creating a view when needed (if the dastaset_t passed in is owning)
[C API][C++ API] Rework file tree organization splitting all dataset classes, structs, and functions into a separate file within include/core/dataset.hpp
[C++ API] Rename host/device_standard/padded_index type
Index contains graph + dataset
Right now naming of index is associated with the type of dataset bound, but should instead be associated with where the graph is located too
Do we need to introduce a type for the graph? (ex. Host_graph vs device_graph)
[C API] Migrate merge() Dataset API to have users pass in concatenated dataset + integer offset.
Migrate merge() the same way we did with extend(). Users should be responsible with concatenating the 2 datasets into 1 merged dataset and provide that single merged dataset + integer offset. Merge() should then merge graph only.
Think about how to pre-calculate merged storage size after bitset filter:
At C API layer we already centralized an update to a single UpdateDataset function that internally dispatches to AttachDataset or UpdateDeviceDatasetSameLayout based on whether index is standard vs padded
However, at C++ API layer this can’t be easily done because attach_dataset doesn’t mutate the input index and instead returns a new index whereas update_device_dataset_same_layout mutates the original padded input dataset. C++ has guarantees about immutability for thread safety so we need to revisit how the update_dataset() function can be unified at the C++ API layer.
[C API][Language Wrappers]
Cagra-Q path in C API layer and downstream language wrappers was accidentally disabled when compression params was removed.
Need to expose make_vpq_dataset() factory + rebind compressed dataset on uncompressed index functions at C API and downstream language wrappers. C++ API already has this.
Overview
Tracking follow-up items for Dataset API
Sub-tasks:
[C API] C-based dataset_t needs to store a pointer to a proper C++ Dataset instance.
Need accessors for matrix shape information
Need to test these getters in both c and python tests
[C++ API] Fully support padded dataset as input to CAGRA build
Verify it works for all of the build algo methods
Verify this in build kernel code
[Language Wrappers] Remove dataset views from language wrappers
[REVIEW] New Dataset API Clarifying Ownership #1846 (comment)
[REVIEW] New Dataset API Clarifying Ownership #1846 (comment)
@tarang-jain mentioned in slack:
We should remove the explicit "view" type above and add a member to the dataset_t to designate whether it's owning (this isn't unlike Python ndarray)
We should make sure the c++ impl of the C apis is properly creating a view when needed (if the dastaset_t passed in is owning)
Follow up with @aamijar, @tarang-jain, @divyegala
[Tests] Add test coverage
[REVIEW] New Dataset API Clarifying Ownership #1846 (comment)
[REVIEW] New Dataset API Clarifying Ownership #1846 (comment)
[REVIEW] New Dataset API Clarifying Ownership #1846 (comment)
[C++ API] Reuse mdarray/mdspan to simplify Dataset API and make it easier to convert between mdarray and datasets
Address @achirkin ’s C++ changes here: rhttps://github.com/[REVIEW] New Dataset API Clarifying Ownership #1846#discussion_r3553946083
[C++ API] More permanent type placeholder to satisfy DatasetViewT template parameter:
[REVIEW] New Dataset API Clarifying Ownership #1846 (comment)
[C API][C++ API] Rework file tree organization splitting all dataset classes, structs, and functions into a separate file within include/core/dataset.hpp
[REVIEW] New Dataset API Clarifying Ownership #1846 (comment)
[REVIEW] New Dataset API Clarifying Ownership #1846 (comment)
[C++ API] Rename host/device_standard/padded_index type
Index contains graph + dataset
Right now naming of index is associated with the type of dataset bound, but should instead be associated with where the graph is located too
Do we need to introduce a type for the graph? (ex. Host_graph vs device_graph)
[C API] Migrate merge() Dataset API to have users pass in concatenated dataset + integer offset.
Migrate merge() the same way we did with extend(). Users should be responsible with concatenating the 2 datasets into 1 merged dataset and provide that single merged dataset + integer offset. Merge() should then merge graph only.
Think about how to pre-calculate merged storage size after bitset filter:
[REVIEW] New Dataset API Clarifying Ownership #1846 (comment)
[C++ API] Check for hidden H2D copies in ACE path
Was previously added to preserve / mirror upstream ACE behavior
Need to re-evaluate because this goes against our contract of never hiding memory allocation from user
[REVIEW] New Dataset API Clarifying Ownership #1846 (comment)
[REVIEW] New Dataset API Clarifying Ownership #1846 (comment)
[C++ API] Fix update_dataset at C++ API layer
At C API layer we already centralized an update to a single UpdateDataset function that internally dispatches to AttachDataset or UpdateDeviceDatasetSameLayout based on whether index is standard vs padded
However, at C++ API layer this can’t be easily done because attach_dataset doesn’t mutate the input index and instead returns a new index whereas update_device_dataset_same_layout mutates the original padded input dataset. C++ has guarantees about immutability for thread safety so we need to revisit how the update_dataset() function can be unified at the C++ API layer.
[C API][Language Wrappers]
Cagra-Q path in C API layer and downstream language wrappers was accidentally disabled when compression params was removed.
Need to expose make_vpq_dataset() factory + rebind compressed dataset on uncompressed index functions at C API and downstream language wrappers. C++ API already has this.