-
Notifications
You must be signed in to change notification settings - Fork 15
chore: ED-1722 Add typed dicts #1034
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
Summary of ChangesHello @clinton-encord, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the SDK's data handling by introducing and consistently applying Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Unit test report (Python 3.9.24, Pydantic 1.10.22)272 tests 272 ✅ 7s ⏱️ Results for commit f7908aa. ♻️ This comment has been updated with latest results. |
Unit test report (Python 3.9.24, Pydantic 2.12.3)272 tests 272 ✅ 8s ⏱️ Results for commit f7908aa. ♻️ This comment has been updated with latest results. |
| confidence=d["confidence"], | ||
| manual_annotation=d.get("manualAnnotation", True), | ||
| reviews=d.get("reviews"), | ||
| reviews=None, # To be deprecated. Always set to None. |
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.
If you look at LabelRowV2._to_encord_object, this is never sent out anyways.
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.
Code Review
This pull request introduces typed dictionaries across the SDK, which is a significant step towards improving type safety and code clarity. The changes are comprehensive and well-executed. I've identified a few areas for improvement, primarily concerning code duplication and minor simplifications, which I've detailed in the review comments. Overall, this is a valuable contribution to the codebase.
encord/objects/coordinates.py
Outdated
| def get_coordinates_from_frame_object_dict(frame_object_dict: FrameObject) -> Coordinates: | ||
| if frame_object_dict["shape"] == Shape.BOUNDING_BOX: | ||
| return BoundingBoxCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.ROTATABLE_BOUNDING_BOX: | ||
| return RotatableBoundingBoxCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.POLYGON: | ||
| return PolygonCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.POINT: | ||
| coords = frame_object_dict["point"]["0"] | ||
| if "x" in coords and "y" in coords and "z" in coords: | ||
| return PointCoordinate3D.from_dict(frame_object_dict) # type: ignore | ||
| elif "x" in coords and "y" in coords: | ||
| return PointCoordinate.from_dict(frame_object_dict) # type: ignore | ||
| else: | ||
| raise ValueError(f"Invalid point coordinates in {frame_object_dict}") | ||
| elif frame_object_dict["shape"] == Shape.POLYLINE: | ||
| return PolylineCoordinates.from_dict(frame_object_dict) | ||
| elif "skeleton" in frame_object_dict: | ||
|
|
||
| def _with_visibility_enum(point: dict): | ||
| if point.get(Visibility.INVISIBLE.value): | ||
| point["visibility"] = Visibility.INVISIBLE | ||
| elif point.get(Visibility.OCCLUDED.value): | ||
| point["visibility"] = Visibility.OCCLUDED | ||
| elif point.get(Visibility.SELF_OCCLUDED.value): | ||
| point["visibility"] = Visibility.SELF_OCCLUDED | ||
| elif point.get(Visibility.VISIBLE.value): | ||
| point["visibility"] = Visibility.VISIBLE | ||
| return point | ||
|
|
||
| values = [_with_visibility_enum(pnt) for pnt in frame_object_dict["skeleton"].values()] | ||
| skeleton_frame_object_label = { | ||
| "name": frame_object_dict["name"], | ||
| "values": values, | ||
| } | ||
| return SkeletonCoordinates.from_dict(skeleton_frame_object_label) | ||
| elif "bitmask" in frame_object_dict: | ||
| return BitmaskCoordinates.from_dict(frame_object_dict) | ||
| elif "cuboid" in frame_object_dict: | ||
| return CuboidCoordinates.from_dict(frame_object_dict) | ||
| else: | ||
| raise NotImplementedError(f"Getting coordinates for `{frame_object_dict}` is not supported yet.") | ||
|
|
||
|
|
||
| def get_geometric_coordinates_from_frame_object_dict( | ||
| frame_object_dict: FrameObject, | ||
| ) -> GeometricCoordinates: | ||
| if frame_object_dict["shape"] == Shape.BOUNDING_BOX: | ||
| return BoundingBoxCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.ROTATABLE_BOUNDING_BOX: | ||
| return RotatableBoundingBoxCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.POLYGON: | ||
| return PolygonCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.POINT: | ||
| coords = frame_object_dict["point"]["0"] | ||
| if "x" in coords and "y" in coords and "z" in coords: | ||
| return PointCoordinate3D.from_dict(frame_object_dict) # type: ignore | ||
| elif "x" in coords and "y" in coords: | ||
| return PointCoordinate.from_dict(frame_object_dict) # type: ignore | ||
| else: | ||
| raise ValueError(f"Invalid point coordinates in {frame_object_dict}") | ||
| elif frame_object_dict["shape"] == Shape.POLYLINE: | ||
| return PolylineCoordinates.from_dict(frame_object_dict) | ||
| elif "skeleton" in frame_object_dict: | ||
|
|
||
| def _with_visibility_enum(point: dict): | ||
| if point.get(Visibility.INVISIBLE.value): | ||
| point["visibility"] = Visibility.INVISIBLE | ||
| elif point.get(Visibility.OCCLUDED.value): | ||
| point["visibility"] = Visibility.OCCLUDED | ||
| elif point.get(Visibility.SELF_OCCLUDED.value): | ||
| point["visibility"] = Visibility.SELF_OCCLUDED | ||
| elif point.get(Visibility.VISIBLE.value): | ||
| point["visibility"] = Visibility.VISIBLE | ||
| return point | ||
|
|
||
| values = [_with_visibility_enum(pnt) for pnt in frame_object_dict["skeleton"].values()] | ||
| skeleton_frame_object_label = { | ||
| "name": frame_object_dict["name"], | ||
| "values": values, | ||
| } | ||
| return SkeletonCoordinates.from_dict(skeleton_frame_object_label) | ||
| elif "bitmask" in frame_object_dict: | ||
| return BitmaskCoordinates.from_dict(frame_object_dict) | ||
| elif "cuboid" in frame_object_dict: | ||
| raise NotImplementedError("Cuboid is not a two dimensional coordinate.") | ||
| else: | ||
| raise NotImplementedError(f"Getting coordinates for `{frame_object_dict}` is not supported yet.") |
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.
There is significant code duplication between get_coordinates_from_frame_object_dict and get_geometric_coordinates_from_frame_object_dict. This can be refactored to improve maintainability. get_geometric_coordinates_from_frame_object_dict could call get_coordinates_from_frame_object_dict and then perform its specific checks.
For example:
def get_geometric_coordinates_from_frame_object_dict(
frame_object_dict: FrameObject,
) -> GeometricCoordinates:
coordinates = get_coordinates_from_frame_object_dict(frame_object_dict)
if isinstance(coordinates, CuboidCoordinates):
raise NotImplementedError("Cuboid is not a two dimensional coordinate.")
# The cast is safe because we've excluded the non-geometric types.
return cast(GeometricCoordinates, coordinates)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.
@clinton-encord sounds like a good idea?
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.
definitely!
| for answer_dict in answers_list: | ||
| feature_hashes: Set[str] = {answer["featureHash"] for answer in answer_dict["answers"]} | ||
| all_feature_hashes.update(feature_hashes) | ||
| for frame_range in ranges_list_to_ranges(answer_dict["range"]): | ||
| ranges.append((frame_range, feature_hashes)) |
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.
This seems to me like a bug. Answers could also be text, but here we seem to assume its a list?
SDK integration test report285 tests ±0 277 ✅ ±0 12m 37s ⏱️ - 5m 35s For more details on these failures, see this check. Results for commit f7908aa. ± Comparison against base commit dbabc87. ♻️ This comment has been updated with latest results. |
encord/objects/coordinates.py
Outdated
|
|
||
| @staticmethod | ||
| def from_dict(d: Dict) -> "PolygonCoordinates": | ||
| def from_dict(d: PolygonFrameObject) -> "PolygonCoordinates": |
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.
technically this is a public method, so I think we should narrow the type more here. If I do
PolygonCoordinates.from_dict({"polygon": {"0": {"x": 0, "y": 0}}}) then it'll complain that featureHash is missing; but I don't care, I just care about the coordinates. So it should probably just be that?
class PolygonCoordsDict(TypedDict):
polygon: LegacyPolygonDict
polygons: Optional[PolygonDict]
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.
Oh true! Gotcha!
encord/objects/coordinates.py
Outdated
| return [coord for point in coordinates for coord in [point.x, point.y]] | ||
|
|
||
|
|
||
| PolylineDict = Union[Dict[str, PointDict], list[PointDict], Dict[str, PointDict3D], list[PointDict3D]] |
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.
nit: can we put all the coordinates dict types together at the top?
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.
Done!
encord/objects/coordinates.py
Outdated
| def get_coordinates_from_frame_object_dict(frame_object_dict: FrameObject) -> Coordinates: | ||
| if frame_object_dict["shape"] == Shape.BOUNDING_BOX: | ||
| return BoundingBoxCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.ROTATABLE_BOUNDING_BOX: | ||
| return RotatableBoundingBoxCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.POLYGON: | ||
| return PolygonCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.POINT: | ||
| coords = frame_object_dict["point"]["0"] | ||
| if "x" in coords and "y" in coords and "z" in coords: | ||
| return PointCoordinate3D.from_dict(frame_object_dict) # type: ignore | ||
| elif "x" in coords and "y" in coords: | ||
| return PointCoordinate.from_dict(frame_object_dict) # type: ignore | ||
| else: | ||
| raise ValueError(f"Invalid point coordinates in {frame_object_dict}") | ||
| elif frame_object_dict["shape"] == Shape.POLYLINE: | ||
| return PolylineCoordinates.from_dict(frame_object_dict) | ||
| elif "skeleton" in frame_object_dict: | ||
|
|
||
| def _with_visibility_enum(point: dict): | ||
| if point.get(Visibility.INVISIBLE.value): | ||
| point["visibility"] = Visibility.INVISIBLE | ||
| elif point.get(Visibility.OCCLUDED.value): | ||
| point["visibility"] = Visibility.OCCLUDED | ||
| elif point.get(Visibility.SELF_OCCLUDED.value): | ||
| point["visibility"] = Visibility.SELF_OCCLUDED | ||
| elif point.get(Visibility.VISIBLE.value): | ||
| point["visibility"] = Visibility.VISIBLE | ||
| return point | ||
|
|
||
| values = [_with_visibility_enum(pnt) for pnt in frame_object_dict["skeleton"].values()] | ||
| skeleton_frame_object_label = { | ||
| "name": frame_object_dict["name"], | ||
| "values": values, | ||
| } | ||
| return SkeletonCoordinates.from_dict(skeleton_frame_object_label) | ||
| elif "bitmask" in frame_object_dict: | ||
| return BitmaskCoordinates.from_dict(frame_object_dict) | ||
| elif "cuboid" in frame_object_dict: | ||
| return CuboidCoordinates.from_dict(frame_object_dict) | ||
| else: | ||
| raise NotImplementedError(f"Getting coordinates for `{frame_object_dict}` is not supported yet.") | ||
|
|
||
|
|
||
| def get_geometric_coordinates_from_frame_object_dict( | ||
| frame_object_dict: FrameObject, | ||
| ) -> GeometricCoordinates: | ||
| if frame_object_dict["shape"] == Shape.BOUNDING_BOX: | ||
| return BoundingBoxCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.ROTATABLE_BOUNDING_BOX: | ||
| return RotatableBoundingBoxCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.POLYGON: | ||
| return PolygonCoordinates.from_dict(frame_object_dict) | ||
| elif frame_object_dict["shape"] == Shape.POINT: | ||
| coords = frame_object_dict["point"]["0"] | ||
| if "x" in coords and "y" in coords and "z" in coords: | ||
| return PointCoordinate3D.from_dict(frame_object_dict) # type: ignore | ||
| elif "x" in coords and "y" in coords: | ||
| return PointCoordinate.from_dict(frame_object_dict) # type: ignore | ||
| else: | ||
| raise ValueError(f"Invalid point coordinates in {frame_object_dict}") | ||
| elif frame_object_dict["shape"] == Shape.POLYLINE: | ||
| return PolylineCoordinates.from_dict(frame_object_dict) | ||
| elif "skeleton" in frame_object_dict: | ||
|
|
||
| def _with_visibility_enum(point: dict): | ||
| if point.get(Visibility.INVISIBLE.value): | ||
| point["visibility"] = Visibility.INVISIBLE | ||
| elif point.get(Visibility.OCCLUDED.value): | ||
| point["visibility"] = Visibility.OCCLUDED | ||
| elif point.get(Visibility.SELF_OCCLUDED.value): | ||
| point["visibility"] = Visibility.SELF_OCCLUDED | ||
| elif point.get(Visibility.VISIBLE.value): | ||
| point["visibility"] = Visibility.VISIBLE | ||
| return point | ||
|
|
||
| values = [_with_visibility_enum(pnt) for pnt in frame_object_dict["skeleton"].values()] | ||
| skeleton_frame_object_label = { | ||
| "name": frame_object_dict["name"], | ||
| "values": values, | ||
| } | ||
| return SkeletonCoordinates.from_dict(skeleton_frame_object_label) | ||
| elif "bitmask" in frame_object_dict: | ||
| return BitmaskCoordinates.from_dict(frame_object_dict) | ||
| elif "cuboid" in frame_object_dict: | ||
| raise NotImplementedError("Cuboid is not a two dimensional coordinate.") | ||
| else: | ||
| raise NotImplementedError(f"Getting coordinates for `{frame_object_dict}` is not supported yet.") |
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.
@clinton-encord sounds like a good idea?
Introduction and Explanation
Part of preparatory work for adding spaces. Add more types to SDK.
JIRA
Link ticket(s)
Documentation
There should be enough internal documentation for a product owner to write customer-facing documentation or a separate PR linked if writing the customer documentation directly. Link all that are relevant below.
Tests
Make a quick statement and post any relevant links of CI / test results. If the testing infrastructure isn’t yet in-place, note that instead.
Known issues
If there are any known issues with the solution, make a statement about what they are and why they are Ok to leave unsolved for now. Make tickets for the known issues linked to the original ticket linked above