[PLA-595] Don't create sub-annotation keyframe for every keyframe when importing annotations

[saurbhc]

Problem

As of now, when importing annotations, we automatically make every keyframe a "sub-annotation keyframe" by marking that the sub-annotation has changed.

Solution

during import, between D-Py and backend we still use this format which include both annotation and sub-annotation data at the same time, for each "keyframe". It would be kinda-possible to drop sub-annotation data from the request whenever we know it didn't change since last one, which likely wouldn't create on backend side sub-annotation keyframe.

Changelog

Don't create sub-annotation keyframe for every keyframe when importing annotations

[linear]

PLA-595 Request: Don't create sub-annotation keyframe for every keyframe when importing annotations

[JBWilkie]

Tested and LGTM - Duplicate sub-annotations (those that haven't changed since the last change) are removed the import payload correctly. However, this doesn't appear to accomplish the overall goal of PLA-595, which makes me think it may require backend work

[JBWilkie]

@saurbhc I know I approved this, but I found an issue in the case that there is >1 sub-annotation per annotation. I made this change to support those cases. Could you please take a look and verify?

[saurbhc]

@JBWilkie thank you for updating the code 💯

The code is already quite efficient, but there's a small optimisation you can make, I understand the intent to use all() and any() which both iterate over all elements -- you can use a set to store the previous sub-annotations and check if the current sub-annotation is in the set? something like:

last_subs = set()
for _, _annotation in annotation.frames.items():
    _annotation: dt.Annotation
    subs = []
    for sub in _annotation.subs:
        sub_tuple = (sub.annotation_type, sub.data)
        if sub_tuple in last_subs:
            # drop sub-annotation whenever we know it didn't change since last one
            # which likely wouldn't create on backend side sub-annotation keyframe.
            # this is a workaround for the backend not handling duplicate sub-annotations.
            continue
        subs.append(sub)
        last_subs.add(sub_tuple)
    _annotation.subs = subs

• last_subs is a set that stores tuples of (sub.annotation_type, sub.data)
• This should be faster because checking if an element is in a set is an O(1) operation, while all() and any() are O(n) operations.

What do you think?

[saurbhc]

Ah, wait!

I just learned that if the sub-annotation (sub.data) is a list of attributes, then sub.data will be a list which is un-hashable. we can convert it to tuple first like

sub_tuple = (sub.annotation_type, tuple(sub.data))

but it already seems a bit complicated and we might need to handle it for other datatypes too.

Let's Ignore this optimization bit! Thank you!!

[JBWilkie]

Ah that's a shame about the list of attributes, otherwise this is a very nice change and makes it much more readable

[JBWilkie]

But thank you for your work here @saurbhc! Please merge whenever :)