[ML] DF Analytics - auto-populate model_memory_limit #50714

alvarezmelissa87 · 2019-11-14T19:38:19Z

Summary

Fix for #43740 (Improve UX regarding df analytics model memory limit)

Auto-populates model_memory_limit field using ml/data_frame/analytics/_estimate_memory_usage endpoint.

validateModelMemoryLimitUnits has been updated to be be more generic. cc @jgowdyelastic

Validates that mml is not empty and has valid content in both the advanced editor:

and the form:

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-11-14T19:38:21Z

Pinging @elastic/ml-ui (:ml)

elasticmachine · 2019-11-14T20:48:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 3b43be0

elasticmachine · 2019-11-14T21:32:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b605812

alvarezmelissa87 · 2019-11-14T23:52:42Z

cc @droberts195

darnautov · 2019-11-15T08:52:42Z

...lytics/pages/analytics_management/components/create_analytics_form/create_analytics_form.tsx

+      const jobConfig = getJobConfigFromFormState(form);
+      delete jobConfig.dest;


it can be const { dest, ...jobConfig} = getJobConfigFromFormState(form);

Thanks for taking a look! 😄
I think that's a good option if mutability is a concern, but in this case jobConfig isn't relied on for anything else. I think in this case I prefer clarity of semantics and functionality by explicitly using delete.
Unless I'm missing some other benefit that this has over using delete 🤔
cc @darnautov

darnautov · 2019-11-15T09:02:28Z

x-pack/legacy/plugins/ml/public/services/ml_api_service/index.d.ts

@@ -69,6 +69,7 @@ declare interface Ml {
    getDataFrameAnalyticsStats(analyticsId?: string): Promise<GetDataFrameAnalyticsStatsResponse>;
    createDataFrameAnalytics(analyticsId: string, analyticsConfig: any): Promise<any>;
    evaluateDataFrameAnalytics(evaluateConfig: any): Promise<any>;
+    estimateMemoryUsage(jobConfig: any): Promise<any>;


could you please provide types here?

Added type: b63f93e

peteharverson · 2019-11-15T13:53:16Z

x-pack/legacy/plugins/ml/server/client/elasticsearch_ml.js

@@ -170,6 +170,16 @@ export const elasticsearchJsPlugin = (Client, config, components) => {
    method: 'POST'
  });

+  ml.estimateMemoryUsage = ca({


I think DataFrameAnalytics should be in the function name here. More verbose, but otherwise it isn't clear that is can't be used to estimate the MML for anomaly detection jobs for example. estimateDataFrameAnalyticsMemoryUsage !?

peteharverson · 2019-11-15T13:54:56Z

x-pack/legacy/plugins/ml/public/services/ml_api_service/index.d.ts

@@ -69,6 +69,7 @@ declare interface Ml {
    getDataFrameAnalyticsStats(analyticsId?: string): Promise<GetDataFrameAnalyticsStatsResponse>;
    createDataFrameAnalytics(analyticsId: string, analyticsConfig: any): Promise<any>;
    evaluateDataFrameAnalytics(evaluateConfig: any): Promise<any>;
+    estimateMemoryUsage(jobConfig: any): Promise<any>;


As discussed below, I think DataFrameAnalytics needs to be in the function name here.

Updated to include DataFrameAnalytics in the function name: b63f93e

...lytics/pages/analytics_management/components/create_analytics_form/create_analytics_form.tsx

peteharverson · 2019-11-15T14:07:44Z

...lytics/pages/analytics_management/components/create_analytics_form/create_analytics_form.tsx

+              defaultMessage: 'Model memory limit',
+            })}
+          >
+            <EuiFieldText


Could the default value (or the one from the endpoint) be used as a placeholder?

Placeholder added: f8c50cf

peteharverson · 2019-11-15T14:09:07Z

...lic/data_frame_analytics/pages/analytics_management/hooks/use_create_analytics_form/state.ts

