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] DF Analytics: Creation wizard part 1 #67564

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented May 27, 2020

Summary

Related meta issue: #66661
This is the first PR for switching over to the new creation wizard.

To be addressed in follow ups:

  • job cloning
  • hyper parameter validation via explain API
    • disable continue if there is invalid input in the Hyper parameters form
  • remove deprecated code and state handling refactor
  • misc style tweaks

NOTE:

Cloning action and tests have been commented out for now until cloning has been updated for the new wizard

Adds source selection modal for saved searches/indices:

image

Config step:

Preview source data:

image

Shows table of included fields for excludes selection

image

Additional Options

image

Job details

image

Creation

View summary of each step and select to start job immediately

image

Checklist

Delete any items that are not applicable to this PR.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@qn895
Copy link
Member

qn895 commented May 28, 2020

For this one, and it's more of a user experience thing, I think it's more intuitive to either mark the Job ID and the Destination Index as required or at least somehow highlight that I need to input something before the continue button becomes enabled.

Screen Shot 2020-05-27 at 7 40 08 PM

Also, turning on the JSON editor after creating the job will show just this without the "return to the job list" button.
Screen Shot 2020-05-27 at 7 46 10 PM

The search index feature doesn't work with - or _

2020-05-27 at 7 51 PM
2020-05-27 at 7 57 PM

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-df-analytics-creation-wizard branch 2 times, most recently from 4be5357 to d0b23d9 Compare May 31, 2020 23:06
'Regularization parameter to prevent overfitting on the training data set. Must be a non negative value',
})}
>
<EuiFieldNumber
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the Continue button be disabled if there is invalid input in the Hyper parameters form?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - that's planned for a follow-up as we will need to hit the explain API to validate this area and that requires some refactoring.


