[BUG] Annotation schema attribute definitions not filtering when defined on classes

[geoffrp]

System information

• OS Platform and Distribution (e.g., Linux Ubuntu 16.04): Ubuntu 20.04.3 LTS
• FiftyOne installed from (pip or source): pip
• FiftyOne version (run fiftyone --version): v0.15.1
• Python version: Python 3.9.10

Commands to reproduce

When submitting a schema, attributes are not filtered if attributes are only attached to classes. For example, if a sample of object1 had fields of condition1, condition2, and other_field, the following schema would allow other_field into CVAT annotations:

        "type": "detections",
        "classes": [
            "object0",
            {
                "classes": ["object1"],
                "attributes": {
                    "condition1": {
                        "type": "checkbox",
                        "default": False,
                    },
                    "condition2": {
                        "type": "checkbox",
                        "default": False,
                    },
                },
            }
     ]
}

This schema would block the other_field field from going to annotations:

        "type": "detections",
        "classes": [
            "object0",
            {
                "classes": ["object1"],
                "attributes": {
                    "condition1": {
                        "type": "checkbox",
                        "default": False,
                    },
                    "condition2": {
                        "type": "checkbox",
                        "default": False,
                    },
                },
            }
     ],
     "attributes": {"test": {"type": "checkbox", "default": "False"}}
}

Describe the problem

Schemas need not define global attributes, they can also be defined class by class.

Code to reproduce issue

import fiftyone as fo
import fiftyone.zoo as foz
import tempfile
import numpy as np
from PIL import Image

USERNAME = ...
PASSWORD = ...

# Attributes are only defined by class, `bad` will go through despite not being listed
schema = {
    "test_field": {
        "type": "classification",
        "classes": [
            {
                "classes": ["test"],
                "attributes": {
                    "good": {
                        "type": "checkbox",
                        "default": True,
                    }
                },
            }
        ],
    }
}

# Include an attribute dict to force filtering in _get_attributes
hacky_schema =  {
    "test_field": {
        "type": "classification",
        "classes": [
            {
                "classes": ["test"],
                "attributes": {
                    "good": {
                        "type": "checkbox",
                        "default": True,
                    }
                },
            }
        ],
        'attributes': {
            "another_one": {
                        "type": "checkbox",
                        "default": True,
                    }
        }
    }
}

with tempfile.TemporaryDirectory() as tmpdir:
    # Generate images
    for i in range(3):
        imarray = np.random.rand(100, 100, 3) * 255
        im = Image.fromarray(imarray.astype("uint8")).convert("RGBA")
        im.save(f"{tmpdir}/temp{i}.png")
    # Create a dataset
    dataset = fo.Dataset.from_images_dir(tmpdir)

    # Assign some bogus classification tags with attributes
    for sample in dataset:
        sample["test_field"] = fo.Classification(label="test")
        # Assign a desirable and undesirable attribute to the classification
        sample["test_field"]["good"] = True
        sample["test_field"]["bad"] = ":("
        sample.save()
    dataset.save()

    # Annotate and go to CVAT to see result

    dataset.annotate(
        "test_annotation",
        # Swap schemas here
        label_schema=hacky_schema,
        backend="cvat",
        url="https://cvat.org/",
        username=USERNAME,
        password=PASSWORD,
    )

What areas of FiftyOne does this bug affect?

• App: FiftyOne application issue
• [x ] Core: Core fiftyone Python library issue
• Server: Fiftyone server issue

Willingness to contribute

The FiftyOne Community encourages bug fix contributions. Would you or another
member of your organization be willing to contribute a fix for this bug to the
FiftyOne codebase?

• Yes. I can contribute a fix for this bug independently.
• [x ] Yes. I would be willing to contribute a fix for this bug with guidance
  from the FiftyOne community.
• No. I cannot contribute a bug fix at this time.

[geoffrp]

It looks like the problem lies at:

fiftyone/utils/annotations.py:801

    if "attributes" in label_info:
        attributes = label_info["attributes"]

