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] DF Analytics models list: persist pagination through refresh interval #76695

Merged
merged 6 commits into from
Sep 8, 2020
Merged

[ML] DF Analytics models list: persist pagination through refresh interval #76695

merged 6 commits into from
Sep 8, 2020

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Sep 3, 2020 •
edited
Loading

Summary

Related issue #75828
Persists pagination for analytics models list. The functionality is now consistent with anomaly detection and analytics jobs list.

This PR

  • switches the EuiInMemoryTable used in the analytics models list to the EuiBasicTable in order to be able to externally control pagination

  • Uses the useTableSettings custom hook to control pagination/sort for the basic table

  • uses the AnalyticsSearchBar component which handles filtering of the table

image

  • Also updates the extractError function for a new error format so that errors show up correctly in the analytics wizard

Checklist

Delete any items that are not applicable to this PR.

@alvarezmelissa87 alvarezmelissa87 added :ml release_note:skip Skip the PR/issue when compiling release notes Feature:Data Frame Analytics ML data frame analytics features v7.10.0 labels Sep 3, 2020
@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner September 3, 2020 19:23
@alvarezmelissa87 alvarezmelissa87 self-assigned this Sep 3, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Sep 3, 2020
Closed
3 tasks
@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-dfanalytics-models-page-persist branch from bd1f238 to c11f954 Compare September 3, 2020 20:05
darnautov
darnautov reviewed Sep 4, 2020
View reviewed changes
...nalytics/pages/analytics_management/components/analytics_search_bar/analytics_search_bar.tsx
@@ -20,6 +20,68 @@ import {
Value,
DataFrameAnalyticsListRow,
} from '../analytics_list/common';
import { ModelItem } from '../models_management/models_list';

export function filterAnalyticsModels(
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this function a bit confusing. We have a collection that we want to filter based on the set of clauses.
Why not have something like

return items.filter(item => {
 return // check item against clauses
)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 Agree the filtering functionality could be improved. Currently it matches what's been used in the anomaly detection list and analytics list. Might be worth refactoring all of them so the search always works the same way. I think that change is outside the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd understand if you reused filterJobs from x-pack/plugins/ml/public/application/jobs/jobs_list/components/utils.js, but you introduced a brand new function. It would be great to have a generic one that allows you to filter the collection based on clauses. I suspect we don't need to invent our own, just get an existing funtion from EUI.

darnautov
darnautov reviewed Sep 4, 2020
View reviewed changes
...data_frame_analytics/pages/analytics_management/components/models_management/models_list.tsx
const filterList = () => {
if (searchQueryText !== '') {
const query = EuiSearchBar.Query.parse(searchQueryText);
let clauses: any = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible to avoid using any here?

@qn895
Copy link
Member

qn895 commented Sep 4, 2020

Gave this a good test and functionality looks good 👍

peteharverson
peteharverson approved these changes Sep 7, 2020
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.

Tested and LGTM

darnautov
darnautov approved these changes Sep 8, 2020
View reviewed changes
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

Latest edits LGTM

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-dfanalytics-models-page-persist branch from 5b16035 to 08e00c3 Compare September 8, 2020 14:59
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
ml 8.2MB +5.0KB 8.2MB

page load bundle size

id value diff baseline
ml 584.4KB +142.0B 584.2KB

History

  • 💚 Build #72764 succeeded 5b160352ccb58bc3649269931f7917bd0316bceb
  • 💚 Build #72752 succeeded e132747913497159f245f33bca035eb9e2893cac
  • 💚 Build #72437 succeeded 3821426487efb0cb6e7bc9d16930ec19aca66bb7
  • 💚 Build #72422 succeeded c11f95475b23e9bc739bf328339fc6c04e4a20db
  • 💚 Build #72398 succeeded bd1f238478c8415c035d6fc94a38dbecff779529

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

@alvarezmelissa87 alvarezmelissa87 merged commit 8bc6898 into elastic:master Sep 8, 2020
@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Sep 8, 2020
Merged
@alvarezmelissa87 alvarezmelissa87 deleted the ml-dfanalytics-models-page-persist branch September 8, 2020 17:58
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Sep 8, 2020
@alvarezmelissa87
[ML] DF Analytics models list: persist pagination through refresh int…
61e9afd 
…erval (elastic#76695)

* ensure wizard error shows up correctly

* wip: switch to basic table for model management

* add selection and multi job action to models list

* update error extraction function

* use generic types in hook

* simplify filtered items
alvarezmelissa87 added a commit that referenced this pull request Sep 8, 2020
@alvarezmelissa87
[ML] DF Analytics models list: persist pagination through refresh int…
be0a77f 
…erval (#76695) (#76966)

* ensure wizard error shows up correctly

* wip: switch to basic table for model management

* add selection and multi job action to models list

* update error extraction function

* use generic types in hook

* simplify filtered items
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peteharverson peteharverson peteharverson approved these changes

@darnautov darnautov darnautov approved these changes

@walterra walterra Awaiting requested review from walterra

Assignees

@alvarezmelissa87 alvarezmelissa87

Labels
Feature:Data Frame Analytics ML data frame analytics features :ml release_note:skip Skip the PR/issue when compiling release notes v7.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alvarezmelissa87 @elasticmachine @qn895 @kibanamachine @darnautov @peteharverson