Remove task_subsets from ProjectWriteSerializer
#8492
Conversation
WalkthroughThe changes involve the removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Serializer
Client->>API: Create/Update Project (without task_subsets)
API->>Serializer: Validate Data
Serializer-->>API: Data Validated
API-->>Client: Response with Project Details
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 (
|
This is a generated field, so it makes no sense to write it. In practice, attempting to do so when creating a project causes a crash in `ProjectWriteSerializer.create`, while doing it when updating a project has no effect.
c77a537 to
84fdcbb
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
changelog.d/20241001_131450_roman.md (1)
1-5: LGTM with minor suggestions for improvement.The changelog entry accurately reflects the changes described in the PR objectives. It's concise, clear, and provides a useful reference to the PR.
To make it even more informative, consider:
- Briefly mentioning the motivation (e.g., "to prevent crashes and remove ineffective code").
- Specifying the affected API or module (e.g., "in the ProjectWriteSerializer").
These additions would provide more context for users and developers reading the changelog.
cvat/apps/engine/serializers.py (4)
Line range hint
1387-1393: Consider adding docstring for better documentationWhile the class is well-structured, adding a docstring to explain its purpose and any important details would improve documentation and maintainability.
Line range hint
1455-1460: Consider adding error handling for label updatesWhile the update_child_objects_on_labels_update method is called when labels are updated, it might be beneficial to add some error handling or logging in case of any issues during the update process.
Line range hint
1517-1522: Consider deprecating the compatibility methodThe get_has_related_context method is used for compatibility with version 2.3.0. Consider adding a deprecation warning and plan to remove this in future versions to maintain clean code.
import warnings @extend_schema_field(serializers.BooleanField) def get_has_related_context(self, obj: dict) -> bool: warnings.warn("get_has_related_context is deprecated and will be removed in future versions", DeprecationWarning) return obj['related_files'] != 0
Line range hint
1568-1571: Consider adding type checking in to_internal_valueThe to_internal_value method currently converts all values to strings. Consider adding type checking to ensure that only appropriate types are converted, preventing potential issues with unexpected input types.
def to_internal_value(self, data): if not isinstance(data['value'], (str, int, float, bool)): raise serializers.ValidationError("Invalid type for 'value'") data['value'] = str(data['value']) return super().to_internal_value(data)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- changelog.d/20241001_131450_roman.md (1 hunks)
- cvat/apps/engine/serializers.py (1 hunks)
- cvat/schema.yml (0 hunks)
💤 Files with no reviewable changes (1)
- cvat/schema.yml
🔇 Additional comments (12)
cvat/apps/engine/serializers.py (12)
Line range hint
1381-1385: LGTM: Field definitions are appropriateThe field definitions for ProjectWriteSerializer look good. They include necessary fields like labels, owner_id, assignee_id, and storage-related fields. The use of write_only=True for sensitive fields is a good practice.
Line range hint
1395-1423: LGTM: Create method is well-implementedThe create method is properly implemented with the following good practices:
- Use of @transaction.atomic for data consistency.
- Proper handling of label creation.
- Configuring source/target storages for import/export.
- Setting assignee_updated_date when an assignee is provided.
Line range hint
1425-1453: LGTM: Update method handles necessary changesThe update method correctly handles updates to various fields, including labels and assignee. The use of @transaction.atomic ensures data consistency during updates.
Line range hint
1462-1503: LGTM: Validate method ensures proper label mappingThe validate method does a good job of ensuring that all labels can be mapped when moving a project. It checks for both top-level labels and sublabels, which is crucial for maintaining data integrity.
Line range hint
1505-1509: LGTM: AboutSerializer is concise and appropriateThe AboutSerializer is well-defined with appropriate fields and max_lengths for name, description, and version. It's a simple and effective serializer for its purpose.
Line range hint
1511-1515: LGTM: FrameMetaSerializer structure is appropriateThe FrameMetaSerializer is well-structured with relevant fields for frame metadata. The use of IntegerField for width, height, and related_files is appropriate.
Line range hint
1524-1529: LGTM: PluginsSerializer is concise and clearThe PluginsSerializer is well-defined, using boolean fields to represent the status of various plugins. This structure is simple and effective for its purpose.
Line range hint
1531-1556: LGTM: DataMetaReadSerializer is comprehensive and well-structuredThe DataMetaReadSerializer provides a thorough representation of data metadata. Notable points:
- Appropriate use of FrameMetaSerializer for the 'frames' field.
- Clear constraints on fields like 'image_quality'.
- Helpful explanation for 'included_frames' using help_text.
The structure effectively captures all necessary metadata for the Data model.
Line range hint
1549-1553: Good use of help_text for clarityThe custom help_text for the 'size' field provides clear explanation of what the field represents, which is particularly helpful for API users.
Line range hint
1558-1562: LGTM: DataMetaWriteSerializer is appropriately minimalThe DataMetaWriteSerializer is correctly designed to only allow modification of the 'deleted_frames' field. This approach limits the risk of unintended changes to other metadata fields during write operations.
Line range hint
1564-1566: LGTM: AttributeValSerializer structure is appropriateThe AttributeValSerializer correctly defines the necessary fields 'spec_id' and 'value' with appropriate types and constraints.
Line range hint
1-2077: Overall, the serializers are well-implemented with minor improvement opportunitiesAfter reviewing several key serializers in this file, the overall implementation appears to be solid and well-structured. The serializers appropriately handle various aspects of the CVAT system, from projects and tasks to data metadata and attributes.
Some minor suggestions for improvement include:
- Adding docstrings to complex serializers for better documentation.
- Considering deprecation of compatibility methods.
- Enhancing type checking in certain methods.
These small changes could further improve the code's maintainability and robustness. Great job on the implementation overall!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8492 +/- ##
===========================================
+ Coverage 74.34% 74.35% +0.01%
===========================================
Files 394 394
Lines 42216 42211 -5
Branches 3896 3896
===========================================
+ Hits 31387 31388 +1
+ Misses 10829 10823 -6
|
Motivation and context
This is a generated field, so it makes no sense to write it. In practice, attempting to do so when creating a project causes a crash in
ProjectWriteSerializer.create, while doing it when updating a project has no effect.How has this been tested?
Checklist
developbranch[ ] 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
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes
task_subsetsparameter from project creation and update endpoints to streamline the API.