-
Notifications
You must be signed in to change notification settings - Fork 25.7k
[ML] Adding elser default endpoint for EIS #122066
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
[ML] Adding elser default endpoint for EIS #122066
Conversation
…uttner/elasticsearch into ml-eis-elser-default-endpoint
|
Pinging @elastic/ml-core (Team:ML) |
| private Set<String> getAuthorizedDefaultModelIds(ElasticInferenceServiceAuthorization auth) { | ||
| var authorizedModels = auth.getAuthorizedModelIds(); | ||
| var authorizedDefaultModelIds = new HashSet<>(defaultModelsConfigs.keySet()); | ||
| var authorizedDefaultModelIds = new TreeSet<>(defaultModelsConfigs.keySet()); |
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.
Using a TreeSet to ensure sorting for tests
| configuration = new Configuration(authRef.get().taskTypesAndModels.getAuthorizedTaskTypes()); | ||
|
|
||
| defaultConfigIds().forEach(modelRegistry::addDefaultIds); | ||
| defaultConfigIds().forEach(modelRegistry::putDefaultIdIfAbsent); |
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.
The reason I'm using putDefaultIdIfAbsent is because we have some integration tests that use the same model registry to test revoking the authorization. addDefaultIds throws an IllegalStateException when you attempt to add the same default id which for tests we don't want to do.
In production this shouldn't be an issue because this will only be called when a node starts which will have a completely empty model registry anyway.
|
Hi @jonathan-buttner, I've created a changelog YAML for you. |
maxjakob
left a comment
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.
The values look right 👍
For code changes I would defer to @davidkyle
davidkyle
left a comment
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.
LGTM
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
* Adding elser default endpoint * [CI] Auto commit changes from spotless * Fixing test and allowing duplicate calls * [CI] Auto commit changes from spotless * Update docs/changelog/122066.yaml --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> (cherry picked from commit b9d1222) # Conflicts: # x-pack/plugin/inference/qa/inference-service-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceGetModelsWithElasticInferenceServiceIT.java
* Adding elser default endpoint * [CI] Auto commit changes from spotless * Fixing test and allowing duplicate calls * [CI] Auto commit changes from spotless * Update docs/changelog/122066.yaml --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> (cherry picked from commit b9d1222) # Conflicts: # x-pack/plugin/inference/qa/inference-service-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceGetModelsWithElasticInferenceServiceIT.java
This PR adds the elser v2 default inference endpoint for EIS.
model id:
elser-v2default endpoint id:
.elser-v2-elasticTesting
Request using the default endpoint
Example response