Fix dataset downloading #7864
Fix dataset downloading #7864
Conversation
- prolong export cache lifetime on cache requests - change 500 to 404 on missing cache file during downloading - remove rq job removal after result retrieval - make export filenames more distinct - fix possible infinite deferring of dependent rq jobs
WalkthroughThe changes introduce new functionalities for handling concurrent export and cleanup processes in the CVAT application, alongside updates to package dependencies. Key additions include new classes and methods for multiprocessing, lock management, and export file handling. These improvements aim to enhance the reliability and efficiency of export operations. Additionally, multiple package versions have been upgraded to maintain compatibility and security. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant CVATApp
participant ExportManager
participant LockManager
participant FileHandler
User->>CVATApp: Initiate export
CVATApp->>ExportManager: Start export process
ExportManager->>LockManager: Acquire lock
LockManager-->>ExportManager: Lock acquired
ExportManager->>FileHandler: Export data
FileHandler-->>ExportManager: Data exported
ExportManager->>LockManager: Release lock
LockManager-->>ExportManager: Lock released
ExportManager-->>CVATApp: Export completed
CVATApp-->>User: Export successful
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 0
Out of diff range and nitpick comments (3)
cvat/apps/engine/views.py (3)
Line range hint
44-44: Remove unused import to clean up the code.- import cvat.apps.dataset_manager.views # pylint: disable=unused-import
Line range hint
1966-1966: Remove unnecessary f-string as it does not contain any placeholders.- server_address = request.scheme + '://' + server_address = str(request.scheme) + '://'
Line range hint
2713-2713: Remove unnecessary f-string as it does not contain any placeholders.- server_address += request.get_host() + server_address = server_address + str(request.get_host())
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- cvat/apps/dataset_manager/apps.py (1 hunks)
- cvat/apps/dataset_manager/default_settings.py (1 hunks)
- cvat/apps/dataset_manager/util.py (2 hunks)
- cvat/apps/dataset_manager/views.py (4 hunks)
- cvat/apps/engine/views.py (4 hunks)
- cvat/requirements/base.in (1 hunks)
- cvat/requirements/base.txt (10 hunks)
- cvat/requirements/development.txt (3 hunks)
- cvat/requirements/production.txt (1 hunks)
- utils/dataset_manifest/requirements.txt (2 hunks)
Files skipped from review due to trivial changes (5)
- cvat/apps/dataset_manager/apps.py
- cvat/apps/dataset_manager/default_settings.py
- cvat/requirements/base.in
- cvat/requirements/development.txt
- utils/dataset_manifest/requirements.txt
Additional Context Used
Ruff (4)
cvat/apps/dataset_manager/views.py (1)
27-27:
.util.LockModeimported but unusedcvat/apps/engine/views.py (3)
44-44:
cvat.apps.dataset_manager.viewsimported but unused
1966-1966: f-string without any placeholders
2713-2713: f-string without any placeholders
Additional comments not posted (6)
cvat/requirements/production.txt (1)
13-13: Update ofexceptiongroupto1.2.1appears safe and beneficial.cvat/apps/dataset_manager/util.py (1)
93-146: Introduced a robust locking mechanism usingRedlock. Ensure thorough testing, especially for edge cases like rapid lock acquisition and release.cvat/requirements/base.txt (1)
Line range hint
53-325: Updates to package versions inbase.txtare minor and likely include improvements or security patches. Verify compatibility with existing codebase.cvat/apps/dataset_manager/views.py (1)
Line range hint
27-149: Integration of new locking mechanism and error handling in dataset management views appears well-implemented. Recommend additional testing for robustness.cvat/apps/engine/views.py (2)
2962-2962: Ensure that the new caching logic in_export_annotationsis correctly implemented and tested.
3022-3022: Confirm that the error handling inexport_annotationscovers all potential failure scenarios effectively.Verification successful
The output from the revised script provides a comprehensive list of exception handling within the
export_annotationsmethod and related areas in thecvat/apps/engine/views.pyfile. The exceptions handled includeValidationError,PermissionDenied,NotFound,AttributeError,IntegrityError,APIException,ValueError,CloudStorageModel.DoesNotExist, and generalException. This indicates a robust error handling mechanism that covers a wide range of potential issues that could arise during the execution of theexport_annotationsmethod.Given this information, it appears that the error handling within the
export_annotationsmethod is indeed comprehensive, covering various specific and general exceptions that could occur. This aligns with the initial review comment's concern about ensuring that all potential failure scenarios are effectively covered.Conclusion:
The error handling in theexport_annotationsmethod appears to be comprehensive and well-implemented, addressing a variety of error conditions that could potentially disrupt the functionality.
- The method handles specific exceptions like
ValidationError,PermissionDenied, andNotFound, which are crucial for robust API behavior.- It also covers database-related errors through
AttributeErrorandIntegrityError.- General exceptions and unexpected errors are caught by broad
Exceptionhandling.- Special cases like
CloudStorageModel.DoesNotExistare also considered, which is important for operations involving external storage.This thorough approach ensures that the method can gracefully handle errors and provide appropriate responses, thereby enhancing the reliability and user experience of the dataset exporting functionalities.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check all branches of error handling in the `export_annotations` method to ensure comprehensive coverage. # Test: Search for error handling branches in the method. Expect: Each branch handles a specific error scenario appropriately. rg --type python $'export_annotations'Length of output: 68
Script:
#!/bin/bash # Description: Re-check all branches of error handling in the `export_annotations` method to ensure comprehensive coverage. # Test: Search for error handling branches in the method. Expect: Each branch handles a specific error scenario appropriately. rg 'export_annotations' --files-with-matches | xargs rg 'except'Length of output: 3895
|
/check |
|
❌ Some checks failed |
|
❌ Some checks failed |
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (6)
cvat/apps/dataset_manager/util.py (1)
Line range hint
16-41: Consider adding more detailed docstrings for the utility functions to improve maintainability and clarity for other developers.+ """ + Creates a zip archive from a directory. + + Args: + src_path (str): The source directory path. + dst_path (str): The destination zip file path. + """tests/python/rest_api/test_resource_import_export.py (2)
Line range hint
32-57: Comprehensive test cases for exporting resources to cloud storage. Consider adding comments to explain the test scenarios in detail for better maintainability.+ """ + Test saving resources to cloud storage with specific configurations. + This test covers various object types and ensures that the export functionality + respects the specified cloud storage locations and settings. + """
Line range hint
182-207: Import functionality tests are well-covered. Ensure that edge cases, such as network failures or partial data availability, are also tested.Would you like me to help by adding some tests for edge cases involving network failures or partial data availability?
cvat/apps/engine/views.py (3)
Line range hint
44-44: Remove unused import to clean up the code.- import cvat.apps.dataset_manager.views # pylint: disable=unused-import
Line range hint
1967-1967: The f-string used here does not contain any placeholders, which is unnecessary. Consider using a regular string if no formatting is needed.- server_address = request.scheme + '://' + server_address = str(request.scheme) + '://'
Line range hint
2714-2714: Similar to the previous comment, an f-string is used here without placeholders. Use a regular string instead.- filename_pattern = build_annotations_file_name( + filename_pattern = str(build_annotations_file_name(
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- changelog.d/20240508_144202_mzhiltso_fix_dataset_downloading.md (1 hunks)
- cvat/apps/dataset_manager/default_settings.py (1 hunks)
- cvat/apps/dataset_manager/util.py (2 hunks)
- cvat/apps/dataset_manager/views.py (4 hunks)
- cvat/apps/engine/views.py (4 hunks)
- cvat/requirements/base.in (1 hunks)
- cvat/requirements/base.txt (10 hunks)
- cvat/requirements/development.txt (1 hunks)
- cvat/requirements/testing.txt (1 hunks)
- tests/python/rest_api/test_resource_import_export.py (5 hunks)
- utils/dataset_manifest/requirements.txt (2 hunks)
Files skipped from review due to trivial changes (1)
- cvat/requirements/testing.txt
Files skipped from review as they are similar to previous changes (4)
- cvat/apps/dataset_manager/default_settings.py
- cvat/requirements/base.in
- cvat/requirements/base.txt
- cvat/requirements/development.txt
Additional Context Used
Ruff (3)
cvat/apps/engine/views.py (3)
44-44:
cvat.apps.dataset_manager.viewsimported but unused
1967-1967: f-string without any placeholders
2714-2714: f-string without any placeholders
Additional comments not posted (5)
changelog.d/20240508_144202_mzhiltso_fix_dataset_downloading.md (1)
1-5: Changelog entry is clear and properly linked to the PR.utils/dataset_manifest/requirements.txt (2)
8-9: Ensure that non-binary flags forlxmlandxmlsecare required for your project's needs.Verification successful
The comprehensive search across the codebase confirms the consistent use of
--no-binaryflags forlxmlandxmlsecacross various requirement files and Dockerfiles. This indicates a deliberate and justified choice to compile these libraries from source, likely for compatibility or optimization reasons specific to the project's needs. The presence of related development libraries in Dockerfiles further supports this requirement.
- utils/dataset_manifest/requirements.txt: Both
lxmlandxmlsecare set to compile from source.- cvat/requirements/: Multiple environment-specific files (base, testing, development, production) also enforce non-binary installation.
- Dockerfile: Includes libraries like
libxmlsec1-devandlibxmlsec1-openssl, which are related toxmlsec, suggesting system-level dependencies that might require source compilation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify compatibility of updated package versions. # Test: Search for the package usage in the Python files. Expect: No deprecated functions used. rg --type py 'lxml|xmlsec'Length of output: 28
Script:
#!/bin/bash # Description: Comprehensive search for any references to 'lxml' or 'xmlsec' in the entire codebase. # Search all files for 'lxml' or 'xmlsec' to ensure no usage is missed. rg 'lxml|xmlsec'Length of output: 1026
21-21: Update totqdm==4.66.4looks good. Ensure it's tested for compatibility with your project's usage.cvat/apps/dataset_manager/util.py (1)
104-141: Locking mechanism usingRedlockis implemented correctly. Ensure thorough testing, especially for edge cases in concurrent environments.cvat/apps/dataset_manager/views.py (1)
179-227: Robust error handling inclear_export_cache. Ensure that all possible exceptions are considered and handled appropriately.
… zm/fix-dataset-downloading
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
cvat/apps/engine/views.py (1)
Line range hint
44-44: Remove unused import.- import cvat.apps.dataset_manager.views # pylint: disable=unused-import
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- cvat/apps/dataset_manager/default_settings.py (1 hunks)
- cvat/apps/dataset_manager/tests/test_rest_api_formats.py (9 hunks)
- cvat/apps/dataset_manager/util.py (2 hunks)
- cvat/apps/dataset_manager/views.py (3 hunks)
- cvat/apps/engine/tests/test_rest_api_3D.py (3 hunks)
- cvat/apps/engine/tests/utils.py (4 hunks)
- cvat/apps/engine/views.py (4 hunks)
- cvat/requirements/base.in (1 hunks)
- cvat/requirements/base.txt (13 hunks)
- cvat/requirements/development.txt (3 hunks)
- cvat/requirements/testing.in (1 hunks)
- cvat/requirements/testing.txt (1 hunks)
Files not reviewed due to errors (1)
- cvat/apps/dataset_manager/util.py (no review received)
Files skipped from review due to trivial changes (5)
- cvat/apps/dataset_manager/default_settings.py
- cvat/apps/engine/tests/test_rest_api_3D.py
- cvat/requirements/base.in
- cvat/requirements/development.txt
- cvat/requirements/testing.in
Additional Context Used
Ruff (4)
cvat/apps/dataset_manager/views.py (1)
29-29:
.util.EXPORT_CACHE_DIR_NAMEimported but unusedcvat/apps/engine/views.py (3)
44-44:
cvat.apps.dataset_manager.viewsimported but unused
1967-1967: f-string without any placeholders
2714-2714: f-string without any placeholders
Additional comments not posted (33)
cvat/requirements/testing.txt (3)
9-10: Ensure consistency of--no-binaryoptions across all requirements files.
14-14: LGTM! The update tofakeredis[lua]ensures compatibility with Lua scripts.
16-16: LGTM! The addition oflupais appropriate for Lua scripting support.cvat/apps/engine/tests/utils.py (2)
51-91: LGTM! Theclear_rq_jobsfunction is well-structured and includes necessary error handling.
95-96: LGTM! The_clear_rq_jobsmethod improves test isolation and reliability.cvat/requirements/base.txt (5)
9-10: Ensure consistency of--no-binaryoptions across all requirements files.
56-56: LGTM! The update tocryptographyincludes important security patches and bug fixes.
74-74: LGTM! The update todjangoincludes important security patches and bug fixes.
105-105: LGTM! The update todjango-health-checkincludes important security patches and bug fixes.
189-190: LGTM! The addition ofmmh3andpotteryis appropriate for the new locking mechanisms.Also applies to: 215-216
cvat/apps/dataset_manager/views.py (12)
55-60: LGTM! The function correctly returns the TTL for different database instances.
60-77: LGTM! The initial setup for logging and determining the database instance is correct.
77-79: LGTM! The cache TTL and directory setup is correct.
85-95: LGTM! The export filename creation and directory preparation are correct.
97-127: LGTM! The locking mechanism and export process are correctly implemented.
130-137: LGTM! The exception handling is correctly implemented.
162-164: LGTM! The initial setup and backward compatibility comment are correct.
166-173: LGTM! The locking mechanism and file existence check are correctly implemented.
175-193: LGTM! The cache TTL check and file removal process are correctly implemented.
200-201: LGTM! The function correctly returns a list of export formats.
Line range hint
203-204: LGTM! The function correctly returns a list of import formats.
Line range hint
206-210: LGTM! The function correctly returns a dictionary of importers and exporters.cvat/apps/dataset_manager/tests/test_rest_api_formats.py (8)
11-11: New imports are necessary for concurrency handling and test enhancements.Also applies to: 17-19, 22-23, 25-25
97-97: Inheriting_DbTestBasefromApiTestBaseensures consistency and consolidates the test base classes.
419-420: Adding_clear_rq_jobsensures a clean test environment and avoids interference from previous tests.
526-527: Adding_clear_rq_jobsensures a clean test environment and avoids interference from previous tests.
613-614: Adding_clear_rq_jobsensures a clean test environment and avoids interference from previous tests.
1265-1590: TheExportBehaviorTestclass and its methods are well-structured and necessary for testing concurrency handling in export operations.
2058-2059: Adding_clear_rq_jobsensures a clean test environment and avoids interference from previous tests.
2140-2141: Adding_clear_rq_jobsensures a clean test environment and avoids interference from previous tests.cvat/apps/engine/views.py (3)
2963-2964: LGTM! Ensure the logic for handling cache TTL and instance update time is correct.Also applies to: 2967-2967, 2969-2969
Verification successful
The
_export_annotationsfunction incvat/apps/engine/views.pycorrectly handles the cache TTL and instance update time. The function retrieves the cache TTL usingdm.views.get_export_cache_ttl(db_instance)and the instance update time usingtimezone.localtime(db_instance.updated_date). If thedb_instanceis aProject, it updates theinstance_update_timeto the maximum update time of its tasks.
- The cache TTL is correctly retrieved and used in the function.
- The instance update time is correctly retrieved and updated if necessary.
Therefore, the logic for handling cache TTL and instance update time is correct.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic for handling cache TTL and instance update time. # Test: Search for the function usage. Expect: Correct handling of cache TTL and instance update time. rg --type python $'_export_annotations'Length of output: 69
Script:
#!/bin/bash # Description: Verify the logic for handling cache TTL and instance update time. # Test: Search for the function usage. Expect: Correct handling of cache TTL and instance update time. rg --type py '_export_annotations'Length of output: 1280
Script:
#!/bin/bash # Description: Extract the implementation of `_export_annotations` function from `cvat/apps/engine/views.py`. # Extract the function implementation. ast-grep --lang python --pattern $'def _export_annotations($_, $_, $$$):\n $$$' cvat/apps/engine/views.pyLength of output: 13779
3034-3035: LGTM! Ensure the logic for handling cloud storage and temporary files is correct.Also applies to: 3036-3036, 3037-3037, 3038-3038, 3039-3039, 3040-3040, 3041-3041, 3042-3042, 3043-3043, 3044-3044, 3045-3045, 3046-3046, 3047-3047, 3048-3048, 3049-3049
3078-3078: LGTM! Ensure the logic for handling cloud storage and temporary files is correct.Also applies to: 3093-3094
Verification successful
The
_import_project_datasetfunction and its usages have been extracted for review. Here are the relevant code segments:Function Definition
def _import_project_dataset(request, rq_id_template, rq_func, db_obj, format_name, filename=None, conv_mask_to_poly=True, location_conf=None): format_desc = {f.DISPLAY_NAME: f for f in dm.views.get_import_formats()}.get(format_name) if format_desc is None: raise serializers.ValidationError( "Unknown input format '{}'".format(format_name)) elif not format_desc.ENABLED: return Response(status=status.HTTP_405_METHOD_NOT_ALLOWED) rq_id = rq_id_template.format(db_obj.pk, request.user)Function Usages
Usage in
import_annotationsmethod:if request.method in {'POST', 'OPTIONS'}: return self.import_annotations( request=request, db_obj=self._object, import_func=_import_project_dataset, rq_func=dm.project.import_dataset_as_project, rq_id_template=self.IMPORT_RQ_ID_TEMPLATE )Usage in another context:
conv_mask_to_poly = to_bool(request.query_params.get('conv_mask_to_poly', True)) tmp_dir = self._object.get_tmp_dirname() uploaded_file = None if os.path.isfile(os.path.join(tmp_dir, filename)): uploaded_file = os.path.join(tmp_dir, filename) return _import_project_dataset( request=request, filename=uploaded_file, rq_id_template=self.IMPORT_RQ_ID_TEMPLATE, rq_func=dm.project.import_dataset_as_project, db_obj=self._object, )Analysis
- The function
_import_project_datasetincludes logic to handle different dataset formats and raises appropriate errors for unknown or disabled formats.- The usages of the function correctly pass the necessary parameters, including handling temporary files (
uploaded_file) and cloud storage configurations.Based on the extracted code segments and the analysis, the logic for handling cloud storage and temporary files appears to be correctly implemented.
Final Response
The logic for handling cloud storage and temporary files in the
_import_project_datasetfunction and its usages is correctly implemented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic for handling cloud storage and temporary files. # Test: Search for the function usage. Expect: Correct handling of cloud storage and temporary files. rg --type python $'_import_project_dataset'Length of output: 73
Script:
#!/bin/bash # Description: Verify the logic for handling cloud storage and temporary files. # Test: Search for the function usage. Expect: Correct handling of cloud storage and temporary files. rg --type py '_import_project_dataset'Length of output: 356
Script:
#!/bin/bash # Description: Extract the definition and usages of `_import_project_dataset` function for detailed review. # Extract the function definition rg --type py --context 10 'def _import_project_dataset' cvat/apps/engine/views.py # Extract the usages of the function rg --type py --context 5 '_import_project_dataset' cvat/apps/engine/views.pyLength of output: 2662
… zm/fix-dataset-downloading
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/dataset_manager/tests/test_rest_api_formats.py (9 hunks)
Additional comments not posted (6)
cvat/apps/dataset_manager/tests/test_rest_api_formats.py (6)
11-23: New imports for concurrency handling and utility functions.
97-97: New class_DbTestBasefor database-related test setup and utility functions.
1265-1265: New classExportBehaviorTestfor testing concurrent export and cleanup operations.
1368-1590: New test methodtest_concurrent_export_and_cleanupfor testing concurrent export and cleanup operations.
1591-1787: New test methodtest_concurrent_download_and_cleanupfor testing concurrent download and cleanup operations.
1789-1811: New test methodtest_export_can_create_file_and_cleanup_jobfor testing export file creation and cleanup job scheduling.
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (3)
cvat/apps/engine/views.py (3)
Line range hint
44-44: Remove unused import to clean up the code.- import cvat.apps.dataset_manager.views # pylint: disable=unused-import
Line range hint
1967-1967: The f-string is used without placeholders, which is unnecessary. Convert it to a regular string.- instance_timestamp = datetime.strftime(instance_update_time, "%Y_%m_%d_%H_%M_%S") + instance_timestamp = datetime.strftime(instance_update_time, "%Y_%m_%d_%H_%M_%S")
Line range hint
2714-2714: The f-string is used without placeholders, which is unnecessary. Convert it to a regular string.- instance_timestamp = datetime.strftime(instance_update_time, "%Y_%m_%d_%H_%M_%S") + instance_timestamp = datetime.strftime(instance_update_time, "%Y_%m_%d_%H_%M_%S")
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/engine/views.py (5 hunks)
Additional Context Used
Ruff (3)
cvat/apps/engine/views.py (3)
44-44:
cvat.apps.dataset_manager.viewsimported but unused
1967-1967: f-string without any placeholders
2714-2714: f-string without any placeholders
Additional comments not posted (4)
cvat/apps/engine/views.py (4)
2963-2975: Ensure proper handling of theinstance_update_timefor projects with multiple tasks. The logic to determine the latest update time across all tasks and the project itself is sound.
Line range hint
2976-3055: The logic to handle outdated exports and re-enqueue jobs is complex but correctly implements the requirements. It checks if the existing job's data is outdated compared to the instance update time and handles it accordingly. This includes canceling outdated jobs and re-enqueuing them if necessary.
3021-3033: The logic to handle file existence and re-enqueue jobs if the file does not exist is correctly implemented. It ensures that files are re-exported if they are not found, which is crucial for maintaining data integrity.
3039-3055: Properly handle failed jobs by providing detailed error information to the client. This is crucial for debugging and understanding why a job failed.
There was a problem hiding this comment.
Actionable comments posted: 6
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/dataset_manager/tests/test_rest_api_formats.py (9 hunks)
Additional comments not posted (2)
cvat/apps/dataset_manager/tests/test_rest_api_formats.py (2)
17-17: Ensure proper usage of context managers to handle resources efficiently.Verification successful
Ensure proper usage of context managers to handle resources efficiently
The usage of
ExitStackandcontextmanagerincvat/apps/dataset_manager/tests/test_rest_api_formats.pyappears to be correct and follows proper resource management practices. Here are the key points:
@contextmanagerDecorators: These are used to define context managers for handling processes and locks, ensuring that resources are properly managed and released.ExitStack: This is used to manage multiple context managers, ensuring that all resources are properly cleaned up even if an error occurs.The code snippets provided show that these constructs are used appropriately to handle resources efficiently.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of ExitStack and contextmanager in the file to ensure proper resource management. # Test: Search for ExitStack and contextmanager usage. Expect: Proper usage patterns. rg --type python $'ExitStack|contextmanager' cvat/apps/dataset_manager/tests/test_rest_api_formats.pyLength of output: 133
Script:
#!/bin/bash # Description: Verify the usage of ExitStack and contextmanager in the file to ensure proper resource management. # Test: Search for ExitStack and contextmanager usage. Expect: Proper usage patterns. rg 'ExitStack|contextmanager' cvat/apps/dataset_manager/tests/test_rest_api_formats.pyLength of output: 269
Script:
#!/bin/bash # Description: Extract surrounding lines of ExitStack and contextmanager usage for detailed verification. # Extract 5 lines before and after each match to understand the context. rg -C 5 'ExitStack|contextmanager' cvat/apps/dataset_manager/tests/test_rest_api_formats.pyLength of output: 2772
1792-1792: Ensure proper handling of file usage conflicts during cleanup operations.Verification successful
Ensure proper handling of file usage conflicts during cleanup operations
The search results indicate that
LockNotAvailableErrorandFileIsBeingUsedErrorare indeed referenced in the test filecvat/apps/dataset_manager/tests/test_rest_api_formats.py. This suggests that there are mechanisms in place to handle file usage conflicts during cleanup operations.
LockNotAvailableErroris imported and used multiple times.FileIsBeingUsedErroris also imported and used in assertions.These references confirm that the code includes error handling for file usage conflicts.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that file usage conflicts are handled correctly in cleanup operations. # Test: Search for conflict handling in cleanup operations. Expect: Proper error handling mechanisms. rg --type python $'LockNotAvailableError|FileIsBeingUsedError' cvat/apps/dataset_manager/tests/test_rest_api_formats.pyLength of output: 151
Script:
#!/bin/bash # Description: Verify that file usage conflicts are handled correctly in cleanup operations. # Test: Search for conflict handling in cleanup operations. Expect: Proper error handling mechanisms. rg --type py $'LockNotAvailableError|FileIsBeingUsedError' cvat/apps/dataset_manager/tests/test_rest_api_formats.pyLength of output: 1084
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7864 +/- ##
===========================================
+ Coverage 83.35% 83.37% +0.02%
===========================================
Files 380 383 +3
Lines 40032 40171 +139
Branches 3766 3769 +3
===========================================
+ Hits 33367 33492 +125
- Misses 6665 6679 +14
|
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range and nitpick comments (1)
cvat/apps/dataset_manager/views.py (1)
99-129: Add detailed logging for export operations.Consider adding more detailed logging in the
exportfunction to provide better visibility into the export process. This could include logging the start and end of the export operation, any errors encountered, and the final status of the export. This would help in debugging and monitoring the export operations.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- cvat/apps/dataset_manager/tests/test_rest_api_formats.py (9 hunks)
- cvat/apps/dataset_manager/util.py (2 hunks)
- cvat/apps/dataset_manager/views.py (3 hunks)
Additional Context Used
Ruff (1)
cvat/apps/dataset_manager/views.py (1)
29-29:
.util.EXPORT_CACHE_DIR_NAMEimported but unused
Learnings (1)
cvat/apps/dataset_manager/tests/test_rest_api_formats.py (1)
User: zhiltsov-max PR: cvat-ai/cvat#7864 File: cvat/apps/dataset_manager/tests/test_rest_api_formats.py:1266-1266 Timestamp: 2024-05-17T12:20:07.528Z Learning: The CVAT project prefers using the `attrs` module over `dataclasses` due to its additional features and is commonly used across the project.
Additional comments not posted (4)
cvat/apps/dataset_manager/tests/test_rest_api_formats.py (4)
1266-1266: Use Python'sdataclassesmodule for simplicity and efficiency.- @define + from dataclasses import dataclass - @dataclass class SharedBase:Likely invalid or redundant comment.
1265-1265: Enhance theExportBehaviorTestclass with additional comments for better understanding.+ # This class tests the behavior of concurrent export and cleanup operations. class ExportBehaviorTest(_DbTestBase):Likely invalid or redundant comment.
419-419: Optimize the cleanup of RQ jobs to enhance readability and understanding of its functionality.- self._clear_rq_jobs() # clean up from previous tests and iterations + self.clearResourceQueueJobs() # Clear resource queue jobs from previous tests and iterationsLikely invalid or redundant comment.
97-97: Consider renaming_DbTestBasetoDatabaseTestBasefor improved readability and adherence to Python naming conventions.- class _DbTestBase(ApiTestBase): + class DatabaseTestBase(ApiTestBase):Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
cvat/apps/dataset_manager/tests/test_rest_api_formats.py (1)
421-421: Repeated calls to_clear_rq_jobscan be optimized.Consider refactoring the repeated calls to
_clear_rq_jobsacross different test methods into a setup or teardown method to reduce redundancy and improve maintainability.Also applies to: 528-528, 615-615, 865-865, 2071-2071, 2153-2153
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- cvat/apps/dataset_manager/tests/test_rest_api_formats.py (9 hunks)
- cvat/apps/dataset_manager/util.py (2 hunks)
Additional Context Used
Learnings (2)
cvat/apps/dataset_manager/util.py (1)
User: zhiltsov-max PR: cvat-ai/cvat#7864 File: cvat/apps/dataset_manager/util.py:149-157 Timestamp: 2024-05-27T14:31:27.282Z Learning: zhiltsov-max prefers error messages in the `get_export_cache_dir` function to remain less detailed to facilitate manual investigation.cvat/apps/dataset_manager/tests/test_rest_api_formats.py (2)
User: zhiltsov-max PR: cvat-ai/cvat#7864 File: cvat/apps/dataset_manager/tests/test_rest_api_formats.py:2061-2061 Timestamp: 2024-05-27T14:34:32.440Z Learning: The project uses a specific naming convention for functions and methods, which should be adhered to in suggestions and comments.User: zhiltsov-max PR: cvat-ai/cvat#7864 File: cvat/apps/dataset_manager/tests/test_rest_api_formats.py:1266-1266 Timestamp: 2024-05-17T12:20:07.528Z Learning: The CVAT project prefers using the `attrs` module over `dataclasses` due to its additional features and is commonly used across the project.
Additional comments not posted (14)
cvat/apps/dataset_manager/util.py (2)
149-157: Review error handling inget_export_cache_dir.The error handling in
get_export_cache_diris appropriate as it raises aFileNotFoundErrorwhen the base directory does not exist. This is crucial for debugging and ensuring that the file operations are performed on valid directories.
106-144: Review the use of locking mechanism.The implementation of
get_export_cache_lockusesRedlockfor distributed locking, which is generally a robust choice. However, consider the following:
- Verify that the
auto_release_timeset byttlis appropriate for the operations being locked.- Ensure that the lock acquisition logic (
blockandacquire_timeout) is tested under high concurrency to prevent deadlocks or long wait times.cvat/apps/dataset_manager/tests/test_rest_api_formats.py (12)
1267-1267: TheExportBehaviorTestclass effectively handles concurrency tests.This class is well-structured to test various concurrency issues related to dataset export and cleanup.
1268-1268: Usage ofattrsoverdataclassesis consistent with project standards.Good adherence to project conventions by using
attrsfor defining data classes.
1272-1272: Inheritance fromSharedBaseis correctly implemented inSharedBoolandSharedString.The inheritance hierarchy is clear and correctly utilizes shared functionality from
SharedBase.Also applies to: 1284-1284
1298-1298: Custom exception_LockTimeoutErroris well-defined for specific timeout errors.This custom exception helps in handling specific timeout scenarios clearly within the tests.
1301-1301: Method overloading forset_conditionis correctly used to handle different types.The use of method overloading here allows for flexible setting of conditions based on variable type, enhancing code robustness.
Also applies to: 1305-1305
1326-1326: Utility functionsside_effectandchain_side_effectsare well-implemented for test flexibility.These functions provide a robust way to handle side effects in a controlled manner during testing, which is crucial for accurate concurrency testing.
Also applies to: 1339-1339
1358-1358: Theprocess_closingcontext manager is a critical addition for safely handling process termination.This method ensures that processes are properly closed after tests, preventing potential resource leaks.
1370-1370: Thetest_concurrent_export_and_cleanupmethod is comprehensive in testing race conditions.This method effectively simulates and tests potential race conditions between export and cleanup operations, ensuring robustness in concurrent environments.
1593-1593: Thetest_concurrent_download_and_cleanupmethod thoroughly tests file handling under concurrency.This test method is well-designed to ensure that file download and cleanup operations handle concurrency correctly without data loss or corruption.
1794-1794: Thetest_export_can_create_file_and_cleanup_jobmethod correctly tests the export functionality under normal and edge conditions.This method ensures that the export functionality is robust, handling both file creation and subsequent cleanup effectively.
1881-1881: Thetest_cleanup_can_remove_filemethod effectively tests the cleanup functionality of the export system.This method ensures that files are properly removed after their intended lifecycle, preventing unnecessary storage usage.
99-99: Consider renaming_DbTestBasetoDatabaseTestBasefor improved readability.- class _DbTestBase(ApiTestBase): + class DatabaseTestBase(ApiTestBase):Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- cvat/apps/dataset_manager/tests/test_rest_api_formats.py (9 hunks)
- cvat/apps/dataset_manager/util.py (2 hunks)
- cvat/requirements/base.txt (13 hunks)
- cvat/requirements/development.txt (3 hunks)
- cvat/requirements/testing.txt (1 hunks)
Files skipped from review due to trivial changes (1)
- cvat/requirements/development.txt
Additional Context Used
Learnings (2)
cvat/apps/dataset_manager/util.py (1)
User: zhiltsov-max PR: cvat-ai/cvat#7864 File: cvat/apps/dataset_manager/util.py:149-157 Timestamp: 2024-05-27T14:31:27.282Z Learning: zhiltsov-max prefers error messages in the `get_export_cache_dir` function to remain less detailed to facilitate manual investigation.cvat/apps/dataset_manager/tests/test_rest_api_formats.py (2)
User: zhiltsov-max PR: cvat-ai/cvat#7864 File: cvat/apps/dataset_manager/tests/test_rest_api_formats.py:2061-2061 Timestamp: 2024-05-27T14:34:32.440Z Learning: The project uses a specific naming convention for functions and methods, which should be adhered to in suggestions and comments.User: zhiltsov-max PR: cvat-ai/cvat#7864 File: cvat/apps/dataset_manager/tests/test_rest_api_formats.py:1266-1266 Timestamp: 2024-05-17T12:20:07.528Z Learning: The CVAT project prefers using the `attrs` module over `dataclasses` due to its additional features and is commonly used across the project.
Additional comments not posted (13)
cvat/requirements/testing.txt (3)
9-9: Updated coverage tool version to 7.2.3.
12-14: Updated fakeredis and its dependency lupa for testing environments.
14-14: Updated sortedcontainers to maintain compatibility with the new fakeredis version.cvat/requirements/base.txt (3)
72-72: Updated Django to 4.2.13, ensuring the application is up-to-date with the latest security patches and bug fixes.
103-103: Updated django-health-check to 3.18.2 for improved health checking capabilities.
213-214: Added new dependency, pottery, for enhanced Redis functionalities, likely related to the new locking mechanisms.cvat/apps/dataset_manager/util.py (3)
105-144: The implementation of Redis locking using the Redlock algorithm inget_export_cache_lockis correctly handled. Good use of context management and error handling.
149-157: The functionget_export_cache_dircorrectly checks for the existence of the directory and raises an appropriate error if not found. This aligns with your preference for less detailed error messages to facilitate manual investigation.
190-231: The functionparse_export_file_patheffectively uses regular expressions to parse the export file path and extract necessary metadata, with robust error handling for parsing failures.cvat/apps/dataset_manager/tests/test_rest_api_formats.py (4)
1268-1268: Usingattrsfor defining data classes is consistent with project standards. Good use of the library's features for multiprocessing compatibility.
1794-1794: The methodtest_concurrent_download_and_cleanupeffectively tests TOCTOU issues with file handling. It's a critical test that ensures the robustness of file operations under concurrency.
99-99: Consider renaming_DbTestBasetoDatabaseTestBasefor better readability and adherence to Python naming conventions.- class _DbTestBase(ApiTestBase): + class DatabaseTestBase(ApiTestBase):Skipped due to learnings
User: zhiltsov-max PR: cvat-ai/cvat#7864 File: cvat/apps/dataset_manager/tests/test_rest_api_formats.py:2061-2061 Timestamp: 2024-05-27T14:34:32.440Z Learning: The project uses a specific naming convention for functions and methods, which should be adhered to in suggestions and comments.
2024-2024: TheProjectDumpUploadclass tests are comprehensive and cover a wide range of scenarios. Ensure that all new formats added to the system are included in these tests to maintain coverage.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- utils/dataset_manifest/requirements.txt (2 hunks)
Files skipped from review due to trivial changes (1)
- utils/dataset_manifest/requirements.txt
Motivation and context
This PR addresses several problems:
This PR fixes the problems by the following:
finished RQ export jobs are not removed automatically on result retrieval. Now, they just use the export cache lifetime instead (should be continued in another PR)How 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
New Features
Bug Fixes
Dependencies
cryptographyto42.0.7djangoto4.2.13django-health-checkto3.18.2freezegunto1.5.1jinja2to3.1.4limitsto3.12.0lxmlto5.2.2orjsonto3.10.3potteryversion3.0.0tqdmto4.66.4