[Inference API] Update authorized endpoints when their fingerprint or version changed#143567
Conversation
… version changed This PR modifies the `AuthorizationPoller` so that it selects to persist endpoints that: - are new - have a different fingerprint - have a newer version Updating endpoints that have a different fingerprint allows dynamic updates when EIS modifies the endpoint metadata. Updating endpoints that have a different version allows persisting `EndpointMetadata` fields that could not be parsed before. In addition, in this PR I have removed the logic for handling out-of-sync endpoints added in elastic#138934 as it is no longer needed. This was necessary for authorized endpoints only. For those, if they are not in cluster state we'll now overwrite the doc in the index and update the cluster state. This ammends the out-of-sync problem if it ever occurs.
|
Pinging @elastic/search-inference-team (Team:Search - Inference) |
|
Hi @dimitris-athanasiou, I've created a changelog YAML for you. |
jonathan-buttner
left a comment
There was a problem hiding this comment.
Looking good! Left a few questions and suggestions.
| ); | ||
| } | ||
|
|
||
| private void testEndpointGetsUpdated_GivenFingerprintChanged(String originalFingerprint, String updatedFingerprint) throws Exception { |
There was a problem hiding this comment.
Just a heads up I've struggled to get these type of tests to succeed reliably. A number of them have been muted: #138012
If you have time, another set of eyes would be good to figure out why they are so flaky. I've tried to fix them a few times 😞
There was a problem hiding this comment.
I did notice those issues. I wonder if #143584 might have been some of that. But I couldn't find full logs. Next time we get a failure I'll dive in.
.../internalClusterTest/java/org/elasticsearch/xpack/inference/integration/ModelRegistryIT.java
Outdated
Show resolved
Hide resolved
|
|
||
| var modelsWithoutDuplicates = models.stream().distinct().toList(); | ||
|
|
||
| Set<String> duplicateInferenceIds = findDuplicateInferenceIds(modelsWithoutDuplicates); |
There was a problem hiding this comment.
In a situation where we receive multiple models with different definitions but the same inference id we could apply them all right? That certainly would be inefficient but I don't think it'd cause errors right? If we consider this functionality similar to elasticsearch's _bulk api I think we could return successes.
There was a problem hiding this comment.
The handling we do here would need to change if we accepted duplicate models.
But I don't see anything that stops us from changing this. Then again, given this is only an internal action we control, I wonder if keeping the duplicate check would help us pick up some issue.
Not sure. Do you have a preference?
There was a problem hiding this comment.
We have discussed this offline and decided to keep the duplicate id check in. In addition, we'll remove the var modelsWithoutDuplicates = models.stream().distinct().toList(); line as it is no longer necessary.
.../internalClusterTest/java/org/elasticsearch/xpack/inference/integration/ModelRegistryIT.java
Show resolved
Hide resolved
...plugin/inference/src/main/java/org/elasticsearch/xpack/inference/registry/ModelRegistry.java
Outdated
Show resolved
Hide resolved
...plugin/inference/src/main/java/org/elasticsearch/xpack/inference/registry/ModelRegistry.java
Show resolved
Hide resolved
|
@jonathan-buttner I have addressed review feedback. This is ready for another look. |
| ResourceNotFoundException e = expectThrows( | ||
| ResourceNotFoundException.class, | ||
| () -> modelRegistry.getMinimalServiceSettings( | ||
| Set.of("model_id_" + randomIntBetween(0, createdModels.size() - 1), "non_matching_id"), |
There was a problem hiding this comment.
How about we move the string id logic to a helper method since we use it a couple times?
… version changed (elastic#143567) This PR modifies the `AuthorizationPoller` so that it selects to persist endpoints that: - are new - have a different fingerprint - have a newer version Updating endpoints that have a different fingerprint allows dynamic updates when EIS modifies the endpoint metadata. Updating endpoints that have a different version allows persisting `EndpointMetadata` fields that could not be parsed before. In addition, in this PR I have removed the logic for handling out-of-sync endpoints added in elastic#138934 as it is no longer needed. This was necessary for authorized endpoints only. For those, if they are not in cluster state we'll now overwrite the doc in the index and update the cluster state. This ammends the out-of-sync problem if it ever occurs.
This fixes debug log messages added in elastic#143567 where the endpoint id is not correctly included.
) This fixes debug log messages added in #143567 where the endpoint id is not correctly included.
… version changed (elastic#143567) This PR modifies the `AuthorizationPoller` so that it selects to persist endpoints that: - are new - have a different fingerprint - have a newer version Updating endpoints that have a different fingerprint allows dynamic updates when EIS modifies the endpoint metadata. Updating endpoints that have a different version allows persisting `EndpointMetadata` fields that could not be parsed before. In addition, in this PR I have removed the logic for handling out-of-sync endpoints added in elastic#138934 as it is no longer needed. This was necessary for authorized endpoints only. For those, if they are not in cluster state we'll now overwrite the doc in the index and update the cluster state. This ammends the out-of-sync problem if it ever occurs.
…tic#143743) This fixes debug log messages added in elastic#143567 where the endpoint id is not correctly included.
This PR modifies the
AuthorizationPollerso that it selects to persist endpoints that:Updating endpoints that have a different fingerprint allows dynamic updates when EIS modifies the endpoint metadata.
Updating endpoints that have a different version allows persisting
EndpointMetadatafields that could not be parsed before.In addition, in this PR I have removed the logic for handling out-of-sync endpoints added in #138934 as it is no longer needed. This was necessary for authorized endpoints only. For those, if they are not in cluster state we'll now overwrite the doc in the index and update the cluster state. This ammends the out-of-sync problem if it ever occurs.