[Search] Consolidate ML model fetch calls #176257

demjened · 2024-02-05T20:24:01Z

Summary

With the introduction of fetch_ml_models.ts, the fetching and enriching of ML models for Search purposes has been consolidated in that API. This allows us to remove the dependency on the older method that works with ML plugin-specific TrainedModel entities.

This PR makes the following changes:

Switch over code that depend on ML models to use the new function from fetch_ml_models.ts (that already does sorting/filtering).
Move the fetch process to ml_inference_logic.ts, and begin periodically polling after mounting the logic. This enables passing down values to lower components, e.g. model_select_logic.ts, instead of repeating the fetch there.
Use MlModel instead of TrainedModel/MlTrainedModelConfig. This requires adding some missing properties to MlModel: types, inputFieldNames, version.
Remove the old fetch methods (x-pack/plugins/enterprise_search/server/lib/ml/ml_*_logic.ts).
Remove the "no models available" component and condition, since as of 8.12 at least the ELSER/E5 placeholders are always present.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

demjened · 2024-02-05T20:50:07Z

...nterprise_search_content/components/search_index/pipelines/ml_inference/inference_config.tsx

-  if (!selectedMLModel || configuration.existingPipeline) return null;
-  const modelType = getMLType(getMlModelTypesForModelConfig(selectedMLModel));
+  if (!selectedModel || configuration.existingPipeline) return null;
+  const modelType = getMLType([selectedModel.type]);


I think this could be simplified by using selectedModel.type directly. I'll check if there are any side effects and modify the code in a separate PR.

Looking at getMLType() I think this is still needed to show the correct type for the lang_ident model. I think without that check this might have show built-in instead. But you should confirm that.

Good call - it does change the type from lang_ident to classification. I think that's OK - the built-in language identification model's type is classification, and I don't see any logic that depends on the value of modelType. But for the sake of less moving parts I restored the original value parsing in this line and in another.

demjened · 2024-02-05T21:01:00Z

...terprise_search_content/components/search_index/pipelines/ml_inference/ml_inference_logic.ts

-        !API_REQUEST_COMPLETE_STATUSES.includes(mlModelsStatus) ||
-        !API_REQUEST_COMPLETE_STATUSES.includes(mappingStatus),
+      () => [selectors.mappingStatus],
+      (mappingStatus: Status) => !API_REQUEST_COMPLETE_STATUSES.includes(mappingStatus),


The isLoading flag for the pipeline configuration panel no longer depends on the status of model fetching. The "models loading" state is now displayed in the model selector. Other steps of the workflow (e.g. generating pipeline configuration) depend on the models data, but it's very unlikely the user will get to that stage before the models finish loading (and even if they do, they just need to wait a little).

If we keep this dependency, the pipeline configuration panel's rendering will be blocked by the fetching of the models.

demjened · 2024-02-05T21:06:12Z

...terprise_search_content/components/search_index/pipelines/ml_inference/model_select_logic.ts

+      () => [selectors.selectableModelsFromMLInferenceLogic],
+      (selectableModels) => selectableModels, // Pass-through


I wonder if we need these pass-through selectors, or we should just expose the imported selectors directly like startPollingModels.

Discussed with @TattdCodeMonkey that we don't need plain pass-through selectors. I'll remove these in a follow-up PR.

demjened · 2024-02-05T21:19:42Z

/ci

demjened · 2024-02-05T21:56:42Z

...terprise_search_content/components/search_index/pipelines/ml_inference/ml_inference_logic.ts

