-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Add support for per-partition categorization jobs #74592
Changes from 16 commits
5cb13f5
6a3e91f
283db47
9b513cf
10746ca
ae61c2a
ad453e3
cbb857c
5aaf226
32a5ded
1591562
a35ec15
abe77e7
f0a3cf4
5ddeaab
98bb431
9ca076b
ddb2fd8
bb698b6
1f3c5ec
e796419
02e17b3
6620328
646ef99
a94ecaf
621d3c5
0a813e1
3686b73
c5a56ca
3801a38
dd2f28c
96e6919
976b46a
043f4f2
b48cda6
5298fb3
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 | ||||
---|---|---|---|---|---|---|
|
@@ -57,6 +57,7 @@ export class JobCreator { | |||||
private _stopAllRefreshPolls: { | ||||||
stop: boolean; | ||||||
} = { stop: false }; | ||||||
private _partitionField: string | null; | ||||||
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: use
Suggested change
and there is no need to defined it in the constructor. 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. After discussing with James, we decided to remove this _partitionField altogether here 0a813e1 |
||||||
|
||||||
protected _wizardInitialized$ = new BehaviorSubject<boolean>(false); | ||||||
public wizardInitialized$ = this._wizardInitialized$.asObservable(); | ||||||
|
@@ -81,6 +82,7 @@ export class JobCreator { | |||||
} | ||||||
|
||||||
this._datafeed_config.query = query; | ||||||
this._partitionField = null; | ||||||
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. keeping a separate copy of the partition field name will lead to this becoming out of sync with the real partition field name in the job config, if the user edits the job's JSON. this will make the I don't see how we can deal with multiple categorisation detectors with differing partition fields in the UI. Even though it is possible to configure in the JSON. 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. As from our conversation, I have removed |
||||||
} | ||||||
|
||||||
public get type(): JOB_TYPE { | ||||||
|
@@ -622,6 +624,56 @@ export class JobCreator { | |||||
return JSON.stringify(this._datafeed_config, null, 2); | ||||||
} | ||||||
|
||||||
private _initPerPartitionCategorization() { | ||||||
if (this._job_config.analysis_config.per_partition_categorization === undefined) { | ||||||
this._job_config.analysis_config.per_partition_categorization = {}; | ||||||
} | ||||||
if (this._job_config.analysis_config.per_partition_categorization?.enabled === undefined) { | ||||||
this._job_config.analysis_config.per_partition_categorization!.enabled = false; | ||||||
} | ||||||
if (this._job_config.analysis_config.per_partition_categorization?.stop_on_warn === undefined) { | ||||||
this._job_config.analysis_config.per_partition_categorization!.stop_on_warn = false; | ||||||
} | ||||||
} | ||||||
|
||||||
public get perPartitionCategorization() { | ||||||
return this._job_config.analysis_config.per_partition_categorization?.enabled === true; | ||||||
} | ||||||
|
||||||
public set perPartitionCategorization(enabled: boolean) { | ||||||
this._initPerPartitionCategorization(); | ||||||
this._job_config.analysis_config.per_partition_categorization!.enabled = enabled; | ||||||
} | ||||||
|
||||||
public get partitionStopOnWarn() { | ||||||
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. should this name be 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. Updated here a94ecaf |
||||||
return this._job_config.analysis_config.per_partition_categorization?.stop_on_warn === true; | ||||||
} | ||||||
|
||||||
public set partitionStopOnWarn(enabled: boolean) { | ||||||
this._initPerPartitionCategorization(); | ||||||
this._job_config.analysis_config.per_partition_categorization!.stop_on_warn = enabled; | ||||||
} | ||||||
|
||||||
public get categorizationPerPartitionField() { | ||||||
return this._partitionField; | ||||||
} | ||||||
|
||||||
public set categorizationPerPartitionField(fieldName: string | null) { | ||||||
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 should only be changing detectors which are using |
||||||
if (fieldName === null) { | ||||||
this._detectors.forEach((detector) => { | ||||||
delete detector.partition_field_name; | ||||||
}); | ||||||
this._partitionField = null; | ||||||
} else { | ||||||
if (this._partitionField !== fieldName) { | ||||||
this._partitionField = fieldName; | ||||||
this._detectors.forEach((detector) => { | ||||||
detector.partition_field_name = fieldName; | ||||||
}); | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
protected _overrideConfigs(job: Job, datafeed: Datafeed) { | ||||||
this._job_config = job; | ||||||
this._datafeed_config = datafeed; | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,13 +4,19 @@ | |
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import React, { Fragment, FC } from 'react'; | ||
import React, { Fragment, FC, useContext } from 'react'; | ||
import { EuiFlexGroup, EuiFlexItem } from '@elastic/eui'; | ||
|
||
import { SummaryCountField } from '../summary_count_field'; | ||
import { CategorizationField } from '../categorization_field'; | ||
import { CategorizationPerPartitionField } from '../categorization_partition_field'; | ||
import { JobCreatorContext } from '../../../job_creator_context'; | ||
import { isAdvancedJobCreator } from '../../../../../common/job_creator'; | ||
|
||
export const ExtraSettings: FC = () => { | ||
const { jobCreator } = useContext(JobCreatorContext); | ||
const showCategorizationPerPartitionField = | ||
isAdvancedJobCreator(jobCreator) && jobCreator.categorizationFieldName !== null; | ||
return ( | ||
<Fragment> | ||
<EuiFlexGroup gutterSize="xl"> | ||
|
@@ -21,6 +27,13 @@ export const ExtraSettings: FC = () => { | |
<SummaryCountField /> | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
{showCategorizationPerPartitionField && ( | ||
<EuiFlexGroup gutterSize="xl"> | ||
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. do you need a flex group just for one item? 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. Removed here a94ecaf |
||
<EuiFlexItem> | ||
<CategorizationPerPartitionField /> | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
)} | ||
</Fragment> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
/* | ||
* 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, { FC, useContext, useEffect, useState } from 'react'; | ||
import { EuiFormRow } from '@elastic/eui'; | ||
import { i18n } from '@kbn/i18n'; | ||
import { JobCreatorContext } from '../../../job_creator_context'; | ||
import { | ||
AdvancedJobCreator, | ||
CategorizationJobCreator, | ||
isCategorizationJobCreator, | ||
} from '../../../../../common/job_creator'; | ||
import { newJobCapsService } from '../../../../../../../services/new_job_capabilities_service'; | ||
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. Please avoid using static imports for services. Put them in a context instead. 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 agree this would be better in a context, however 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 save this issue for a follow up PR like James mentioned |
||
import { Description } from './description'; | ||
import { CategorizationPerPartitionSwitch } from './categorization_per_partition_switch'; | ||
import { CategorizationPerPartitionFieldSelect } from './categorization_per_partition_input'; | ||
import { CategorizationPerPartitionStopOnWarnSwitch } from './categorization_stop_on_warn_switch'; | ||
|
||
export const CategorizationPerPartitionField: FC = () => { | ||
const { jobCreator: jc, jobCreatorUpdate, jobCreatorUpdated } = useContext(JobCreatorContext); | ||
const jobCreator = jc as AdvancedJobCreator | CategorizationJobCreator; | ||
const [enablePerPartitionCategorization, setEnablePerPartitionCategorization] = useState(false); | ||
const [categorizationPartitionFieldName, setCategorizationPartitionFieldName] = useState< | ||
string | null | ||
>(jobCreator.categorizationPerPartitionField); | ||
|
||
const { catFields } = newJobCapsService; | ||
|
||
const filteredCategories = catFields.filter((c) => c.id !== jobCreator.categorizationFieldName); | ||
|
||
useEffect(() => { | ||
jobCreator.categorizationPerPartitionField = categorizationPartitionFieldName; | ||
jobCreatorUpdate(); | ||
}, [categorizationPartitionFieldName]); | ||
|
||
useEffect(() => { | ||
// set the first item in category as partition field by default | ||
// because API requires partition_field to be defined in each detector with mlcategory | ||
// if per-partition categorization is enabled | ||
if ( | ||
jobCreator.perPartitionCategorization && | ||
jobCreator.categorizationPerPartitionField === null | ||
) { | ||
jobCreator.categorizationPerPartitionField = filteredCategories[0].id; | ||
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. just to be safe, it would be worth adding a check for 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. Updated in a94ecaf |
||
} | ||
setCategorizationPartitionFieldName(jobCreator.categorizationPerPartitionField); | ||
setEnablePerPartitionCategorization(jobCreator.perPartitionCategorization); | ||
}, [jobCreatorUpdated]); | ||
|
||
const isCategorizationJob = isCategorizationJobCreator(jobCreator); | ||
return ( | ||
<Description isOptional={isCategorizationJob === false}> | ||
<EuiFormRow | ||
label={i18n.translate( | ||
'xpack.ml.newJob.wizard.extraStep.categorizationJob.perPartitionCategorizationLabel', | ||
{ | ||
defaultMessage: 'Enable per-partition categorization', | ||
} | ||
)} | ||
> | ||
<CategorizationPerPartitionSwitch /> | ||
</EuiFormRow> | ||
|
||
{enablePerPartitionCategorization && ( | ||
<> | ||
<EuiFormRow | ||
label={i18n.translate( | ||
'xpack.ml.newJob.wizard.extraStep.categorizationJob.stopOnWarnLabel', | ||
{ | ||
defaultMessage: 'Stop on warn', | ||
} | ||
)} | ||
> | ||
<CategorizationPerPartitionStopOnWarnSwitch /> | ||
</EuiFormRow> | ||
<EuiFormRow | ||
label={i18n.translate( | ||
'xpack.ml.newJob.wizard.extraStep.categorizationJob.partitionFieldLabel', | ||
{ | ||
defaultMessage: 'Partition field', | ||
} | ||
)} | ||
> | ||
<CategorizationPerPartitionFieldSelect | ||
fields={filteredCategories} | ||
changeHandler={setCategorizationPartitionFieldName} | ||
selectedField={categorizationPartitionFieldName || ''} | ||
/> | ||
</EuiFormRow> | ||
</> | ||
)} | ||
</Description> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/* | ||
* 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, { FC, useContext } from 'react'; | ||
import { EuiComboBox, EuiComboBoxOptionOption } from '@elastic/eui'; | ||
|
||
import { JobCreatorContext } from '../../../job_creator_context'; | ||
import { Field } from '../../../../../../../../../common/types/fields'; | ||
import { createFieldOptions } from '../../../../../common/job_creator/util/general'; | ||
|
||
interface Props { | ||
fields: Field[]; | ||
changeHandler(i: string | null): void; | ||
selectedField: string | null; | ||
} | ||
|
||
export const CategorizationPerPartitionFieldSelect: FC<Props> = ({ | ||
fields, | ||
changeHandler, | ||
selectedField, | ||
}) => { | ||
const { jobCreator } = useContext(JobCreatorContext); | ||
const options: EuiComboBoxOptionOption[] = [ | ||
...createFieldOptions(fields, jobCreator.additionalFields), | ||
]; | ||
|
||
const selection: EuiComboBoxOptionOption[] = []; | ||
if (selectedField !== null) { | ||
selection.push({ label: selectedField }); | ||
} | ||
|
||
function onChange(selectedOptions: EuiComboBoxOptionOption[]) { | ||
const option = selectedOptions[0]; | ||
if (typeof option !== 'undefined') { | ||
changeHandler(option.label); | ||
} else { | ||
changeHandler(null); | ||
} | ||
} | ||
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 all gets invoked on each render. Please use hooks accordingly ( 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. Updated here a94ecaf |
||
|
||
return ( | ||
<EuiComboBox | ||
singleSelection={{ asPlainText: true }} | ||
options={options} | ||
selectedOptions={selection} | ||
onChange={onChange} | ||
isClearable={true} | ||
data-test-subj="mlCategorizationPerPartitionFieldNameSelect" | ||
pheyos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/* | ||
* 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, { FC, useContext, useEffect, useState } from 'react'; | ||
import { i18n } from '@kbn/i18n'; | ||
import { EuiSwitch } from '@elastic/eui'; | ||
import { JobCreatorContext } from '../../../job_creator_context'; | ||
import { AdvancedJobCreator, CategorizationJobCreator } from '../../../../../common/job_creator'; | ||
|
||
export const CategorizationPerPartitionSwitch: FC = () => { | ||
const { jobCreator: jc, jobCreatorUpdate } = useContext(JobCreatorContext); | ||
const jobCreator = jc as AdvancedJobCreator | CategorizationJobCreator; | ||
const [enablePerPartitionCategorization, setEnablePerPartitionCategorization] = useState( | ||
jobCreator.perPartitionCategorization | ||
); | ||
|
||
const toggleEnablePerPartitionCategorization = () => | ||
setEnablePerPartitionCategorization(!enablePerPartitionCategorization); | ||
|
||
useEffect(() => { | ||
// also turn off stop on warn if per_partition_categorization is turned off | ||
if (enablePerPartitionCategorization === false) { | ||
jobCreator.partitionStopOnWarn = false; | ||
} | ||
|
||
jobCreator.perPartitionCategorization = enablePerPartitionCategorization; | ||
jobCreatorUpdate(); | ||
}, [enablePerPartitionCategorization]); | ||
|
||
return ( | ||
<EuiSwitch | ||
name="switch" | ||
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.
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. Updated here a94ecaf |
||
disabled={false} | ||
checked={enablePerPartitionCategorization} | ||
onChange={toggleEnablePerPartitionCategorization} | ||
data-test-subj="mlJobWizardSwitchCategorizationPerPartitionField" | ||
pheyos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
label={i18n.translate('xpack.ml.newJob.wizard.perPartitionCategorizationSwitchLabel', { | ||
defaultMessage: 'Enable per-partition categorization', | ||
})} | ||
/> | ||
); | ||
}; |
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.
It's recommended to use
{ FormattedMessage } from '@kbn/i18n/react';
inside of components instead ofi18n
.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.
Updated all the
i18n.translate
to using FormattedMessage here 3686b73