[AL-2778][AL-2779] Add name field to annotations #614

farkob · 2022-06-27T16:40:29Z

Adds name field to annotation objects where feature_id required. Makes both fields optional and require one field to be present.

jtsodapop · 2022-06-27T18:23:19Z

labelbox/data/serialization/ndjson/label.py

                    annot,
                (VideoClassificationAnnotation, VideoObjectAnnotation)):
-                video_annotations[annot.feature_schema_id].append(annot)
+                video_annotations[annot.name or annot.schema_id].append(annot)


I think it likely doesn't actually do anything to this line since we don't use video_annotations outside of the purpose of having the values(), but maybe we should make it so that it is annot.schema_id or annot.name instead of the reverse?

yeah good idea

jtsodapop · 2022-06-27T18:28:21Z

labelbox/data/serialization/ndjson/base.py

@@ -1,3 +1,4 @@
+from typing import Optional


writing the comment here, but it looks like the test cases are failing as a result of these changes. they will likely need some updates

jtsodapop · 2022-07-01T19:23:20Z

labelbox/data/annotation_types/feature.py

+        res = super().dict(*args, **kwargs)
+        if 'name' in res and res['name'] is None:
+            res.pop('name')
+        if 'featureSchemaId' in res and res.featureSchemaId is None:


is there a reason we use res['name'] and res.featureSchemaId?

ah forgot to change it

jtsodapop

LGTM, nit comments + i think some extra lines that don't need to be in there.

jtsodapop · 2022-07-01T19:24:52Z

labelbox/data/serialization/ndjson/classification.py

@@ -1,3 +1,4 @@
+from tkinter import N


Suggestion maybe this was added via your editor? if so, let's drop

jtsodapop · 2022-07-01T19:26:52Z

labelbox/data/serialization/ndjson/label.py


        video_annotations = defaultdict(list)
        for annot in label.annotations:
+            # print(dict(annot))


suggestion think this may be extra line we can cut out

kkim-labelbox

Code LGTM other than one super minor nit. Could you add tests as well?

kkim-labelbox · 2022-07-01T20:24:12Z

labelbox/data/serialization/ndjson/classification.py

+    @validator('name', pre=True, always=True)
+    def validate_name(cls, v, values):
+        if v is None and 'schema_id' not in values:
+            raise ValueError("Name is not set. Either set name or schema_id.")


Name and schema_id are not set. Either set name or schema_id

kkim-labelbox · 2022-07-01T20:34:50Z

labelbox/data/serialization/ndjson/classification.py

                    feature_schema_id: Cuid) -> "NDRadioSubclass":
-        return cls(answer=NDFeature(schema_id=radio.answer.feature_schema_id),
+        return cls(answer=NDFeature(name=radio.answer.name,
+                                    schema_id=radio.answer.feature_schema_id),


Unnecessary newline? unless github messed up formatting?

formatter did this 🤷

kkim-labelbox · 2022-07-01T21:23:45Z

It might also be good to add updates to docstrings that talk about schema_ids being required for MAL:

data/annotation_types/feature.py - FeatureSchema suggests using assign_feature_schema_ids to find schema_ids based on name. This is no longer necessary, so it'll be good to note that name can be provided to MAL
data/annotation_types/collection.py - LabelList.assign_feature_schema_ids says schema_ids required for MAL
data/annotation_types/label.py - Label.assign_feature_schema_ids says schema_ids required fr MAL

kkim-labelbox

I had a couple nit comments around wording, and have a request for more test cases.
Could you also add tests for the serialization/deserialization logic, since a bulk of your change touches the annotation type conversion logic?
I see we have a suite of tests under tests/data/serialization/ndsjon that are calling NDJsonConverter.serialize/deserialize across different data types. It'd be great to add tests cases here, for when only name is present. I just want to make sure we're covering test cases for the changes being made, and these additional test cases will ensure that the serialization/deserialization logic are working properly. Thanks!

kkim-labelbox · 2022-07-11T17:31:25Z

labelbox/data/annotation_types/label.py

@@ -1,5 +1,6 @@
 from collections import defaultdict
 from typing import Any, Callable, Dict, List, Union, Optional
+import warnings


Unnecessary import?

kkim-labelbox · 2022-07-11T18:15:12Z

labelbox/data/annotation_types/collection.py

        Returns:
            LabelList. useful for chaining these modifying functions
+
+        Warning: assign_feature_schema_ids is now obsolete, you can


Warning: assign_feature_schema_ids is now obsolete, you can now use names directly without having to lookup schema_ids.

Warning might be too "harsh". I'd just put "Note:"

I'd add you can now import annotations using names directly without having to lookup schema_ids

kkim-labelbox · 2022-07-11T18:15:29Z

labelbox/data/annotation_types/label.py

            Label. useful for chaining these modifying functions
+
+        Warning: assign_feature_schema_ids is now obsolete, you can
+                 now use names directly without having to lookup schema_ids.


Same wording as above

kkim-labelbox · 2022-07-11T18:24:20Z

labelbox/schema/bulk_import_request.py

+        if self.name:
+            if self.name not in valid_feature_schemas_by_name:
+                raise ValueError(
+                    f"name {self.name} is not valid for the provided project's ontology."


Let's make sure the error outputted to customers are all formatted and capitalized consistently.
Name {self.name} is not valid for the provided project's ontology

kkim-labelbox · 2022-07-11T18:24:48Z

labelbox/schema/bulk_import_request.py

+        if self.schemaId:
+            if self.schemaId not in valid_feature_schemas_by_id:
+                raise ValueError(
+                    f"schema id {self.schemaId} is not valid for the provided project's ontology."


Capitalize Schema

kkim-labelbox · 2022-07-13T17:46:23Z

tests/data/assets/ndjson/classification_import_name_only.json

@@ -0,0 +1,20 @@
+[
+  {
+    "answer": { "schemaId": "ckrb1sfl8099g0y91cxbd5ftb" },


Could you use name here instead of schemaId for answer?

kkim-labelbox · 2022-07-13T17:47:10Z

tests/data/assets/ndjson/classification_import_name_only.json

+  },
+  {
+    "answer": "a value",
+    "name": "classification b",


There are two classifications named classification b

# Conflicts: # labelbox/data/serialization/ndjson/label.py # labelbox/data/serialization/ndjson/objects.py

kkim-labelbox

Added one more tiny comment, but LGTM otherwise!

kkim-labelbox · 2022-07-13T20:23:37Z

labelbox/data/annotation_types/label.py

        Returns:
            Label. useful for chaining these modifying functions
+
+        Warning: You can now import annotations using names directly without having to lookup schema_ids


Change this to Note: You can now import annotations using names directly without having to lookup schema_ids, and remove import warning?

[AL-2778][AL-2779] Add name field to annotations

4ed6785

farkob requested a review from jtsodapop June 27, 2022 16:40

github-actions bot added the active-learning label Jun 27, 2022

jtsodapop reviewed Jun 27, 2022

View reviewed changes

farkob added 11 commits June 28, 2022 19:48

exclude none values when serializing

99f6ff4

remove only necessary fields

09b5d45

override dicts

d841ef4

change object access

ebd97c1

update tests

25db9d4

remove more None

69cf86f

use camel case

a28ff68

remove from dict

bf4b3e4

updates

f9a93d9

no by_alias

bee42bb

Merge remote-tracking branch 'origin/develop' into farkob/schema_id_name

86f338c

farkob requested review from jtsodapop and msokoloff1 July 1, 2022 19:06

update error

7f948f9

jtsodapop reviewed Jul 1, 2022

View reviewed changes

pr comments

1aae84c

kkim-labelbox suggested changes Jul 1, 2022

View reviewed changes

farkob added 2 commits July 11, 2022 19:44

fix validation, add tests, add comments

4ac94f3

Merge remote-tracking branch 'origin/develop' into farkob/schema_id_name

8a44aaa

kkim-labelbox reviewed Jul 11, 2022

View reviewed changes

farkob added 3 commits July 13, 2022 19:48

serialization and deserialization tests

e83b12c

formatting

611b80d

add name to video bbox

4ed0663

kkim-labelbox reviewed Jul 13, 2022

View reviewed changes

farkob added 3 commits July 13, 2022 22:36

fix field name and answers

b2048e8

Merge remote-tracking branch 'origin/develop' into farkob/schema_id_name

2ba0762

# Conflicts: # labelbox/data/serialization/ndjson/label.py # labelbox/data/serialization/ndjson/objects.py

fix semantic conflict and add more tests

c077327

kkim-labelbox approved these changes Jul 13, 2022

View reviewed changes

update comment, remove unnecessary import

b08d63a

farkob merged commit 5e1e228 into develop Jul 13, 2022

farkob deleted the farkob/schema_id_name branch July 13, 2022 22:36

[AL-2778][AL-2779] Add name field to annotations #614

[AL-2778][AL-2779] Add name field to annotations #614

Uh oh!

Conversation

farkob commented Jun 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtsodapop left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkim-labelbox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkim-labelbox commented Jul 1, 2022

Uh oh!

kkim-labelbox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkim-labelbox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants