Adds graph build time metrics in NativeEngines990KnnVectorsWriter#2018
Adds graph build time metrics in NativeEngines990KnnVectorsWriter#2018heemin32 merged 1 commit intoopensearch-project:mainfrom
Conversation
shatejas
requested review from
VijayanB,
heemin32,
jmazanec15,
junqiu-lei,
luyuncheng,
martin-gaievski,
naveentatikonda,
navneet1v,
ryanbogan and
vamshin
as code owners
src/main/java/org/opensearch/knn/index/codec/KNN990Codec/NativeEngines990KnnVectorsWriter.java
Outdated
Show resolved
Hide resolved
shatejas
force-pushed
the
native-metric
branch
from
9b6f9a6 to
47415b3
Compare
navneet1v
left a comment
navneet1v
left a comment
There was a problem hiding this comment.
Approving the change. But I do believe this function: trainAndIndex is becoming hard to extend. @Vikasht34 lets do some refactoring on this after the freeze.
| indexOperation.buildAndWrite(writer, knnVectorValues); | ||
| long time_in_millis = stopWatch.totalTime().millis(); | ||
| graphBuildTime.set(graphBuildTime.getValue() + time_in_millis); | ||
| log.warn("Graph build took " + time_in_millis + " ms for " + operationName); |
There was a problem hiding this comment.
Why are we publishing this as warn level but not info level?
There was a problem hiding this comment.
I think that status quo is not correct.
src/main/java/org/opensearch/knn/index/codec/KNN990Codec/NativeEngines990KnnVectorsWriter.java
Outdated
Show resolved
Hide resolved
| flatVectorsWriter.mergeOneField(fieldInfo, mergeState); | ||
| // For merge, pick values from flat vector and reindex again. This will use the flush operation to create graphs | ||
| trainAndIndex(fieldInfo, this::getKNNVectorValuesForMerge, NativeIndexWriter::mergeIndex, mergeState); | ||
| trainAndIndex( |
There was a problem hiding this comment.
The class seems to only have a component test. Will try to cover it in the existing component test and create an issue to add unit test around this so it can be taken as a follow up
There was a problem hiding this comment.
@heemin32 do we need UTs to check if the log is coming?
There was a problem hiding this comment.
Not about logging but about adding build time in metrics.
There was a problem hiding this comment.
The class seems to only have a component test. Will try to cover it in the existing component test and create an issue to add unit test around this so it can be taken as a follow up
I believe, all these static method is making it harder to write a unit test. I wish we can rely more on dependency injection using Guice.
There was a problem hiding this comment.
I believe, all these static method is making it harder to write a unit test. I wish we can rely more on dependency injection using Guice.
if we want to make these things work via DI then its a change which is out of scope of this PR.
There was a problem hiding this comment.
I know. Just sharing my long term desire and testing the water :)
Signed-off-by: Tejas Shah <shatejas@amazon.com>
shatejas
force-pushed
the
native-metric
branch
from
93d6042 to
4d65e9f
Compare
) Signed-off-by: Tejas Shah <shatejas@amazon.com> (cherry picked from commit e6c5953)
…ensearch-project#2018) Signed-off-by: Tejas Shah <shatejas@amazon.com> Signed-off-by: Akash Shankaran <akash.shankaran@intel.com>
…ensearch-project#2018) Signed-off-by: Tejas Shah <shatejas@amazon.com>
3 participants
Description
Adds graph build time. This was missed in the initial PR
Related Issues
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.