-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Changes from 6 commits
92b9fbb
fd5debb
c41b081
610cff9
a2fac52
e15558d
f5d3ce6
e50e4e1
0d64859
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'; | ||
|
||
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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, there are three different function definition formats in this file: Unless the function is in the |
||
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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this isn't triggering the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
$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.`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
$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 => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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 |
---|---|---|
|
@@ -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 }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. object literal keys shouldn't be quoted There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,3 +7,4 @@ | |
|
||
|
||
export { validateJob } from './job_validation'; | ||
export { validateCardinality } from './validate_cardinality'; |
There was a problem hiding this comment.
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