Skip to content
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

Add Enterprise Search API endpoints for 1 Click ELSER ML Model Deployment #155213

Merged
merged 62 commits into from
Apr 26, 2023
Merged

Add Enterprise Search API endpoints for 1 Click ELSER ML Model Deployment #155213

merged 62 commits into from
Apr 26, 2023

Conversation

markjhoy
Copy link
Contributor

@markjhoy markjhoy commented Apr 18, 2023

Summary

Adds Enterprise Search internal API endpoints for deploying and monitoring the deployment status of an ELSER ML model (and possibly other models in the future) via the 1 click deployment process. This is to not allow a direct call from the Kibana front end to the underlying Elasticsearch ML endpoints.

Closes https://github.com/elastic/enterprise-search-team/issues/4295 and https://github.com/elastic/enterprise-search-team/issues/4397

Checklist

For maintainers

kibanamachine and others added 20 commits April 18, 2023 23:36
….com:markjhoy/kibana into markjhoy/4295_add_ELSER_ent_search_endpoints
….com:markjhoy/kibana into markjhoy/4295_add_ELSER_ent_search_endpoints
@demjened
Copy link
Contributor

demjened commented Apr 23, 2023

@markjhoy Great first draft, thanks!

There are a couple of issues we need to sort out before ELSER download process can work properly:

  • In get_ml_model_deployment_status.ts the trainedModelsProvider.getTrainedModels() call returns a 404 error if the model does not exist, so we need to wrap it in a try/catch block rather than checking modelDetailsResponse. We should check for error.statusCode !== 404 within the catch block; this is what that API returns upon a missing model.
  • While the model is downloading (but not deployed), the modelDetailsResponse.trained_model_configs property does not exist. The correct check is !modelDetailsResponse.trained_model_configs || !modelDetailsResponse.trained_model_configs[0].fully_defined.
  • We'll need to make sure the types in the ML JS client have been updated before this PR can be merged: MlTrainedModelConfig.fully_defined, MlInclude.definition_status.
  • Are you sure the new deployment state is called is_fully_downloaded, not fully_downloaded? The latter is more in line with the other states, e.g. fully_allocated.

@markjhoy
Copy link
Contributor Author

  • In get_ml_model_deployment_status.ts the trainedModelsProvider.getTrainedModels() call returns a 404 error if the model does not exist, so we need to wrap it in a try/catch block rather than checking modelDetailsResponse. We should check for error.statusCode !== 404 within the catch block; this is what that API returns upon a missing model.

Ah - gotcha - I wasn't 100% certain.... updated to check this.

  • While the model is downloading (but not deployed), the modelDetailsResponse.trained_model_configs property does not exist. The correct check is !modelDetailsResponse.trained_model_configs || !modelDetailsResponse.trained_model_configs[0].fully_defined.

Good catch. Updated.

  • We'll need to make sure the types in the ML JS client have been updated before this PR can be merged: MlTrainedModelConfig.fully_defined, MlInclude.definition_status.

Yep - hopefully that'll come soon...

  • Are you sure the new deployment state is called is_fully_downloaded, not fully_downloaded? The latter is more in line with the other states, e.g. fully_allocated.

I think that was some of the initial wording that was there, but subsequently changed... I've cleaned up the code better to reflect the current state of things. 👍

Copy link
Contributor

@TattdCodeMonkey TattdCodeMonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, the TODOs and missing types make a little uneasy given where we are in release cycle.

trainedModelsProvider: MlTrainedModels | undefined
): Promise<MlModelDeploymentStatus> => {
if (!trainedModelsProvider) {
throw new Error('Machine Learning is not enabled');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@demjened I think this is ok since we don't fetch model status if ml is disabled, but we should ensure thats the case.

@markjhoy markjhoy merged commit c964441 into elastic:main Apr 26, 2023
@markjhoy
Copy link
Contributor Author

the TODOs and missing types make a little uneasy given where we are in release cycle

100% agree... hopefully the missing types will be there before BC2 and we can alleviate these....

@jgowdyelastic
Copy link
Member

jgowdyelastic commented Apr 26, 2023

the TODOs and missing types make a little uneasy given where we are in release cycle

100% agree... hopefully the missing types will be there before BC2 and we can alleviate these....

Just to confirm, there were two sets of missing types affecting this PR. The ones in the elasticsearch-js client, and the ones for ML's TrainedModelsProvider.
The TrainedModelsProvider types are now up to date and have been used in this PR.

The elasticsearch-js client types I do not believe will make this version as historically a client update comes with a lot of types churn throughout kibana.
These missing types are minimal, one missing param in the request and one property in the response of the get trained models api.

@markjhoy markjhoy added auto-backport Deprecated - use backport:version if exact versions are needed backport:auto-version and removed backport:skip This commit does not require backporting labels Apr 27, 2023
@sphilipse sphilipse added v8.9.0 and removed v8.8.0 labels Apr 27, 2023
@kibanamachine kibanamachine added backport:skip This commit does not require backporting labels Apr 27, 2023
@sphilipse sphilipse added v8.8.0 and removed backport:skip This commit does not require backporting auto-backport Deprecated - use backport:version if exact versions are needed backport:auto-version v8.9.0 labels Apr 27, 2023
markjhoy added a commit that referenced this pull request Apr 27, 2023
…ELSER ML Model Deployment (#156040)

## Summary

Backports #155213 to 8.8 (as it
just barely missed the cut off and it's needed with other Enterprise
Search work for 8.8)

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
@markjhoy markjhoy deleted the markjhoy/4295_add_ELSER_ent_search_endpoints branch April 27, 2023 17:19
@markjhoy
Copy link
Contributor Author

Just to confirm, there were two sets of missing types affecting this PR. The ones in the elasticsearch-js client, and the ones for ML's TrainedModelsProvider. The TrainedModelsProvider types are now up to date and have been used in this PR.

The elasticsearch-js client types I do not believe will make this version as historically a client update comes with a lot of types churn throughout kibana. These missing types are minimal, one missing param in the request and one property in the response of the get trained models api.

Thanks @jgowdyelastic -- we also have a follow on task to check and update this in the future when things are available: https://github.com/elastic/enterprise-search-team/issues/4432

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 17, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 155213 locally

@sphilipse sphilipse removed backport missing Added to PRs automatically when the are determined to be missing a backport. v8.8.0 labels Jan 18, 2024
@sphilipse
Copy link
Member

Removing version/backport labels because it's too late to add backports now.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 19, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 155213 locally

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 155213 locally

@sphilipse sphilipse removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 24, 2024
@demjened demjened added the backport:skip This commit does not require backporting label Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes Team:EnterpriseSearch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants