Repository: voxel51/fiftyone
Pull request: Gracefully handle None-valued tag fields
## Change log

Resolves #3546 

By convention, all non-required FO fields should be nullable, but the implementation of `tag_samples()`, `untag_samples()`, `tag_labels()`, and `untag_labels()` use the `$addToSet` operator, which gracefully handles missing fields, but unfortunately cannot handle `null` fields. So if a user clears their tags via `clear_sample_field("tags")`, for example, then all tagging operations will start to fail.

Note that the default behavior is to set `tags = []`, so having `tags` fields that contain `None` is likely **rare**.

## Implementation choice

Why use `try-except`? I considered two alternatives:

1. Refactor methods like `tag_samples()` to use two separate batch edits: one for documents whose `tags` field contains a list, and a separate one for documents whose `tags` field is missing/null:

```py
view1 = self.exists("tags").match_tags(tags, bool=False, all=True)
view1._edit_sample_tags({"$addToSet": {"tags": {"$each": list(tags)}}})

view2 = self.exists("tags", bool=False)
view2._edit_sample_tags({"$set": {"tags": tags}})
```

The drawback here is that this *always* requires two full passes over the samples collection, which would roughly halve performance just to handle the null case, which I assume is **rare**.

2. Encode the conditional operation in the update operation. Unfortunately, `$addToSet` is not available in aggregation pipelines, so the only way to achieve this would be something like this:

```py
# current
update = {"$addToSet": {"tags": {"$each": tags}}}

# new aggregation pipeline-style
pipeline = [
    {
        "$cond": {
            "if": {"$gt": ["$tags", None]},
            "then": {"$setUnion": ["$tags", tags]},
            "else": tags,
        },
    }
]
```

Unfortunately, `$setUnion` is not a 1-1 replacement for `$addToSet`: it will silently remove existing duplicates and makes no guarantee that the ordering of the existing tags will stay the same. So, really a non-starter.


<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit


- **Bug Fixes**
	- Improved error handling in tagging functionalities to address issues with null values in tags, ensuring more robust data operations.

- **Documentation**
	- Updated method descriptions in the dataset module for clarity and accuracy.

- **Tests**
	- Added tests to verify the robustness of the tagging functionalities when encountering null values.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Problem statement:
[BUG] Clearing the tags field and then tagging samples raises error
To reproduce:
```python
import fiftyone.zoo as foz

dataset = foz.load_zoo_dataset("quickstart").clone()
dataset.clear_sample_field("tags")
dataset.tag_samples("test")

# ValueError: Cannot apply $addToSet to non-array field. Field named 'tags' has non-array type null
```

This is happening because `clear_sample_field` sets the values of the field to `None` for each sample, but `tag_samples()` then tries to add a value to what it expects is a list.

We should probably add a check to `tag_samples()` to verify that the field isn't `None` before adding samples. Alternatively, we could have `clear_sample_field("tags")` check if you are clearing the tags field and set it to `[]` instead of `None`, but that field could still be set to `None` in other ways, so it seems the first approach would be preferred.

Hints from issue discussion:

Commit message: handle None-valued tag fields

Patch to review:
diff --git a/fiftyone/core/collections.py b/fiftyone/core/collections.py
index 43098224227..ab15faf4f27 100644
--- a/fiftyone/core/collections.py
+++ b/fiftyone/core/collections.py
@@ -1611,7 +1611,27 @@ def tag_samples(self, tags):
 
         # We only need to process samples that are missing a tag of interest
         view = self.match_tags(tags, bool=False, all=True)
