-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Default semantic_text fields to ELSER on EIS when available #134708
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?
Default semantic_text fields to ELSER on EIS when available #134708
Conversation
Hey @ioanatia! 👋 Just implemented the dynamic ELSER default selection for semantic_text fields. The approach is intentionally minimal -leverages existing A few things I'd especially appreciate feedback on:
Still draft mode, but wanted to get your input before finalizing. Thanks! |
...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
relying on but I think we need more tests. |
In terms of tests, what we can also do is to add another mock service in https://github.com/elastic/elasticsearch/tree/main/x-pack/plugin/inference/qa/test-service-plugin/src/main/java/org/elasticsearch/xpack/inference/mock and then use it in yaml tests. the InferenceService allows to register default endpoints: elasticsearch/server/src/main/java/org/elasticsearch/inference/InferenceService.java Line 229 in 66582a3
I think we can mock the default endpoints we have in EIS and use the same name in the mock inference service. |
...rc/main/java/org/elasticsearch/xpack/inference/services/elastic/ElasticInferenceService.java
Show resolved
Hide resolved
Hi @mridula-s109, I've created a changelog YAML for you. |
Pinging @elastic/search-relevance (Team:Search - Relevance) |
Thanks @mridula-s109 ! Can we merge this around Oct 22-24? This would mean it goes live on serverless Monday 27, ECH in 9.3, and fits our rollout plan cc @maxjakob for visibility |
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.
Looking better! I left some comments about how to clean up the tests. IMO the biggest thing left is the default inference ID assertion in InferenceSemanticTextIT
. If that is truly returning variable values, it could be an indicator of something we need to address. Or it could be a race condition in a flaky test 😁 . Best to characterize it and address it early.
...e-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceSemanticTextIT.java
Outdated
Show resolved
Hide resolved
...e-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceSemanticTextIT.java
Outdated
Show resolved
Hide resolved
...e-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceSemanticTextIT.java
Outdated
Show resolved
Hide resolved
...e-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceSemanticTextIT.java
Outdated
Show resolved
Hide resolved
...e-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceSemanticTextIT.java
Outdated
Show resolved
Hide resolved
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java
Outdated
Show resolved
Hide resolved
...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java
Outdated
Show resolved
Hide resolved
...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java
Outdated
Show resolved
Hide resolved
FYI folks we're working on some rate limiting issues with ELSER and we may want to defer this. Please check with @maxjakob or myself before merging. |
Thanks for letting me know @seanhandley , will do the same! |
Thanks @Mikep86! All comments addressed. Have verified about the default assertion in the SemanticTextEISDefaultIT, there is no flakiness or underlying issue at the moment. Since we're holding off on merging due to the ELSER rate limiting work, no rush on further review. Take your time! 😊 |
...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java
Outdated
Show resolved
Hide resolved
...e-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceSemanticTextIT.java
Outdated
Show resolved
Hide resolved
* | ||
* My understanding is that the @Before will be run after the node starts up and wouldn't be sufficient to handle | ||
* this scenario. That is why this needs to be @BeforeClass. | ||
*/ |
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 was initially unsure about keeping this comment due to potential redundancy, but decided to leave it as-is since the added verbosity makes it more explanatory.
@ioanatia @Mikep86 quick update: I’ve addressed the review feedback and pushed the latest changes. |
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 to merge once we get the 👍 from @seanhandley and @maxjakob
@Before | ||
public void setUp() throws Exception { | ||
super.setUp(); | ||
// Ensure the mock EIS server has an authorized response ready before each test | ||
mockEISServer.enqueueAuthorizeAllModelsResponse(); | ||
} |
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.
Based on my understanding of the conversation in #134708 (comment) (and the linked references), I think we only need the @BeforeClass
annotated method. However, this additional @Before
method will be harmless at worst, so nothing to block over. As @ioanatia said earlier, we can sync with the ML team and clean this up later.
🚨 Please check with search inference team on when to merge this - we're working out some rate limiting issues ahead of GA of ELSER on EIS
Summary
Implements dynamic default selection for semantic_text fields to automatically use ELSER on EIS
(
.elser-2-elastic
) when available, with graceful fallback to ML nodes (.elser-2-elasticsearch
).Problem
Currently, semantic_text fields are hardcoded to default to
.elser-2-elasticsearch
(ML nodes). This missesopportunities for better performance and cost efficiency when EIS is available.
Solution
ModelRegistry.containsDefaultConfigId()
to detect EIS availability.elser-2-elastic
when available, falls back to.elser-2-elasticsearch
Changes
getPreferredElserInferenceId()
method for dynamic selection logicTesting
Impact