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.

Sign up for GitHub

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

Jump to bottom

[ML] Fixes handling of built-in models #92154

Merged
merged 5 commits into from
Feb 22, 2021
Merged

[ML] Fixes handling of built-in models #92154

merged 5 commits into from
Feb 22, 2021

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Feb 22, 2021 •
edited by peteharverson
Loading

Summary

Fixes #85179, preventing the user deleting the built-in lang_ident model, and displaying the description of the model in a column in the table and in the details tab of the expanded row.

  • Adds build-in type for prepackaged models and renders it under the Type column
  • Restricts model actions for built-in models
  • Disables selection of the build-models with an appropriate message
  • Adds description column to the models' table
  • Adds description to the model details tab
  • Removes unused code for filtering, as it's not relevant to EuiInMemoryTable

image

Checklist

@darnautov darnautov self-assigned this Feb 22, 2021
@darnautov darnautov requested a review from a team as a code owner February 22, 2021 12:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@darnautov darnautov changed the title [ML] handle built in model [ML] Add special handling for built-in models Feb 22, 2021
@darnautov darnautov changed the title [ML] Add special handling for built-in models [ML] Fixes handling of built-in models Feb 22, 2021
@darnautov darnautov added the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 22, 2021
@darnautov darnautov enabled auto-merge (squash) February 22, 2021 14:02
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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 6.4MB 6.3MB -1.1KB

History

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

cc @darnautov

alvarezmelissa87
alvarezmelissa87 reviewed Feb 22, 2021
View reviewed changes
...ata_frame_analytics/pages/analytics_management/components/models_management/expanded_row.tsx
.filter(({ description }) => {
return description !== undefined;
.filter(({ description: d }) => {
return d !== undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: could be implicit return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean .filter(({ description: d }) => d !== undefined)?

qn895
qn895 reviewed Feb 22, 2021
View reviewed changes
...data_frame_analytics/pages/analytics_management/components/models_management/models_list.tsx
...model,
// Extract model types
...(typeof model.inference_config === 'object'
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to check if this is not null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't be null. And it seems like now it's always an object. A while ago it was undefined for the build-in model lang_ident_model_1 , but now it contains classification config.
So it's ok to keep as is

@qn895
Copy link
Member

qn895 commented Feb 22, 2021

Tested and LGTM

alvarezmelissa87
alvarezmelissa87 approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Tested and LGTM ⚡

peteharverson
peteharverson approved these changes Feb 22, 2021
View reviewed changes
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.

Code LGTM

@darnautov darnautov merged commit a75abd7 into elastic:master Feb 22, 2021
@darnautov darnautov deleted the ML-85179-handle-built-in-model branch February 22, 2021 17:00
@kibanamachine kibanamachine mentioned this pull request Feb 22, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 22, 2021
@darnautov @kibanamachine
[ML] Fixes handling of built-in models (elastic#92154)
872406f 
* [ML] add description column and details tab

* [ML] restrict build-in models actions

* [ML] add description to the details tab

* [ML] add flex with wrap to the type column

* [ML] remove unused code for filtering
@kibanamachine kibanamachine mentioned this pull request Feb 22, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 22, 2021
@darnautov @kibanamachine
[ML] Fixes handling of built-in models (elastic#92154)
527a35b 
* [ML] add description column and details tab

* [ML] restrict build-in models actions

* [ML] add description to the details tab

* [ML] add flex with wrap to the type column

* [ML] remove unused code for filtering
@kibanamachine
Copy link
Contributor

💚 Backport successful

✅ 7.12 / #92237
✅ 7.x / #92238

Successful backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Feb 22, 2021
@kibanamachine @darnautov
[ML] Fixes handling of built-in models (#92154) (#92237)
1e55a34 
* [ML] add description column and details tab

* [ML] restrict build-in models actions

* [ML] add description to the details tab

* [ML] add flex with wrap to the type column

* [ML] remove unused code for filtering

Co-authored-by: Dima Arnautov <dmitrii.arnautov@elastic.co>
kibanamachine added a commit that referenced this pull request Feb 22, 2021
@kibanamachine @darnautov
[ML] Fixes handling of built-in models (#92154) (#92238)
08517f0 
* [ML] add description column and details tab

* [ML] restrict build-in models actions

* [ML] add description to the details tab

* [ML] add flex with wrap to the type column

* [ML] remove unused code for filtering

Co-authored-by: Dima Arnautov <dmitrii.arnautov@elastic.co>
@darnautov darnautov mentioned this pull request May 3, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvarezmelissa87 alvarezmelissa87 alvarezmelissa87 approved these changes

@peteharverson peteharverson peteharverson approved these changes

@qn895 qn895 Awaiting requested review from qn895

Assignees

@darnautov darnautov

Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Data Frame Analytics ML data frame analytics features :ml release_note:fix v7.12.0 v7.13.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ML] Add special handling for built-in lang-ident model
6 participants
@darnautov @elasticmachine @kibanamachine @qn895 @alvarezmelissa87 @peteharverson