-        view._edit_sample_tags(update)
+
+        try:
+            view._edit_sample_tags(update)
+        except ValueError as e:
+            #
+            # $addToSet cannot handle null-valued fields, so if we get an error
+            # about null-valued fields, replace them with [] and try again.
+            # Note that its okay to run $addToSet multiple times as the tag
+            # won't be added multiple times.
+            #
+            # For future reference, the error message looks roughly like this:
+            #   ValueError: Cannot apply $addToSet to non-array field. Field
+            #               named 'tags' has non-array type null
+            #
+            if "null" in str(e):
+                none_tags = self.exists("tags", bool=False)
+                none_tags.set_field("tags", []).save()
+
+                view._edit_sample_tags(update)
+            else:
+                raise e
 
     def untag_samples(self, tags):
         """Removes the tag(s) from all samples in this collection, if
@@ -1680,9 +1700,31 @@ def _tag_labels(self, tags, label_field, ids=None, label_ids=None):
             tags = list(tags)
             update_fcn = lambda path: {"$addToSet": {path: {"$each": tags}}}
 
-        return self._edit_label_tags(
-            update_fcn, label_field, ids=ids, label_ids=label_ids
-        )
+        try:
+            return self._edit_label_tags(
+                update_fcn, label_field, ids=ids, label_ids=label_ids
+            )
+        except ValueError as e:
+            #
+            # $addToSet cannot handle null-valued fields, so if we get an error
+            # about null-valued fields, replace them with [] and try again.
+            # Note that its okay to run $addToSet multiple times as the tag
+            # won't be added multiple times.
+            #
+            # For future reference, the error message looks roughly like this:
+            #   ValueError: Cannot apply $addToSet to non-array field. Field
+            #               named 'tags' has non-array type null
+            #
+            if "null" in str(e):
+                _, tags_path = self._get_label_field_path(label_field, "tags")
+                none_tags = self.filter_labels(label_field, F("tags") == None)
+                none_tags.set_field(tags_path, []).save()
+
+                return self._edit_label_tags(
+                    update_fcn, label_field, ids=ids, label_ids=label_ids
+                )
+            else:
+                raise e
 
     def untag_labels(self, tags, label_fields=None):
         """Removes the tag from all labels in the specified label field(s) of
diff --git a/fiftyone/core/dataset.py b/fiftyone/core/dataset.py
index 67029587cc4..091c5710274 100644
--- a/fiftyone/core/dataset.py
+++ b/fiftyone/core/dataset.py
@@ -1778,7 +1778,7 @@ def clear_sample_field(self, field_name):
         The field will remain in the dataset's schema, and all samples will
         have the value ``None`` for the field.
 
-        You can use dot notation (``embedded.field.name``) to clone embedded
+        You can use dot notation (``embedded.field.name``) to clear embedded
         frame fields.
 
         Args:
@@ -1792,7 +1792,7 @@ def clear_sample_fields(self, field_names):
         The field will remain in the dataset's schema, and all samples will
         have the value ``None`` for the field.
 
-        You can use dot notation (``embedded.field.name``) to clone embedded
+        You can use dot notation (``embedded.field.name``) to clear embedded
         frame fields.
 
         Args:
@@ -1807,7 +1807,7 @@ def clear_frame_field(self, field_name):
         The field will remain in the dataset's frame schema, and all frames
         will have the value ``None`` for the field.
 
-        You can use dot notation (``embedded.field.name``) to clone embedded
+        You can use dot notation (``embedded.field.name``) to clear embedded
         frame fields.
 
         Only applicable to datasets that contain videos.
@@ -1824,7 +1824,7 @@ def clear_frame_fields(self, field_names):
         The fields will remain in the dataset's frame schema, and all frames
         will have the value ``None`` for the field.
 
-        You can use dot notation (``embedded.field.name``) to clone embedded
+        You can use dot notation (``embedded.field.name``) to clear embedded
         frame fields.
 
         Only applicable to datasets that contain videos.
diff --git a/tests/unittests/view_tests.py b/tests/unittests/view_tests.py
index ffb6e32da1c..491e5a742fd 100644
--- a/tests/unittests/view_tests.py
+++ b/tests/unittests/view_tests.py
@@ -3314,6 +3314,31 @@ def test_tag_samples(self):
         tags = self.dataset.count_values("tags")
         self.assertDictEqual(tags, {})
 
