[ML] Support multiple model deployments #155375
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
darnautov
added
release_note:enhancement
:ml
Feature:3rd Party Models
darnautov
force-pushed
the
ml-154886-support-multiple-deployments
branch
from
8ad8e57
to
9893f1d
Compare
darnautov
changed the title
|
Pinging @elastic/ml-ui (:ml) |
qn895
reviewed
Apr 24, 2023
qn895
reviewed
Apr 24, 2023
x-pack/plugins/ml/server/models/model_management/memory_usage.ts
Outdated
Show resolved
Hide resolved
qn895
reviewed
Apr 24, 2023
x-pack/plugins/ml/public/application/model_management/models_list.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/model_management/deployment_setup.tsx
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/model_management/force_stop_dialog.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/model_management/deployment_setup.tsx
Show resolved
Hide resolved
…yments' into ml-154886-support-multiple-deployments
x-pack/plugins/ml/public/application/model_management/deployment_setup.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/model_management/force_stop_dialog.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/model_management/force_stop_dialog.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/model_management/force_stop_dialog.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/model_management/test_models/test_flyout.tsx
Show resolved
Hide resolved
| @@ -491,7 +491,6 @@ export function getMlClient( | |||
| return mlClient.startTrainedModelDeployment(...p); | |||
| }, | |||
| async updateTrainedModelDeployment(...p: Parameters<MlClient['updateTrainedModelDeployment']>) { | |||
| await modelIdsCheck(p); | |||
There was a problem hiding this comment.
These calls to modelIdsCheck are needed to restrict access to models that are in the current space only.
Without them a user can update, stop and infer a model from spaces they do not have access to.
There was a problem hiding this comment.
The problem here with deployment IDs. We don't create saved objects for them, because they only live in deployment stats
There was a problem hiding this comment.
Ok, I this is going to need extra work which could be done after as a bug fix.
We need to know the model ID for each of these commands, otherwise we can't enforce space awareness.
We could change the endpoints so modelId is always sent as part of the request body, and we rename the path variable to deploymentId, e.g.
/api/ml/trained_models/{deploymentId}/deployment/_stop
There was a problem hiding this comment.
Thinking about it more, we'll have to leave the path variable as modelId so it is compatible with the Elasticsearch API that it forwards the request on to.
So the real model ID will probably need to be called something else, but still added to the body so we can check the space.
peteharverson
approved these changes
Apr 25, 2023
qn895
approved these changes
Apr 25, 2023
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @darnautov |
3 tasks
jgowdyelastic
added a commit
that referenced
this pull request
May 3, 2023
…6238) Fixes issues raised in #155375 (comment) Kibana trained model endpoints for `_stop`, `_update` and `infer` now require the model ID was well as the deployment ID to be passed to them. Also fixes the stop trained model api when stopping more than one model. It's very likely the elasticsearch `_stop` api will not support a comma separated list of deployment IDs for this release, and so this change calls `_stop` in a loop for each deployment. It also allows for better reporting if any of the deployments fail to stop.
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
May 3, 2023
…stic#156238) Fixes issues raised in elastic#155375 (comment) Kibana trained model endpoints for `_stop`, `_update` and `infer` now require the model ID was well as the deployment ID to be passed to them. Also fixes the stop trained model api when stopping more than one model. It's very likely the elasticsearch `_stop` api will not support a comma separated list of deployment IDs for this release, and so this change calls `_stop` in a loop for each deployment. It also allows for better reporting if any of the deployments fail to stop. (cherry picked from commit 9559bee)
kibanamachine
added a commit
that referenced
this pull request
May 3, 2023
#156238) (#156483) # Backport This will backport the following commits from `main` to `8.8`: - [[ML] Fixing space checks for recently changed trained model apis (#156238)](#156238) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"James Gowdy","email":"jgowdy@elastic.co"},"sourceCommit":{"committedDate":"2023-05-03T08:42:12Z","message":"[ML] Fixing space checks for recently changed trained model apis (#156238)\n\nFixes issues raised in\r\nhttps://github.com//pull/155375#discussion_r1176594934\r\nKibana trained model endpoints for `_stop`, `_update` and `infer` now\r\nrequire the model ID was well as the deployment ID to be passed to them.\r\n\r\n\r\nAlso fixes the stop trained model api when stopping more than one model.\r\nIt's very likely the elasticsearch `_stop` api will not support a comma\r\nseparated list of deployment IDs for this release, and so this change\r\ncalls `_stop` in a loop for each deployment. It also allows for better\r\nreporting if any of the deployments fail to stop.","sha":"9559bee36139c5a896b70ab16c57d1e24c6a94d9","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["non-issue",":ml","release_note:skip","Feature:3rd Party Models","v8.8.0","v8.9.0"],"number":156238,"url":"#156238 Fixing space checks for recently changed trained model apis (#156238)\n\nFixes issues raised in\r\nhttps://github.com//pull/155375#discussion_r1176594934\r\nKibana trained model endpoints for `_stop`, `_update` and `infer` now\r\nrequire the model ID was well as the deployment ID to be passed to them.\r\n\r\n\r\nAlso fixes the stop trained model api when stopping more than one model.\r\nIt's very likely the elasticsearch `_stop` api will not support a comma\r\nseparated list of deployment IDs for this release, and so this change\r\ncalls `_stop` in a loop for each deployment. It also allows for better\r\nreporting if any of the deployments fail to stop.","sha":"9559bee36139c5a896b70ab16c57d1e24c6a94d9"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"#156238 Fixing space checks for recently changed trained model apis (#156238)\n\nFixes issues raised in\r\nhttps://github.com//pull/155375#discussion_r1176594934\r\nKibana trained model endpoints for `_stop`, `_update` and `infer` now\r\nrequire the model ID was well as the deployment ID to be passed to them.\r\n\r\n\r\nAlso fixes the stop trained model api when stopping more than one model.\r\nIt's very likely the elasticsearch `_stop` api will not support a comma\r\nseparated list of deployment IDs for this release, and so this change\r\ncalls `_stop` in a loop for each deployment. It also allows for better\r\nreporting if any of the deployments fail to stop.","sha":"9559bee36139c5a896b70ab16c57d1e24c6a94d9"}}]}] BACKPORT--> Co-authored-by: James Gowdy <jgowdy@elastic.co>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
With elastic/elasticsearch#95168 it's possible to provide an optional deployment ID for start, stop and infer.
elastic/elasticsearch#95440 also updated the
_statsAPI to provide state per deployment.This PR update the Trained Models UI to support multiple model deployments.
Checklist