Skip to content

Commit

Permalink
[ML] Prevent training_percent of 0 for analytics job (#61789)
Browse files Browse the repository at this point in the history
  • Loading branch information
peteharverson authored Mar 30, 2020
1 parent 4d8bae4 commit 300df64
Show file tree
Hide file tree
Showing 7 changed files with 104 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ export interface LoadExploreDataArg {

export const SEARCH_SIZE = 1000;

export const TRAINING_PERCENT_MIN = 1;
export const TRAINING_PERCENT_MAX = 100;

export const defaultSearchQuery = {
match_all: {},
};
Expand Down Expand Up @@ -172,6 +175,19 @@ export const getDependentVar = (analysis: AnalysisConfig) => {
return depVar;
};

export const getTrainingPercent = (analysis: AnalysisConfig) => {
let trainingPercent;

if (isRegressionAnalysis(analysis)) {
trainingPercent = analysis.regression.training_percent;
}

if (isClassificationAnalysis(analysis)) {
trainingPercent = analysis.classification.training_percent;
}
return trainingPercent;
};

export const getPredictionFieldName = (analysis: AnalysisConfig) => {
// If undefined will be defaulted to dependent_variable when config is created
let predictionFieldName;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,12 @@ import {
} from '@elastic/eui';

import { i18n } from '@kbn/i18n';
import { collapseLiteralStrings } from '../../../../../../../../../../src/plugins/es_ui_shared/console_lang/lib/json_xjson_translation_tools';

import { CreateAnalyticsFormProps } from '../../hooks/use_create_analytics_form';
import { xJsonMode } from '../../../../../components/custom_hooks';

export const CreateAnalyticsAdvancedEditor: FC<CreateAnalyticsFormProps> = ({ actions, state }) => {
const {
resetAdvancedEditorMessages,
setAdvancedEditorRawString,
setFormState,
setJobConfig,
} = actions;
const { setAdvancedEditorRawString, setFormState } = actions;

const { advancedEditorMessages, advancedEditorRawString, isJobCreated, requestMessages } = state;

Expand All @@ -45,12 +39,6 @@ export const CreateAnalyticsAdvancedEditor: FC<CreateAnalyticsFormProps> = ({ ac

const onChange = (str: string) => {
setAdvancedEditorRawString(str);
try {
const resultJobConfig = JSON.parse(collapseLiteralStrings(str));
setJobConfig(resultJobConfig);
} catch (e) {
resetAdvancedEditorMessages();
}
};

// Temp effect to close the context menu popover on Clone button click
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,14 @@ export const CreateAnalyticsFlyout: FC<CreateAnalyticsFormProps> = ({
state,
}) => {
const { closeModal, createAnalyticsJob, startAnalyticsJob } = actions;
const { isJobCreated, isJobStarted, isModalButtonDisabled, isValid, cloneJob } = state;
const {
isJobCreated,
isJobStarted,
isModalButtonDisabled,
isValid,
isAdvancedEditorValidJson,
cloneJob,
} = state;

const headerText = !!cloneJob
? i18n.translate('xpack.ml.dataframe.analytics.clone.flyoutHeaderTitle', {
Expand Down Expand Up @@ -61,7 +68,7 @@ export const CreateAnalyticsFlyout: FC<CreateAnalyticsFormProps> = ({
{!isJobCreated && !isJobStarted && (
<EuiButton
className="mlAnalyticsCreateFlyout__footerButton"
disabled={!isValid || isModalButtonDisabled}
disabled={!isValid || !isAdvancedEditorValidJson || isModalButtonDisabled}
onClick={createAnalyticsJob}
fill
data-test-subj="mlAnalyticsCreateJobFlyoutCreateButton"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ import {
ANALYSIS_CONFIG_TYPE,
DfAnalyticsExplainResponse,
FieldSelectionItem,
TRAINING_PERCENT_MIN,
TRAINING_PERCENT_MAX,
} from '../../../../common/analytics';
import { shouldAddAsDepVarOption, OMIT_FIELDS } from './form_options_validation';

Expand Down Expand Up @@ -631,15 +633,15 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
})}
>
<EuiRange
min={0}
max={100}
min={TRAINING_PERCENT_MIN}
max={TRAINING_PERCENT_MAX}
step={1}
showLabels
showRange
showValue
value={trainingPercent}
// @ts-ignore Property 'value' does not exist on type 'EventTarget' | (EventTarget & HTMLInputElement)
onChange={e => setFormState({ trainingPercent: e.target.value })}
onChange={e => setFormState({ trainingPercent: +e.target.value })}
data-test-subj="mlAnalyticsCreateJobFlyoutTrainingPercentSlider"
/>
</EuiFormRow>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@ type SourceIndex = DataFrameAnalyticsConfig['source']['index'];

const getMockState = ({
index,
trainingPercent = 75,
modelMemoryLimit = '100mb',
}: {
index: SourceIndex;
trainingPercent?: number;
modelMemoryLimit?: string;
}) =>
merge(getInitialState(), {
Expand All @@ -31,7 +33,9 @@ const getMockState = ({
jobConfig: {
source: { index },
dest: { index: 'the-destination-index' },
analysis: {},
analysis: {
classification: { dependent_variable: 'the-variable', training_percent: trainingPercent },
},
model_memory_limit: modelMemoryLimit,
},
});
Expand Down Expand Up @@ -151,6 +155,24 @@ describe('useCreateAnalyticsForm', () => {
.isValid
).toBe(false);
});

test('validateAdvancedEditor(): check training percent validation', () => {
// valid training_percent value
expect(
validateAdvancedEditor(getMockState({ index: 'the-source-index', trainingPercent: 75 }))
.isValid
).toBe(true);
// invalid training_percent numeric value
expect(
validateAdvancedEditor(getMockState({ index: 'the-source-index', trainingPercent: 102 }))
.isValid
).toBe(false);
// invalid training_percent numeric value if 0
expect(
validateAdvancedEditor(getMockState({ index: 'the-source-index', trainingPercent: 0 }))
.isValid
).toBe(false);
});
});

describe('validateMinMML', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import numeral from '@elastic/numeral';
import { isEmpty } from 'lodash';
import { isValidIndexName } from '../../../../../../../common/util/es_utils';

import { collapseLiteralStrings } from '../../../../../../../../../../src/plugins/es_ui_shared/console_lang/lib/json_xjson_translation_tools';

import { Action, ACTION } from './actions';
import { getInitialState, getJobConfigFromFormState, State } from './state';
import {
Expand All @@ -29,9 +31,12 @@ import {
} from '../../../../../../../common/constants/validation';
import {
getDependentVar,
getTrainingPercent,
isRegressionAnalysis,
isClassificationAnalysis,
ANALYSIS_CONFIG_TYPE,
TRAINING_PERCENT_MIN,
TRAINING_PERCENT_MAX,
} from '../../../../common/analytics';
import { indexPatterns } from '../../../../../../../../../../src/plugins/data/public';

Expand Down Expand Up @@ -141,6 +146,7 @@ export const validateAdvancedEditor = (state: State): State => {

let dependentVariableEmpty = false;
let excludesValid = true;
let trainingPercentValid = true;

if (
jobConfig.analysis === undefined &&
Expand Down Expand Up @@ -169,6 +175,30 @@ export const validateAdvancedEditor = (state: State): State => {
message: '',
});
}

const trainingPercent = getTrainingPercent(jobConfig.analysis);
if (
trainingPercent !== undefined &&
(isNaN(trainingPercent) ||
trainingPercent < TRAINING_PERCENT_MIN ||
trainingPercent > TRAINING_PERCENT_MAX)
) {
trainingPercentValid = false;

state.advancedEditorMessages.push({
error: i18n.translate(
'xpack.ml.dataframe.analytics.create.advancedEditorMessage.trainingPercentInvalid',
{
defaultMessage: 'The training percent must be a value between {min} and {max}.',
values: {
min: TRAINING_PERCENT_MIN,
max: TRAINING_PERCENT_MAX,
},
}
),
message: '',
});
}
}

if (sourceIndexNameEmpty) {
Expand Down Expand Up @@ -249,6 +279,7 @@ export const validateAdvancedEditor = (state: State): State => {
state.isValid =
maxDistinctValuesError === undefined &&
excludesValid &&
trainingPercentValid &&
state.form.modelMemoryLimitUnitValid &&
!jobIdEmpty &&
jobIdValid &&
Expand Down Expand Up @@ -365,7 +396,23 @@ export function reducer(state: State, action: Action): State {
return getInitialState();

case ACTION.SET_ADVANCED_EDITOR_RAW_STRING:
return { ...state, advancedEditorRawString: action.advancedEditorRawString };
let resultJobConfig;
try {
resultJobConfig = JSON.parse(collapseLiteralStrings(action.advancedEditorRawString));
} catch (e) {
return {
...state,
advancedEditorRawString: action.advancedEditorRawString,
isAdvancedEditorValidJson: false,
advancedEditorMessages: [],
};
}

return {
...validateAdvancedEditor({ ...state, jobConfig: resultJobConfig }),
advancedEditorRawString: action.advancedEditorRawString,
isAdvancedEditorValidJson: true,
};

case ACTION.SET_FORM_STATE:
const newFormState = { ...state.form, ...action.payload };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export interface State {
indexNames: EsIndexName[];
indexPatternsMap: SourceIndexMap;
isAdvancedEditorEnabled: boolean;
isAdvancedEditorValidJson: boolean;
isJobCreated: boolean;
isJobStarted: boolean;
isModalButtonDisabled: boolean;
Expand Down Expand Up @@ -140,6 +141,7 @@ export const getInitialState = (): State => ({
indexNames: [],
indexPatternsMap: {},
isAdvancedEditorEnabled: false,
isAdvancedEditorValidJson: true,
isJobCreated: false,
isJobStarted: false,
isModalVisible: false,
Expand Down

0 comments on commit 300df64

Please sign in to comment.