-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Allow updating inference_id of semantic_text fields #136120
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
base: main
Are you sure you want to change the base?
Allow updating inference_id of semantic_text fields #136120
Conversation
Previously the `inference_id` of `semantic_text` fields was not updatable. This commit allows users to update the `inference_id` of a `semantic_text` field. This is particularly useful for scenarios where the user wants to switch to using the same model but from a different service. There are two circumstances when the update is allowed. - No values have been written for the `semantic_text` field. The inference endpoint can be changed freely as there is no need for compatibility between the current and the new endpoint. - The new inference endpoint is compatible with the previous one. The `model_settings` of the new inference endpoint are compatible with those of the current endpoint, thus the update is allowed.
Pinging @elastic/search-relevance (Team:Search - Relevance) |
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
Hi @dimitris-athanasiou, I've created a changelog YAML for you. |
PR is ready for review. However, I intend to add documentation changes soon. |
docs/changelog/136120.yaml
Outdated
@@ -0,0 +1,5 @@ | |||
pr: 136120 | |||
summary: Allow updating `inference_id` of `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.
Search
or 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.
I would say mapping
🔍 Preview links for changed docs |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
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.
Couple of very minor wording suggestions from me :)
docs/reference/elasticsearch/mapping-reference/semantic-text.md
Outdated
Show resolved
Hide resolved
docs/reference/elasticsearch/mapping-reference/semantic-text.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Liam Thompson <[email protected]>
Co-authored-by: Liam Thompson <[email protected]>
to create the endpoint. If `search_inference_id` is specified, the {{infer}} | ||
endpoint will only be used at index time. | ||
|
||
::::{applies-switch} |
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.
Forgot to say that I think you can indent this whole applies-switch section a little bit, otherwise LGTM :)
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 can't seem to be able to do 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.
no worries :)
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! I've left a few comments but I think it's pretty close.
docs/changelog/136120.yaml
Outdated
@@ -0,0 +1,5 @@ | |||
pr: 136120 | |||
summary: Allow updating `inference_id` of `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.
I would say mapping
docs/reference/elasticsearch/mapping-reference/semantic-text.md
Outdated
Show resolved
Hide resolved
...rc/main/java/org/elasticsearch/xpack/inference/mock/TestSparseInferenceServiceExtension.java
Outdated
Show resolved
Hide resolved
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Show resolved
Hide resolved
* @param modelSettings the new model settings. If null the mapper will be returned unchanged. | ||
* @return A mapper with the copied settings applied | ||
*/ | ||
private SemanticTextFieldMapper copyWithNewModelSettingsIfNotSet( |
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.
So we're doing a null check here before we copy the settings, but we silently ignore if that case happens. Should we throw if it's not null and this method is called?
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 could be that it's not null because the update itself contains model_settings
. In that case the explicitly set model_settings
take precedence.
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.
Didn't review this file, assume it's the same as the other yaml test
...e/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_field_mapping.yml
Show resolved
Hide resolved
Co-authored-by: Kathleen DeRusso <[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.
Partial review, didn't get to the tests. I'll pick it up next week.
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Mike Pellegrini <[email protected]>
Previously the
inference_id
ofsemantic_text
fields was not updatable. This commit allows users to update theinference_id
of asemantic_text
field. This is particularly useful for scenarios where the user wants to switch to using the same model but from a different service.There are two circumstances when the update is allowed.
semantic_text
field.The inference endpoint can be changed freely as there is no need for compatibility between the current and the new endpoint.
The
model_settings
of the new inference endpoint are compatible with those of the current endpoint, thus the update is allowed.