`HDBSCAN` output arrays may become invalid

[jcrist]

The C++ api in HDBSCAN returns a struct containing buffers that are later exposed to python by wrapping them as UnownedMemory. However, this is done incorrectly in a few ways:

• Most wrapped buffers set the HDBSCAN estimator itself as the owner. However, the underlying buffers are tied independently to the lifetime of the estimator. Subsequent fit calls drop the buffers, causing the originally returned cupy arrays to become invalid.
• Some wrapped buffers accidentally set None as the owner (like in here, where the optional owner kwarg is not provided), meaning they may become invalid if the estimator is dropped before the array values since they're not holding a reference.

To remedy both of these bugs, a non-user-facing python class HDBSCANOutput (or a better name) should be created, and the buffer lifetimes tied to it. It can then be stored as python-level attribute on the HDBSCAN estimator. Upon refit, the attribute can be cleared/overwritten to decouple the references (letting python's reference counting decide if the buffers can be released yet or not).

I have seen memory errors from this, but cannot seem to achieve a clean reproducer (presumably because the original buffer location has yet to be reused). Looking at the code listed above though should (hopefully) be enough for others to clearly see the bug here.

[jcrist]

@divyegala pointed out offline a 3rd issue that I had missed - the delete_hdbscan_output function is mistakenly using del instead of a call to free, meaning the underlying struct isn't being deleted at all but rather leaking (and delete_hdbscan_output is effectively a no-op). (

cuml/python/cuml/cuml/cluster/hdbscan/hdbscan.pyx

Line 319 in 9559f85

del output

edit: nvm - looks like del should do the correct thing here and act as a delete call? (at least per https://cython.readthedocs.io/en/latest/src/userguide/wrapping_CPlusPlus.html). When wrapping C++ in cython I'm much more used to using unique_ptr or explicit free calls, so had never seen this overload before.