[Search][Index Management] Removing Model deployment from Kibana #198409
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
Samiul-TheSoccerFan
added
release_note:skip
Samiul-TheSoccerFan
requested review from
sphilipse,
saikatsarkar056 and
a team
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
saikatsarkar056
approved these changes
Oct 31, 2024
ElenaStoeva
reviewed
Oct 31, 2024
There was a problem hiding this comment.
Hey @Samiul-TheSoccerFan, it seems that with the added logic, the button will always be disabled even after the model is successfully deployed. I tested it and IMO, from a user's perspective, it looks as if the "Try again" button is to try saving the mappings again instead of trying to redeploy the model, so it gets confusing why the button stays disabled after the model is deployed:
Screen.Recording.2024-10-31.at.17.28.06.mov
Wouldn't it be better if we enable the button if the model is deployed (not sure if there is an easy way to check this)?
Also, could you please add in the PR description a link to a related issue (if there's one) and add test instructions since we have new members in @elastic/kibana-management who might not be familiar with the semantic text functionality.
|
@ElenaStoeva We discussed offline and this PR should not be merged as-is. As you noticed, the "Try Again" button is disabled after a successful model deployment and there is no way for the user to dismiss the modal besides clicking "Cancel". |
Mikep86
previously requested changes
Oct 31, 2024
|
@ElenaStoeva : I added the test scenarios that might help test this PR. I also added you in the slack thread |
Samiul-TheSoccerFan
changed the title
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
sphilipse
reviewed
Nov 6, 2024
| @@ -104,7 +104,7 @@ export const useDetailsPageMappingsModelManagement = () => { | |||
| Record<string, TrainedModelStat['deployment_stats'] | undefined> | |||
| >((acc, { model_id: modelId, deployment_stats: stats }) => { | |||
| if (modelId && stats) { | |||
| acc[stats.deployment_id] = stats; | |||
| acc[modelId] = stats; | |||
There was a problem hiding this comment.
I don't think this is correct. Semantic text models should be referenced by their deployment ID, not their model ID, because they only reference the model ID with the deployment ID that is identical to the inference endpoint name.
| @@ -34,9 +34,9 @@ const getCustomInferenceIdMap = ( | |||
| ? { | |||
| trainedModelId: model.service_settings.model_id, | |||
| isDeployable: model.service === Service.elser || model.service === Service.elasticsearch, | |||
| isDeployed: modelStatsById[model.inference_id]?.state === 'started', | |||
| isDeployed: modelStatsById[model.service_settings.model_id]?.state === 'started', | |||
There was a problem hiding this comment.
I don't think this is correct. We should be looking at the inference ID, because we don't care about the model in general, we care about the deployment of the model that is specific to the inference ID.
Samiul-TheSoccerFan
dismissed
Mikep86’s stale review
requesting another review
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
|
|
Removing @Mikep86 from the reviewers list as he is currently on PTO, and this PR needs to be merged in time for the release. |
Samiul-TheSoccerFan
requested review from
ElenaStoeva
and removed request for
Mikep86
ElenaStoeva
approved these changes
Nov 6, 2024
|
Starting backport for target branches: 8.16, 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Nov 6, 2024
…stic#198409) ## Summary Clicking on the `Try Again` button tries to redeploy and receives an error. We tried to disable the `Try Again` button if there are no errors in deployment stage. ### Before https://github.com/user-attachments/assets/b1c7b7ce-afba-42ba-a958-d9ad7cbc8777 ### After https://github.com/user-attachments/assets/15ca3a78-a1fc-4079-8ab6-f4b54e7ed333 --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit bc313f4)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Nov 6, 2024
…stic#198409) ## Summary Clicking on the `Try Again` button tries to redeploy and receives an error. We tried to disable the `Try Again` button if there are no errors in deployment stage. ### Before https://github.com/user-attachments/assets/b1c7b7ce-afba-42ba-a958-d9ad7cbc8777 ### After https://github.com/user-attachments/assets/15ca3a78-a1fc-4079-8ab6-f4b54e7ed333 --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit bc313f4)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Nov 6, 2024
…na (#198409) (#199201) # Backport This will backport the following commits from `main` to `8.16`: - [[Search][Index Management] Removing Model deployment from Kibana (#198409)](#198409) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Samiul Monir","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-06T18:04:24Z","message":"[Search][Index Management] Removing Model deployment from Kibana (#198409)\n\n## Summary\r\n\r\nClicking on the `Try Again` button tries to redeploy and receives an\r\nerror. We tried to disable the `Try Again` button if there are no errors\r\nin deployment stage.\r\n\r\n\r\n### Before\r\n\r\n\r\nhttps://github.com/user-attachments/assets/b1c7b7ce-afba-42ba-a958-d9ad7cbc8777\r\n\r\n### After\r\n\r\n\r\nhttps://github.com/user-attachments/assets/15ca3a78-a1fc-4079-8ab6-f4b54e7ed333\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"bc313f4b48680624a0f778eb02eb10158f604a15","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Search","v8.16.0","backport:version","v8.17.0"],"title":"[Search][Index Management] Removing Model deployment from Kibana","number":198409,"url":"https://github.com/elastic/kibana/pull/198409","mergeCommit":{"message":"[Search][Index Management] Removing Model deployment from Kibana (#198409)\n\n## Summary\r\n\r\nClicking on the `Try Again` button tries to redeploy and receives an\r\nerror. We tried to disable the `Try Again` button if there are no errors\r\nin deployment stage.\r\n\r\n\r\n### Before\r\n\r\n\r\nhttps://github.com/user-attachments/assets/b1c7b7ce-afba-42ba-a958-d9ad7cbc8777\r\n\r\n### After\r\n\r\n\r\nhttps://github.com/user-attachments/assets/15ca3a78-a1fc-4079-8ab6-f4b54e7ed333\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"bc313f4b48680624a0f778eb02eb10158f604a15"}},"sourceBranch":"main","suggestedTargetBranches":["8.16","8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198409","number":198409,"mergeCommit":{"message":"[Search][Index Management] Removing Model deployment from Kibana (#198409)\n\n## Summary\r\n\r\nClicking on the `Try Again` button tries to redeploy and receives an\r\nerror. We tried to disable the `Try Again` button if there are no errors\r\nin deployment stage.\r\n\r\n\r\n### Before\r\n\r\n\r\nhttps://github.com/user-attachments/assets/b1c7b7ce-afba-42ba-a958-d9ad7cbc8777\r\n\r\n### After\r\n\r\n\r\nhttps://github.com/user-attachments/assets/15ca3a78-a1fc-4079-8ab6-f4b54e7ed333\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"bc313f4b48680624a0f778eb02eb10158f604a15"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Samiul Monir <[email protected]>
kibanamachine
added a commit
that referenced
this pull request
Nov 6, 2024
#198409) (#199202) # Backport This will backport the following commits from `main` to `8.x`: - [[Search][Index Management] Removing Model deployment from Kibana (#198409)](#198409) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Samiul Monir","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-06T18:04:24Z","message":"[Search][Index Management] Removing Model deployment from Kibana (#198409)\n\n## Summary\r\n\r\nClicking on the `Try Again` button tries to redeploy and receives an\r\nerror. We tried to disable the `Try Again` button if there are no errors\r\nin deployment stage.\r\n\r\n\r\n### Before\r\n\r\n\r\nhttps://github.com/user-attachments/assets/b1c7b7ce-afba-42ba-a958-d9ad7cbc8777\r\n\r\n### After\r\n\r\n\r\nhttps://github.com/user-attachments/assets/15ca3a78-a1fc-4079-8ab6-f4b54e7ed333\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"bc313f4b48680624a0f778eb02eb10158f604a15","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Search","v8.16.0","backport:version","v8.17.0"],"title":"[Search][Index Management] Removing Model deployment from Kibana","number":198409,"url":"https://github.com/elastic/kibana/pull/198409","mergeCommit":{"message":"[Search][Index Management] Removing Model deployment from Kibana (#198409)\n\n## Summary\r\n\r\nClicking on the `Try Again` button tries to redeploy and receives an\r\nerror. We tried to disable the `Try Again` button if there are no errors\r\nin deployment stage.\r\n\r\n\r\n### Before\r\n\r\n\r\nhttps://github.com/user-attachments/assets/b1c7b7ce-afba-42ba-a958-d9ad7cbc8777\r\n\r\n### After\r\n\r\n\r\nhttps://github.com/user-attachments/assets/15ca3a78-a1fc-4079-8ab6-f4b54e7ed333\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"bc313f4b48680624a0f778eb02eb10158f604a15"}},"sourceBranch":"main","suggestedTargetBranches":["8.16","8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198409","number":198409,"mergeCommit":{"message":"[Search][Index Management] Removing Model deployment from Kibana (#198409)\n\n## Summary\r\n\r\nClicking on the `Try Again` button tries to redeploy and receives an\r\nerror. We tried to disable the `Try Again` button if there are no errors\r\nin deployment stage.\r\n\r\n\r\n### Before\r\n\r\n\r\nhttps://github.com/user-attachments/assets/b1c7b7ce-afba-42ba-a958-d9ad7cbc8777\r\n\r\n### After\r\n\r\n\r\nhttps://github.com/user-attachments/assets/15ca3a78-a1fc-4079-8ab6-f4b54e7ed333\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"bc313f4b48680624a0f778eb02eb10158f604a15"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Samiul Monir <[email protected]>
mgadewoll
pushed a commit
to mgadewoll/kibana
that referenced
this pull request
Nov 7, 2024
…stic#198409) ## Summary Clicking on the `Try Again` button tries to redeploy and receives an error. We tried to disable the `Try Again` button if there are no errors in deployment stage. ### Before https://github.com/user-attachments/assets/b1c7b7ce-afba-42ba-a958-d9ad7cbc8777 ### After https://github.com/user-attachments/assets/15ca3a78-a1fc-4079-8ab6-f4b54e7ed333 --------- Co-authored-by: Elastic Machine <[email protected]>
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
Clicking on the
Try Againbutton tries to redeploy and receives an error. We tried to disable theTry Againbutton if there are no errors in deployment stage.Before
with-errors.mov
After
without-error-message.mov