+    def test_tag_samples_none(self):
+        view = self.dataset[:2]
+
+        view.clear_sample_field("tags")
+
+        for tags in view.values("tags"):
+            self.assertIsNone(tags)
+
+        view.untag_samples("test")
+        view.untag_samples(["test1", "test2"])
+
+        counts = view.count_sample_tags()
+        self.assertDictEqual(counts, {})
+
+        view.tag_samples("test")
+        view.tag_samples(["test1", "test2"])
+
+        counts = view.count_sample_tags()
+        self.assertDictEqual(counts, {"test": 2, "test1": 2, "test2": 2})
+
+        view.set_field("tags", []).save()
+
+        for tags in view.values("tags"):
+            self.assertListEqual(tags, [])
+
     def test_tag_labels(self):
         self._setUp_classification()
         self._setUp_detections()
@@ -3344,6 +3369,58 @@ def test_tag_labels(self):
         tags = self.dataset.count_label_tags("test_dets")
         self.assertDictEqual(tags, {})
 
+    def test_tag_labels_none(self):
+        self._setUp_classification()
+        self._setUp_detections()
+
+        # Test classifications
+        view = self.dataset.filter_labels("test_clf", F("confidence") > 0.95)
+        view.clear_sample_field("test_clf.tags")
+
+        for tags in view.values("test_clf.tags"):
+            self.assertIsNone(tags)
+
+        view.untag_labels("test", label_fields="test_clf")
+        view.untag_labels(["test1", "test2"], label_fields="test_clf")
+
+        counts = view.count_label_tags()
+        self.assertDictEqual(counts, {})
+
+        view.tag_labels("test", label_fields="test_clf")
+        view.tag_labels(["test1", "test2"], label_fields="test_clf")
+
+        counts = view.count_label_tags()
+        self.assertDictEqual(counts, {"test": 1, "test1": 1, "test2": 1})
+
+        view.set_field("test_clf.tags", []).save()
+
+        for tags in view.values("test_clf.tags"):
+            self.assertListEqual(tags, [])
+
+        # Test detections
+        view = self.dataset.filter_labels("test_dets", F("confidence") > 0.7)
+        view.clear_sample_field("test_dets.detections.tags")
+
+        for tags in view.values("test_dets.detections.tags", unwind=True):
+            self.assertIsNone(tags)
+
+        view.untag_labels("test", label_fields="test_dets")
+        view.untag_labels(["test1", "test2"], label_fields="test_dets")
+
+        counts = view.count_label_tags()
+        self.assertDictEqual(counts, {})
+
+        view.tag_labels("test", label_fields="test_dets")
+        view.tag_labels(["test1", "test2"], label_fields="test_dets")
+
+        counts = view.count_label_tags()
+        self.assertDictEqual(counts, {"test": 3, "test1": 3, "test2": 3})
+
+        view.set_field("test_dets.detections.tags", []).save()
+
+        for tags in view.values("test_dets.detections.tags", unwind=True):
+            self.assertListEqual(tags, [])
+
     def test_match(self):
         self.sample1["value"] = "value"
         self.sample1.save()


Code review comments:
diff_hunk: @@ -1778,7 +1778,7 @@ def clear_sample_field(self, field_name):
         The field will remain in the dataset's schema, and all samples will
         have the value ``None`` for the field.
 
-        You can use dot notation (``embedded.field.name``) to clone embedded
+        You can use dot notation (``embedded.field.name``) to clear embedded
         frame fields.
path: fiftyone/core/dataset.py
text: @user1:
Good eye but now I think the word "frame" is another copy pasta accident.
```suggestion
        You can use dot notation (``embedded.field.name``) to clear embedded
        sample fields.
```

diff_hunk: @@ -1792,7 +1792,7 @@ def clear_sample_fields(self, field_names):
         The field will remain in the dataset's schema, and all samples will
         have the value ``None`` for the field.
 
-        You can use dot notation (``embedded.field.name``) to clone embedded
+        You can use dot notation (``embedded.field.name``) to clear embedded
         frame fields.
path: fiftyone/core/dataset.py
text: @user1:
```suggestion
        You can use dot notation (``embedded.field.name``) to clear embedded
        sample fields.
```