-  events: {},
+  events: ({ actions }) => ({
+    afterMount: () => {
+      actions.startPollingModels();


For some reason the GET /ml/models API is called twice upon opening the pipeline config flyout, and populates the models, so this hook is technically not needed. But I can't find what triggers the other call, so I'm leaving it here while I investigate.

The 2nd call is caused by useEffect() -> setIndexName in add_inference_pipeline_flyout.tsx. I removed the listener, moved the fetch operations there, and removed the afterMount hook.

demjened · 2024-02-05T21:57:51Z

@elasticmachine merge upstream

…-fix'

…ithub.com/elastic/kibana into demjened/remove-ml-model-fetch-redundancy

…-fix'

…ithub.com/elastic/kibana into demjened/remove-ml-model-fetch-redundancy

…-fix'

demjened · 2024-02-07T21:40:58Z

...rch_content/components/search_index/pipelines/ml_inference/add_inference_pipeline_flyout.tsx

+    // Trigger fetching of initial data: existing ML pipelines, available models, index mapping
+    makeMlInferencePipelinesRequest(undefined);
+    startPollingModels();
+    makeMappingRequest({ indexName });


Moved initial fetch operations here from setIndexName listener.

demjened · 2024-02-08T00:06:53Z

@elasticmachine merge upstream

sphilipse

LGTM overall :)

TattdCodeMonkey · 2024-02-08T16:55:00Z

...rch_content/components/search_index/pipelines/ml_inference/add_inference_pipeline_flyout.tsx

@@ -103,9 +107,6 @@ export const AddInferencePipelineContent = ({ onClose }: AddInferencePipelineFly
      </EuiFlyoutBody>
    );
  }
-  if (supportedMLModels.length === 0) {


👍 good to see this removed since we allow the lang ident model now there should never be 0 ML Models

TattdCodeMonkey · 2024-02-08T16:57:57Z

...nterprise_search_content/components/search_index/pipelines/ml_inference/inference_config.tsx

-  if (!selectedMLModel || configuration.existingPipeline) return null;
-  const modelType = getMLType(getMlModelTypesForModelConfig(selectedMLModel));
+  if (!selectedModel || configuration.existingPipeline) return null;
+  const modelType = getMLType([selectedModel.type]);


Looking at getMLType() I think this is still needed to show the correct type for the lang_ident model. I think without that check this might have show built-in instead. But you should confirm that.

...terprise_search_content/components/search_index/pipelines/ml_inference/ml_inference_logic.ts

TattdCodeMonkey · 2024-02-08T17:04:18Z

...terprise_search_content/components/search_index/pipelines/ml_inference/ml_inference_logic.ts

@@ -359,7 +349,7 @@ export const MLInferenceLogic = kea<
    },
    startTextExpansionModelSuccess: () => {
      // Refresh ML models list when the text expansion model is started
-      actions.makeMLModelsRequest(undefined);
+      actions.startPollingModels();


I wonder if we should have a refreshModels action on CachedFetchModlesApiLogic instead of re-using startPollingModels to force an update.

I'm fine with this for now, but its an improvement we could consider, it may not be worth the overhead though.

@TattdCodeMonkey For my understanding - is the issue that this action happens outside the pipeline configuration screen (ELSER callout), and so it unnecessarily starts polling the models rather than refreshing them once?

It's more a semantic quibble, that really isn't important.

If I under the code correctly we are calling startPollingModels() here to force a refresh the model list after an update instead of waiting for the next poll, correct? If thats the case it would be a little nicer to have an action that is descriptive of that intent vs re-using startPollingModels().

BUT calling startPollingModels() works, so is it worth the complexity of introducing another action that may require other special handing to not interfere with the existing polling? maybe not, I could make the argument both ways. So feel free to ignore me this time if you want :)

Gotcha - yeah, it might not be worth the effort. But let's revisit this once these components and logics have been refactored.

…ithub.com/elastic/kibana into demjened/remove-ml-model-fetch-redundancy

kibana-ci · 2024-02-08T20:25:43Z

💚 Build Succeeded

Buildkite Build
Commit: 26ff287

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	2278	2273	-5

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.7MB	2.7MB	-4.1KB

Unknown metric groups

miscellaneous assets size

id	before	after	diff
`enterpriseSearch`	3.4MB	3.3MB	-90.0KB

History

💚 Build #192109 succeeded 95f0916
💔 Build #192092 failed 92019fd
💚 Build #191736 succeeded 1bb28ad
💔 Build #191691 failed a1db2de
💔 Build #191498 failed 32d9885
💔 Build #191487 failed b1ba5fc

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

## Summary With the introduction of [fetch_ml_models.ts](https://github.com/elastic/kibana/blob/main/x-pack/plugins/enterprise_search/server/lib/ml/fetch_ml_models.ts), the fetching and enriching of ML models for Search purposes has been consolidated in that API. This allows us to remove the dependency on the older method that works with ML plugin-specific `TrainedModel` entities. This PR makes the following changes: - Switch over code that depend on ML models to use the new function from `fetch_ml_models.ts` (that already does sorting/filtering). - Move the fetch process to `ml_inference_logic.ts`, and begin periodically polling after mounting the logic. This enables passing down values to lower components, e.g. `model_select_logic.ts`, instead of repeating the fetch there. - Use `MlModel` instead of `TrainedModel/MlTrainedModelConfig`. This requires adding some missing properties to `MlModel`: `types`, `inputFieldNames`, `version`. - Remove the old fetch methods (`x-pack/plugins/enterprise_search/server/lib/ml/ml_*_logic.ts`). - Remove the "no models available" component and condition, since as of 8.12 at least the ELSER/E5 placeholders are always present. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

demjened added 6 commits January 31, 2024 15:35

Use new fetch function to get selectable models

723c275

Remove "no models" panel

9bef7ca

Clean up

86b139e

Update tests

92051b0

Remove unused util function

bd02a99

Fix linter errors

dfa677c

demjened added release_note:skip Skip the PR/issue when compiling release notes Team:EnterpriseSearch v8.13.0 labels Feb 5, 2024

Remove unused API logic

b1ba5fc

demjened commented Feb 5, 2024

View reviewed changes

demjened added 2 commits February 5, 2024 16:33

Delete unused i18n keys

7658a08

Add comments to new props

5c7a99b

demjened marked this pull request as ready for review February 5, 2024 21:52

demjened requested a review from a team February 5, 2024 21:52

demjened commented Feb 5, 2024

View reviewed changes

kibanamachine and others added 9 commits February 5, 2024 16:57

Merge branch 'main' into demjened/remove-ml-model-fetch-redundancy

85efbce

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

32d9885

…-fix'

Fix broken tests

82f3fbf

Merge branch 'demjened/remove-ml-model-fetch-redundancy' of https://g…

0177f4e

…ithub.com/elastic/kibana into demjened/remove-ml-model-fetch-redundancy

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

a1db2de

…-fix'

Fix broken tests

4d07414

Merge branch 'demjened/remove-ml-model-fetch-redundancy' of https://g…

6b3d808

…ithub.com/elastic/kibana into demjened/remove-ml-model-fetch-redundancy

Move initial fetches to flyout

0473769

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

92019fd

…-fix'

demjened force-pushed the demjened/remove-ml-model-fetch-redundancy branch from 05403ad to 92019fd Compare February 7, 2024 21:40

demjened commented Feb 7, 2024

View reviewed changes

Merge branch 'main' into demjened/remove-ml-model-fetch-redundancy

95f0916

sphilipse approved these changes Feb 8, 2024

View reviewed changes

TattdCodeMonkey approved these changes Feb 8, 2024

View reviewed changes

demjened added 3 commits February 8, 2024 14:06

Extract type badge from tags

27bc3b0

Merge branch 'demjened/remove-ml-model-fetch-redundancy' of https://g…

de34ae5

…ithub.com/elastic/kibana into demjened/remove-ml-model-fetch-redundancy

Pass-through export type

26ff287

demjened merged commit e3f1d12 into main Feb 8, 2024
16 checks passed

demjened deleted the demjened/remove-ml-model-fetch-redundancy branch February 8, 2024 20:36

kibanamachine added the backport:skip This commit does not require backporting label Feb 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search] Consolidate ML model fetch calls #176257

