Repository navigation
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
chyunsu3
marked this pull request as ready for review
August 12, 2026 07:25
chyunsu3
added
breaking
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Mergeability Score: 🔵 Low · up to The public constructor now transfers buffer ownership, but its documentation does not describe that contract, creating a bounded risk of incorrect caller lifetime management. The PR is mergeable with explicit owner follow-up to document the behavior. Pre-merge checks:
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 29.55% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The changes remove RMM and RAFT from build, runtime, CMake, C++, Python, tests, and documentation while using CUDA streams and device buffers. Out of Scope Changes check The changes are aligned with dependency removal and its required API, implementation, test, packaging, and documentation updates. Title check The title clearly summarizes the primary dependency changes and the transition toward removing RAFT. It is concise and related to the changeset. Description check The description addresses the Conda recipes, Python documentation, and CI changes in the pull request. It also references the RAFT and RMM dependency updates, although its note about retaining librmm … Full details: Description check
-
Explanation
The description addresses the Conda recipes, Python documentation, and CI changes in the pull request. It also references the RAFT and RMM dependency updates, although its note about retaining librmm conflicts with the changeset.
-
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
-
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
python/nvforest/tests/test_nvforest.py (1)
893-920: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for invalid stream inputs.
Test
stream=object()and assert theTypeError. If two GPUs are available, create a stream on the other device and assert the documentedValueError. This covers the new validation inForestInferenceImpl.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/nvforest/tests/test_nvforest.py` around lines 893 - 920, Add invalid stream coverage to test_cuda_core_stream: pass stream=object() to nvforest.load_from_sklearn and assert TypeError, then when at least two GPUs are available create a stream on the other device and assert the documented ValueError from ForestInferenceImpl validation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/nvforest/forest_model.hpp`:
- Around line 153-170: Update the allocating buffer constructors in the predict
path around concrete_forest.predict to pass device_index() and stream for both
buffer_in and buffer_out, rather than default device and stream values. Preserve
the existing memory-type branching and copy behavior, and add GPU coverage
exercising a non-default stream with a model on a nonzero device.
In `@python/nvforest/nvforest/_base.py`:
- Line 37: Update the documentation statements associated with the abstract
input/copy methods in the base model API to qualify CUDA-stream copying as
GPU-only behavior. Clarify that CPU execution ignores stream and does not use a
model CUDA stream, applying the same wording consistently at all referenced
occurrences.
In `@python/nvforest/nvforest/_factory.py`:
- Line 75: Preserve a one-release migration path for the removed handle keyword
by retaining it as a deprecated compatibility parameter and forwarding or
adapting it consistently. Update load_model, load_from_sklearn, and
load_from_treelite_model in python/nvforest/nvforest/_factory.py at lines 75-75,
177-177, and 239-239, plus CPUForestInferenceClassifier at 263-263,
CPUForestInferenceRegressor at 400-400, GPUForestInferenceClassifier at 529-529,
and GPUForestInferenceRegressor at 668-668 in
python/nvforest/nvforest/_forest_inference.py; emit the established deprecation
warning and preserve current behavior for callers using the replacement API.
---
Nitpick comments:
In `@python/nvforest/tests/test_nvforest.py`:
- Around line 893-920: Add invalid stream coverage to test_cuda_core_stream:
pass stream=object() to nvforest.load_from_sklearn and assert TypeError, then
when at least two GPUs are available create a stream on the other device and
assert the documented ValueError from ForestInferenceImpl validation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a55058ce-9a20-45d4-9c93-f01284338d54
📒 Files selected for processing (38)
ci/build_wheel_libnvforest.shci/build_wheel_nvforest.shci/release/update-version.shconda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-133_arch-aarch64.yamlconda/environments/all_cuda-133_arch-x86_64.yamlconda/environments/clang_tidy_cuda-129_arch-x86_64.yamlconda/environments/clang_tidy_cuda-133_arch-x86_64.yamlconda/environments/cpp_all_cuda-129_arch-x86_64.yamlconda/environments/cpp_all_cuda-133_arch-x86_64.yamlconda/recipes/libnvforest/recipe.yamlconda/recipes/nvforest/recipe.yamlcpp/CMakeLists.txtcpp/cmake/thirdparty/get_raft.cmakecpp/cmake/thirdparty/get_rmm.cmakecpp/include/nvforest/detail/device_id/gpu.hppcpp/include/nvforest/detail/device_setter/gpu.hppcpp/include/nvforest/detail/owning_buffer/gpu.hppcpp/include/nvforest/forest_model.hppcpp/include/nvforest/handle.hppcpp/include/nvforest/treelite_importer.hppcpp/src/detail/device_buffer.cucpp/tests/CMakeLists.txtcpp/tests/treelite_importer.cppdependencies.yamlpython/libnvforest/libnvforest/load.pypython/libnvforest/pyproject.tomlpython/nvforest/nvforest/__init__.pypython/nvforest/nvforest/_base.pypython/nvforest/nvforest/_factory.pypython/nvforest/nvforest/_forest_inference.pypython/nvforest/nvforest/_handle.pypython/nvforest/nvforest/detail/cuda_stream.pxdpython/nvforest/nvforest/detail/forest_inference.pyxpython/nvforest/nvforest/detail/handle.pxdpython/nvforest/pyproject.tomlpython/nvforest/tests/test_nvforest.py
💤 Files with no reviewable changes (16)
- python/libnvforest/libnvforest/load.py
- python/nvforest/nvforest/detail/handle.pxd
- conda/environments/cpp_all_cuda-129_arch-x86_64.yaml
- cpp/cmake/thirdparty/get_rmm.cmake
- conda/environments/cpp_all_cuda-133_arch-x86_64.yaml
- conda/environments/clang_tidy_cuda-133_arch-x86_64.yaml
- ci/release/update-version.sh
- cpp/include/nvforest/treelite_importer.hpp
- cpp/include/nvforest/handle.hpp
- python/libnvforest/pyproject.toml
- conda/environments/clang_tidy_cuda-129_arch-x86_64.yaml
- cpp/cmake/thirdparty/get_raft.cmake
- cpp/tests/CMakeLists.txt
- python/nvforest/nvforest/_handle.py
- ci/build_wheel_nvforest.sh
- conda/recipes/libnvforest/recipe.yaml
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/nvforest/README.md`:
- Around line 108-111: Add cudaStreamSynchronize(stream) immediately after the
predict call in the README example and before reading or freeing output,
ensuring asynchronous GPU work completes before output access or cleanup.
In `@docs/source/build.rst`:
- Around line 24-31: Update the “Relation to RAPIDS” section to clarify that
standalone C++ builds fetch and link Treelite and rapids_logger through CPM,
rather than stating CCCL is the only dependency. Revise the Python dependencies
section to list the package names cuda-bindings, cuda-core, cupy-cuda13x,
libnvforest, NumPy, scikit-learn, and Treelite.
In `@docs/source/getting_started.rst`:
- Around line 252-253: In both C++ examples, update the stream setup before
cudaStreamCreate(&stream) to call cudaSetDevice(0), ensuring the stream is
created on device_id 0 before it is passed to the model importer. Apply this
change at docs/source/getting_started.rst lines 252-253 and
cpp/include/nvforest/README.md lines 129-135.
In `@docs/source/python_api.rst`:
- Around line 8-11: Update the GPU stream documentation near the
loading-function guidance to state that when device_id=None, nvForest resolves
the current CUDA device rather than deriving it from stream.device. Clarify that
supplied streams must match this resolved device and that a mismatch raises
ValueError.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ccef133-470f-4d7a-8123-508f6ce3f7ab
📒 Files selected for processing (8)
README.mdcpp/include/nvforest/Implementation.mdcpp/include/nvforest/README.mddocs/source/build.rstdocs/source/conf.pydocs/source/cpp_api.rstdocs/source/getting_started.rstdocs/source/python_api.rst
chyunsu3
marked this pull request as draft
August 12, 2026 08:07
Contributor
Author
|
I might want to break up the PR to make it easier to review. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/getting_started.rst`:
- Around line 277-283: Update the example around fm and stream so the model is
destroyed before the caller-owned stream: enclose fm’s lifetime in a scope,
ensure queued work has completed, then call cudaStreamDestroy(stream) after that
scope.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fd62c401-4999-42fc-b581-0b50f28fa454
📒 Files selected for processing (46)
README.mdci/build_wheel_libnvforest.shci/build_wheel_nvforest.shci/release/update-version.shconda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-133_arch-aarch64.yamlconda/environments/all_cuda-133_arch-x86_64.yamlconda/environments/clang_tidy_cuda-129_arch-x86_64.yamlconda/environments/clang_tidy_cuda-133_arch-x86_64.yamlconda/environments/cpp_all_cuda-129_arch-x86_64.yamlconda/environments/cpp_all_cuda-133_arch-x86_64.yamlconda/recipes/libnvforest/recipe.yamlconda/recipes/nvforest/recipe.yamlcpp/CMakeLists.txtcpp/cmake/thirdparty/get_raft.cmakecpp/cmake/thirdparty/get_rmm.cmakecpp/include/nvforest/Implementation.mdcpp/include/nvforest/README.mdcpp/include/nvforest/detail/device_id/gpu.hppcpp/include/nvforest/detail/device_setter/gpu.hppcpp/include/nvforest/detail/owning_buffer/gpu.hppcpp/include/nvforest/forest_model.hppcpp/include/nvforest/handle.hppcpp/include/nvforest/treelite_importer.hppcpp/src/detail/device_buffer.cucpp/tests/CMakeLists.txtcpp/tests/treelite_importer.cppdependencies.yamldocs/source/build.rstdocs/source/conf.pydocs/source/cpp_api.rstdocs/source/getting_started.rstdocs/source/python_api.rstpython/libnvforest/libnvforest/load.pypython/libnvforest/pyproject.tomlpython/nvforest/nvforest/__init__.pypython/nvforest/nvforest/_base.pypython/nvforest/nvforest/_factory.pypython/nvforest/nvforest/_forest_inference.pypython/nvforest/nvforest/_handle.pypython/nvforest/nvforest/detail/cuda_stream.pxdpython/nvforest/nvforest/detail/forest_inference.pyxpython/nvforest/nvforest/detail/handle.pxdpython/nvforest/pyproject.tomlpython/nvforest/tests/test_nvforest.py
💤 Files with no reviewable changes (16)
- cpp/include/nvforest/treelite_importer.hpp
- cpp/cmake/thirdparty/get_rmm.cmake
- cpp/cmake/thirdparty/get_raft.cmake
- cpp/include/nvforest/handle.hpp
- cpp/tests/CMakeLists.txt
- conda/environments/clang_tidy_cuda-129_arch-x86_64.yaml
- python/nvforest/nvforest/_handle.py
- ci/release/update-version.sh
- python/nvforest/nvforest/detail/handle.pxd
- python/libnvforest/pyproject.toml
- conda/environments/clang_tidy_cuda-133_arch-x86_64.yaml
- python/libnvforest/libnvforest/load.py
- conda/environments/cpp_all_cuda-133_arch-x86_64.yaml
- ci/build_wheel_nvforest.sh
- conda/recipes/libnvforest/recipe.yaml
- conda/environments/cpp_all_cuda-129_arch-x86_64.yaml
🚧 Files skipped from review as they are similar to previous changes (29)
- docs/source/conf.py
- python/nvforest/tests/test_nvforest.py
- conda/recipes/nvforest/recipe.yaml
- conda/environments/all_cuda-133_arch-x86_64.yaml
- README.md
- docs/source/cpp_api.rst
- docs/source/python_api.rst
- docs/source/build.rst
- cpp/tests/treelite_importer.cpp
- cpp/include/nvforest/Implementation.md
- python/nvforest/nvforest/detail/cuda_stream.pxd
- conda/environments/all_cuda-129_arch-aarch64.yaml
- cpp/CMakeLists.txt
- conda/environments/all_cuda-129_arch-x86_64.yaml
- python/nvforest/nvforest/init.py
- conda/environments/all_cuda-133_arch-aarch64.yaml
- cpp/src/detail/device_buffer.cu
- python/nvforest/nvforest/detail/forest_inference.pyx
- cpp/include/nvforest/forest_model.hpp
- ci/build_wheel_libnvforest.sh
- cpp/include/nvforest/README.md
- cpp/include/nvforest/detail/device_setter/gpu.hpp
- cpp/include/nvforest/detail/device_id/gpu.hpp
- cpp/include/nvforest/detail/owning_buffer/gpu.hpp
- python/nvforest/pyproject.toml
- python/nvforest/nvforest/_factory.py
- dependencies.yaml
- python/nvforest/nvforest/_forest_inference.py
- python/nvforest/nvforest/_base.py
bdice
reviewed
Aug 12, 2026
| struct owning_device_buffer_type_erased_impl { | ||
| owning_device_buffer_type_erased_impl(int device_id, std::size_t size, cudaStream_t stream) | ||
| : buffer_{[&stream, device_id, size]() { | ||
| auto device = cuda::device_ref{device_id}; |
Contributor
There was a problem hiding this comment.
Can you use cuda::device_ref as the argument instead of converting an int? It would be more strongly typed that way. Or does that cause problems for CPU builds?
Contributor
Author
There was a problem hiding this comment.
Using cuda::device_ref in the argument would indeed cause issues for CPU builds, since the same signature would need to be visible in the CPU and GPU builds.
Minimal reproducer
#include <cuda/devices>
int main() {
cuda::device_ref device{0};
return 0;
}cmake_minimum_required(VERSION 4.0 FATAL_ERROR)
project(foobar LANGUAGES CXX VERSION 0.0.1)
find_package(CCCL CONFIG REQUIRED)
add_executable(foobar main.cc)
target_link_libraries(foobar PRIVATE CCCL::CCCL ${CMAKE_DL_LIBS})Error:
/home/phcho/miniforge3/envs/nvforest_dev/bin/x86_64-conda-linux-gnu-ld: CMakeFiles/foobar.dir/main.cc.o: in function
cuda::__4::__throw_cuda_error(cudaError, char const*, char const*, cuda::std::__4::source_location)': main.cc:(.text.unlikely._ZN4cuda3__418__throw_cuda_errorE9cudaErrorPKcS3_NS_3std3__415source_locationE[_ZN4cuda3__418__throw_cuda_errorE9cudaErrorPKcS3_NS_3std3__415source_locationE]+0x64): undefined reference tocudaGetErrorString'
collect2: error: ld returned 1 exit status
chyunsu3
marked this pull request as ready for review
August 13, 2026 01:00
Contributor
Author
|
Temporarily un-drafting this PR to run the CI. I will put it back to draft once the CI finishes running. As this PR has grown quite large, I will open smaller PRs to make it easier to review. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/nvforest/decision_forest.hpp`:
- Around line 165-170: Update the decision_forest constructor’s Doxygen
documentation to explicitly state that all rvalue buffer arguments are
move-constructed and transfer ownership, and that callers must not rely on the
source buffers after construction. Anchor the documentation to the constructor
corresponding to nodes_, root_node_indexes_, node_id_mapping_, bias_,
vector_output_, and categorical_storage_.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b44ced48-905c-4dc0-be60-c17c3a6b3ca9
📒 Files selected for processing (13)
README.mdcpp/include/nvforest/decision_forest.hppcpp/include/nvforest/forest_model.hppdocs/source/build.rstdocs/source/getting_started.rstdocs/source/python_api.rstpython/nvforest/nvforest/__init__.pypython/nvforest/nvforest/_factory.pypython/nvforest/nvforest/_forest_inference.pypython/nvforest/nvforest/_typing.pypython/nvforest/nvforest/detail/cuda_stream.pxdpython/nvforest/nvforest/detail/forest_inference.pyxpython/nvforest/tests/test_nvforest.py
💤 Files with no reviewable changes (1)
- docs/source/python_api.rst
🚧 Files skipped from review as they are similar to previous changes (7)
- python/nvforest/nvforest/init.py
- README.md
- python/nvforest/nvforest/detail/forest_inference.pyx
- docs/source/getting_started.rst
- cpp/include/nvforest/forest_model.hpp
- python/nvforest/nvforest/_forest_inference.py
- python/nvforest/nvforest/_factory.py
Contributor
Author
chyunsu3
added a commit
that referenced
this pull request
Aug 13, 2026
…ctor (#196) Currently, buffer arguments to `decision_forest` are passed to member variables without `std::move`, triggering the copy constructor. This behavior is not intended, since the arguments are all rvalues (`&&`). The extra buffer copies are problematic because the copy constructor uses the default stream. If the initial host-to-device copy occurs on a non-default stream, then the copy construction is unordered with respect to the host-to-device copy (i.e. may occur before or after). So the forest model may end up with garbage value. Fix. Ensure that the buffer arguments are moved into the `decision_forest`. This way, we do not make extra copies after the initial host-to-device copy. I found this bug while working on #193, since it was using a non-default stream.
rapids-bot Bot
pushed a commit
that referenced
this pull request
Sep 8, 2026
Extracted from #193 ~~Requires #196 for the CI to pass.~~ - [x] Remove RAFT and RMM from CMake config - [x] Remove the use of `raft::handle_t` and `nvforest::handle_t` and use CUDA streams directly - [x] Use `cuda::buffer` instead of `rmm::device_buffer`. Since `cuda::buffer` requires NVCC to build, we need to use PIMPL with type erasure. - [x] Fix all gtests - [x] Update the Python layer - [x] Add test coverage for using stream with wrong device. Authors: - Philip Hyunsu Cho (https://github.com/chyunsu3) Approvers: - Bradley Dice (https://github.com/bdice) - Simon Adorf (https://github.com/csadorf) URL: #195
rapids-bot Bot
pushed a commit
that referenced
this pull request
Oct 19, 2026
**Companion PR: NVIDIA/cuml#8601. To minimize the disruption in the cuML CI, we should merge this PR together with NVIDIA/cuml#8601 . * Remove the use of `raft::handle_t` and `nvforest::handle_t` and use CUDA streams directly. * Create `predict()` overloads that takes in either a single CUDA stream or a pool of multiple CUDA streams. * Use `cuda::buffer` instead of `rmm::device_buffer`. Since `cuda::buffer` requires NVCC to build, we need to use PIMPL with type erasure. * Update all C++ tests. * Update the Python layer. The `handle` parameter is now deprecated and its use will trigger a `FutureWarning`. Users are directed to use `stream` instead. * Remove `cpp/cmake/thirdparty/get_rmm.cmake`. For now, we retain `cpp/cmake/thirdparty/get_raft.cmake`, since we need to wait one release cycle until we can remove the deprecated `handle` parameter. * Add test coverage for using stream with wrong device. After this PR, #193 will update `dependencies.yaml` to remove the RMM dependency and update the docs. History * This PR is part of #193 * This is a re-submission of #195 (which was previously reverted to unblock the cuML CI). * #195 originally targeted 26.10, whereas this PR targets 26.12. Authors: - Philip Hyunsu Cho (https://github.com/chyunsu3) - Simon Adorf (https://github.com/csadorf) Approvers: - Bradley Dice (https://github.com/bdice) - Simon Adorf (https://github.com/csadorf) URL: #219
chyunsu3
marked this pull request as ready for review
October 20, 2026 02:45
chyunsu3
changed the title
chyunsu3
force-pushed
the
remove_rmm_raft
branch
from
October 20, 2026 03:05
37ef00f to
527e158
Compare
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
3 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.
Closes #169
Follow-up to #219
Update Conda recipes, Python docstrings, and CI scripts to be consistent with the changed introduced in #219.
Note: We are keeping
librmmdependency for now, since it's required by RAFT. We can remove it once we remove RAFT dependency fully.