Skip to content

Commit

Permalink
[ML] Validate manual model memory input (elastic#59056)
Browse files Browse the repository at this point in the history 
* [ML] validate mml based on estimated value

* [ML] better memoize

* [ML] memoryInputValidator unit tests

* [ML] cache mml errors

* [ML] prevent override

* [ML] fix validators, add unit tests

* [ML] ignore typing issue with numeral

* [ML] fix validateMinMML

* [ML] fix useCreateAnalyticsForm test

* [ML] setEstimatedModelMemoryLimit to the fallback value in case of an error
  • Loading branch information
@darnautov
darnautov committed Mar 4, 2020
1 parent 617c874 commit 4edde17
Show file tree
Hide file tree
Showing 8 changed files with 197 additions and 26 deletions.
28 changes: 27 additions & 1 deletion x-pack/legacy/plugins/ml/common/util/validators.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { maxLengthValidator } from './validators';
import { maxLengthValidator, memoryInputValidator } from './validators';

describe('maxLengthValidator', () => {
test('should allow a valid input', () => {
Expand All @@ -20,3 +20,29 @@ describe('maxLengthValidator', () => {
});
});
});

describe('memoryInputValidator', () => {
test('should detect missing units', () => {
expect(memoryInputValidator()('10')).toEqual({
invalidUnits: {
allowedUnits: 'B, KB, MB, GB, TB, PB',
},
});
});

test('should accept valid input', () => {
expect(memoryInputValidator()('100PB')).toEqual(null);
});

test('should accept valid input with custom allowed units', () => {
expect(memoryInputValidator(['B', 'KB'])('100KB')).toEqual(null);
});

test('should detect not allowed units', () => {
expect(memoryInputValidator(['B', 'KB'])('100MB')).toEqual({
invalidUnits: {
allowedUnits: 'B, KB',
},
});
});
});
24 changes: 22 additions & 2 deletions x-pack/legacy/plugins/ml/common/util/validators.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { ALLOWED_DATA_UNITS } from '../constants/validation';

/**
* Provides a validator function for maximum allowed input length.
* @param maxLength Maximum length allowed.
Expand Down Expand Up @@ -44,8 +46,8 @@ export function patternValidator(
* @param validators
*/
export function composeValidators(
...validators: Array<(value: string) => { [key: string]: any } | null>
): (value: string) => { [key: string]: any } | null {
...validators: Array<(value: any) => { [key: string]: any } | null>
): (value: any) => { [key: string]: any } | null {
return value => {
const validationResult = validators.reduce((acc, validator) => {
return {
Expand All @@ -56,3 +58,21 @@ export function composeValidators(
return Object.keys(validationResult).length > 0 ? validationResult : null;
};
}

export function requiredValidator() {
return (value: any) => {
return value === '' || value === undefined || value === null ? { required: true } : null;
};
}

export function memoryInputValidator(allowedUnits = ALLOWED_DATA_UNITS) {
return (value: any) => {
if (typeof value !== 'string' || value === '') {
return null;
}
const regexp = new RegExp(`\\d+(${allowedUnits.join('|')})$`, 'i');
return regexp.test(value.trim())
? null
: { invalidUnits: { allowedUnits: allowedUnits.join(', ') } };
};
}
34 changes: 22 additions & 12 deletions ...ics/pages/analytics_management/components/create_analytics_form/create_analytics_form.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React, { Fragment, FC, useEffect } from 'react';
import React, { Fragment, FC, useEffect, useMemo } from 'react';

import {
EuiComboBox,
Expand Down Expand Up @@ -36,7 +36,7 @@ import { JOB_ID_MAX_LENGTH } from '../../../../../../../common/constants/validat
import { Messages } from './messages';
import { JobType } from './job_type';
import { JobDescriptionInput } from './job_description';
import { mmlUnitInvalidErrorMessage } from '../../hooks/use_create_analytics_form/reducer';
import { getModelMemoryLimitErrors } from '../../hooks/use_create_analytics_form/reducer';
import {
IndexPattern,
indexPatterns,
Expand All @@ -49,7 +49,7 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
services: { docLinks },
} = useMlKibana();
const { ELASTIC_WEBSITE_URL, DOC_LINK_VERSION } = docLinks;
const { setFormState } = actions;
const { setFormState, setEstimatedModelMemoryLimit } = actions;
const mlContext = useMlContext();
const { form, indexPatternsMap, isAdvancedEditorEnabled, isJobCreated, requestMessages } = state;

Expand Down Expand Up @@ -77,7 +77,7 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
loadingFieldOptions,
maxDistinctValuesError,
modelMemoryLimit,
modelMemoryLimitUnitValid,
modelMemoryLimitValidationResult,
previousJobType,
previousSourceIndex,
sourceIndex,
Expand All @@ -89,6 +89,10 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
} = form;
const characterList = indexPatterns.ILLEGAL_CHARACTERS_VISIBLE.join(', ');

const mmlErrors = useMemo(() => getModelMemoryLimitErrors(modelMemoryLimitValidationResult), [
modelMemoryLimitValidationResult,
]);

const isJobTypeWithDepVar =
jobType === JOB_TYPES.REGRESSION || jobType === JOB_TYPES.CLASSIFICATION;

Expand Down Expand Up @@ -154,6 +158,9 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
const resp: DfAnalyticsExplainResponse = await ml.dataFrameAnalytics.explainDataFrameAnalytics(
jobConfig
);
const expectedMemoryWithoutDisk = resp.memory_estimation?.expected_memory_without_disk;

setEstimatedModelMemoryLimit(expectedMemoryWithoutDisk);

// If sourceIndex has changed load analysis field options again
if (previousSourceIndex !== sourceIndex || previousJobType !== jobType) {
Expand All @@ -168,15 +175,15 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
}

setFormState({
modelMemoryLimit: resp.memory_estimation?.expected_memory_without_disk,
...(!modelMemoryLimit ? { modelMemoryLimit: expectedMemoryWithoutDisk } : {}),
excludesOptions: analyzedFieldsOptions,
loadingFieldOptions: false,
fieldOptionsFetchFail: false,
maxDistinctValuesError: undefined,
});
} else {
setFormState({
modelMemoryLimit: resp.memory_estimation?.expected_memory_without_disk,
...(!modelMemoryLimit ? { modelMemoryLimit: expectedMemoryWithoutDisk } : {}),
});
}
} catch (e) {
Expand All @@ -189,14 +196,16 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
) {
errorMessage = e.message;
}
const fallbackModelMemoryLimit =
jobType !== undefined
? DEFAULT_MODEL_MEMORY_LIMIT[jobType]
: DEFAULT_MODEL_MEMORY_LIMIT.outlier_detection;
setEstimatedModelMemoryLimit(fallbackModelMemoryLimit);
setFormState({
fieldOptionsFetchFail: true,
maxDistinctValuesError: errorMessage,
loadingFieldOptions: false,
modelMemoryLimit:
jobType !== undefined
? DEFAULT_MODEL_MEMORY_LIMIT[jobType]
: DEFAULT_MODEL_MEMORY_LIMIT.outlier_detection,
modelMemoryLimit: fallbackModelMemoryLimit,
});
}
}, 400);
Expand Down Expand Up @@ -642,7 +651,8 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
label={i18n.translate('xpack.ml.dataframe.analytics.create.modelMemoryLimitLabel', {
defaultMessage: 'Model memory limit',
})}
helpText={!modelMemoryLimitUnitValid && mmlUnitInvalidErrorMessage}
isInvalid={modelMemoryLimitValidationResult !== null}
error={mmlErrors}
>
<EuiFieldText
placeholder={
Expand All @@ -653,7 +663,7 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
disabled={isJobCreated}
value={modelMemoryLimit || ''}
onChange={e => setFormState({ modelMemoryLimit: e.target.value })}
isInvalid={modelMemoryLimit === ''}
isInvalid={modelMemoryLimitValidationResult !== null}
data-test-subj="mlAnalyticsCreateJobFlyoutModelMemoryInput"
/>
</EuiFormRow>
Expand Down
5 changes: 4 additions & 1 deletion ...ata_frame_analytics/pages/analytics_management/hooks/use_create_analytics_form/actions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export enum ACTION {
SET_JOB_CONFIG,
SET_JOB_IDS,
SWITCH_TO_ADVANCED_EDITOR,
SET_ESTIMATED_MODEL_MEMORY_LIMIT,
}

export type Action =
Expand Down Expand Up @@ -59,7 +60,8 @@ export type Action =
}
| { type: ACTION.SET_IS_MODAL_VISIBLE; isModalVisible: State['isModalVisible'] }
| { type: ACTION.SET_JOB_CONFIG; payload: State['jobConfig'] }
| { type: ACTION.SET_JOB_IDS; jobIds: State['jobIds'] };
| { type: ACTION.SET_JOB_IDS; jobIds: State['jobIds'] }
| { type: ACTION.SET_ESTIMATED_MODEL_MEMORY_LIMIT; value: State['estimatedModelMemoryLimit'] };

