[IVF] Simplify how we build CentroidAssignments #130709
Conversation
elasticsearchmachine
added
the
Team:Search Relevance
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
iverase
changed the title
| this.cachedCentroids = cachedCentroids; | ||
| this.assignmentsByCluster = assignmentsByCluster; | ||
| } | ||
| record CentroidAssignments(int numCentroids, float[][] centroids, int[][] assignmentsByCluster) { |
There was a problem hiding this comment.
This is what I started with originally when I did this hkmeans work but if I remember correctly @benwtrent had concerns about the centroids being in memory on merge and had also originally in the code prior to hkmeans dropped the centroids from in memory after write and then read them in one at a time with what is now the CentroidSupplier. I like the simplification but I lack the intuition or the evaluations to suggest that leaving the centroids in memory on merge is ok, which I think is happening here because we'll hold onto the CentroidAssignments object after doing the calculateAndWriteCentroids call which will keep the centroids loaded. May also be worth considering refactoring the CentroidSupplier logic if we think it's ok to keep the centroids loaded in memory. I may be misunderstanding too so please feel free to correct me here. But i'll defer to Ben here largely.
There was a problem hiding this comment.
See my other comment, we are moving how we use CentroidAssignments during merging so it is only on heap when it is needed.
| try { | ||
| centroidTemp = mergeState.segmentInfo.dir.createTempOutput(mergeState.segmentInfo.name, "civf_", IOContext.DEFAULT); | ||
| centroidTempName = centroidTemp.getName(); | ||
| centroidAssignments = calculateAndWriteCentroids( | ||
| CentroidAssignments centroidAssignments = calculateAndWriteCentroids( |
There was a problem hiding this comment.
@john-wagster Having the CentroidAssignments here will prevent having the centroids in memory during merge
There was a problem hiding this comment.
ah ok yes sorry I missed the subtle change there. I can get behind this. +1
This commit removes the flag for cache centroids when building cetroid assigments as I don't think is needed. It makes CentroidAssignments a record too.