If attributes isn't included, we iterate over all fields, adding those that aren't None. Maybe classes must be populated to give information on how to parse class-specific attributes?

[ehofesmann]

Thanks for bringing up this issue and the reproduction code @geoffrp!
To recap there are three different ways that you can define the attributes being annotated in order or priority:

1. For a single class in one field in your schema, you can provide explicit attributes that only apply to that one class.

schema = {
    "test_field": {
        "type": "classification",
        "classes": [
            {
                "classes": ["test"],
                "attributes": {
                    "good": {
                        "type": "checkbox",
                        "default": True,
                    }
                },
            }
        ],
    }
}

2. For all classes of a specific field in your schema, you can provide the attributes directly under the label field to apply to all classes in that field, like your hacky schema.

hacky_schema =  {
    "test_field": {
        "type": "classification",
        "classes": [
            {
                "classes": ["test"],
                "attributes": {
                    "good": {
                        "type": "checkbox",
                        "default": True,
                    }
                },
            }
        ],
        'attributes': {
            "another_one": {
                        "type": "checkbox",
                        "default": True,
                    }
        }
    }
}

3. For all label fields, you can provide a value to the attributes keyword argument when calling dataset.annotate(). For existing fields, this can either be a boolean to load all or no attributes, or a dictionary/list defining the specific attributes to load. This will then be applied to all label fields annotated in that run.

results = dataset.annotate(anno_key, label_schema=label_schema, attributes=True)

So the easiest way to currently do what you want is to define the class-specific attributes in the way that you have, and then add the attributes=False argument when calling dataset.annotate() to prevent all other attributes from being loaded. This is necessary since by default, attributes=True so all existing attributes will always be loaded no matter what you define within the label schema.

There are a few options going forward:

1. Set attributes=False by default so that no attributes are loaded unless specified. I vote against this since a common workflow is to just provide a label field directly to dataset.annotate(), and you would expect everything to be loaded in that case.

2. Update the code so that attributes=True still, but if you provide class- or field-level attributes, then it will internally set attributes=False. There is also a problem with this approach since a supported workflow should be to allow defining NEW attributes class- or field-wise and then still letting attributes=True to automatically load the remaining existing attributes.

schema = {
    "test_field": {
        "type": "classification",
        "classes": [
            {
                "classes": ["test"],
                "attributes": {
                    "new_attr": {
                        "type": "checkbox",
                        "default": True,
                    }
                },
            }
        ],
    }
}

results = dataset.annotate(anno_key, label_schema=schema, attributes=True)
# Annotate all existing attrs and `new_attr`

3. Default attributes=None which has the properties of 2) where it will load all attributes unless any specific class- or field-level attributes are provided, but still allows the option to explicitly set attributes to True/False. The main downside of this is that it is more complex and likely harder to understand what is happening for new users, but I think this would probably be the best approach.

I'd be interested in hearing your thoughts @geoffrp and @brimoor

[brimoor]

Since it is possible to get the behavior that @geoffrp wants by simply providing an additional parameter, I see an argument for option 4:

4. Don't change any code, just update/clarify the docs to explain the implications of top-level attributes=True being the default behavior.

Side comment: +1 on @ehofesmann's comment that attributes should be included by default in simple syntaxes like:

dataset.annotate(anno_key, label_field="existing_field_with_attributes")

This is consistent with other I/O in the codebase, such as how there's an extra_attrs parameter that defaults to True on many exporters to automatically include everything known about an object unless otherwise requested.

[geoffrp]

@brimoor @ehofesmann I think that option 4 could work here, and re-reading the documentation, this line slipped by me:

label_schema (None): a dictionary defining the label schema to use. If this argument is provided, it takes precedence over label_field and label_type

The presentation in the documentation pushes label schema as a catch-all method of specifying the whole annotation task, but there are other parameters that gate certain functions. Maybe for some users, it is useful to have all attributes show up to contextualize the annotation, so having the open attributes=True could be default behavior. I think a little bit of clarification in the label schema section could make this more intuitive.

I'd be happy to submit a documentation update that might make this clearer for users in my use case.