Remove RMM and RAFT dependency - #193
Draft
chyunsu3 wants to merge 14 commits into
Draft
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⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request removes RAFT and RMM integration. C++ prediction now accepts CUDA streams and uses CUDA-backed device buffers. Python inference accepts stream-like CUDA objects. Packaging and environment definitions replace legacy dependencies with ChangesDependency and packaging migration
C++ CUDA stream backend
Python CUDA stream API
Estimated code review effort: 4 (Complex) | ~60 minutes 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
| precision: Optional[str] = None, | ||
| device_id: Optional[int] = None, | ||
| handle: Optional[Handle] = None, | ||
| stream: Optional[Stream] = None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve a migration path for the removed handle keyword.
Existing callers that pass handle= now fail before inference starts. Keep a deprecated compatibility adapter for one release cycle, or make this removal part of a documented breaking API release.
python/nvforest/nvforest/_factory.py#L75-L75: retain or deprecate thehandlekeyword forload_model.python/nvforest/nvforest/_factory.py#L177-L177: retain or deprecate thehandlekeyword forload_from_sklearn.python/nvforest/nvforest/_factory.py#L239-L239: retain or deprecate thehandlekeyword forload_from_treelite_model.python/nvforest/nvforest/_forest_inference.py#L263-L263: provide a migration path forCPUForestInferenceClassifier.python/nvforest/nvforest/_forest_inference.py#L400-L400: provide a migration path forCPUForestInferenceRegressor.python/nvforest/nvforest/_forest_inference.py#L529-L529: provide a migration path forGPUForestInferenceClassifier.python/nvforest/nvforest/_forest_inference.py#L668-L668: provide a migration path forGPUForestInferenceRegressor.
📍 Affects 2 files
python/nvforest/nvforest/_factory.py#L75-L75(this comment)python/nvforest/nvforest/_factory.py#L177-L177python/nvforest/nvforest/_factory.py#L239-L239python/nvforest/nvforest/_forest_inference.py#L263-L263python/nvforest/nvforest/_forest_inference.py#L400-L400python/nvforest/nvforest/_forest_inference.py#L529-L529python/nvforest/nvforest/_forest_inference.py#L668-L668
🤖 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/nvforest/_factory.py` at 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.
Source: Coding guidelines
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
Comment on lines
+252
to
+253
| cudaStream_t stream{}; | ||
| cudaStreamCreate(&stream); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- getting_started.rst relevant sections ---'
sed -n '220,275p' docs/source/getting_started.rst
printf '%s\n' '--- README relevant sections ---'
sed -n '95,150p' cpp/include/nvforest/README.md
printf '%s\n' '--- device and stream usage in both files ---'
rg -n -C 3 'cuda(SetDevice|StreamCreate|StreamDestroy)|device_id|import|inference|stream' \
docs/source/getting_started.rst cpp/include/nvforest/README.mdRepository: rapidsai/nvforest
Length of output: 21392
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- importer and stream implementation references ---'
rg -n -C 4 'import_from_treelite_model|cudaSetDevice|cudaStreamCreate|cudaStreamGetDevice|device_id' \
cpp python tests docs | head -n 500
printf '%s\n' '--- public declarations and implementation candidates ---'
rg -l 'import_from_treelite_model|cudaStreamCreate' cpp | head -n 80Repository: rapidsai/nvforest
Length of output: 38759
🌐 Web query:
CUDA Runtime API cudaStreamCreate current device stream associated device official documentation
💡 Result:
According to the official CUDA Runtime API documentation, the cudaStreamCreate function creates a new asynchronous stream on the context that is currently bound to the calling host thread [1][2]. If no context is currently bound to the calling host thread at the time of the call, the runtime selects the primary context for the device, makes it current to the calling thread, and initializes it before creating the stream on that context [1][3]. Because CUDA streams are inherently bound to a specific context, and contexts are associated with a specific device, the created stream becomes associated with the device corresponding to that context [4]. To determine the device associated with a stream, you can use the cudaStreamGetDevice function, which retrieves the device on which the specified stream is scheduled to run [5].
Citations:
- 1: https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__STREAM.html
- 2: https://docs.nvidia.com/cuda/archive/13.0.2/cuda-runtime-api/group__CUDART__STREAM.html
- 3: https://docs.nvidia.com/cuda/archive/12.6.3/cuda-runtime-api/group__CUDART__STREAM.html
- 4: https://stackoverflow.com/questions/31474784/are-cuda-streams-device-associated-and-how-do-i-get-a-streams-device
- 5: https://docs.nvidia.com/cuda/archive/13.2.1/pdf/CUDA_Runtime_API.pdf
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- treelite importer implementation ---'
sed -n '430,535p' cpp/include/nvforest/treelite_importer.hpp
printf '%s\n' '--- device setter implementation and uses ---'
rg -n -C 5 'struct device_setter|class device_setter|device_setter\{|device_context|set_current|cudaSetDevice' \
cpp/include cpp/src | head -n 400
printf '%s\n' '--- README includes and complete import example ---'
sed -n '1,85p' cpp/include/nvforest/README.md
printf '%s\n' '--- importer tests covering device and stream behavior ---'
rg -n -C 5 'device_id|stream|cudaStream' cpp/tests/treelite_importer.cpp cpp/tests/treelite_importer_invalid_inputs.cppRepository: rapidsai/nvforest
Length of output: 16491
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- importer call path around GPU setup ---'
sed -n '250,440p' cpp/include/nvforest/treelite_importer.hpp
printf '%s\n' '--- read-only documentation invariant check ---'
python3 - <<'PY'
from pathlib import Path
getting_started = Path("docs/source/getting_started.rst").read_text()
readme = Path("cpp/include/nvforest/README.md").read_text()
for name, text, stream_marker, device_marker in [
("docs/source/getting_started.rst", getting_started, "cudaStreamCreate(&stream);", "device_id"),
("cpp/include/nvforest/README.md", readme, "checkCuda(cudaStreamCreate(&stream));", "0, // device_id"),
]:
stream_pos = text.index(stream_marker)
window = text[max(0, stream_pos - 300):stream_pos + 300]
print(name)
print("stream_creation_position:", stream_pos)
print("device_selection_before_stream:", "cudaSetDevice" in window or "set_current" in window)
print("device_id_context:", device_marker in window or device_marker in text[stream_pos:stream_pos + 600])
PYRepository: rapidsai/nvforest
Length of output: 10519
Bind the CUDA stream to device_id before creation.
When device_id is 0, call cudaSetDevice(0) before cudaStreamCreate(&stream) in both C++ examples. Otherwise, a caller whose current device is not 0 creates a stream for the wrong device and passes it to the model importer.
📍 Affects 2 files
docs/source/getting_started.rst#L252-L253(this comment)cpp/include/nvforest/README.md#L129-L135
🤖 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 `@docs/source/getting_started.rst` around lines 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.
Sources: Path instructions, MCP tools
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
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
Closes #168
raft::handle_tandnvforest::handle_tand use CUDA streams directlycuda::bufferinstead ofrmm::device_buffer. Sincecuda::bufferrequires NVCC to build, we need to use PIMPL with type erasure.