[ML] Add include categorical filter type to detector rules #27

davidkyle · 2018-03-27T15:39:57Z

Detector rules have a E_Categorical condition that applies to text fields, when used in conjunction with the action E_FilterResults the effect is to filter the results i.e. don't generate results for these field values

This PR introduces the inverse functionality i.e. generate results only for those field values in the filter list.

I renamed the enum E_Categorical to E_Match but left the JSON parsing code as is for backwards compatibility. I'm not entirely happy with the naming any suggestions are welcome or perhaps this is best left for the anticipated rewrite of rules

sophiec20 · 2018-03-27T16:20:52Z

Is this not still a E_Categorical condition, except it's type would be include or exclude.

Alternatively, if done at that level, how about E_CategoricalIncludeList and E_CategoricalExcludeList?

davidkyle · 2018-03-27T16:25:13Z

Alternatively, if done at that level, how about E_CategoricalIncludeList and E_CategoricalExcludeList?

👍 It's an improvement

tveasey

This looks pretty good, but I'm +1 on renaming the conditions as per Sophie's suggestion and making the rule specification match, since I don't think we need to preserve backwards compatibility at the moment.

tveasey · 2018-03-28T08:48:19Z

lib/api/CDetectionRulesJsonParser.cc

@@ -35,6 +35,7 @@ const std::string TARGET_FIELD_NAME("target_field_name");
 const std::string TARGET_FIELD_VALUE("target_field_value");
 const std::string TYPE("type");
 const std::string CATEGORICAL("categorical");
+const std::string NOT_MATCH("not_match");


I think we should rename both these strings to be consistent, i.e. categorical_include and categorical_exclude or something similar. There is going to be a break to backwards compatibility of rule syntax anyway and as it is I think this naming is confusing.

I've realised backwards compatibility isn't an issue here we can replace the categorical string with anything we want as the job config is created in x-pack java on the same node the job runs on. C++ and Java are upgraded in step so on a v7 node there will be v7 java writing config v7 autodetect can read. The BWC problems are in x-pack.

I wondered whether mixed version clusters in rolling upgrades would cause a problem, but I guess in these cases do we ensure only compatible versions talk to one another. In any case, I think we only have one user of rules at this point except one (who needs to update their rules configuration anyway) so any BWC is surely moot.

tveasey · 2018-03-28T08:56:17Z

lib/model/unittest/CDetectionRuleTest.cc

        condition.fieldName(attributeFieldName);
        condition.valueFilter(valueFilter);
        CDetectionRule rule;
        rule.addCondition(condition);

+        bool isCategoricalType = CRuleCondition::E_Match == conditionType;


As per Sophie's suggestion, I think renaming this, and subsequent examples, to something like isCategoricalInclude would make this clearer.

davidkyle · 2018-03-28T15:54:38Z

The rules have a condition (gt, lte, etc) and an action (filter results) I think using include/exclude confuses the condition with the action. If E_CategoricalInclude is used and the condition is true when the pattern matches then when used with filter_results it would have the effect of excluding the results. Otherwise E_CategoricalInclude could be defined to return false if the pattern matches and that doesn't make much sense either.

The best I can come up with is categorical_match and categorical_complement which at least are technically correct if a little prosaic

tveasey · 2018-03-28T16:49:53Z

Another option would be to introduce a new action filter_complement, or include_results and exclude_results, rather than a new condition. This is actually slightly more powerful than introducing a categorical only condition, since we could apply it to metric conditions as well.

davidkyle · 2018-03-28T17:19:42Z

filter_complement would be a larger code change due to the way the code is structured see https://github.com/elastic/ml-cpp/blob/master/lib/model/CAnomalyDetectorModel.cc#L564 where the action is passed as the parameter.

We have <, <=, >, >= for numerical conditions so we can effectively negate those anyway

tveasey · 2018-03-28T21:34:35Z

