[ML] Undeploy elser when inference model deleted #104230
Conversation
|
Pinging @elastic/ml-core (Team:ML) |
|
Hi @maxhniebergall, I've created a changelog YAML for you. |
|
To create an integration test for this feature I added a blocking download elser call which adds about 20s to the runtime. It was helpful for verifying the functionality of this change, but if that downloading elser causes problems or takes too long, I can disable this IT. |
| @@ -57,11 +68,29 @@ protected void masterOperation( | |||
| ClusterState state, | |||
| ActionListener<AcknowledgedResponse> listener | |||
| ) { | |||
| modelRegistry.deleteModel(request.getModelId(), listener.delegateFailureAndWrap((l, r) -> l.onResponse(AcknowledgedResponse.TRUE))); | |||
| SubscribableListener.<ModelRegistry.UnparsedModel>newForked(modelConfigListener -> { | |||
| // ELSER not downloaded case | ||
| { | ||
| String modelId = randomAlphaOfLength(10).toLowerCase(); | ||
| expectThrows(ResponseException.class, () -> putModel(modelId, elserConfig, TaskType.SPARSE_EMBEDDING)); |
There was a problem hiding this comment.
nit: maybe check the message from the exception that it shows the model doesn't exist
| new ElasticsearchStatusException("Failed to stop model " + request.getModelId(), RestStatus.INTERNAL_SERVER_ERROR) | ||
| ); | ||
| } | ||
| }).addListener(listener.delegateFailure((l3, didDeleteModel) -> listener.onResponse(AcknowledgedResponse.of(didDeleteModel)))); |
There was a problem hiding this comment.
I wonder if this should be listener.delegateFailureAndWrap() just in case 🤷♂️. The only thing it is doing is onResponse so I guess it probably doesn't matter.
There was a problem hiding this comment.
yea, I think the AndWrap method is only needed when the lambda could throw a runtime exception, and I don't think that should be possible here
…ub.com/elastic/elasticsearch into undeployElserWhenInferenceModelDeleted
...ice-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceBaseRestTest.java
Outdated
Show resolved
Hide resolved
...e-service-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceCrudIT.java
Outdated
Show resolved
Hide resolved
| expectThrows(ResponseException.class, () -> putModel(modelId, elserConfig, TaskType.SPARSE_EMBEDDING)); | ||
| } | ||
|
|
||
| downloadElserBlocking(); |
There was a problem hiding this comment.
This will be a slow test, we should look at testing the happy case elsewhere
There was a problem hiding this comment.
Where should we be testing slow tests like this?
...ice-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceBaseRestTest.java
Outdated
Show resolved
Hide resolved
* Added stop top InferenceService interface and Elser * New integration tests * undeploy ELSER deployment when _inf ELSER model deleted * Update docs/changelog/104230.yaml * Added check for platform architecture in integration test * improvements from PR comments
Previously, when an _inference model was deleted, this action did not also delete the corresponding trained model deployment. Now, for the ELSER service, the trained model deployment associated with an inference model is deleted/undeployed when the inference model is deleted.
closes https://github.com/elastic/ml-team/issues/1099