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] Force stop deployment in use #80431

Merged

Conversation

dimitris-athanasiou
Copy link
Contributor

Implements a force parameter to the stop deployment API.
This allows a user to forcefully stop a deployment. Currently,
this specifically allows stopping a deployment that is in use
by ingest processors.

Implements a `force` parameter to the stop deployment API.
This allows a user to forcefully stop a deployment. Currently,
this specifically allows stopping a deployment that is in use
by ingest processors.
@elasticmachine elasticmachine added the Team:ML Meta label for the ML team label Nov 5, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

@sethmlarson sethmlarson added the Team:Clients Meta label for clients team label Nov 5, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/clients-team (Team:Clients)

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

Can we add YAML tests for these new parameters?

}
},
"body":{
"description":"The stop deployment parameters"
Copy link
Contributor

Choose a reason for hiding this comment

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

What sorts of options can be passed in the body? I don't see a request body documented for this API.

Copy link
Member

@benwtrent benwtrent Nov 5, 2021

Choose a reason for hiding this comment

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

There isn't one documented yet (it will be if we do this).

I am not a HUGE fan of this convention (its elsewhere in ES), where the query params can optionally be supplied in the body. I would much rather have one or the other, but this change does follow convention.

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

Going to switch my review to approve to not be in the way, but it would be appreciated to add docs+tests for these parameters and the request body. We currently don't have this API modeled in the Elasticsearch specification and these resources help us accomplish that! 🙇‍♂️

@benwtrent
Copy link
Member

We currently don't have this API modeled in the Elasticsearch specification and these resources help us accomplish that!

It is experimental :D. So, specification might be a ways off...

@sethmlarson
Copy link
Contributor

@benwtrent We do model experimental APIs (knn_search, tasks.*) to get support for them in language clients.

@dimitris-athanasiou
Copy link
Contributor Author

@sethmlarson There is a problem with having a YML test for this API. To set up such a test we'd need to first start a deployment and that takes too long for actual models. We have a small dummy model we use for testing in PyTorchModelIT. Perhaps we can make that an asset to use in YML tests but we've avoided to do so up to now. I'll look into it and whether it makes the tests too slow.

@dimitris-athanasiou
Copy link
Contributor Author

run elasticsearch-ci/bwc

@dimitris-athanasiou
Copy link
Contributor Author

@elasticmachine update branch

@dimitris-athanasiou
Copy link
Contributor Author

run elasticsearch-ci/docs

@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.0 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 80431

dimitris-athanasiou added a commit to dimitris-athanasiou/elasticsearch that referenced this pull request Nov 8, 2021
Implements a `force` parameter to the stop deployment API.
This allows a user to forcefully stop a deployment. Currently,
this specifically allows stopping a deployment that is in use
by ingest processors.

Co-authored-by: Elastic Machine <[email protected]>
dimitris-athanasiou added a commit that referenced this pull request Nov 8, 2021
Implements a `force` parameter to the stop deployment API.
This allows a user to forcefully stop a deployment. Currently,
this specifically allows stopping a deployment that is in use
by ingest processors.

Co-authored-by: Elastic Machine <[email protected]>
@dimitris-athanasiou dimitris-athanasiou deleted the force-stop-deployment branch November 8, 2021 13:56
@sethmlarson
Copy link
Contributor

@dimitris-athanasiou No problem! Also if only the negative cases are YAML-testable we get value from those too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml Machine learning >non-issue Team:Clients Meta label for clients team Team:ML Meta label for the ML team v8.0.0-rc2 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants