-
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 checkbox to enable model plot in Advanced job wizard #25468
[ML] Add checkbox to enable model plot in Advanced job wizard #25468
Conversation
Pinging @elastic/ml-ui |
$scope.ui.cardinalityValidator.message = `Creating model plots is resource intensive and not recommended | ||
where the cardinality of the selected fields is greater than 100. Estimated cardinality | ||
for this job is ${validationResult.highCardinality}. | ||
If you enable model plot with this configuration we recommend you use a dedicated results index.`; |
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.
Probably worth adding where that dedicated results index is configured. Something like If you enable model plot with this configuration we recommend you select a dedicated results index on the Job Details tab.
?
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.
Do you mean as a warning callout as well?
$scope.ui.cardinalityValidator.status = STATUS.WARNING; | ||
} | ||
}) | ||
.catch((error) => { console.log('Cardinality check error:', error); }); |
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.
Is it worth displaying some sort of warning in the same callout for the error case. Something like An error occurred validating the configuration for running the job with model plot enabled. Creating model plots can be resource intensive and not recommended where the cardinality of the selected fields is high. You may want to select a dedicated results index on the Job Details tab.
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.
Good one, @peteharverson! 😄 I'll go back and add this to the other wizards as well in a subsequent PR
return response; | ||
} | ||
|
||
export function getMinimalValidJob() { |
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.
Can you add a comment to say when this function would be used
💚 Build Succeeded |
💚 Build Succeeded |
💚 Build Succeeded |
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.
LGTM!
} from '@elastic/eui'; | ||
|
||
|
||
export const EnableModelPlotCallout = ({ message }) => ( |
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.
We should probably have a unit test for this component, even though it is fairly simple.
💔 Build Failed |
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.
Latest changes LGTM
retest |
💔 Build Failed |
4c16507
to
aefa3e3
Compare
💚 Build Succeeded |
…c#25468) * Move cardinality success check to utils * enableModelPlot checkbox base added * Run cardinality check on add/update fields * Handle changes made via json * only run cardinality check if model plot enabled * Handle model plot enabled via EditJSON tab * show message on cardinality check error * multi-metric + pop: show message on cardinality check error * add test for callout component * Fix flexitem overflow in IE11
#25831) * Move cardinality success check to utils * enableModelPlot checkbox base added * Run cardinality check on add/update fields * Handle changes made via json * only run cardinality check if model plot enabled * Handle model plot enabled via EditJSON tab * show message on cardinality check error * multi-metric + pop: show message on cardinality check error * add test for callout component * Fix flexitem overflow in IE11
Summary
Related issue: #18164
Follow up to: #24914
Adds an
Enable model plot
checkbox in theAnalysis Configuration
tab in theAdvanced
job creation wizard. Creating a job with this checked sets"model_plot_config":{"enabled":true}.
for that job.Flow:
Enable model plot
is checked cardinality check is run. Callout is shown when cardinality is greater than the threshold (100).-- Changes made in
Detectors
area or fromAdd detector
modal.currentConfigs
and if changes occur via theEdit JSON
tab, we update the ui and run cardinality again ifenableModelPlot
is set to true and detectors have been updated.Use dedicated index
is NOT automatically checked when cardinality returns a warning. This option is in a separate tab and, therefore, may not be seen by the user so we don't change it.MultiMetric
+Population
wizards by still showing callout with warning message.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorialsFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately