Skip to content

Commit

Permalink
Fix dataset downloading (#7864)
Browse files Browse the repository at this point in the history
<!-- Raise an issue to propose your change
(https://github.com/cvat-ai/cvat/issues).
It helps to avoid duplication of efforts from multiple independent
contributors.
Discuss your ideas with maintainers to be sure that changes will be
approved and merged.
Read the [Contribution guide](https://docs.cvat.ai/docs/contributing/).
-->

<!-- Provide a general summary of your changes in the Title above -->

### Motivation and context
<!-- Why is this change required? What problem does it solve? If it
fixes an open
issue, please link to the issue here. Describe your changes in detail,
add
screenshots. -->

This PR addresses several problems:
- when requesting a dataset download, it's possible to get the 500 error
with the message "The result file does not exist in export cache", which
isn't expected for this request
- when downloading the dataset the same error can be obtained if the
file is requested right before the cache expiration
- there are several
[TOCTOU](https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use)
bugs related to dataset cache file existence checks
- under some conditions, it's possible that the export job is never
started
- the finished RQ jobs were removed automatically on result reporting
(after the client requested the result). This made it hard to debug
problems for admins, as the jobs were often removed already

This PR fixes the problems by the following:
- introduced dataset cache file locking (via redis) during reading,
writing, and removal
- the 500 error is changed to automatic reexporting attempt on export
status request
- the 500 error is changed to 404 when the file is not available for
downloading
- the exported files are now have different names for each instance
update time
- the lifetime of the exported files is now automatically prolonged on
each export request for the file (given the export is still valid)
- the deferred export jobs are now checked to have ghost dependencies.
If so, the requested job is restarted
- added several environment variables for configuration
- <s>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)</s>

### How has this been tested?
<!-- Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc. -->

### Checklist
<!-- Go over all the following points, and put an `x` in all the boxes
that apply.
If an item isn't applicable for some reason, then ~~explicitly
strikethrough~~ the whole
line. If you don't do that, GitHub will show incorrect progress for the
pull request.
If you're unsure about any of these, don't hesitate to ask. We're here
to help! -->
- [ ] I submit my changes into the `develop` branch
- [ ] I have created a changelog fragment <!-- see top comment in
CHANGELOG.md -->
- [ ] I have updated the documentation accordingly
- [ ] I have added tests to cover my changes
- [ ] I have linked related issues (see [GitHub docs](

https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword))
- [ ] I have increased versions of npm packages if it is necessary

([cvat-canvas](https://github.com/cvat-ai/cvat/tree/develop/cvat-canvas#versioning),

[cvat-core](https://github.com/cvat-ai/cvat/tree/develop/cvat-core#versioning),

[cvat-data](https://github.com/cvat-ai/cvat/tree/develop/cvat-data#versioning)
and

[cvat-ui](https://github.com/cvat-ai/cvat/tree/develop/cvat-ui#versioning))

### License

- [ ] I submit _my code changes_ under the same [MIT License](
https://github.com/cvat-ai/cvat/blob/develop/LICENSE) that covers the
project.
  Feel free to contact the maintainers if that's a concern.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Improved reliability of file handling during export and cleanup
processes.
- Introduced new functionality for managing export cache locks and
directories.

- **Bug Fixes**
- Addressed race conditions in concurrent export and cleanup operations.

- **Dependencies**
- Updated multiple packages to their latest versions for enhanced
security and performance:
    - `cryptography` to `42.0.7`
    - `django` to `4.2.13`
    - `django-health-check` to `3.18.2`
    - `freezegun` to `1.5.1`
    - `jinja2` to `3.1.4`
    - `limits` to `3.12.0`
    - `lxml` to `5.2.2`
    - `orjson` to `3.10.3`
    - Added `pottery` version `3.0.0`
    - Updated `tqdm` to `4.66.4`
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
zhiltsov-max authored May 28, 2024
1 parent 49b10ba commit f883838
Show file tree
Hide file tree
Showing 16 changed files with 1,264 additions and 169 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
### Fixed

- The 500 / "The result file does not exist in export cache" error
on dataset export request
(<https://github.com/cvat-ai/cvat/pull/7864>)
18 changes: 18 additions & 0 deletions cvat/apps/dataset_manager/apps.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Copyright (C) 2024 CVAT.ai Corporation
#
# SPDX-License-Identifier: MIT

from django.apps import AppConfig


class DatasetManagerConfig(AppConfig):
name = "cvat.apps.dataset_manager"

def ready(self) -> None:
from django.conf import settings

from . import default_settings

for key in dir(default_settings):
if key.isupper() and not hasattr(settings, key):
setattr(settings, key, getattr(default_settings, key))
14 changes: 14 additions & 0 deletions cvat/apps/dataset_manager/default_settings.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Copyright (C) 2024 CVAT.ai Corporation
#
# SPDX-License-Identifier: MIT

import os

DATASET_CACHE_TTL = int(os.getenv("CVAT_DATASET_CACHE_TTL", 60 * 60 * 10))
"Base lifetime for cached exported datasets, in seconds"

DATASET_CACHE_LOCK_TIMEOUT = int(os.getenv("CVAT_DATASET_CACHE_LOCK_TIMEOUT", 10))
"Timeout for cache lock acquiring, in seconds"

DATASET_EXPORT_LOCKED_RETRY_INTERVAL = int(os.getenv("CVAT_DATASET_EXPORT_LOCKED_RETRY_INTERVAL", 60))
"Retry interval for cases the export cache lock was unavailable, in seconds"
Loading

0 comments on commit f883838

Please sign in to comment.