Add get_feature_names#4
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 1 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdded runtime dependency Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (2)
src/model.rs (2)
325-337: Guard assumptions when mapping feature indices to namesThe pattern of:
- calling
get_feature_names()to getall_names, then- calling
get_feature_indices(...)and indexingall_names[i]is clean, but it rests entirely on the C API guaranteeing that:
- every index returned by
indices_fnis< all_names.len(), and- the index space used by
Get*FeatureIndicesmatches that ofGetModelUsedFeaturesNames.If that contract is ever violated (version skew, bug, or undocumented change),
all_names[i]will panic or read garbage.Suggested follow‑ups:
- Add a short debug‑only sanity check before indexing, e.g.
debug_assert!(i < all_names.len());.- Document in the
get_specific_feature_namesdoc comment that it assumes indices are in bounds for the array returned byGetModelUsedFeaturesNames.The rest of the FFI wrapper (
indices_fnsignature,indices_ptr/counthandling) looks consistent with the surrounding code.Also applies to: 356-371
373-403: Type‑specific feature name accessors are clean; consider version gating for text/embedding APIsThe four type‑specific getters:
get_float_feature_namesget_cat_feature_namesget_text_feature_namesget_embedding_feature_namesnicely reuse
get_specific_feature_namesand the correspondingsys::Get*FeatureIndicesfunctions, keeping the surface API small and consistent.Two non‑blocking concerns to keep in mind:
Version/feature availability
GetTextFeatureIndicesandGetEmbeddingFeatureIndicesmay not exist in older CatBoost binaries, while this Rust code calls them unconditionally (unlike theget_*_features_countmethods, which are behind#[cfg(...)]flags).- If you intend to support multiple CatBoost versions, you may want to mirror the
#[cfg(catboost_text_count)]/#[cfg(catboost_embeddings)]gating here, or clearly document that this crate now targets a minimum CatBoost version where these functions are guaranteed to exist.Failure mode if indices retrieval fails
- Right now, any failure in the underlying
Get*FeatureIndicesorGetModelUsedFeaturesNameswill bubble up viaCatBoostError, which is appropriate, but it might be worth mentioning in the Rustdoc that these functions can error if the underlying C API doesn’t support them or returns an error.Overall, the API design is good; I’d just align the version gating/documentation with how you already handle text/embedding feature counts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cargo.toml(1 hunks)src/model.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/model.rs (1)
src/error.rs (1)
check_return_value(15-21)
🔇 Additional comments (2)
Cargo.toml (1)
13-15: libc dependency addition looks appropriate; just confirm version and usage scopeAdding
libcas a runtime dependency to get access tofreeis a standard pattern for FFI with a C library and fits the new helpers inmodel.rs. Please just double‑check that:
- this exact
0.2.177version aligns with your intended MSRV and other deps, and- you don’t need to gate it behind any feature flags (e.g., if you later introduce a
no-ffior similar mode).src/model.rs (1)
339-354:get_feature_namesFFI pattern is sound; ensure CatBoost version compatibilityThe implementation of
get_feature_names:
- correctly initializes
names_ptr/count,- uses
GetModelUsedFeaturesNamesonce, checks the boolean return viaCatBoostError::check_return_value, and- delegates pointer conversion and freeing to
get_feature_names_from_c.This is a solid, minimal wrapper.
One thing to watch:
GetModelUsedFeaturesNamesis only available in newer versions of the CatBoost evaluation library. If your build script still supports older CatBoost binaries, this symbol (and possibly the corresponding header declaration) might be missing, causing compilation or link failures.It would be good to:
- Explicitly state in the crate docs or README that these APIs require a sufficiently new CatBoost evaluation library, or
- Gate them behind an appropriate Cargo feature /
#[cfg(...)]tied to the CatBoost version you download inbuild.rs.
| fn get_feature_indices_from_c( | ||
| indices_ptr: *mut usize, | ||
| count: usize, | ||
| err_msg: &str, | ||
| ) -> CatBoostResult<Vec<usize>> { | ||
| if indices_ptr.is_null() { | ||
| if count == 0 { | ||
| return Ok(Vec::new()); | ||
| } | ||
| return Err(CatBoostError { | ||
| description: err_msg.to_owned(), | ||
| }); | ||
| } | ||
| let mut indices = Vec::with_capacity(count); | ||
| for i in 0..count { | ||
| indices.push(unsafe { *indices_ptr.add(i) }); | ||
| } | ||
| unsafe { libc::free(indices_ptr as *mut _) }; | ||
| Ok(indices) | ||
| } |
There was a problem hiding this comment.
Add documentation confirming the C API memory ownership contract
The get_feature_indices_from_c and get_feature_names_from_c functions both use libc::free() to deallocate pointers returned from the CatBoost C API, which requires verifying and documenting the memory ownership contract:
- Confirm in CatBoost C API documentation or comments that these functions allocate with
malloc-compatible allocators and expect callers to deallocate withfree - Add a SAFETY comment in each helper function explaining this assumption
- Consider centralizing the pattern in a single unsafe wrapper (e.g.,
from_c_allocated_buffer<T>) to reduce duplication and make the contract explicit
Without documented verification of the allocator compatibility, this represents a potential source of undefined behavior (heap corruption/use-after-free), particularly if CatBoost or its dependencies use custom allocators or library-specific deallocators.
🤖 Prompt for AI Agents
In src/model.rs around lines 278 to 297, both get_feature_indices_from_c and
get_feature_names_from_c call libc::free on pointers returned by the CatBoost C
API without documenting the allocator contract; update the code by (1) adding a
SAFETY comment above each helper stating that the CatBoost C API must return
malloc-compatible buffers that the caller must free with libc::free (cite the
exact CatBoost C API doc or source line that confirms this), (2) centralize the
pattern by implementing a single unsafe helper (e.g.,
from_c_allocated_buffer<T>) that reads count elements, frees the pointer with
libc::free, and returns a Vec<T>, and (3) replace the existing functions to call
that helper so the ownership contract and unsafe boundary are explicit and
documented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/model.rs (4)
278-303: Excellent safety documentation—past concerns addressed.The comprehensive safety documentation and centralized memory management pattern directly addresses the previous review comment about documenting the C API memory ownership contract. The implementation correctly copies data before freeing and handles null pointers appropriately.
Optional: Consider using
std::slice::from_raw_partsfor a more idiomatic conversion:unsafe fn from_c_allocated_buffer<T: Copy>(ptr: *mut T, count: usize) -> Vec<T> { if ptr.is_null() { return Vec::new(); } let result = unsafe { std::slice::from_raw_parts(ptr, count).to_vec() }; unsafe { libc::free(ptr as *mut _) }; result }
356-380: Consider adding feature gate to prevent dead code.This helper function is only called by feature-gated public APIs and itself calls
get_feature_names()which is also feature-gated. Adding the same gate here would prevent dead code warnings when the feature is disabled.Apply this diff:
/// Get names of specific type of features used in model, /// returns error if index out of bounds + #[cfg(catboost_feature_indices)] fn get_specific_feature_names(
382-415: Public API looks good; private helper could also use feature gate.
get_feature_namesis properly implemented with appropriate error handling and memory management. Theget_feature_indiceshelper has the same dead code consideration asget_specific_feature_names—it's only used by feature-gated code but isn't itself gated.Consider adding
#[cfg(catboost_feature_indices)]toget_feature_indices(line 400) as well to maintain consistency withget_specific_feature_names.
417-451: LGTM! Consistent public API implementation.All four feature name getter functions follow a consistent pattern with proper feature gating and error handling. The implementation correctly delegates to
get_specific_feature_nameswith the appropriate C function for each feature type.Optional: The doc comments could be enhanced with information about return values and potential failure cases. For example:
/// Get names of float features used in model /// /// # Returns /// A vector of feature names, or an error if: /// - The C API call fails /// - Feature indices are out of bounds /// /// # Example /// ```ignore /// let float_names = model.get_float_feature_names()?; /// ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cargo.toml(1 hunks)src/model.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/model.rs (1)
src/error.rs (1)
check_return_value(15-21)
🔇 Additional comments (2)
src/model.rs (1)
305-354: LGTM! Proper C memory management for both simple and nested structures.Both functions correctly handle the C memory lifecycle:
get_feature_indices_from_cuses the centralized helper for flat arraysget_feature_names_from_cproperly handles the 2D structure by freeing each inner string pointer before freeing the outer array- Null pointer and empty cases are handled correctly
Cargo.toml (1)
13-14: Thelibcversion 0.2.178 is valid and free from known vulnerabilities.Version 0.2.178 is the latest stable release on crates.io with no security advisories in the RustSec database. The dependency is appropriate for FFI memory management operations.
|
i apologize i missed this PR, will review and merge later today. |
| return Vec::new(); | ||
| } | ||
| let mut result = Vec::with_capacity(count); | ||
| for i in 0..count { |
There was a problem hiding this comment.
i think u can use std::slice::from_raw_parts.
unsafe { std::slice::from_raw_parts(ptr, count) }.to_vec()
| let mut names = Vec::with_capacity(count); | ||
| for i in 0..count { | ||
| let ptr = unsafe { *names_ptr.add(i) }; | ||
| let s = unsafe { CStr::from_ptr(ptr) } |
There was a problem hiding this comment.
if this panics then im not sure the underlying string will be freed, i think maybe better to implement a guard around all free's wdyt?
There was a problem hiding this comment.
maybe we can use https://github.com/bluss/scopeguard or just implement a CFreeGuard struct.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model.rs`:
- Around line 360-369: The helper functions get_specific_feature_names and
get_feature_indices must be gated with #[cfg(catboost_feature_indices)] so the
compiler doesn't type-check their bodies when the feature is disabled; add
#[cfg(catboost_feature_indices)] above the fn get_specific_feature_names(...)
and above fn get_feature_indices(...) (both are private) so their calls to
get_feature_names() and other cfg-gated items are only compiled when
catboost_feature_indices is enabled.
- Around line 351-355: The conversion from C pointers to Rust Strings must check
each inner char* for null and validate UTF‑8 before converting: in the block
that builds guards (the Vec<CFreeGuard<c_char>> and subsequent mapping), iterate
with index over the original str_ptrs/guards and for each g.0 return a
CatBoostError if the pointer is null instead of calling CStr::from_ptr; then
call CStr::from_ptr and use to_str() (not to_string_lossy()) to validate UTF‑8
and return a CatBoostError on invalid UTF‑8; only after all checks succeed,
collect owned Strings and return Ok(Vec<String>). Ensure the error paths free
resources consistently via the CFreeGuard wrapper.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed2ef684-b20c-43fc-a9a7-810fce8a4145
📒 Files selected for processing (2)
examples/advanced_usage.rssrc/model.rs
3 participants
I already created a PR in the original catboost repo. But I also found this code here and not sure yet which crate I should use.
Summary by CodeRabbit
New Features
Examples
Bug Fixes