[feature/semantic-text] semantic text copy to support #106689

carlosdelest · 2024-03-22T16:46:06Z

Adds copy_to support to semantic_text.

Changes:

Iterate on the source fields for calculating inference
Allow inference to be applied from multiple responses to a single field

…erver (elastic#105012)" This reverts commit f4d3ab9.

…copy-to-support-inference # Conflicts: # server/src/main/java/org/elasticsearch/action/bulk/BulkShardRequestInferenceProvider.java # server/src/test/java/org/elasticsearch/action/bulk/BulkOperationTests.java

…ject mappers can provide underlying field mappers

…s and object mappers can provide underlying field mappers" This reverts commit 05aa06f.

…copy-to-support-inference # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferencePlugin.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilter.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/mapper/InferenceResultFieldMapper.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextModelSettings.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilterTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/mapper/InferenceResultFieldMapperTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java # x-pack/plugin/inference/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_inference.yml # x-pack/plugin/inference/src/yamlRestTest/resources/rest-api-spec/test/inference/20_semantic_text_field_mapper.yml

carlosdelest · 2024-03-27T15:39:51Z

I'm dropping support for multifields after our conversations, so this PR is now adding just copy_to support.

@jimczi , @Mikep86 - should we double check that all source fields are provided when doing an update, so we can redo inference on them?

I would say no for the moment. I can think of having source fields that are potentially null, and thus users would not have an easy way out of it. We could include docs that include the limitations for copy_to behaviour in semantic_text. WDYT?

…copy-to-support-inference

Mikep86 · 2024-03-27T16:17:28Z

@carlosdelest

I'm dropping support for multifields after our conversations, so this PR is now adding just copy_to support.

@jimczi , @Mikep86 - should we double check that all source fields are provided when doing an update, so we can redo inference on them?

I would say no for the moment. I can think of having source fields that are potentially null, and thus users would not have an easy way out of it. We could include docs that include the limitations for copy_to behaviour in semantic_text. WDYT?

I agree. Partial update handling has some more moving parts, such as attributing each chunk to the source that generated it, that are best left to a separate PR.

carlosdelest · 2024-03-27T16:46:07Z

test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java

@@ -1030,7 +1030,7 @@ public final void testMinimalIsInvalidInRoutingPath() throws IOException {
        }
    }

