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] MultiMetric/Population Job creation: Allow model plot enablement via checkbox #24914

Merged
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
import ReactDOM from 'react-dom';

import { EnableModelPlotCheckbox } from './enable_model_plot_checkbox_view.js';
import { ml } from 'plugins/ml/services/ml_api_service';
Copy link
Member

Choose a reason for hiding this comment

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

nit, although this file has no jest tests, it's still a good habit to use relative paths for includes


import { uiModules } from 'ui/modules';
const module = uiModules.get('apps/ml');

module.directive('mlEnableModelPlotCheckbox', function () {
return {
restrict: 'AE',
replace: false,
scope: {
formConfig: '=',
ui: '=ui',
getJobFromConfig: '='
},
link: function ($scope, $element) {
const STATUS = {
FAILED: -1,
NOT_RUNNING: 0,
RUNNING: 1,
FINISHED: 2,
WARNING: 3,
};
const errorHandler = (error) => {
Copy link
Member

Choose a reason for hiding this comment

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

nit, there are three different function definition formats in this file:
Arrow: const errorHandler = (error) => {
Expression: const validateCardinality = function () {
Declaration: function updateCheckbox() {

Unless the function is in the $scope, we should use function declarations. Kibana also mentions this in their style guide.
https://github.com/elastic/kibana/blob/master/style_guides/js_style_guide.md#function-definitions

console.log('Cardinality could not be validated', error);
$scope.ui.cardinalityValidator.status = STATUS.FAILED;
$scope.ui.cardinalityValidator.message = 'Cardinality could not be validated';
};

// Only model plot cardinality relevant
// format:[{id:"cardinality_model_plot_high",modelPlotCardinality:11405}, {id:"cardinality_partition_field",fieldName:"clientip"}]
const checkCardinalitySuccess = (data) => {
const response = {
success: true,
};
// There were no fields to run cardinality on.
if (Array.isArray(data) && data.length === 0) {
return response;
}

for (let i = 0; i < data.length; i++) {
if (data[i].id === 'success_cardinality') {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if these IDs used for job validation messages were in a constants file, but not needed for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good for the follow up PR with the advanced wizard. 👍

break;
}

if (data[i].id === 'cardinality_model_plot_high') {
response.success = false;
response.highCardinality = data[i].modelPlotCardinality;
break;
}
}

return response;
};

const validateCardinality = function () {
$scope.ui.cardinalityValidator.status = STATUS.RUNNING;
Copy link
Member

Choose a reason for hiding this comment

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

this isn't triggering the $watch when toggling the checkbox.
This is due to the toggle happing in react, so the call to this function isn't trigging the digest cycle.
I'd suggest adding a $scope.$applyAsync(); call to the bottom of this function and to the bottom of the (response) => { function

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Nov 5, 2018

Choose a reason for hiding this comment

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

Hey I'm not quite sure what you mean it isn't triggering the $watch - when I tested it I can see the copy of the checkbox changing to Validating cardinality... as it should when $scope.ui.cardinalityValidator.status is updated to RUNNING. I did have to put a setTimeout on it to test it though since it happens so fast. Is that what you're referring to? cc @peteharverson, @walterra if you see something similar

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll try to reproduce on a local checkout. IMHO if we need to use $applyAsync then all the code needs to be wrapped in the callback of $applyAsync, not the other way around :)

$scope.ui.cardinalityValidator.message = '';

// create temporary job since cardinality validation expects that format
const tempJob = $scope.getJobFromConfig($scope.formConfig);

ml.validateCardinality(tempJob)
.then((response) => {
const validationResult = checkCardinalitySuccess(response);

if (validationResult.success === true) {
$scope.formConfig.enableModelPlot = true;
$scope.ui.cardinalityValidator.status = STATUS.FINISHED;
} else {
$scope.ui.cardinalityValidator.message = `The estimated cardinality of ${validationResult.highCardinality}
of fields relevant to creating model plots might result in resource intensive jobs.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Only one job is getting created here, so change the end part of the message to 'in a resource intensive job'. Does the response. Is there any way we could use the same String as the job_validation code on the server? That might be best left for the localization work?

$scope.ui.cardinalityValidator.status = STATUS.WARNING;
// show the advanced section so the warning message is visible since validation failed
$scope.ui.showAdvanced = true;
}
})
.catch(errorHandler);
};

// Re-validate cardinality for updated fields/splitField
// when enable model plot is checked and form valid
const revalidateCardinalityOnFieldChange = () => {
if ($scope.formConfig.enableModelPlot === true && $scope.ui.formValid === true) {
validateCardinality();
}
};

$scope.handleCheckboxChange = (isChecked) => {
if (isChecked) {
$scope.formConfig.enableModelPlot = true;
validateCardinality();
} else {
$scope.formConfig.enableModelPlot = false;
$scope.ui.cardinalityValidator.status = STATUS.FINISHED;
$scope.ui.cardinalityValidator.message = '';
}
};

// Update checkbox on these changes
$scope.$watch('ui.formValid', updateCheckbox, true);
$scope.$watch('ui.cardinalityValidator.status', updateCheckbox, true);
// MultiMetric: Fire off cardinality validatation when fields and/or split by field is updated
$scope.$watch('formConfig.fields', revalidateCardinalityOnFieldChange, true);
$scope.$watch('formConfig.splitField', revalidateCardinalityOnFieldChange, true);
// Population: Fire off cardinality validatation when overField is updated
$scope.$watch('formConfig.overField', revalidateCardinalityOnFieldChange, true);

function updateCheckbox() {
// disable if (check is running && checkbox checked) or (form is invalid && checkbox unchecked)
const checkboxDisabled = (
($scope.ui.cardinalityValidator.status === STATUS.RUNNING &&
$scope.formConfig.enableModelPlot === true) ||
($scope.ui.formValid !== true &&
$scope.formConfig.enableModelPlot === false)
);
const validatorRunning = ($scope.ui.cardinalityValidator.status === STATUS.RUNNING);
const warningStatus = ($scope.ui.cardinalityValidator.status === STATUS.WARNING);
const checkboxText = (validatorRunning) ? 'Validating cardinality...' : 'Enable model plot';

const props = {
checkboxDisabled,
checkboxText,
onCheckboxChange: $scope.handleCheckboxChange,
warningContent: $scope.ui.cardinalityValidator.message,
warningStatus,
};

ReactDOM.render(
React.createElement(EnableModelPlotCheckbox, props),
$element[0]
);
}

updateCheckbox();
}
};
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/



import PropTypes from 'prop-types';
import React, { Fragment, Component } from 'react';

import {
EuiCallOut,
EuiCheckbox,
EuiFlexGroup,
EuiFlexItem,
EuiIconTip,
} from '@elastic/eui';


export class EnableModelPlotCheckbox extends Component {
constructor(props) {
super(props);

this.state = {
checked: false,
};
}

tooltipContent = `Select to enable model plot. Stores model information along with results.
Can add considerable overhead to the performance of the system.`;

warningTitle = 'Proceed with caution!';

onChange = e => {
Copy link
Member

Choose a reason for hiding this comment

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

nit, when an arrow function uses curlies, we tend to wrap the parameters in parentheses.
onChange = (e) => {
https://github.com/elastic/kibana/blob/master/style_guides/js_style_guide.md#arrow-functions

this.setState({
checked: e.target.checked,
});

this.props.onCheckboxChange(e.target.checked);
};

renderWarningCallout = () => (
<Fragment>
<EuiFlexGroup direction="column">
<EuiFlexItem grow={false}>
<EuiCallOut
title={this.warningTitle}
color="warning"
iconType="help"
>
<p>
{this.props.warningContent}
</p>
</EuiCallOut>
</EuiFlexItem>
</EuiFlexGroup>
</Fragment>
);

render() {
return (
<Fragment>
<EuiFlexGroup alignItems="center" gutterSize="s" responsive={false}>
<EuiFlexItem grow={false}>
<EuiCheckbox
id="new_job_enable_model_plot"
label={this.props.checkboxText}
onChange={this.onChange}
disabled={this.props.checkboxDisabled}
checked={this.state.checked}
/>
</EuiFlexItem>

<EuiFlexItem grow={false}>
<EuiIconTip
content={this.tooltipContent}
size={'s'}
/>
</EuiFlexItem>
</EuiFlexGroup>
{ this.props.warningStatus && this.renderWarningCallout() }
</Fragment>
);
}
}

EnableModelPlotCheckbox.propTypes = {
checkboxDisabled: PropTypes.bool,
checkboxText: PropTypes.string.isRequired,
onCheckboxChange: PropTypes.func.isRequired,
warningStatus: PropTypes.bool.isRequired,
warningContent: PropTypes.string.isRequired,
};

EnableModelPlotCheckbox.defaultProps = {
checkboxDisabled: false,
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/


import './enable_model_plot_checkbox_directive.js';
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@
<i ml-info-icon="new_job_advanced_settings" ></i>
</div>
<div class='advanced-group' ng-show="ui.showAdvanced">
<div class="form-group">
<ml-enable-model-plot-checkbox
form-config='formConfig'
ui='ui'
get-job-from-config='getJobFromConfig'>
</ml-enable-model-plot-checkbox>
</div>
<div class="form-group">
<label class='kuiCheckBoxLabel kuiVerticalRhythm'>
<input type="checkbox"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ module
formValid: false,
bucketSpanValid: true,
bucketSpanEstimator: { status: 0, message: '' },
cardinalityValidator: { status: 0, message: '' },
aggTypeOptions: filterAggTypes(aggTypes.byType[METRIC_AGG_TYPE]),
fields: [],
splitFields: [],
Expand Down Expand Up @@ -202,6 +203,7 @@ module
description: '',
jobGroups: [],
useDedicatedIndex: false,
enableModelPlot: false,
isSparseData: false,
modelMemoryLimit: DEFAULT_MODEL_MEMORY_LIMIT
};
Expand Down Expand Up @@ -513,6 +515,9 @@ module
}
};

// expose this function so it can be used in the enable model plot checkbox directive
$scope.getJobFromConfig = mlMultiMetricJobService.getJobFromConfig;

addJobValidationMethods($scope, mlMultiMetricJobService);

function loadCharts() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,12 @@ export function MultiMetricJobServiceProvider() {
const job = mlJobService.getBlankJob();
job.data_description.time_field = formConfig.timeField;

if (formConfig.enableModelPlot === true) {
job.model_plot_config = { 'enabled': true };
Copy link
Member

Choose a reason for hiding this comment

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

object literal keys shouldn't be quoted

} else if (formConfig.enableModelPlot === false) {
delete job.model_plot_config;
}

_.each(formConfig.fields, (field, key) => {
let func = field.agg.type.mlName;
if (formConfig.isSparseData) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import 'plugins/ml/jobs/new_job/simple/components/fields_selection';
import 'plugins/ml/jobs/new_job/simple/components/influencers_selection';
import 'plugins/ml/jobs/new_job/simple/components/bucket_span_selection';
import 'plugins/ml/jobs/new_job/simple/components/general_job_details';
import 'plugins/ml/jobs/new_job/simple/components/enable_model_plot_checkbox';
import 'plugins/ml/jobs/new_job/simple/components/agg_types_filter';
import 'plugins/ml/components/job_group_select';
import 'plugins/ml/components/full_time_range_selector';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ module
formValid: false,
bucketSpanValid: true,
bucketSpanEstimator: { status: 0, message: '' },
cardinalityValidator: { status: 0, message: '' },
aggTypeOptions: filterAggTypes(aggTypes.byType[METRIC_AGG_TYPE]),
fields: [],
overFields: [],
Expand Down Expand Up @@ -207,6 +208,7 @@ module
description: '',
jobGroups: [],
useDedicatedIndex: false,
enableModelPlot: false,
modelMemoryLimit: DEFAULT_MODEL_MEMORY_LIMIT
};

Expand Down Expand Up @@ -540,6 +542,9 @@ module
}
};

// expose this function so it can be used in the enable model plot checkbox directive
$scope.getJobFromConfig = mlPopulationJobService.getJobFromConfig;

addJobValidationMethods($scope, mlPopulationJobService);

function loadCharts() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,12 @@ export function PopulationJobServiceProvider(Private) {
const job = mlJobService.getBlankJob();
job.data_description.time_field = formConfig.timeField;

if (formConfig.enableModelPlot === true) {
job.model_plot_config = { 'enabled': true };
Copy link
Member

Choose a reason for hiding this comment

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

object literal keys shouldn't be quoted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! thanks 😄

} else if (formConfig.enableModelPlot === false) {
delete job.model_plot_config;
}

formConfig.fields.forEach(field => {
let func = field.agg.type.mlName;
if (formConfig.isSparseData) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import 'plugins/ml/jobs/new_job/simple/components/fields_selection_population';
import 'plugins/ml/jobs/new_job/simple/components/influencers_selection';
import 'plugins/ml/jobs/new_job/simple/components/bucket_span_selection';
import 'plugins/ml/jobs/new_job/simple/components/general_job_details';
import 'plugins/ml/jobs/new_job/simple/components/enable_model_plot_checkbox';
import 'plugins/ml/jobs/new_job/simple/components/agg_types_filter';
import 'plugins/ml/components/job_group_select';
import 'plugins/ml/components/full_time_range_selector';
Expand Down
8 changes: 8 additions & 0 deletions x-pack/plugins/ml/public/services/ml_api_service/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,14 @@ export const ml = {
});
},

validateCardinality(obj) {
return http({
url: `${basePath}/validate/cardinality`,
method: 'POST',
data: obj
});
},

getDatafeeds(obj) {
const datafeedId = (obj && obj.datafeedId) ? `/${obj.datafeedId}` : '';
return http({
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/ml/server/models/job_validation/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@


export { validateJob } from './job_validation';
export { validateCardinality } from './validate_cardinality';
Loading