Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: rapidsai/nvforest/.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)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe documentation now includes an nvForest announcement blog post, usage guidance, optimization details, benchmark results, migration guidance, and updated Sphinx navigation. ChangesnvForest documentation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to The documentation integration is correctly connected and has no actionable merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
csadorf
left a comment
There was a problem hiding this comment.
LGTM. Thanks for putting together this initial post and the blog infrastructure. I left three minor comments to clarify the FIL history, migration timeline, and benchmark results.
| We are proud to announce nvForest, a lightweight library for fast inference | ||
| for tree-based models on NVIDIA GPUs. nvForest builds on our past work on the | ||
| `Forest Inference Library (FIL) <https://developer.nvidia.com/blog/supercharge-tree-based-model-inference-with-forest-inference-library-in-nvidia-cuml/>`_, | ||
| which has lived inside cuML until now. With the release of nvForest, this |
There was a problem hiding this comment.
I'd rather say something like ‘which was previously part of cuML’ or ‘which was previously maintained and distributed in cuML.’
| If you are using FIL today through ``cuml.fil.ForestInference``, you do not | ||
| need to rewrite your code immediately. Going forward, | ||
| ``cuml.fil.ForestInference`` will become a lightweight shim that uses | ||
| nvForest underneath and emits a deprecation warning pointing you to nvForest. |
There was a problem hiding this comment.
I think this needs to lead with the need to migrate. As written, it initially sounds as though users do not need to act and that we will maintain the shim indefinitely; the 26.10 deadline in the next sentence is relatively soon.
There was a problem hiding this comment.
+1. cuml.fil.ForestInference is no longer available in cuML 26.10.
|
|
||
| Speedup factor by tree count. | ||
|
|
||
| A single NVIDIA H100 (80GB HBM3) was used for GPU results, and a 2-socket |
There was a problem hiding this comment.
Are the figures showing only GPU results? If so, could we make that explicit? I assume the CPU machine here refers to the RandomForestRegressor.predict baseline comparison, but it is not clear from the surrounding text.
| cuML, scikit-learn, XGBoost, LightGBM, and any other Treelite-compatible | ||
| model. It provides state-of-the-art performance on tree inference, a small |
There was a problem hiding this comment.
Should we even mention "Treelite" here?
| If you are using FIL today through ``cuml.fil.ForestInference``, you do not | ||
| need to rewrite your code immediately. Going forward, | ||
| ``cuml.fil.ForestInference`` will become a lightweight shim that uses | ||
| nvForest underneath and emits a deprecation warning pointing you to nvForest. |
There was a problem hiding this comment.
+1. cuml.fil.ForestInference is no longer available in cuML 26.10.
Adds new blog tab to the nvForest docs and includes draft of initial blog post.