Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] ELSER v2 download in the Trained Models UI #167407

Merged
merged 25 commits into from Oct 3, 2023

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Sep 27, 2023

Summary

Adds support for ELSER v2 download from the Trained Models UI.

  • Marks an appropriate model version for the current cluster configuration with the recommended flag.
  • Updates the state column with better human-readable labels and colour indicators.
  • Adds a callout promoting a new version of ELSER
image

Notes for reviews

Checklist

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@peteharverson
Copy link
Contributor

As discussed separately, we need to think of a better way of directing the user to which of the two ELSER models is recommended for their architecture. With the current use of a badge, it can appear like we're recommending of all the users' third party, models from DFA jobs, and ELSER models, that this one is especially recommended.

I think simply adding an extra sentence to the end of the model description would be an improvement over the badge.

Any thoughts on a better UX here @mdefazio ?

image

@mdefazio
Copy link
Contributor

mdefazio commented Sep 27, 2023

@peteharverson

Went back and forth on this as well, and agree there's some oddities for this iteration. The accent badge feels too close to an error and draws too much attention in my opinion.

I had played around with other placements of the badge (as we discussed in Slack), but agree that simple text might be preferable here.

Here's an option appending Recommended to the description. The one small effect of this that I would recommend is moving the 'Tech preview' badge to the id column to give us some more room. Not a deal breaker, but something to consider.

cc/ @julianrosado in case you have some thoughts here too.

image

I'm also dropping in an option showing a badge next to the download button. Not sure how much complexity this brings to the implementation (with conditional column rendering), but provides a clear landmark near the download action.

image

@mdefazio
Copy link
Contributor

@darnautov @peteharverson I've gone back and forth on the additional badge for 'recommended'. Here are two options and I'll leave it to you for what's possible at this stage.

Note the additional banner. This should show if v1 is already downloaded. If not, then shouldn't need this and they would only see v2 anyway.
(No icon necessary, but note the option to dismiss)

EuiCallout size='s' color='primary' onDismiss =(...)
Title: New ELSER model now available
Content: ELSER model v2 release shows faster performance and better relevance for improved information retrieval . <View documentation>
Link should go to the ELSER documentation

image

Alternative version using the badge next to the download button for extra affordance
image

@mdefazio
Copy link
Contributor

Link should go to the...

@peteharverson might have a more accurate link

@peteharverson
Copy link
Contributor

I confirmed with @szabosteve that the link should go to the existing docs page, which will be updated for ELSER v2.

@darnautov
Copy link
Contributor Author

Many thanks for the mockups @mdefazio, ELSER callout has been added in 106ce06

Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI text and doc URL LGTM!

>
<FormattedMessage
id="xpack.ml.trainedModels.modelsList.newElserModelDescription"
defaultMessage="ELSER model v2 release shows faster performance and better relevance for improved information retrieval."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
defaultMessage="ELSER model v2 release shows faster performance and better relevance for improved information retrieval."
defaultMessage="The ELSER v2 model has improved speed and relevance, delivering faster, more performant information retrieval."

Totally optional rewording suggestion :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to substitue "information retrieval" for "text expansion" or "semantic search"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the text for the callout with @arisonl and will go with A new version of ELSER that shows faster performance and improved relevance is now available. View documentation for information on how to start using it.

@darnautov
Copy link
Contributor Author

@elasticmachine merge upstream

}),
'data-test-subj': 'mlModelsTableRowDownloadModelAction',
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be @ts-ignore be removed or have a reason added to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment in beef167

@@ -95,17 +109,75 @@ interface Props {
updatePageState?: (update: Partial<ListingPageUrlState>) => void;
}

export const getModelStateColor = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is pretty big, could this function be moved to a utils file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored in beef167

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested (including on a deployment with a v1 model downloaded), and LGTM.

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@darnautov
Copy link
Contributor Author

@elasticmachine merge upstream

@darnautov darnautov enabled auto-merge (squash) October 3, 2023 08:13
@darnautov darnautov merged commit 0c6dfbf into elastic:main Oct 3, 2023
19 checks passed
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1816 1817 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/ml-trained-models-utils 22 29 +7

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.6MB +64.0B
lists 147.1KB 147.1KB +33.0B
ml 3.5MB 3.5MB +3.2KB
total +3.3KB

Page load bundle

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

id before after diff
core 368.1KB 368.1KB +33.0B
Unknown metric groups

API count

id before after diff
@kbn/ml-trained-models-utils 22 29 +7

History

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

cc @darnautov

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 3, 2023
@peteharverson peteharverson added release_note:feature Makes this part of the condensed release notes and removed release_note:enhancement labels Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting :ml release_note:feature Makes this part of the condensed release notes Team:ML Team label for ML (also use :ml) v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants