Fix task creation with gt_pool validation and cloud storage data
#8539
Conversation
WalkthroughThe changes primarily focus on enhancing task creation and validation processes within the CVAT application. Modifications include new validation steps for task parameters, improvements in manifest file handling, and updates to segment creation logic. The testing suite has also been enhanced with new test cases and improved error handling, ensuring comprehensive coverage of task management features. Additionally, function signatures and type annotations have been refined across several files for better clarity and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TaskManager
participant CloudStorage
participant ManifestHandler
User->>TaskManager: Create Task
TaskManager->>TaskManager: Validate Parameters
TaskManager->>CloudStorage: Check Job File Mapping
CloudStorage-->>TaskManager: Return Mapping
TaskManager->>ManifestHandler: Handle Manifest Files
ManifestHandler-->>TaskManager: Return Processed Manifest
TaskManager->>User: Task Created Successfully
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
utils/dataset_manifest/core.py (2)
730-744: New methodreorderlooks good, but could benefit from some improvements.The new
reordermethod in theImageManifestManagerclass is a good addition for reordering images based on a provided list. However, there are a few points to consider:
- The method handles potential duplicates in the reordered list, which is good.
- Error handling is implemented for missing images, which is appropriate.
Suggestions for improvement:
- Consider adding type hints for better code readability and maintainability.
- The method could benefit from some inline comments explaining the logic, especially for handling duplicates.
- Consider adding a check for an empty
reordered_imageslist to avoid unnecessary processing.Here's a suggested improvement with type hints and comments:
- def reorder(self, reordered_images: List[str]) -> None: + def reorder(self, reordered_images: List[str]) -> None: """ The method takes a list of image names and reorders its content based on this new list. Due to the implementation of Honeypots, the reordered list of image names may contain duplicates. """ + if not reordered_images: + raise ValueError("The reordered_images list cannot be empty") + + # Create a dictionary of unique images from the current manifest unique_images: Dict[str, Any] = {} for _, image_details in self: if image_details.full_name not in unique_images: unique_images[image_details.full_name] = image_details try: + # Create a new manifest with the reordered images, handling potential duplicates self.create(content=(unique_images[x] for x in reordered_images)) except KeyError as ex: raise InvalidManifestError(f"Previous manifest does not contain {ex} image")These changes improve the method's robustness and readability.
Line range hint
1-744: Consider refactoring for improved modularity and testability.While the new
reordermethod is the only change in this file, there are some general observations and suggestions for the overall codebase:
The file is quite long and contains multiple classes and functions. Consider splitting it into smaller, more focused modules for better maintainability.
Some classes, like
_ManifestManager,VideoManifestManager, andImageManifestManager, could benefit from more extensive use of type hints throughout their methods.The error handling could be more consistent across the file. Consider creating custom exception classes for different types of errors that can occur during manifest operations.
Some methods, like
createinImageManifestManager, are quite long and could potentially be split into smaller, more focused methods.Consider adding more unit tests, especially for edge cases and error conditions, to ensure the robustness of the manifest management system.
To improve the overall structure, consider the following refactoring steps:
- Split the file into separate modules, e.g.,
base_manifest.py,video_manifest.py,image_manifest.py, andvalidators.py.- Create a custom exceptions module, e.g.,
manifest_exceptions.py, to centralize all custom exceptions.- Implement more comprehensive logging throughout the classes to aid in debugging and monitoring.
- Review and update the docstrings for all classes and methods to ensure they are up-to-date and follow a consistent format (e.g., Google style or NumPy style).
These changes would significantly improve the maintainability and readability of the codebase.
tests/python/shared/utils/helpers.py (1)
Line range hint
32-38: Add assertion to ensuresizeslength matchescountCurrently, there is no assertion to check that when
sizesis provided, its length matchescount. This could lead to anIndexErrorifsizesis shorter thancount.Consider adding the following assertion:
assert not (prefixes and filenames), "prefixes cannot be used together with filenames" assert not prefixes or len(prefixes) == count assert not filenames or len(filenames) == count +assert not sizes or len(sizes) == count
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- cvat/apps/engine/task.py (1 hunks)
- tests/python/rest_api/test_tasks.py (7 hunks)
- tests/python/shared/utils/helpers.py (3 hunks)
- utils/dataset_manifest/core.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
cvat/apps/engine/task.py (1)
1327-1328: Manifest Reordering Implemented CorrectlyThe manifest is properly updated to reflect the new image order based on the frame index mapping in
new_db_images.tests/python/rest_api/test_tasks.py (4)
35-35: Importing 'Iterable' is appropriateThe addition of
Iterableto the imports enhances type hinting and improves code clarity.
Line range hint
1536-1616: Enhancement of_create_task_with_cloud_datawith additional parametersThe method
_create_task_with_cloud_datanow includesorg,filenames,task_spec_kwargs, anddata_spec_kwargsas parameters. This extension increases the flexibility and reusability of the method by allowing customization of task and data specifications.
1991-1993: Passingdata_spec_kwargsanddata_typecorrectlyThe inclusion of
data_spec_kwargsanddata_typewhen calling_create_task_with_cloud_dataensures that additional data specifications and data types are accurately passed, enhancing the method's versatility.
2558-2634: Well-implemented test for task creation with validation and cloud dataThe
test_can_create_task_with_validation_and_cloud_datamethod effectively tests task creation with different validation modes and cloud storage data. The assertions verify that the GT job is created and that job metadata corresponds to the chunk data.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8539 +/- ##
========================================
Coverage 74.26% 74.27%
========================================
Files 400 400
Lines 43218 43222 +4
Branches 3909 3909
========================================
+ Hits 32096 32103 +7
+ Misses 11122 11119 -3
|
Marishka17
requested review from
Bobronium and
bsekachev
and removed request for
azhavoro
|
|
Failed checks ( |
Motivation and context
How has this been tested?
Added corresponding REST API test
Checklist
developbranch- [ ] I have updated the documentation accordingly- [ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores