not prefetching images when not needed #8676
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 changes in the Changes
Poem
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 and nitpick comments (1)
cvat/apps/dataset_manager/task.py (1)
127-130: Enhance error handling with more specific exception messageThe error handling for
StopIterationis good, but the error message could be more descriptive.Consider adding more context to the error message:
- raise models.Job.DoesNotExist() + raise models.Job.DoesNotExist(f"Job with id {pk} does not exist")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cvat/apps/dataset_manager/task.py(1 hunks)
🔇 Additional comments (2)
cvat/apps/dataset_manager/task.py (2)
116-124: LGTM: Improved queryset handling to prevent memory leaks
The introduction of is_custom_queryset flag and the modified control flow helps prevent unnecessary caching of querysets, which aligns with the PR's objective of addressing memory leaks.
116-130: Verify performance impact of queryset changes
While the changes address memory leaks, we should verify that they don't introduce performance regressions in frequently accessed jobs.
Let's analyze the queryset usage patterns:
✅ Verification successful
Based on the search results, I can now make an informed assessment about the queryset changes and their performance impact. Let me generate the final response:
Queryset changes are safe and follow established patterns
The changes to job queryset handling are consistent with existing patterns across the codebase. Key observations:
- The modified code path in
dataset_manager/task.pyfollows the same queryset optimization patterns used in other critical paths likeengine/views.pyandquality_control/quality_reports.py - The caching behavior (
get_cached) is used selectively only when custom querysets are provided, which is the correct approach - The prefetch logic remains intact through
add_prefetch_info()which is crucial forJobAnnotationperformance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for patterns of job queryset usage that might be affected by the caching changes
# Look for job queryset usage patterns
rg -A 5 "Job\.objects" cvat/apps/
# Look for potential performance-critical paths
rg -A 5 "JobAnnotation" cvat/apps/
Length of output: 39203
Eldies
force-pushed
the
dl/no-queryset-cache
branch
from
3ca5a3f
to
e7d7860
Compare
Eldies
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8676 +/- ##
===========================================
+ Coverage 74.18% 74.20% +0.02%
===========================================
Files 401 401
Lines 43510 43511 +1
Branches 3950 3950
===========================================
+ Hits 32278 32289 +11
+ Misses 11232 11222 -10
|
|
@Eldies, Could you please provide the difference in memory usage and number of db queries (before/after the patch)? |
cvat/apps/dataset_manager/task.py
Outdated
|
|
||
| Prefetch('segment__task__label_set', queryset=label_qs), | ||
| Prefetch('segment__task__project__label_set', queryset=label_qs), | ||
| ) | ||
|
|
||
| def __init__(self, pk, *, is_prefetched=False, queryset=None): | ||
| def __init__(self, pk, *, is_prefetched: bool = False, queryset: QuerySet = None, prefetch_images: bool = True): |
There was a problem hiding this comment.
-
I guess it's not the desired approach to have both
is_prefetchedandprefetch_imagesoptions, considering they are unrelated. Additionally, the nameis_prefetcheddoesn't accurately reflect its purpose, as it appears to create a lock for the database row. -
I wonder if it would be better to set
prefetch_images=Falseby default and explicitly passprefetch_images=Trueonly when needed?
@zhiltsov-max, I'm also unsure why we lock the job row only from TaskAnnotation. For instance, why don't we lock the row when updating job annotations directly?
I'm talking about this code:
if is_prefetched:
self.db_job: models.Job = queryset.select_related(
'segment__task'
).select_for_update().get(id=pk)
else:
self.db_job: models.Job = get_cached(queryset, pk=int(pk))
There was a problem hiding this comment.
I can't say, this conditional logic was added in ba74709.
For instance, why don't we lock the row when updating job annotations directly?
I'm not sure what you mean here. Could you phrase it in more detail? I can guess that the whole update was supposed to be a single request, so that no lock is needed. Maybe there was some deadlock somewhere.
There was a problem hiding this comment.
set prefetch_images=False by default
There was a problem hiding this comment.
as far as I understand from https://github.com/cvat-ai/cvat/pull/5160/files#diff-ed7ab63c7c54f5d87f982240b298e7830de8e01da28b819779b44bc601db6f7bR74
is_prefetched actually is related to my prefetch_images - it was to determine whether images should be prefetched. But somewhere along the way this behaviour was broken.
select_for_update was there earlier, from (at least) ae6a489 (in cvat/apps/engine/annotation.py) and was removed for the case when images are to be prefetched
There was a problem hiding this comment.
Since is_prefetched is only used in TaskAnnotation.init_from_db and in all the other cases lock was removed two years ago and no problems emerged (?), I believe that the lock is not required and is_prefetched can be removed.
Made a commit which removes it
cvat/apps/dataset_manager/task.py
Outdated
| @@ -93,6 +94,12 @@ def add_prefetch_info(cls, queryset): | |||
| ]) | |||
| label_qs = JobData.add_prefetch_info(label_qs) | |||
|
|
|||
| task_data_queryset = models.Data.objects.select_related('video') | |||
There was a problem hiding this comment.
I guess task_data_queryset should be models.Data.objects.all() by default and select_related('video') also should be called only when we need to obtain video data details. (In my case, the number of database queries is also reduced by 2 * len(jobs) for video tasks)
cvat/apps/dataset_manager/task.py
Outdated
| @@ -1018,7 +1022,7 @@ def put_job_data(pk, data): | |||
| @plugin_decorator | |||
| @transaction.atomic | |||
| def patch_job_data(pk, data, action): | |||
There was a problem hiding this comment.
How about get_job_data, put_job_data? Could you please check all places where JobAnnotation is used? (not only in OSS)
There was a problem hiding this comment.
checked them and set prefetch_images=True when needed
changelog.d/20241113_130658_dmitrii.lavrukhin_no_queryset_cache.md
Outdated
Show resolved
Hide resolved
Eldies
force-pushed
the
dl/no-queryset-cache
branch
from
0927df6
to
399cae8
Compare
Eldies
force-pushed
the
dl/no-queryset-cache
branch
2 times, most recently
from
9e2f4f0
to
1c2b19d
Compare
Eldies
force-pushed
the
dl/no-queryset-cache
branch
from
1c2b19d
to
a091103
Compare
…e.md Co-authored-by: Maria Khrustaleva <maria@cvat.ai>
cvat/apps/dataset_manager/task.py
Outdated
|
|
||
| Prefetch('segment__task__label_set', queryset=label_qs), | ||
| Prefetch('segment__task__project__label_set', queryset=label_qs), | ||
| ) | ||
|
|
||
| def __init__(self, pk, *, is_prefetched=False, queryset=None): | ||
| def __init__(self, pk, *, is_prefetched: bool = False, queryset: QuerySet = None, prefetch_images: bool = False): |
There was a problem hiding this comment.
| def __init__(self, pk, *, is_prefetched: bool = False, queryset: QuerySet = None, prefetch_images: bool = False): | |
| def __init__(self, pk, *, is_prefetched: bool = False, queryset: QuerySet | None = None, prefetch_images: bool = False): |
|
Motivation and context
While importing annotations to task, all jobs of the task are loaded from db to ram. Related data is prefetched, specifically all image models which belong to the task.
As a result, each job holds its own copy of all the image models.
If there are a lot of jobs and a lot of images in the task, a lot of memory can be occupied.
And images are not utilised on annotations import/delete. Hence - do not prefetch images in these cases.
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
Refactor
JobAnnotationclass for clearer control flow and initialization.