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

[ML] Support force stop deployment #118563

Merged
merged 6 commits into from
Nov 25, 2021

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Nov 15, 2021

Summary

Adds support for force stop.
image

If the model has associated pipelines, asks for the user confirmation to perform force stop deployment.

Checklist

@darnautov darnautov added release_note:enhancement :ml auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 Feature:3rd Party Models ML 3rd party models Team:ML Team label for ML (also use :ml) labels Nov 15, 2021
@darnautov darnautov self-assigned this Nov 15, 2021
@darnautov darnautov requested a review from a team as a code owner November 15, 2021 16:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

title={
<FormattedMessage
id="xpack.ml.trainedModels.modelsList.forceStopDialog.title"
defaultMessage="Force stop model {modelId}?"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest removing the term Force from this dialog, as it seems like an API level detail that doesn't need to be shown in the UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 9ed4433

>
<FormattedMessage
id="xpack.ml.trainedModels.modelsList.forceStopDialog.pipelinesWarning"
defaultMessage="Selected model has associated pipelines: "
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe reword to something like The selected model is being used by the following pipelines: @lcawl any thoughts on the text in this dialog?

Copy link
Contributor

@lcawl lcawl Nov 24, 2021

Choose a reason for hiding this comment

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

IMO the impact is unclear. Will the ingest pipelines fail until you start a new deployment or will the inference processor just be skipped? Perhaps something like this:

You can't use these ingest pipelines until you restart the model:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 9ed4433

Copy link
Contributor

Choose a reason for hiding this comment

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

Any index request hitting a pipeline that refers to a model that doesn't have a started deployment will fail. The bulk response will contain an item for each one of them explaining the model wasn't deployed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I've added similar information in the docs and API spec via elastic/elasticsearch#81026 and elastic/elasticsearch-specification#1061

@darnautov
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1578 1579 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.5MB 3.5MB +1.2KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @darnautov

Copy link
Contributor

@lcawl lcawl left a comment

Choose a reason for hiding this comment

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

Text LGTM

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

LGTM

@darnautov darnautov merged commit 8aec972 into elastic:main Nov 25, 2021
@darnautov darnautov deleted the ml-trained-models-force-stop branch November 25, 2021 09:54
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 25, 2021
* support force stop

* fix i18n, revert tests

* update text

* remove "Force" from the confirmation modals
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 25, 2021
* [ML] Support `force` stop deployment  (#118563)

* support force stop

* fix i18n, revert tests

* update text

* remove "Force" from the confirmation modals

* update i18n import

Co-authored-by: Dima Arnautov <[email protected]>
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
* support force stop

* fix i18n, revert tests

* update text

* remove "Force" from the confirmation modals
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:3rd Party Models ML 3rd party models :ml release_note:enhancement Team:ML Team label for ML (also use :ml) v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants