Skip to content
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

SDK/CLI: improve mask support in the auto-annotation functionality #8724

Merged
merged 6 commits into from
Nov 25, 2024

Conversation

SpecLad
Copy link
Contributor

@SpecLad SpecLad commented Nov 20, 2024

Motivation and context

While it is already possible to output mask shapes from AA functions, which the driver will accept, it's not convenient to do so. Improve the practicalities of it in several ways:

  • Add mask and polygon helpers to the interface module.

  • Add a helper function to encode masks into the format CVAT expects.

  • Add a built-in torchvision-based instance segmentation function.

  • Add an equivalent of the conv_mask_to_poly parameter for Nuclio functions.

Add another extra for the masks module, because NumPy is a fairly beefy dependency that most SDK users probably will not need (and conversely, I don't think we can implement encode_mask efficiently without using NumPy).

How has this been tested?

Manual and unit tests.

Checklist

  • I submit my changes into the develop branch
  • I have created a changelog fragment
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • [ ] I have linked related issues (see GitHub docs)
  • [ ] I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced new auto-annotation functions for instance segmentation and keypoint detection.
    • Added support for converting mask shapes to polygon shapes during auto-annotation.
    • New helper functions (mask, polygon, encode_mask) to enhance SDK functionality.
  • Bug Fixes

    • Improved error handling for mask shape conversions in the annotation process.
  • Documentation

    • Updated installation instructions to include the new masks extra.
    • Enhanced documentation for auto-annotation API, clarifying usage and parameters.
  • Tests

    • Expanded test coverage for auto-annotation features and the new encode_mask function.

Sorry, something went wrong.

Copy link
Contributor

coderabbitai bot commented Nov 20, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The pull request introduces significant enhancements to the CVAT SDK and CLI, focusing on auto-annotation capabilities. Key changes include the addition of a ref input parameter in workflow triggers, updates to the installation commands for the CVAT SDK to include a new masks extra, and the introduction of new helper functions for mask handling. The auto-annotation process has been expanded with new classes and methods for instance segmentation and keypoint detection, along with comprehensive testing improvements to ensure functionality and error handling.

Changes

File Path Change Summary
.github/workflows/full.yml Added required input parameter ref and environment variable CVAT_VERSION. Updated SDK installation command.
.github/workflows/main.yml Introduced publish_dev_images job; updated SDK installation command in unit_testing.
changelog.d/20241120_143739_roman_aa_masks.md Added new helper functions for auto-annotation; introduced torchvision_instance_segmentation function.
cvat-cli/src/cvat_cli/_internal/commands.py Added --conv-mask-to-poly argument to TaskAutoAnnotate; updated execute method to include this parameter.
cvat-sdk/README.md Updated installation instructions to include masks extra.
cvat-sdk/cvat_sdk/auto_annotation/__init__.py Added mask and polygon to the public interface.
cvat-sdk/cvat_sdk/auto_annotation/driver.py Introduced conv_mask_to_poly parameter in _AnnotationMapper and annotate_task.
cvat-sdk/cvat_sdk/auto_annotation/functions/_torchvision.py Added TorchvisionFunction class for model handling.
cvat-sdk/cvat_sdk/auto_annotation/functions/torchvision_detection.py Introduced _TorchvisionDetectionFunction class, removed constructor.
cvat-sdk/cvat_sdk/auto_annotation/functions/torchvision_instance_segmentation.py Added _TorchvisionInstanceSegmentationFunction class for instance segmentation.
cvat-sdk/cvat_sdk/auto_annotation/functions/torchvision_keypoint_detection.py Introduced _TorchvisionKeypointDetectionFunction class.
cvat-sdk/cvat_sdk/auto_annotation/interface.py Added conv_mask_to_poly property and new helper functions polygon and mask.
cvat-sdk/cvat_sdk/masks.py Introduced encode_mask function for mask encoding.
site/content/en/docs/api_sdk/sdk/_index.md Updated installation instructions to reflect changes in SDK components.
site/content/en/docs/api_sdk/sdk/auto-annotation.md Enhanced documentation for auto-annotation functions and clarified usage.
tests/python/cli/cmtp_function.py Introduced a detection function for testing auto-annotation.
tests/python/cli/test_cli.py Added test_auto_annotate_with_cmtp to test CLI functionality.
tests/python/sdk/test_auto_annotation.py Added tests for conv_mask_to_poly and instance segmentation behavior.
tests/python/sdk/test_masks.py Established a test suite for encode_mask function, covering valid and invalid cases.

Poem

In the fields of code we hop,
New features bloom, we won't stop!
Masks and polygons, oh what fun,
Auto-annotations for everyone!
With tests to guard our joyful play,
Let's code and create, hip-hip-hooray! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@SpecLad SpecLad marked this pull request as ready for review November 20, 2024 12:46
While it is already possible to output mask shapes from AA functions, which
the driver _will_ accept, it's not convenient to do so. Improve the
practicalities of it in several ways:

* Add `mask` and `polygon` helpers to the interface module.

* Add a helper function to encode masks into the format CVAT expects.

* Add a built-in torchvision-based instance segmentation function.

* Add an equivalent of the `conv_mask_to_poly` parameter for Nuclio
  functions.

Add another extra for the `masks` module, because NumPy is a fairly
beefy dependency that most SDK users probably will not need (and
conversely, I don't think we can implement `encode_mask` efficiently
without using NumPy).
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (28)
changelog.d/20241120_143739_roman_aa_masks.md (1)

7-9: Consider adding more details about the instance segmentation function.

While the entry is clear, it could be more informative by briefly mentioning:

  • The purpose/benefits of this function
  • Any key features or capabilities
  • The underlying torchvision model being used

Example enhancement:

- \[SDK\] Added a new built-in auto-annotation function,
  `torchvision_instance_segmentation`
+ \[SDK\] Added a new built-in auto-annotation function,
+ `torchvision_instance_segmentation`, which provides pre-trained
+ instance segmentation capabilities using torchvision's Mask R-CNN model
tests/python/cli/cmtp_function.py (1)

16-22: Add docstring and verify test data values

While the implementation is functional for testing purposes, consider:

  1. Adding a docstring to explain:

    • The purpose of this test function
    • The meaning of the returned coordinates/values
    • Why these specific test values were chosen
  2. Verify that the hardcoded values ([0, 0, 0, 1, 1, 1] and [1, 0, 0, 0, 0]) are appropriate test cases for your scenarios.

 def detect(
     context: cvataa.DetectionFunctionContext, image: PIL.Image.Image
 ) -> list[models.LabeledShapeRequest]:
+    """Test detection function that returns either a polygon or mask shape.
+    
+    Args:
+        context: Detection context containing configuration
+        image: Input image (unused in this test)
+    
+    Returns:
+        List containing either a polygon or mask shape based on conv_mask_to_poly flag
+    """
     if context.conv_mask_to_poly:
         return [cvataa.polygon(0, [0, 0, 0, 1, 1, 1])]
     else:
         return [cvataa.mask(0, [1, 0, 0, 0, 0])]
cvat-sdk/cvat_sdk/auto_annotation/functions/torchvision_detection.py (2)

Line range hint 14-24: Consider adding mask support to align with PR objectives

The current implementation only supports rectangle detection shapes (cvataa.rectangle). Since this PR aims to improve mask support in auto-annotation, consider adding mask shape support here as well.

Example addition:

    def detect(
        self, context: cvataa.DetectionFunctionContext, image: PIL.Image.Image
    ) -> list[models.LabeledShapeRequest]:
        conf_threshold = context.conf_threshold or 0
        results = self._model([self._transforms(image)])

        return [
-           cvataa.rectangle(label.item(), [x.item() for x in box])
+           cvataa.rectangle(label.item(), [x.item() for x in box])
+           if not result.get("masks") else
+           cvataa.mask(label.item(), result["masks"][i])
            for result in results
            for box, label, score in zip(result["boxes"], result["labels"], result["scores"])
            if score >= conf_threshold
        ]

Line range hint 1-27: Consider adding docstrings and type hints

The class and its method would benefit from comprehensive docstrings explaining:

  • Expected model output format
  • Shape generation logic
  • Parameter descriptions
  • Return value format

Example addition:

class _TorchvisionDetectionFunction(TorchvisionFunction):
    """Implements object detection using torchvision models.
    
    The function expects the model to return a dictionary containing 'boxes',
    'labels', and 'scores' tensors, compatible with torchvision detection models.
    """

    def detect(
        self, 
        context: cvataa.DetectionFunctionContext, 
        image: PIL.Image.Image
    ) -> list[models.LabeledShapeRequest]:
        """Detect objects in the given image.
        
        Args:
            context: Detection context containing configuration like confidence threshold
            image: Input image to process
            
        Returns:
            List of labeled shapes (rectangles) for detected objects
        """
cvat-sdk/cvat_sdk/auto_annotation/functions/_torchvision.py (2)

20-26: Add documentation and error handling for the spec property.

Consider adding property documentation and validation for the metadata categories.

     @cached_property
     def spec(self) -> cvataa.DetectionFunctionSpec:
+        """Get the detection function specification.
+
+        Returns:
+            DetectionFunctionSpec with labels from model categories
+
+        Raises:
+            ValueError: If categories are missing from weights metadata
+        """
+        if "categories" not in self._weights.meta:
+            raise ValueError("Model weights metadata does not contain categories")
+
         return cvataa.DetectionFunctionSpec(
             labels=[
                 cvataa.label_spec(cat, i) for i, cat in enumerate(self._weights.meta["categories"])
             ]
         )

1-26: Consider adding memory management and device handling.

As this class handles deep learning models, consider adding:

  1. Device management (CPU/GPU) through a device parameter
  2. Context manager support for proper resource cleanup
  3. Memory optimization through model unloading when not in use

This would improve resource utilization, especially when handling multiple models or working with limited resources.

cvat-sdk/cvat_sdk/masks.py (3)

12-22: Consider enhancing the docstring

While the function signature and current documentation are good, consider adding:

  • Return value description explaining the format of the output list
  • Example usage with sample input/output
  • Explanation of how the encoded format represents the mask

Example addition:

 def encode_mask(bitmap: ArrayLike, /, bbox: Sequence[float]) -> list[float]:
     """
     Encodes an image mask into an array of numbers suitable for the "points"
     attribute of a LabeledShapeRequest object of type "mask".

     bitmap must be a boolean array of shape (H, W), where H is the height and
     W is the width of the image that the mask applies to.

     bbox must have the form [x1, y1, x2, y2], where (0, 0) <= (x1, y1) < (x2, y2) <= (W, H).
     The mask will be limited to points between (x1, y1) and (x2, y2).
+
+    Returns:
+        A list of floats where:
+        - All elements except the last 4 represent run lengths of alternating
+          false/true values in the flattened mask
+        - The last 4 elements are the adjusted bounding box coordinates [x1, y1, x2-1, y2-1]
+
+    Example:
+        >>> bitmap = np.array([[True, True], [False, True]], dtype=bool)
+        >>> bbox = [0, 0, 2, 2]
+        >>> encode_mask(bitmap, bbox)
+        [2, 1, 1, 0, 0, 1, 1]  # Run lengths followed by bbox coordinates
     """

24-32: Consider adding stricter bbox validation

The current validation is good, but consider adding explicit length and type checking for the bbox parameter.

+    if not isinstance(bbox, Sequence) or len(bbox) != 4:
+        raise ValueError("bbox must be a sequence of 4 numbers")
+
+    if not all(isinstance(x, (int, float)) for x in bbox):
+        raise ValueError("bbox coordinates must be numbers")
+
     x1, y1 = map(math.floor, bbox[0:2])
     x2, y2 = map(math.ceil, bbox[2:4])

34-42: Add explanatory comments for the encoding logic

The implementation is efficient and correct, but the logic is complex. Consider adding comments to explain the key steps.

     flat = bitmap[y1:y2, x1:x2].ravel()

+    # Calculate indices where mask values change (0->1 or 1->0)
+    # prepend/append with opposite values to handle edge cases
     (run_indices,) = np.diff(flat, prepend=[not flat[0]], append=[not flat[-1]]).nonzero()
+
+    # Calculate run lengths based on whether the mask starts with True or False
     if flat[0]:
         run_lengths = np.diff(run_indices, prepend=[0])
     else:
         run_lengths = np.diff(run_indices)

+    # Return run lengths followed by adjusted bbox coordinates (CVAT expects 0-based inclusive coordinates)
     return run_lengths.tolist() + [x1, y1, x2 - 1, y2 - 1]
cvat-sdk/cvat_sdk/auto_annotation/functions/torchvision_keypoint_detection.py (2)

Line range hint 15-29: Add class and method documentation.

While the implementation is clean and efficient, consider adding docstrings to:

  • Explain the purpose of _TorchvisionKeypointDetectionFunction
  • Document the spec method's functionality and return value
  • Describe the structure of the generated detection spec

Line range hint 31-57: Consider improving readability of nested transformations.

While the implementation is functionally correct, the nested list comprehension with multiple zip operations might be hard to maintain. Consider breaking it down into more readable steps using intermediate variables.

Example refactor:

    def detect(
        self, context: cvataa.DetectionFunctionContext, image: PIL.Image.Image
    ) -> list[models.LabeledShapeRequest]:
        conf_threshold = context.conf_threshold or 0
        results = self._model([self._transforms(image)])
        
+        shapes = []
+        for result in results:
+            for keypoints, label, score in zip(
+                result["keypoints"], result["labels"], result["scores"]
+            ):
+                if score >= conf_threshold:
+                    elements = [
+                        cvataa.keypoint(
+                            keypoint_id,
+                            [keypoint[0].item(), keypoint[1].item()],
+                            occluded=not keypoint[2].item(),
+                        )
+                        for keypoint_id, keypoint in enumerate(keypoints)
+                    ]
+                    shapes.append(cvataa.skeleton(label.item(), elements=elements))
+        return shapes
-        return [
-            cvataa.skeleton(
-                label.item(),
-                elements=[
-                    cvataa.keypoint(
-                        keypoint_id,
-                        [keypoint[0].item(), keypoint[1].item()],
-                        occluded=not keypoint[2].item(),
-                    )
-                    for keypoint_id, keypoint in enumerate(keypoints)
-                ],
-            )
-            for result in results
-            for keypoints, label, score in zip(
-                result["keypoints"], result["labels"], result["scores"]
-            )
-            if score >= conf_threshold
-        ]
tests/python/sdk/test_masks.py (3)

20-48: Consider splitting test cases into separate methods.

While the test cases are comprehensive and well-documented, consider splitting them into separate test methods for better organization and clarity:

  • test_encode_mask_starts_with_one
  • test_encode_mask_starts_with_zero
  • test_encode_mask_full_image

This would make the test suite more maintainable and failures more specific.

-    def test_encode_mask(self):
-        bitmap = np.array(
-            [
-                np.fromstring("0 0 1 1 1 0", sep=" "),
-                np.fromstring("0 1 1 0 0 0", sep=" "),
-            ],
-            dtype=np.bool_,
-        )
-        bbox = [2.9, 0.9, 4.1, 1.1]
-        assert encode_mask(bitmap, bbox) == [0, 4, 2, 2, 0, 4, 1]
-
-        bbox = [1, 0, 5, 2]
-        assert encode_mask(bitmap, bbox) == [1, 5, 2, 1, 0, 4, 1]
-
-        bbox = [0, 0, 6, 2]
-        assert encode_mask(bitmap, bbox) == [2, 3, 2, 2, 3, 0, 0, 5, 1]
+    def setup_method(self):
+        self.bitmap = np.array(
+            [
+                np.fromstring("0 0 1 1 1 0", sep=" "),
+                np.fromstring("0 1 1 0 0 0", sep=" "),
+            ],
+            dtype=np.bool_,
+        )
+
+    def test_encode_mask_starts_with_one(self):
+        """Test case where cropped mask starts with 1"""
+        bbox = [2.9, 0.9, 4.1, 1.1]
+        assert encode_mask(self.bitmap, bbox) == [0, 4, 2, 2, 0, 4, 1]
+
+    def test_encode_mask_starts_with_zero(self):
+        """Test case where cropped mask starts with 0"""
+        bbox = [1, 0, 5, 2]
+        assert encode_mask(self.bitmap, bbox) == [1, 5, 2, 1, 0, 4, 1]
+
+    def test_encode_mask_full_image(self):
+        """Test edge case: full image mask"""
+        bbox = [0, 0, 6, 2]
+        assert encode_mask(self.bitmap, bbox) == [2, 3, 2, 2, 3, 0, 0, 5, 1]

50-66: Consider adding descriptive error messages to assertions.

While the error cases are well-covered, adding explicit error messages would make test failures more informative and easier to debug.

     def test_encode_mask_invalid(self):
         with pytest.raises(ValueError):
-            encode_mask([True], [0, 0, 1, 1])  # not 2D
+            encode_mask([True], [0, 0, 1, 1], msg="Array must be 2D")
 
         with pytest.raises(ValueError):
-            encode_mask([[1]], [0, 0, 1, 1])  # not boolean
+            encode_mask([[1]], [0, 0, 1, 1], msg="Array must be boolean")
 
         for bad_bbox in [
             [-0.1, 0, 1, 1],
             [0, -0.1, 1, 1],
             [0, 0, 1.1, 1],
             [0, 0, 1, 1.1],
             [1, 0, 0, 1],
             [0, 1, 1, 0],
         ]:
-            with pytest.raises(ValueError):
+            with pytest.raises(ValueError, match=f"Invalid bbox: {bad_bbox}"):
                 encode_mask([[True]], bad_bbox)

19-19: Consider adding more edge cases to the test suite.

The test coverage could be enhanced by adding the following test cases:

  • Empty masks (all zeros)
  • Single-pixel masks
  • Complex patterns (checkerboard, diagonal lines)
  • Large masks to test performance

Would you like me to provide example implementations for these additional test cases?

site/content/en/docs/api_sdk/sdk/_index.md (1)

45-49: Documentation needs usage examples for the masks module

While the installation instructions for the masks extra are clear, there's no corresponding import example or usage section like there is for other modules (e.g., PyTorch adapter). Consider adding a usage section:

 To use the `cvat_sdk.masks` module, request the `masks` extra:

 ```bash
 pip install "cvat-sdk[masks]"

+For the masks module:
+
+python +import cvat_sdk.masks +


</blockquote></details>
<details>
<summary>cvat-sdk/cvat_sdk/auto_annotation/interface.py (1)</summary><blockquote>

`181-194`: **Excellent addition of shape helper functions!**

The new `polygon` and `mask` helper functions are well-implemented and maintain consistency with existing helper functions. The `mask` function's documentation appropriately guides users toward the recommended `encode_mask` function.


Consider adding a code example in the docstring showing how to use `encode_mask` with this function, as this would make it even more user-friendly.

</blockquote></details>
<details>
<summary>site/content/en/docs/api_sdk/sdk/auto-annotation.md (2)</summary><blockquote>

`190-199`: **Enhance the code example comments for clarity.**

The example is good but could be more specific about the input types.

Consider updating the comments to be more explicit:

```diff
 cvataa.mask(my_label, encode_mask(
-    my_mask,  # boolean 2D array, same size as the input image
-    [x1, y1, x2, y2],  # top left and bottom right coordinates of the mask
+    my_mask,  # numpy.ndarray of bool type with shape (height, width)
+    [x1, y1, x2, y2],  # bounding box coordinates: [left, top, right, bottom]
 ))

273-280: Consider enhancing the instance segmentation documentation.

While the section effectively introduces the functionality, it could be more helpful with additional details.

Consider adding:

  1. Installation requirements (similar to torchvision_detection section)
  2. A brief example showing how to use the conv_mask_to_poly parameter:
Example usage with polygon conversion:
```python
annotate_task(<client>, <task ID>, create_torchvision(<model name>, conv_mask_to_poly=True))

</blockquote></details>
<details>
<summary>tests/python/cli/test_cli.py (1)</summary><blockquote>

`365-385`: **Add docstring and enhance test coverage**

The test method would benefit from the following improvements:
1. Add a docstring explaining the test's purpose and the significance of the `--conv-mask-to-poly` flag
2. Add assertions to verify other shape attributes remain intact after conversion
3. Consider adding edge cases (e.g., complex masks, empty masks)


Here's a suggested enhancement:

```diff
 def test_auto_annotate_with_cmtp(self, fxt_new_task: Task):
+    """
+    Test auto-annotation with mask-to-polygon conversion.
+    
+    Verifies that:
+    1. Auto-annotation produces mask shapes by default
+    2. The --conv-mask-to-poly flag successfully converts masks to polygons
+    3. Shape attributes are preserved during conversion
+    """
     self.run_cli(
         "auto-annotate",
         str(fxt_new_task.id),
         f"--function-module={__package__}.cmtp_function",
         "--clear-existing",
     )

     annotations = fxt_new_task.get_annotations()
     assert annotations.shapes[0].type.value == "mask"
+    # Store original shape attributes for comparison
+    original_label = annotations.shapes[0].label
+    original_points = annotations.shapes[0].points

     self.run_cli(
         "auto-annotate",
         str(fxt_new_task.id),
         f"--function-module={__package__}.cmtp_function",
         "--clear-existing",
         "--conv-mask-to-poly",
     )

     annotations = fxt_new_task.get_annotations()
     assert annotations.shapes[0].type.value == "polygon"
+    # Verify shape attributes are preserved
+    assert annotations.shapes[0].label == original_label
+    assert len(annotations.shapes[0].points) > 0  # Ensure valid polygon points
cvat-sdk/cvat_sdk/auto_annotation/driver.py (3)

222-226: Consider enhancing the error message for better debugging

The validation logic is well-placed and correct. Consider making the error message more informative by including the shape details.

-                    raise BadFunctionError(
-                        "function output mask shape despite conv_mask_to_poly=True"
-                    )
+                    raise BadFunctionError(
+                        f"function output mask shape (frame {shape.frame}) despite conv_mask_to_poly=True"
+                    )

246-246: Consider enhancing parameter documentation

The parameter addition and its documentation are well-implemented. Consider adding an example to illustrate the parameter's usage.

Add an example like:

"""
Example:
    # To ensure mask shapes are not allowed in the output
    annotate_task(client, task_id, detection_function, conv_mask_to_poly=True)
"""

Also applies to: 282-284


Line range hint 1-332: Well-structured implementation of mask conversion control

The implementation successfully adds mask conversion control while maintaining:

  • Clean separation of concerns
  • Proper error handling
  • Backward compatibility
  • Consistent coding style

The changes align well with the PR objectives of improving mask support in the auto-annotation functionality.

cvat-cli/src/cvat_cli/_internal/commands.py (1)

479-484: Enhance help text for --conv-mask-to-poly argument

The implementation is correct, but consider making the help text more descriptive to better explain the purpose and impact of this option. For example:

-            help="Convert mask shapes to polygon shapes",
+            help="Convert mask shapes to polygon shapes during auto-annotation. This can be useful for compatibility with annotation formats that don't support masks.",
cvat-sdk/cvat_sdk/auto_annotation/functions/torchvision_instance_segmentation.py (3)

19-21: Add a docstring to the detect method for better documentation

Adding a docstring to the detect method will enhance readability and help other developers understand its purpose, parameters, and return value.


35-37: Add a docstring to the _generate_shapes method for better documentation

Including a docstring for the _generate_shapes method will improve code maintainability and clarity.


54-54: Parameterize the tolerance value in approximate_polygon

The hardcoded tolerance=2.5 may not be optimal for all use cases. Consider making it a configurable parameter or adding a comment to explain the rationale behind this specific value.

tests/python/sdk/test_auto_annotation.py (2)

676-683: Clarify the use of mask values 0.49 and 0.5 in make_mask function

In the make_mask function, the mask is initialized with 0.49 and certain regions are set to 0.5. Using values close to the threshold may lead to unexpected results due to floating-point precision. Consider using 0 and 1 for mask values unless testing threshold behavior is intentional.


809-855: Enhance test coverage in test_torchvision_instance_segmentation

Consider adding test cases for scenarios such as empty masks, multiple labels, or varying confidence thresholds to ensure comprehensive coverage and robustness of the instance segmentation functionality.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3eec9fe and 00222de.

⛔ Files ignored due to path filters (1)
  • cvat-sdk/gen/templates/openapi-generator/setup.mustache is excluded by !**/gen/**
📒 Files selected for processing (19)
  • .github/workflows/full.yml (1 hunks)
  • .github/workflows/main.yml (1 hunks)
  • changelog.d/20241120_143739_roman_aa_masks.md (1 hunks)
  • cvat-cli/src/cvat_cli/_internal/commands.py (3 hunks)
  • cvat-sdk/README.md (1 hunks)
  • cvat-sdk/cvat_sdk/auto_annotation/__init__.py (1 hunks)
  • cvat-sdk/cvat_sdk/auto_annotation/driver.py (6 hunks)
  • cvat-sdk/cvat_sdk/auto_annotation/functions/_torchvision.py (1 hunks)
  • cvat-sdk/cvat_sdk/auto_annotation/functions/torchvision_detection.py (1 hunks)
  • cvat-sdk/cvat_sdk/auto_annotation/functions/torchvision_instance_segmentation.py (1 hunks)
  • cvat-sdk/cvat_sdk/auto_annotation/functions/torchvision_keypoint_detection.py (1 hunks)
  • cvat-sdk/cvat_sdk/auto_annotation/interface.py (2 hunks)
  • cvat-sdk/cvat_sdk/masks.py (1 hunks)
  • site/content/en/docs/api_sdk/sdk/_index.md (1 hunks)
  • site/content/en/docs/api_sdk/sdk/auto-annotation.md (2 hunks)
  • tests/python/cli/cmtp_function.py (1 hunks)
  • tests/python/cli/test_cli.py (1 hunks)
  • tests/python/sdk/test_auto_annotation.py (4 hunks)
  • tests/python/sdk/test_masks.py (1 hunks)
🔇 Additional comments (24)
changelog.d/20241120_143739_roman_aa_masks.md (1)

1-13: LGTM! Well-structured changelog entries.

The changelog follows proper formatting, with clear categorization and consistent style. Each entry is properly linked to the PR for traceability.

tests/python/cli/cmtp_function.py (2)

1-8: LGTM! Proper license header and focused imports

The file has appropriate copyright notice, license declaration, and imports only the necessary modules.


9-13: Verify the label ID assignment

The label ID of 0 for "car" is valid, but please verify that this aligns with the test requirements and expectations in the broader test suite.

cvat-sdk/cvat_sdk/auto_annotation/__init__.py (2)

13-14: LGTM! Import statements align with the PR objectives.

The addition of mask and polygon imports supports the new mask handling functionality while maintaining clean code organization.


30-31: LGTM! Proper public API exposure.

The new mask and polygon functions are correctly exposed in the module's public interface through __all__, following Python best practices for API design.

cvat-sdk/cvat_sdk/auto_annotation/functions/torchvision_detection.py (2)

13-13: Verify the impact of removed initialization method

The class no longer has an explicit __init__ method for configuring the model name and weights. Ensure that the parent class TorchvisionFunction properly handles these configurations.


Line range hint 27-27: LGTM: Factory pattern implementation

The assignment of the class to create follows the factory pattern, making it easy to instantiate detection functions.

cvat-sdk/cvat_sdk/auto_annotation/functions/_torchvision.py (1)

1-10: LGTM! Well-structured imports and proper licensing.

The file header is properly licensed and the imports are well-organized, following Python best practices.

cvat-sdk/README.md (2)

23-27: LGTM! Clear installation instructions for the new masks module.

The addition of the masks installation instructions is well-documented and follows the same format as other package extras.


29-30: LGTM! Clear separation of PyTorch-related functionality.

The modified text effectively distinguishes between the PyTorch adapter and auto-annotation functions, making it clearer for users to understand when they need the pytorch extra.

cvat-sdk/cvat_sdk/masks.py (1)

1-10: LGTM! Well-structured header with appropriate imports

The file has proper licensing, minimal necessary imports, and good use of type hints.

cvat-sdk/cvat_sdk/auto_annotation/functions/torchvision_keypoint_detection.py (2)

12-12: LGTM! Clean import structure and proper inheritance.

The code follows Python best practices with clear imports and proper base class inheritance.


Line range hint 60-60: LGTM! Clean factory pattern implementation.

The assignment follows the expected pattern for creating factory functions.

tests/python/sdk/test_masks.py (1)

7-16: LGTM! Well-structured error handling.

The code gracefully handles the case when NumPy is not installed, allowing the tests to be skipped rather than fail. This is a good practice for optional dependencies.

cvat-sdk/cvat_sdk/auto_annotation/interface.py (1)

71-80: Well-structured abstract property addition!

The conv_mask_to_poly property is well-documented and follows proper abstract method patterns. The documentation clearly explains its purpose and when implementations can ignore this value.

site/content/en/docs/api_sdk/sdk/auto-annotation.md (1)

184-185: LGTM! Well-documented helper functions.

The new mask and polygon helper functions are properly documented in the table, maintaining consistency with the existing format.

.github/workflows/full.yml (1)

159-159: Verify SDK installation configuration across all test jobs

The addition of the masks extra to the SDK installation aligns with the PR objectives to enhance mask support in auto-annotation. However, please verify if the masks extra should also be added to SDK installations in other jobs (e.g., unit_testing, e2e_testing) for consistency.

cvat-sdk/cvat_sdk/auto_annotation/driver.py (3)

102-102: LGTM: Constructor parameter addition is clean and well-integrated

The new conv_mask_to_poly parameter is properly added to the constructor and stored as an instance variable, maintaining the class's initialization pattern.

Also applies to: 106-106


234-234: LGTM: Context class modification maintains backward compatibility

The addition of conv_mask_to_poly with a default value of False ensures backward compatibility while extending the functionality.


311-313: LGTM: Context initialization properly includes new parameter

The context initialization correctly passes through all parameters, maintaining the existing pattern.

.github/workflows/main.yml (2)

169-169: LGTM: SDK installation updated to include masks support

The addition of the masks extra to the SDK installation command aligns with the PR objectives to improve mask support in the auto-annotation functionality.


Line range hint 391-449: LGTM: Well-structured publish job for development images

The new publish_dev_images job is properly configured with:

  • Appropriate conditional execution for the develop branch
  • Correct dependency chain on test jobs
  • Secure handling of Docker Hub credentials using secrets
  • Clear image tagging and pushing process
cvat-cli/src/cvat_cli/_internal/commands.py (1)

496-496: LGTM: Parameter handling is implemented correctly

The conv_mask_to_poly parameter is properly integrated into the method signature and correctly passed to the underlying SDK function.

Also applies to: 522-522

tests/python/sdk/test_auto_annotation.py (1)

311-343: Verify that received_cmtp is correctly set when an exception is raised

In the test_conv_mask_to_poly method, after expecting a BadFunctionError, you assert that received_cmtp is True. Please ensure that the detect function is called and received_cmtp is set before the exception is raised. Otherwise, the assertion may fail if the exception occurs before received_cmtp is assigned.

@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 15.38462% with 66 lines in your changes missing coverage. Please review.

Project coverage is 74.15%. Comparing base (3eec9fe) to head (794e8ae).
Report is 4 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8724      +/-   ##
===========================================
- Coverage    74.18%   74.15%   -0.03%     
===========================================
  Files          401      404       +3     
  Lines        43510    43571      +61     
  Branches      3950     3953       +3     
===========================================
+ Hits         32278    32312      +34     
- Misses       11232    11259      +27     
Components Coverage Δ
cvat-ui 78.59% <ø> (+0.09%) ⬆️
cvat-server 70.37% <15.38%> (-0.13%) ⬇️
---- 🚨 Try these New Features:

@SpecLad SpecLad requested a review from zhiltsov-max November 21, 2024 10:02
@SpecLad SpecLad merged commit 3265e1f into cvat-ai:develop Nov 25, 2024
33 checks passed
@SpecLad SpecLad deleted the aa-masks branch November 25, 2024 12:53
@cvat-bot cvat-bot bot mentioned this pull request Nov 29, 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.

None yet

3 participants