-
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] More helpful job validation success messages. #21079
[ML] More helpful job validation success messages. #21079
Conversation
Pinging @elastic/ml-ui |
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
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
💔 Build Failed |
retest |
💔 Build Failed |
💔 Build Failed |
retest |
💚 Build Succeeded |
This provides more helpful texts for job validation success messages. Previously only a list of checks was shown to the user without any further explanation. This PR addresses this issue in the following way: - At the bottom of the modal an introductory brief text about job validation including a link to documentation is inserted. - The success messages in the list now provide a more helpful text including (where applicable) a deep link to documentation - The messages now support a richer Callout layout including a header and additional text.
This provides more helpful texts for job validation success messages. Previously only a list of checks was shown to the user without any further explanation. This PR addresses this issue in the following way: - At the bottom of the modal an introductory brief text about job validation including a link to documentation is inserted. - The success messages in the list now provide a more helpful text including (where applicable) a deep link to documentation - The messages now support a richer Callout layout including a header and additional text.
@@ -35,57 +35,70 @@ | |||
}, | |||
"categorization_filters_valid": { | |||
"status": "SUCCESS", | |||
"text": "Categorization filters" | |||
"text": "Categorization filters checks passed.", | |||
"url": "https://www.elastic.co/guide/en/x-pack/{{version}}/ml-configuring-categories.html#ml-configuring-categories" |
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.
I think this should point to this URL instead (with {{version}} as appropriate): https://www.elastic.co/guide/en/elastic-stack-overview/current/ml-configuring-categories.html
The same is true for all other "x-pack" URLs
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.
With respect to:
"Cardinality of over_field "{{fieldName}}" is below 10 and might be less suitable for population analysis."
You might consider changing this to ... "might not be suitable". Otherwise it makes me think "less suitable than what?"
@@ -35,57 +35,70 @@ | |||
}, | |||
"categorization_filters_valid": { | |||
"status": "SUCCESS", | |||
"text": "Categorization filters" | |||
"text": "Categorization filters checks passed.", | |||
"url": "https://www.elastic.co/guide/en/x-pack/{{version}}/ml-configuring-categories.html#ml-configuring-categories" | |||
}, | |||
"categorization_filters_invalid": { | |||
"status": "ERROR", | |||
"text": "The categorization filters are invalid." |
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.
If we can't provide more details about why the categorization filters are invalid, would it be helpful to at least point them here for more information about that property?: https://www.elastic.co/guide/en/elasticsearch/reference/current/ml-job-resource.html#ml-analysisconfig
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.
Does this message get displayed when both categorization_filters
and a categorization_analyzer
are configured? If so then the message could be "Categorization filters are not permitted with a categorization analyzer. Instead add a char_filter
within the categorization_analyzer
." It should still link to the docs as well, as this whole area of the config is complex.
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.
Here's the current code that does the validation: https://github.com/elastic/kibana/blob/master/x-pack/plugins/ml/common/util/job_utils.js#L264
At the moment we check that every filter is a valid regular expression and when filters are set that job.analysis_config.categorization_field_name
must also be set. We could extend this in the future, but to match the current validation, I suggest to change the text for now to:
The categorization filters configuration is invalid. Make sure filters are valid regular expressions and "categorization_field_name" is set.
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.
@droberts195 I added a note to the job validation meta issue about categorization_filters/categorization_analyzer
#18368
With respect to
... The sentence might be clearer as follows:
|
With respect to
It might be helpful to refer to the xpack.ml.max_model_memory_limit setting (https://www.elastic.co/guide/en/elasticsearch/reference/current/ml-settings.html). |
With respect to
I think a better place to link would be https://www.elastic.co/guide/en/elasticsearch/reference/current/ml-job-resource.html#ml-apilimits where the model_memory_limit property is defined. |
Created a follow up with improvements to messages after more feedback: #21191 |
Fixes #19068.
This provides more helpful texts for job validation success messages. Previously only a list of checks was shown to the user without any further explanation. This PR addresses this issue in the following way: