Fix null detections in exports & API. Don't mark images as processed too soon#1312
Conversation
✅ Deploy Preview for antenna-ssec canceled.
|
✅ Deploy Preview for antenna-preview canceled.
|
✅ Deploy Preview for antenna-preview canceled.
|
|
Warning Review limit reached
More reviews will be available in 34 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR fixes issue ChangesNull-marker lifecycle and pipeline ordering
Sequence Diagram(s)sequenceDiagram
participant Pipeline as save_results
participant CD as create_detections
participant CC as create_classifications
participant CO as create_occurrences
participant CDN as create_detections (null markers)
Note over Pipeline: OLD: null markers added to results.detections BEFORE real processing
Pipeline->>CD: create_detections(results.detections including nulls)
Pipeline->>CC: create_classifications(...)
Pipeline->>CO: create_occurrences(...) ← phantom Occurrences created here
Note over Pipeline: NEW: null markers created AFTER all real processing
Pipeline->>CD: create_detections(real detections only)
Pipeline->>CC: create_classifications(...)
Pipeline->>CO: create_occurrences(...) ← no phantom Occurrences
Pipeline->>CDN: create_detections(null_detection_responses for undetected images)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a pipeline persistence ordering bug where null (bbox=None) “processed, nothing found” detection markers were created too early, causing images to be skipped on retry after downstream failures and occasionally creating phantom Occurrence rows tied only to null detections.
Changes:
- Reorders
save_resultsto persist real detections/classifications/occurrences first, then creates null detection markers in a second pass. - Ensures null detections never enter the classification/occurrence creation paths.
- Adds regression tests for “no phantom occurrence on null” and “no processed marker after failure”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
ami/ml/models/pipeline.py |
Moves null-marker creation to after real detection/classification/occurrence persistence and saves nulls via a separate create_detections call. |
ami/ml/tests.py |
Adds tests covering the phantom-occurrence regression and retry behavior after a simulated downstream failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Plan for the takeaway-review follow-up work on PR #1312: move-null-to-end + null-detection abstraction (DetectionQuerySet.valid / .null_markers, Detection.is_null_marker, Detection.build_null_marker) + sweep call sites + tighten OccurrenceQuerySet.valid + cleanup command for project 171. Captures rationale for splitting transaction.atomic into a separate follow-up PR (PR-1261 scar). Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude says: Pushed takeaway-review follow-up commits on top of the original fix. Summary of what's new since the original push: Commits added (e5f004a is the new HEAD):
What's NOT in this PR (deferred to follow-up):
Local test status:
Manual e2e on a dev deployment is queued next. |
|
Claude says: E2E validation on a dev deployment complete. All three paths pass. Path 1 — Happy path (async_api job)Job completed in 28.21s. 10 images, 68 detections, 121 classifications, 0 failed. NATS path through the new ordering works end-to-end. Path 2 — Broker outage simulationLive Pre-fix behavior would have shown Path 3 — Calc-field DB errorSame shape, with Same result — null marker never persisted because the failure runs before null creation in the new order. Cleanup command dry-runRan on dev DB to validate idempotent dry-run behavior (the real project-171 cleanup will run after merge against a prod DB): The 112 phantoms match the delta between Branch is in good shapeTest counts:
Ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ami/main/api/views.py (1)
613-628:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
has_detectionsstill disagrees withwith_detections.This prefetch correctly drops null markers, but
filter_by_has_detections()still checks for anyDetectionrow./captures/?has_detections=true&with_detections=truecan therefore return captures whosefiltered_detectionsis empty.Suggested fix
def filter_by_has_detections(self, queryset: QuerySet) -> QuerySet: has_detections = self.request.query_params.get("has_detections") if has_detections is not None: has_detections = BooleanField(required=False).clean(has_detections) queryset = queryset.annotate( - has_detections=models.Exists(Detection.objects.filter(source_image=models.OuterRef("pk"))), + has_detections=models.Exists( + Detection.objects.valid().filter(source_image=models.OuterRef("pk")) + ), ).filter(has_detections=has_detections) return queryset🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/api/views.py` around lines 613 - 628, The has_detections check currently looks for any Detection row while filtered_detections drops null/non-qualifying rows, causing mismatches; update filter_by_has_detections to only count detections that meet the same criteria used for the prefetch (i.e. use the annotated Detection queryset created from Detection.objects.valid() that includes occurrence_meets_criteria or an Exists/Subquery against qualifying_occurrence_ids) so the filter requires existence of at least one Detection with occurrence_meets_criteria=True (or occurrence_id in qualifying_occurrence_ids and score >= score) rather than any Detection row; adjust the logic referencing filter_by_has_detections, Detection.objects.valid(), occurrence_meets_criteria, qualifying_occurrence_ids and filtered_detections accordingly.ami/ml/models/pipeline.py (1)
442-459:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDedup the legacy
bbox=[]null marker here too.This lookup only matches canonical
bbox IS NULLrows. If the image already has a legacy empty-list sentinel for the same algorithm, reprocessing will create a second null marker instead of reusing the existing one.Suggested fix
- existing_detection = Detection.objects.filter( - source_image=source_image, - bbox__isnull=True, - detection_algorithm=detection_algo, - ).first() + existing_detection = ( + Detection.objects.filter( + source_image=source_image, + detection_algorithm=detection_algo, + ) + .null_markers() + .first() + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/ml/models/pipeline.py` around lines 442 - 459, The dedupe query only checks for bbox__isnull=True and misses legacy empty-list sentinel rows, causing duplicate null markers; update the existing_detection lookup in the pipeline to include both canonical NULL and the legacy sentinel by OR-ing bbox__isnull=True with bbox equal to the legacy sentinel (use Detection.NULL_BBOX or the empty-list value) — e.g., replace the single filter(...) call that sets existing_detection with a query using Q(...) or bbox__in=[None, Detection.NULL_BBOX] while keeping the same source_image and detection_algorithm (refer to Detection, existing_detection, detection_algo, and detection_resp.algorithm.key).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ami/main/management/commands/cleanup_null_only_occurrences.py`:
- Around line 76-82: The success message currently uses the delete() return
values (null_deleted, phantom_deleted) which include cascaded rows; replace
those with the pre-calculated counters (null_count and phantom_count) when
writing the final message—inside the same transaction.atomic() block after
orphan_null_markers.delete() and phantom_occs.delete(), call
self.stdout.write(self.style.SUCCESS(f"Deleted {phantom_count} phantom
occurrences and {null_count} orphan null markers.")) so the log reports only the
intended occurrence counts.
In `@ami/main/models.py`:
- Around line 4165-4169: The sampling for detections_only is out of sync: the
annotate uses source_images_with_detections_count with
filter=~null_detections_q("images__detections__") but
SourceImageCollection.sample_detections_only() still uses the simpler
detections__isnull=False and can include null-marker-only images; update
sample_detections_only to apply the same filter logic (i.e. use
null_detections_q("images__detections__") negated or reuse the same annotated
queryset/condition) so the sampled collection matches the
source_images_with_detections_count semantics and only includes images that the
new counter considers as having detections.
- Around line 2837-2844: The null-marker builder currently sets
timestamp=timezone.now() in Detection.build_null_marker which forces the marker
to use processing time; remove that explicit timestamp (or set timestamp=None)
so the Detection instance is created without a timestamp and Detection.save()
can backfill the correct capture timestamp; update the build_null_marker
constructor call that uses cls(NULL_BBOX, source_image, detection_algorithm,
...) accordingly and keep other fields (source_image, bbox=cls.NULL_BBOX,
detection_algorithm) unchanged.
---
Outside diff comments:
In `@ami/main/api/views.py`:
- Around line 613-628: The has_detections check currently looks for any
Detection row while filtered_detections drops null/non-qualifying rows, causing
mismatches; update filter_by_has_detections to only count detections that meet
the same criteria used for the prefetch (i.e. use the annotated Detection
queryset created from Detection.objects.valid() that includes
occurrence_meets_criteria or an Exists/Subquery against
qualifying_occurrence_ids) so the filter requires existence of at least one
Detection with occurrence_meets_criteria=True (or occurrence_id in
qualifying_occurrence_ids and score >= score) rather than any Detection row;
adjust the logic referencing filter_by_has_detections,
Detection.objects.valid(), occurrence_meets_criteria, qualifying_occurrence_ids
and filtered_detections accordingly.
In `@ami/ml/models/pipeline.py`:
- Around line 442-459: The dedupe query only checks for bbox__isnull=True and
misses legacy empty-list sentinel rows, causing duplicate null markers; update
the existing_detection lookup in the pipeline to include both canonical NULL and
the legacy sentinel by OR-ing bbox__isnull=True with bbox equal to the legacy
sentinel (use Detection.NULL_BBOX or the empty-list value) — e.g., replace the
single filter(...) call that sets existing_detection with a query using Q(...)
or bbox__in=[None, Detection.NULL_BBOX] while keeping the same source_image and
detection_algorithm (refer to Detection, existing_detection, detection_algo, and
detection_resp.algorithm.key).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 277e1e60-2244-4c0e-9ce4-f2f61f800478
📒 Files selected for processing (8)
ami/main/api/views.pyami/main/management/commands/cleanup_null_only_occurrences.pyami/main/models.pyami/main/tests.pyami/ml/models/pipeline.pyami/ml/tests.pydocs/claude/planning/pr-1312-null-marker-followup.mddocs/claude/sessions/2026-05-19-pr-1312-premptive-processed-marker.md
✅ Files skipped from review due to trivial changes (2)
- docs/claude/sessions/2026-05-19-pr-1312-premptive-processed-marker.md
- docs/claude/planning/pr-1312-null-marker-followup.md
Five drift / quick-win fixes from PR #1312 review: 1. ami/main/api/views.py — filter_by_has_detections now uses Detection.objects.valid() so /captures/?has_detections=true agrees with the with_detections prefetch (which already drops null markers). Without this, has_detections=true could return captures whose filtered_detections array was empty. 2. ami/main/models.py — SourceImageCollection.sample_detections_only now samples by Detection.objects.valid() instead of detections__isnull=False, matching the tightened source_images_with_detections_count annotation. A detections_only collection no longer admits images that have only null markers. 3. ami/main/models.py — Detection.build_null_marker drops timestamp=timezone.now(). Detection.save() backfills timestamp from the source image's capture time, so the marker sorts/filters by capture time rather than processing time. Test asserts timestamp is None on the builder output. 4. ami/ml/models/pipeline.py — get_or_create_detection null-marker dedup now goes through .null_markers() so legacy bbox=[] sentinels from older runs are re-used. The lookup is still detection_algorithm-scoped, so the wider .null_markers() predicate stays narrow at the call site (no false matches across algorithms). 5. ami/main/management/commands/cleanup_null_only_occurrences.py — success message now reports the pre-calculated phantom_count / null_count instead of the .delete() return tuple, which includes cascade-deleted rows and would mislead the operator about what the command targeted. 68/68 in ami.main null-marker tests + ami.ml.tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude says: Pushed CodeRabbit feedback round in Inline-thread items (3) — resolved on each thread:
Outside-diff items (2) — applied in same commit:
68/68 in |
|
Claude says: Cleanup command validated on a dev deployment (project 9) — 112 phantom occurrences + 112 orphan null markers deleted, post-cleanup counts match |
mihow
changed the title
|
Claude says: (posted on Michael's behalf — part of a structural review pass over the four open PSv2-readiness PRs: #1276, #1279, #1312, #1324, toward the umbrella #910.) This one reads as the strongest of the four. The fix is structural rather than a delay: the null-marker write moves to the absolute end of Two things seem worth a look before merge / before running the cleanup command at scale:
Tiny cleanup, low priority: Test suite state and the e2e runs across the three failure paths look good. With item 1 patched and a human eyeball on item 2, this seems ready — and given #1312 fixes the "lost work under async" class, it's arguably the first of the four PSv2-readiness PRs that should land. |
mihow
added
the
PSv2
Issue #1310: null detections (empty-bbox sentinels marking "image processed, nothing found") were created before create_detections / create_classifications / create_and_update_occurrences_for_detections ran. Two consequences: 1. If any of those downstream steps failed, the image was already flagged as processed via the null marker — filter_processed_images would skip it on the next run, leaving the image permanently in a "processed but no detections" state. Observed on project 171 (400 captures with only null detections). 2. create_and_update_occurrences_for_detections iterated every detection including nulls, so each null marker spawned a phantom Occurrence with determination=NULL. Fix in ami/ml/models/pipeline.py save_results: - Run create_detections / create_classifications / create_and_update_occurrences on the real DetectionResponses only. - After those succeed, build null DetectionResponses for images that ended up without any detections and persist them via a second create_detections call. - Null responses never enter the classification / occurrence loops, so no phantom Occurrence is created even in the happy path. Tests in ami/ml/tests.py TestPipeline: - test_null_detection_does_not_create_phantom_occurrence: asserts the happy path "pipeline found nothing" creates the null marker but no Occurrence. - test_captures_not_marked_processed_after_failure: asserts that when a downstream step (create_classifications) raises, the image without a real detection is left unmarked and filter_processed_images re-yields it. Co-Authored-By: Claude <noreply@anthropic.com>
Plan for the takeaway-review follow-up work on PR #1312: move-null-to-end + null-detection abstraction (DetectionQuerySet.valid / .null_markers, Detection.is_null_marker, Detection.build_null_marker) + sweep call sites + tighten OccurrenceQuerySet.valid + cleanup command for project 171. Captures rationale for splitting transaction.atomic into a separate follow-up PR (PR-1261 scar). Co-Authored-By: Claude <noreply@anthropic.com>
Five drift / quick-win fixes from PR #1312 review: 1. ami/main/api/views.py — filter_by_has_detections now uses Detection.objects.valid() so /captures/?has_detections=true agrees with the with_detections prefetch (which already drops null markers). Without this, has_detections=true could return captures whose filtered_detections array was empty. 2. ami/main/models.py — SourceImageCollection.sample_detections_only now samples by Detection.objects.valid() instead of detections__isnull=False, matching the tightened source_images_with_detections_count annotation. A detections_only collection no longer admits images that have only null markers. 3. ami/main/models.py — Detection.build_null_marker drops timestamp=timezone.now(). Detection.save() backfills timestamp from the source image's capture time, so the marker sorts/filters by capture time rather than processing time. Test asserts timestamp is None on the builder output. 4. ami/ml/models/pipeline.py — get_or_create_detection null-marker dedup now goes through .null_markers() so legacy bbox=[] sentinels from older runs are re-used. The lookup is still detection_algorithm-scoped, so the wider .null_markers() predicate stays narrow at the call site (no false matches across algorithms). 5. ami/main/management/commands/cleanup_null_only_occurrences.py — success message now reports the pre-calculated phantom_count / null_count instead of the .delete() return tuple, which includes cascade-deleted rows and would mislead the operator about what the command targeted. 68/68 in ami.main null-marker tests + ami.ml.tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
force-pushed
the
fix/premptive-processed-marker
branch
from
dda794f to
2136405
Compare
Adds test_null_marker_not_persisted_when_broker_dispatch_fails to TestPipeline. Patches create_detection_images.delay to raise, asserts the unmatched image has no null marker persisted and that filter_processed_images yields it for re-processing. Verified RED against current ordering — null persistence still runs before delay, so the assertion fails. Next commit moves null persistence to the absolute final step. Co-Authored-By: Claude <noreply@anthropic.com>
Closes the failure window the previous fix left open: null markers were persisted after real-detection / classification / occurrence saves but BEFORE source_image.save, create_detection_images.delay, update_calculated_fields_for_events, and Deployment.update_calculated_fields. A raise in any of those four steps (broker outage, DB error) still left the image flagged as processed. Null markers now run as the last write in save_results so they only persist when every prior step succeeds. Remaining failure window is the return statement. Makes RED test from prior commit pass. Co-Authored-By: Claude <noreply@anthropic.com>
Introduces a single source of truth for "this detection row is a sentinel that records that an algorithm ran against an image and found nothing": - Detection.NULL_BBOX = None (canonical bbox value for new null markers) - Detection.is_null_marker (recognises both bbox=None and legacy bbox=[]) - Detection.build_null_marker(source_image, detection_algorithm) classmethod - DetectionQuerySet.valid() — consumer default, excludes null markers - DetectionQuerySet.null_markers() — narrow, for "has this image been processed?" checks (renamed from .null_detections()) valid() is named to grow: future predicates to fold in include soft-delete tombstones, detections missing an algorithm reference, and detections missing classifications. Consumers asking "give me detections" should default to .valid(). Adds TestDetectionNullMarker covering: is_null_marker for bbox=None / bbox=[] / real bbox, build_null_marker field setup, and disjointness of .valid() / .null_markers() over a fixture with all three row types. Next commit sweeps existing inline NULL_DETECTIONS_FILTER usage to the new API. Co-Authored-By: Claude <noreply@anthropic.com>
…null_markers() Migrates 7 inline NULL_DETECTIONS_FILTER usages to the new manager methods: - Detection.objects.exclude(NULL_DETECTIONS_FILTER) → .valid() - self.detections.exclude(NULL_DETECTIONS_FILTER) → self.detections.all().valid() - subquery .exclude(NULL_DETECTIONS_FILTER) → .valid() - aggregate filter at SourceImageCollectionQuerySet.with_source_images_with_detections_count was the drifted inline ~Q(...bbox__isnull=True) & ~Q(...bbox=[]); now uses a new null_detections_q(prefix) helper for relation-prefixed Q expressions. Touched: - ami/main/models.py: Deployment.get_detections_count, Event.get_detections_count, SourceImage.create_occurrences_from_detections, _annotate_detections_count_subquery, SourceImageCollectionQuerySet.with_source_images_with_detections_count - ami/main/api/views.py: OccurrenceViewSet prefetch_queryset, DetectionViewSet.queryset - ami/ml/models/pipeline.py: filter_processed_images null-only and unclassified checks NULL_DETECTIONS_FILTER constant is retained at module level. Direct get_or_create_detection lookup keeps bbox__isnull=True (algorithm-scoped, narrower than .null_markers() which also includes legacy bbox=[] from other pipelines); added a comment pointing readers to Detection.NULL_BBOX for the canonical sentinel. 176/176 tests in ami.ml, ami.main.TestDetectionNullMarker, ami.jobs pass. Co-Authored-By: Claude <noreply@anthropic.com>
OccurrenceQuerySet.valid() previously only excluded occurrences with no detections at all. Field bug from Issue #1310 created two new phantom shapes that still leaked to the API: 1. Occurrences whose only detections are null-marker sentinels (no real bounding box backing them). 2. Occurrences with determination__isnull=True. valid() now requires at least one .valid() Detection (real, non-null) AND a non-null determination. Built on top of the new Detection.objects.valid() helper so both layers stay in sync as the predicate grows (soft-delete, missing algo, etc.). Downstream callers updated automatically: ami/exports/format_types.py and OccurrenceViewSet.get_queryset both invoke OccurrenceQuerySet.valid and will now filter out the project-171 phantoms once existing rows are cleaned up (next commit). Adds TestOccurrenceValidQuerySet covering all three exclusion shapes. Co-Authored-By: Claude <noreply@anthropic.com>
One-shot per-project cleanup for the Issue #1310 field bug. Deletes: - Phantom occurrences (no valid detections OR null determination) - Orphan null-marker Detection rows on source images with no real detections After running, the affected source images become eligible for re-processing by filter_processed_images on the next ML run. Dry-run by default; pass --commit to delete. python manage.py cleanup_null_only_occurrences --project 171 # dry-run python manage.py cleanup_null_only_occurrences --project 171 --commit Idempotent — re-running on a cleaned project reports zero candidates and exits without touching the database. Adds TestCleanupNullOnlyOccurrencesCommand covering dry-run, commit, and idempotency. Valid occurrences and null markers on images with at least one real detection are explicitly preserved. Co-Authored-By: Claude <noreply@anthropic.com>
Five drift / quick-win fixes from PR #1312 review: 1. ami/main/api/views.py — filter_by_has_detections now uses Detection.objects.valid() so /captures/?has_detections=true agrees with the with_detections prefetch (which already drops null markers). Without this, has_detections=true could return captures whose filtered_detections array was empty. 2. ami/main/models.py — SourceImageCollection.sample_detections_only now samples by Detection.objects.valid() instead of detections__isnull=False, matching the tightened source_images_with_detections_count annotation. A detections_only collection no longer admits images that have only null markers. 3. ami/main/models.py — Detection.build_null_marker drops timestamp=timezone.now(). Detection.save() backfills timestamp from the source image's capture time, so the marker sorts/filters by capture time rather than processing time. Test asserts timestamp is None on the builder output. 4. ami/ml/models/pipeline.py — get_or_create_detection null-marker dedup now goes through .null_markers() so legacy bbox=[] sentinels from older runs are re-used. The lookup is still detection_algorithm-scoped, so the wider .null_markers() predicate stays narrow at the call site (no false matches across algorithms). 5. ami/main/management/commands/cleanup_null_only_occurrences.py — success message now reports the pre-calculated phantom_count / null_count instead of the .delete() return tuple, which includes cascade-deleted rows and would mislead the operator about what the command targeted. 68/68 in ami.main null-marker tests + ami.ml.tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
force-pushed
the
fix/premptive-processed-marker
branch
from
2136405 to
8e4bbf3
Compare
mihow
changed the title
…-determination phantom Rename the 'orphan' terminology for left-behind null-marker detections to 'dangling' (the standard term for a reference with no live target, already used for storage blobs in this module) across the cleanup command's output, the valid() docstring, and the tests. Also add a fixture case to the cleanup command test: an occurrence with a real detection but no determination. This is the other arm of the tightened valid() definition, and the test now asserts the command treats it as a phantom and deletes it.
…ntinel The cleanup_null_only_occurrences command deleted any occurrence excluded by Occurrence.valid(), which also matches occurrences that have a real, classified detection but a missing determination. Deleting those SET_NULLs the detection's occurrence FK (Detection.occurrence is on_delete=SET_NULL) and strands a classified detection on an image that filter_processed_images then skips forever. Narrow the command's phantom predicate to "has no valid detection backing it" so those partial-write occurrences are preserved; the test now asserts they survive with their detection's FK intact. Also drop the legacy bbox=[] null-marker form. It has zero rows in prod, demo, and staging, no code writes it (the field defaults to SQL NULL and new markers use Detection.NULL_BBOX = None), so the dual-form predicate was dead weight. The sole sentinel is now bbox IS NULL, and NULL_DETECTIONS_FILTER is defined via null_detections_q() so the constant and helper cannot drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ami/ml/models/pipeline.py (1)
1064-1082: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSplit incoming null detections out before the early persistence path.
The new final block only delays app-generated null markers. If the processing service sends a
DetectionResponsewithbbox=None, line 1064 still persists it before classifications, occurrence updates, crop dispatch, and calculated-field updates—and line 1079 can still create a phantom occurrence for it. Filterresults.detectionsto real detections for the early path, then persist both incoming and generated null markers in the final block.Proposed fix
+ real_detection_responses = [detection for detection in results.detections if detection.bbox is not None] + incoming_null_detection_responses = [detection for detection in results.detections if detection.bbox is None] + detections = create_detections( - detections=results.detections, + detections=real_detection_responses, algorithms_known=algorithms_known, logger=job_logger, ) classifications = create_classifications( detections=detections, - detection_responses=results.detections, + detection_responses=real_detection_responses, algorithms_known=algorithms_known, logger=job_logger, ) @@ - null_detection_responses = create_null_detections_for_undetected_images( + generated_null_detection_responses = create_null_detections_for_undetected_images( results=results, detection_algorithm=detection_algorithm, logger=job_logger, ) + null_detection_responses = incoming_null_detection_responses + generated_null_detection_responses create_detections( detections=null_detection_responses, algorithms_known=algorithms_known,Also applies to: 1102-1116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/ml/models/pipeline.py` around lines 1064 - 1082, Filter out null detections (those with bbox=None) from results.detections before the early processing path starting at create_detections(). Create a filtered list of only real detections and pass this filtered list to create_detections(), create_classifications(), and create_and_update_occurrences_for_detections() instead of the raw results.detections. Preserve the original results.detections (including null markers) for the final persistence block that handles both incoming and generated null markers, ensuring null detections bypass the early classification, occurrence creation, and crop dispatch steps.
🧹 Nitpick comments (2)
ami/main/tests.py (1)
6041-6070: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit pixel-coordinate bboxes in new null-marker tests.
These tests currently use
bbox=[0.0, 0.0, 1.0, 1.0], which reads like normalized coordinates. Switching to clear pixel literals (e.g.[10, 10, 20, 20]) would better match repo conventions and reduce ambiguity in future refactors/validation changes.♻️ Suggested test-literal cleanup
- bbox=[0.0, 0.0, 1.0, 1.0], + bbox=[10, 10, 20, 20],Based on learnings,
Detection.bboxin this repository uses absolute pixel coordinate space (not normalized [0–1] floats).Also applies to: 6115-6150, 6209-6257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/tests.py` around lines 6041 - 6070, In the test methods test_is_null_marker_false_for_real_detection, test_build_null_marker_sets_canonical_fields, and test_valid_and_null_markers_are_disjoint_and_complete (and the other test ranges noted), replace the bbox values of [0.0, 0.0, 1.0, 1.0] with explicit pixel-coordinate values like [10, 10, 20, 20] to accurately reflect that Detection.bbox uses absolute pixel coordinate space rather than normalized coordinates, which will improve clarity and consistency with repository conventions.Source: Learnings
ami/ml/tests.py (1)
1261-1262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the canonical null-marker queryset in these assertions.
Line 1261, Line 1307, and Line 1347 hardcode
bbox__isnull=True. That bypasses the new null-marker abstraction and can miss legacybbox=[]sentinels covered by.null_markers(). Align these checks with the canonical API.♻️ Proposed refactor
- null_dets = image.detections.filter(bbox__isnull=True) + null_dets = Detection.objects.null_markers().filter(source_image=image) @@ - null_dets = image_without_real.detections.filter(bbox__isnull=True) + null_dets = Detection.objects.null_markers().filter(source_image=image_without_real) @@ - null_dets = image_without_real.detections.filter(bbox__isnull=True) + null_dets = Detection.objects.null_markers().filter(source_image=image_without_real)Also applies to: 1307-1310, 1347-1350
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/ml/tests.py` around lines 1261 - 1262, Replace the hardcoded bbox__isnull=True filter in the three assertion locations (around lines 1261, 1307, and 1347) with the canonical .null_markers() method. Instead of using image.detections.filter(bbox__isnull=True), change these to use image.detections.null_markers() which properly handles both the new null-marker abstraction and legacy bbox=[] sentinels. This ensures the assertions correctly validate null markers through the proper API abstraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ami/main/api/views.py`:
- Line 1848: The `use_aggregation` variable assignment currently only checks for
the presence of the "collection" key in request.query_params without validating
whether it has a non-empty value. This causes unnecessary aggregation mode
activation when collection is passed as a blank query parameter (e.g.,
?collection=). Modify the condition to not only check if "collection" is present
in request.query_params but also validate that it contains a non-empty/valid
value before setting use_aggregation to true.
In `@ami/main/models.py`:
- Line 2445: The fetch_image_content() function lacks URL validation, creating
an SSRF vulnerability. Add URL validation logic to fetch_image_content() that
blocks unsafe URL schemes (file://, ftp://, etc.), rejects private and
link-local IP ranges (127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16,
169.254.0.0/16), and disables HTTP redirects or validates redirect targets
against the same restrictions. Additionally, add validation in the
S3StorageSourceSerializer to validate the public_base_url field at the source to
prevent malicious values from being set in the first place.
In `@ami/ml/models/pipeline.py`:
- Around line 121-128: The `iter(images)` call at line 121 forces Django
QuerySets into memory when processing large collections. Create a helper
function that detects if `images` is a Django QuerySet and uses
`iterator(chunk_size=batch_size)` to stream results instead of materializing
them all at once. If `images` is not a QuerySet, fall back to using
`iter(images)`. Update the `image_iter` assignment to use this helper function
so that QuerySets remain lazy-evaluated throughout the iteration in the while
loop.
---
Outside diff comments:
In `@ami/ml/models/pipeline.py`:
- Around line 1064-1082: Filter out null detections (those with bbox=None) from
results.detections before the early processing path starting at
create_detections(). Create a filtered list of only real detections and pass
this filtered list to create_detections(), create_classifications(), and
create_and_update_occurrences_for_detections() instead of the raw
results.detections. Preserve the original results.detections (including null
markers) for the final persistence block that handles both incoming and
generated null markers, ensuring null detections bypass the early
classification, occurrence creation, and crop dispatch steps.
---
Nitpick comments:
In `@ami/main/tests.py`:
- Around line 6041-6070: In the test methods
test_is_null_marker_false_for_real_detection,
test_build_null_marker_sets_canonical_fields, and
test_valid_and_null_markers_are_disjoint_and_complete (and the other test ranges
noted), replace the bbox values of [0.0, 0.0, 1.0, 1.0] with explicit
pixel-coordinate values like [10, 10, 20, 20] to accurately reflect that
Detection.bbox uses absolute pixel coordinate space rather than normalized
coordinates, which will improve clarity and consistency with repository
conventions.
In `@ami/ml/tests.py`:
- Around line 1261-1262: Replace the hardcoded bbox__isnull=True filter in the
three assertion locations (around lines 1261, 1307, and 1347) with the canonical
.null_markers() method. Instead of using
image.detections.filter(bbox__isnull=True), change these to use
image.detections.null_markers() which properly handles both the new null-marker
abstraction and legacy bbox=[] sentinels. This ensures the assertions correctly
validate null markers through the proper API abstraction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0422a8e7-8613-4eda-b223-3416334f77a5
📒 Files selected for processing (6)
ami/main/api/views.pyami/main/management/commands/cleanup_null_only_occurrences.pyami/main/models.pyami/main/tests.pyami/ml/models/pipeline.pyami/ml/tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ami/main/management/commands/cleanup_null_only_occurrences.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ami/ml/models/pipeline.py (1)
1064-1082: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSplit incoming null detections out before the early persistence path.
The new final block only delays app-generated null markers. If the processing service sends a
DetectionResponsewithbbox=None, line 1064 still persists it before classifications, occurrence updates, crop dispatch, and calculated-field updates—and line 1079 can still create a phantom occurrence for it. Filterresults.detectionsto real detections for the early path, then persist both incoming and generated null markers in the final block.Proposed fix
+ real_detection_responses = [detection for detection in results.detections if detection.bbox is not None] + incoming_null_detection_responses = [detection for detection in results.detections if detection.bbox is None] + detections = create_detections( - detections=results.detections, + detections=real_detection_responses, algorithms_known=algorithms_known, logger=job_logger, ) classifications = create_classifications( detections=detections, - detection_responses=results.detections, + detection_responses=real_detection_responses, algorithms_known=algorithms_known, logger=job_logger, ) @@ - null_detection_responses = create_null_detections_for_undetected_images( + generated_null_detection_responses = create_null_detections_for_undetected_images( results=results, detection_algorithm=detection_algorithm, logger=job_logger, ) + null_detection_responses = incoming_null_detection_responses + generated_null_detection_responses create_detections( detections=null_detection_responses, algorithms_known=algorithms_known,Also applies to: 1102-1116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/ml/models/pipeline.py` around lines 1064 - 1082, Filter out null detections (those with bbox=None) from results.detections before the early processing path starting at create_detections(). Create a filtered list of only real detections and pass this filtered list to create_detections(), create_classifications(), and create_and_update_occurrences_for_detections() instead of the raw results.detections. Preserve the original results.detections (including null markers) for the final persistence block that handles both incoming and generated null markers, ensuring null detections bypass the early classification, occurrence creation, and crop dispatch steps.
🧹 Nitpick comments (2)
ami/main/tests.py (1)
6041-6070: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit pixel-coordinate bboxes in new null-marker tests.
These tests currently use
bbox=[0.0, 0.0, 1.0, 1.0], which reads like normalized coordinates. Switching to clear pixel literals (e.g.[10, 10, 20, 20]) would better match repo conventions and reduce ambiguity in future refactors/validation changes.♻️ Suggested test-literal cleanup
- bbox=[0.0, 0.0, 1.0, 1.0], + bbox=[10, 10, 20, 20],Based on learnings,
Detection.bboxin this repository uses absolute pixel coordinate space (not normalized [0–1] floats).Also applies to: 6115-6150, 6209-6257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/tests.py` around lines 6041 - 6070, In the test methods test_is_null_marker_false_for_real_detection, test_build_null_marker_sets_canonical_fields, and test_valid_and_null_markers_are_disjoint_and_complete (and the other test ranges noted), replace the bbox values of [0.0, 0.0, 1.0, 1.0] with explicit pixel-coordinate values like [10, 10, 20, 20] to accurately reflect that Detection.bbox uses absolute pixel coordinate space rather than normalized coordinates, which will improve clarity and consistency with repository conventions.Source: Learnings
ami/ml/tests.py (1)
1261-1262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the canonical null-marker queryset in these assertions.
Line 1261, Line 1307, and Line 1347 hardcode
bbox__isnull=True. That bypasses the new null-marker abstraction and can miss legacybbox=[]sentinels covered by.null_markers(). Align these checks with the canonical API.♻️ Proposed refactor
- null_dets = image.detections.filter(bbox__isnull=True) + null_dets = Detection.objects.null_markers().filter(source_image=image) @@ - null_dets = image_without_real.detections.filter(bbox__isnull=True) + null_dets = Detection.objects.null_markers().filter(source_image=image_without_real) @@ - null_dets = image_without_real.detections.filter(bbox__isnull=True) + null_dets = Detection.objects.null_markers().filter(source_image=image_without_real)Also applies to: 1307-1310, 1347-1350
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/ml/tests.py` around lines 1261 - 1262, Replace the hardcoded bbox__isnull=True filter in the three assertion locations (around lines 1261, 1307, and 1347) with the canonical .null_markers() method. Instead of using image.detections.filter(bbox__isnull=True), change these to use image.detections.null_markers() which properly handles both the new null-marker abstraction and legacy bbox=[] sentinels. This ensures the assertions correctly validate null markers through the proper API abstraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ami/main/api/views.py`:
- Line 1848: The `use_aggregation` variable assignment currently only checks for
the presence of the "collection" key in request.query_params without validating
whether it has a non-empty value. This causes unnecessary aggregation mode
activation when collection is passed as a blank query parameter (e.g.,
?collection=). Modify the condition to not only check if "collection" is present
in request.query_params but also validate that it contains a non-empty/valid
value before setting use_aggregation to true.
In `@ami/main/models.py`:
- Line 2445: The fetch_image_content() function lacks URL validation, creating
an SSRF vulnerability. Add URL validation logic to fetch_image_content() that
blocks unsafe URL schemes (file://, ftp://, etc.), rejects private and
link-local IP ranges (127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16,
169.254.0.0/16), and disables HTTP redirects or validates redirect targets
against the same restrictions. Additionally, add validation in the
S3StorageSourceSerializer to validate the public_base_url field at the source to
prevent malicious values from being set in the first place.
In `@ami/ml/models/pipeline.py`:
- Around line 121-128: The `iter(images)` call at line 121 forces Django
QuerySets into memory when processing large collections. Create a helper
function that detects if `images` is a Django QuerySet and uses
`iterator(chunk_size=batch_size)` to stream results instead of materializing
them all at once. If `images` is not a QuerySet, fall back to using
`iter(images)`. Update the `image_iter` assignment to use this helper function
so that QuerySets remain lazy-evaluated throughout the iteration in the while
loop.
---
Outside diff comments:
In `@ami/ml/models/pipeline.py`:
- Around line 1064-1082: Filter out null detections (those with bbox=None) from
results.detections before the early processing path starting at
create_detections(). Create a filtered list of only real detections and pass
this filtered list to create_detections(), create_classifications(), and
create_and_update_occurrences_for_detections() instead of the raw
results.detections. Preserve the original results.detections (including null
markers) for the final persistence block that handles both incoming and
generated null markers, ensuring null detections bypass the early
classification, occurrence creation, and crop dispatch steps.
---
Nitpick comments:
In `@ami/main/tests.py`:
- Around line 6041-6070: In the test methods
test_is_null_marker_false_for_real_detection,
test_build_null_marker_sets_canonical_fields, and
test_valid_and_null_markers_are_disjoint_and_complete (and the other test ranges
noted), replace the bbox values of [0.0, 0.0, 1.0, 1.0] with explicit
pixel-coordinate values like [10, 10, 20, 20] to accurately reflect that
Detection.bbox uses absolute pixel coordinate space rather than normalized
coordinates, which will improve clarity and consistency with repository
conventions.
In `@ami/ml/tests.py`:
- Around line 1261-1262: Replace the hardcoded bbox__isnull=True filter in the
three assertion locations (around lines 1261, 1307, and 1347) with the canonical
.null_markers() method. Instead of using
image.detections.filter(bbox__isnull=True), change these to use
image.detections.null_markers() which properly handles both the new null-marker
abstraction and legacy bbox=[] sentinels. This ensures the assertions correctly
validate null markers through the proper API abstraction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0422a8e7-8613-4eda-b223-3416334f77a5
📒 Files selected for processing (6)
ami/main/api/views.pyami/main/management/commands/cleanup_null_only_occurrences.pyami/main/models.pyami/main/tests.pyami/ml/models/pipeline.pyami/ml/tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ami/main/management/commands/cleanup_null_only_occurrences.py
🛑 Comments failed to post (3)
ami/main/api/views.py (1)
1848-1848: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Avoid switching to aggregation mode for blank
collectionquery values.
use_aggregation = "collection" in request.query_paramstreats?collection=as present and forces the heavier aggregation path even when no collection filter is actually applied. Gate on a validated/non-empty value instead.💡 Suggested fix
- use_aggregation = "collection" in request.query_params + collection_raw = request.query_params.get("collection") + use_aggregation = bool(collection_raw)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/api/views.py` at line 1848, The `use_aggregation` variable assignment currently only checks for the presence of the "collection" key in request.query_params without validating whether it has a non-empty value. This causes unnecessary aggregation mode activation when collection is passed as a blank query parameter (e.g., ?collection=). Modify the condition to not only check if "collection" is present in request.query_params but also validate that it contains a non-empty/valid value before setting use_aggregation to true.ami/main/models.py (1)
2445-2445: 🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect the fetch helper and public_base_url exposure without executing repo code. fd -i '^media.py$' -x rg -n -C20 'def fetch_image_content' {} rg -n -C3 '\bpublic_base_url\b|find_or_generate_thumbnail_for_label|thumbnails_enabled' --type=pyRepository: RolnickLab/antenna
Length of output: 1314
🏁 Script executed:
# Find the find_or_generate_thumbnail_for_label function and public_base_url definition fd -type f -name "*.py" | head -20Repository: RolnickLab/antenna
Length of output: 327
🏁 Script executed:
# Search for find_or_generate_thumbnail_for_label function definition and usage rg -n "def find_or_generate_thumbnail_for_label|public_base_url" --type=py -A5 -B2Repository: RolnickLab/antenna
Length of output: 156
🏁 Script executed:
# Check for any URL validation or security checks related to fetch_image_content rg -n "fetch_image_content|SSRF|url.*validat|safe.*url" --type=py -B3 -A3Repository: RolnickLab/antenna
Length of output: 156
🏁 Script executed:
# Search for the function and public_base_url more broadly rg -n "def find_or_generate_thumbnail_for_label" rg -n "public_base_url" -A 2 -B 2 rg -n "class SourceImage" -A 50Repository: RolnickLab/antenna
Length of output: 156
🏁 Script executed:
# Look for URL validation patterns in the codebase rg -n "validate.*url|check.*url|scheme|urlparse|private.*ip|link.*local|redirect" -iRepository: RolnickLab/antenna
Length of output: 156
🏁 Script executed:
# Check what file contains the line in question (ami/main/models.py line 2445) fd models.py -type f -exec wc -l {} \;Repository: RolnickLab/antenna
Length of output: 327
🏁 Script executed:
# Find models.py file find . -name "models.py" -path "*/ami/main/*" # Search for the functions without specifying type rg "def find_or_generate_thumbnail_for_label" rg "public_base_url" rg "class SourceImage"Repository: RolnickLab/antenna
Length of output: 177
🏁 Script executed:
# Check the actual line 2445 in the models.py file cat ami/main/models.py | sed -n '2440,2450p'Repository: RolnickLab/antenna
Length of output: 911
🏁 Script executed:
# Look for any URL validation mechanisms rg "requests\." -A 3 -B 3 | head -100Repository: RolnickLab/antenna
Length of output: 156
🏁 Script executed:
# Get context around line 2445 - find the function name and broader context sed -n '2400,2480p' ami/main/models.pyRepository: RolnickLab/antenna
Length of output: 4178
🏁 Script executed:
# Search for find_or_generate_thumbnail_for_label and SourceImage definitions grep -n "def find_or_generate_thumbnail_for_label\|class SourceImage\|public_base_url" ami/main/models.py | head -20Repository: RolnickLab/antenna
Length of output: 1714
🏁 Script executed:
# Look for public_url method definition grep -n "def public_url" ami/main/models.py -A 10Repository: RolnickLab/antenna
Length of output: 1283
🏁 Script executed:
# Get the full public_url method implementation sed -n '2197,2240p' ami/main/models.pyRepository: RolnickLab/antenna
Length of output: 1971
🏁 Script executed:
# Check if there's any URL validation in the build_public_url method grep -n "def build_public_url\|build_public_url" ami/main/models.py -A 10Repository: RolnickLab/antenna
Length of output: 1264
🏁 Script executed:
# Check how public_base_url is populated and if it's API-writable grep -n "public_base_url" ami/main/models.py -B 2 -A 2 | head -40Repository: RolnickLab/antenna
Length of output: 2075
🏁 Script executed:
# Check for any SSRF or URL validation utilities in the codebase find . -name "*.py" -type f -exec grep -l "urlparse\|scheme\|private.*ip\|ssrf" {} \;Repository: RolnickLab/antenna
Length of output: 426
🏁 Script executed:
# Check the files that mention URL validation grep -n "urlparse\|scheme\|private" ami/exports/utils.py ami/main/models.py ami/tests/test_storage.py 2>/dev/nullRepository: RolnickLab/antenna
Length of output: 1416
🏁 Script executed:
# Check if there's any serializer validation on public_base_url find . -name "*serializer*" -type f -exec grep -l "public_base_url\|SourceImage\|S3StorageSource" {} \;Repository: RolnickLab/antenna
Length of output: 245
🏁 Script executed:
# Check the imports in models.py to see what URL utilities are available head -50 ami/main/models.pyRepository: RolnickLab/antenna
Length of output: 1919
🏁 Script executed:
# Look for any URL validation or sanitization middleware/utilities grep -rn "def.*url\|http\|scheme" ami/utils/ --include="*.py" 2>/dev/null | head -30Repository: RolnickLab/antenna
Length of output: 1196
🏁 Script executed:
# Check serializer validation for public_base_url and SourceImage grep -n "public_base_url" ami/main/api/serializers.py -B 3 -A 3Repository: RolnickLab/antenna
Length of output: 838
🏁 Script executed:
# Check if there's any validation in S3StorageSource or SourceImage serializers grep -n "class.*Serializer\|public_base_url" ami/main/api/serializers.py | head -40Repository: RolnickLab/antenna
Length of output: 2429
🏁 Script executed:
# Look for any URL validation utilities in ami/utils/ ls -la ami/utils/*.pyRepository: RolnickLab/antenna
Length of output: 855
🏁 Script executed:
# Check the complete fetch_image_content and related functions sed -n '1,50p' ami/utils/media.pyRepository: RolnickLab/antenna
Length of output: 1709
Add URL validation to
fetch_image_content()to prevent SSRF attacks.The vulnerability is confirmed:
public_base_urlis API-writable (exposed inS3StorageSourceSerializer) and used to construct URLs fetched server-side infind_or_generate_thumbnail_for_label(). Thefetch_image_content()function performs no validation—it accepts any scheme, private/link-local hosts, and follows redirects. An attacker could injectfile://,http://localhost:*, orhttp://169.254.169.254/*to exfiltrate internal resources.Required mitigations:
- Block unsafe schemes (
file://,ftp://, etc.)- Reject private/link-local IP ranges (127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16)
- Disable HTTP redirects or validate redirect targets against the same restrictions
- Consider adding
public_base_urlvalidation in the serializer as a secondary controlThe existing timeout (5.0s connect, 30.0s read) is insufficient without URL validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/models.py` at line 2445, The fetch_image_content() function lacks URL validation, creating an SSRF vulnerability. Add URL validation logic to fetch_image_content() that blocks unsafe URL schemes (file://, ftp://, etc.), rejects private and link-local IP ranges (127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16), and disables HTTP redirects or validates redirect targets against the same restrictions. Additionally, add validation in the S3StorageSourceSerializer to validate the public_base_url field at the source to prevent malicious values from being set in the first place.ami/ml/models/pipeline.py (1)
121-128: 🚀 Performance & Scalability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, locate and inspect the file find . -name "pipeline.py" -path "*/ml/models/*" | head -5Repository: RolnickLab/antenna
Length of output: 184
🏁 Script executed:
# Check file size to determine reading strategy wc -l ami/ml/models/pipeline.pyRepository: RolnickLab/antenna
Length of output: 187
🏁 Script executed:
# Read the specific lines mentioned in the review (121-128 and around 258-273) sed -n '115,135p' ami/ml/models/pipeline.pyRepository: RolnickLab/antenna
Length of output: 1101
🏁 Script executed:
# Read the second location mentioned (around 258-273) sed -n '250,280p' ami/ml/models/pipeline.pyRepository: RolnickLab/antenna
Length of output: 1427
🏁 Script executed:
# Check for existing imports to understand context head -50 ami/ml/models/pipeline.pyRepository: RolnickLab/antenna
Length of output: 1402
🌐 Web query:
Django QuerySet len() vs count() eager evaluation behavior💡 Result:
In Django, the behavior of len and count on a QuerySet differs significantly in terms of database interaction and evaluation: len Calling len on a QuerySet forces the evaluation of the entire QuerySet [1][2]. It fetches all records from the database and loads them into memory as model instances [3]. Consequently, it returns the length of the result list held in Python [1][2]. Because it retrieves all objects, this method is memory-intensive and slow for large datasets [3]. count Calling count performs a SELECT COUNT(*) query at the database level [1][2][4]. It returns an integer representing the number of records without loading the actual objects into memory [1][4]. This is significantly more efficient than len when you only need to know the number of records [1][2][3]. Caching Behavior Both methods respect the QuerySet's internal result cache: If the QuerySet has already been fully evaluated and cached (e.g., by iterating over it), calling len will return the length of the cached results without hitting the database [4][3]. Similarly, if the QuerySet is already fully cached, count will return the length of that cached result set directly, avoiding an additional database query [4]. Summary Recommendation Use count if you only need the number of records, as it avoids loading data into memory [1][3]. Use len only if you have already retrieved the QuerySet data and want to avoid an additional, redundant database hit for the count, or if you are already planning to process the full collection of objects in memory [3].
Citations:
- 1: https://docs.djangoproject.com/en/6.0/ref/models/querysets/
- 2: https://docs.djangoproject.com/en/5.2/ref/models/querysets/
- 3: https://stackoverflow.com/questions/14327036/count-vs-len-on-a-django-queryset
- 4: https://github.com/django/django/blob/0ee8aa5c3908e2f8f8408083ff213e6e5eed4d4d/django/db/models/query.py
🌐 Web query:
Django QuerySet iterator() chunk_size lazy evaluation performance💡 Result:
In Django, the QuerySet iterator method is a performance optimization tool used to reduce memory consumption when processing large result sets [1][2][3]. Core Performance and Behavioral Characteristics: - Memory Management: By default, Django QuerySets cache results in memory after the first iteration to avoid subsequent database hits [1][2]. The iterator method disables this caching mechanism, fetching rows from the database as needed [1][4][3]. This allows you to process large numbers of objects without loading the entire result set into RAM [1][2][3]. - Lazy Evaluation: Like standard QuerySets, calling iterator does not execute the database query immediately [1]. The query is executed only when you begin iterating over the result set [5][1]. - Chunking: The chunk_size parameter determines how many rows are fetched from the database at once into memory [6][7]. When iterator is used, Django uses server-side cursors (where supported by the database backend) to manage the retrieval of records in the specified chunk sizes [6][4]. - Prefetching Support: Since Django 4.1, iterator supports prefetch_related provided that a chunk_size is explicitly defined [5][4][3]. In this scenario, Django performs the prefetch operations within the defined chunks [6][7]. Without a specified chunk_size, prefetch_related calls are ignored when using iterator [5][8]. When to Use iterator: - Use it when you have a very large result set that would cause your application to exceed available memory if loaded entirely [1][2]. - Use it primarily for one-time processing tasks, such as management commands, bulk data exports, or migrations [2]. When to Avoid iterator: - Avoid it if you need to reuse the QuerySet results, as the absence of caching means that any subsequent iteration will trigger a completely new database query [2]. - Avoid it for standard web request views where results are typically paginated; using standard QuerySet evaluation is generally more efficient for small to medium-sized sets because it leverages the built-in result cache [2]. Always profile your specific use case to determine if the memory savings provided by iterator outweigh the potential overhead of disabling result caching and the nuances of chunked prefetching [5][4].
Citations:
- 1: https://docs.djangoproject.com/en/5.2/topics/db/optimization/
- 2: https://stackoverflow.com/questions/12681653/when-to-use-or-not-use-iterator-in-the-django-orm
- 3: https://www.labdigital.nl/blog/working-with-huge-data-sets-in-django
- 4: https://stackoverflow.com/questions/14144408/memory-efficient-constant-and-speed-optimized-iteration-over-a-large-table-in
- 5: https://docs.djangoproject.com/en/5.2/ref/models/querysets/
- 6: https://docs.djangoproject.com/en/5.0/_modules/django/db/models/query/
- 7: https://github.com/django/django/blob/stable/6.0.x/django/db/models/query.py
- 8: https://django.readthedocs.io/en/stable/ref/models/querysets.html
🏁 Script executed:
# Verify QuerySet import is available for type checking rg "from django.db" ami/ml/models/pipeline.py | head -10Repository: RolnickLab/antenna
Length of output: 185
🏁 Script executed:
# Check if QuerySet is used or imported elsewhere in the file rg "QuerySet" ami/ml/models/pipeline.pyRepository: RolnickLab/antenna
Length of output: 681
🏁 Script executed:
# Get more context around line 259 to see the full collect_images function sed -n '240,290p' ami/ml/models/pipeline.pyRepository: RolnickLab/antenna
Length of output: 2102
🏁 Script executed:
# Get more context around line 121 to see the full filter_processed_images function signature sed -n '100,140p' ami/ml/models/pipeline.pyRepository: RolnickLab/antenna
Length of output: 2140
🏁 Script executed:
# Check the function signatures to understand parameters sed -n '105,115p' ami/ml/models/pipeline.pyRepository: RolnickLab/antenna
Length of output: 938
🏁 Script executed:
# Verify if images parameter can actually be a QuerySet in filter_processed_images sed -n '70,130p' ami/ml/models/pipeline.pyRepository: RolnickLab/antenna
Length of output: 3161
🏁 Script executed:
# Check the actual return type hint of collect_images to understand how filter_processed_images receives images sed -n '220,240p' ami/ml/models/pipeline.pyRepository: RolnickLab/antenna
Length of output: 783
🏁 Script executed:
# Verify what happens after filter_processed_images - does it list() the result? sed -n '260,275p' ami/ml/models/pipeline.pyRepository: RolnickLab/antenna
Length of output: 791
Optimize QuerySet evaluation to prevent memory bloat on large collections.
len(images)at line 259 materializes Django QuerySets into memory before processing. Since images can be a QuerySet fromcollection.imagesordeployment-filtered queries, large collections load every capture up front. Usecount()for totals anditerator(chunk_size=batch_size)for iteration to keep results lazy and streamed.The same issue affects
filter_processed_images()when it receives a QuerySet: callingiter(images)at line 121 may force evaluation. Add a helper to detect QuerySets and stream them viaiterator()instead.Proposed fix
+from django.db.models import QuerySet + @@ - if not pipeline_classifier_ids: + def iter_input_images(): + if isinstance(images, QuerySet): + return images.iterator(chunk_size=batch_size) + return iter(images) + + if not pipeline_classifier_ids: task_logger.warning(f"Pipeline {pipeline} has no classification algorithms saved. Will reprocess all images.") @@ - yield from images + yield from iter_input_images() return - image_iter = iter(images) + image_iter = iter_input_images() @@ - total_images = len(images) + total_images = images.count() if isinstance(images, QuerySet) else len(images)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/ml/models/pipeline.py` around lines 121 - 128, The `iter(images)` call at line 121 forces Django QuerySets into memory when processing large collections. Create a helper function that detects if `images` is a Django QuerySet and uses `iterator(chunk_size=batch_size)` to stream results instead of materializing them all at once. If `images` is not a QuerySet, fall back to using `iter(images)`. Update the `image_iter` assignment to use this helper function so that QuerySets remain lazy-evaluated throughout the iteration in the while loop.
…_detection branch [skip ci] Comment-only. Clarify that existing_detection is the single pre-existing row matching the incoming response's identity (the null-marker sentinel in the null branch, the same-bbox detection in the real branch), not "all detections for the image" — and why the null branch needs .null_markers() while the real branch is disambiguated by the bbox match alone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Fixes #1310.
Null detections (empty-bbox sentinels marking "image processed, nothing found") were being created before the downstream save steps inside
save_results. Two consequences:filter_processed_imageswould then skip it on retry, leaving the image permanently stuck as "processed, zero detections." Observed in production where several hundred captures had only null detections and no real ones.create_and_update_occurrences_for_detectionsiterated every detection including nulls, so each null marker spawned anOccurrencewithdetermination=NULL. Those leaked throughOccurrenceQuerySet.valid()(which only excluded occurrences with zero detections, not occurrences whose only detection is a null).Reviewer heads-up — silent semantic change to
OccurrenceQuerySet.valid()valid()changes meaning from "has any detection" to "has at least oneDetection.objects.valid()row ANDdeterminationis not null." Three call sites pick this up without any line change at the call site:OccurrenceViewSet.get_queryset(ami/main/api/views.py) — intended target of the fix. Phantom occurrences stop appearing in the list endpoint.occurrences_count(ami/main/api/views.py) — will silently decrease on any deployment that has accumulated phantoms. No-op on clean deployments.ami/exports/format_types.py) — null-determination occurrences will be excluded from exports. Probably correct (DwC requirestaxonID) but not validated against an actual export run in this PR.If any of those three are load-bearing in a way I'm missing, flag it.
Changes
test(ml)— RED test for the broker-outage path: asserts the null marker is never persisted ifcreate_detection_images.delayraises andfilter_processed_imagesre-yields the image.fix(ml)— move null persistence to the absolute final step insave_results. Null markers now run after thesource_image.save()loop,create_detection_images.delay(),update_calculated_fields_for_events, andDeployment.update_calculated_fields(save=True). Closes the silent-bug window the prior reorder left open.refactor(main)— null-marker abstraction onDetection.Detection.NULL_BBOX = None— canonical sentinel value for new writes.Detection.is_null_markerproperty — recognises bothbbox=Noneand legacybbox=[].Detection.build_null_marker(source_image, detection_algorithm)classmethod — single construction point.DetectionQuerySet.valid()— consumer default (excludes null markers).DetectionQuerySet.null_markers()— narrow, for "has this image been processed?" checks.refactor(main)— sweep inlineNULL_DETECTIONS_FILTERcall sites to the new manager methods acrossami/main/models.py,ami/main/api/views.py,ami/ml/models/pipeline.py, plus anull_detections_q(prefix)helper for relation-prefixed Q expressions.fix(main)— tightenOccurrenceQuerySet.valid()to require at least one valid detection AND a non-null determination. Closes the phantom-Occurrence leak. See the reviewer heads-up above for the consumers that pick up the new semantic.feat(main)—cleanup_null_only_occurrencesmanagement command for per-project cleanup of the field bug. Dry-run by default. Deletes phantom occurrences (no valid detections OR null determination) and dangling null-marker Detection rows on source images that have no real detection. Idempotent.Test plan
test_null_marker_not_persisted_when_broker_dispatch_fails— RED, then GREEN after move-to-end.TestDetectionNullMarker—is_null_markerforNone/[]/ real bbox,build_null_markerfield setup,valid()/null_markers()disjointness.TestOccurrenceValidQuerySet— fixture with real / null-only-detection / null-determination occurrences; assertsvalid()returns only the real, fully-determined one.TestCleanupNullOnlyOccurrencesCommand— dry-run reports without deleting;--commitdeletes phantoms (both the no-real-detection arm and the null-determination arm) while preserving valid rows and null markers on images that also have a real detection; idempotent on second run.ami/main/tests.py+ami/ml/tests.py+ami/jobs/tests/pass locally.Manual e2e (dev deployment)
create_detection_images.delayto raise mid-job. 0 null markers persisted, 0 phantoms; image stays in thefilter_processed_imagesyield list.update_calculated_fields_for_eventsto raise. Same result: 0 null markers persisted.--commitremoved exactly those rows and leftvalid()counts unchanged; a second dry-run reported0 / 0(idempotent). Running it against any affected production deployment is a post-merge ops step.Out of scope — deferred follow-up
transaction.atomic()wrap. The persistence block (real detections → classifications → occurrences → calc-fields → null marker) can still partially commit if a mid-block step raises. This PR closes the ordering window (null marker writing before downstream steps); it does not close the within-block partial-commit window. A narrowtransaction.atomic()wrap withtransaction.on_commitfor celery dispatch is the structural fix, deferred to a separate PR because transaction changes carry concurrency risk (see theselect_for_update+ATOMIC_REQUESTScontention introduced by #1261) and need their own multi-worker e2e.Dual-form
bbox=Nonevsbbox=[]. New writes go throughDetection.NULL_BBOX = None; legacy rows still carrybbox=[]..null_markers()/.is_null_marker/null_detections_q()all recognise both, so no consumer breaks, but the dual form persists until a data migration backfills legacy rows. Worth a follow-up ticket.Re-classification gap. Adjacent:
filter_processed_imagescurrently reprocesses from scratch because there is no mechanism to reclassify existing detections. Worth a separate ticket.Summary by CodeRabbit
New Features
Bug Fixes
Tests