Yes, it is definitely less useful. There are still some cases where it will make rules involving metric conditions slightly more concise because of the implied logical connectives we use in conditions, but if it is a much larger change I'm happy to keep as is. I sort of wondered as well whether this is really more logically an action, keep vs remove, rather than a condition when you originally suggested this change, but don't have strong feelings.

davidkyle · 2018-03-29T09:49:07Z

I pushed another commit with renaming E_Categorical -> E_CategoricalMatch and using E_CategoricalComplement

tveasey

LGTM

davidkyle · 2018-04-04T16:11:03Z

I pushed another commit enabling backwards compatibility for parsing the rules with the old style type: categorical this also has the advantage of not breaking the x-pack builds after this is merged and before the x-pack change has passed intake. This can be reverted later

dimitris-athanasiou

LGTM

* Add whitelist rule condition type * Enable BWC for parsing categorical rules

davidkyle · 2018-12-19T10:57:04Z

Marking as a non-issue for release notes purposes as this code was reworked significantly in #119 and was never released in this form.

davidkyle added the v7.0.0 label Mar 27, 2018

davidkyle changed the title ~~[ML] Add whitelist condition type to detector rules~~ [ML] Add include categorical filter type to detector rules Mar 27, 2018

sophiec20 added :ml and removed :ml labels Mar 28, 2018

tveasey reviewed Mar 28, 2018

View reviewed changes

tveasey approved these changes Apr 4, 2018

View reviewed changes

davidkyle added 3 commits April 5, 2018 15:48

Add whitelist rule condition type

34c45e3

Enable BWC for parsing categorical rules

f91ca5f

Fix compilation after rebase

a5192b0

davidkyle added review >enhancement labels Apr 9, 2018

dimitris-athanasiou approved these changes Apr 9, 2018

View reviewed changes

davidkyle merged commit 576d86c into elastic:master Apr 9, 2018

davidkyle deleted the whitelist branch April 9, 2018 16:51

droberts195 pushed a commit that referenced this pull request Apr 23, 2018

[ML] Add include categorical filter type to detector rules (#27)

3678f99

* Add whitelist rule condition type * Enable BWC for parsing categorical rules

droberts195 pushed a commit that referenced this pull request Apr 23, 2018

[ML] Add include categorical filter type to detector rules (#27)

6bd1a53

* Add whitelist rule condition type * Enable BWC for parsing categorical rules

davidkyle added >non-issue and removed >enhancement labels Dec 19, 2018

davidkyle mentioned this pull request Jun 20, 2023

[NLP] Catch exceptions thrown during inference and report as errors #2542

Merged

[ML] Add include categorical filter type to detector rules #27

[ML] Add include categorical filter type to detector rules #27

Uh oh!

Conversation

davidkyle commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sophiec20 commented Mar 27, 2018

Uh oh!

davidkyle commented Mar 27, 2018

Uh oh!

tveasey left a comment

Choose a reason for hiding this comment

Uh oh!

tveasey Mar 28, 2018

Choose a reason for hiding this comment

Uh oh!

davidkyle Mar 28, 2018

Choose a reason for hiding this comment

Uh oh!

tveasey Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tveasey Mar 28, 2018

Choose a reason for hiding this comment

Uh oh!

davidkyle commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tveasey commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidkyle commented Mar 28, 2018

Uh oh!

tveasey commented Mar 28, 2018

Uh oh!

davidkyle commented Mar 29, 2018

Uh oh!

tveasey left a comment

Choose a reason for hiding this comment

Uh oh!

davidkyle commented Apr 4, 2018

Uh oh!

dimitris-athanasiou left a comment

Choose a reason for hiding this comment

Uh oh!

davidkyle commented Dec 19, 2018

Uh oh!

Uh oh!

davidkyle commented Mar 27, 2018 •

edited

Loading

tveasey Mar 28, 2018 •

edited

Loading

davidkyle commented Mar 28, 2018 •

edited

Loading

tveasey commented Mar 28, 2018 •

edited

Loading