ci: add CPU-only C++ build coverage - #119
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. |
f9cf095 to
6ddd13d
Compare
|
/ok to test 5a76c2d |
|
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
WalkthroughAdds a CI job that builds C++ with NVFOREST_ENABLE_GPU=OFF (gated to C++ changes) and surfaces NVFOREST_EXTRA_CMAKE_ARGS to the conda recipe; updates source/tests for CPU-only builds (owned exception messages, conditional CUDA usage, Treelite importer runtime validation, and raft_proto test handle adaptation). ChangesCPU-only C++ build CI coverage and supporting fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 14-15: The public header unconditionally includes
<cuda_runtime_api.h>, forcing CUDA as a dependency; wrap that include in an
NVFOREST_ENABLE_GPU guard (as done in cuda_stream.hpp) so the header only pulls
in CUDA when GPU support is enabled, and leave the existing conditional usage of
cudaGetDevice() (e.g., where used in functions/methods referencing cudaGetDevice
or GPU memory types around line ~294) unchanged; ensure the guard uses `#ifdef`
NVFOREST_ENABLE_GPU / `#endif` around the include to allow CPU-only consumers to
build without CUDA headers.
In `@cpp/include/nvforest/treelite_importer.hpp`:
- Around line 374-377: The code uses tl_model.num_class[0] in a modulo without
validating it; validate the model's class info before the grove-per-class loop
by confirming tl_model.num_class has at least one element and that
tl_model.num_class[0] > 0, and if not throw a model_import_error with a clear
message; do this check prior to calling num_trees/entering the for-loop (or
immediately before the modulo in the loop) so tree_id % tl_model.num_class[0]
cannot cause division-by-zero or UB when processing trees in num_trees, and
reference the symbols num_trees, tl_model, num_class, tree_id, and
model_import_error when locating the insertion point.
🪄 Autofix (Beta)
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: 8fa75113-7012-4038-8fab-da37136da3d2
📒 Files selected for processing (6)
.github/workflows/pr.yamlconda/recipes/libnvforest/recipe.yamlcpp/include/nvforest/exceptions.hppcpp/include/nvforest/forest_model.hppcpp/include/nvforest/treelite_importer.hppcpp/tests/treelite_importer.cpp
|
/merge |
Adds a PR CI job that builds libnvforest with
NVFOREST_ENABLE_GPU=OFF, reusing the existing conda C++ build path by passingNVFOREST_EXTRA_CMAKE_ARGSthrough the libnvforest recipe.Closes #118