From 67daf74c5430a3cc63a8373b7c9fc0b910f02cdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksandar=20Ivi=C4=87?= Date: Fri, 28 Feb 2025 13:15:09 +0100 Subject: [PATCH] [DAR-5929] Allow annotation and text properties import (#1009) * [DAR-5929] Allow annotation and text properties import * [DAR-5929] Revert change of base url * [DAR-5929] Add additional tests * [DAR-5929] Support empty values for text properties and test same name for item and section/annotation level props * [DAR-5929] Fix linter * [DAR-5929] Remove accidental import * [DAR-5929] Add support for null value text properties * [DAR-5929] Remove unnecessary check for empty string * [DAR-5929] Support null values for text item level props * [DAR-5929] Revert change of base url * [DAR-5929] Format files * [DAR-5929] Support null values for multi-select and single-select --------- Co-authored-by: Umberto --- .gitignore | 1 + darwin/importer/importer.py | 168 +++++++++++------- e2e_tests/cli/test_import.py | 3 +- .../image_1.json | 40 +++++ .../image_2.json | 35 ++++ .../image_3.json | 15 ++ .../image_4.json | 5 + .../image_5.json | 25 +++ .../.v7/metadata.json | 15 ++ .../image_1.json | 4 + .../image_2.json | 2 +- .../image_3.json | 10 +- .../.v7/metadata.json | 7 + .../image_1.json | 5 + .../image_2.json | 5 + .../image_3.json | 5 + .../mini_uct.json | 20 +++ e2e_tests/setup_tests.py | 13 ++ 18 files changed, 304 insertions(+), 74 deletions(-) diff --git a/.gitignore b/.gitignore index 5aaf6f18c..5221d3dbc 100644 --- a/.gitignore +++ b/.gitignore @@ -168,6 +168,7 @@ test_output_* !darwin/future/tests/data # scripts test.py +run.py # pyenv .python-version diff --git a/darwin/importer/importer.py b/darwin/importer/importer.py index 731342f87..22452f6e8 100644 --- a/darwin/importer/importer.py +++ b/darwin/importer/importer.py @@ -457,9 +457,11 @@ def _serialize_item_level_properties( _, team_item_properties_lookup = _get_team_properties_annotation_lookup( client, dataset.team ) + # We will skip text item properties that have value null for item_property_value in item_property_values: item_property = team_item_properties_lookup[item_property_value["name"]] item_property_id = item_property.id + value = None if ( item_property.type == "single_select" or item_property.type == "multi_select" @@ -472,25 +474,30 @@ def _serialize_item_level_properties( ), None, ) - value = {"id": item_property_value_id} - elif item_property.type == "text": + if item_property_value_id is not None: + value = {"id": item_property_value_id} + elif item_property.type == "text" and item_property_value["value"] is not None: value = {"text": item_property_value["value"]} - actors: List[dt.DictFreeForm] = [] - actors.extend( - _handle_annotators( - import_annotators, item_property_value=item_property_value + if value is not None: + actors: List[dt.DictFreeForm] = [] + actors.extend( + _handle_annotators( + import_annotators, item_property_value=item_property_value + ) + ) + + actors.extend( + _handle_reviewers( + import_reviewers, item_property_value=item_property_value + ) + ) + serialized_item_level_properties.append( + { + "actors": actors, + "property_id": item_property_id, + "value": value, + } ) - ) - actors.extend( - _handle_reviewers(import_reviewers, item_property_value=item_property_value) - ) - serialized_item_level_properties.append( - { - "actors": actors, - "property_id": item_property_id, - "value": value, - } - ) return serialized_item_level_properties @@ -623,6 +630,14 @@ def _import_properties( t_prop: FullProperty = team_properties_annotation_lookup[ (a_prop.name, annotation_class_id) ] + if t_prop.type == "text": + set_text_property_value( + annotation_property_map, + annotation_id, + a_prop, + t_prop, + ) + continue # if property value is None, update annotation_property_map with empty set if a_prop.value is None: @@ -759,14 +774,15 @@ def _import_properties( continue # check if property value is different in m_prop (.v7/metadata.json) options - for m_prop_option in m_prop_options: - if m_prop_option.get("value") == a_prop.value: - break - else: - if a_prop.value: - raise ValueError( - f"Annotation: '{annotation_name}' -> Property '{a_prop.value}' not found in .v7/metadata.json, found: {m_prop.property_values}" - ) + if m_prop.type != "text": + for m_prop_option in m_prop_options: + if m_prop_option.get("value") == a_prop.value: + break + else: + if a_prop.value: + raise ValueError( + f"Annotation: '{annotation_name}' -> Property '{a_prop.value}' not found in .v7/metadata.json, found: {m_prop.property_values}" + ) # get team property t_prop: FullProperty = team_properties_annotation_lookup[ @@ -782,43 +798,50 @@ def _import_properties( continue # check if property value is different in t_prop (team) options - for t_prop_val in t_prop.property_values or []: - if t_prop_val.value == a_prop.value: - break - else: - # if it is, update it - full_property = FullProperty( - id=t_prop.id, - name=a_prop.name, - type=m_prop_type, - required=m_prop.required, - description=m_prop.description - or "property-updated-during-annotation-import", - slug=client.default_team, - annotation_class_id=int(annotation_class_id), - property_values=[ - PropertyValue( - value=a_prop.value, - color=m_prop_option.get("color"), # type: ignore - ) - ], - granularity=t_prop.granularity, - ) - # Don't attempt the same propery update multiple times - if ( - full_property - not in annotation_and_section_level_properties_to_update - ): - annotation_and_section_level_properties_to_update.append( - full_property + if t_prop.type != "text": + for t_prop_val in t_prop.property_values or []: + if t_prop_val.value == a_prop.value: + break + else: + # if it is, update it + full_property = FullProperty( + id=t_prop.id, + name=a_prop.name, + type=m_prop_type, + required=m_prop.required, + description=m_prop.description + or "property-updated-during-annotation-import", + slug=client.default_team, + annotation_class_id=int(annotation_class_id), + property_values=[ + PropertyValue( + value=a_prop.value, + color=m_prop_option.get("color"), # type: ignore + ) + ], + granularity=t_prop.granularity, ) - continue + # Don't attempt the same propery update multiple times + if ( + full_property + not in annotation_and_section_level_properties_to_update + ): + annotation_and_section_level_properties_to_update.append( + full_property + ) + continue assert t_prop.id is not None - assert t_prop_val.id is not None - annotation_property_map[annotation_id][str(a_prop.frame_index)][ - t_prop.id - ].add(t_prop_val.id) + + if t_prop.type == "text": + set_text_property_value( + annotation_property_map, annotation_id, a_prop, t_prop + ) + else: + assert t_prop_val.id is not None + annotation_property_map[annotation_id][str(a_prop.frame_index)][ + t_prop.id + ].add(t_prop_val.id) # Create/Update team item properties based on metadata ( @@ -998,12 +1021,17 @@ def _import_properties( ] = set() break - for prop_val in prop.property_values or []: - if prop_val.value == a_prop.value: - annotation_property_map[annotation_id][frame_index][ - prop.id - ].add(prop_val.id) - break + if prop.type == "text": + set_text_property_value( + annotation_property_map, annotation_id, a_prop, prop + ) + else: + for prop_val in prop.property_values or []: + if prop_val.value == a_prop.value: + annotation_property_map[annotation_id][frame_index][ + prop.id + ].add(prop_val.id) + break break _assign_item_properties_to_dataset( item_properties, team_item_properties_lookup, client, dataset, console @@ -2488,3 +2516,13 @@ def slot_is_medical(slot: Dict[str, Any]) -> bool: def slot_is_handled_by_monai(slot: Dict[str, Any]) -> bool: return slot.get("metadata", {}).get("medical", {}).get("handler") == "MONAI" + + +def set_text_property_value(annotation_property_map, annotation_id, a_prop, t_prop): + if a_prop.value is None: + # here we will remove the property value + annotation_property_map[annotation_id][str(a_prop.frame_index)][t_prop.id] = [] + else: + annotation_property_map[annotation_id][str(a_prop.frame_index)][t_prop.id] = [ + {"text": a_prop.value} + ] diff --git a/e2e_tests/cli/test_import.py b/e2e_tests/cli/test_import.py index daccbf663..bb6554bff 100644 --- a/e2e_tests/cli/test_import.py +++ b/e2e_tests/cli/test_import.py @@ -217,7 +217,8 @@ def assert_same_item_level_properties( Ensures that all expected item-level properties are present in exported item-level properties """ for expected_item_level_property in expected_item_level_properties: - assert expected_item_level_property in actual_item_level_properties + if expected_item_level_property["value"] is not None: + assert expected_item_level_property in actual_item_level_properties def compare_annotations_export( diff --git a/e2e_tests/data/import/image_annotations_with_subtypes/image_1.json b/e2e_tests/data/import/image_annotations_with_subtypes/image_1.json index c5255aa3b..c30b51097 100644 --- a/e2e_tests/data/import/image_annotations_with_subtypes/image_1.json +++ b/e2e_tests/data/import/image_annotations_with_subtypes/image_1.json @@ -62,6 +62,16 @@ "frame_index": 0, "name": "single_select-1", "value": "1" + }, + { + "frame_index": 0, + "name": "section-text-1", + "value": "Section test" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "slot_names": [ @@ -93,6 +103,11 @@ "frame_index": 0, "name": "single_select-1", "value": "1" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": null } ], "slot_names": [ @@ -122,6 +137,11 @@ "frame_index": 0, "name": "multi_select-1", "value": "2" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "slot_names": [ @@ -158,6 +178,16 @@ "frame_index": 0, "name": "single_select-1", "value": "2" + }, + { + "frame_index": 0, + "name": "section-text-1", + "value": "Section test" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "slot_names": [ @@ -220,6 +250,11 @@ "frame_index": 0, "name": "single_select-1", "value": "2" + }, + { + "frame_index": 0, + "name": "section-text-1", + "value": "Section test" } ], "slot_names": [ @@ -245,6 +280,11 @@ "frame_index": 0, "name": "multi_select-1", "value": "1" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "skeleton": { diff --git a/e2e_tests/data/import/image_annotations_with_subtypes/image_2.json b/e2e_tests/data/import/image_annotations_with_subtypes/image_2.json index 9a91cd116..83cdd9d97 100644 --- a/e2e_tests/data/import/image_annotations_with_subtypes/image_2.json +++ b/e2e_tests/data/import/image_annotations_with_subtypes/image_2.json @@ -62,6 +62,16 @@ "frame_index": 0, "name": "single_select-1", "value": "1" + }, + { + "frame_index": 0, + "name": "section-text-1", + "value": "Section test" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "slot_names": [ @@ -93,6 +103,11 @@ "frame_index": 0, "name": "single_select-1", "value": "1" + }, + { + "frame_index": 0, + "name": "section-text-1", + "value": "Section test" } ], "slot_names": [ @@ -122,6 +137,11 @@ "frame_index": 0, "name": "multi_select-1", "value": "2" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "slot_names": [ @@ -220,6 +240,16 @@ "frame_index": 0, "name": "single_select-1", "value": "2" + }, + { + "frame_index": 0, + "name": "section-text-1", + "value": "Section test" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "slot_names": [ @@ -288,6 +318,11 @@ "frame_index": 0, "name": "single_select-1", "value": "1" + }, + { + "frame_index": 0, + "name": "section-text-1", + "value": "Section test" } ], "slot_names": [ diff --git a/e2e_tests/data/import/image_annotations_with_subtypes/image_3.json b/e2e_tests/data/import/image_annotations_with_subtypes/image_3.json index 907c2049b..3ced6d5b5 100644 --- a/e2e_tests/data/import/image_annotations_with_subtypes/image_3.json +++ b/e2e_tests/data/import/image_annotations_with_subtypes/image_3.json @@ -62,6 +62,11 @@ "frame_index": 0, "name": "single_select-1", "value": "1" + }, + { + "frame_index": 0, + "name": "section-text-1", + "value": "Section test" } ], "slot_names": [ @@ -93,6 +98,11 @@ "frame_index": 0, "name": "single_select-1", "value": "1" + }, + { + "frame_index": 0, + "name": "section-text-1", + "value": "Section test" } ], "slot_names": [ @@ -122,6 +132,11 @@ "frame_index": 0, "name": "multi_select-1", "value": "2" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "slot_names": [ diff --git a/e2e_tests/data/import/image_annotations_with_subtypes/image_4.json b/e2e_tests/data/import/image_annotations_with_subtypes/image_4.json index dd9c4b30d..6c1be2c46 100644 --- a/e2e_tests/data/import/image_annotations_with_subtypes/image_4.json +++ b/e2e_tests/data/import/image_annotations_with_subtypes/image_4.json @@ -220,6 +220,11 @@ "frame_index": 0, "name": "single_select-1", "value": "2" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "slot_names": [ diff --git a/e2e_tests/data/import/image_annotations_with_subtypes/image_5.json b/e2e_tests/data/import/image_annotations_with_subtypes/image_5.json index 85799627c..f670bfb57 100644 --- a/e2e_tests/data/import/image_annotations_with_subtypes/image_5.json +++ b/e2e_tests/data/import/image_annotations_with_subtypes/image_5.json @@ -62,6 +62,11 @@ "frame_index": 0, "name": "single_select-1", "value": "1" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "slot_names": [ @@ -93,6 +98,16 @@ "frame_index": 0, "name": "single_select-1", "value": "1" + }, + { + "frame_index": 0, + "name": "section-text-1", + "value": "Section test" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "slot_names": [ @@ -158,6 +173,16 @@ "frame_index": 0, "name": "single_select-1", "value": "2" + }, + { + "frame_index": 0, + "name": "section-text-1", + "value": "Section test" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "slot_names": [ diff --git a/e2e_tests/data/import/image_new_annotations_with_item_level_properties/.v7/metadata.json b/e2e_tests/data/import/image_new_annotations_with_item_level_properties/.v7/metadata.json index f296760a5..2516d3403 100644 --- a/e2e_tests/data/import/image_new_annotations_with_item_level_properties/.v7/metadata.json +++ b/e2e_tests/data/import/image_new_annotations_with_item_level_properties/.v7/metadata.json @@ -52,6 +52,13 @@ "color": "rgba(255,199,0,1.0)" } ] + }, + { + "name": "test_same_name_text", + "type": "text", + "description": "", + "required": false, + "property_values": [] } ], "sub_types_settings": { @@ -97,6 +104,14 @@ "required": false, "property_values": [], "granularity": "item" + }, + { + "name": "test_same_name_text", + "type": "text", + "description": "", + "required": false, + "property_values": [], + "granularity": "item" } ] } \ No newline at end of file diff --git a/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_1.json b/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_1.json index 328ad56a8..6497226d1 100644 --- a/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_1.json +++ b/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_1.json @@ -594,6 +594,10 @@ { "name": "new_item_level_property_text", "value": "sample text" + }, + { + "name": "test_same_name_text", + "value": null } ] } \ No newline at end of file diff --git a/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_2.json b/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_2.json index 69eff4fc9..3413d3445 100644 --- a/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_2.json +++ b/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_2.json @@ -481,7 +481,7 @@ }, { "name": "new_item_level_property_single_select", - "value": "1" + "value": null }, { "name": "new_item_level_property_text", diff --git a/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_3.json b/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_3.json index c9c1c1ec1..75c077d57 100644 --- a/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_3.json +++ b/e2e_tests/data/import/image_new_annotations_with_item_level_properties/image_3.json @@ -517,19 +517,15 @@ "properties": [ { "name": "new_item_level_property_multi_select", - "value": "1" - }, - { - "name": "new_item_level_property_multi_select", - "value": "2" + "value": null }, { "name": "new_item_level_property_single_select", - "value": "1" + "value": null }, { "name": "new_item_level_property_text", - "value": "sample text" + "value": null } ] } \ No newline at end of file diff --git a/e2e_tests/data/import/image_new_annotations_with_properties/.v7/metadata.json b/e2e_tests/data/import/image_new_annotations_with_properties/.v7/metadata.json index 8205569ad..c228ee41a 100644 --- a/e2e_tests/data/import/image_new_annotations_with_properties/.v7/metadata.json +++ b/e2e_tests/data/import/image_new_annotations_with_properties/.v7/metadata.json @@ -109,6 +109,13 @@ "color": "rgba(0,255,85,1.0)" } ] + }, + { + "name": "sec-lvl-text", + "type": "text", + "description": "", + "required": false, + "property_values": [] } ], "sub_types_settings": { diff --git a/e2e_tests/data/import/image_new_annotations_with_properties/image_1.json b/e2e_tests/data/import/image_new_annotations_with_properties/image_1.json index 994bde7e1..25948651e 100644 --- a/e2e_tests/data/import/image_new_annotations_with_properties/image_1.json +++ b/e2e_tests/data/import/image_new_annotations_with_properties/image_1.json @@ -189,6 +189,11 @@ "frame_index": 0, "name": "single_select-1", "value": "2" + }, + { + "frame_index": 0, + "name": "sec-lvl-text", + "value": "Section text test" } ], "slot_names": [ diff --git a/e2e_tests/data/import/image_new_annotations_with_properties/image_2.json b/e2e_tests/data/import/image_new_annotations_with_properties/image_2.json index 74778af52..c533ed8fd 100644 --- a/e2e_tests/data/import/image_new_annotations_with_properties/image_2.json +++ b/e2e_tests/data/import/image_new_annotations_with_properties/image_2.json @@ -189,6 +189,11 @@ "frame_index": 0, "name": "single_select-1", "value": "2" + }, + { + "frame_index": 0, + "name": "sec-lvl-text", + "value": "Section text test" } ], "slot_names": [ diff --git a/e2e_tests/data/import/image_new_annotations_with_properties/image_3.json b/e2e_tests/data/import/image_new_annotations_with_properties/image_3.json index d04dfb70e..e20b5462c 100644 --- a/e2e_tests/data/import/image_new_annotations_with_properties/image_3.json +++ b/e2e_tests/data/import/image_new_annotations_with_properties/image_3.json @@ -189,6 +189,11 @@ "frame_index": 0, "name": "single_select-1", "value": "2" + }, + { + "frame_index": 0, + "name": "sec-lvl-text", + "value": "Section text test" } ], "slot_names": [ diff --git a/e2e_tests/data/import/video_annotations_with_subtypes/mini_uct.json b/e2e_tests/data/import/video_annotations_with_subtypes/mini_uct.json index 2eb80bfc5..4122ad482 100644 --- a/e2e_tests/data/import/video_annotations_with_subtypes/mini_uct.json +++ b/e2e_tests/data/import/video_annotations_with_subtypes/mini_uct.json @@ -147,6 +147,16 @@ "frame_index": 1, "name": "single_select-1", "value": "2" + }, + { + "frame_index": 1, + "name": "section-text-1", + "value": "Section test 1" + }, + { + "frame_index": "global", + "name": "annotation-text-1", + "value": "Annotation test" } ], "ranges": [ @@ -293,6 +303,11 @@ "frame_index": 8, "name": "single_select-1", "value": "2" + }, + { + "frame_index": 8, + "name": "section-text-1", + "value": "Section test 8" } ], "ranges": [ @@ -599,6 +614,11 @@ "frame_index": 5, "name": "single_select-1", "value": "2" + }, + { + "frame_index": 5, + "name": "section-text-1", + "value": "Section test 5" } ], "ranges": [ diff --git a/e2e_tests/setup_tests.py b/e2e_tests/setup_tests.py index 7ad4daed7..0edb755a8 100644 --- a/e2e_tests/setup_tests.py +++ b/e2e_tests/setup_tests.py @@ -97,6 +97,19 @@ def add_properties_to_class( "content-type": "application/json", "Authorization": f"ApiKey {config.api_key}", } + # setup text properties + for text_granularity in ["annotation", "section"]: + payload = { + "name": f"{text_granularity}-text-1", + "type": "text", + "required": False, + "annotation_class_id": annotation_class_info["id"], + "description": f"Description for a text property on {text_granularity} level", + "granularity": f"{text_granularity}", + "property_values": [], + } + requests.post(url, json=payload, headers=headers) + for property_type in ["single_select", "multi_select"]: payload = { "required": False,