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] Better model memory limit validation #21270

Merged

Conversation

jgowdyelastic
Copy link
Member

Adds validation for the model memory limit value which checks that the unit is an elastic search support unit specified here: https://www.elastic.co/guide/en/elasticsearch/reference/6.x/common-options.html#byte-units

This is run with the normal server side job validation as well as in the job creation pages and in the edit job flyout.

Edit job flyout

image

Single metric wizard

image

Multi-metric/Population wizards

image

Advanced job creation

image

Job validation modal

image

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

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.

A comment about the units in the error message, but otherwise LGTM

@@ -1026,6 +1026,12 @@ module.controller('MlNewJob',
tabs[0].checks.groupIds.message = msg;
}

if (validationResults.contains('model_memory_limit_units_invalid')) {
tabs[0].checks.modelMemoryLimit.valid = false;
const msg = `Model memory limit data unit unrecognized. It must be B, KB, MB, GB, TB or PB`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally the units in the message should come from the new ALLOWED_DATA_UNITS constant

@@ -50,6 +50,12 @@ export function populateValidationMessages(validationResults, checks) {
checks.groupIds.message = msg;
}

if (validationResults.contains('model_memory_limit_units_invalid')) {
checks.modelMemoryLimit.valid = false;
const msg = `Model memory limit data unit unrecognized. It must be B, KB, MB, GB, TB or PB`;
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, the units referenced in the message should come from ALLOWED_DATA_UNITS .

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jgowdyelastic jgowdyelastic merged commit 60706bf into elastic:master Jul 27, 2018
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Jul 27, 2018
@lcawl lcawl added the enhancement New value added to drive a business result label Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Anomaly Detection ML anomaly detection :ml review v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants