-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Semantic text dense vector support #105515
Semantic text dense vector support #105515
Conversation
…ference results" This reverts commit bd4e19e.
…dense-vector-support # Conflicts: # server/src/main/java/org/elasticsearch/inference/ServiceSettings.java
@@ -90,7 +92,13 @@ public void onResponse(ModelRegistry.UnparsedModel unparsedModel) { | |||
var service = inferenceServiceRegistry.getService(unparsedModel.service()); | |||
if (service.isEmpty() == false) { | |||
InferenceProvider inferenceProvider = new InferenceProvider( | |||
service.get().parsePersistedConfig(inferenceId, unparsedModel.taskType(), unparsedModel.settings()), | |||
service.get() | |||
.parsePersistedConfigWithSecrets( |
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.
Secrets are needed in order to perform inference on external services
for (InferenceResults inferenceResults : results.transformToCoordinationFormat()) { | ||
String inferenceFieldName = inferenceFieldNames.get(i++); | ||
Map<String, Object> inferenceFieldResult = new LinkedHashMap<>(); | ||
inferenceFieldResult.putAll(new ModelSettings(inferenceProvider.model).asMap()); |
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.
Add model settings information to make it available to field mapping
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.
Some refactorings to provide mocks for the new methods used to retrieve config and inference
…dense-vector-support # Conflicts: # x-pack/plugin/inference/qa/test-service-plugin/src/main/java/org/elasticsearch/xpack/inference/mock/AbstractTestInferenceService.java # x-pack/plugin/inference/qa/test-service-plugin/src/main/java/org/elasticsearch/xpack/inference/mock/TestDenseInferenceServiceExtension.java # x-pack/plugin/inference/qa/test-service-plugin/src/main/java/org/elasticsearch/xpack/inference/mock/TestSparseInferenceServiceExtension.java
Pinging @elastic/es-search (Team:Search) |
…dense-vector-support
On Serverless it is not possible to configure deprecation indexing (it is always off). This commit updates the behaviour of `ElasticsearchCluster` to no longer attempt to configure deprecation indexing on stateless nodes.
…tures` conditions (elastic#105763)
increase ILM logging to TRACE. Relates to elastic#103981
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!
} | ||
} | ||
Integer dimensions = modelSettings.dimensions(); | ||
if (dimensions == 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.
Couldn't dims be calculated automatically if they weren't provided?
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.
They could! But it would signal that we're not getting what we expect from the inference results, which will always include them for dense vectors.
@@ -21,7 +21,7 @@ public class InferenceRestIT extends ESClientYamlSuiteTestCase { | |||
public static ElasticsearchCluster cluster = ElasticsearchCluster.local() | |||
.setting("xpack.security.enabled", "false") | |||
.setting("xpack.security.http.ssl.enabled", "false") | |||
.plugin("org.elasticsearch.xpack.inference.mock.TestInferenceServicePlugin") | |||
.plugin("inference-service-test") |
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.
Oh cool, you can just refer to the name here?
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.
TIL as well 😆
...rence/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_inference.yml
Show resolved
Hide resolved
…ests (elastic#105731) This setting requires expensive processing due to verification the integrity of many important files during a shard recovery or relocation. Therefore, it takes lots of time for the files to clean up and the assertShardFolder check may not complete in 30s. Fixes elastic#105202
Addresses test bug introduced in elastic#105721: we must consume all the `SnapshotInfo` instances before completing the final listener. Closes elastic#105922
- text: "inference test" | ||
inference: [0.1, 0.2, 0.3, 0.4, 0.5] | ||
- text: "another inference test" | ||
inference: [-0.1, -0.2, -0.3, -0.4, -0.5] |
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.
We should check that the mapping is as expected?
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.
We won't be doing mapping updates, just the raw indexing into the Lucene doc. How can we check that?
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've added some tests for checking the mapping does not create additional fields, is that what you were thinking of?
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 was thinking of checking that the field is updated with the right parameters (dimensions and similarity).
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.
It is not - there is no change to the mappings that reflect this change:
- The semantic_text field keeps having just
inference_id
andtype
. - There are no other fields created, as the metadata field mapper deals with the results without creating mapping changes.
Is there any mapping change you'd expect from this operation?
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.
oh I see, the mapping change is on the inner field and they are not exposed in the mapping?
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.
That is correct. There is no mapping change, the metadata field mapper performs the indexing directly, without a mapping change.
AFAIU we can't do updates in the metadata field mapper for holding the changes. And we can't create a mapping with the same field path as a metadata field.
Is there something I'm missing?
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.
no that's just me trying to understand how to best test this (checking that we're creating the right field type in Lucene). We probably need a unit test that checks that we're doing the right thing but that can be a follow up.
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.
There's some unit tests provided by @Mikep86 that check the Lucene doc structure.
Also, we can check the end-to-end scenario once we have the semantic_query ready, so we can check retrieving docs work
feature_1: 0.1 | ||
feature_2: 0.2 | ||
feature_3: 0.3 | ||
feature_4: 0.4 |
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.
We should check the response too? And then verify that the mapping is as expected?
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.
These tests provide the actual inference results so we can check that the parsing is done as expected, and the inference format is correct.
What would you like to check in the responses?
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 misread, I thought it was a bulk index but it's just one document sorry.
…5930) This change ensures that the matches implementation of the `SourceConfirmedTextQuery` only checks the current document instead of calling advance on the two phase iterator. The latter tries to find the first doc that matches the query instead of restricting the search to the current doc. This can lead to abnormally slow highlighting if the query is very restrictive and the highlight is done on a non-matching document. Closes elastic#103298
…c#105912) * Bugfix for CCR queries using text expansion * Fix test * PR feedback * Fix test * Minor cleanup * Edit comment * One more comment clarification --------- Co-authored-by: Elastic Machine <[email protected]>
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 left a minor comment, LGTM otherwise
@@ -8,7 +8,6 @@ | |||
|
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 a fan of the renaming, why not keeping the original name? ModelSettings
is too generic imo and we only need these settings for the field mapping.
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.
Makes sense, renaming it back to SemanticTextModelSettings
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.
Good step in the right direction. I like the reuse of the mapper builders. Maybe even allowing MORE model types in the future.
We do need to figure out how to prevent inference results formats from changing once we have parsed a particular kind of field, but that discussion is outside the scope of this pr.
b1a3ee8
into
elastic:feature/semantic-text
Adds support for dense vector models.
To make sure the field mappers receive enough information to perform indexing, model information is included into each document that is ingested under
model_settings
field: