-
Notifications
You must be signed in to change notification settings - Fork 25.4k
Configurable Inference timeout during Query time #131551
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
Configurable Inference timeout during Query time #131551
Conversation
Pinging @elastic/search-eng (Team:SearchOrg) |
Pinging @elastic/search-relevance (Team:Search - Relevance) |
Hi @Samiul-TheSoccerFan, I've created a changelog YAML for you. |
@Mikep86 Do we need to ping ML team in the PR too? |
@Samiul-TheSoccerFan Yes, we should ping the ML team since it touches code they own |
Pinging @elastic/ml-core (Team:ML) |
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 left a comment, if you could take a look that'd be great!
...plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/SenderService.java
Outdated
Show resolved
Hide resolved
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.
Change looks good to me, but some more tests need to be updated
...ugin/core/src/main/java/org/elasticsearch/xpack/core/ml/search/SparseVectorQueryBuilder.java
Show resolved
Hide resolved
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 left a few suggestions.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferencePlugin.java
Outdated
Show resolved
Hide resolved
...plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/SenderService.java
Outdated
Show resolved
Hide resolved
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.
Partial review, I think we have unhandled edge cases and potentially divergent default values to manage.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferencePlugin.java
Outdated
Show resolved
Hide resolved
if (timeout == null) { | ||
timeout = clusterService.getClusterSettings().get(InferencePlugin.INFERENCE_QUERY_TIMEOUT); | ||
} |
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.
We only want to apply this timeout if the input type is SEARCH
or INTERNAL_SEARCH
. Which brings up another edge case: If we allow timeout to be null now, we need to set default timeouts for the other input types as well.
@elasticmachine update branch |
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.
Got to everything except SageMakerServiceTests
. I will take a look at those in a follow-up review.
.../plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/ServiceUtils.java
Outdated
Show resolved
Hide resolved
.../plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/ServiceUtils.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferencePlugin.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/Utils.java
Show resolved
Hide resolved
...in/inference/src/test/java/org/elasticsearch/xpack/inference/services/ServiceUtilsTests.java
Outdated
Show resolved
Hide resolved
...in/inference/src/test/java/org/elasticsearch/xpack/inference/services/ServiceUtilsTests.java
Outdated
Show resolved
Hide resolved
.../elasticsearch/xpack/inference/services/elasticsearch/ElasticsearchInternalServiceTests.java
Show resolved
Hide resolved
.../elasticsearch/xpack/inference/services/elasticsearch/ElasticsearchInternalServiceTests.java
Outdated
Show resolved
Hide resolved
.../elasticsearch/xpack/inference/services/elasticsearch/ElasticsearchInternalServiceTests.java
Outdated
Show resolved
Hide resolved
.../elasticsearch/xpack/inference/services/elasticsearch/ElasticsearchInternalServiceTests.java
Outdated
Show resolved
Hide resolved
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.
And a review of SageMakerServiceTests
:)
...rc/test/java/org/elasticsearch/xpack/inference/services/sagemaker/SageMakerServiceTests.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/elasticsearch/xpack/inference/services/sagemaker/SageMakerServiceTests.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/elasticsearch/xpack/inference/services/sagemaker/SageMakerServiceTests.java
Outdated
Show resolved
Hide resolved
@elasticmachine update branch |
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 have a refactor PR that will probably cause a merge conflict with this PR
In #131759 the clusterService
member is removed from all the Inference Service implementations as intellij was reporting it as being unused. @Samiul-TheSoccerFan do you need the clusterService
here? I'm happy to close my PR without merging otherwise if you just need it for the ElasticsearchInternalService
I can work around that.
@davidkyle Yes, we do need to pass the |
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.
One little thing to fix up, then we're good to go 👍
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferencePlugin.java
Outdated
Show resolved
Hide resolved
@elasticmachine update branch |
@elasticmachine update branch |
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!
…-tracking * upstream/main: (26 commits) Add release notes for v9.1.0 release (elastic#131953) Unmute multi_node generative tests (elastic#132021) Avoid re-enqueueing merge tasks (elastic#132020) Fix file entitlements for shared data dir (elastic#131748) ES|QL brute force l2_norm vector function (elastic#132025) Make ES|QL SAMPLE not a pipeline breaker (elastic#132014) Speed up tail computation in MemorySegmentES91OSQVectorsScorer (elastic#132001) Remove deprecated usages in `TransportPutFollowAction` (elastic#132038) Simulate impact of shard movement using shard-level write load (elastic#131406) Remove RemoteClusterService.getConnections() method (elastic#131948) Fix off by one in ValuesBytesRefAggregator (elastic#132032) Use unicode strings in data generation by default (elastic#132028) Adding index.refresh_interval as a data stream setting (elastic#131482) [ES|QL] Add more Min/MaxOverTime CSV tests (elastic#131070) Restrict remote ENRICH after FORK (elastic#131945) Fix decoding of non-ascii field names in ignored source (elastic#132018) [docs] Use centrally maintained version variables (elastic#131939) Configurable Inference timeout during Query time (elastic#131551) ESQL: Allow pruning columns added by InlineJoin (elastic#131204) ESQL: Fail `profile` on text response formats (elastic#128627) ...
This PR focuses on introducing user configurable
inference timeout
settings and use that as timeout during inference calls. Currently, it is hardcoded to10s
and the goal is to make it configurable.Setup
GET the default settings:
Update the inference timeout value:
GET the updated settings: