NVIDIA / NVIDIA/cuvs

[FEA][C++ API] Abstract common functions into dataset/dataset_view structs which currently do nothing + Reuse mdarray/mdspan to simplify Dataset API

Open
#2,395 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

feature request
Dominant language
Cuda
Stars
854
Forks
236
Avg merge
3d 3h
Merged PRs (30d)
62

Description

Address @Artem’s C++ changes here:
https://github.com/NVIDIA/cuvs/pull/1846#discussion_r3553946083

2 tasks:

  1. dataset and dataset_view struct contain no information.
template <typename ContainerType, typename DataT, typename IdxT, typename Accessor>
struct dataset;

template <typename ContainerType, typename DataT, typename IdxT, typename Accessor>
struct dataset_view;

Only concrete dataset types like padded_dataset, standard_dataset, and vpq_dataset have information. However, that information is replicated for each of the concrete datasets even though the code logic is the same. This can be abstracted out and moved one level up to reduce code redundancy. In particular: view() function and n_rows() function are same regardless of concrete dataset type and can be abstracted one level up into the dataset struct. This makes it so that later we can just have operations on some basic common dataset struct features without specifying concrete dataset type and only dispatch on actual concrete dataset type later in the nested calls.

  1. We have:
template <typename DataT, typename IdxT, typename Accessor>
using dense_owning_matrix = std::conditional_t<Accessor::is_device_accessible,
                                               raft::device_matrix<DataT, IdxT, raft::row_major>,
                                               raft::host_matrix<DataT, IdxT, raft::row_major>>;

template <typename DataT, typename IdxT, typename Accessor>
using dense_view_matrix =
  std::conditional_t<Accessor::is_device_accessible,
                     raft::device_matrix_view<const DataT, IdxT, raft::row_major>,
                     raft::host_matrix_view<const DataT, IdxT, raft::row_major>>;

Have dense dataset use implementation of mdarray because it's just the same thing. For compressed dataset this would be something different, perhaps several mdarrays for codebooks and any additional info. We have a rhombus of indirection where we are first converting from concrete dataset to dense dataset and then to underlying mdarray when we can just directly go from concrete dataset to mdarray and get rid of the middle layer. The dense_dataset layer of indirection is unecessary. Get rid of the conditional.

This may involve removing host or device accessor to get rid of indirection in dense dataset. Just pass that host or device accessor directly to mdarray as shown here through a comment on the BBQ dataset PR: https://github.com/NVIDIA/cuvs/pull/2506#discussion_r3877889073

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start from the dataset, dataset_view, dense_dataset, padded_dataset, standard_dataset, and vpq_dataset definitions, then read the linked pull-request discussions for the intended API direction. Trace view() and n_rows() duplication and the dense-to-mdarray conversion; done means common behavior is centralized and dense datasets use mdarray directly without the unnecessary conditional layer.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend-api-design
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
32/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.