[RELEASE] raft v24.06 #2341
Merged
[RELEASE] raft v24.06 #2341
Conversation
This file contains 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
Forward-merge branch-24.04 into branch-24.06 [skip ci]
Forward-merge branch-24.04 into branch-24.06 [skip ci]
Forward-merge branch-24.04 into branch-24.06 [skip ci]
Forward-merge branch-24.04 to branch-24.06
Forward-merge branch-24.04 to branch-24.06
Forward-merge branch-24.04 to branch-24.06
Forward-merge branch-24.04 to branch-24.06
…2207) We want to get rid of raw memory resources so move to the new interface instead Authors: - Michael Schellenberger Costa (https://github.com/miscco) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2207
This PR removes the use of the deprecated `cuco::sentinel` namespace. Needed by rapidsai/rapids-cmake#569 Authors: - Yunsong Wang (https://github.com/PointKernel) Approvers: - Divye Gala (https://github.com/divyegala) URL: #2243
Forward-merge branch-24.04 into branch-24.06 [skip ci]
This PR adds the support for subspace dim (pq_dim) = 4 in CAGRA-Q Authors: - tsuki (https://github.com/enp1s0) Approvers: - Artem M. Chirkin (https://github.com/achirkin) - Corey J. Nolet (https://github.com/cjnolet) URL: #2244
Closes #2230. I am also adding `nn_descent` to the build parameters of cagra Authors: - Micka (https://github.com/lowener) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2231
- This PR is one part of the feature of #1969 - Add the API of 'select_k' accepting CSR as input Authors: - James Rong (https://github.com/rhdong) Approvers: - Ben Frederickson (https://github.com/benfred) - Micka (https://github.com/lowener) - Corey J. Nolet (https://github.com/cjnolet) Authors: - rhdong (https://github.com/rhdong) Approvers: - Artem M. Chirkin (https://github.com/achirkin) - Corey J. Nolet (https://github.com/cjnolet) URL: #2140
Forward-merge branch-24.04 into branch-24.06
The init order of class members needs to follow the order listed in the class. This corrects the error for the cagra index type, as it was found to be wrong via rapidsai/cuvs#66 Authors: - Robert Maynard (https://github.com/robertmaynard) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2254
Issue: rapidsai/build-planning#30 Authors: - Kyle Edwards (https://github.com/KyleFromNVIDIA) Approvers: - Bradley Dice (https://github.com/bdice) - Jake Awe (https://github.com/AyodeAwe) URL: #2266
This PR fixes a compilation error in CAGRA when enabling log output. Authors: - tsuki (https://github.com/enp1s0) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2262
Removes the need for us to install GTest or GBench in our raft testing CI Authors: - Robert Maynard (https://github.com/robertmaynard) Approvers: - Bradley Dice (https://github.com/bdice) - Jake Awe (https://github.com/AyodeAwe) URL: #2265
Authors: - Paul Taylor (https://github.com/trxcllnt) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2271
…p to 2.6x faster) (#2259) This PR implements two optimizations to `coalescedReductionThinKernel` which is used for coalesced reductions of tall matrices (many rows) and/or thin (few columns): 1. Process multiple rows per warp to increase bytes in flight and amortize load latencies. 2. Use a vectorized reduction to avoid the LSU bottleneck and have fewer global stores (and at least partially coalesced). The benchmark below shows the achieved SOL percentage on A30. I also measured that on H200, it achieved 84% SOL for 32 columns and up to 94% for 512 columns.  Authors: - Louis Sugy (https://github.com/Nyrio) - Tamas Bela Feher (https://github.com/tfeher) Approvers: - Tamas Bela Feher (https://github.com/tfeher) - Corey J. Nolet (https://github.com/cjnolet) URL: #2259
Closes #2261 For reviewers: Many of changes are simple textual replace of `rmm::mr::device_memory_resource *` with `rmm::device_async_resource_ref`. However there are several places where RAFT used a default value of `nullptr` for `device_memory_resource*` parameters. This is incompatible with a `resource_ref`, which is a lightweight non-owning reference class, not a pointer. In most places, I was able to either remove the default parameter value, or use `rmm::mr::get_current_device_resource()`. In the case of ivf_pq, I removed the deprecated versions of `search` that took an `mr` parameter. I removed the unused old src/util/memory_pool.cpp and its headers. Authors: - Mark Harris (https://github.com/harrism) Approvers: - Artem M. Chirkin (https://github.com/achirkin) - Dante Gama Dessavre (https://github.com/dantegd) - Corey J. Nolet (https://github.com/cjnolet) URL: #2269
* Update the `cuda11.8-conda` devcontainer's base image * Remove the devcontainer when the VSCode window closes * Adds a descriptive name to the running container: ```shell $ docker ps -a CONTAINER ID IMAGE ... NAMES 0dbb364fe544 vsc-raft-... ... rapids-raft-24.06-cuda12.2-conda $ docker rm -f rapids-raft-24.06-cuda12.2-conda ``` Authors: - Paul Taylor (https://github.com/trxcllnt) Approvers: - Jake Awe (https://github.com/AyodeAwe) URL: #2275
I noticed that `./build.sh clean` was executing some CMake code when it shouldn't. Authors: - Micka (https://github.com/lowener) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2270
Authors: - Paul Taylor (https://github.com/trxcllnt) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2274
`InnerProduct` Distance Metric for CAGRA search. InnerProduct in graph building is supported using IVF-PQ for building the graph. NNDescent does not currently support any other metric except L2Expanded. Authors: - Tarang Jain (https://github.com/tarang-jain) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Tamas Bela Feher (https://github.com/tfeher) - tsuki (https://github.com/enp1s0) - Corey J. Nolet (https://github.com/cjnolet) URL: #2260
Comments in `cpp/include/raft/neighbors/cagra_serialize.cuh` are outdated. Authors: - Yinzuo Jiang (https://github.com/jiangyinzuo) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2283
Contributes to rapidsai/build-planning#15. Authors: - Kyle Edwards (https://github.com/KyleFromNVIDIA) Approvers: - Corey J. Nolet (https://github.com/cjnolet) - Bradley Dice (https://github.com/bdice) URL: #2285
Contributes to rapidsai/build-planning#54. The `libraft-headers` and `libraft-headers-only` conda packages are bundling `rmm`'s headers. I believe that's because the `librmm` conda package isn't available in the `libraft*` conda build environment, and as a result it's getting `rmm` via CPM (thanks to `rapids-cmake`). As a result, this project and any that depend on it are seeing warnings like the following in conda builds where `conda`'s `path_conflict` setting is set to `warn` or `prevent` (like #2245): ```text This transaction has incompatible packages due to a shared path. packages: rapidsai-nightly/linux-64::librmm-24.04.00a38-cuda11_240326_ga98931b9_38, rapidsai-nightly/linux-64::libraft-headers-only-24.04.00a93-cuda11_240326_g9637b3c2_93 path: 'include/rmm/mr/device/arena_memory_resource.hpp' ``` To fix that, this proposes the following changes: * make `librmm` a `host` dependency of the following conda packages: `libraft-headers-only`, `libraft-headers` ### Benefits of this change * slightly smaller `libraft-headers` and `libraft-headers-only` conda packages * reduced risk of runtime and installation issues caused by file clobbering ## Notes for reviewers ### History of changes to the `librmm` dependency for `libraft-headers`: * both `run` and `host`: #508 * both `run` and `host`, but ignoring its `run_exports`: #1264 * just `run`, but ignoring its `run_exports`: #2102 In particular, #2102 referred to the `host` dependency on `librmm` as "extraneous" but from a packaging perspective, I don't think it is. `librmm` being in `host` means it'll be present in the build environment, which means its headers will be *found* instead of *downloaded*, and therefore not packaging into the `libraft*` conda packages. ### How I tested this Built all the `raft` conda packages locally from `branch-24.06` and confirmed that they contain `rmm` headers. Then again from this branch and confirmed they were gone. ```shell docker run \ --rm \ --env-file "${PWD}/aws-creds.env" \ -v $(pwd):/opt/work \ -w /opt/work \ -it rapidsai/ci-conda:cuda12.2.2-ubuntu22.04-py3.10-amd64 \ bash CI="true" \ ci/build_cpp.sh # On 'branch-24.06', this showed the rmm headers being packaged. # On this branch, they're omitted. tar --bzip2 -tf \ /tmp/conda-bld-output/linux-64/libraft-headers-only-24.06.00a50-cuda12_240430_g1e0e2283_50.tar.bz2 \ | grep 'include/rmm' \ | wc -l ``` Also checked the CI logs from `conda-cpp-build` jobs here. On other recent PRs, I see CPM downloading `rmm` ... ```text -- CPM: Adding package [email protected] (branch-24.06) ``` ... and all the `rmm` headers getting installed as part of the `libraft-headers` package ```text -- Installing: /opt/conda/conda-bld/_h_env_placehold_placehold_..._placeho/include/rmm/cuda_stream.hpp ``` ([build link](https://github.com/rapidsai/raft/actions/runs/8904352932)) Here, I see `librmm` coming through via the conda package requirements ... ```text The following NEW packages will be INSTALLED: ... librmm: 24.06.00a17-cuda12_240430_g26fa9ecb_17 rapidsai-nightly ``` ... and being used instead of downloads via CPM ... ```text -- CPM: Using local package [email protected] ``` ... and no installation of the `rmm` headers as part of building any `libraft` packages. ([build link](https://github.com/rapidsai/raft/actions/runs/8910675575/job/24470450187?pr=2284)) Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Bradley Dice (https://github.com/bdice) - Ray Douglass (https://github.com/raydouglass) URL: #2284
Until now, raft has stored a map of NVTX colors (annotation -> color) to avoid using the same color for different annotations and keep using the same color for the same annotations. This map is a shared state. During an extensive ANN_BENCH throughput testing it has turned out that the mutex guarding the map can sometimes become a bottleneck when the number of concurrent threads is really large (>~ 256). This PR replaces the unordered map and the mutex guarding it with a deterministic hash value of the annotation instead (which is stateless). **Pros:** - No shared state, no mutexes. - Assigns the same colors to the same annotations across program runs. **Cons:** - Sometimes different annotations can have the same color (hash collisions). Authors: - Artem M. Chirkin (https://github.com/achirkin) Approvers: - Tamas Bela Feher (https://github.com/tfeher) - Corey J. Nolet (https://github.com/cjnolet) URL: #2310
Re-enable move constructor for the `configured_raft_resources`. It was implicitly deleted before, which was exposed and made explicit in #2269 . Allowing move semantics here means avoiding an extra unwanted overhead during algorithm preparation in the benchmarks tool. Authors: - Artem M. Chirkin (https://github.com/achirkin) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2311
Split instances of RaftCagra into multiple files to compile them in parallel and thus reduce the total benchmark compile time. Authors: - Artem M. Chirkin (https://github.com/achirkin) Approvers: - Robert Maynard (https://github.com/robertmaynard) - Corey J. Nolet (https://github.com/cjnolet) URL: #2313
Introduce a new virtual member `uses_stream()` for the `AnnGPU` class. Overriding this allows an algorithm inform the benchmark whether the stream synchronization is needed between benchmark iterations. This is relevant for a potential persistent kernel where the CPU threads use an independent mechanics to synchronize and get the results from the GPU. This is different from just not implementing `AnnGPU` for an algorithm in that it allows the algorithm to decide whether the synchronization is needed (depending on input parameters at runtime), while still providing the `get_sync_stream()` functionality. Authors: - Artem M. Chirkin (https://github.com/achirkin) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2314
Replace the `size_t` type in the `AnnBase::search` for the output neighbor indices with a common `AnnBase::index_type`. This PR stops short of changing the behavior of the benchmarks, since it keeps `using index_type = size_t`. The introduction of the new type has couple benefits: - Makes the usage of the `index_type` more clear in the code, distinguishing it from the extents type, which is usually `size_t` as well. - Makes it possible to quickly change the alias to `uint32_t` during development and experiments. This is needed to avoid calling extra `linalg::map` on the produced results when the algorithm output is not compatible with `size_t`. As a small extra change, I've factored out common IVF-PQ - CAGRA-Q refinement code into a separate `refine_helper` function. Authors: - Artem M. Chirkin (https://github.com/achirkin) Approvers: - Tamas Bela Feher (https://github.com/tfeher) URL: #2315
I found incorrect citation information in README.md. This PR fixes that and also updates the CAGRA paper information. Authors: - tsuki (https://github.com/enp1s0) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2318
Align our constuctor order to match what C++ spec says will occur: ``` Then, non-static data members are initialized in the order they were declared in the class definition (again regardless of the order of the mem-initializers). ``` Otherwise consumer of raft will get warnings when building with `-Wall -Wextra` and using the `index` type. Authors: - Robert Maynard (https://github.com/robertmaynard) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2317
…2319) The scale_obs function was calling a custom kernel to scale the elements of a matrix column by the l2-norm of the column. There were two issues: 1. The kernel launch parameters would go out of bounds if the graph was too large. The Y dimension is limited to 65535, but there was no logic in the function to ensure that we didn't set the Y value larger than that 3. A bug in the kernel, the column norm was not being calculated correctly... the outer loop was terminating, hence we were really only computing the column norm of the last column in the block. Then we were normalizing all columns in the block by that value instead of by each value. To simplify (there's going to be some optimization work done this summer), I replaced this with a simple thrust call that will scale the values correctly. Authors: - Chuck Hastings (https://github.com/ChuckHastings) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2319
* add missing files to update-version.sh * use alternate regex * remove unneeded sed following #2285
Cleans up a collection of anti-patterns in the raft CMake code while also enabling building faiss from latest `main` Authors: - Robert Maynard (https://github.com/robertmaynard) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2323
### Brief Add another workspace memory resource that does not have the explicit memory limit. That is, after the change we have the following: 1. `rmm::mr::get_current_device_resource()` is default for all allocations, as before. It is used for the allocations with unlimited lifetime, e.g. returned to the user. 2. `raft::get_workspace_resource()` is for temporary allocations and forced to have fixed size, as before. However, it becomes smaller and should be used only for allocations, which do not scale with problem size. It defaults to a thin layer on top of the `current_device_resource`. 3. `raft::get_large_workspace_resource()` _(new)_ is for temporary allocations, which can scale with the problem size. Unlike `workspace_resource`, its size is not fixed. By default, it points to the `current_device_resource`, but the user can set it to something backed by the host memory (e.g. managed memory) to avoid OOM exceptions when there's not enough device memory left. ## Problem We have a list of issues/preference/requirements, some of which contradict others 1. We rely on RMM to handle all allocations and we often use [`rmm::mr::pool_memory_resource`](https://github.com/rapidsai/raft/blob/9fb05a2ab3d72760a09f1b7051e711d773682ef1/cpp/bench/ann/src/raft/raft_ann_bench_utils.h#L73) for performance reasons (to avoid lots of cudaMalloc calls in the loops) 2. Historically, we've used managed memory allocators as a workaround to [avoid OOM errors](https://github.com/rapidsai/raft/blob/5e80c1d2159e00a204ab5db0f5ca3f9ec43187c7/cpp/include/raft/neighbors/detail/ivf_pq_build.cuh#L1788-L1795) or [improve speed (by increasing batch sizes)](https://github.com/rapidsai/raft/blob/5e80c1d2159e00a204ab5db0f5ca3f9ec43187c7/cpp/include/raft/neighbors/detail/ivf_pq_build.cuh#L1596-L1603). 3. However, the design goal is to avoid setting allocators on our own and to give the full control to the user (hence the workaround in 2 [was removed](addb059#diff-f7f070424d71da5321d470416d1a4ca3605c4290c34c4a1c1d8b2240747000d2)). 4. We introduced the [workspace resource](#1356) earlier to allow querying the available memory reliably and maximize the batch sizes accordingly (see also issue [#1310](#1310)). Without this, some of our batched algorithms either fail with OOM or severely underperform due to small batch sizes. 5. However, we cannot just put all of RAFT temporary allocations into the limited `workspace_resource`, because some of them scale with the problem size and would inevitably fail with OOM at some point. 6. Setting the workspace resource to the managed memory is not advisable as well for performance reasons: we have lots of small allocations in performance critical sections, so we need a pool, but a pool in the managed memory inevitably outgrows the device memory and makes the whole program slow. ## Solution I propose to split the workspace memory into two: 1. small, fixed-size workspace for small, frequent allocations 2. large workspace for the allocations that scale with the problem size Notes: - We still leave the full control over the allocator types to the user. - Neither of the workspace resource should have unlimited lifetime / returned to the user. As a result, if the user sets the managed memory as the large workspace resource, the memory is guaranteed to be released after the function call. - We have the option to use the slow managed memory without a pool for large allocations, while still using a fast pool for small allocations. - We have more flexible control over which allocations are "large" and which are "small", so hopefully using the managed memory is not so bad for performance. Authors: - Artem M. Chirkin (https://github.com/achirkin) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2322
Store operations are void. Authors: - Aaron Siddhartha Mondal (https://github.com/aaronmondal) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2292
Too long index file name would lead to a crash while calling the index serialization routines. Such long filenames can occur if we try to specialize many parameters for CAGRA ann index. This PR fixes the issue by replacing the long index file name with a hash. Drawback is the filename will not be descriptive. Authors: - Tamas Bela Feher (https://github.com/tfeher) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2280
jupyer -> jupyter Authors: - Ikko Eltociear Ashimine (https://github.com/eltociear) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2308
Authors: - Divye Gala (https://github.com/divyegala) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2297
Replace hyphens with underscores in `raft-ann-bench` to make it a valid Python identifier. Also add a Python 3.11 tag to `raft-ann-bench`, and use the `VERSION` file instead of an attribute. Authors: - Kyle Edwards (https://github.com/KyleFromNVIDIA) Approvers: - Divye Gala (https://github.com/divyegala) - Mike Sarahan (https://github.com/msarahan) URL: #2333
Change the imported package name to reflect the new name as of #2333. Authors: - Kyle Edwards (https://github.com/KyleFromNVIDIA) Approvers: - Bradley Dice (https://github.com/bdice) - Divye Gala (https://github.com/divyegala) URL: #2338
- This PR is one part of the feature of #1969 - Add the API of 'search_with_filtering' for brute force. Authors: - James Rong (https://github.com/rhdong) ```shell ***WARNING*** CPU scaling is enabled, the benchmark real time measurements may be noisy and will incur extra overhead. ----------------------------------------------------------------------------------------------------- Benchmark Time CPU Iterations ----------------------------------------------------------------------------------------------------- KNN/float/int64_t/brute_force_filter_knn/0/0/0/manual_time 33.1 ms 69.9 ms 21 1000000#128#1000#255#0#InnerProduct#NO_COPY#SEARCH KNN/float/int64_t/brute_force_filter_knn/1/0/0/manual_time 38.0 ms 74.8 ms 18 1000000#128#1000#255#0#L2Expanded#NO_COPY#SEARCH KNN/float/int64_t/brute_force_filter_knn/2/0/0/manual_time 41.7 ms 78.5 ms 17 1000000#128#1000#255#0.8#InnerProduct#NO_COPY#SEARCH KNN/float/int64_t/brute_force_filter_knn/3/0/0/manual_time 57.5 ms 94.3 ms 12 1000000#128#1000#255#0.8#L2Expanded#NO_COPY#SEARCH KNN/float/int64_t/brute_force_filter_knn/4/0/0/manual_time 19.7 ms 56.4 ms 35 1000000#128#1000#255#0.9#InnerProduct#NO_COPY#SEARCH KNN/float/int64_t/brute_force_filter_knn/5/0/0/manual_time 26.1 ms 62.8 ms 27 1000000#128#1000#255#0.9#L2Expanded#NO_COPY#SEARCH``` Authors: - rhdong (https://github.com/rhdong) - Artem M. Chirkin (https://github.com/achirkin) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Robert Maynard (https://github.com/robertmaynard) - Corey J. Nolet (https://github.com/cjnolet) - Divye Gala (https://github.com/divyegala) URL: #2294
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
❄️ Code freeze for
branch-24.06and v24.06 releaseWhat does this mean?
Only critical/hotfix level issues should be merged into
branch-24.06until release (merging of this PR).What is the purpose of this PR?
branch-24.06intomainfor the release