[PLA-585][external] Allow imports where some annotation frames are missing a main type #788

JBWilkie · 2024-03-14T15:57:19Z

Problem

This PR solves one main problem (1) but includes lots of clean up and refactoring of old V1 code and concepts as well (2 & 3). It's a very large PR, so I've annotated it heavily to help with reviewing it

1: In IO-1163, we introduced logic to exports to compress the resulting JSON if we determine that the export size was sufficiently large that it would cause it to OOM. This compression took the form of only exporting keyframes, and adding an only_keyframes field in the JSON to reflect this. Unfortunately, darwin-py import logic was built on the assumption that every frame has a main annotation type, which this change violates, breaking these imports
2: Removal of the concept of a complex_polygon annotation type. In V1, we stored polygons as complex polygons if they had >1 path. We removed this distinction in V2, instead referring to all polygons as type polygon. Therefore we should update darwin-py to reflect this
3: Removal of old functions & erroneous deprecation messages. Darwin-py contains a lot of unused functions from the V1 version of the platform. These have already been re-written for V2 and will thus never be used again. There were also many deprecation messages that were not needed, out of date or wrong (functions were actually in use)

Solution

1: For each annotation with only_keyframes set to True, get the first frame with a main type. Store this type & annotation data. Then, iterate through each frame of the annotation. If the main type is missing, use the stored value to populate it for import. Otherwise if the main type is present, update the stored annotation data. Repeat this for the entire annotation
2: Removed the concept of complex polygons from darwin-py, instead referring to all polygons simply as type polygon to reflect how we store polygons in our DB
3: Removed the unused functions, and removed incorrect deprecation messages

Changelog

Updated import logic to support importing of annotations that were compressed during export to avoid memory cap errors
Removed the distinction between complex polygons & polygons
Removed unused functions & erroneous deprecation warnings

…id E2E flakiness

…id E2E test flakiness

…sed to avoid OOM

linear · 2024-03-14T15:57:22Z

PLA-585 BUG: darwin-py cannot parse Darwin JSON 2.0 when only_keyframes is set to True

JBWilkie · 2024-03-14T16:02:45Z

darwin/dataset/download_manager.py