// Actions wrapping the dispatcher exposed by the custom hook
export interface ActionDispatchers {
Expand All @@ -73,4 +75,5 @@ export interface ActionDispatchers {
setJobConfig: (payload: State['jobConfig']) => void;
startAnalyticsJob: () => void;
switchToAdvancedEditor: () => void;
setEstimatedModelMemoryLimit: (value: State['estimatedModelMemoryLimit']) => void;
}
30 changes: 28 additions & 2 deletions ...rame_analytics/pages/analytics_management/hooks/use_create_analytics_form/reducer.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { merge } from 'lodash';
import { DataFrameAnalyticsConfig } from '../../../../common';

import { ACTION } from './actions';
import { reducer, validateAdvancedEditor } from './reducer';
import { reducer, validateAdvancedEditor, validateMinMML } from './reducer';
import { getInitialState, JOB_TYPES } from './state';

type SourceIndex = DataFrameAnalyticsConfig['source']['index'];
Expand Down Expand Up @@ -41,13 +41,19 @@ describe('useCreateAnalyticsForm', () => {
const initialState = getInitialState();
expect(initialState.isValid).toBe(false);

const updatedState = reducer(initialState, {
const stateWithEstimatedMml = reducer(initialState, {
type: ACTION.SET_ESTIMATED_MODEL_MEMORY_LIMIT,
value: '182222kb',
});

const updatedState = reducer(stateWithEstimatedMml, {
type: ACTION.SET_FORM_STATE,
payload: {
destinationIndex: 'the-destination-index',
jobId: 'the-analytics-job-id',
sourceIndex: 'the-source-index',
jobType: JOB_TYPES.OUTLIER_DETECTION,
modelMemoryLimit: '200mb',
},
});
expect(updatedState.isValid).toBe(true);
Expand Down Expand Up @@ -146,3 +152,23 @@ describe('useCreateAnalyticsForm', () => {
).toBe(false);
});
});

describe('validateMinMML', () => {
test('should detect a lower value', () => {
expect(validateMinMML('10mb')('100kb')).toEqual({
min: { minValue: '10mb', actualValue: '100kb' },
});
});

test('should allow a bigger value', () => {
expect(validateMinMML('10mb')('1GB')).toEqual(null);
});

test('should allow the same value', () => {
expect(validateMinMML('1024mb')('1gb')).toEqual(null);
});

test('should ignore empty parameters', () => {
expect(validateMinMML((undefined as unknown) as string)('')).toEqual(null);
});
});
Loading

0 comments on commit 4edde17

Please sign in to comment.