-
Notifications
You must be signed in to change notification settings - Fork 25.7k
[ML] Do not create inference endpoint if ID is used in existing mappings #137055
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
Changes from 7 commits
9d5ca1c
98a0091
5fb900e
614a61e
e95e06e
a6fba17
5ec69c7
d566301
d33ebfb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| pr: 137055 | ||
| summary: Do not create inference endpoint if ID is used in existing mappings | ||
| area: Machine Learning | ||
| type: bug | ||
| issues: | ||
| - 124272 |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| package org.elasticsearch.xpack.inference; | ||
|
|
||
| import org.apache.http.util.EntityUtils; | ||
| import org.elasticsearch.client.Request; | ||
| import org.elasticsearch.client.Response; | ||
| import org.elasticsearch.client.ResponseException; | ||
| import org.elasticsearch.common.Strings; | ||
|
|
@@ -211,7 +212,7 @@ public void testDeleteEndpointWhileReferencedBySemanticText() throws IOException | |
| final String endpointId = "endpoint_referenced_by_semantic_text"; | ||
| final String searchEndpointId = "search_endpoint_referenced_by_semantic_text"; | ||
| final String indexName = randomAlphaOfLength(10).toLowerCase(); | ||
| final Function<String, String> buildErrorString = endpointName -> " Inference endpoint " | ||
| final Function<String, String> buildErrorString = endpointName -> "Inference endpoint " | ||
| + endpointName | ||
| + " is being used in the mapping for indexes: " | ||
| + Set.of(indexName) | ||
|
|
@@ -303,6 +304,74 @@ public void testDeleteEndpointWhileReferencedBySemanticTextAndPipeline() throws | |
| deleteIndex(indexName); | ||
| } | ||
|
|
||
| public void testCreateEndpoint_withInferenceIdReferencedBySemanticText() throws IOException { | ||
| final String endpointId = "endpoint_referenced_by_semantic_text"; | ||
| final String otherEndpointId = "other_endpoint_referenced_by_semantic_text"; | ||
| final String indexName1 = randomAlphaOfLength(10).toLowerCase(); | ||
| final String indexName2 = randomValueOtherThan(indexName1, () -> randomAlphaOfLength(10).toLowerCase()); | ||
|
|
||
| putModel(endpointId, mockDenseServiceModelConfig(128), TaskType.TEXT_EMBEDDING); | ||
| putModel(otherEndpointId, mockDenseServiceModelConfig(), TaskType.TEXT_EMBEDDING); | ||
| // Create two indices, one where the inference ID of the endpoint we'll be deleting and | ||
| // recreating is used for inference_id and one where it's used for search_inference_id | ||
| putSemanticText(endpointId, otherEndpointId, indexName1); | ||
| putSemanticText(otherEndpointId, endpointId, indexName2); | ||
|
|
||
| // Confirm that we can create the endpoint with different settings if there | ||
| // are documents in the indices which do not use the semantic text field | ||
| var request = new Request("PUT", indexName1 + "/_create/1"); | ||
| request.setJsonEntity("{\"non_inference_field\": \"value\"}"); | ||
| assertStatusOkOrCreated(client().performRequest(request)); | ||
|
|
||
| request = new Request("PUT", indexName2 + "/_create/1"); | ||
| request.setJsonEntity("{\"non_inference_field\": \"value\"}"); | ||
| assertStatusOkOrCreated(client().performRequest(request)); | ||
|
|
||
| assertStatusOkOrCreated(client().performRequest(new Request("GET", "_refresh"))); | ||
|
|
||
| deleteModel(endpointId, "force=true"); | ||
| putModel(endpointId, mockDenseServiceModelConfig(64), TaskType.TEXT_EMBEDDING); | ||
|
|
||
| // Index a document with the semantic text field into each index | ||
| request = new Request("PUT", indexName1 + "/_create/2"); | ||
| request.setJsonEntity("{\"inference_field\": \"value\"}"); | ||
| assertStatusOkOrCreated(client().performRequest(request)); | ||
|
|
||
| request = new Request("PUT", indexName2 + "/_create/2"); | ||
| request.setJsonEntity("{\"inference_field\": \"value\"}"); | ||
| assertStatusOkOrCreated(client().performRequest(request)); | ||
|
|
||
| assertStatusOkOrCreated(client().performRequest(new Request("GET", "_refresh"))); | ||
|
|
||
| deleteModel(endpointId, "force=true"); | ||
|
|
||
| // Try to create an inference endpoint with the same ID but different dimensions | ||
| // from when the document with the semantic text field was indexed | ||
| ResponseException responseException = assertThrows( | ||
| ResponseException.class, | ||
| () -> putModel(endpointId, mockDenseServiceModelConfig(128), TaskType.TEXT_EMBEDDING) | ||
| ); | ||
|
Comment on lines
+348
to
+353
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this should fail even if there aren't any docs in the index.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean that it should fail if no docs with semantic text fields were ever indexed, or it should fail if docs were indexed but then removed from the index? In the former case, we wouldn't have stored the model details, because that information is only populated when we get the inference results back when ingesting a document with a semantic text field. In the latter case, the endpoint creation does fail with these changes, because the semantic text field mapping isn't cleared when documents are deleted. Since we don't persist information about the model until a doc with a semantic text field is indexed, I don't think there's any risk to the user if they create an endpoint with certain settings, delete it, then create a new one with different settings, but never index any documents with semantic text fields before creating the endpoint for the second time.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep, you are correct. I have been out of the loop here for a bit. LGTM! |
||
| assertThat( | ||
| responseException.getMessage(), | ||
| containsString( | ||
| "Inference endpoint [" | ||
| + endpointId | ||
| + "] could not be created because the inference_id is being used in mappings with incompatible settings for indices: [" | ||
| ) | ||
| ); | ||
| assertThat(responseException.getMessage(), containsString(indexName1)); | ||
| assertThat(responseException.getMessage(), containsString(indexName2)); | ||
| assertThat( | ||
| responseException.getMessage(), | ||
| containsString("Please either use a different inference_id or update the index mappings to refer to a different inference_id.") | ||
| ); | ||
|
|
||
| deleteIndex(indexName1); | ||
| deleteIndex(indexName2); | ||
|
|
||
| deleteModel(otherEndpointId, "force=true"); | ||
| } | ||
|
|
||
| public void testUnsupportedStream() throws Exception { | ||
| String modelId = "streaming"; | ||
| putModel(modelId, mockCompletionServiceModelConfig(TaskType.SPARSE_EMBEDDING, "streaming_completion_test_service")); | ||
|
|
||
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.
Do these need to be public?
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 realize its consistency, but having it in this PR implies a behavior access change, and there isn't.
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.
My mistake, I was originally using these in tests, but then realized that
ServiceFieldshas the same Strings defined and already public. I'll put them back how they were. It would be nice if we could have a single source of truth for these sorts of constants, because they're defined independently in a lot of different places, which makes it a real pain if we ever want to update them.