-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add usage stats for semantic_text fields #135262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add usage stats for semantic_text fields #135262
Conversation
|
Pinging @elastic/ml-core (Team:ML) |
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
|
Hi @dimitris-athanasiou, I've created a changelog YAML for you. |
docs/changelog/135262.yaml
Outdated
| @@ -0,0 +1,5 @@ | |||
| pr: 135262 | |||
| summary: Add usage stats for `semantic_text` fields | |||
| area: "Search" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the area should be Search or Machine Learning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Vector Search" is my vote
|
Pinging @elastic/search-relevance (Team:Search - Relevance) |
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Are there any existing yaml tests that we could update here too?
.../core/src/test/java/org/elasticsearch/xpack/core/inference/usage/SemanticTextStatsTests.java
Outdated
Show resolved
Hide resolved
...ce/src/main/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageAction.java
Show resolved
Hide resolved
c0144ff to
615b98d
Compare
|
I have added YAML tests as per your suggestion @kderusso |
e9b092d to
40c5646
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I left some comments, but they're almost all nits. Per our offline discussion, I think we need to figure out if we want to filter out all hidden indices (i.e. those that start with .). Other than that, this PR is in good shape 🚀
docs/changelog/135262.yaml
Outdated
| @@ -0,0 +1,5 @@ | |||
| pr: 135262 | |||
| summary: Add usage stats for `semantic_text` fields | |||
| area: "Search" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Vector Search" is my vote
...ce/src/main/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageAction.java
Outdated
Show resolved
Hide resolved
...ce/src/main/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageAction.java
Outdated
Show resolved
Hide resolved
...ce/src/main/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageAction.java
Outdated
Show resolved
Hide resolved
...ce/src/main/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageAction.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageActionTests.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageActionTests.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/inference/inference_usage.yml
Outdated
Show resolved
Hide resolved
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/inference/inference_usage.yml
Outdated
Show resolved
Hide resolved
| type: semantic_text | ||
| field_2: | ||
| type: semantic_text | ||
| inference_id: .multilingual-e5-small-elasticsearch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note on default endpoint usage in CI: We have noticed flakiness when we actually use them to generate embeddings due to deployment timeouts and failures. This reference should be fine because it's not actually triggering deployment of the E5 model, but something to be aware of in general.
76a002b to
86e022e
Compare
|
@Mikep86 Regarding hidden indices. I have excluded them too for now. They are probably of little value telemetry-wise. However, if we want to collect them in the future we can easily have a separate section for them by adding a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good, thanks for the changes! Functionality looks solid. I left a few comments for one more round of small changes, then I think we're good to merge!
...ugin/core/src/main/java/org/elasticsearch/xpack/core/inference/InferenceFeatureSetUsage.java
Outdated
Show resolved
Hide resolved
...ce/src/main/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageAction.java
Outdated
Show resolved
Hide resolved
...ce/src/main/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageAction.java
Show resolved
Hide resolved
...ce/src/main/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageAction.java
Show resolved
Hide resolved
...c/test/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageActionTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I have some minor comments in my previous review, but none of them are blocking.
This change enhances usage reporting for the inference plugin to account for usage of `semantic_text` fields. Usage is bucketed by service and task_type. For each bucket this adds a `semantic_text` object that contains: - `field_count`: the number of `semantic_text` fields that use an inference endpoint of that service/task_type. - `indices_count`: the number of indices that contain at least one `semantic_field` referencing an inference endpoint of that service/task_type. - `inference_id_count`: the number of distinct inference endpoints of that service/task_type used by `semantic_text` fields. In addition, this change adds two new kinds of buckets that facalitate getting aggregate usage information: - `_all` buckets are added by `task_type`. These allow summation of usage info for all inference endpoints of a particular `task_type`. - default model buckets are added. Those contain usage info for models that are included by default.
This reverts commit d72707d.
cb6e0c8 to
5a93315
Compare
5a93315 to
3d4f273
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
...ce/src/main/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageAction.java
Show resolved
Hide resolved
| Map<String, ModelStats> endpointStats | ||
| ) { | ||
| for (TaskType taskType : TaskType.values()) { | ||
| if (taskType == TaskType.ANY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RERANK, COMPLETION and CHAT_COMPLETION task types will never appear in a semantic text field and also can be skipped here.
Add a static function TaskType [] semanticTextTaskTypes( return SPARSE + TEXT) to TaskType and just iterate those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point we are adding top level _all stats buckets. Even though we won't have semantic_text_stats for those tasks, we can still have top level buckets for count and for the consistency/symmetry with what we do for the semantic_text supporting tasks. What do you reckon?
...ce/src/main/java/org/elasticsearch/xpack/inference/action/TransportInferenceUsageAction.java
Show resolved
Hide resolved
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/inference/inference_usage.yml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some suggestions about how to simplify the implementation
| private final boolean isCompatibleWithSemanticText; | ||
|
|
||
| TaskType(boolean isCompatibleWithSemanticText) { | ||
| this.isCompatibleWithSemanticText = isCompatibleWithSemanticText; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally wouldn't modify TaskType directly to add this metadata. We only need it in TransportInferenceUsageAction, I would build a set of task types that need semantic text stats in that class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 632b7cb
| } | ||
|
|
||
| public ModelStats(String service, TaskType taskType, long count) { | ||
| this(service, taskType, count, taskType.isCompatibleWithSemanticText() ? new SemanticTextStats() : null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be simpler to always set semanticTextStats to null by default. Code paths in TransportInferenceUsageAction can pass in a non-null instance as necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 632b7cb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the continued iterations on this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This change enhances usage reporting for the inference plugin to account for usage of
semantic_textfields.Usage is bucketed by service and task_type. For each bucket this adds a
semantic_textobject that contains:field_count: the number ofsemantic_textfields that use an inference endpoint of that service/task_type.indices_count: the number of indices that contain at least onesemantic_fieldreferencing an inference endpoint of that service/task_type.inference_id_count: the number of distinct inference endpoints of that service/task_type used bysemantic_textfields.In addition, this change adds two new kinds of buckets that facalitate getting aggregate usage information:
_allbuckets are added bytask_type. These allow summation of usage info for all inference endpoints of a particulartask_type.