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] Clone analytics job #59791

Merged
merged 30 commits into from
Mar 15, 2020
Merged

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Mar 10, 2020

Summary

Adds support of analytics job cloning.

In case the job has been created using the UI form the flyout for cloning appears with pre-populated inputs except for the job id and destination index. Besides, the clone action detects any settings that are not supported by the form and opens the advanced editor straightaway.

Mar-12-2020 17-41-24

Checklist

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@darnautov
Copy link
Contributor Author

There is an issue with the context menu and not only visual. All the other context items are initialized with Eui button components which end up with nested buttons in the result DOM, hence you can observe some react warnings. I'm going to fix it in this PR.
image

@peteharverson
Copy link
Contributor

peteharverson commented Mar 10, 2020

Gave this a quick test, and I think we also need to handle cloning of an analytics job which was created using the advanced editor. For example, I switched to the advanced editor and added one of the optional params documented here.

image

When I then clone that job, the flyout opens in the standard 'form' mode and my advanced parameter is lost. We should probably check to see if the job config contains fields or values which aren't editable in the form editor, and if so, switch the flyout to the advanced mode.

@alvarezmelissa87
Copy link
Contributor

Great addition overall! 😄 Thanks for your work on this. Just have a couple of questions/comments. Everything works as expected, aside from the issue below.

Even though the model_memory_limit from the source job is part of the data that gets cloned over the call to _explain (that usually fetches that estimate) is still happening.

This can cause an override of the cloned value if the origin job had a source index that is not an index pattern (created via dev tools, for example)

image

You can see the source_index doesn't get populated in the form (though it does when advanced editor is open) though the dependent_variable does.

Would it be worth ensuring that extra _explain call doesn't happen initially? Or just handling origin jobs that may have been created with an index that has not had an index pattern created 🤔

@peteharverson
Copy link
Contributor

I am having a few issues where the cloned model memory limit is cleared and set back to the default, which is below the minimum required, so the attempt to start the job fails after saving (I have been using the bank classification job from the ML QA dev bootstrap df_analytics suite).

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. Just some minor comments on the code.

source: {
index: ['glass_withoutdupl_norm'],
query: {
// TODO check default for `match`
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to leave in this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed one of the outlier detection jobs (created by env bootstrap) has query match instead of match_all. As I saw in all the other cases match_all is a default, so probably it's just a typo in a bootstrap job.

@alvarezmelissa87
Copy link
Contributor

alvarezmelissa87 commented Mar 12, 2020

I'm having an issue creating new analytics jobs - when I click 'create' on the form after filling it in I get a blank screen with this in the console.

I'm only seeing this when it's the very first analytics job being created.

image

Also, the Excludes dropdown is not getting populated initially. Apologies for missing this in the slack convo about the explain endpoint being called twice. It would still need to be called once just to populate the excludes dropdown.

@darnautov
Copy link
Contributor Author

Thanks @alvarezmelissa87 , good spot with excluded options and missing input ref! Just pushed the fixes.

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 latest edits, and LGTM. I didn't test the issue you found @alvarezmelissa87 when creating your first analytics job - would you be able to confirm that is fixed for you please.

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.

Latest edits LGTM ⚡️

…s-jobs

# Conflicts:
#	x-pack/plugins/ml/public/application/data_frame_analytics/pages/analytics_management/components/create_analytics_form/create_analytics_form.tsx
#	x-pack/test/functional/services/machine_learning/api.ts
@darnautov darnautov requested a review from pheyos March 14, 2020 13:36
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@darnautov darnautov merged commit 96ac1aa into elastic:master Mar 15, 2020
@darnautov darnautov deleted the ML-clone-analytics-jobs branch March 15, 2020 21:41
darnautov added a commit to darnautov/kibana that referenced this pull request Mar 16, 2020
* [ML] clone analytics job

* [ML] flyout clone header

* [ML] improve clone action context menu item

* [ML] support advanced job cloning

* [ML] extractCloningConfig

* [ML] fix isAdvancedSetting condition, add test

* [ML] clone job header

* [ML] job description placeholder

* [ML] setEstimatedModelMemoryLimit on source index change

* [ML] Fix types.

* [ML] useUpdateEffect in create_analytics_form.tsx

* [ML] setJobClone action

* [ML] remove CreateAnalyticsFlyoutWrapper instance from the create_analytics_button.tsx

* [ML] fix types

* [ML] hack to align Clone button with the other actions

* [ML] unknown props lead to advanced editor

* [ML] rename maximum_number_trees ot max_trees

* [ML] fix forceInput

* [ML] populate excludesOptions on the first update, skip setting mml on the fist update

* [ML] init functional test for cloning analytics jobs

* [ML] functional tests

* [ML] fix functional tests imports

* [ML] fix indices names for functional tests

* [ML] functional tests for outlier detection and regression jobs cloning

* [ML] delete james tag

* [ML] fix tests arrangement

Co-authored-by: Walter Rafelsberger <walter@elastic.co>
darnautov added a commit that referenced this pull request Mar 16, 2020
* [ML] clone analytics job

* [ML] flyout clone header

* [ML] improve clone action context menu item

* [ML] support advanced job cloning

* [ML] extractCloningConfig

* [ML] fix isAdvancedSetting condition, add test

* [ML] clone job header

* [ML] job description placeholder

* [ML] setEstimatedModelMemoryLimit on source index change

* [ML] Fix types.

* [ML] useUpdateEffect in create_analytics_form.tsx

* [ML] setJobClone action

* [ML] remove CreateAnalyticsFlyoutWrapper instance from the create_analytics_button.tsx

* [ML] fix types

* [ML] hack to align Clone button with the other actions

* [ML] unknown props lead to advanced editor

* [ML] rename maximum_number_trees ot max_trees

* [ML] fix forceInput

* [ML] populate excludesOptions on the first update, skip setting mml on the fist update

* [ML] init functional test for cloning analytics jobs

* [ML] functional tests

* [ML] fix functional tests imports

* [ML] fix indices names for functional tests

* [ML] functional tests for outlier detection and regression jobs cloning

* [ML] delete james tag

* [ML] fix tests arrangement

Co-authored-by: Walter Rafelsberger <walter@elastic.co>

Co-authored-by: Walter Rafelsberger <walter@elastic.co>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 16, 2020
…o alerting/view-in-app

* 'alerting/view-in-app' of github.com:gmmorris/kibana: (33 commits)
  [SIEM] Adds 'Closes and opens signals' Cypress test (elastic#59950)
  [NP] Graph migration (elastic#59409)
  [ML] Clone analytics job  (elastic#59791)
  Move VALUE_CLICK_TRIGGER and APPLY_FILTER_TRIGGER to ui_action… (elastic#60202)
  Handle improperly defined Watcher Logging Action text parameter. (elastic#60169)
  Move select range trigger to uiActions (elastic#60168)
  [SIEM][CASES] Configure cases: Final (elastic#59358)
  Closes elastic#59784. Sets xpack.apm.serviceMapEnabled default value true. (elastic#60153)
  [siem/cypress] update junit filename to be picked up by runbld (elastic#60156)
  OSS logic added to test environment  (elastic#60134)
  Move canvas setup into app mount (elastic#59926)
  enable triggers_actions_ui plugin by default (elastic#60137)
  Expose Elasticsearch from start and deprecate from setup (elastic#59886)
  [SIEM] [Case] Insert timeline bugfix and limit 25 cases (elastic#60136)
  [ML] Client side cut over (elastic#60100)
  Disable query bar on service map routes (elastic#60118)
  Move subscribe_with_scope to kibana_legacy (elastic#59781)
  [Ingest] Agent Config create with sys monitoring (elastic#60111)
  [Watcher UI] Not possible to edit a watch that was created with the API if the ID contains a dot (elastic#59383)
  Actually fetch functionbeat fields needed for telemetry (elastic#60054)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 16, 2020
* master: (40 commits)
  skips 'config_open.ts' files from linter check (elastic#60248)
  [Searchprofiler] Spacing between rendered shards (elastic#60238)
  Add UiSettings validation & Kibana default route redirection (elastic#59694)
  [SIEM][CASE] Change configuration button (elastic#60229)
  [Step 1][App Arch] Saved object migrations from kibana plugin to new platform  (elastic#59044)
  adds new test (elastic#60064)
  [Uptime] Index Status API to Rest (elastic#59657)
  [SIEM] Adds 'Closes and opens signals' Cypress test (elastic#59950)
  [NP] Graph migration (elastic#59409)
  [ML] Clone analytics job  (elastic#59791)
  Move VALUE_CLICK_TRIGGER and APPLY_FILTER_TRIGGER to ui_action… (elastic#60202)
  Handle improperly defined Watcher Logging Action text parameter. (elastic#60169)
  Move select range trigger to uiActions (elastic#60168)
  [SIEM][CASES] Configure cases: Final (elastic#59358)
  Closes elastic#59784. Sets xpack.apm.serviceMapEnabled default value true. (elastic#60153)
  [siem/cypress] update junit filename to be picked up by runbld (elastic#60156)
  OSS logic added to test environment  (elastic#60134)
  Move canvas setup into app mount (elastic#59926)
  enable triggers_actions_ui plugin by default (elastic#60137)
  Expose Elasticsearch from start and deprecate from setup (elastic#59886)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants