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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8676 +/- ##
========================================
Coverage 74.20% 74.20%
========================================
Files 401 401
Lines 43504 43507 +3
Branches 3950 3950
========================================
+ Hits 32280 32284 +4
+ Misses 11224 11223 -1
|
|
|
@Eldies, Could you please provide the difference in memory usage and number of db queries (before/after the patch)? |
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.