[ML] Trained models: Replace download button by extending deploy action #205699

rbrtj · 2025-01-07T10:28:45Z

Summary

Removes the download model button by extending the deploy action.
The model download begins automatically after clicking Start Deployment.
It is possible to queue one deployment while the model is still downloading.
Navigating away from the Trained Models page will not interrupt the downloading or deployment process.
State column renamed to Model State
Responsiveness fix: icons overlap

Screen.Recording.2025-01-14.at.10.23.55.mov

…ments

…rations && active operations navigation blocker in models list

darnautov · 2025-01-08T12:07:54Z

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

+    try {
+      await this.ensureModelIsDownloaded(modelId);
+
+      this.addActiveOperation({ modelId, type: 'deploying' });


how do you queue several deployments of the same model?
in the cloud environment, the depoyment process can take minutes.

It was a mistake. Previously user could start only one deployment at a time, made changes to follow the same pattern in:
b9d6c5f

darnautov · 2025-01-08T12:13:39Z

We should make the Actions column look a bit neater with primary actions. Either increasing the column width or use icons

darnautov · 2025-01-08T12:15:35Z

After the model downloading I see the "Read to deploy" state, while it should move to "Deploying" right after the downloading

darnautov · 2025-01-08T12:27:13Z

At the moment you allow 2 start deployment actions, but not more. Any particular reason for that?

Also, it is possible to queue deployments with the same ID.

same_deployment_id_issue.mov

darnautov · 2025-01-08T12:28:40Z

When the user deletes a model after starting a download, we should cancel all queued deployment requests:

start_after_delte.mov

darnautov · 2025-01-08T12:33:40Z

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

+  );
+
+  // Navigation blocker when there are active operations
+  useUnsavedChangesPrompt({


We should allow the user to navigate to another Kibana page, but prompt on closing the Kibana browser tab.

done in 5acf8dd

darnautov · 2025-01-08T12:50:55Z

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

+    this.modelState$.complete();
+    this.downloadStatus$.complete();
+    this.stopPolling$.complete();
+    this.pollingSubscription?.unsubscribe();


it seems like you already unsubscribe in this.stopPolling

Done in 9856fe7

darnautov · 2025-01-08T12:54:21Z

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

+      )
+      .subscribe({
+        next: (downloadStatus) => {
+          const currentItems = this.modelState$.getValue().items;


You should use withLatestFrom operator instead of static getValue

Done in 9856fe7

darnautov · 2025-01-08T12:57:12Z

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

+    this.abortedDownloads.add(modelId);
+  }
+
+  public getModelDownloadState$(modelId: string): Observable<ModelDownloadState | undefined> {


I could not find the usage of this method anywhere

Also keep in mind, it returns a new Observable instance on each call. It will not work with useObservable hook as expected.

It was a leftover, removed in 173321f

darnautov · 2025-01-08T13:03:58Z

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

+    return this.downloadStatus$.getValue()[modelId];
+  }
+
+  private async ensureModelIsDownloaded(modelId: string) {


Mixing concepts makes it difficult to follow the logic. I would suggest sticking to one, either observables or promises. If you prefer the reactive approach, you should create a queue of model deployments. And this queue should wait for model download completion.

Refactor to follow the more reactive pattern done in: 173321f

darnautov · 2025-01-08T13:07:59Z

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

+      if (!refresh) return;
+
+      // Register callback for expanded rows updates
+      const removeCallback = trainedModelsService.onFetch((modelItems) => {


What is the motivation for registring a callback? Did you consider using an effect that tracks model items?

Done in: 173321f

darnautov · 2025-01-08T13:09:54Z

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

    },
  } = useMlKibana();

+  const trainedModelsService = useTrainedModelsService();
+  const isInitialized = trainedModelsService.isInitialized();


It is a static value and the React component will never get this update. We should either get rid of is altogether or make it part of state (or observable)

Removed in: 27f8d9f

darnautov · 2025-01-08T13:11:08Z

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

+  useEffect(() => {
+    return () => {
+      if (trainedModelsService) {
+        trainedModelsService.destroy();


I reckon we should only destroy on leave if there are no deployments queued

Done in 5acf8dd

…ace-download-button-by-extending-deploy-action

…current operations state

rbrtj · 2025-01-09T12:22:01Z

At the moment you allow 2 start deployment actions, but not more. Any particular reason for that?

Also, it is possible to queue deployments with the same ID.

same_deployment_id_issue.mov

Previously only one deployment could be started at a time, fixed in:
b9d6c5f

…ore reactive pattern

…nsavedChangesPrompt non blocking spa functionality

rbrtj · 2025-01-13T15:08:25Z

...ages/shared/kbn-unsaved-changes-prompt/src/unsaved_changes_prompt/unsaved_changes_prompt.tsx

 }

-export const useUnsavedChangesPrompt = ({


Note: This is just a proposal, so it is possible to use useUnsavedChangesPrompt without blocking SPA navigation. If you don't like the implementation, we can separate the relevant part of the hook into our package

…pping

elasticmachine · 2025-01-14T10:25:47Z

Pinging @elastic/ml-ui (:ml)

peteharverson · 2025-01-15T11:39:56Z

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

          );
        },
        onClick: async (item) => {
+          if (isModelDownloadItem(item) && item.state === MODEL_STATE.NOT_DOWNLOADED) {


Was it a deliberate decision to start downloading the model as soon as the deployment modal is opened? I wonder if we should wait to begin the download until the user clicks the 'Start' button in the deployment modal, otherwise there is no way to cancel the download.

The behavior is described in: #195593 (comment)
It is possible to cancel the download by hitting 'Delete Model':

Screen.Recording.2025-01-15.at.14.47.59.mov

peteharverson · 2025-01-15T11:48:52Z

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

+  // Navigation blocker when there are active operations
+  useUnsavedChangesPrompt({
+    hasUnsavedChanges: activeOperations.length > 0,
+    blockSpaNavigation: false,


As discussed, can you add toast notification in to inform the user that a model has finished deploying if they have navigated away from the page?

Done in: 832418b
I will consult on the toast text after the PR is approved.

Screen.Recording.2025-01-15.at.15.51.51.mov

peteharverson · 2025-01-15T11:55:42Z

@rbrtj @darnautov do we want to keep the current behavior in the 'Add trained models' flyout, where it just does the download part?

sabarasaba

I like the new prop for the unsaved changes prop hook, thanks @rbrtj!

peteharverson · 2025-01-15T17:43:07Z

The UX when you try to deploy a model which is downloaded but not available in the current space could be improved, but this also applies to main so I have raised a separate issue for this - #206834.

peteharverson · 2025-01-15T17:49:54Z

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

+                  text: i18n.translate(
+                    'xpack.ml.trainedModels.modelsList.startSuccessDescription',
+                    {
+                      defaultMessage: 'Deployment for "{modelId}" has been started successfully.',


I'd remove been from this message. Or do you know the deployment ID at this point, in which case something like

Deployment .elser_model_2_ingest has started successfully.

Either

Suggested change

defaultMessage: 'Deployment for "{modelId}" has been started successfully.',

defaultMessage: 'Deployment for "{modelId}" has started successfully.',

or

Suggested change

defaultMessage: 'Deployment for "{modelId}" has been started successfully.',

defaultMessage: '"DeploymentID" has started successfully.',

The second one would be better if possible, so we wouldn't need to distinguish between model and deployment (Deployment for modelID) in the message as it makes the toast harder to skim.

Updated with option 2 in 62b3a61

Additionally, the error message has been updated to include the deploymentId instead of the modelId

…tending-deploy-action

joana-cps · 2025-01-20T11:18:07Z

Review: Start Deployment Modal

Can we change the text "Downloading" to one of these options?
It's unclear what are we exactly downloading. I know that on the table we have "model state" but here it's unclear.

Use model name (might be too long)
Use "Downloading model"

Minor UI fixes:

Download bar size

Add divider after download progress bar for visual separation.

joana-cps · 2025-01-20T11:23:00Z

✅ Design Review
Besides the previous comment, LGTM. Behavious are correct and UI responsiveness was fixed and looks pretty neat 👌

darnautov · 2025-01-21T11:27:07Z

If you start model download and duplicate the browser's tab, the Start deployment actions is not disabled. We should keep the state in sync across browsers tabs.

…odal

rbrtj · 2025-01-21T12:11:04Z

@joana-cps

Updated the downloading label inside the modal and adjusted the text color in 0a6ff9f

Adjusted the download bar size and added a divider in a199de2
Inside the modal, we now display only the ready for deployment or downloading states. Other states, such as deployed, are hidden
Done in (e651ce4)

elasticmachine · 2025-01-21T12:23:47Z

💔 Build Failed

Buildkite Build
Commit: 0a6ff9f

Failed CI Steps

Quick Checks

History

💔 Build #268743 failed a199de2
💔 Build #268722 failed 62b3a61
💛 Build #267692 was flaky 60129c2
💚 Build #267425 succeeded 832418b
💚 Build #266880 succeeded 630e994

cc @rbrtj

szabosteve

UI text LGTM!

rbrtj added 9 commits January 2, 2025 12:01

introducing trained models service to manage models download & deploy…

0de3198

…ments

rename action to Start Deployment

d0078f9

add a queue for deployments started before the model is downloaded

1474865

rename State column to Model state

0d52bc0

model actions cleanup

1727e2c

rename Delete action to Delete Model && update actions order

6f96af1

trained models service small refactor

cf1ca47

align error handling with previous implementation

31c18e8

fix trained models service flow to correctly keep track of active ope…

3b646de

…rations && active operations navigation blocker in models list

darnautov reviewed Jan 8, 2025

View reviewed changes

rbrtj added 3 commits January 9, 2025 10:43

Merge remote-tracking branch 'upstream/main' into trained-models-repl…

0c0eadf

…ace-download-button-by-extending-deploy-action

fix loading state in trained models service

2646bef

ensure user can start one deployment at a time && isLoading based on …

b9d6c5f

…current operations state

rbrtj added 5 commits January 10, 2025 14:11

refactor trained models service and corresponding components to use m…

173321f

…ore reactive pattern

cancel all request when user deletes a model after starting a download

c079ddf

use withLatestFrom for polling logic

9856fe7

remove isInitialized

27f8d9f

use trained models service hook - improve singletone behavior && useU…

5acf8dd

…nsavedChangesPrompt non blocking spa functionality

rbrtj commented Jan 13, 2025

View reviewed changes

rbrtj added 4 commits January 13, 2025 23:17

fix trained models list actions and expanding row icon buttons overla…

8847920

…pping

display deploying state

11b951e

simplify startModelDeployment$

a81df16

update breakpoint

7863d66

rbrtj self-assigned this Jan 14, 2025

rbrtj added release_note:enhancement :ml v9.0.0 Feature:3rd Party Models ML 3rd party models Team:ML Team label for ML (also use :ml) backport:version Backport to applied version labels v8.18.0 labels Jan 14, 2025

kibanamachine and others added 2 commits January 14, 2025 10:35

[CI] Auto-commit changed files from 'node scripts/yarn_deduplicate'

c23ad1d

remove unused translations

630e994

peteharverson reviewed Jan 15, 2025

View reviewed changes

sabarasaba approved these changes Jan 15, 2025

View reviewed changes

display success toast after deploying the model

832418b

peteharverson reviewed Jan 15, 2025

View reviewed changes

Merge branch 'main' into trained-models-replace-download-button-by-ex…

60129c2

…tending-deploy-action

rbrtj requested a review from darnautov January 20, 2025 09:03

rbrtj added 2 commits January 21, 2025 11:09

update start deployment toast messages

62b3a61

deployment modal: add divider, allow progress bar to take more space

a199de2

rbrtj added 2 commits January 21, 2025 12:50

show only downloading and ready for deployment states in deployment m…

e651ce4

…odal

update progress label inside deployment modal && update label color

0a6ff9f

szabosteve approved these changes Jan 21, 2025

View reviewed changes

	defaultMessage: 'Deployment for "{modelId}" has been started successfully.',
	defaultMessage: 'Deployment for "{modelId}" has started successfully.',

	defaultMessage: 'Deployment for "{modelId}" has been started successfully.',
	defaultMessage: '"DeploymentID" has started successfully.',

[ML] Trained models: Replace download button by extending deploy action #205699

Are you sure you want to change the base?

[ML] Trained models: Replace download button by extending deploy action #205699

Conversation

rbrtj commented Jan 7, 2025 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darnautov commented Jan 8, 2025

darnautov commented Jan 8, 2025

darnautov commented Jan 8, 2025

darnautov commented Jan 8, 2025

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

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

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

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

Choose a reason for hiding this comment

rbrtj commented Jan 9, 2025

Choose a reason for hiding this comment

elasticmachine commented Jan 14, 2025

Choose a reason for hiding this comment

rbrtj Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson commented Jan 15, 2025

sabarasaba left a comment

Choose a reason for hiding this comment

peteharverson commented Jan 15, 2025

Choose a reason for hiding this comment

szabosteve Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joana-cps commented Jan 20, 2025

Review: Start Deployment Modal

Minor UI fixes:

joana-cps commented Jan 20, 2025 • edited Loading

darnautov commented Jan 21, 2025

rbrtj commented Jan 21, 2025

elasticmachine commented Jan 21, 2025 • edited Loading

💔 Build Failed

Failed CI Steps

History

szabosteve left a comment

Choose a reason for hiding this comment

rbrtj commented Jan 7, 2025 •

edited

Loading

rbrtj Jan 15, 2025 •

edited

Loading

szabosteve Jan 20, 2025 •

edited

Loading

joana-cps commented Jan 20, 2025 •

edited

Loading

elasticmachine commented Jan 21, 2025 •

edited

Loading