Reuse raft::mdarray/mdspan directly in dense dataset storage - #2530
Open
HowardHuang1 wants to merge 20 commits into
Open
HowardHuang1 wants to merge 20 commits into
HowardHuang1 wants to merge 20 commits into
Conversation
…2395) dense_owning_matrix/dense_view_matrix (and the VPQ codebook/code matrix aliases) picked between raft::device_matrix and raft::host_matrix via std::conditional_t, even though those are themselves just aliases for raft::mdarray/raft::mdspan with the exact accessor already computed as Accessor. Point the aliases at raft::mdarray/raft::mdspan directly instead. dense_row_major_dataset_owning_storage/_view_storage also wrapped their matrix/view as a field and hand-forwarded view()/data_handle(), which raft::mdarray/raft::mdspan already provide natively. They now inherit from the matrix/view type instead, so those forwards are no longer needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…dataset-functions-one-level-up-and-reuse-mdarray-26_10
as_dataset_view()/n_rows()/dim() can be shared across all dataset
kinds with zero per-kind dispatch inside dataset/dataset_view
themselves. Adds cuvs::neighbors::experimental::{dataset,dataset_view}
as a Spec/policy-based prototype: every member is a one-line forward
to spec_type::get_*(), with all kind-specific logic (empty_spec,
mdarray_spec, vpq_spec) living outside dataset/dataset_view, which
never name or branch on a concrete kind. dataset and dataset_view
stay two independent, non-inheriting types (no shared_ptr, no
"sometimes owning" object).
Purely additive: lives in its own namespace, not referenced by any
existing type, alias, or call site. Verified via a standalone sandbox
(all three specs, both dataset and dataset_view, asserting n_rows/dim/
as_dataset_view) and a full rebuild + DATASET_C_TEST (7/7) +
CAGRA_C_TEST (14/14) + PREPROCESSING_TEST (226/226) + NEIGHBORS_TEST
(339/339), all passing unchanged from before this commit.
Limitations / not yet done:
- Not wired up: the real dataset/dataset_view types (Container-tagged)
and every downstream consumer (CAGRA build/search, serialization,
the C API, compile-time classification traits like
dataset_view_kind_of/is_padded_dataset_view_v) are untouched. This
prototype does not replace them yet.
- mdarray_spec's layout choice for the padded case is unverified
against CAGRA's actual alignment requirements -- needs confirmation
before real use.
- No sparse (CSR/COO) or scalar-quantized specs; only empty/dense/vpq,
matching today's four kinds minus the empty/dense split.
- This design is still an open thread with the team, not finalized;
land as prototype only, pending further review.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…VIDIA#2395) common.hpp's dataset<ContainerType,DataT,IdxT,Accessor>/dataset_view<...> (four hand-specialized structs tagged by ContainerType) are replaced by dataset<T,IdxT,SpecT>/dataset_view<T,IdxT,SpecT>: two independent, non-inheriting types where every member is a one-line forward to a per-kind Spec (empty_dataset_spec, padded_dataset_spec, standard_dataset_spec, vpq_dataset_spec) -- no dispatch inside dataset/dataset_view themselves. VPQ's codebooks move from three flat, ungrouped matrices (vq_code_book, pq_code_book, data as direct members) into a dictionary_type{vq_code_book, pq_code_book} bundle alongside data, giving every kind the same shape (one data slot + one optional dictionary slot). The old dataset_view -> owner back-pointer (.dset()) is removed; a VPQ view now holds its own dictionary_view() directly, matching padded/standard views holding their own state. Public aliases (device_padded_dataset<T,IdxT> etc.) keep their names and 2-arg construction signatures, so most call sites are unaffected. Call sites that reached into dataset internals directly needed updating: .view()/.stride() on a dataset or dataset_view -> .data_view(); raw .vq_code_book/.pq_code_book/.data member access -> .dictionary_view()/ .data_view(); old 3-arg VPQ construction -> 2-arg (codes, dictionary_type{vq,pq}). Touches CAGRA build/search/serialize/merge, VPQ training (pq.cuh), SCANN, Vamana, HNSW export, multi-GPU CAGRA, and the C API's product-quantizer codebook accessors (pq.cpp) -- the one place the C API reaches into dataset internals rather than going through the opaque cuvsDataset handle. Also: removes detail::vpq_dataset_spec_impl (only empty/padded/standard/ vpq_dataset_spec exist as public specs; padded and standard share dense_dataset_spec_impl since two tags need the same body, but VPQ had no second tag to share with, so its body is now inlined directly into vpq_dataset_spec, consistent with how empty_dataset_spec is already written). Verified: full rebuild clean; DATASET_C_TEST (7/7), CAGRA_C_TEST (14/14), PREPROCESSING_TEST (226/226), NEIGHBORS_TEST (339/339) all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cjnolet
added
improvement
…el-up-and-reuse-mdarray-26_10
aamijar
reviewed
Sep 14, 2026
aamijar
left a comment
aamijar
left a comment
Member
There was a problem hiding this comment.
Hi Howard, from our discussion offline, let's think of how the naming should look like for our two dataset view methods.
Current PR:
dataset.data_view() returns the raft_device_matrix_view type
dataset.as_dataset_view() returns the dataset_view typeProposed:
dataset.as_matrix_view() returns raft_device_matrix_view type
dataset.view() returns dataset_view type
What do you think about the proposed naming? Open to suggestions.
aamijar
reviewed
Sep 14, 2026
aamijar
left a comment
aamijar
left a comment
Member
There was a problem hiding this comment.
Hi Howard, I see in this PR you are introducing some structure called dictionary? Is that necessary? We should make things as simple as possible.
…dataset-functions-one-level-up-and-reuse-mdarray-26_10
…euse-mdarray-26_10' of https://github.com/HowardHuang1/cuvs into hh-abstract-common-dataset-functions-one-level-up-and-reuse-mdarray-26_10
Contributor
Author
|
/ok to test 03049e3 |
Merging upstream main (26.12) pulled in code written against pieces of the
old ContainerType-tagged dataset/dataset_view design that this branch
already replaced with dataset<T,IdxT,SpecT>. Git merged these files
without conflict (mostly new/independently-touched code), but the result
didn't compile or was silently wrong against the new API. No functional
changes beyond restoring correct usage of the existing Spec-based API.
- bbq.hpp: BBQ was written as a partial specialization of the old
4-parameter dataset<ContainerType,DataT,IdxT,Accessor> template, which
no longer exists. BBQ's shape (a runtime-sized set of alternate
quantized encodings, mutated in place by the C API) doesn't fit the
dataset<T,IdxT,SpecT> two-slot (data + dictionary) model that
padded/standard/vpq share, so it gets standalone bbq_dataset/
bbq_dataset_view types instead of a SpecT -- same public API as before,
just no longer named as a dataset<>/dataset_view<> specialization.
Re-pointed the surrounding trait specializations (owning_dataset_for_view,
is_bbq_dataset, dataset_view_kind_of, dataset_view_is_device_accessible)
at the new type names; dropped the now-invalid cagra_view_element_type
specialization in favor of a plain value_type member.
- factory.cuh: the merge dropped key_hash/operator==(key,key) (needed by
raft::cache::lru's descriptor cache) entirely, and reverted two of the
make_key() overloads back to is_vpq_dataset_v (owning-only trait, always
false for a view) and flat dataset.pq_code_book access. Restored
key_hash/operator==, fixed the predicate to is_vpq_dataset_view_v, and
fixed the accessor to go through dictionary_view().pq_code_book.
- cagra.cuh, cagra_build.cuh: reverted back to the old .dset() owner
back-pointer (dataset_view no longer holds one) in three spots, a bare
dataset.stride() call (moved to dataset.data_view().stride()), and flat
vpq_dset.data/.vq_code_book/.pq_code_book member access in
reconstruct_vpq_queries(); rewritten to use data_view()/dictionary_view().
- test_iterative_cagra_q.cu: a new test added by the merge still used the
old 3-argument VPQ dataset constructor (vq_code_book, pq_code_book,
codes); switched to the current 2-argument form (codes, dictionary_type
{vq_code_book, pq_code_book}).
Verified: full rebuild clean (including the CUB/Thrust version mismatch
from a stale CMakeCache CUB_DIR, fixed by a clean reconfigure -- unrelated
to source changes); DATASET_C_TEST, CAGRA_C_TEST, PREPROCESSING_TEST, and
NEIGHBORS_TEST all pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…dataset-functions-one-level-up-and-reuse-mdarray-26_10
Contributor
Author
|
/ok to test b2e4bb4 |
cjnolet
reviewed
Sep 29, 2026
cjnolet
reviewed
Sep 29, 2026
cjnolet
reviewed
Sep 29, 2026
cjnolet
reviewed
Sep 29, 2026
cjnolet
reviewed
Sep 29, 2026
cjnolet
reviewed
Sep 29, 2026
cjnolet
reviewed
Sep 29, 2026
cjnolet
reviewed
Sep 29, 2026
…dataset-functions-one-level-up-and-reuse-mdarray-26_10
…2395) dense_row_major_dataset_view_storage inherits from the mdspan type, so it already is the view; its view() just returned *this. Drop it and convert at the two places that called view() on a dataset view (cagra.cuh, cagra_search.cuh) by using data_view() directly. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
data_view() returned the raft mdspan-derived view of a dataset's rows, while as_dataset_view() returns the dataset_view type; the two names did not convey that distinction. Rename data_view() to as_matrix_view() so "matrix view" consistently means an mdspan derivative (e.g. device_matrix_view) and "dataset view" means dataset_view. as_dataset_view() is unchanged. Mechanical rename across dataset/dataset_view, CAGRA, Vamana, HNSW, MG, tiered index, C API, pq.hpp docs and tests. Verified: full rebuild clean; DATASET_C_TEST (9/9), CAGRA_C_TEST (15/15), PREPROCESSING_TEST (226/226), NEIGHBORS_TEST (371/371) pass. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…dataset-functions-one-level-up-and-reuse-mdarray-26_10
…DIA#2395) dataset<T,IdxT,SpecT> and dataset_view<T,IdxT,SpecT> carried VPQ-specific state and methods (a dictionary slot holding the codebooks, and encoded_row_length/vq_n_centers/pq_n_centers/pq_len/pq_bits/pq_dim gated behind `requires` clauses). Padded, standard and empty datasets have no such state, so the shared structs were leaking one kind's abstraction into every kind, and BBQ could not use them at all (it needed standalone types). dataset/dataset_view now hold exactly one payload (data_type / view_type, chosen by the spec) and expose only what every dataset has: n_rows(), dim(), as_matrix_view(), as_dataset_view() and data(). A spec has three functions: get_data_view(), get_n_rows(), get_dim(). Anything else a kind needs is state and methods of that kind's payload, reached through data(). Removed from dataset/dataset_view: - dictionary_type, dictionary_view_type, dictionary_view(), release_dictionary(), release_data() and the std::monostate placeholders that padded/standard/empty used for "no dictionary" - compressed_dataset_spec and the compressed/uncompressed constructor split - the six VPQ helper methods VPQ: vpq_owning_storage / vpq_view_storage are the payloads. Each is the uint8_t codes mdarray/mdspan (so as_matrix_view().data_handle()/extent() keep working) plus vq_code_book and pq_code_book, with the helper methods as members. Construction is (codes, vq_code_book, pq_code_book). Callers that used dictionary_view() now use as_matrix_view(); callers of the helpers use data().pq_bits() etc. BBQ: bbq_dataset_spec plugs the quantizer payloads (bbq_owning_storage / bbq_view_storage, formerly the standalone bbq_dataset / bbq_dataset_view) into dataset/dataset_view like any other kind. quantizers, add_quantizer, has_layout and get_quantizer are reached through data(). This undoes the standalone-BBQ workaround from b70daa5, which only existed because of the dictionary slot. owning_dataset_for_view and dataset_view_is_device_accessible now work for BBQ through the generic templates; dataset_view_kind_of keeps a BBQ partial specialization. Not moved yet: the VPQ and BBQ payloads/specs/traits still live in cuvs::neighbors (common.hpp and bbq.hpp). Relocating them out of cuvs::neighbors is the next step. Also: - ann_cagra_bbq.cuh, ann_nn_descent_bbq.cuh: rmm::cuda_stream_view -> cuda::stream_ref. These did not compile at HEAD (BBQ NVIDIA#2654 merged alongside the stream_ref migration NVIDIA#2521) and are the tests that exercise the BBQ change. - Regenerated Fern pages for the changed headers (fern-api-reference hook). Verified: full rebuild clean. DATASET_C_TEST (9/9), CAGRA_C_TEST (15/15), PREPROCESSING_TEST (226/226), NEIGHBORS_TEST (371/371), NEIGHBORS_ANN_CAGRA_BBQ_UINT32_TEST (105), NEIGHBORS_ANN_NN_DESCENT_TEST (572), NEIGHBORS_ANN_CAGRA_MERGE_TEST (26) pass. NEIGHBORS_ANN_CAGRA_FLOAT_UINT32_TEST was still running at commit time. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…et.hpp (NVIDIA#2395) Datasets are a core type, not a neighbors one: quantizers (pq.hpp, bbq.hpp) and clustering also need them and should not depend on anything in neighbors. Create cuvs/core/dataset.hpp as the root of the dataset header tree and move the whole dataset section of neighbors/common.hpp into it: dataset and dataset_view, the accessor aliases and dense/empty storage, the empty/padded/standard/vpq specs and their aliases, the kind traits, with_accessor/device_counterpart, the CAGRA row-width helpers, and the make_*_padded_dataset / make_*_standard_dataset factories. neighbors/common.hpp includes the new header, so none of its 67 includers change. This is a pure cut and paste: the 1187 moved lines are byte-identical to the removed block, the namespace is still cuvs::neighbors, and there are no call-site changes. The Fern API reference is regenerated for the new header (fern-api-reference hook). Not done yet (follow-ups): the VPQ and BBQ pieces and the dataset_view_kind enum still live in the moved block and will move into quantize/pq.hpp and quantize/bbq.hpp as children of dataset; the dataset types then move from cuvs::neighbors to cuvs::core. Verified: full rebuild clean; DATASET_C_TEST (9), CAGRA_C_TEST (15), PREPROCESSING_TEST (226), NEIGHBORS_TEST (371), NEIGHBORS_ANN_CAGRA_BBQ_UINT32_TEST (105), NEIGHBORS_ANN_NN_DESCENT_TEST (572), NEIGHBORS_ANN_CAGRA_MERGE_TEST (26), NEIGHBORS_ANN_CAGRA_FLOAT_UINT32_TEST (3594) pass. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…VIDIA#2395) core/dataset.hpp is the root of the dataset header tree and must not know about any compressed kind. Quantizers (pq.hpp, bbq.hpp) include it and define their datasets as children of dataset/dataset_view, with no dependency on anything in neighbors/. core/dataset.hpp: - Remove the dataset_view_kind enum and dataset_view_kind_of, which named vpq_f16/vpq_f32/bbq, and every VPQ-specific type and trait. - Padded/standard/empty classification is built directly on the spec types. New generic dataset_view_has_spec_v<V, SpecPred> lets any kind classify its own views with a spec predicate. - is_dataset_view_v replaces "kind != unknown"; it is false (never a hard error) for non-dataset types such as the plain mdspans passed to the deprecated build() overloads. - with_accessor is generic: every spec provides rebind_accessor<NewAccessor>, replacing the eight per-kind specializations. - compatible_host_device_dataset_views_v is "same type as device_counterpart_t<H>" instead of comparing kind enums. quantize/pq.hpp (cuvs::preprocessing::quantize::pq): - Now holds the VPQ payloads (vpq_owning_storage / vpq_view_storage), vpq_dataset_spec, the device_/host_vpq_dataset[_view] aliases, the is_vpq_* traits (including the f16/f32 variants) and vpq_params. - Includes core/dataset.hpp and cluster/kmeans.hpp only; no longer includes neighbors/common.hpp. quantize/bbq.hpp (cuvs::preprocessing::quantize::bbq): - Includes core/dataset.hpp instead of neighbors/common.hpp. The BBQ payloads, bbq_dataset_spec, aliases and traits move here from cuvs::neighbors, classified through dataset_view_has_spec_v. This is a breaking change to the public C++ API: device_vpq_dataset[_view], host_vpq_dataset[_view], vpq_params, is_vpq_*, device_bbq_dataset[_view] and is_bbq_* are no longer in cuvs::neighbors. Call sites (CAGRA, NN-descent, Vamana, SCANN, the C API, benchmarks and tests) are updated; cagra.hpp and the serialize/test helpers now include pq.hpp explicitly. Fern API pages are regenerated. Not done yet: the dataset types themselves are still in cuvs::neighbors (core/dataset.hpp), so pq.hpp/bbq.hpp still name cuvs::neighbors::dataset; moving them to cuvs::core is the next step. Verified: full rebuild clean (13 test targets, including after clang-format). DATASET_C_TEST (9), CAGRA_C_TEST (15), PREPROCESSING_TEST (226), NEIGHBORS_TEST (371), NEIGHBORS_ANN_CAGRA_BBQ_UINT32_TEST (105), NEIGHBORS_ANN_NN_DESCENT_TEST (572), NEIGHBORS_ANN_CAGRA_MERGE_TEST (26), NEIGHBORS_ANN_CAGRA_FLOAT_UINT32_TEST (3594), NEIGHBORS_ANN_SCANN_TEST (105), NEIGHBORS_ANN_VAMANA_TEST (1260), NEIGHBORS_HNSW_TEST (256), NEIGHBORS_TIERED_INDEX_TEST (72) and NEIGHBORS_MG_TEST pass. The ANN benchmarks are not built in this configuration, so those sources were updated but not compiled. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Datasets are a core type, so they now live in the namespace that matches
their header: everything declared in cuvs/core/dataset.hpp moves from
cuvs::neighbors to cuvs::core. The quantizers (pq.hpp, bbq.hpp) build their
datasets on cuvs::core::dataset / dataset_view and no longer name anything
in neighbors.
Moved to cuvs::core:
- dataset, dataset_view and the empty/padded/standard specs and aliases
(device_/host_{empty,padded,standard}_dataset[_view])
- the kind traits, dataset_view_has_spec_v, is_dataset_view_v,
with_accessor, device_counterpart, owning_dataset_for_view and
ann_dataset_view
- the make_{device,host}_{padded,standard}_dataset[_view] factories and the
CAGRA row-width helpers
- the accessor aliases and dense-storage helpers in core::detail
This is a hard break of the public C++ API with no compatibility aliases in
cuvs::neighbors: user code that names cuvs::neighbors::make_device_padded_dataset,
device_padded_dataset_view and the other moved names must switch to
cuvs::core. The PR should be labelled breaking.
Call sites are updated mechanically across cpp, c and examples: about 480
fully-qualified references, plus unqualified uses inside
namespace cuvs::neighbors (mostly cagra.hpp and the C API). The unused
`nb = cuvs::neighbors` alias in cagra.cuh is removed. The Fern API pages are
regenerated and list the types as core::*.
The rewrite had to avoid identifiers that share a name with the moved API:
local variables and parameters named like a type alias (for example
device_empty_dataset_view in add_nodes.cuh and the C API's
device_padded_dataset parameters), and the C API's own static helpers
make_device_padded_dataset / make_host_padded_dataset_view and friends in
c/src/neighbors/cagra.cpp, which are not the core factories. Those are left
unqualified; type names are only qualified where used as templates and
function names only where called.
The examples and the ANN benchmarks are not built in this configuration, so
those sources were updated but not compiled.
Verified: full rebuild clean (13 test targets, including after
clang-format); all pre-commit hooks pass. DATASET_C_TEST (9), CAGRA_C_TEST
(15), PREPROCESSING_TEST (226), NEIGHBORS_TEST (371),
NEIGHBORS_ANN_CAGRA_BBQ_UINT32_TEST (105), NEIGHBORS_ANN_NN_DESCENT_TEST
(572), NEIGHBORS_ANN_CAGRA_MERGE_TEST (26),
NEIGHBORS_ANN_CAGRA_FLOAT_UINT32_TEST (3594), NEIGHBORS_ANN_SCANN_TEST
(105), NEIGHBORS_ANN_VAMANA_TEST (1260), NEIGHBORS_HNSW_TEST (256),
NEIGHBORS_TIERED_INDEX_TEST (72) and NEIGHBORS_MG_TEST pass.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
cjnolet
reviewed
Oct 3, 2026
cjnolet
reviewed
Oct 3, 2026
…dataset-functions-one-level-up-and-reuse-mdarray-26_10
core holds vocabulary types shared by every algorithm, so nothing in it may
carry the name of a child namespace (cagra, ann, ...). Rename the four
identifiers in core/dataset.hpp that did, and reword the comments that tied
the header to CAGRA.
Renames (no behaviour change; unique identifiers, mechanical across cpp, c
and examples):
- ann_dataset_view -> dataset_like (46 uses)
- cagra_required_row_width -> padded_row_width (18 uses)
- matrix_row_width_matches_cagra_required
-> matrix_has_padded_row_width (15 uses)
- cagra_view_element_type_t -> dataset_view_value_t (2 uses)
dataset_like is a structural concept: any type with n_rows() and dim().
Owning datasets and dataset views both satisfy it, so the doc comment no
longer claims it checks for a non-owning view; is_dataset_view_v remains the
nominal check for a literal dataset_view<...>. The padded-width helpers are
general row-padding arithmetic used by the make_*_padded_dataset factories,
not anything specific to CAGRA, so they are renamed rather than moved.
Comments in core/dataset.hpp that referred to "dense graph build", "for
CAGRA", the CAGRA row-width banner, and
cuvs::neighbors::detail::deserialize_standard() are reworded generically. The
header no longer mentions cagra, ann, ivf, hnsw, vamana, scann, brute force,
nn_descent, neighbors, graph, knn or recall in code or comments.
Verified: full rebuild clean (14 test targets, including the Roaring test now
that the upstream stream-API fix is merged); all pre-commit hooks pass.
DATASET_C_TEST (9), CAGRA_C_TEST (15), PREPROCESSING_TEST (226),
CORE_ROARING_ALLOWLIST_TEST (6), NEIGHBORS_TEST (371),
NEIGHBORS_ANN_CAGRA_BBQ_UINT32_TEST (105), NEIGHBORS_ANN_NN_DESCENT_TEST (572),
NEIGHBORS_ANN_CAGRA_MERGE_TEST (26), NEIGHBORS_ANN_SCANN_TEST (105),
NEIGHBORS_ANN_VAMANA_TEST (1260), NEIGHBORS_HNSW_TEST (256) and
NEIGHBORS_TIERED_INDEX_TEST (72) pass. NEIGHBORS_ANN_CAGRA_FLOAT_UINT32_TEST
and NEIGHBORS_MG_TEST had not finished at commit time (the float suite had
1700+ passing and no failures).
The owning and view VPQ payloads were two separate structs that each declared vq_code_book and pq_code_book, and the call sites reached them through a mix of as_matrix_view() and data() on the same dataset. Define the payload once and make every call site bind it a single time. Unified payload (quantize/pq.hpp): - vpq_storage<CodesT, BookT> is the single definition: it derives from the encoded-rows matrix and holds the VQ and PQ codebooks, sharing the existing helper methods (pq_bits, pq_len, ...). - vpq_owning_storage and vpq_view_storage are now aliases of it, so every existing name and member is unchanged. The owning form uses mdarrays; the view form uses their const_view_type. - The type aliases are now grouped as an owning pair: vpq_data_matrix (encoded uint8 rows) and vpq_codebook_matrix (renamed from vpq_vq_book_matrix, which was misleading because it also types the PQ codebook). The non-owning forms are derived from each with ::const_view_type, so the separate vpq_codes_view alias and the accessor using-declaration it needed are removed. A compile-time check confirmed the derived view type is identical to the spelled-out one for device and host accessors, half and float codebooks, and 32- and 64-bit indices. Call sites: - Owning-dataset code binds the payload once as `auto const& vpq = X.data()` and uses it for shape, helpers, pointers and strides: the decode test helper (vpq_utils.cuh), pq.cuh (transform, inverse_transform, vpq_convert_math_type, vpq_build_half) and the C API codebook getters. .view() is called only where a view type is required (functions taking views, the ScaNN device lambda, DLPack export). - Code that receives a dataset_view binds the view payload as `auto const& vpq_view = dataset_view.data()`: cagra build(), the descriptor-cache make_key(), the VPQ distance descriptor init()/priority(), and reconstruct_vpq_queries(). The parameter in those functions that was called `dataset` is renamed `dataset_view` (and reconstruct_vpq_queries' `vpq_view` parameter becomes `dataset_view`) so the owning/view distinction is visible from the names. Only code identifiers are renamed, not error strings or comments. - scann_build.cuh binds the codebook through a const reference, keeping the read-only view the original code produced (a non-const `data()` would otherwise have yielded a mutable view). No behaviour change. Verified: formatting hooks pass; full rebuild clean (14 test targets). DATASET_C_TEST (9), CAGRA_C_TEST (14, with CagraC.BuildSearchACEDisk excluded because a stale /tmp/cagra_ace_test_disk from an earlier run makes it fail), PREPROCESSING_TEST (226), CORE_ROARING_ALLOWLIST_TEST (6), NEIGHBORS_ANN_CAGRA_BBQ_UINT32_TEST (105), NEIGHBORS_ANN_NN_DESCENT_TEST (572), NEIGHBORS_ANN_CAGRA_MERGE_TEST (26) and NEIGHBORS_ANN_SCANN_TEST (105) pass. NEIGHBORS_ANN_VAMANA_TEST, NEIGHBORS_HNSW_TEST, NEIGHBORS_TIERED_INDEX_TEST, NEIGHBORS_TEST, NEIGHBORS_ANN_CAGRA_FLOAT_UINT32_TEST and NEIGHBORS_MG_TEST had not finished at commit time (no failures so far).
Contributor
Author
|
/ok to test 5ef4515 |
achirkin
requested changes
Oct 6, 2026
Comment on lines
+54
to
+58
| template <typename T, typename IdxT, typename SpecT> | ||
| struct dataset; | ||
|
|
||
| template <typename T, typename IdxT, typename SpecT> | ||
| struct dataset_view; |
Contributor
There was a problem hiding this comment.
Do you still need these forward declarations? Why?
Comment on lines
+130
to
+131
| struct dense_row_major_dataset_owning_storage : public MatrixT { | ||
| uint32_t logical_dim_; |
Contributor
There was a problem hiding this comment.
Please, try to remove these "storage" wrappers on top of mdarrays/mdspans. I see you probably added them to augment the mdarrays with extra logical_dim_ variable to represent padded arrays, right? That is not necessary; a more mdspan-friendly implementation is to reuse the strided/padded layout policy, then use the stride as the "physical" dim and extents as "logical" dim. In raft, we have raft::layout_left_padded<T> to simplify this job.
Check here how it is done in the reference implementation: https://github.com/NVIDIA/cuvs/pull/2532/changes#diff-0dc3b1e88d514cb85de23c490c4c0f494dc436faedf466c0caaf92753a721e07R453-R457
Comment on lines
+302
to
+304
| private: | ||
| data_type data_; | ||
| }; |
Contributor
There was a problem hiding this comment.
We still need a dictionary member to support compressed dataset. Without that, the current implementation is always just a wrapper on top of mdarray.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #2395.
This PR reworks our original container based structure with container type tags and allows us to abstract more shared functionality one level up from the concrete dataset types and move them into the dataset and dataset_view structs. Shared functions like as_dataset_view(), n_rows(), dim() are all abstracted one level up and a new dictionary_view to handle vpq (and all future compressed dataset type) codebooks is introduced in this PR.
dense_owning_matrix/dense_view_matrix (and the VPQ codebook/code matrix aliases) picked between raft::device_matrix and raft::host_matrix via std::conditional_t, even though those are themselves just aliases for raft::mdarray/raft::mdspan with the exact accessor already computed as Accessor. Point the aliases at raft::mdarray/raft::mdspan directly instead.
dense_row_major_dataset_owning_storage/_view_storage also wrapped their matrix/view as a field and hand-forwarded view()/data_handle(), which raft::mdarray/raft::mdspan already provide natively. They now inherit from the matrix/view type instead, so those forwards are no longer needed.