semantic_text - extract Index Metadata inference information to separate class #106328

carlosdelest · 2024-03-13T18:54:09Z

Creates a new class for storing field inference information.

Field inference information is composed of:

Inference ID associated to each field
Source paths that the field must calculate inference on, besides its actual value. This will provide copy_to and multifield capabilities.

This refactoring allows control on the serialization of this information and provide bwc on it for the future.

Some renaming has been performed on methods to refer to inference id instead of model id.

…s class

…results

…ference results" This reverts commit bd4e19e.

carlosdelest · 2024-03-13T18:54:34Z

server/src/main/java/org/elasticsearch/cluster/metadata/FieldInferenceMetadata.java

This is the class that will hold inference information, and the one I'd like to have your early feedback on :) .

carlosdelest · 2024-03-13T18:55:05Z

server/src/main/java/org/elasticsearch/cluster/metadata/FieldInferenceMetadata.java

+    public static final ParseField INFERENCE_FOR_FIELDS_FIELD = new ParseField("inference_for_fields");
+    public static final ParseField SOURCE_FIELDS_FIELD = new ParseField("source_fields");
+
+    public FieldInferenceMetadata(


I've prioritised using field -> inference IDs map, as it seems clearer from the mapping perspective as well.

The class hides the actual implementation and we can calculate the reverse information on demand

carlosdelest · 2024-03-13T18:55:49Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

@@ -632,8 +631,7 @@ public Iterator<Setting<?>> settings() {
    private final Double writeLoadForecast;
    @Nullable
    private final Long shardSizeInBytesForecast;
-    // Key: model ID, Value: Fields that use model
-    private final ImmutableOpenMap<String, Set<String>> fieldsForModels;
+    private final FieldInferenceMetadata fieldInferenceMetadata;


IndexMetadata now uses the new class

carlosdelest · 2024-03-13T19:03:18Z

server/src/main/java/org/elasticsearch/action/bulk/BulkShardRequestInferenceProvider.java

@@ -77,8 +77,8 @@ public static void getInstance(
    ) {
        Set<String> inferenceIds = new HashSet<>();
        shardIds.stream().map(ShardId::getIndex).collect(Collectors.toSet()).stream().forEach(index -> {
-            var fieldsForModels = clusterState.metadata().index(index).getFieldsForModels();
-            inferenceIds.addAll(fieldsForModels.keySet());
+            var fieldsForInferenceIds = clusterState.metadata().index(index).getFieldInferenceMetadata().getFieldsForInferenceIds();


I've done some renamings to start using inference ids vs model

Mikep86

Restricted my review to FieldInferenceMetadata for now. It looks great!

Mikep86 · 2024-03-13T21:01:45Z

server/src/main/java/org/elasticsearch/cluster/metadata/FieldInferenceMetadata.java

+    private final ImmutableOpenMap<String, String> inferenceIdForField;
+
+    // Keys: field names. Values: Field names that provide source for this field (either as copy_to or multifield sources)
+    private final ImmutableOpenMap<String, Set<String>> sourceFields;


We may want to change this to ImmutableOpenMap<String, List<String>> so that the source field iteration order is consistent

I don't think order is of consequence. FieldTypeLookup.sourcePaths(), which is used for similar purposes, also returns a Set.

I bring it up because @benwtrent brought it up during multi-field/copy_to support discovery. Depending on how we handle inference generation for multi-valued fields in the bulk action, order could matter.

For example, if we handle it by concatenating all the values, order could affect the chunks that are produced and therefore the inference results.

Mikep86 · 2024-03-13T21:26:57Z

server/src/main/java/org/elasticsearch/cluster/metadata/FieldInferenceMetadata.java

+        return sourceFields.get(field);
+    }
+
+    public Map<String, Set<String>> getFieldsForInferenceIds() {


Could this method be accessed by multiple threads? Should we mark fieldsForInferenceIds as volatile or maybe even wrap it in AtomicReference?

Yes, it can be accessed by multiple threads.

I didn't want to overdo synchronization / early optimization, and have kept it light on purpose.

I think it's preferrable to recalculate vs having the syncing overhead - but happy to hear your thoughts.

Agree we don't need to go heavy on synchronization here. Worst case, a race condition causes this calculation to be performed in two (or more) threads simultaneously.

I think it could be a good idea to mark fieldsForInferenceIds as volatile. This shouldn't have any syncing overhead, besides any compiler optimizations that are disabled.

Makes sense, adding that

We need to have a single version of this map that is immutable and final. It's a small map so there's no need to compute all the flavours preemptively or even to cache them. The consumer can rebuild data structures on top if their access pattern is different.
Can we just keep a Map<String, FieldInference> where the keys are the semantic text fields and FieldInference contains the inference id and the list of source fields?
This way it's easy to extract the FieldInference for a field query.
For ingest we need to visit the entire map anyway so any shape is ok.

Just to be clear I mean only keeping ImmutableOpenMap<String, FieldInference> fieldInferenceMap and adding an accessor for it. That's all we need. All this caching/reverting is non-sense at this moment imo.

I see that this is a similar pattern to other Index Metadata related structures. Changing it

Mikep86 · 2024-03-13T21:30:26Z

server/src/main/java/org/elasticsearch/cluster/metadata/FieldInferenceMetadata.java

If I understand correctly, FieldInferenceMetadata is immutable and will be replaced with a new instance upon a mapping change. Thus, fieldsForInferenceIds (if requested) will be valid for the entire lifetime of the FieldInferenceMetadata instance.

Do I have that right?

Correct! A mapping update (or any cluster state change) means that we will have a new instance of FieldInferenceMetadata

…e maps

jimczi · 2024-03-14T21:19:59Z

server/src/main/java/org/elasticsearch/cluster/metadata/FieldInferenceMetadata.java

+    }
+
+    public Set<String> getSourceFields(String field) {
+        return getInferenceSafe(field, FieldInference::sourceFields);


Let's expose the fieldInferenceMap directly, we don't need to dictate how the access should look like.

jimczi · 2024-03-14T21:21:07Z

server/src/main/java/org/elasticsearch/cluster/metadata/FieldInferenceMetadata.java

+        }
+    }
+
+    public record FieldInference(String inferenceId, Set<String> sourceFields)


Maybe rename into FieldInferenceOptions since we'll probably need to add more later (shouldFailOnError, chunking, ...)?

Makes sense 👍

…ame FieldInference to FieldInferenceOptions

…osdelest/semantic-text-index-metadata-update

jimczi

LGTM

carlosdelest · 2024-03-19T08:28:57Z

@elasticmachine run elasticsearch-ci/part-1

carlosdelest · 2024-03-19T09:24:31Z

@elasticmachine run elasticsearch-ci/bwc-snapshots

carlosdelest · 2024-03-19T10:19:39Z

@elasticmachine run elasticsearch-ci/8.14.0 / bwc-snapshots

Mikep86 · 2024-03-19T15:28:02Z

server/src/test/java/org/elasticsearch/action/bulk/BulkOperationTests.java

+        FieldInferenceMetadata fieldInferenceMetadata = new FieldInferenceMetadata(
+            Map.of(
+                FIRST_INFERENCE_FIELD_SERVICE_1,
+                new FieldInferenceMetadata.FieldInferenceOptions(INFERENCE_SERVICE_1_ID, Set.of()),


If we want this test to reflect production, the source field set should never be empty. In this case, it should contain FIRST_INFERENCE_FIELD_SERVICE_1

Correct - this will be fixed when we start calculating inference for copy_to and multifields, as the way of retrieving the inference texts will start using the source information for fields 👍

Mikep86 · 2024-03-19T17:26:36Z

server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java

@@ -37,7 +37,7 @@ public void testEmpty() {
        assertNotNull(names);
        assertThat(names, hasSize(0));

-        Map<String, Set<String>> fieldsForModels = lookup.getFieldsForModels();
+        Map<String, String> fieldsForModels = lookup.getInferenceIdsForFields();


We should probably clean up/rename any vars named fieldsForModels or similar

Mikep86 · 2024-03-19T17:32:54Z

...rence/src/test/java/org/elasticsearch/cluster/metadata/SemanticTextClusterMetadataTests.java

-        assertEquals(Map.of("test_model", Set.of("field")), indexService.getMetadata().getFieldsForModels());
+        assertEquals(
+            indexService.getMetadata().getFieldInferenceMetadata().getFieldInferenceOptions().get("field").inferenceId(),
+            "test_model"


Nitpick: the assertEquals method signature is assertEquals(<expected>, <actual>). Reversing the args is the same functionally, but if the assertion fails it will generate a message with an incorrect expected value.

Well spotted! I'm changing that to Hamcrest assertions, which are easier to read 👍

Mikep86 and others added 30 commits March 13, 2024 19:07

Added skeleton code for SemanticQueryBuilder

da4a550

Add boost and query name to XContent

330f867

Add dimensions and similarity to ServiceSettings, create ModelSetting…

ed2abf7

…s class

Change implementation of asMap() to avoid extra nesting in inference …

1ee7653

…results

Fix BulkOperationTests

80c01c7

Fix tests of SemanticTextInferenceResultFieldMapper

a4c1184

Revert "Change implementation of asMap() to avoid extra nesting in in…

f28e051

…ference results" This reverts commit bd4e19e.

Add service extension for dense vector embeddings

0d515a4

Fix bug in model settings

d53583e

Fix spotless

1edab71

Refactored inference services with common abstract class

3c14e12

Added modelsForFields to QueryRewriteContext

adb7711

Updated IndicesService to create the modelsForFields map

ebb3827

Updated SemanticQueryBuilder to implement doRewrite

8a466a9

Added semanticQuery to SemanticTextFieldMapper

5d27f6a

Updated SemanticQueryBuilder to implement doToQuery

6abf24c

Updated SemanticQueryBuilder to add fromXContent

b434da5

Added SemanticQueryBuilder to inference plugin

ba852b8

Use Lucene queries to build semantic query

b2c4574

Spotless

1361f08

New class for dealing with field inference metadata

ddf374c

Include FieldInferenceMetadata into IndexMetadata

354bb09

Create new FieldInferenceMetadata structure

c88b9cd

Use FieldInferenceMetadata structure in lookups, some renaming

db7f531

Use FieldInferenceMetadata structure in dependencies

32293d0

Use FieldInferenceMetadata structure in dependencies

949a8d0

Renaming fields

21bf90b

Fix rebasing

a21e9e4

Fix rebasing

d14cc53

Fix rebasing

053080a

carlosdelest commented Mar 13, 2024

View reviewed changes

carlosdelest requested review from Mikep86 and jimczi March 13, 2024 19:07

Mikep86 reviewed Mar 13, 2024

View reviewed changes

carlosdelest added 8 commits March 14, 2024 17:05

Rework FieldInferenceMetadata to have a single map instead of multipl…

bb76d53

…e maps

Serialization fixes

83ccfd2

Test fixes

a222d79

Spotless

a026612

Fix serialization issues when inference is empty

f56db05

Fix test

0edc8d3

Use empty diff state to avoid bwc errors

ba6f00f

Fix parsing error, styling

f3a6af0

jimczi reviewed Mar 14, 2024

View reviewed changes

Remove accessors for FieldInferenceMetadata, use the map instead. Ren…

88af861

…ame FieldInference to FieldInferenceOptions

carlosdelest changed the title ~~WIP - semantic_text - extract Index Metadata inference information to separate class~~ semantic_text - extract Index Metadata inference information to separate class Mar 18, 2024

Spotless

3b8db71

carlosdelest marked this pull request as ready for review March 18, 2024 11:18

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Mar 18, 2024

carlosdelest requested a review from jimczi March 18, 2024 16:18

Merge remote-tracking branch 'origin/feature/semantic-text' into carl…

ce19bc9

…osdelest/semantic-text-index-metadata-update

jimczi approved these changes Mar 19, 2024

View reviewed changes

carlosdelest added the >non-issue label Mar 19, 2024

carlosdelest merged commit 3ca808b into elastic:feature/semantic-text Mar 19, 2024
13 of 15 checks passed

Mikep86 reviewed Mar 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

semantic_text - extract Index Metadata inference information to separate class #106328

semantic_text - extract Index Metadata inference information to separate class #106328

carlosdelest commented Mar 13, 2024 •

edited

Loading

carlosdelest Mar 13, 2024

carlosdelest Mar 13, 2024

carlosdelest Mar 13, 2024

carlosdelest Mar 13, 2024

Mikep86 left a comment

Mikep86 Mar 13, 2024

carlosdelest Mar 14, 2024

Mikep86 Mar 14, 2024

Mikep86 Mar 13, 2024

carlosdelest Mar 14, 2024

Mikep86 Mar 14, 2024

carlosdelest Mar 14, 2024

jimczi Mar 14, 2024

jimczi Mar 14, 2024

carlosdelest Mar 18, 2024

Mikep86 Mar 13, 2024

carlosdelest Mar 14, 2024

jimczi Mar 14, 2024

carlosdelest Mar 18, 2024

jimczi Mar 14, 2024

carlosdelest Mar 18, 2024 •

edited

Loading

jimczi left a comment

carlosdelest commented Mar 19, 2024

carlosdelest commented Mar 19, 2024

carlosdelest commented Mar 19, 2024

Mikep86 Mar 19, 2024

carlosdelest Mar 19, 2024

Mikep86 Mar 19, 2024

Mikep86 Mar 19, 2024

carlosdelest Mar 20, 2024

semantic_text - extract Index Metadata inference information to separate class #106328

semantic_text - extract Index Metadata inference information to separate class #106328

Conversation

carlosdelest commented Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikep86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosdelest Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

carlosdelest commented Mar 19, 2024

carlosdelest commented Mar 19, 2024

carlosdelest commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosdelest commented Mar 13, 2024 •

edited

Loading

carlosdelest Mar 18, 2024 •

edited

Loading