return (
<Fragment>
<Messages messages={requestMessages} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should clear the messages if the user goes back to edit some of the settings to correct the original errors? Otheriwse it can be slighly confusing when the messages are still there when they try to create the job a second time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will address in the follow up for getting job cloning working 👌

@alvarezmelissa87 alvarezmelissa87 requested review from peteharverson, qn895 and a team June 1, 2020 13:21
@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-df-analytics-creation-wizard branch from 706f449 to 6dee420 Compare June 3, 2020 13:40
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Great job overall! Added some comments, but nothing major.

@@ -421,7 +436,7 @@ export function getEvalQueryBody({

const searchQueryClone = cloneDeep(searchQuery);

if (isResultsSearchBoolQuery(searchQueryClone)) {
if (searchQueryClone && isResultsSearchBoolQuery(searchQueryClone)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As isResultsSearchBoolQuery is a type guard can we move the check against searchQueryClone && inside that function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved in cbdf3d5495501b77667dcb273df7817594782d25

@@ -431,7 +446,7 @@ export function getEvalQueryBody({
}

query = searchQueryClone;
} else if (isQueryStringQuery(searchQueryClone)) {
} else if (searchQueryClone && isQueryStringQuery(searchQueryClone)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As isQueryStringQuery is a type guard can we move the check against searchQueryClone && inside that function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in cbdf3d5495501b77667dcb273df7817594782d25

Comment on lines +62 to +68
const advancedFirstCol: ListItems[] = [];
const advancedSecondCol: ListItems[] = [];
const advancedThirdCol: ListItems[] = [];

const hyperFirstCol: ListItems[] = [];
const hyperSecondCol: ListItems[] = [];
const hyperThirdCol: ListItems[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: For readability and the ability to write a unit test in isolation we could move all of this list items preparation (also the code below this that set's up the translate strings) to a helper function or custom hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to do this in a follow-up 👍

standardizationEnabled,
} = form;

const isDepVarJob =
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: In another file the same check is called isRegOrClassJob . We should pick one for consistency or move both usages to a helper function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in cbdf3d5495501b77667dcb273df7817594782d25

modelMemoryLimitValidationResult,
]);

const isRegOrClassJob =
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: In another file the same check is called isDepVarJob. We should pick one for consistency or move both usages to a helper function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in cbdf3d5495501b77667dcb273df7817594782d25

const containsOutlierFieldsCb = ({ name, type }: Field) =>
!OMIT_FIELDS.includes(name) && name !== EVENT_RATE_FIELD_ID && BASIC_NUMERICAL_TYPES.has(type);

const callbacks: any = {
Copy link
Contributor

Choose a reason for hiding this comment

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

any could be something like Record<ANALYSIS_CONFIG_TYPE, (f: Field) => boolean>.

[ANALYSIS_CONFIG_TYPE.OUTLIER_DETECTION]: containsOutlierFieldsCb,
};

const messages: any = {
Copy link
Contributor

Choose a reason for hiding this comment

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

any could be something like Record<ANALYSIS_CONFIG_TYPE, JSX.Element>.

jobConfig?.analysis?.classification !== undefined &&
formState.numTopClasses !== undefined
) {
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a note why the ignore is necessary?

standardization_enabled: formState.standardizationEnabled,
}
);
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a note why the ignore is necessary.

@@ -279,6 +356,11 @@ export function getCloneFormStateFromJobConfig(
resultState.dependentVariable = analysisConfig.dependent_variable;
resultState.numTopFeatureImportanceValues = analysisConfig.num_top_feature_importance_values;
resultState.trainingPercent = analysisConfig.training_percent;

if (isClassificationAnalysis(analyticsJobConfig.analysis)) {
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a note why the ignore is necessary.

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Functional test changes LGTM 🎉

@pheyos
Copy link
Member

pheyos commented Jun 3, 2020

Checking stability of the test changes in https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/505/ ...

For some reason, Jenkins has issues fetching the branches of this fork, so the flaky runner doesn't start.

key={item.id}
isSelected={isItemSelected(item.id)}
key={item[tableItemId]}
isSelected={isItemSelected(item[tableItemId])}
isSelectable={true}
hasActions={true}
data-test-subj="mlFlyoutJobSelectorTableRow"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be changed to something like mlCustomSelectionTableRow. The mlFlyoutJobSelectorTable type data-test-subj is used in three places I think in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in fdf122f9dce26a106075d73672c0510fa95dd2f8

id: 'checkbox',
isCheckbox: true,
textOnly: false,
width: '24px',
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be 32px to match the width of the euiTableRowCellCheckbox. This ensures the selection border isn't cropped.

24px:
image

32px:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to 32px in fdf122f9dce26a106075d73672c0510fa95dd2f8

@@ -157,6 +157,9 @@ export function JobSelectorTable({
filterDefaultFields={!singleSelection ? JOB_FILTER_FIELDS : undefined}
items={jobs}
onTableChange={(selectionFromTable) => onSelection({ selectionFromTable })}
radioDisabledCheck={(item) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR, but as this file is being touched by this PR, could you set the width of the first checkbox column to 32px to stop the selection border being cropped. It is used twice - once for the jobs table, and once for the groups table. It needs to match the width of the euiTableRowCellCheckbox.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to 32px in fdf122f9dce26a106075d73672c0510fa95dd2f8

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Latest edits LGTM

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-df-analytics-creation-wizard branch from fdf122f to e984987 Compare June 4, 2020 14:35
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💚 Build #51757 succeeded fdf122f9dce26a106075d73672c0510fa95dd2f8
  • 💚 Build #51558 succeeded cbdf3d5495501b77667dcb273df7817594782d25
  • 💚 Build #51429 succeeded 0406a647b262b1c0a943f03e3056141005938aff
  • 💚 Build #51400 succeeded fbe09905a15b818282298e69d66cfda9b477083a
  • 💚 Build #51386 succeeded 6dee42063a219842c7a5d068310ce07d331c4dde

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@alvarezmelissa87 alvarezmelissa87 merged commit edc4d58 into elastic:master Jun 4, 2020
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Jun 4, 2020
* create newJob route and start of wizard

* wip: create configStep component

* finish configStep form and details

* wip: create andvanced step components

* create details step component

* createStep component

* ensure advanced options are correct for each job type

* add validation to each step

* use custom table for excludes

* move customSelectionTable to shared components

* form validation for advanced fields

* wip: source index selection modal

* add source index preview

* update details

* ensure advanced parameters added to config on creation

* can create job from savedSearch. can set source query in ui

* validate source object has supported fields

* eslint updates

* update tests. comment out clone action for now

* add create button to advanced editor

* remove deprecated test helper functions

* fix translation errors

* update help text. read only once job created.

* fix functional tests

* add nextStepNav to df service for tests

* fix excludes table page jump and hyperParameter not showing in details

* fix checkbox width for custom table
@alvarezmelissa87 alvarezmelissa87 deleted the ml-df-analytics-creation-wizard branch June 4, 2020 19:17
alvarezmelissa87 added a commit that referenced this pull request Jun 4, 2020
* create newJob route and start of wizard

* wip: create configStep component

* finish configStep form and details

* wip: create andvanced step components

* create details step component

* createStep component

* ensure advanced options are correct for each job type

* add validation to each step

* use custom table for excludes

* move customSelectionTable to shared components

* form validation for advanced fields

* wip: source index selection modal

* add source index preview

* update details

* ensure advanced parameters added to config on creation

* can create job from savedSearch. can set source query in ui

* validate source object has supported fields

* eslint updates

* update tests. comment out clone action for now

* add create button to advanced editor

* remove deprecated test helper functions

* fix translation errors

* update help text. read only once job created.

* fix functional tests

* add nextStepNav to df service for tests

* fix excludes table page jump and hyperParameter not showing in details

* fix checkbox width for custom table
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants