[ML] Trained Models: Prevents multiple clicks in Delete Model dialog by rbrtj · Pull Request #211580 · elastic/kibana

rbrtj · 2025-02-18T14:15:25Z

Fix for #210004
Additionally:

Prevents the deletion of already deployed models
Fixes a bug where, after selecting a model and deleting it from the selected models actions, the model remained selected

elasticmachine · 2025-02-18T14:15:28Z

Pinging @elastic/ml-ui (:ml)

rbrtj · 2025-02-18T14:16:40Z

x-pack/platform/plugins/shared/ml/public/application/model_management/delete_models_modal.tsx

+        force: pipelinesCount > 0,
+      })
+      .subscribe({
+        next: () => {


I decided to keep the selection and the modal open if the deletion fails

x-pack/platform/plugins/shared/ml/common/types/trained_models.ts

x-pack/platform/plugins/shared/ml/public/application/model_management/delete_models_modal.tsx

x-pack/platform/plugins/shared/ml/public/application/model_management/trained_models_service.ts

darnautov · 2025-02-19T14:15:05Z

x-pack/platform/plugins/shared/ml/public/application/model_management/trained_models_service.ts

+  public deleteModel(modelIds: string[], options: DeleteModelOptions) {
+    return of(modelIds).pipe(
+      tap(() => modelIds.forEach((modelId) => this.removeScheduledDeployments({ modelId }))),
+      switchMap(() =>
+        forkJoin(
+          modelIds.map((modelId) =>
+            from(this.trainedModelsApiService.deleteTrainedModel(modelId, options))
+          )
+        )
+      ),
+      tap({
+        next: () => this.fetchModels(),
+        error: (error) => {
+          this.displayErrorToast?.(
+            error,
+            i18n.translate('xpack.ml.trainedModels.modelsList.fetchDeletionErrorMessage', {
+              defaultMessage: '{modelsCount, plural, one {Model} other {Models}} deletion failed',
+              values: {
+                modelsCount: modelIds.length,
+              },
+            })
+          );
+          this.fetchModels();
+        },
+      })
+    );
+  }
+


What is the advantage of using RxJS in this case? Additionally, what is the reasoning behind moving this logic to the service?

I wouldn't say there's much advantage, I just wanted to keep model actions within the trained models service, as it is currently responsible for downloading, updating, deploying... Additionally, it's quicker to unit test now since there are already existing tests for the service.

x-pack/platform/plugins/shared/ml/public/application/model_management/trained_models_service.ts

x-pack/platform/plugins/shared/ml/public/application/model_management/delete_models_modal.tsx

x-pack/platform/plugins/shared/ml/public/application/model_management/models_list.tsx

jgowdyelastic · 2025-02-21T14:01:36Z

x-pack/platform/plugins/shared/ml/public/application/model_management/trained_models_service.ts

  }

+  public async deleteModels(modelIds: string[], options: DeleteModelParams['options']) {
+    modelIds.forEach((modelId) => this.removeScheduledDeployments({ modelId }));


By calling removeScheduledDeployments up front for each model before deleting, you might end up in an unexpected state if one of the models fails to delete. I think it would be safer to call this inside the map below for each successful model deletion.

Also I think it might be safer to delete the models one at a time, so we can correctly report which models have failed to delete. I appreciate this will take longer to run compared to a Promise.all.
Alternatively It could still happen inside a Promise.all, but we catch and report the failure on a per model basis.

Updated to use Promise.allSettled and correctly report which models have failed to delete.
d11b67c

darnautov

LGTM! Please check the error message text with István

darnautov · 2025-02-24T10:05:57Z

x-pack/platform/plugins/shared/ml/public/application/model_management/trained_models_service.ts

+        undefined,
+        i18n.translate('xpack.ml.trainedModels.modelsList.fetchDeletionErrorMessage', {
+          defaultMessage:
+            'Failed to delete the following {count, plural, one {model} other {models}}: {modelIds}',


I wonder if we need such a long message a in the error toast. Would it be enough just to list the models? cc @szabosteve

Yup, as the toast title already contains the main message – {modelsCount, plural, one {Model} other {Models}} deletion failed – just the list of models should be enough here.

Updated in: f5d1140

jgowdyelastic

LGTM

elasticmachine · 2025-02-26T15:55:51Z

💚 Build Succeeded

Buildkite Build
Commit: 6707256

Metrics [docs]

Async chunks

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

id	before	after	diff
`ml`	5.2MB	5.2MB	+932.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`ml`	105	106	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`ml`	77.2KB	77.3KB	+18.0B

History

💚 Build #278731 succeeded c53e4b4
💚 Build #278516 succeeded aa5ecbe
💔 Build #278486 failed d11b67c
💛 Build #278342 was flaky 9c65b03
💔 Build #278299 failed 0f50e1e
💔 Build #278161 failed 8e0be3a

cc @rbrtj

kibanamachine · 2025-02-26T22:46:35Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13554876589

…lastic#211580) Fix for elastic#210004 Additionally: * Prevents the deletion of already deployed models * Fixes a bug where, after selecting a model and deleting it from the selected models actions, the model remained selected (cherry picked from commit efcc63c)

kibanamachine · 2025-02-26T22:52:08Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…alog (#211580) (#212595) # Backport This will backport the following commits from `main` to `8.x`: - [[ML] Trained Models: Prevent multiple clicks in Delete Model dialog (#211580)](#211580)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Robert Jaszczurek <92210485+rbrtj@users.noreply.github.com>

…lastic#211580) Fix for elastic#210004 Additionally: * Prevents the deletion of already deployed models * Fixes a bug where, after selecting a model and deleting it from the selected models actions, the model remained selected

…alog (elastic#211580) (elastic#212595) # Backport This will backport the following commits from `main` to `8.x`: - [[ML] Trained Models: Prevent multiple clicks in Delete Model dialog (elastic#211580)](elastic#211580)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Robert Jaszczurek <92210485+rbrtj@users.noreply.github.com>

…lastic#211580) Fix for elastic#210004 Additionally: * Prevents the deletion of already deployed models * Fixes a bug where, after selecting a model and deleting it from the selected models actions, the model remained selected

fix delete model dialog

14ed949

rbrtj added release_note:fix :ml Feature:3rd Party Models ML 3rd party models Team:ML Team label for ML (also use :ml) t// backport:version Backport to applied version labels v9.1.0 v8.19.0 labels Feb 18, 2025

rbrtj self-assigned this Feb 18, 2025

rbrtj requested a review from a team as a code owner February 18, 2025 14:15

rbrtj requested a review from darnautov February 18, 2025 14:15

rbrtj commented Feb 18, 2025

View reviewed changes

darnautov reviewed Feb 19, 2025

View reviewed changes

rbrtj added 4 commits February 20, 2025 11:09

move isModelDeployed to the models list

81734fc

reuse DeleteModelParams && add unit test for deleteModels method

c7cf2df

remove setSelectedModels callback

6452884

Merge branch 'main' into ml-delete-model-dialog-allows-multiple-clicks

3f24d69

rbrtj requested a review from darnautov February 20, 2025 12:58

darnautov reviewed Feb 20, 2025

View reviewed changes

x-pack/platform/plugins/shared/ml/public/application/model_management/trained_models_service.ts Outdated Show resolved Hide resolved

darnautov reviewed Feb 20, 2025

View reviewed changes

x-pack/platform/plugins/shared/ml/public/application/model_management/delete_models_modal.tsx Show resolved Hide resolved

rbrtj added 2 commits February 20, 2025 18:50

use async function to delete models

8e0be3a

fix models with pipelines deletion

0f50e1e

rbrtj requested a review from darnautov February 21, 2025 11:05

update trained models service tests

9c65b03

darnautov reviewed Feb 21, 2025

View reviewed changes

x-pack/platform/plugins/shared/ml/public/application/model_management/delete_models_modal.tsx Outdated Show resolved Hide resolved

x-pack/platform/plugins/shared/ml/public/application/model_management/models_list.tsx Show resolved Hide resolved

jgowdyelastic reviewed Feb 21, 2025

View reviewed changes

rbrtj added 2 commits February 21, 2025 16:06

onDelete callback

c3b85e9

update model deletion

d11b67c

rbrtj requested a review from darnautov February 21, 2025 16:05

rbrtj requested a review from jgowdyelastic February 21, 2025 16:05

rbrtj and others added 2 commits February 21, 2025 17:44

cleanup translations

aa5ecbe

Merge branch 'main' into ml-delete-model-dialog-allows-multiple-clicks

c53e4b4

darnautov approved these changes Feb 24, 2025

View reviewed changes

jgowdyelastic approved these changes Feb 24, 2025

View reviewed changes

rbrtj and others added 2 commits February 26, 2025 14:42

update model deletion failed toast message

f5d1140

Merge branch 'main' into ml-delete-model-dialog-allows-multiple-clicks

6707256

rbrtj merged commit efcc63c into elastic:main Feb 26, 2025
9 checks passed

kibanamachine mentioned this pull request Feb 26, 2025

[8.x] [ML] Trained Models: Prevent multiple clicks in Delete Model dialog (#211580) #212595

Merged

nastasha-solomon changed the title ~~[ML] Trained Models: Prevent multiple clicks in Delete Model dialog~~ Jun 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Trained Models: Prevents multiple clicks in Delete Model dialog#211580

[ML] Trained Models: Prevents multiple clicks in Delete Model dialog#211580
rbrtj merged 14 commits intoelastic:mainfrom
rbrtj:ml-delete-model-dialog-allows-multiple-clicks

rbrtj commented Feb 18, 2025 •

edited by kibanamachine

Loading

elasticmachine commented Feb 18, 2025

rbrtj Feb 18, 2025

Uh oh!

Uh oh!

Uh oh!

darnautov Feb 19, 2025

rbrtj Feb 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgowdyelastic Feb 21, 2025

rbrtj Feb 21, 2025

darnautov left a comment

darnautov Feb 24, 2025

szabosteve Feb 25, 2025

rbrtj Feb 26, 2025

jgowdyelastic left a comment

elasticmachine commented Feb 26, 2025

Uh oh!

kibanamachine commented Feb 26, 2025

kibanamachine commented Feb 26, 2025

Labels

6 participants

Conversation

rbrtj commented Feb 18, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticmachine commented Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darnautov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

elasticmachine commented Feb 26, 2025

💚 Build Succeeded

Metrics [docs]

Async chunks

Public APIs missing exports

Page load bundle

History

Uh oh!

kibanamachine commented Feb 26, 2025

kibanamachine commented Feb 26, 2025

💚 All backports created successfully

Questions ?

Labels

6 participants

rbrtj commented Feb 18, 2025 •

edited by kibanamachine

Loading