-    private String minimalIsInvalidRoutingPathErrorMessage(Mapper mapper) {
+    protected String minimalIsInvalidRoutingPathErrorMessage(Mapper mapper) {


I had to take back this to protected so it can be overriden by our tests

carlosdelest · 2024-03-27T16:46:43Z

...ain/java/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilter.java

-                            item.id(),
-                            new FieldInferenceResponseAccumulator(
+                    String fieldName = entry.getKey();
+                    for (var sourceField : entry.getValue().sourceFields()) {


Now we iterate on all the source fields for retrieving inference - so we're adding an additional loop here

carlosdelest · 2024-03-27T16:47:15Z

...nce/src/main/java/org/elasticsearch/xpack/inference/mapper/InferenceMetadataFieldMapper.java

-        Map<String, Object> fieldMap = new LinkedHashMap<>();
-        fieldMap.put(INFERENCE_ID, model.getInferenceEntityId());
+
+        Map<String, Object> fieldMap = (Map<String, Object>) inferenceMap.computeIfAbsent(field, s -> new LinkedHashMap<>());


Multiple inference results can be applied to a single field from each source field - so we need to be prepared for that

carlosdelest · 2024-03-27T16:49:34Z

...rence/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_inference.yml

@@ -310,3 +310,38 @@ setup:
        id: doc_1
        body:
          non_inference_field: "non inference test"
+


Any other tests come to mind?

carlosdelest · 2024-03-27T16:50:05Z

@elasticmachine run elasticsearch-ci/part-2

elasticsearchmachine · 2024-03-27T17:59:08Z

Pinging @elastic/es-search (Team:Search)

…copy-to-support-inference # Conflicts: # server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilter.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/cluster/metadata/SemanticTextClusterMetadataTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilterTests.java # x-pack/plugin/inference/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_inference.yml

Mikep86

Looks good overall, just one question I sign off on it

...ain/java/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilter.java

…_text field

...ain/java/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilter.java

carlosdelest · 2024-04-01T18:58:43Z

...ain/java/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilter.java

+                        var value = XContentMapValues.extractValue(sourceField, docMap);
+                        if (value == null) {
+                            if (isUpdateRequest) {
+                                addInferenceResponseFailure(


I've added this check for disallowing updates that do not use all source fields. WDYT?

This looks good, except for one edge case: Say that a user wants to use an update request to delete a field from a document. Traditionally, they would do this by explicitly setting the field value to null. How do we tell the difference between the null we get when the user didn't set a value and the null we get when the user explicitly set it?

IMO since we have a way to support partial updates in the near future (chunk source attribution), we could just live with this edge case for now.

Mikep86

LGTM!

Mikep86 · 2024-04-01T20:10:57Z

...ain/java/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilter.java

+                        var value = XContentMapValues.extractValue(sourceField, docMap);
+                        if (value == null) {
+                            if (isUpdateRequest) {
+                                addInferenceResponseFailure(


This looks good, except for one edge case: Say that a user wants to use an update request to delete a field from a document. Traditionally, they would do this by explicitly setting the field value to null. How do we tell the difference between the null we get when the user didn't set a value and the null we get when the user explicitly set it?

IMO since we have a way to support partial updates in the near future (chunk source attribution), we could just live with this edge case for now.

carlosdelest · 2024-04-02T14:38:05Z

Merging this after checking with Jim and Mike.

We can change the way we deal with missing source fields later.

jimczi and others added 20 commits March 14, 2024 01:52

Revert "Extract interface from ModelRegistry so it can be used from s…

7aaa3b6

…erver (elastic#105012)" This reverts commit f4d3ab9.

inference as an action filter

ebc26d2

add more tests

86ddc9d

Merge branch 'feature/semantic-text' into carlosdelest/semantic-text-…

dd73d01

…copy-to-support-inference # Conflicts: # server/src/main/java/org/elasticsearch/action/bulk/BulkShardRequestInferenceProvider.java # server/src/test/java/org/elasticsearch/action/bulk/BulkOperationTests.java

Merge from feature branch

c5de0da

Allow SemanticTextFieldMapper to be a multifield

cf62b1b

Add multifields / copy_to tests to lookup and metadata

f029015

First iteration for adding inference support for copy_to / multifields

d3f9d86

Add tests for copy_to

140caa3

Spotless

7d1c92a

Minor changes from previous PR

e023a19

Add getMapper(field) to Mapper, so both field with multifields and ob…

05aa06f

…ject mappers can provide underlying field mappers

multifields support

c80677f

Add copy_to support

5896c60

Spotless

df0cc90

Fix tests

6d4bbf3

Remove the need to get mappings by using MappingLookup.getFieldType()

068615a

Revert "Add getMapper(field) to Mapper, so both field with multifield…

5c3d9c4

…s and object mappers can provide underlying field mappers" This reverts commit 05aa06f.

PR feedback

ace17dd

carlosdelest changed the title ~~Carlosdelest/semantic text copy to support inference~~ semantic text copy to / multifield support Mar 26, 2024

carlosdelest added 3 commits March 26, 2024 16:40

Fix merge with feature branch

47a22d7

Fix merge with feature branch

5311424

Remove multifield testing

707b3f1

carlosdelest changed the title ~~semantic text copy to / multifield support~~ semantic text copy to support Mar 27, 2024

carlosdelest added 2 commits March 27, 2024 16:47

Now, actually drop multifields support

5e5b32a

Merge branch 'feature/semantic-text' into carlosdelest/semantic-text-…

1abf95c

…copy-to-support-inference

Fix merge with main

29dd33e

carlosdelest commented Mar 27, 2024

View reviewed changes

carlosdelest marked this pull request as ready for review March 27, 2024 17:50

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

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

mark-vieira added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Mar 27, 2024

elasticsearchmachine added the Team:Search Meta label for search team label Mar 27, 2024

carlosdelest changed the title ~~semantic text copy to support~~ [feature/semantic-text] semantic text copy to support Mar 27, 2024

elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Mar 27, 2024

carlosdelest added 2 commits April 1, 2024 10:00

Fix merge with feature branch

82ffb5b

carlosdelest added the v8.14.0 label Apr 1, 2024

carlosdelest requested review from jimczi and Mikep86 April 1, 2024 09:37

Mikep86 reviewed Apr 1, 2024

View reviewed changes

...ain/java/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilter.java Outdated Show resolved Hide resolved

carlosdelest added 2 commits April 1, 2024 20:34

Check that bulk updates use all source fields for updating a semantic…

6b26200

…_text field

Add check for inference results when no value is provided

bf5b837

carlosdelest commented Apr 1, 2024

View reviewed changes

carlosdelest requested a review from Mikep86 April 1, 2024 18:58

Mikep86 approved these changes Apr 1, 2024

View reviewed changes

carlosdelest merged commit b6ca8d2 into elastic:feature/semantic-text Apr 2, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature/semantic-text] semantic text copy to support #106689

[feature/semantic-text] semantic text copy to support #106689

carlosdelest commented Mar 22, 2024 •

edited

Loading

carlosdelest commented Mar 27, 2024

Mikep86 commented Mar 27, 2024

carlosdelest Mar 27, 2024

carlosdelest Mar 27, 2024

carlosdelest Mar 27, 2024

carlosdelest Mar 27, 2024

carlosdelest commented Mar 27, 2024

elasticsearchmachine commented Mar 27, 2024

Mikep86 left a comment

carlosdelest Apr 1, 2024

Mikep86 Apr 1, 2024

Mikep86 left a comment

Mikep86 Apr 1, 2024

carlosdelest commented Apr 2, 2024

[feature/semantic-text] semantic text copy to support #106689

[feature/semantic-text] semantic text copy to support #106689

Conversation

carlosdelest commented Mar 22, 2024 • edited Loading

carlosdelest commented Mar 27, 2024

Mikep86 commented Mar 27, 2024

carlosdelest Mar 27, 2024

Choose a reason for hiding this comment

carlosdelest Mar 27, 2024

Choose a reason for hiding this comment

carlosdelest Mar 27, 2024

Choose a reason for hiding this comment

carlosdelest Mar 27, 2024

Choose a reason for hiding this comment

carlosdelest commented Mar 27, 2024

elasticsearchmachine commented Mar 27, 2024

Mikep86 left a comment

Choose a reason for hiding this comment

carlosdelest Apr 1, 2024

Choose a reason for hiding this comment

Mikep86 Apr 1, 2024

Choose a reason for hiding this comment

Mikep86 left a comment

Choose a reason for hiding this comment

Mikep86 Apr 1, 2024

Choose a reason for hiding this comment

carlosdelest commented Apr 2, 2024

carlosdelest commented Mar 22, 2024 •

edited

Loading