fix: stabilize IVF-PQ k-means training - #71
Conversation
Also finalize degenerate splits that produce a single non-empty child; previously such clusters were re-queued unchanged, looping forever on highly duplicated data.
Zero padding fabricated origin centroids on highly duplicated data, which became reachable once degenerate splits stopped looping forever. Also restore non-panicking d=0 assignment behavior.
|
I think the batched hierarchical splitting changes the clustering policy in a way that can significantly hurt quality on skewed datasets. At I isolated the scheduling difference while keeping the PR's current assignment implementation and the new degenerate-split handling. On a deterministic skewed dataset (
That is about a 9.7% increase in quantization error. The added recall scenario uses fairly uniform generated populations, so it does not cover this regression mode. Could we keep the parallel assignment and degenerate-split fixes, but avoid replacing best-first splitting with a breadth-first schedule? Alternatively, the parallel split selection needs to preserve the population-aware allocation property, with a skewed-distribution quality regression test. |
shyjsarah
left a comment
There was a problem hiding this comment.
Thanks for the update. I re-reviewed the latest head 60eecb1. The strict largest-first hierarchy behavior and concurrent sub_data memory regression from the previous revision are now fixed.
There is still one blocking memory-safety issue:
[Blocking] Validate tensor shapes before entering unsafe SGEMM
kmeans_train_with_init accepts data, n, d, k, and initial_centroids independently, but does not validate:
data.len() == n * dinitial_centroids.len() == k * d
In assign_clusters_fast/assign_block, the claimed row count reaches sgemm_a_bt before the first bounds-checked row access. For example, three rows of data with n=4, d=2, k=2, and valid initial centroids causes SGEMM to read a fourth row beyond the allocation. ASan reproduces a heap-buffer-overflow inside matrixmultiply.
Since this is reachable through a safe public API, malformed input may return an error or panic, but must not cause undefined behavior.
Please use checked multiplication to validate shapes at the public boundary and add defensive assertions at the unsafe SGEMM boundary. Tests should cover short/long data, invalid initial-centroid lengths, and dimension overflow.
Non-blocking: parallel assignment can retain one ip_matrix per Rayon worker. With a large custom max_points_per_centroid, peak scratch memory therefore scales with the ambient worker count. An aggregate scratch budget or worker-buffer reuse would make the memory behavior more predictable.
Local formatting, Clippy, core tests, 1/2/4/8-thread K-means tests, and the latest GitHub CI all pass.
|
+1 |
1 similar comment
|
+1 |
Summary
Fixes correctness and termination edge cases in IVF-PQ hierarchical k-means while preserving the historical strict largest-first split policy. The PR also parallelizes row-independent cluster assignment with bounded, deterministic blocks; the measured speedup is modest now that hierarchy scheduling is intentionally serial again.
Changes
k=2,d=768split collapses to one SGEMM.ip_matrixscratch across Rayon workers to about 16 MiB, with a serial fallback when per-worker blocks are too small.TRAIN_TARGET=1to run the target244606 x 768workload.Target benchmark
Command:
TRAIN_TARGET=1 cargo +1.97.0 bench -p paimon-vindex-core --bench ivfpq_train_benchDeterministic
target-768input:n=244606,d=768,nlist=1024,pq_m=96, inner product, OPQ disabled. Results are three-run medians on an Apple M3 Pro with 11 CPU cores and 36 GiB RAM. The 11-thread measurements alternated the base and PR binaries to reduce system-load drift.6374d05731d93e6374d05731d93etrain_total_sis the authoritative end-to-end measurement; phase timings come from separate stage replays and are not additive. The earlier 4.5x coarse result depended on the reverted batched hierarchy scheduler and is not claimed by this version. No 16-core Xeon result is available; 11 threads matches the test machine's physical CPU count.Testing
cargo +1.97.0 test -p paimon-vindex-core --lib— 439 passed, 1 ignoredcargo +1.97.0 fmt --all -- --checkgit diff --checkTRAIN_TARGET=1benchmark at 1 and 11 threads, three runs per configurationNotes
data.len()andinitial_centroids.len()must exactly match their declared shapes; invalid or overflowing shapes now panic instead of silently ignoring trailing elements or risking out-of-bounds SGEMM access.