[Enterprise Search] Replace model selection dropdown with list #171436

demjened · 2023-11-16T18:07:55Z

Summary

This PR replaces the model selection dropdown in the ML inference pipeline configuration flyout with a cleaner selection list. The model cards also contain fast deploy action buttons for promoted models (ELSER, E5). The list is periodically updated.

Old:

New:

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)

demjened · 2023-11-16T18:55:48Z

...erprise_search_content/components/search_index/pipelines/ml_inference/configure_pipeline.tsx

-    {
-      disabled: true,
-      inputDisplay:
-        existingPipeline && pipelineName.length > 0
-          ? MODEL_REDACTED_VALUE
-          : MODEL_SELECT_PLACEHOLDER,
-      value: MODEL_SELECT_PLACEHOLDER_VALUE,
-    },


This logic can be safely removed:

The "Select model" placeholder is no longer relevant in the selection list component

The new/existing pipeline choice has been split into separate tabs. Therefore if an existing pipeline is selected with a redacted model, the model selection component won't be rendered in that tab, and there's no need for supporting the disabled placeholder.

...ns/enterprise_search_content/components/search_index/pipelines/ml_inference/model_select.tsx

demjened · 2023-11-20T23:07:42Z

@elasticmachine merge upstream

demjened · 2023-11-21T14:06:44Z

@elasticmachine merge upstream

demjened · 2023-11-21T15:47:25Z

@elasticmachine merge upstream

demjened · 2023-11-21T17:33:33Z

@elasticmachine merge upstream

demjened · 2023-11-30T20:20:53Z

x-pack/plugins/enterprise_search/server/lib/ml/start_ml_model_deployment.ts

@@ -43,7 +43,7 @@ export const startMlModelDeployment = async (
  // we're downloaded already, but not deployed yet - let's deploy it
  const startRequest: MlStartTrainedModelDeploymentRequest = {
    model_id: modelName,
-    wait_for: 'started',
+    wait_for: 'starting',


This change has a minor side effect - the ELSER callout's "Start single-threaded" button will turn the status into "started" while the model is starting. This is fine for two reasons:

Starting the ELSER model takes about a second, so it is very unlikely the user will run a pipeline with the model in the starting phase;

Currently the status label displays "started" both for the "starting" and "started" phase in the ELSER callout.

demjened · 2023-11-30T20:22:21Z

...applications/enterprise_search_content/components/search_index/pipelines/ml_model_health.tsx

+    case TrainedModelState.NotDeployed:
+    case MlModelDeploymentState.NotDeployed:


Accommodating for the broader spectrum of status values of MlModelDeploymentState while maintaining backward compatibility with other components that still use TrainedModelState.

...ise_search_content/components/search_index/pipelines/ml_inference/model_select_logic.test.ts

x-pack/plugins/enterprise_search/common/ml_inference_pipeline/index.ts

...rprise_search_content/components/search_index/pipelines/ml_inference/model_select_option.tsx

TattdCodeMonkey · 2023-11-30T21:21:02Z

...rprise_search_content/components/search_index/pipelines/ml_inference/model_select_option.tsx

            </EuiFlexGroup>
          </EuiFlexItem>
        </EuiFlexGroup>
      </EuiFlexItem>
+      {/* Status indicator OR action button */}


rather than a comment it would make more sense to move this to a component with a descriptive name.

But that maybe a personal preference thing, comments in JSX seem like a code smell to me, but I could be wrong there, @sphilipse ?

TattdCodeMonkey · 2023-11-30T21:21:27Z

...rprise_search_content/components/search_index/pipelines/ml_inference/model_select_option.tsx

+          />
+        )}
+      </EuiFlexItem>
+      {/* Actions menu */}


same here if you want the comment, make a custom component.

…-fix'

…astic/kibana into demjened/model-selection-list

…-fix'

demjened · 2023-12-01T14:34:46Z

@elasticmachine merge upstream

Mikep86

Partial review, can finish later this afternoon

...terprise_search_content/components/search_index/pipelines/ml_inference/model_select.test.tsx

...ns/enterprise_search_content/components/search_index/pipelines/ml_inference/model_select.tsx

Mikep86

LGTM! We can address my comments after FF :)

Mikep86 · 2023-12-01T20:37:06Z

...ns/enterprise_search_content/components/search_index/pipelines/ml_inference/model_select.tsx

+    (models ?? []).map((model) => ({
+      ...model,
+      label: model.modelId,
+      checked: model.modelId === modelID ? 'on' : undefined,


I'm seeing a lot of warnings in my developer console when the list is rendered (see the attached log). I believe it's due to the prop names introduced here by MlModel.

I was wondering why PipelineSelectOptionProps was structured like it is, I think this is why...

localhost-1701462507607.log

Yeah, I've seen those too - no idea why React is complaining. I surely don't want to lowercase all props just to fix that 😅
Maybe @TattdCodeMonkey knows why this is happening?
Example warning:

react-dom.development.js:67 Warning: React does not recognize the `deploymentState` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `deploymentstate` instead. If you accidentally passed it from a parent component, remove it from the DOM element.

Mikep86 · 2023-12-01T20:57:05Z

x-pack/plugins/enterprise_search/server/lib/ml/utils.ts

  description: i18n.translate('xpack.enterpriseSearch.modelCard.elserPlaceholder.description', {
    defaultMessage:
-      'ELSER is designed to efficiently use context in natural language queries with better results than BM25 alone.',
+      "ELSER is Elastic's NLP model for English semantic search, utilizing sparse vectors. It prioritizes intent and contextual meaning over literal term matching, optimized specifically for English documents and queries on the Elastic platform.",


(Hopefully) dumb question: Do we need to make any changes to the files in translations/ to propagate this change?

kibana-ci · 2023-12-01T21:48:29Z

💚 Build Succeeded

Buildkite Build
Commit: aecc18a

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	2234	2240	+6

Async chunks

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

id	before	after	diff
`enterpriseSearch`	2.6MB	2.7MB	+6.4KB

History

💚 Build #181001 succeeded e0cb309
💔 Build #180935 failed b8b8e03
💔 Build #180739 failed d1dc88d
💔 Build #180707 failed d84baee
💔 Build #180680 failed 04fd434
💔 Build #180677 failed e126386

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

## Summary We are removing the E5 model deployment callout from the Pipelines tab in favor of the model selector list with action buttons (#171436). The orphan code cleanup will happen in a separate PR. <img width="636" alt="Screenshot 2023-12-04 at 14 18 43" src="https://github.com/elastic/kibana/assets/14224983/1b0fbce1-34aa-44d4-a9b9-f783761ea2bd">

demjened added release_note:skip Skip the PR/issue when compiling release notes v8.12.0 labels Nov 16, 2023

demjened commented Nov 16, 2023

View reviewed changes

demjened marked this pull request as ready for review November 16, 2023 19:21

demjened requested a review from a team November 16, 2023 19:21

TattdCodeMonkey reviewed Nov 16, 2023

View reviewed changes

...ns/enterprise_search_content/components/search_index/pipelines/ml_inference/model_select.tsx Outdated Show resolved Hide resolved

demjened requested a review from a team November 20, 2023 21:34

Replace model select dropdown with list, add polling and action buttons

1f07826

demjened force-pushed the demjened/model-selection-list branch from a7bdae7 to 1f07826 Compare November 30, 2023 20:10

demjened requested a review from a team as a code owner November 30, 2023 20:10

demjened added the Team:EnterpriseSearch label Nov 30, 2023

Reverting kibana.yml config change... ooops

03272ce

demjened commented Nov 30, 2023

View reviewed changes

demjened added 2 commits November 30, 2023 15:32

Add unit tests for pipeline name generation

9c361a9

Fix i18n key

e126386

demjened commented Nov 30, 2023

View reviewed changes

...ise_search_content/components/search_index/pipelines/ml_inference/model_select_logic.test.ts Show resolved Hide resolved

demjened added 2 commits November 30, 2023 15:40

Fix check error

04fd434

Fix linting errors

01d4879

demjened requested a review from TattdCodeMonkey November 30, 2023 20:49

TattdCodeMonkey reviewed Nov 30, 2023

View reviewed changes

kibanamachine and others added 5 commits November 30, 2023 21:31

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

d84baee

…-fix'

Update model titles, descriptions and licenses

4125126

Add more unit tests, fix rendering condition

256308c

Merge branch 'demjened/model-selection-list' of https://github.com/el…

d24832b

…astic/kibana into demjened/model-selection-list

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

d1dc88d

…-fix'

pgayvallet removed the request for review from a team December 1, 2023 09:26

TattdCodeMonkey approved these changes Dec 1, 2023

View reviewed changes

Merge branch 'main' into demjened/model-selection-list

b8b8e03

Mikep86 reviewed Dec 1, 2023

View reviewed changes

demjened added 3 commits December 1, 2023 11:51

Add i18n, use constant for trained model path

e0cb309

Fix responsive layout, dynamic model selection list height

1722d2b

Revert dynamic list height (for now)

aecc18a

Mikep86 approved these changes Dec 1, 2023

View reviewed changes

demjened merged commit 2c4d0a3 into main Dec 1, 2023
30 checks passed

demjened deleted the demjened/model-selection-list branch December 1, 2023 21:50

kibanamachine added the backport:skip This commit does not require backporting label Dec 1, 2023

demjened mentioned this pull request Dec 4, 2023

[Enterprise Search] Remove E5 callout #172511

Merged

demjened added release_note:feature Makes this part of the condensed release notes and removed release_note:skip Skip the PR/issue when compiling release notes labels Dec 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enterprise Search] Replace model selection dropdown with list #171436

[Enterprise Search] Replace model selection dropdown with list #171436

demjened commented Nov 16, 2023 •

edited

Loading

demjened Nov 16, 2023 •

edited

Loading

demjened commented Nov 20, 2023

demjened commented Nov 21, 2023

demjened commented Nov 21, 2023

demjened commented Nov 21, 2023

demjened Nov 30, 2023 •

edited

Loading

demjened Nov 30, 2023 •

edited

Loading

TattdCodeMonkey Nov 30, 2023

TattdCodeMonkey Nov 30, 2023

demjened commented Dec 1, 2023

Mikep86 left a comment

Mikep86 left a comment

Mikep86 Dec 1, 2023

demjened Dec 1, 2023

Mikep86 Dec 1, 2023

kibana-ci commented Dec 1, 2023

		case TrainedModelState.NotDeployed:
		case MlModelDeploymentState.NotDeployed:

[Enterprise Search] Replace model selection dropdown with list #171436

[Enterprise Search] Replace model selection dropdown with list #171436

Conversation

demjened commented Nov 16, 2023 • edited Loading

Summary

Checklist

demjened Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

demjened commented Nov 20, 2023

demjened commented Nov 21, 2023

demjened commented Nov 21, 2023

demjened commented Nov 21, 2023

demjened Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

demjened Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

TattdCodeMonkey Nov 30, 2023

Choose a reason for hiding this comment

TattdCodeMonkey Nov 30, 2023

Choose a reason for hiding this comment

demjened commented Dec 1, 2023

Mikep86 left a comment

Choose a reason for hiding this comment

Mikep86 left a comment

Choose a reason for hiding this comment

Mikep86 Dec 1, 2023

Choose a reason for hiding this comment

demjened Dec 1, 2023

Choose a reason for hiding this comment

Mikep86 Dec 1, 2023

Choose a reason for hiding this comment

kibana-ci commented Dec 1, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

demjened commented Nov 16, 2023 •

edited

Loading

demjened Nov 16, 2023 •

edited

Loading

demjened Nov 30, 2023 •

edited

Loading

demjened Nov 30, 2023 •

edited

Loading