Expose raft::memory_tracking_resources to C/Python/Java/Rust#2073
Expose raft::memory_tracking_resources to C/Python/Java/Rust#2073achirkin wants to merge 5 commits into
Conversation
|
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
WalkthroughThis PR adds background CSV memory-allocation tracking to CuVS resources across C, Python, Java, and Rust. The core ChangesMemory Tracking Resources Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
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 `@c/src/core/c_api.cpp`:
- Around line 42-54: The function cuvsResourcesCreateWithMemoryTracking must
validate the output handle and interval: before dereferencing res check that res
is not null and throw std::invalid_argument if it is, and validate
sample_interval_ms > 0 (throw std::invalid_argument for non-positive values) to
avoid unstable sampling; then only allocate the raft::memory_tracking_resources
(res_ptr) and assign *res = reinterpret_cast<uintptr_t>(res_ptr) after these
checks so the translate_exceptions wrapper still returns errors consistently.
In `@java/cuvs-java/src/main/java/com/nvidia/cuvs/spi/CuVSProvider.java`:
- Around line 50-53: Change the new abstract method in the CuVSProvider
interface into a default method to preserve SPI compatibility: declare default
CuVSResources newCuVSResources(Path tempDirectory, Path memoryTrackingCsvPath,
Duration memoryTrackingSampleInterval) throws Throwable, and implement it to
delegate to the existing older overload (e.g., call the existing
newCuVSResources(Path tempDirectory) overload) so existing providers continue to
work; if no older overload exists, have the default implementation throw an
UnsupportedOperationException with a clear message so providers can opt-in to
override the new behavior.
In `@python/cuvs/cuvs/common/resources.pyx`:
- Around line 75-85: The __cinit__ currently treats an empty string
memory_tracking_csv_path as falsy and silently skips memory tracking; change the
validation to explicitly check for None versus empty string: if
memory_tracking_csv_path is None call cuvsResourcesCreate, otherwise verify the
provided path is a non-empty string/bytes (e.g., check length > 0) and then
encode it and call cuvsResourcesCreateWithMemoryTracking with csv_bytes and
memory_tracking_sample_interval_ms, and if the path is empty raise a Python
ValueError; keep using check_cuvs around the C calls (referencing __cinit__,
memory_tracking_csv_path, cuvsResourcesCreateWithMemoryTracking,
cuvsResourcesCreate, check_cuvs).
In `@rust/cuvs/src/resources.rs`:
- Around line 49-50: The current cast from Duration::as_millis() (u128) to i64
for sample_interval_ms is lossy; replace the direct as_millis() as i64 cast with
a fallible conversion using .try_into() on the u128 result (e.g., let
sample_interval_ms: i64 = ...as_millis().try_into()?), and propagate or return a
clear error if conversion fails (do not silently wrap/truncate before passing to
the C FFI). Update the surrounding function (the code that defines
sample_interval_ms and uses sample_interval) to return or propagate an error (or
otherwise handle the failure) so invalid large durations aren’t passed into the
FFI.
🪄 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: f8434388-318b-4816-a423-a13a3977db91
📒 Files selected for processing (15)
c/include/cuvs/core/c_api.hc/src/core/c_api.cppdocs/source/api_basics.rstjava/cuvs-java/src/main/java/com/nvidia/cuvs/CuVSResources.javajava/cuvs-java/src/main/java/com/nvidia/cuvs/spi/CuVSProvider.javajava/cuvs-java/src/main/java/com/nvidia/cuvs/spi/UnsupportedProvider.javajava/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/CuVSResourcesImpl.javajava/cuvs-java/src/main/java22/com/nvidia/cuvs/spi/JDKProvider.javajava/cuvs-java/src/test/java/com/nvidia/cuvs/MemoryTrackingResourcesIT.javapython/cuvs/cuvs/common/c_api.pxdpython/cuvs/cuvs/common/resources.pyxpython/cuvs/cuvs/tests/test_memory_tracking.pyrust/cuvs-sys/src/bindings.rsrust/cuvs/Cargo.tomlrust/cuvs/src/resources.rs
| /// | ||
| /// `sample_interval` controls the minimum time between successive CSV | ||
| /// samples; when `None`, the C++ default of 10 ms is used. | ||
| pub fn with_memory_tracking<P: AsRef<Path>>( |
There was a problem hiding this comment.
@yan-zaretskiy can we get your review on these parts?
tfeher
left a comment
There was a problem hiding this comment.
C and Python changes LGTM
Expose the new
raft::memory_tracking_resourcesin the public API of all supported languages and add documentation.See Raft #2973 for more information about memory tracking resources.
Resolves rapidsai/raft#2982