Add implementation to update service settings method for AI21 service#142597
Conversation
|
Pinging @elastic/search-inference-team (Team:Search - Inference) |
| * @return the updated service settings | ||
| */ | ||
| default ServiceSettings updateServiceSettings(Map<String, Object> serviceSettings, TaskType taskType) { | ||
| return this; |
There was a problem hiding this comment.
Keeping this default implementation as is in order to avoid having to implement for every service at once
| ); | ||
|
|
||
| Model mergedParsedModel = service.get().buildModelFromConfigAndSecrets(mergedModelConfigurations, mergedModelSecrets); | ||
| if (mergedParsedModel.equals(existingParsedModel)) { |
There was a problem hiding this comment.
This change was requested by @DonalEvans here: #140082 (comment)
| * @param taskType the task type for which the service settings are being updated | ||
| * @return the updated service settings | ||
| */ | ||
| default ServiceSettings updateServiceSettings(Map<String, Object> serviceSettings, TaskType taskType) { |
There was a problem hiding this comment.
Adding TaskType param in order to support the Custom Service Settings
There was a problem hiding this comment.
Rather than changing this interface, would it be possible to add a TaskType field to CustomServiceSettings that gets set when they're first created? Task type is not something that can be updated, so it seems like it should be safe to copy the task type from the old settings into the new settings when doing an update.
There was a problem hiding this comment.
That is a good idea. Made the change in the original PR . I believe it would work. Removed changes related to Custom Service from this PR.
|
|
||
| // Rate limit for AI21 is 10 requests / sec or 200 requests / minute. Setting default to 200 requests / minute | ||
| protected static final RateLimitSettings DEFAULT_RATE_LIMIT_SETTINGS = new RateLimitSettings(200); | ||
| private static final RateLimitSettings DEFAULT_RATE_LIMIT_SETTINGS = new RateLimitSettings(200); |
There was a problem hiding this comment.
It's not used outside of this class so there's no need in keeping it protected.
|
|
||
| var extractedRateLimitSettings = RateLimitSettings.of( | ||
| serviceSettings, | ||
| this.rateLimitSettings, |
There was a problem hiding this comment.
Using existing RateLimitSettings as default value
| config.getTaskType(), | ||
| config.getService(), | ||
| ConfigurationParseContext.PERSISTENT | ||
| ConfigurationParseContext.REQUEST |
There was a problem hiding this comment.
Usage of REQUEST instead of PERSISTENT allows RateLimitSettings to be validated during model creation and makes more sense semantically.
|
|
||
| @Override | ||
| public ServiceSettings updateServiceSettings(Map<String, Object> serviceSettings) { | ||
| public ServiceSettings updateServiceSettings(Map<String, Object> serviceSettings, TaskType taskType) { |
There was a problem hiding this comment.
No changes to behavior, just minor refactoring and updating the param set
| serviceSettings = new HashMap<>(serviceSettings); | ||
|
|
||
| Integer maxBatchSize = ElasticInferenceServiceSettingsUtils.parseMaxBatchSize(serviceSettings, validationException); | ||
| var extractedMaxBatchSize = ElasticInferenceServiceSettingsUtils.parseMaxBatchSize(serviceSettings, validationException); |
There was a problem hiding this comment.
Existing variable name hides the class field. That's bad practice.
| } | ||
|
|
||
| var numAllocations = extractOptionalPositiveInteger( | ||
| var extractedNumAllocations = extractOptionalPositiveInteger( |
There was a problem hiding this comment.
Same issue with existing variable name hiding the class field
| verifyNoModelRegistryMutations(); | ||
| } | ||
|
|
||
| public void testMasterOperation_UpdatedModelIsEqualToExistingModel_ValidationAndUpdateIsSkipped() { |
There was a problem hiding this comment.
New test to check early return logic
...csearch/xpack/inference/services/ai21/completion/Ai21ChatCompletionServiceSettingsTests.java
Show resolved
Hide resolved
| assertThat("update should create a new instance", updatedInstance, not(equalTo(testInstance))); | ||
| assertThat(updatedInstance.getClass(), equalTo(testInstance.getClass())); | ||
| assertThat(updatedInstance, instanceOf(ElasticsearchInternalServiceSettings.class)); | ||
| var updatedElasticSearchInternalServiceSettings = (ElasticsearchInternalServiceSettings) updatedInstance; |
There was a problem hiding this comment.
This change is caused by the fact that ElasticsearchInternalServiceSettings now returns ElasticsearchInternalServiceSettings and there's no need in such casting
…ice-settings-method-ai-21
… service settings classes
|
Hi @DonalEvans |
This PR is a part of a broader initiative related to endpoint _update operation: #122356
In particular - this PR provides implementation for
updateServiceSettingsmethod used in_updateoperation.Initially the changes present in this PR were a part of a bigger PR #140082, but following @jonathan-buttner's request a smaller PR was created in order to simplify the review process.
API reference for the update operation can be found here: https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-inference-update
Currently Service Settings
updateServiceSettingsmethod being called during endpoint update operation is implemented only forGoogleVertexAiEmbeddingsServiceSettingsandElasticsearchInternalServiceSettings.This PR makes changes to these existing method implementations with minor refactoring, also providing the new method implementation for AI21 service.
Default implementation in
ServiceSettingsinterface still returns unchangedthisinstance. This is done in order to avoid making changes to every provider at once.CustomServiceSettingsrequiretaskTypeparameter so it was added to the method signature.model_idfield in AI21 request service settings map is immutable, meaning it cannot be updated with _update operation. Decision was made here: #140082 (comment)Since this was previously a part of a bigger PR with a lot of refactoring - some of this refactoring is present here as well.
gradle check?