Optimize validation layout updates #8789
Conversation
zhiltsov-max
requested review from
azhavoro,
Marishka17 and
nmanovic
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request enhance various functionalities within the CVAT application. Key updates include improvements to honeypot task management, type hint modifications for better type safety in dataset handling, and the introduction of new classes and methods for cache management and validation frame selection. Additionally, the test suite has been expanded with new test cases to ensure robust validation frame handling and annotation management. Overall, the updates aim to standardize processes, improve error handling, and enhance the clarity of code. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TaskViewSet
participant HoneypotFrameSelector
participant JobAnnotation
User->>TaskViewSet: Request to create task
TaskViewSet->>HoneypotFrameSelector: Initialize frame selector
HoneypotFrameSelector->>HoneypotFrameSelector: Select frames
TaskViewSet->>JobAnnotation: Add frames to job
JobAnnotation-->>TaskViewSet: Return job with frames
TaskViewSet-->>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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
zhiltsov-max
changed the title
![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 (5)
tests/python/rest_api/test_tasks.py (2)
2268-2276: Prevent PotentialValueErrorWhenvalidation_frame_countsIs EmptyIf
validation_frame_countsis empty, callingmax()andmin()on an empty sequence will raise aValueError. Consider adding a check to ensure thatvalidation_frame_countsis not empty before performing these operations.
4480-4488: Prevent PotentialValueErrorWhenvalidation_frame_countsIs EmptySimilarly, ensure that
validation_frame_countsis not empty before callingmax()andmin()to avoid aValueError.changelog.d/20241205_161129_mzhiltso_update_honeypot_selection.md (1)
3-4: Improve Grammar in Change DescriptionConsider rephrasing the sentence for grammatical correctness:
- Improved the uniformity of validation frame distribution in honeypot tasks and random honeypot rerollscvat/apps/dataset_manager/bindings.py (1)
53-53: LGTM! Type hints improve code clarity and type safety.The addition of type hints
QuerySet[Label]for both the input parameter and return type makes the method signature more explicit and helps catch type-related issues early.Consider adding a docstring to describe the method's purpose and parameters:
def add_prefetch_info(cls, queryset: QuerySet[Label]) -> QuerySet[Label]: """ Add prefetch related fields to the label queryset. Args: queryset: The label queryset to prefetch related fields for Returns: The queryset with prefetched related fields """cvat/apps/engine/views.py (1)
1775-1775: Improved validation and error handling for task validation layout.The changes enhance robustness by:
- Using type casting to ensure type safety
- Adding validation check for layout existence
- Providing clear error message when validation is not configured
Consider extracting the validation logic into a separate method for better code organization:
def _validate_layout_exists(self, db_task: models.Task) -> models.ValidationLayout: validation_layout = getattr(db_task.data, 'validation_layout', None) if not validation_layout: raise ValidationError( "Task has no validation setup configured. " "Validation must be initialized during task creation" ) return validation_layout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
changelog.d/20241205_161129_mzhiltso_update_honeypot_selection.md(1 hunks)cvat/apps/dataset_manager/bindings.py(1 hunks)cvat/apps/dataset_manager/task.py(1 hunks)cvat/apps/engine/cache.py(3 hunks)cvat/apps/engine/quality_control.py(1 hunks)cvat/apps/engine/serializers.py(10 hunks)cvat/apps/engine/task.py(3 hunks)cvat/apps/engine/views.py(1 hunks)dev/format_python_code.sh(1 hunks)tests/python/rest_api/test_tasks.py(2 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud
cvat/apps/engine/quality_control.py
[notice] 19-19: Results that depend on random number generation should be reproducible
Provide a seed for this random generator.See more on SonarQube Cloud
🔇 Additional comments (6)
cvat/apps/engine/quality_control.py (1)
18-21: 🛠️ Refactor suggestion
Provide a seed for reproducibility when initializing the random generator
The random number generator is initialized without a seed when rng is not provided. This can lead to non-reproducible results in frame selection. To ensure consistent and reproducible behavior, consider adding an optional seed parameter to the constructor and use it to initialize the random generator.
🧰 Tools
🪛 GitHub Check: SonarCloud
[notice] 19-19: Results that depend on random number generation should be reproducible
Provide a seed for this random generator.See more on SonarQube Cloud
cvat/apps/engine/serializers.py (2)
1514-1514: Verify if setting db_image.real_frame = -1 is appropriate
Assigning -1 to db_image.real_frame may have unintended consequences if real_frame is expected to be a non-negative integer representing a valid frame index. Verify that this assignment correctly represents the intended logic and that downstream code handles this value appropriately.
1566-1572: Ensure compatibility of bulk_create with update_conflicts parameter
The use of bulk_create with update_conflicts=True and update_fields parameters requires Django 2.2 or higher. Confirm that the project's Django version supports these parameters to prevent runtime errors.
Run the following script to check the Django version:
✅ Verification successful
Django version is compatible with bulk_create parameters
The project uses Django 4.2.16, which fully supports bulk_create with update_conflicts, update_fields, and unique_fields parameters, as these features were introduced in Django 2.2. The code is safe to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that Django version is 2.2 or higher.
# Expected result: The Django version should be 2.2.0 or higher.
# Command to get the installed Django version
python -c "import django; print(django.get_version())"
Length of output: 153
Script:
#!/bin/bash
# Let's check the requirements files for Django version
rg "django[>=<]" -i --no-filename
Length of output: 92
dev/format_python_code.sh (1)
33-33: LGTM!
The addition of cvat/apps/engine/quality_control.py to the formatting script is appropriate.
cvat/apps/engine/cache.py (1)
68-68: LGTM!
The import of format_list is appropriate for use in logging messages.
cvat/apps/dataset_manager/task.py (1)
87-87: LGTM!
The updated type annotations for the add_prefetch_info method enhance type safety and clarity.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8789 +/- ##
===========================================
- Coverage 73.93% 73.88% -0.06%
===========================================
Files 409 408 -1
Lines 43930 44086 +156
Branches 3986 3986
===========================================
+ Hits 32478 32571 +93
- Misses 11452 11515 +63
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ist to own take_by, add comments, remove serializer context uses
…dates' into zm/optimize-validation-layout-updates
…tion-layout-updates
…honeypot_real_frames updates
|
/check |
|
❌ Some checks failed |
|
@zhiltsov-max, need to fix remark issue |
Co-authored-by: Maria Khrustaleva <[email protected]>
There was a problem hiding this comment.
@zhiltsov-max, please fix this comment: #8789 (comment)
LGTM
|
Motivation and context
Fixes #8686
Includes #8689
take_byandchunked_listuses in the server codedisabled_framesandframe_selection_methodare used simultaneously inPATCH /tasks/id/validation_layoutHow has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation