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] Add UI test for feature importance features #82677

Merged
merged 21 commits into from
Nov 17, 2020
Merged

[ML] Add UI test for feature importance features #82677

merged 21 commits into from
Nov 17, 2020

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Nov 5, 2020

Summary

This PR adds UI functional testing for the Total Feature Importance panel as well as the decision path in the results data grid.

Checklist

Delete any items that are not applicable to this PR.

@qn895 qn895 self-assigned this Nov 5, 2020
@qn895 qn895 changed the title [ML] WIP - Add UI test for feature importance features [ML] Add UI test for feature importance features Nov 5, 2020
@qn895 qn895 marked this pull request as ready for review November 5, 2020 16:28
@qn895 qn895 requested a review from a team as a code owner November 5, 2020 16:28
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@qn895 qn895 added the release_note:skip Skip the PR/issue when compiling release notes label Nov 5, 2020
alvarezmelissa87
alvarezmelissa87 approved these changes Nov 5, 2020
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.

LGTM ⚡

@pheyos
Copy link
Member

pheyos commented Nov 6, 2020 •
edited
Loading

During my local test runs, I've seen a failure a few times, where the popover didn't display the decision path, but just the "old" json-only content (without tabs). Reloading the results view fixed it and a few runs later it didn't reproduce any more.
Checking CI test stabilitzy in a flaky test runner job ...

@pheyos
Copy link
Member

pheyos commented Nov 6, 2020

The flaky test runner job ran into some test failures. Looks like the one that I've experienced locally, see for example this error screenshot from the flaky test runner job.

@qn895
Copy link
Member Author

qn895 commented Nov 9, 2020

Looking at the failure screenshots, seems like the decision path popover is behaving as expected. We made it so that the chart/json tab will only show up if there are 2 or more features from the ml.feature_importance cell. Based on the screenshots, seems like we are only getting 1 feature back, thus why the chart is not showing. I'm currently looking into the cell result is inconsistent given the same job config.

@qn895
Copy link
Member Author

qn895 commented Nov 10, 2020

Started flaky test runner

@qn895
Copy link
Member Author

qn895 commented Nov 11, 2020

Started flaky runner test for training percentage at 50%

@pheyos
Copy link
Member

pheyos commented Nov 12, 2020

Started flaky runner test for training percentage at 50%

2 failures in 50 runs. This time, we're hitting the 2 minute timeout during job execution:

Error: timed out waiting for analytics state to be stopped -- last error: Error: expected analytics state to be stopped but got analyzing

So running bank marketing with 50% takes just too long. Maybe we need to pick a different test data set that reliably gives us 2 features with a short run time?

@qn895
Copy link
Member Author

qn895 commented Nov 12, 2020

So running bank marketing with 50% takes just too long. Maybe we need to pick a different test data set that reliably gives us 2 features with a short run time?

I was running the test runner 35% and it failed as well with the same problems with only one variable returned, which is unfortunate. Currently testing other data sets.

@qn895
Copy link
Member Author

qn895 commented Nov 12, 2020 •
edited
Loading

Started flaky test runner for a73a162 which uses iowa housing prices data set.

@qn895
Copy link
Member Author

qn895 commented Nov 12, 2020 •
edited
Loading

@pheyos @peteharverson Updated to a different data set and looks like the flaky test runner got no failures 🎉

@pheyos
Copy link
Member

pheyos commented Nov 16, 2020

@elasticmachine merge upstream

@pheyos
Copy link
Member

pheyos commented Nov 16, 2020 •
edited
Loading

Kicked off another flaky test runner job, just to make sure.
=> No failure in 50 runs ✔️

pheyos
pheyos approved these changes Nov 16, 2020
View reviewed changes
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

LGTM

@qn895
Copy link
Member Author

qn895 commented Nov 16, 2020

@elasticmachine merge upstream

@qn895
Copy link
Member Author

qn895 commented Nov 16, 2020

@elasticmachine merge upstream

@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 5.2MB 5.2MB +638.0B

History

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

@qn895 qn895 merged commit 9c0164a into elastic:master Nov 17, 2020
@qn895 qn895 deleted the ml-feature-importance-functional-tests branch November 17, 2020 16:07
@qn895 qn895 mentioned this pull request Nov 17, 2020
Merged
qn895 added a commit to qn895/kibana that referenced this pull request Nov 17, 2020
@qn895 @kibanamachine
[ML] Add UI test for feature importance features (elastic#82677)
e92456c 
Co-authored-by: Kibana Machine <[email protected]>
qn895 added a commit that referenced this pull request Nov 17, 2020
@qn895 @kibanamachine
[7.x] [ML] Add UI test for feature importance features (#82677) (#83556)
cb8f264 
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 17, 2020
@gmmorris
Merge branch 'master' into alerts/action-groups-as-conditions
2985ee2 
* master: (51 commits)
  [ML] Persisted URL state for the Data frame analytics jobs and models pages (elastic#83439)
  adds xpack.security.authc.selector.enabled setting (elastic#83551)
  skip flaky suite (elastic#77279)
  [ML] Improve support for script and aggregation fields in anomaly detection jobs (elastic#81923)
  [Workplace Search] Migrate SourcesLogic from ent-search (elastic#83544)
  [ML] Add UI test for feature importance features (elastic#82677)
  [Maps] Improve icons for all layer types (elastic#83503)
  Replace experimental badge with Beta (elastic#83468)
  [Fleet][EPM] Unified install and archive (elastic#83384)
  Move src/legacy/server/keystore to src/cli (elastic#83483)
  Used SO for saving the API key IDs that should be deleted (elastic#82211)
  [Uptime] Mock implementation to account for math flakiness test (elastic#83535)
  [Workplace Search] Enable check for org context based on URL (elastic#83487)
  [App Search] Added all Document related routes and logic (elastic#83324)
  [Alerting UI] Fix console error when setting connector params (elastic#83333)
  [Discover] Allow custom name for fields via index pattern field management (elastic#70039)
  [Uptime] Fix monitor list down histogram (elastic#83411)
  remove headers timeout hack, rely on nodejs timeouts (elastic#83419)
  [ML] Update console autocomplete for ML data frame evaluate API (elastic#83151)
  [Lens] Color in dimension trigger (elastic#76871)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peteharverson peteharverson peteharverson left review comments

@alvarezmelissa87 alvarezmelissa87 alvarezmelissa87 approved these changes

@pheyos pheyos pheyos approved these changes

Assignees

@qn895 qn895

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

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