Skip to content

Conversation

Gabefire
Copy link
Collaborator

@Gabefire Gabefire commented Sep 16, 2024

Description

  • Removed NDJsonConverter.deserialize from sdk
  • Notes on changes to some tests included

@Gabefire Gabefire requested a review from a team as a code owner September 16, 2024 18:13
(OntologyKind.ResponseCreation, GenericDataRowData),
],
)
def test_generic_data_row_type_by_global_key(
Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

)


def test_create_from_label_objects(
Copy link
Collaborator Author

@Gabefire Gabefire Sep 16, 2024

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

Copy link
Contributor

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

@Gabefire Gabefire changed the title Remove deserialize completely [PLT-1463] Remove deserialize completely Sep 16, 2024
@Gabefire Gabefire requested a review from vbrodsky September 16, 2024 18:41
on:
push:
branches: [develop]
branches: [develop, v6]
Copy link
Collaborator Author

@Gabefire Gabefire Sep 16, 2024

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 (
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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.

Copy link
Contributor

@vbrodsky vbrodsky left a 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

@Gabefire Gabefire merged commit fdb039a into v6 Sep 18, 2024
20 of 26 checks passed
@Gabefire Gabefire deleted the gu/remove_deserialize branch September 18, 2024 01:41
vbrodsky pushed a commit that referenced this pull request Sep 18, 2024
vbrodsky pushed a commit that referenced this pull request Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants