Pass custom label schema attribute keys to annotation backends #5502
Conversation
WalkthroughThe changes modify the attribute handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Formatter
Caller->>Formatter: Call _format_attributes(attr)
Formatter->>Formatter: Copy attr to _attr
Formatter->>Formatter: Extract keys (attr_type, values, default, mutable, read_only)
Formatter->>Formatter: Validate attribute type and check for errors
Formatter->>Caller: Return modified _attr
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
fiftyone/utils/annotations.py
Outdated
| default = attr.get("default", None) | ||
| mutable = attr.get("mutable", None) | ||
| read_only = attr.get("read_only", None) | ||
| _attr = attr.copy() |
There was a problem hiding this comment.
Should we use deepcopy here instead?
There was a problem hiding this comment.
The current implementation doesn't modify nested values so it doesn't matter. However, agree that deepcopy would be more proper.
| @@ -978,7 +979,7 @@ def _format_attributes(backend, attributes): | |||
| ) | |||
| ) | |||
|
|
|||
| _attr = {"type": attr_type} | |||
| _attr["type"] = attr_type | |||
There was a problem hiding this comment.
Not a 100% sure how this works, we already are getting attr_type from _attr in line 959 and attrr_type is not modified or updated in between?
There was a problem hiding this comment.
If @ehofesmann implements my suggestion above, then this line would be required.
As currently implemented, I agree it is a no-op.
fiftyone/utils/annotations.py
Outdated
| read_only = attr.get("read_only", None) | ||
| _attr = attr.copy() | ||
| attr_type = _attr.get("type", None) | ||
| values = _attr.get("values", None) |
There was a problem hiding this comment.
@ehofesmann FYI I believe this slightly changes how _attr is constructed.
For example:
- Previously if
attr["mutable"] == None, then_attrwould not contain a"mutable"key, since it is only added if it is non-None - Now as implemented,
_attrwould contain a"mutable"key with valueNone
I'm not sure if that change in behavior breaks anything, but you could maintain the current behavior if you used pop() instead of get():
attr_type = _attr.pop("type", None)
values = _attr.pop("values", None)
default = _attr.pop("default", None)
mutable = _attr.pop("mutable", None)
read_only = _attr.pop("read_only", None)| @@ -978,7 +979,7 @@ def _format_attributes(backend, attributes): | |||
| ) | |||
| ) | |||
|
|
|||
| _attr = {"type": attr_type} | |||
| _attr["type"] = attr_type | |||
There was a problem hiding this comment.
If @ehofesmann implements my suggestion above, then this line would be required.
As currently implemented, I agree it is a no-op.
fiftyone/utils/annotations.py
Outdated
| default = attr.get("default", None) | ||
| mutable = attr.get("mutable", None) | ||
| read_only = attr.get("read_only", None) | ||
| _attr = attr.copy() |
There was a problem hiding this comment.
The current implementation doesn't modify nested values so it doesn't matter. However, agree that deepcopy would be more proper.
What changes are proposed in this pull request?
Previously, the keys of attribute properties in an annotation label schema were filtered to only include a preconfigured set of values. For example, if you were to pass in this label schema:
It would only pass along the "type" and "values" properties and remove the "otherattrf". This means that if a new annotation backend has custom attribute properties, there is no way to define those in the label schema.
How is this patch tested? If it is not, please explain why.
The cvat, labelbox, and labelstudio tests were rerun and passed.
Printing the label schema from inside of an annotation backend now shows the custom attributes.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Annotation label schema attributes now support custom attributes for annotation backends.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit