-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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?
Changes from 11 commits
66b167e
190d6e7
efefc59
ab3a1e9
ac2da85
566efba
177f4fe
7b2036f
bb43271
5eb4d29
893df83
3743de7
c0802fc
f06ca8a
33c3de7
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,5 @@ | ||
pr: 136120 | ||
summary: Allow updating `inference_id` of `semantic_text` fields | ||
area: "Mapping" | ||
type: enhancement | ||
issues: [] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -150,6 +150,7 @@ public class SemanticTextFieldMapper extends FieldMapper implements InferenceFie | |
public static final NodeFeature SEMANTIC_TEXT_SPARSE_VECTOR_INDEX_OPTIONS = new NodeFeature( | ||
"semantic_text.sparse_vector_index_options" | ||
); | ||
public static final NodeFeature SEMANTIC_TEXT_UPDATABLE_INFERENCE_ID = new NodeFeature("semantic_text.updatable_inference_id"); | ||
|
||
public static final String CONTENT_TYPE = "semantic_text"; | ||
public static final String DEFAULT_ELSER_2_INFERENCE_ID = DEFAULT_ELSER_ID; | ||
|
@@ -243,7 +244,7 @@ public Builder( | |
|
||
this.inferenceId = Parameter.stringParam( | ||
INFERENCE_ID_FIELD, | ||
false, | ||
true, | ||
mapper -> ((SemanticTextFieldType) mapper.fieldType()).inferenceId, | ||
DEFAULT_ELSER_2_INFERENCE_ID | ||
).addValidator(v -> { | ||
|
@@ -326,9 +327,68 @@ protected Parameter<?>[] getParameters() { | |
@Override | ||
protected void merge(FieldMapper mergeWith, Conflicts conflicts, MapperMergeContext mapperMergeContext) { | ||
SemanticTextFieldMapper semanticMergeWith = (SemanticTextFieldMapper) mergeWith; | ||
semanticMergeWith = copySettings(semanticMergeWith, mapperMergeContext); | ||
|
||
// We make sure to merge the inference field first to catch any model conflicts | ||
final boolean isInferenceIdUpdate = semanticMergeWith.fieldType().inferenceId.equals(inferenceId.get()) == false; | ||
final boolean hasExplicitModelSettings = modelSettings.get() != null; | ||
|
||
if (isInferenceIdUpdate && hasExplicitModelSettings) { | ||
validateModelsAreCompatibleWhenInferenceIdIsUpdated(semanticMergeWith.fieldType().inferenceId, conflicts); | ||
// As the mapper previously had explicit model settings, we need to apply to the new merged mapper | ||
// the resolved model settings if not explicitly set. | ||
semanticMergeWith = copyWithNewModelSettingsIfNotSet( | ||
semanticMergeWith, | ||
modelRegistry.getMinimalServiceSettings(semanticMergeWith.fieldType().inferenceId), | ||
mapperMergeContext | ||
); | ||
} | ||
|
||
semanticMergeWith = copyModelSettingsIfNotSet(semanticMergeWith, mapperMergeContext); | ||
dimitris-athanasiou marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
// We make sure to merge the inference field first to catch any model conflicts. | ||
// If inference_id is updated and there are no explicit model settings, we should be | ||
// able to switch to the new inference field without the need to check for conflicts. | ||
if (isInferenceIdUpdate == false || hasExplicitModelSettings) { | ||
mergeInferenceField(mapperMergeContext, semanticMergeWith); | ||
} | ||
|
||
super.merge(semanticMergeWith, conflicts, mapperMergeContext); | ||
conflicts.check(); | ||
} | ||
|
||
private void validateModelsAreCompatibleWhenInferenceIdIsUpdated(String newInferenceId, Conflicts conflicts) { | ||
MinimalServiceSettings currentModelSettings = modelSettings.get(); | ||
MinimalServiceSettings updatedModelSettings = modelRegistry.getMinimalServiceSettings(newInferenceId); | ||
if (currentModelSettings != null && updatedModelSettings == null) { | ||
throw new IllegalArgumentException( | ||
"Cannot merge [" | ||
dimitris-athanasiou marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
+ CONTENT_TYPE | ||
+ "] field [" | ||
+ leafName() | ||
+ "] because inference endpoint [" | ||
+ newInferenceId | ||
+ "] does not exist." | ||
); | ||
} | ||
if (canMergeModelSettings(currentModelSettings, updatedModelSettings, conflicts) == false) { | ||
dimitris-athanasiou marked this conversation as resolved.
Show resolved
Hide resolved
|
||
throw new IllegalArgumentException( | ||
"Cannot merge [" | ||
dimitris-athanasiou marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
+ CONTENT_TYPE | ||
+ "] field [" | ||
+ leafName() | ||
+ "] because inference endpoint [" | ||
+ inferenceId.get() | ||
+ "] with model settings [" | ||
+ currentModelSettings | ||
+ "] is not compatible with new inference endpoint [" | ||
+ newInferenceId | ||
+ "] with model settings [" | ||
+ updatedModelSettings | ||
+ "]." | ||
); | ||
} | ||
} | ||
|
||
private void mergeInferenceField(MapperMergeContext mapperMergeContext, SemanticTextFieldMapper semanticMergeWith) { | ||
try { | ||
var context = mapperMergeContext.createChildContext(semanticMergeWith.leafName(), ObjectMapper.Dynamic.FALSE); | ||
var inferenceField = inferenceFieldBuilder.apply(context.getMapperBuilderContext()); | ||
|
@@ -341,9 +401,6 @@ protected void merge(FieldMapper mergeWith, Conflicts conflicts, MapperMergeCont | |
: ""; | ||
throw new IllegalArgumentException(errorMessage, e); | ||
} | ||
|
||
super.merge(semanticMergeWith, conflicts, mapperMergeContext); | ||
conflicts.check(); | ||
} | ||
|
||
/** | ||
|
@@ -499,18 +556,35 @@ private void validateIndexOptions(SemanticTextIndexOptions indexOptions, String | |
} | ||
|
||
/** | ||
* As necessary, copy settings from this builder to the passed-in mapper. | ||
* As necessary, copy model settings from this builder to the passed-in mapper. | ||
* Used to preserve {@link MinimalServiceSettings} when updating a semantic text mapping to one where the model settings | ||
* are not specified. | ||
* | ||
* @param mapper The mapper | ||
* @return A mapper with the copied settings applied | ||
*/ | ||
private SemanticTextFieldMapper copySettings(SemanticTextFieldMapper mapper, MapperMergeContext mapperMergeContext) { | ||
private SemanticTextFieldMapper copyModelSettingsIfNotSet(SemanticTextFieldMapper mapper, MapperMergeContext mapperMergeContext) { | ||
return copyWithNewModelSettingsIfNotSet(mapper, modelSettings.getValue(), mapperMergeContext); | ||
} | ||
|
||
/** | ||
* Creates a new mapper with the new model settings if model settings are not set on the mapper. | ||
* If the mapper already has model settings or the new model settings are null, the mapper is | ||
* returned unchanged. | ||
* | ||
* @param mapper The mapper | ||
* @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 commentThe 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 commentThe 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 |
||
SemanticTextFieldMapper mapper, | ||
@Nullable MinimalServiceSettings modelSettings, | ||
MapperMergeContext mapperMergeContext | ||
) { | ||
SemanticTextFieldMapper returnedMapper = mapper; | ||
if (mapper.fieldType().getModelSettings() == null) { | ||
Builder builder = from(mapper); | ||
builder.setModelSettings(modelSettings.getValue()); | ||
builder.setModelSettings(modelSettings); | ||
returnedMapper = builder.build(mapperMergeContext.getMapperBuilderContext()); | ||
} | ||
|
||
|
@@ -784,6 +858,11 @@ protected void doValidate(MappingLookup mappers) { | |
} | ||
} | ||
|
||
@Override | ||
protected void checkIncomingMergeType(FieldMapper mergeWith) { | ||
super.checkIncomingMergeType(mergeWith); | ||
} | ||
dimitris-athanasiou marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
public static class SemanticTextFieldType extends SimpleMappedFieldType { | ||
private final String inferenceId; | ||
private final String searchInferenceId; | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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 :)