Update high-level SDK used for exporting project|task|job datasets|backups
#8255
Conversation
|
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 recent changes to the CVAT SDK enhance modularity and functionality by introducing mixins for dataset export and backup download capabilities. Key methods have been refactored or removed to streamline operations, while new testing fixtures improve coverage for cloud storage scenarios. The overall design now promotes better code organization, allowing for greater flexibility in file handling and error management. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Downloader
participant Endpoint
Client->>Downloader: Request file
Downloader->>Endpoint: Prepare file request
Endpoint->>Downloader: Return request object
Downloader->>Endpoint: Check status and URL
Endpoint->>Downloader: Confirm URL
Downloader->>Client: Download file
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: 0
Outside diff range, codebase verification and nitpick comments (5)
cvat-sdk/cvat_sdk/core/downloading.py (2)
Line range hint
72-108:
Ensure robust error handling and logging.The
prepare_filemethod is well-structured, but consider adding more detailed logging and handling potential exceptions during the request and response processing to enhance robustness.+ try: response = client.api_client.rest_client.request( method=endpoint.settings["http_method"], url=url, headers=client.api_client.get_common_headers(), ) + except Exception as e: + client.logger.error(f"Failed to initialize background process: {e}") + raise
109-132: Ensure robust error handling and logging.The
prepare_and_download_file_from_endpointmethod is well-structured, but consider adding more detailed logging and handling potential exceptions during the file preparation and download process to enhance robustness.+ try: bg_request = self.prepare_file( endpoint, url_params=url_params, query_params=query_params, status_check_period=status_check_period, ) + except Exception as e: + client.logger.error(f"Failed to prepare file: {e}") + raise assert bg_request.result_url, "Result url was not found in server response"cvat-sdk/cvat_sdk/core/proxies/model_proxy.py (3)
238-239: Validatelocationparameter.The validation for the
locationparameter is correct, but consider using a set for better performance.- if location not in ("local", "cloud_storage", None): + if location not in {"local", "cloud_storage", None}:
258-260: Check for cloud storage usage.The check for default cloud storage usage is appropriate but could be simplified for readability.
- is_cloud_used_by_default = ( - self.target_storage and self.target_storage.location.value == "cloud_storage" - ) + is_cloud_used_by_default = self.target_storage?.location.value == "cloud_storage"
300-301: Validatelocationparameter.The validation for the
locationparameter is correct, but consider using a set for better performance.- if location not in ("local", "cloud_storage", None): + if location not in {"local", "cloud_storage", None}:
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- cvat-sdk/cvat_sdk/core/downloading.py (3 hunks)
- cvat-sdk/cvat_sdk/core/proxies/jobs.py (3 hunks)
- cvat-sdk/cvat_sdk/core/proxies/model_proxy.py (2 hunks)
- cvat-sdk/cvat_sdk/core/proxies/projects.py (3 hunks)
- cvat-sdk/cvat_sdk/core/proxies/tasks.py (3 hunks)
- tests/python/sdk/test_tasks.py (4 hunks)
Additional comments not posted (14)
cvat-sdk/cvat_sdk/core/proxies/jobs.py (1)
Line range hint
24-42:
Verify the integration ofExportDatasetMixin.The inclusion of
ExportDatasetMixinshould be verified to ensure it integrates well with the existing class functionality. Ensure that the mixin methods do not conflict with existing methods in theJobclass.Verification successful
Integration of
ExportDatasetMixinverified.The
ExportDatasetMixinclass, which contains theexport_datasetmethod, does not conflict with any methods in theJobclass. The integration is correct and should function as intended.
ExportDatasetMixinis defined incvat-sdk/cvat_sdk/core/proxies/model_proxy.py.- The
Jobclass incvat-sdk/cvat_sdk/core/proxies/jobs.pydoes not have any conflicting methods.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `ExportDatasetMixin` in the `Job` class. # Test: Search for the methods provided by `ExportDatasetMixin` and ensure there are no conflicts. rg --type python -A 5 $'class ExportDatasetMixin'Length of output: 81
Script:
#!/bin/bash # Step 1: Search for the definition of `ExportDatasetMixin` without specifying the file type. rg 'class ExportDatasetMixin' -A 10 # Step 2: List the methods in `ExportDatasetMixin`. rg 'class ExportDatasetMixin' -A 50 | rg 'def ' # Step 3: List the methods in the `Job` class. rg 'class Job' -A 50 | rg 'def 'Length of output: 9242
cvat-sdk/cvat_sdk/core/proxies/projects.py (1)
Line range hint
22-43:
Verify the integration ofExportDatasetMixinandDownloadBackupMixin.The inclusion of
ExportDatasetMixinandDownloadBackupMixinshould be verified to ensure they integrate well with the existing class functionality. Ensure that the mixin methods do not conflict with existing methods in theProjectclass.Verification successful
Integration of
ExportDatasetMixinandDownloadBackupMixinverified.The
ExportDatasetMixinandDownloadBackupMixinmixins have been successfully integrated into theProjectclass without any method conflicts.
- No
export_datasetmethod found in theProjectclass.- No
download_backupmethod found in theProjectclass.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `ExportDatasetMixin` and `DownloadBackupMixin` in the `Project` class. # Test: Search for the methods provided by the mixins and ensure there are no conflicts. rg --type python -A 5 $'class ExportDatasetMixin' rg --type python -A 5 $'class DownloadBackupMixin'Length of output: 164
Script:
#!/bin/bash # Description: Verify the integration of `ExportDatasetMixin` and `DownloadBackupMixin` in the `Project` class. # Test: Search for the methods provided by the mixins and ensure there are no conflicts. rg --glob '*.py' -A 5 $'class ExportDatasetMixin' rg --glob '*.py' -A 5 $'class DownloadBackupMixin'Length of output: 965
Script:
#!/bin/bash # Description: Verify that there are no method conflicts in the `Project` class. # Test: Search for `export_dataset` and `download_backup` methods in the `Project` class. rg --glob '*.py' -A 5 'class Project' | rg 'def export_dataset' rg --glob '*.py' -A 5 'class Project' | rg 'def download_backup'Length of output: 132
cvat-sdk/cvat_sdk/core/proxies/model_proxy.py (8)
219-219: ClassExportDatasetMixinadded.The addition of this mixin class enhances modularity by encapsulating dataset export functionality.
247-251: Handle missingcloud_storage_id.The code correctly raises an error if
cloud_storage_idis not provided whenlocationiscloud_storage.
272-277: Assertresult_urlbased on location.The assertions ensure that
result_urlis correctly handled based on the location.
278-283: Download file ifresult_urlis available.The code correctly handles file downloading using the
Downloaderclass.
286-286: ClassDownloadBackupMixinadded.The addition of this mixin class enhances modularity by encapsulating backup download functionality.
313-317: Handle missingcloud_storage_id.The code correctly raises an error if
cloud_storage_idis not provided whenlocationiscloud_storage.
332-336: Assertresult_urlbased on location.The assertions ensure that
result_urlis correctly handled based on the location.
338-343: Download file ifresult_urlis available.The code correctly handles file downloading using the
Downloaderclass.cvat-sdk/cvat_sdk/core/proxies/tasks.py (1)
64-65: MixinsExportDatasetMixinandDownloadBackupMixinadded toTask.The addition of these mixins enhances the modularity and reusability of the
Taskclass by encapsulating dataset export and backup download functionalities.tests/python/sdk/test_tasks.py (3)
68-84: Fixturefxt_new_task_with_target_storageadded.The fixture correctly creates a task with specified target storage parameters, enhancing test coverage for cloud storage scenarios.
296-350: Testtest_can_download_datasetmodified.The test correctly accommodates the new fixture and parameterized approach, enhancing flexibility and coverage for different task configurations and download locations.
381-383: Testtest_can_download_framesmodified.The test correctly includes an optional
image_extensionparameter, enhancing flexibility in testing frame downloads.
|
👋 @Marishka17 Your PR may already cover it, but since it seems directly relevant to the high level SDK downloading I wanted to flag this issue I just filed this morning: #8256 |
|
/check |
|
✔️ All checks completed successfully |
|
🚫 Workflows has been canceled |
|
/check |
|
❌ Some checks failed |
Marishka17
changed the title
project|task|job datasets|backupsproject|task|job datasets|backups
Marishka17
force-pushed
the
mk/update_high_level_export_sdk
branch
2 times, most recently
from
c5fc945
to
a7f1027
Compare
Marishka17
force-pushed
the
mk/update_high_level_export_sdk
branch
from
a7f1027
to
29dd818
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8255 +/- ##
===========================================
+ Coverage 83.46% 83.51% +0.04%
===========================================
Files 395 396 +1
Lines 41826 41830 +4
Branches 3881 3881
===========================================
+ Hits 34909 34933 +24
+ Misses 6917 6897 -20
|
Marishka17
force-pushed
the
mk/update_high_level_export_sdk
branch
from
afbecc7
to
0a478d5
Compare
Co-authored-by: Maxim Zhiltsov <zhiltsov.max35@gmail.com>
|
…sets`|`backups` (cvat-ai#8255) - Fixed exporting the same dataset or backup twice in a row using high-level SDK (switched to new export API version) (related cvat-ai#8256) - Fixed exporting a dataset or backup using high-level SDK when the default project or task location refers to cloud storage - Added ability to explicitly specify location when exporting datasets and backups using high-level SDK ## Summary by CodeRabbit - **New Features** - Introduced mixins for exporting datasets and downloading backups, enhancing functionality across multiple classes. - Added a new fixture for testing tasks with specified target storage, improving test coverage. - **Bug Fixes** - Improved error handling in the file download process to ensure validity before proceeding. - **Refactor** - Restructured the downloading mechanism for better modularity and maintainability. - Removed outdated methods in favor of mixin functionality, streamlining class design. - **Tests** - Enhanced the test suite with additional scenarios and flexibility for task management and dataset downloading. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Maxim Zhiltsov <zhiltsov.max35@gmail.com>
Motivation and context
task.export_datasetfails if dataset export has been called once recently. #8256)How has this been tested?
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
Refactor
Tests