-
Notifications
You must be signed in to change notification settings - Fork 68
[PLT-1463] Remove deserialize completely #1818
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
(OntologyKind.ResponseCreation, GenericDataRowData), | ||
], | ||
) | ||
def test_generic_data_row_type_by_global_key( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the generic tests on this file in place of dedicated unit tests libs/labelbox/tests/data/serialization/ndjson/test_generic_data_row_data.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this test file completely since we are removing data classes besides the generic one in V6
@@ -87,27 +60,6 @@ def test_create_from_objects( | |||
) | |||
|
|||
|
|||
def test_create_from_label_objects( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this test completely since we are removing bulk import in V6. This would be complicated to get to work, so not worth our time to fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have a separate PR on BulkImportRequest
@@ -2,9 +2,9 @@ name: LBox Develop | |||
|
|||
on: | |||
push: | |||
branches: [develop] | |||
branches: [develop, v6] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add our V6 branch here just so tests can run we can remove once V6 is merged onto develop
@@ -0,0 +1,79 @@ | |||
from labelbox.data.annotation_types.data.generic_data_row_data import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: on the removed test, we tested all annotation types annotations_by_media_type
. But here we just test with 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think testing withevery annotation was a bit overkill and would be complicated to add in without deserialization existing. I would have to rewrite that annotations_by_media_type
fixture to provide Pydantic models over just the JSON and that fixture is pretty large. I wanted to simplify it by just seeing if the GenericDataRow serializes correctly. We also test for the GenericDataRow type with just the dictionary data={"global_key":global_key} and data={"uid":data_row_id} in our end to end testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Id have to rewrite most of this file https://github.com/Labelbox/labelbox-python/blob/develop/libs/labelbox/tests/data/annotation_import/conftest.py to make it work 😞 specifically the interfaces. Since we cant deserialize the JSON anymore directly to pydantic objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a story to this epic as a tech debt to add more unit testing for annotation serialization... perhaps Co-pilot can help to reduce work
Description