[ML] Add audit warning for 1000 categories found early in job#51146
Merged
droberts195 merged 3 commits intoelastic:masterfrom Jan 17, 2020
Merged
[ML] Add audit warning for 1000 categories found early in job#51146droberts195 merged 3 commits intoelastic:masterfrom
droberts195 merged 3 commits intoelastic:masterfrom
Conversation
If 1000 different category definitions are created for a job in the first 100 buckets it processes then an audit warning will now be created. (This will cause a yellow warning triangle in the ML UI's jobs list.) Such a large number of categories suggests that the field that categorization is working on is not well suited to the ML categorization functionality.
Collaborator
|
Pinging @elastic/ml-core (:ml) |
Contributor
dimitris-athanasiou
left a comment
There was a problem hiding this comment.
Looks good. Left a few minor suggestions.
| "Adjust the analysis_limits.model_memory_limit setting to ensure all data is analyzed"; | ||
| public static final String JOB_AUDIT_MEMORY_STATUS_HARD_LIMIT_PRE_7_2 = "Job memory status changed to hard_limit at {0}; adjust the " + | ||
| "analysis_limits.model_memory_limit setting to ensure all data is analyzed"; | ||
| public static final String JOB_AUDIT_EXCESSIVE_EARLY_CATEGORIES = "{0} categories observed in the first [{1}] buckets." + |
Contributor
There was a problem hiding this comment.
We have square brackets about the second number in this message but not the first. Should we make it consistent?
Author
There was a problem hiding this comment.
The reason I didn't put the first one in square brackets is that it won't vary (at least for a particular version of the product). The first number will always be 1000 unless somebody changes the code, whereas the second number can vary between different occurrences of the audit message.
| private volatile boolean failed; | ||
| private int bucketCount; // only used from the process() thread, so doesn't need to be volatile | ||
| private long priorRunsBucketCount; | ||
| private int currentRunBucketCount; // only used from the process() thread, so doesn't need to be volatile |
Contributor
There was a problem hiding this comment.
should we take this chance and make this a long as well? It seems like something that could hit overflow problems.
Author
There was a problem hiding this comment.
With a 1 second bucket span it will take 68 years to overflow, so pretty unlikely. But maybe I can avoid some casting by making it long, which will make the code cleaner. If so I'll change it.
Author
There was a problem hiding this comment.
There wasn't much casting but I changed it anyway just so both variables have the same type.
| this.bulkResultsPersister = persister.bulkPersisterBuilder(jobId, this::isAlive); | ||
| this.timingStatsReporter = new TimingStatsReporter(timingStats, bulkResultsPersister); | ||
| this.deleteInterimRequired = true; | ||
| this.priorRunsBucketCount = timingStats.getBucketCount(); |
Contributor
There was a problem hiding this comment.
I was wondering where we'd get this from but it's cool we already pass it in!
| @@ -225,6 +231,18 @@ void processResult(AutodetectResult result) { | |||
| CategoryDefinition categoryDefinition = result.getCategoryDefinition(); | |||
| if (categoryDefinition != null) { | |||
| persister.persistCategoryDefinition(categoryDefinition, this::isAlive); | |||
Contributor
There was a problem hiding this comment.
Now that all this has become more complex than a single call to the persister, I'd be tempted to extract this in a handleCategoryDefinition method.
droberts195
added a commit
that referenced
this pull request
Jan 17, 2020
If 1000 different category definitions are created for a job in the first 100 buckets it processes then an audit warning will now be created. (This will cause a yellow warning triangle in the ML UI's jobs list.) Such a large number of categories suggests that the field that categorization is working on is not well suited to the ML categorization functionality.
droberts195
added a commit
that referenced
this pull request
Jan 17, 2020
If 1000 different category definitions are created for a job in the first 100 buckets it processes then an audit warning will now be created. (This will cause a yellow warning triangle in the ML UI's jobs list.) Such a large number of categories suggests that the field that categorization is working on is not well suited to the ML categorization functionality.
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
…c#51146) If 1000 different category definitions are created for a job in the first 100 buckets it processes then an audit warning will now be created. (This will cause a yellow warning triangle in the ML UI's jobs list.) Such a large number of categories suggests that the field that categorization is working on is not well suited to the ML categorization functionality.
This was referenced Feb 3, 2020
droberts195
added a commit
to droberts195/elasticsearch
that referenced
this pull request
Feb 11, 2020
In elastic#51146 a rudimentary check for poor categorization was added to 7.6. This change replaces that warning based on a Java-side check with a new one based on the categorization_status field that the ML C++ sets. categorization_status was added in 7.7 and above by elastic#51879, so this new warning based on more advanced conditions will also be in 7.7 and above. Closes elastic#50749
This was referenced Feb 11, 2020
droberts195
added a commit
that referenced
this pull request
Feb 11, 2020
…2195) In #51146 a rudimentary check for poor categorization was added to 7.6. This change replaces that warning based on a Java-side check with a new one based on the categorization_status field that the ML C++ sets. categorization_status was added in 7.7 and above by #51879, so this new warning based on more advanced conditions will also be in 7.7 and above. Closes #50749
droberts195
added a commit
that referenced
this pull request
Feb 11, 2020
…2195) In #51146 a rudimentary check for poor categorization was added to 7.6. This change replaces that warning based on a Java-side check with a new one based on the categorization_status field that the ML C++ sets. categorization_status was added in 7.7 and above by #51879, so this new warning based on more advanced conditions will also be in 7.7 and above. Closes #50749
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If 1000 different category definitions are created for a job in
the first 100 buckets it processes then an audit warning will now
be created. (This will cause a yellow warning triangle in the
ML UI's jobs list.)
Such a large number of categories suggests that the field that
categorization is working on is not well suited to the ML
categorization functionality.
Relates #50749