Skip to content

[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

Merged
merged 10 commits into from
Sep 18, 2024
Merged

[PLT-1463] Remove deserialize completely #1818

merged 10 commits into from
Sep 18, 2024

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

@@ -87,27 +60,6 @@ def test_create_from_objects(
)


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
@@ -2,9 +2,9 @@ name: LBox Develop

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