[FEA] Multi-node Out of Core Streaming KMeans API#2066
Conversation
…nto combine-batch
…nto combine-batch
…nto combine-batch
There was a problem hiding this comment.
The SNMG batched KMeans C/Python API was merged before and unfortunately now calls the single-GPU path after this PR. We should fix this.
|
/ok to test 78a4278 |
|
/merge |
|
/ok to test 2eed544 |
| recluster_params.n_init = 1; | ||
|
|
||
| auto weight_opt = std::make_optional(raft::make_const_mdspan(weight.view())); | ||
| cuvs::cluster::kmeans::fit(handle, |
There was a problem hiding this comment.
| cuvs::cluster::kmeans::fit(handle, | |
| cuvs::cluster::kmeans::detail::kmeans_fit(handle, |
Worker needlessly emits the following warning :
Multi-GPU handle detected on single-GPU kmeans::fit() entry; falling back to single-GPU. Use cuvs::cluster::kmeans::mg::fit(...) for multi-GPU.
We need to fix this.
There was a problem hiding this comment.
I don't think there is anything wrong with that warning. Yes it is triggered unnecessarily here by our own initialization implementation, but I am not in the favor of calling the detail namespace API. We should stick to calling public functions wherever possible.
There was a problem hiding this comment.
From DM discussions, it looks like calling cuvs::cluster::kmeans::detail::kmeans_fit directly may unnecessarily increase binary size. We should maybe come back at this later since the warning is a bit odd. But I will approve the PR for now to get the PR unstucked.
| * The batch size is controlled by params.streaming_batch_size. | ||
| * | ||
| * Multi-GPU dispatch is selected automatically based on the handle state: | ||
| * - If `raft::resource::is_multi_gpu(handle)` (cuVS SNMG): the full dataset X |
There was a problem hiding this comment.
@viclafargue I brought back this dispatch. So #2319 is not needed anymore.
| * @} | ||
| */ | ||
|
|
||
| namespace mg { |
There was a problem hiding this comment.
Why are we declaring a separate public namespace for mg at all? The whole idea behind consolidating namespaces was for the user to have an easier experience by configuring the appropriate resources type to determine if they want mg operation. Please don't create separate namespaces for mg in the public APIs.
Victor did thag originally and I asked him to remove it.
|
/ok to test ee6c302 |
|
/merge |
github-project-automation
Bot
moved this from In Progress
to Done
in Unstructured Data Processing
8 participants
Merge after #2015 and #2017
Allows a stream of input matrices per worker, that are further batched using the
streaming_batch_sizeparameter. Reasoning: We should be able to supply dask partitions (on host) directly without having to concatenate them into one consolidated matrix.As a part of this PR, we also unify the multi-GPU implementations into one (earlier the out of core implementation was separate).
Tests: We get rid of the separate out of core test file. The single MG testing unit is taking care of both out of core and on device matrices.