Repository navigation
Use PrefetchResourceAdaptor in cuml.accel - #7834
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors install() memory-resource handling to detect and treat a PrefetchResourceAdaptor wrapping PoolMemoryResource(ManagedMemoryResource()) as already enabled, skip enabling for non-CUDA resources, and when enabling use PrefetchResourceAdaptor(PoolMemoryResource(ManagedMemoryResource())). Adds a new SVR unit test for zero supports. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
This is blocked by #7842. |
By default `cuml.accel` enables a `ManagedMemoryResource`, which allocates managed memory. While benchmarking some workloads, I've noticed a measurable performance improvement when wrapping this mr with a `PrefetchResourceAdaptor`. This adaptor schedules a prefetch of memory to the device immediately after allocation. For `cuml`/`cuml.accel` workloads this makes sense - we typically allocate an array, then call a kernel to fill it in, before returning it. This also matches some performance advice noted by other teams (and the official CUDA programming guide) that prefetching may be useful when working with managed memory. This also more closely matches the rmm configuration used by `cudf.pandas` by default (though they also use a `PoolMemoryResource` by default, but always enable prefetching of managed memory even without the pool). IIUC pooling makes sense when you have many smaller allocations (like might happen with pandas), since it can amortize the upstream allocator cost across calls. cuml workloads typically do fewer and larger allocations, so I don't think investigating also enabling pooling is necessary for now.
a9c6b31 to
86aeff3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml_accel_tests/integration/test_svr.py`:
- Around line 82-89: In test_svr_fit_n_support_0 update the assertions to use
tolerance-based float comparisons (e.g., numpy allclose or pytest.approx) for
model.intercept_ and model.predict(X) instead of direct equality, avoid calling
.all() on model.n_support_ (treat it as an int or use np.array(...) then compare
to 0) and keep the shape check for model.dual_coef_ as-is; specifically, replace
exact equality checks on model.intercept_, model.predict(X) with approximate
checks and change the n_support_ assertion to handle scalar or array forms
reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15b7cf76-3e75-4893-bf35-5fe94263a11c
📒 Files selected for processing (2)
python/cuml/cuml/accel/core.pypython/cuml/cuml_accel_tests/integration/test_svr.py
|
/merge |
By default
cuml.accelenables aManagedMemoryResource, which allocates managed memory. While benchmarking some workloads, I've noticed a measurable performance improvement when wrapping this mr with aPrefetchResourceAdaptor. This adaptor schedules a prefetch of memory to the device immediately after allocation. Forcuml/cuml.accelworkloads this makes sense - we typically allocate an array, then call a kernel to fill it in, before returning it. This also matches some performance advice noted by other teams (and the official CUDA programming guide) that prefetching may be useful when working with managed memory.This also more closely matches the rmm configuration used by
cudf.pandasby default (see here). Note thatcudf.pandasalso uses aPoolMemoryResourceby default, but always enables prefetching of managed memory even without the pool. My intuition is that pooling makes sense when you have many smaller allocations (like might happen with pandas), since it can amortize the upstream allocator cost across calls. cuml workloads typically do fewer and larger allocations, so I don't think investigating pooling is necessary for now.Fixes #7846.