-    removed_in="0.8.0",
-    current_version=__version__,
-    details="The api_url parameter will be removed.",
-)
 def download_all_images_from_annotations(


This function is used by pull()

the deprecation warning points to a api_url parameter:

details="The api_url parameter will be removed.",

If we're removing the deprecation warning - Should we remove the api_url parameter from the function download_all_images_from_annotations?

I found a PR #335 which introduced the deprecation 👀

JBWilkie · 2024-03-14T16:12:46Z

darwin/exporter/formats/coco.py

@@ -401,7 +82,7 @@ def _calculate_tag_categories(
                categories[annotation_class.name] = _calculate_category_id(
                    annotation_class
                )
-    return categories
+    return dict(sorted(categories.items(), key=lambda item: item[1]))


These changes are to Ensure COCO categories are always in the same, ascending order to avoid E2E flakiness

Thank you for fixing this!! 🥇

We could also replace the lambda function here and use python's operator.itemgetter (it can be a little faster than lambda function, especially for large dictionaries), by something like:

from operator import itemgetter # importing it - return dict(sorted(categories.items(), key=lambda item: item[1])) + return dict(sorted(categories.items(), key=itemgetter(1)))

JBWilkie · 2024-03-14T16:16:24Z

darwin/importer/importer.py

-    removed_in="0.8.0",
-    current_version=__version__,
-    details=DEPRECATION_MESSAGE,
-)
 def build_main_annotations_lookup_table(


This is used when importing annotations

I wonder what was the goal behind deprecating this? 🤔

Ah found it in this PR e034b37#diff-c59e878e89aaf69bda12258e348fc25d69750c2841667a84da53f2d4f5bf60feR26-R28

Should we make it private instead by prefixing it with _?

JBWilkie · 2024-03-14T16:16:40Z

darwin/importer/importer.py

-    removed_in="0.8.0",
-    current_version=__version__,
-    details=DEPRECATION_MESSAGE,
-)
 def find_and_parse(  # noqa: C901


This is used when importing annotations

RE:

I wonder what was the goal behind deprecating this? 🤔

Ah found it in this PR e034b37#diff-c59e878e89aaf69bda12258e348fc25d69750c2841667a84da53f2d4f5bf60feR26-R28

Should we make it private instead by prefixing it with _?

JBWilkie · 2024-03-14T16:16:55Z

darwin/importer/importer.py

-    removed_in="0.8.0",
-    current_version=__version__,
-    details=DEPRECATION_MESSAGE,
-)
 def build_attribute_lookup(dataset: "RemoteDataset") -> Dict[str, Unknown]:


This is used when importing annotations

RE:

I wonder what was the goal behind deprecating this? 🤔

Ah found it in this PR e034b37#diff-c59e878e89aaf69bda12258e348fc25d69750c2841667a84da53f2d4f5bf60feR26-R28

Should we make it private instead by prefixing it with _?

JBWilkie · 2024-03-14T16:17:09Z

darwin/importer/importer.py

-    removed_in="0.8.0",
-    current_version=__version__,
-    details=DEPRECATION_MESSAGE,
-)
 def get_remote_files(


This is used when importing annotations

RE:

I wonder what was the goal behind deprecating this? 🤔

Ah found it in this PR e034b37#diff-c59e878e89aaf69bda12258e348fc25d69750c2841667a84da53f2d4f5bf60feR26-R28

Should we make it private instead by prefixing it with _?

JBWilkie · 2024-03-14T16:18:42Z

darwin/importer/importer.py

+    if "polygon" in data:
+        if len(annotation.data["paths"]) > 1:
+            data["polygon"] = {
+                "path": annotation.data["paths"][0],


When importing polygons, they have to be structured as a primary path, and any subsequent paths have to be passed in additional_paths in the payload

JBWilkie · 2024-03-14T16:20:15Z

darwin/utils/utils.py

@@ -128,7 +106,6 @@ def is_image_extension_allowed_by_filename(filename: str) -> bool:
    return any(filename.lower().endswith(ext) for ext in SUPPORTED_IMAGE_EXTENSIONS)


-@deprecation.deprecated(deprecated_in="0.8.4", current_version=__version__)
 def is_image_extension_allowed(extension: str) -> bool:


This is used when pulling exports

saurbhc · 2024-03-22T11:07:21Z

darwin/dataset/download_manager.py

-    removed_in="0.8.0",
-    current_version=__version__,
-    details="The api_url parameter will be removed.",
-)
 def download_all_images_from_annotations(


the deprecation warning points to a api_url parameter:

details="The api_url parameter will be removed.",

If we're removing the deprecation warning - Should we remove the api_url parameter from the function download_all_images_from_annotations?

I found a PR #335 which introduced the deprecation 👀

darwin/datatypes.py

saurbhc · 2024-03-22T11:20:15Z

darwin/exporter/exporter.py

@@ -36,6 +36,7 @@ def darwin_to_dt_gen(
                    for d in split_video_annotation(data):
                        d.seq = count
                        count += 1
+                        print(count)


Is the print for debugging? 🤔

saurbhc · 2024-03-22T11:36:12Z

darwin/exporter/formats/coco.py

@@ -401,7 +82,7 @@ def _calculate_tag_categories(
                categories[annotation_class.name] = _calculate_category_id(
                    annotation_class
                )
-    return categories
+    return dict(sorted(categories.items(), key=lambda item: item[1]))


Thank you for fixing this!! 🥇

We could also replace the lambda function here and use python's operator.itemgetter (it can be a little faster than lambda function, especially for large dictionaries), by something like:

from operator import itemgetter # importing it - return dict(sorted(categories.items(), key=lambda item: item[1])) + return dict(sorted(categories.items(), key=itemgetter(1)))

saurbhc · 2024-03-22T11:39:59Z

darwin/exporter/formats/coco.py

@@ -476,6 +157,7 @@ def _build_annotations(
 ) -> Iterator[Optional[Dict[str, Any]]]:
    annotation_id = 0
    for annotation_file in annotation_files:
+        print(annotation_file.filename)


Is the print for debugging? 🤔

saurbhc · 2024-03-22T11:49:11Z

darwin/importer/importer.py

-    removed_in="0.8.0",
-    current_version=__version__,
-    details=DEPRECATION_MESSAGE,
-)
 def build_main_annotations_lookup_table(


I wonder what was the goal behind deprecating this? 🤔

Ah found it in this PR e034b37#diff-c59e878e89aaf69bda12258e348fc25d69750c2841667a84da53f2d4f5bf60feR26-R28

Should we make it private instead by prefixing it with _?

saurbhc · 2024-03-22T11:49:42Z

darwin/importer/importer.py

-    removed_in="0.8.0",
-    current_version=__version__,
-    details=DEPRECATION_MESSAGE,
-)
 def find_and_parse(  # noqa: C901


RE:

I wonder what was the goal behind deprecating this? 🤔

Ah found it in this PR e034b37#diff-c59e878e89aaf69bda12258e348fc25d69750c2841667a84da53f2d4f5bf60feR26-R28

Should we make it private instead by prefixing it with _?

saurbhc · 2024-03-22T11:50:46Z

darwin/importer/importer.py

-    removed_in="0.8.0",
-    current_version=__version__,
-    details=DEPRECATION_MESSAGE,
-)
 def build_attribute_lookup(dataset: "RemoteDataset") -> Dict[str, Unknown]:


RE:

I wonder what was the goal behind deprecating this? 🤔

Ah found it in this PR e034b37#diff-c59e878e89aaf69bda12258e348fc25d69750c2841667a84da53f2d4f5bf60feR26-R28

Should we make it private instead by prefixing it with _?

saurbhc · 2024-03-22T11:50:55Z

darwin/importer/importer.py

-    removed_in="0.8.0",
-    current_version=__version__,
-    details=DEPRECATION_MESSAGE,
-)
 def get_remote_files(


RE:

I wonder what was the goal behind deprecating this? 🤔

Ah found it in this PR e034b37#diff-c59e878e89aaf69bda12258e348fc25d69750c2841667a84da53f2d4f5bf60feR26-R28

Should we make it private instead by prefixing it with _?

Co-authored-by: saurbhc <sc@saurabhchopra.co.uk>

JBWilkie · 2024-04-17T14:05:48Z

@saurbhc I've actioned all points of feedback

saurbhc

Thank you 🙏

JBWilkie added 24 commits February 12, 2024 15:31

WIP

b3ee31a

WIP

ec2c480

WIP

95330f8

WIP

bd559c2

WIP

686a2ff

WIP

6bcbd5e

Ensure COCO categories are always in the same, ascending order to avo…

e83bef2

…id E2E flakiness

Ensure COCO categories are always in the same, ascending order to avo…

6b1560a

…id E2E test flakiness

Merge branch 'master' into PLA-585

5a04079

WIP

2cb96c0

Removed deprecated functions & removed erroneous deprecation labels

e2c36e3

Fixed teststhat broke when removing deprecated functions

ac8fd93

WIP

7b0a55d

Merged master

7f0e797

Merge branch 'master' into PLA-585

3f090e2

Merge branch 'master' into PLA-585

2608b1d

Updated data.zip with v2 data structures

16a74eb

Undo accidental changes

ff67b48

1 failing test remaining (Video annotation parsing) (WIP)

c9e6428

Merge branch 'master' into PLA-585

74c274b

Moved ObjectStore tests to datatypes_test

143292c

Fixed video annotation import test

7258d44

Added support for simple table annotations incase exports are compres…

e5dc371

…sed to avoid OOM

Small fix for importing the raster layer

30c37b0

JBWilkie commented Mar 14, 2024

View reviewed changes

Removed 2 unused functions

8abd161

JBWilkie commented Mar 14, 2024

View reviewed changes

JBWilkie requested a review from saurbhc March 14, 2024 16:25

saurbhc reviewed Mar 22, 2024

View reviewed changes

JBWilkie and others added 9 commits April 17, 2024 11:51

Removed deprecated api_url parameter

61fc286

Replace lambda with itemgetter() for improved performance

24345f5

Turned 4 functions private

25b2110

Removed deprecated api_url parameter from function call

1e91028

Fixed private function calls

a2c9d6f

Removed debugging print statements

20f104f

Linting

51d7e77

Update darwin/datatypes.py

bae925f

Co-authored-by: saurbhc <sc@saurabhchopra.co.uk>

docstring updates

3bfa815

Merged master into branch

d6d4835

saurbhc approved these changes Apr 17, 2024

View reviewed changes

Merge master

81d4809

JBWilkie merged commit 973d9cc into master Apr 18, 2024
16 checks passed

filippocastelli mentioned this pull request Apr 22, 2024

breaking changes introduced in patch semantic increment #824

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PLA-585][external] Allow imports where some annotation frames are missing a main type #788

[PLA-585][external] Allow imports where some annotation frames are missing a main type #788

JBWilkie commented Mar 14, 2024

linear bot commented Mar 14, 2024

JBWilkie Mar 14, 2024

saurbhc Mar 22, 2024

JBWilkie Mar 14, 2024

saurbhc Mar 22, 2024

JBWilkie Mar 14, 2024

saurbhc Mar 22, 2024

JBWilkie Mar 14, 2024

saurbhc Mar 22, 2024

JBWilkie Mar 14, 2024

saurbhc Mar 22, 2024

JBWilkie Mar 14, 2024

saurbhc Mar 22, 2024

JBWilkie Mar 14, 2024

JBWilkie Mar 14, 2024

saurbhc Mar 22, 2024

saurbhc Mar 22, 2024

saurbhc Mar 22, 2024

saurbhc Mar 22, 2024

saurbhc Mar 22, 2024

saurbhc Mar 22, 2024

saurbhc Mar 22, 2024

saurbhc Mar 22, 2024

JBWilkie commented Apr 17, 2024

saurbhc left a comment

[PLA-585][external] Allow imports where some annotation frames are missing a main type #788

[PLA-585][external] Allow imports where some annotation frames are missing a main type #788

Conversation

JBWilkie commented Mar 14, 2024

Problem

Solution

Changelog

linear bot commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JBWilkie commented Apr 17, 2024

saurbhc left a comment

Choose a reason for hiding this comment