+export enum DEFAULT_MODEL_MEMORY_LIMIT {
+  regression = '100mb',
+  // eslint-disable-next-line @typescript-eslint/camelcase
+  outlier_detection = '50mb',


Is this used? When I start in the wizard, and set the type to Outlier detection, the default stays at 100mb.

Yes this is used as the default value placeholder for when jobType is outlier detection.

elasticmachine · 2019-11-15T16:27:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d30ee0f

elasticmachine · 2019-11-15T22:14:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cb14d92

walterra

Code LGTM! Would be good to add some tests to reducer.test.ts since we already have that in place to write tests against it.

peteharverson

Tested latest edits and LGTM

…rity

alvarezmelissa87 · 2019-11-18T16:02:15Z

Good call @walterra 🙌 Added tests in 94598c4
Would you be up for taking a quick look when you get a chance? Thanks! 😄

walterra

Latest changes LGTM

darnautov

LGTM!

elasticmachine · 2019-11-18T17:23:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 94598c4

* create modelMemoryLimit estimation endpoint. add value to form * add validation for model memory limit field * update jest tests * update validateModelMemoryLimitUnitsUtils to be more generic * add placeholder and validation with helpText to modelMemoryLimit field * update endpoint name to estimateDataFrameAnalyticsMemoryUsage for clarity * tweak modelMemoryLimitEmpty check in reducer * add tests for modelMemoryLimit validation

…-fallback * 'master' of github.com:elastic/kibana: (116 commits) [Maps] move apply global filter settting from layer to source (elastic#50523) [SIEM] Fix: Empty `Source` / `Destination` shown when only ports are populated (elastic#50843) [Maps] Delay vector tile layer syncing until spritesheet is loaded (elastic#48955) [Maps] prevent users from overflowing URL when filtering by shape (elastic#50747) [DOCS] Mark Beats central management as discontinued (elastic#49423) [page_objects/common_page] convert to ts (elastic#50771) [NP Kibana Migrations ] kibana plugin home (elastic#50444) [DOCS] Shareables naming convention (elastic#50497) [ML] DF Analytics - auto-populate model_memory_limit (elastic#50714) Increase alerting test stability and reduce flakiness (elastic#50246) [ML] Remaning new_job_new folder (elastic#50917) [Telemetry] Show opt-in changes for OSS users (elastic#50831) [ML] Fix lat_long anomalies table links menu and value formatting (elastic#50916) [Dev] Fix serialising a really big string (elastic#50915) Better explanation about the Prettier recommendation (extension vs. NPM module) (elastic#50629) [Monitoring] Use a basic monitoring user for tests (elastic#47865) [Monitoring] Gracefully handle issue with filebeat indices (elastic#48929) [Monitoring] Improve permissions required around setup mode (elastic#50421) Additional validation for elasticsearch username (elastic#48247) Revert changes to use_kibana_ui_setting (elastic#50877) ... # Conflicts: # src/legacy/core_plugins/console/server/request.test.ts

alvarezmelissa87 added release_note:enhancement :ml v8.0.0 Feature:Data Frame Analytics ML data frame analytics features v7.6.0 labels Nov 14, 2019

alvarezmelissa87 requested a review from a team as a code owner November 14, 2019 19:38

alvarezmelissa87 self-assigned this Nov 14, 2019

alvarezmelissa87 requested review from jgowdyelastic, peteharverson, walterra and darnautov November 14, 2019 19:45

alvarezmelissa87 changed the title ~~Ml regression config mmlimit~~ [ML] DF Analytics - auto-populate model_memory_limit Nov 14, 2019

alvarezmelissa87 mentioned this pull request Nov 14, 2019

[ML] Meta - Regression UI #47890

Closed

46 tasks

darnautov reviewed Nov 15, 2019

View reviewed changes

peteharverson reviewed Nov 15, 2019

View reviewed changes

alvarezmelissa87 force-pushed the ml-regression-config-mmlimit branch from b605812 to d30ee0f Compare November 15, 2019 14:59

walterra reviewed Nov 18, 2019

View reviewed changes

peteharverson approved these changes Nov 18, 2019

View reviewed changes

alvarezmelissa87 added 6 commits November 18, 2019 10:24

create modelMemoryLimit estimation endpoint. add value to form

b25cab8

add validation for model memory limit field

f5e80e8

update jest tests

c8877e9

update validateModelMemoryLimitUnitsUtils to be more generic

f13927f

add placeholder and validation with helpText to modelMemoryLimit field

211762d

update endpoint name to estimateDataFrameAnalyticsMemoryUsage for cla…

74133c7

…rity

tweak modelMemoryLimitEmpty check in reducer

0c4f7d8

alvarezmelissa87 force-pushed the ml-regression-config-mmlimit branch from cb14d92 to 0c4f7d8 Compare November 18, 2019 15:24

add tests for modelMemoryLimit validation

94598c4

walterra approved these changes Nov 18, 2019

View reviewed changes

darnautov approved these changes Nov 18, 2019

View reviewed changes

alvarezmelissa87 merged commit dd06bfc into elastic:master Nov 18, 2019

alvarezmelissa87 mentioned this pull request Nov 18, 2019

[7.x] [ML] DF Analytics - auto-populate model_memory_limit (#50714) #50947

Merged

alvarezmelissa87 deleted the ml-regression-config-mmlimit branch November 18, 2019 18:23

alvarezmelissa87 mentioned this pull request Nov 18, 2019

[ML] Improve UX regarding df analytics model memory limit #43740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] DF Analytics - auto-populate model_memory_limit #50714

[ML] DF Analytics - auto-populate model_memory_limit #50714

alvarezmelissa87 commented Nov 14, 2019 •

edited

Loading

elasticmachine commented Nov 14, 2019

elasticmachine commented Nov 14, 2019

elasticmachine commented Nov 14, 2019

alvarezmelissa87 commented Nov 14, 2019

darnautov Nov 15, 2019

alvarezmelissa87 Nov 15, 2019

darnautov Nov 15, 2019

alvarezmelissa87 Nov 15, 2019

peteharverson Nov 15, 2019

peteharverson Nov 15, 2019

alvarezmelissa87 Nov 15, 2019

peteharverson Nov 15, 2019

alvarezmelissa87 Nov 15, 2019

peteharverson Nov 15, 2019

alvarezmelissa87 Nov 15, 2019

elasticmachine commented Nov 15, 2019

elasticmachine commented Nov 15, 2019

walterra left a comment

peteharverson left a comment

alvarezmelissa87 commented Nov 18, 2019

walterra left a comment

darnautov left a comment

elasticmachine commented Nov 18, 2019

		const jobConfig = getJobConfigFromFormState(form);
		delete jobConfig.dest;

[ML] DF Analytics - auto-populate model_memory_limit #50714

[ML] DF Analytics - auto-populate model_memory_limit #50714

Conversation

alvarezmelissa87 commented Nov 14, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Nov 14, 2019

elasticmachine commented Nov 14, 2019

💔 Build Failed

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

alvarezmelissa87 commented Nov 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 15, 2019

💚 Build Succeeded

elasticmachine commented Nov 15, 2019

💚 Build Succeeded

walterra left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

alvarezmelissa87 commented Nov 18, 2019

walterra left a comment

Choose a reason for hiding this comment

darnautov left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 18, 2019

💚 Build Succeeded

alvarezmelissa87 commented Nov 14, 2019 •

edited

Loading