[Search] Consolidate ML model fetch calls #176257

demjened commented Feb 5, 2024 •

edited

Loading

demjened Feb 5, 2024

TattdCodeMonkey Feb 8, 2024

demjened Feb 8, 2024

demjened Feb 5, 2024 •

edited

Loading

demjened Feb 5, 2024

demjened Feb 8, 2024

demjened commented Feb 5, 2024

demjened Feb 5, 2024

demjened Feb 8, 2024

demjened commented Feb 5, 2024

demjened Feb 7, 2024

demjened commented Feb 8, 2024

sphilipse left a comment

TattdCodeMonkey Feb 8, 2024

TattdCodeMonkey Feb 8, 2024

TattdCodeMonkey Feb 8, 2024

demjened Feb 8, 2024

TattdCodeMonkey Feb 8, 2024 •

edited

Loading

demjened Feb 8, 2024

kibana-ci commented Feb 8, 2024

miscellaneous assets size

		() => [selectors.selectableModelsFromMLInferenceLogic],
		(selectableModels) => selectableModels, // Pass-through

[Search] Consolidate ML model fetch calls #176257

[Search] Consolidate ML model fetch calls #176257

Conversation

demjened commented Feb 5, 2024 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demjened Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demjened commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demjened commented Feb 5, 2024

Choose a reason for hiding this comment

demjened commented Feb 8, 2024

sphilipse 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

Choose a reason for hiding this comment

TattdCodeMonkey Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Feb 8, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

miscellaneous assets size

History

demjened commented Feb 5, 2024 •

edited

Loading

demjened Feb 5, 2024 •

edited

Loading

TattdCodeMonkey Feb 8, 2024 •

edited

Loading