Re: [PR] 38 add support to label logs (flagon-distill)

["EandrewJones (via GitHub)" <gi...@apache.org>]

EandrewJones commented on code in PR #44:
URL: https://github.com/apache/flagon-distill/pull/44#discussion_r1664389606


##########
tests/test_transform.py:
##########
@@ -39,3 +45,24 @@ def test_pairwiseSeq_2():
     test_list = [1, 2, 3, 4]
     result = distill.pairwiseSeq(test_list, split=True)
     assert result == ((1, 2, 3), (2, 3, 4))
+    
+data = testing_utils.setup(os.path.join(DATA_DIR, "sample_data.json"), "integer")
+logs = data[1]
+
+def input_rule(log):
+    return "target" in log and "input" in log["target"]
+
+def test_label_features_1():
+    result = distill.label_features(logs,[FeatureDefinition(input_rule, "input_target")])
+    assert result
+
+def test_label_features_2():
+    with pytest.raises(TypeError):
+        result = distill.label_features(logs,[FeatureDefinition(input_rule, 10)])
+
+def test_label_features_2():
+    with pytest.raises(TypeError):
+        result = distill.label_features(logs,[FeatureDefinition("input_rule", "input_target")])

Review Comment:
   These tests are fine as they test expected behavior. Two things:
   
   1. These two tests are testing the `FeatureDefinition` class, not label_features function. So we should remove the calls to label_feature. Instead, all you need to do is try to instantiate the FeatureDefinition class with the bad arguments.
   2. When creating tests, best practice is to use informative names for the test functions. These should tell the use what behavior is behaving tests, e.g.:
   `def test_feature_definition_does_not_accept_non_string_label"
   `def test_feature_definition_does_not_accept_non_callable_rules`



##########
distill/core/feature_definition.py:
##########
@@ -0,0 +1,31 @@
+from typing import Any, Dict, List, Callable
+
+class FeatureDefinition:
+
+    def __init__(self, label: str, rule: Callable[[Dict[str, Any]], bool]):
+        """
+        Allows users to specify a rule or set of rules and an 
+        associated label and then use this to add labels to  
+        logs in Distill
+        param label: a string we want to add to the log if the rule is met

Review Comment:
   Please add a space between description and params.



##########
tests/test_transform.py:
##########
@@ -39,3 +45,24 @@ def test_pairwiseSeq_2():
     test_list = [1, 2, 3, 4]
     result = distill.pairwiseSeq(test_list, split=True)
     assert result == ((1, 2, 3), (2, 3, 4))
+    
+data = testing_utils.setup(os.path.join(DATA_DIR, "sample_data.json"), "integer")
+logs = data[1]
+
+def input_rule(log):
+    return "target" in log and "input" in log["target"]
+
+def test_label_features_1():
+    result = distill.label_features(logs,[FeatureDefinition(input_rule, "input_target")])
+    assert result

Review Comment:
   Few things here:
   
   First, best practice is to encapsulate any variables, data, etc. that are specific to your test inside the test function itself. 
   
   In this case, you only use the logs and the input_rule inside the test_label_features_1 function, so you should move all of that inside the function (yes, you can define functions inside other functions, you just can't then use the child function outside the scope of the parent function.).
   
   Second, is `logs` actually a list of one or more logs or is it a single log? I see you indexed on data[1] which leads me to believe it's just a single log but it should be the former.
   
   Third, see comment below about naming conventions.
   
   Finally, we want to validate that our label_features function not only ran successfully, but that the logic does what we expect. In this case, assuming the input_rule returns true, we expect the function to add the label we have set. Result should be a list of a logs. We then want to assert that all of the logs which match the rule have indeed been properly labeled, if we know _a priori_ which logs meet our criteria we can directly extract them using their index and then assert that the `"labels"` field is now present in them and that it contains the "input_target" label.
   
   



##########
examples/labels.py:
##########
@@ -0,0 +1,108 @@
+from typing import Any, Dict, List, Callable
+import pandas as pd
+
+
+class FeatureDefinition:
+    # Implement class logic
+    # TODO: Add a very specific type hint to the rule object:
+    # see: https://docs.python.org/3/library/typing.html#annotating-callable-objects
+    def __init__(self, label: str, rule: Callable[[Dict[str, Any]], bool]):
+        if not callable(rule):
+            raise TypeError("Rule not callable")
+        
+        if not isinstance(label, str):
+            raise TypeError("Label is not a string")
+        # Immediately validate the rule, so you can error
+        # out/exit early if it's invalid
+            # TODO: raise an informative error to the user
+            # see:
+            # - https://www.geeksforgeeks.org/python-exception-handling/
+            # - https://docs.python.org/3/tutorial/errors.html#raising-exceptions
+
+        self.label = label
+        self._rule = rule
+
+    # This is a wrapper method around the private rule attribute we
+    # store on self during init.
+    #
+    # Q: Why make the rule private and
+    # wrap the call to it in another method?
+    # A: This encapsulation allows us to expose a nicer set of behavior
+    # and naming conventions to both the user and ourselves as developers.
+    # In `label_features` below, you see that we can then check whether
+    # a log `matches` the definition which reads more like plain english
+    # and is an important part of writing clean, idiomatic python code.
+    # TODO: Implement this wrapper function by using the _rule attribute
+    def matches(self, log: Dict[str, Any]) -> bool:
+            return self._rule(log)
+
+
+def label_features(
+    logs: List[Dict[str, Any]], definitions: List[FeatureDefinition]
+) -> List[Dict[str, Any]]:
+    # Iterate through all the logs
+    for log in logs:
+        # Check whether the log matches the definition
+        # for each definition supplied in the defintions list
+        for definition in definitions:
+            # NOTE: This reads much like an English sentence
+            # and is self-explanatory. I don't need to read the
+            # implementation logic to get a sense of what's happening
+            if definition.matches(log):
+                # NOTE: Since we're mutating the log itself and interacting
+                # with a field that may (does) not already exists, we need
+                # to first check if it is present in our log and instantiate
+                # it if not.
+                if "labels" not in log:
+                    log.update({"labels": list()})
+                log["labels"].append(definition.label)
+    return logs

Review Comment:
   Please move this function to `distill/process.py`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@flagon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@flagon.apache.org
For additional commands, e-mail: notifications-help@flagon.apache.org