From 68ef784df55c06d016e2ff2844b847989852ae65 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Tue, 19 May 2026 17:41:27 -0700 Subject: [PATCH 01/11] fix(ml): create null detection markers only after real saves succeed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ami/ml/models/pipeline.py | 25 +++++++++----- ami/ml/tests.py | 73 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 9 deletions(-) diff --git a/ami/ml/models/pipeline.py b/ami/ml/models/pipeline.py index 7cb54d14f..c913001e8 100644 --- a/ami/ml/models/pipeline.py +++ b/ami/ml/models/pipeline.py @@ -1058,15 +1058,6 @@ def save_results( "Algorithms and category maps must be registered before processing, using /info endpoint." ) - # Ensure all images have detections - # if not, add a NULL detection (empty bbox) to the results - null_detections = create_null_detections_for_undetected_images( - results=results, - detection_algorithm=detection_algorithm, - logger=job_logger, - ) - results.detections = results.detections + null_detections - detections = create_detections( detections=results.detections, algorithms_known=algorithms_known, @@ -1087,6 +1078,22 @@ def save_results( logger=job_logger, ) + # Mark images with no real detections as processed by creating null-bbox sentinels. + # Issue #1310: must run AFTER the real-detection / classification / occurrence steps + # so a failure earlier in the pipeline leaves the image unmarked (and therefore + # re-processed by filter_processed_images on the next run). Null DetectionResponses + # are kept out of the real-detection list so they bypass occurrence creation entirely. + null_detection_responses = create_null_detections_for_undetected_images( + results=results, + detection_algorithm=detection_algorithm, + logger=job_logger, + ) + create_detections( + detections=null_detection_responses, + algorithms_known=algorithms_known, + logger=job_logger, + ) + # Update precalculated counts on source images and events source_images = list(source_images) logger.info(f"Updating calculated fields for {len(source_images)} source images") diff --git a/ami/ml/tests.py b/ami/ml/tests.py index ea375135e..e0a9b2882 100644 --- a/ami/ml/tests.py +++ b/ami/ml/tests.py @@ -13,6 +13,7 @@ Deployment, Detection, Event, + Occurrence, Project, SourceImage, SourceImageCollection, @@ -1245,6 +1246,78 @@ def test_null_detection_deduplication_same_pipeline(self): null_detections = image.detections.filter(bbox__isnull=True) self.assertEqual(null_detections.count(), 1, "Same pipeline should not create duplicate null detections") + def test_null_detection_does_not_create_phantom_occurrence(self): + """ + Issue #1310: a null detection (empty-bbox sentinel marking "image processed, + nothing found") must NOT spawn an Occurrence. Occurrences with no + determination and no real detections leak to the API as ghost rows. + """ + image = self.test_images[0] + results = self.fake_pipeline_results([image], self.pipeline) + results.detections = [] # pipeline found nothing + + save_results(results) + + null_dets = image.detections.filter(bbox__isnull=True) + self.assertEqual(null_dets.count(), 1, "Null marker should still be created") + self.assertIsNone( + null_dets.first().occurrence, + "Null detection must NOT be associated with an Occurrence", + ) + # No phantom Occurrence in DB tied to this image at all + phantom_occs = Occurrence.objects.filter(detections__source_image=image, determination__isnull=True) + self.assertEqual( + phantom_occs.count(), + 0, + "No Occurrence with NULL determination should exist for an image that had no detections", + ) + + def test_captures_not_marked_processed_after_failure(self): + """ + Issue #1310: null markers should only flag images as processed AFTER all + downstream save steps (classifications, occurrences) succeed. If any + downstream step raises, the image must remain unmarked so the next run + re-processes it. + + Reproduces the field bug where 400 images ended up with null markers but + no real detections — created when null-creation ran ahead of a later step + that failed. + """ + from unittest.mock import patch + + from ami.ml.models.pipeline import filter_processed_images + + # Mix: image_with_real has a detection in the response, image_without_real does not. + # The without-real image is the one that would get a null marker. + image_with_real, image_without_real = self.test_images + results = self.fake_pipeline_results(self.test_images, self.pipeline) + # Trim detections to only the first image so the second qualifies for null-marker creation + results.detections = [d for d in results.detections if str(d.source_image_id) == str(image_with_real.pk)] + + # Inject failure in a step that runs AFTER detection bulk_create + with patch( + "ami.ml.models.pipeline.create_classifications", + side_effect=RuntimeError("simulated classification failure"), + ): + with self.assertRaises(RuntimeError): + save_results(results) + + # The image with no real detection must NOT have a null marker — + # the run failed, so it should be re-tried. + null_dets = image_without_real.detections.filter(bbox__isnull=True) + self.assertEqual( + null_dets.count(), + 0, + "Image without real detections must not be marked processed when downstream step fails", + ) + # filter_processed_images should still yield it for the next run + retry_yield = list(filter_processed_images([image_without_real], self.pipeline)) + self.assertEqual( + retry_yield, + [image_without_real], + "Image with failed run must be re-yielded for processing", + ) + class TestAlgorithmCategoryMaps(TestCase): def setUp(self): From b3c64bf67c1c15dc686fee56a6f1d00fc059ac0c Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 20 May 2026 15:45:20 -0700 Subject: [PATCH 02/11] test(ml): RED test for broker-outage leaving null marker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ami/ml/tests.py | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/ami/ml/tests.py b/ami/ml/tests.py index e0a9b2882..898a69d11 100644 --- a/ami/ml/tests.py +++ b/ami/ml/tests.py @@ -1318,6 +1318,45 @@ def test_captures_not_marked_processed_after_failure(self): "Image with failed run must be re-yielded for processing", ) + def test_null_marker_not_persisted_when_broker_dispatch_fails(self): + """ + Issue #1310 (takeaway-review follow-up): null markers must be the FINAL + write in save_results. Failures in any of the trailing steps — + create_detection_images.delay (broker outage), update_calculated_fields_for_events + (DB error), Deployment.update_calculated_fields (DB error) — must leave the + image unmarked. + + This test patches the celery dispatch to raise, simulating a broker + outage between the real-detection save and the null-marker save. + """ + from unittest.mock import patch + + from ami.ml.models.pipeline import filter_processed_images + + image_with_real, image_without_real = self.test_images + results = self.fake_pipeline_results(self.test_images, self.pipeline) + results.detections = [d for d in results.detections if str(d.source_image_id) == str(image_with_real.pk)] + + with patch( + "ami.ml.models.pipeline.create_detection_images.delay", + side_effect=RuntimeError("simulated broker outage"), + ): + with self.assertRaises(RuntimeError): + save_results(results) + + null_dets = image_without_real.detections.filter(bbox__isnull=True) + self.assertEqual( + null_dets.count(), + 0, + "Null marker must not be persisted when create_detection_images.delay fails", + ) + retry_yield = list(filter_processed_images([image_without_real], self.pipeline)) + self.assertEqual( + retry_yield, + [image_without_real], + "Image with failed broker dispatch must be re-yielded for processing", + ) + class TestAlgorithmCategoryMaps(TestCase): def setUp(self): From 9fc34da1073014512660bbf0f429d52c704182ee Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 20 May 2026 15:46:04 -0700 Subject: [PATCH 03/11] fix(ml): move null-marker creation to final step in save_results 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 --- ami/ml/models/pipeline.py | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/ami/ml/models/pipeline.py b/ami/ml/models/pipeline.py index c913001e8..21430b88c 100644 --- a/ami/ml/models/pipeline.py +++ b/ami/ml/models/pipeline.py @@ -1078,22 +1078,6 @@ def save_results( logger=job_logger, ) - # Mark images with no real detections as processed by creating null-bbox sentinels. - # Issue #1310: must run AFTER the real-detection / classification / occurrence steps - # so a failure earlier in the pipeline leaves the image unmarked (and therefore - # re-processed by filter_processed_images on the next run). Null DetectionResponses - # are kept out of the real-detection list so they bypass occurrence creation entirely. - null_detection_responses = create_null_detections_for_undetected_images( - results=results, - detection_algorithm=detection_algorithm, - logger=job_logger, - ) - create_detections( - detections=null_detection_responses, - algorithms_known=algorithms_known, - logger=job_logger, - ) - # Update precalculated counts on source images and events source_images = list(source_images) logger.info(f"Updating calculated fields for {len(source_images)} source images") @@ -1112,6 +1096,22 @@ def save_results( for deployment in Deployment.objects.filter(pk__in=deployment_ids): deployment.update_calculated_fields(save=True) + # Mark images with no real detections as processed by creating null-bbox sentinels. + # Issue #1310: MUST be the final write. Persisting a null marker is the signal that + # filter_processed_images uses to skip an image on future runs, so it can only be + # written after every step that might fail has succeeded. Any raise earlier in the + # pipeline leaves the image unmarked so it gets retried on the next run. + null_detection_responses = create_null_detections_for_undetected_images( + results=results, + detection_algorithm=detection_algorithm, + logger=job_logger, + ) + create_detections( + detections=null_detection_responses, + algorithms_known=algorithms_known, + logger=job_logger, + ) + total_time = time.time() - start_time job_logger.info(f"Saved results from pipeline {pipeline} in {total_time:.2f} seconds") From 8fbd51fc1fa9ce4c41fd392d62e5827e0084aba7 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 20 May 2026 15:48:35 -0700 Subject: [PATCH 04/11] refactor(main): add null-marker abstraction on Detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ami/main/models.py | 39 +++++++++++++++++++++++- ami/main/tests.py | 75 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/ami/main/models.py b/ami/main/models.py index 6ddabfa3e..b06978cbc 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -3024,7 +3024,23 @@ def save(self, *args, **kwargs): class DetectionQuerySet(BaseQuerySet): - def null_detections(self): + def valid(self): + """ + Detections suitable for consumer queries — excludes null-marker sentinels. + + Null markers are rows that record "an algorithm ran against this image and + found nothing." Consumers asking "give me detections" should always go + through .valid(). Future predicates to fold in here: soft-delete tombstones, + detections missing an algorithm reference, detections missing classifications. + """ + return self.exclude(NULL_DETECTIONS_FILTER) + + def null_markers(self): + """ + Sentinel rows that record "this algorithm ran against this image and found + nothing." Only relevant for SourceImage-level "has this been processed?" + questions. Detection consumers should use .valid() instead. + """ return self.filter(NULL_DETECTIONS_FILTER) @@ -3102,6 +3118,27 @@ class Detection(BaseModel): objects = DetectionManager() + NULL_BBOX = None + """Canonical bbox value for null markers (rows that record 'an algorithm ran but + found nothing'). Use Detection.build_null_marker() to construct them. The legacy + bbox=[] form is still recognised by .null_markers() / .is_null_marker for + backwards compatibility with historical rows.""" + + @property + def is_null_marker(self) -> bool: + """True for sentinel rows representing 'no detections found by this algorithm.'""" + return self.bbox is None or self.bbox == [] + + @classmethod + def build_null_marker(cls, source_image, detection_algorithm) -> "Detection": + """Construct (without saving) a null-marker Detection for the given image+algorithm.""" + return cls( + source_image=source_image, + bbox=cls.NULL_BBOX, + detection_algorithm=detection_algorithm, + timestamp=timezone.now(), + ) + def get_bbox(self): if self.bbox: return BoundingBox( diff --git a/ami/main/tests.py b/ami/main/tests.py index 0dca7b836..4201e5621 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -6009,3 +6009,78 @@ def test_verified_count_not_inflated_by_collection_join(self): rows = self._list_by_name(f"{self.list_url}&collection={collection.pk}") # 2 verified cardui occurrences, not 3 — the duplicate detection must not double-count. self.assertEqual(rows["Vanessa cardui"]["verified_count"], 2) + + +class TestDetectionNullMarker(TestCase): + """ + Covers the null-marker abstraction added for Issue #1310 follow-up: + Detection.is_null_marker, Detection.build_null_marker, and + DetectionQuerySet.valid() / .null_markers(). + """ + + def setUp(self): + from ami.ml.models.algorithm import Algorithm + + self.project = Project.objects.create(name="Null Marker Test Project") + self.deployment = Deployment.objects.create(project=self.project, name="dep") + self.source_image = SourceImage.objects.create( + deployment=self.deployment, + project=self.project, + path="nullmarker-test.jpg", + ) + self.algorithm = Algorithm.objects.create(name="test-detector", key="test-detector", task_type="detection") + + def test_is_null_marker_for_bbox_none(self): + det = Detection.objects.create( + source_image=self.source_image, + bbox=None, + detection_algorithm=self.algorithm, + ) + self.assertTrue(det.is_null_marker) + + def test_is_null_marker_for_bbox_empty_list_legacy(self): + det = Detection.objects.create( + source_image=self.source_image, + bbox=[], + detection_algorithm=self.algorithm, + ) + self.assertTrue(det.is_null_marker) + + def test_is_null_marker_false_for_real_detection(self): + det = Detection.objects.create( + source_image=self.source_image, + bbox=[0.0, 0.0, 1.0, 1.0], + detection_algorithm=self.algorithm, + ) + self.assertFalse(det.is_null_marker) + + def test_build_null_marker_sets_canonical_fields(self): + det = Detection.build_null_marker(self.source_image, self.algorithm) + self.assertIsNone(det.bbox) + self.assertEqual(det.source_image, self.source_image) + self.assertEqual(det.detection_algorithm, self.algorithm) + self.assertIsNotNone(det.timestamp) + self.assertTrue(det.is_null_marker) + + def test_valid_and_null_markers_are_disjoint_and_complete(self): + real = Detection.objects.create( + source_image=self.source_image, + bbox=[0.0, 0.0, 1.0, 1.0], + detection_algorithm=self.algorithm, + ) + null_via_none = Detection.objects.create( + source_image=self.source_image, + bbox=None, + detection_algorithm=self.algorithm, + ) + null_via_empty = Detection.objects.create( + source_image=self.source_image, + bbox=[], + detection_algorithm=self.algorithm, + ) + scoped = Detection.objects.filter(source_image=self.source_image) + valid_pks = set(scoped.valid().values_list("pk", flat=True)) + null_pks = set(scoped.null_markers().values_list("pk", flat=True)) + self.assertEqual(valid_pks, {real.pk}) + self.assertEqual(null_pks, {null_via_none.pk, null_via_empty.pk}) + self.assertEqual(valid_pks & null_pks, set()) From a0e6048a459443d9db20a807e4289b7ec077af7e Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 20 May 2026 15:54:39 -0700 Subject: [PATCH 05/11] refactor(main): sweep NULL_DETECTIONS_FILTER call sites to .valid()/.null_markers() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ami/main/api/views.py | 5 ++--- ami/main/models.py | 20 +++++++++++++++----- ami/ml/models/pipeline.py | 6 ++++-- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/ami/main/api/views.py b/ami/main/api/views.py index 3e949f215..1a94a417d 100644 --- a/ami/main/api/views.py +++ b/ami/main/api/views.py @@ -39,7 +39,6 @@ from ami.utils.storages import ConnectionTestResult from ..models import ( - NULL_DETECTIONS_FILTER, Classification, Deployment, Detection, @@ -756,7 +755,7 @@ def prefetch_detections(self, queryset: QuerySet, project: Project | None = None score = get_default_classification_threshold(project, self.request) prefetch_queryset = ( - Detection.objects.exclude(NULL_DETECTIONS_FILTER) + Detection.objects.valid() .annotate( determination_score=models.Max("occurrence__detections__classifications__score"), # Store whether this occurrence should be included based on default filters @@ -1096,7 +1095,7 @@ class DetectionViewSet(DefaultViewSet, ProjectMixin): """ require_project_for_list = True # Unfiltered list scans are too expensive on this table - queryset = Detection.objects.exclude(NULL_DETECTIONS_FILTER).select_related("source_image", "detection_algorithm") + queryset = Detection.objects.valid().select_related("source_image", "detection_algorithm") serializer_class = DetectionSerializer filterset_fields = ["source_image", "detection_algorithm", "source_image__project"] ordering_fields = ["created_at", "updated_at", "detection_score", "timestamp"] diff --git a/ami/main/models.py b/ami/main/models.py index b06978cbc..0ffd4d568 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -105,6 +105,16 @@ def bbox_is_null(bbox) -> bool: return bbox is None or bbox == [] +def null_detections_q(prefix: str = "") -> Q: + """ + Return a Q expression matching null-marker Detection rows, optionally prefixed + for use across relations (e.g. null_detections_q("images__detections__") for an + aggregate filter on a parent table). For Detection queries directly, prefer + Detection.objects.null_markers() / .valid() instead. + """ + return Q(**{f"{prefix}bbox__isnull": True}) | Q(**{f"{prefix}bbox": []}) + + def get_media_url(path: str) -> str: """ If path is a full URL, return it as-is. @@ -836,7 +846,7 @@ def get_detections_count(self) -> int | None: was processed and no detections were found) to stay consistent with ``SourceImage.get_detections_count`` and ``Event.get_detections_count``. """ - qs = Detection.objects.filter(source_image__deployment=self).exclude(NULL_DETECTIONS_FILTER) + qs = Detection.objects.filter(source_image__deployment=self).valid() filter_q = build_occurrence_default_filters_q( project=self.project, request=None, @@ -1271,7 +1281,7 @@ def get_detections_count(self) -> int | None: Excludes null-bbox placeholder detections to stay consistent with ``SourceImage.get_detections_count`` and ``Deployment.get_detections_count``. """ - qs = Detection.objects.filter(source_image__event=self).exclude(NULL_DETECTIONS_FILTER) + qs = Detection.objects.filter(source_image__event=self).valid() filter_q = build_occurrence_default_filters_q( project=self.project, request=None, @@ -2238,7 +2248,7 @@ def get_detections_count(self) -> int: Excludes detections without bounding boxes — those are placeholder records indicating the image was successfully processed and no detections were found. """ - qs = self.detections.exclude(NULL_DETECTIONS_FILTER) + qs = self.detections.all().valid() project = self.project if not project: return qs.distinct().count() @@ -2518,7 +2528,7 @@ def update_detection_counts( if null_only: qs = qs.filter(detections_count__isnull=True) - detection_qs = Detection.objects.filter(source_image_id=models.OuterRef("pk")).exclude(NULL_DETECTIONS_FILTER) + detection_qs = Detection.objects.filter(source_image_id=models.OuterRef("pk")).valid() if project is not None: filter_q = build_occurrence_default_filters_q( project=project, @@ -4658,7 +4668,7 @@ def with_source_images_with_detections_count(self): return self.annotate( source_images_with_detections_count=models.Count( "images", - filter=(~models.Q(images__detections__bbox__isnull=True) & ~models.Q(images__detections__bbox=[])), + filter=~null_detections_q("images__detections__"), distinct=True, ) ) diff --git a/ami/ml/models/pipeline.py b/ami/ml/models/pipeline.py index 21430b88c..19652fe2e 100644 --- a/ami/ml/models/pipeline.py +++ b/ami/ml/models/pipeline.py @@ -546,8 +546,10 @@ def get_or_create_detection( if serialized_bbox is None: # Null detection: algorithm-specific lookup so different pipelines don't share sentinels. - # Use bbox__isnull=True because JSONField filter(bbox=None) matches JSON null literal, - # not SQL NULL which is what Detection(bbox=None) stores. + # Use bbox__isnull=True (not Detection.objects.null_markers()) because we want to find + # an exact match for THIS algorithm — null_markers() also includes legacy bbox=[] rows + # from other pipelines and would be wider than this dedup needs. + # See Detection.NULL_BBOX for the canonical sentinel value used by new writes. assert detection_resp.algorithm, f"No detection algorithm was specified for detection {detection_repr}" try: detection_algo = algorithms_known[detection_resp.algorithm.key] From 9dfb913ac2b8ccb24d831c22ca48898af463624d Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 20 May 2026 15:56:52 -0700 Subject: [PATCH 06/11] fix(main): tighten OccurrenceQuerySet.valid to exclude phantoms 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 --- ami/main/models.py | 15 +++++++- ami/main/tests.py | 87 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/ami/main/models.py b/ami/main/models.py index 0ffd4d568..59ca23234 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -3269,7 +3269,20 @@ def __str__(self) -> str: class OccurrenceQuerySet(BaseQuerySet): def valid(self): - return self.exclude(detections__isnull=True) + """ + Occurrences fit to surface in API responses: at least one real detection AND + a determination set. + + Excludes: + - Occurrences with no detections at all (orphans) + - Occurrences whose only detections are null-marker sentinels (Issue #1310: + field bug created phantom occurrences with no real bounding box backing + them) + - Occurrences with determination__isnull=True (no taxonomic identification, + same field bug shape) + """ + has_valid_detection = Exists(Detection.objects.valid().filter(occurrence_id=OuterRef("pk"))) + return self.filter(has_valid_detection).exclude(determination__isnull=True) def with_detections_count(self): return self.annotate(detections_count=models.Count("detections", distinct=True)) diff --git a/ami/main/tests.py b/ami/main/tests.py index 4201e5621..15011e94b 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -6084,3 +6084,90 @@ def test_valid_and_null_markers_are_disjoint_and_complete(self): self.assertEqual(valid_pks, {real.pk}) self.assertEqual(null_pks, {null_via_none.pk, null_via_empty.pk}) self.assertEqual(valid_pks & null_pks, set()) + + +class TestOccurrenceValidQuerySet(TestCase): + """ + Covers OccurrenceQuerySet.valid() tightening for Issue #1310: + excludes occurrences with no real detections and occurrences missing a + determination, in addition to the existing detections__isnull=True + exclusion. + """ + + def setUp(self): + from ami.main.models import Taxon + from ami.ml.models.algorithm import Algorithm + + self.project = Project.objects.create(name="Occurrence Valid Test Project") + self.deployment = Deployment.objects.create(project=self.project, name="dep") + self.event = Event.objects.create( + project=self.project, + deployment=self.deployment, + group_by="2024-01-01", + start=datetime.datetime(2024, 1, 1, 0, 0), + ) + self.source_image = SourceImage.objects.create( + deployment=self.deployment, + project=self.project, + event=self.event, + path="occvalid-test.jpg", + ) + self.algorithm = Algorithm.objects.create(name="valid-detector", key="valid-detector", task_type="detection") + self.taxon = Taxon.objects.create(name="Occurrence Valid Test Taxon") + + def _make_occurrence_with_real_detection(self) -> Occurrence: + occ = Occurrence.objects.create( + project=self.project, + event=self.event, + deployment=self.deployment, + determination=self.taxon, + ) + Detection.objects.create( + source_image=self.source_image, + bbox=[0.0, 0.0, 1.0, 1.0], + detection_algorithm=self.algorithm, + occurrence=occ, + ) + return occ + + def _make_occurrence_with_only_null_detection(self) -> Occurrence: + occ = Occurrence.objects.create( + project=self.project, + event=self.event, + deployment=self.deployment, + determination=self.taxon, + ) + Detection.objects.create( + source_image=self.source_image, + bbox=None, + detection_algorithm=self.algorithm, + occurrence=occ, + ) + return occ + + def _make_occurrence_with_null_determination(self) -> Occurrence: + occ = Occurrence.objects.create( + project=self.project, + event=self.event, + deployment=self.deployment, + determination=None, + ) + Detection.objects.create( + source_image=self.source_image, + bbox=[0.0, 0.0, 1.0, 1.0], + detection_algorithm=self.algorithm, + occurrence=occ, + ) + return occ + + def test_valid_returns_only_real_with_determination(self): + real = self._make_occurrence_with_real_detection() + null_only = self._make_occurrence_with_only_null_detection() + no_determination = self._make_occurrence_with_null_determination() + + valid_qs = Occurrence.objects.filter(project=self.project).valid() + valid_pks = set(valid_qs.values_list("pk", flat=True)) + + self.assertIn(real.pk, valid_pks) + self.assertNotIn(null_only.pk, valid_pks) + self.assertNotIn(no_determination.pk, valid_pks) From c207ae54b4679343c6b349c99f50af120a578006 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 20 May 2026 15:58:58 -0700 Subject: [PATCH 07/11] feat(main): cleanup_null_only_occurrences management command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../commands/cleanup_null_only_occurrences.py | 84 +++++++++++++++ ami/main/tests.py | 102 ++++++++++++++++++ 2 files changed, 186 insertions(+) create mode 100644 ami/main/management/commands/cleanup_null_only_occurrences.py diff --git a/ami/main/management/commands/cleanup_null_only_occurrences.py b/ami/main/management/commands/cleanup_null_only_occurrences.py new file mode 100644 index 000000000..11e2dd26f --- /dev/null +++ b/ami/main/management/commands/cleanup_null_only_occurrences.py @@ -0,0 +1,84 @@ +""" +Delete phantom Occurrences and orphan null-marker Detections left by the Issue #1310 +field bug, on a per-project basis. + +The bug created two categories of rows that should never have been persisted: +- Occurrence rows with no real detections (or with determination=NULL), surfaced as + ghost rows in the API. +- Detection rows that mark a SourceImage as "processed" while no real detections + exist for it — these prevent filter_processed_images from re-yielding the image + on the next ML run. + +After cleanup, the source images become eligible for re-processing. + +Dry-run by default. Pass --commit to delete. +""" + +from django.core.management.base import BaseCommand, CommandError +from django.db import transaction +from django.db.models import Exists, OuterRef + +from ami.main.models import Detection, Occurrence, Project + + +class Command(BaseCommand): + help = "Delete phantom Occurrences and orphan null-marker Detections (Issue #1310)." + + def add_arguments(self, parser): + parser.add_argument( + "--project", + type=int, + required=True, + help="Project ID to clean up.", + ) + parser.add_argument( + "--commit", + action="store_true", + help="Actually delete. Defaults to dry-run.", + ) + + def handle(self, *args, **options): + project_id: int = options["project"] + commit: bool = options["commit"] + + try: + project = Project.objects.get(pk=project_id) + except Project.DoesNotExist as err: + raise CommandError(f"Project {project_id} does not exist") from err + + all_occs = Occurrence.objects.filter(project=project) + valid_occs = all_occs.valid() + phantom_occs = all_occs.exclude(pk__in=valid_occs.values("pk")) + + has_valid_detection = Detection.objects.valid().filter(source_image_id=OuterRef("source_image_id")) + orphan_null_markers = ( + Detection.objects.filter(source_image__project=project) + .null_markers() + .annotate(_has_valid=Exists(has_valid_detection)) + .filter(_has_valid=False) + ) + + phantom_count = phantom_occs.count() + null_count = orphan_null_markers.count() + + self.stdout.write(f"Project #{project.pk} ({project.name}):") + self.stdout.write(f" Phantom occurrences (no valid detection or null determination): {phantom_count}") + self.stdout.write(f" Orphan null-marker detections on images with no real detections: {null_count}") + + if phantom_count == 0 and null_count == 0: + self.stdout.write(self.style.SUCCESS("Nothing to clean up.")) + return + + if not commit: + self.stdout.write(self.style.WARNING("Dry run — pass --commit to delete.")) + return + + with transaction.atomic(): + null_deleted, _ = orphan_null_markers.delete() + phantom_deleted, _ = phantom_occs.delete() + + self.stdout.write( + self.style.SUCCESS( + f"Deleted {phantom_deleted} phantom occurrences and {null_deleted} orphan null markers." + ) + ) diff --git a/ami/main/tests.py b/ami/main/tests.py index 15011e94b..b46c4dc72 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -6171,3 +6171,105 @@ def test_valid_returns_only_real_with_determination(self): self.assertIn(real.pk, valid_pks) self.assertNotIn(null_only.pk, valid_pks) self.assertNotIn(no_determination.pk, valid_pks) + + +class TestCleanupNullOnlyOccurrencesCommand(TestCase): + """ + Covers ami/main/management/commands/cleanup_null_only_occurrences.py. + Verifies dry-run reports counts without deleting and --commit deletes + phantom occurrences and orphan null markers, leaving valid rows alone. + """ + + def setUp(self): + from ami.main.models import Taxon + from ami.ml.models.algorithm import Algorithm + + self.project = Project.objects.create(name="Cleanup Cmd Test Project") + self.deployment = Deployment.objects.create(project=self.project, name="dep") + self.event = Event.objects.create( + project=self.project, + deployment=self.deployment, + group_by="2024-01-01", + start=datetime.datetime(2024, 1, 1, 0, 0), + ) + self.algorithm = Algorithm.objects.create( + name="cleanup-detector", key="cleanup-detector", task_type="detection" + ) + self.taxon = Taxon.objects.create(name="Cleanup Test Taxon") + + self.img_with_real = SourceImage.objects.create( + deployment=self.deployment, + project=self.project, + event=self.event, + path="with-real.jpg", + ) + self.img_only_null = SourceImage.objects.create( + deployment=self.deployment, + project=self.project, + event=self.event, + path="only-null.jpg", + ) + + self.valid_occurrence = Occurrence.objects.create( + project=self.project, + event=self.event, + deployment=self.deployment, + determination=self.taxon, + ) + self.real_detection = Detection.objects.create( + source_image=self.img_with_real, + bbox=[0.0, 0.0, 1.0, 1.0], + detection_algorithm=self.algorithm, + occurrence=self.valid_occurrence, + ) + self.null_on_processed_image = Detection.objects.create( + source_image=self.img_with_real, + bbox=None, + detection_algorithm=self.algorithm, + ) + + self.phantom_occurrence = Occurrence.objects.create( + project=self.project, + event=self.event, + deployment=self.deployment, + determination=self.taxon, + ) + self.phantom_null = Detection.objects.create( + source_image=self.img_only_null, + bbox=None, + detection_algorithm=self.algorithm, + occurrence=self.phantom_occurrence, + ) + + def _call_command(self, *args): + from io import StringIO + + from django.core.management import call_command + + out = StringIO() + call_command("cleanup_null_only_occurrences", *args, stdout=out) + return out.getvalue() + + def test_dry_run_reports_counts_without_deleting(self): + output = self._call_command(f"--project={self.project.pk}") + self.assertIn("Phantom occurrences", output) + self.assertIn("Orphan null-marker detections", output) + self.assertIn("Dry run", output) + self.assertTrue(Occurrence.objects.filter(pk=self.phantom_occurrence.pk).exists()) + self.assertTrue(Detection.objects.filter(pk=self.phantom_null.pk).exists()) + + def test_commit_deletes_phantoms_and_orphan_null_markers(self): + self._call_command(f"--project={self.project.pk}", "--commit") + self.assertFalse(Occurrence.objects.filter(pk=self.phantom_occurrence.pk).exists()) + self.assertFalse(Detection.objects.filter(pk=self.phantom_null.pk).exists()) + self.assertTrue(Occurrence.objects.filter(pk=self.valid_occurrence.pk).exists()) + self.assertTrue(Detection.objects.filter(pk=self.real_detection.pk).exists()) + self.assertTrue( + Detection.objects.filter(pk=self.null_on_processed_image.pk).exists(), + "Null markers on images with at least one real detection must be kept", + ) + + def test_commit_is_idempotent(self): + self._call_command(f"--project={self.project.pk}", "--commit") + second_run = self._call_command(f"--project={self.project.pk}", "--commit") + self.assertIn("Nothing to clean up", second_run) From 8e4bbf33ba533d7015af8c769081c280aa9dd8eb Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 20 May 2026 16:31:53 -0700 Subject: [PATCH 08/11] fix(main,ml): apply CodeRabbit review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ami/main/api/views.py | 4 +--- .../commands/cleanup_null_only_occurrences.py | 12 +++++----- ami/main/models.py | 6 ++--- ami/main/tests.py | 5 ++++- ami/ml/models/pipeline.py | 22 ++++++++++--------- 5 files changed, 27 insertions(+), 22 deletions(-) diff --git a/ami/main/api/views.py b/ami/main/api/views.py index 1a94a417d..e44ee1b60 100644 --- a/ami/main/api/views.py +++ b/ami/main/api/views.py @@ -703,9 +703,7 @@ def filter_by_has_detections(self, queryset: QuerySet) -> QuerySet: 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")).exclude(NULL_DETECTIONS_FILTER) - ), + has_detections=models.Exists(Detection.objects.valid().filter(source_image=models.OuterRef("pk"))), ).filter(has_detections=has_detections) return queryset diff --git a/ami/main/management/commands/cleanup_null_only_occurrences.py b/ami/main/management/commands/cleanup_null_only_occurrences.py index 11e2dd26f..a974c9fec 100644 --- a/ami/main/management/commands/cleanup_null_only_occurrences.py +++ b/ami/main/management/commands/cleanup_null_only_occurrences.py @@ -74,11 +74,13 @@ def handle(self, *args, **options): return with transaction.atomic(): - null_deleted, _ = orphan_null_markers.delete() - phantom_deleted, _ = phantom_occs.delete() + orphan_null_markers.delete() + phantom_occs.delete() + # Report the pre-calculated counts of the rows we targeted directly. The tuple from + # .delete() also counts cascade-deleted related rows (e.g. classifications under a + # phantom occurrence's detections), which would inflate the numbers and confuse the + # operator about what the command actually targeted. self.stdout.write( - self.style.SUCCESS( - f"Deleted {phantom_deleted} phantom occurrences and {null_deleted} orphan null markers." - ) + self.style.SUCCESS(f"Deleted {phantom_count} phantom occurrences and {null_count} orphan null markers.") ) diff --git a/ami/main/models.py b/ami/main/models.py index 59ca23234..413aff936 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -3146,7 +3146,6 @@ def build_null_marker(cls, source_image, detection_algorithm) -> "Detection": source_image=source_image, bbox=cls.NULL_BBOX, detection_algorithm=detection_algorithm, - timestamp=timezone.now(), ) def get_bbox(self): @@ -5073,10 +5072,11 @@ def sample_greatest_file_size_from_each_event(self, num_each: int = 1): return captures def sample_detections_only(self): - """Sample all source images with detections""" + """Sample all source images with at least one real (non-null-marker) detection.""" qs = self.get_queryset() - return qs.filter(detections__isnull=False).distinct() + valid_detection_image_ids = Detection.objects.valid().values("source_image_id") + return qs.filter(pk__in=valid_detection_image_ids).distinct() def sample_full( self, diff --git a/ami/main/tests.py b/ami/main/tests.py index b46c4dc72..d5396c568 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -6059,7 +6059,10 @@ def test_build_null_marker_sets_canonical_fields(self): self.assertIsNone(det.bbox) self.assertEqual(det.source_image, self.source_image) self.assertEqual(det.detection_algorithm, self.algorithm) - self.assertIsNotNone(det.timestamp) + # timestamp is intentionally not set on the builder — Detection.save() backfills + # it from the source image's capture time, so the marker sorts by capture time + # rather than processing time. + self.assertIsNone(det.timestamp) self.assertTrue(det.is_null_marker) def test_valid_and_null_markers_are_disjoint_and_complete(self): diff --git a/ami/ml/models/pipeline.py b/ami/ml/models/pipeline.py index 19652fe2e..a9b5acbac 100644 --- a/ami/ml/models/pipeline.py +++ b/ami/ml/models/pipeline.py @@ -545,11 +545,10 @@ def get_or_create_detection( ), f"Detection belongs to a different source image: {detection_repr}" if serialized_bbox is None: - # Null detection: algorithm-specific lookup so different pipelines don't share sentinels. - # Use bbox__isnull=True (not Detection.objects.null_markers()) because we want to find - # an exact match for THIS algorithm — null_markers() also includes legacy bbox=[] rows - # from other pipelines and would be wider than this dedup needs. - # See Detection.NULL_BBOX for the canonical sentinel value used by new writes. + # Null marker: algorithm-specific lookup so different pipelines don't share sentinels. + # Use .null_markers() so legacy bbox=[] sentinels from older runs are also matched and + # re-used instead of producing duplicate rows. Detection.NULL_BBOX is the canonical + # sentinel value used for new writes; .null_markers() recognises both forms. assert detection_resp.algorithm, f"No detection algorithm was specified for detection {detection_repr}" try: detection_algo = algorithms_known[detection_resp.algorithm.key] @@ -559,11 +558,14 @@ def get_or_create_detection( "The processing service must declare it in the /info endpoint. " f"Known algorithms: {list(algorithms_known.keys())}" ) from err - 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() + ) else: # Real detection: algorithm-agnostic — same bbox = same physical detection existing_detection = Detection.objects.filter( From fef1cb6c93401c8aa884d59722aefa70523bca26 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Mon, 22 Jun 2026 17:11:47 -0700 Subject: [PATCH 09/11] refactor(main): rename 'orphan' null markers to 'dangling'; test null-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. --- .../commands/cleanup_null_only_occurrences.py | 14 ++++---- ami/main/models.py | 2 +- ami/main/tests.py | 34 +++++++++++++++++-- 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/ami/main/management/commands/cleanup_null_only_occurrences.py b/ami/main/management/commands/cleanup_null_only_occurrences.py index a974c9fec..e96167f8b 100644 --- a/ami/main/management/commands/cleanup_null_only_occurrences.py +++ b/ami/main/management/commands/cleanup_null_only_occurrences.py @@ -1,5 +1,5 @@ """ -Delete phantom Occurrences and orphan null-marker Detections left by the Issue #1310 +Delete phantom Occurrences and dangling null-marker Detections left by the Issue #1310 field bug, on a per-project basis. The bug created two categories of rows that should never have been persisted: @@ -22,7 +22,7 @@ class Command(BaseCommand): - help = "Delete phantom Occurrences and orphan null-marker Detections (Issue #1310)." + help = "Delete phantom Occurrences and dangling null-marker Detections (Issue #1310)." def add_arguments(self, parser): parser.add_argument( @@ -51,7 +51,7 @@ def handle(self, *args, **options): phantom_occs = all_occs.exclude(pk__in=valid_occs.values("pk")) has_valid_detection = Detection.objects.valid().filter(source_image_id=OuterRef("source_image_id")) - orphan_null_markers = ( + dangling_null_markers = ( Detection.objects.filter(source_image__project=project) .null_markers() .annotate(_has_valid=Exists(has_valid_detection)) @@ -59,11 +59,11 @@ def handle(self, *args, **options): ) phantom_count = phantom_occs.count() - null_count = orphan_null_markers.count() + null_count = dangling_null_markers.count() self.stdout.write(f"Project #{project.pk} ({project.name}):") self.stdout.write(f" Phantom occurrences (no valid detection or null determination): {phantom_count}") - self.stdout.write(f" Orphan null-marker detections on images with no real detections: {null_count}") + self.stdout.write(f" Dangling null-marker detections on images with no real detections: {null_count}") if phantom_count == 0 and null_count == 0: self.stdout.write(self.style.SUCCESS("Nothing to clean up.")) @@ -74,7 +74,7 @@ def handle(self, *args, **options): return with transaction.atomic(): - orphan_null_markers.delete() + dangling_null_markers.delete() phantom_occs.delete() # Report the pre-calculated counts of the rows we targeted directly. The tuple from @@ -82,5 +82,5 @@ def handle(self, *args, **options): # phantom occurrence's detections), which would inflate the numbers and confuse the # operator about what the command actually targeted. self.stdout.write( - self.style.SUCCESS(f"Deleted {phantom_count} phantom occurrences and {null_count} orphan null markers.") + self.style.SUCCESS(f"Deleted {phantom_count} phantom occurrences and {null_count} dangling null markers.") ) diff --git a/ami/main/models.py b/ami/main/models.py index 413aff936..9fc1615d9 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -3273,7 +3273,7 @@ def valid(self): a determination set. Excludes: - - Occurrences with no detections at all (orphans) + - Occurrences with no detections at all (empty occurrences) - Occurrences whose only detections are null-marker sentinels (Issue #1310: field bug created phantom occurrences with no real bounding box backing them) diff --git a/ami/main/tests.py b/ami/main/tests.py index d5396c568..e8231d73c 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -6180,7 +6180,7 @@ class TestCleanupNullOnlyOccurrencesCommand(TestCase): """ Covers ami/main/management/commands/cleanup_null_only_occurrences.py. Verifies dry-run reports counts without deleting and --commit deletes - phantom occurrences and orphan null markers, leaving valid rows alone. + phantom occurrences and dangling null markers, leaving valid rows alone. """ def setUp(self): @@ -6244,6 +6244,30 @@ def setUp(self): occurrence=self.phantom_occurrence, ) + # Second phantom shape: a real detection but no determination. valid() + # excludes occurrences with determination=NULL, so the command must + # treat this as a phantom too — it exercises the other exclusion arm, + # distinct from phantom_occurrence (which is excluded for having no real + # detection). + self.img_no_determination = SourceImage.objects.create( + deployment=self.deployment, + project=self.project, + event=self.event, + path="no-determination.jpg", + ) + self.phantom_no_determination = Occurrence.objects.create( + project=self.project, + event=self.event, + deployment=self.deployment, + determination=None, + ) + self.real_detection_no_determination = Detection.objects.create( + source_image=self.img_no_determination, + bbox=[0.0, 0.0, 1.0, 1.0], + detection_algorithm=self.algorithm, + occurrence=self.phantom_no_determination, + ) + def _call_command(self, *args): from io import StringIO @@ -6256,12 +6280,12 @@ def _call_command(self, *args): def test_dry_run_reports_counts_without_deleting(self): output = self._call_command(f"--project={self.project.pk}") self.assertIn("Phantom occurrences", output) - self.assertIn("Orphan null-marker detections", output) + self.assertIn("Dangling null-marker detections", output) self.assertIn("Dry run", output) self.assertTrue(Occurrence.objects.filter(pk=self.phantom_occurrence.pk).exists()) self.assertTrue(Detection.objects.filter(pk=self.phantom_null.pk).exists()) - def test_commit_deletes_phantoms_and_orphan_null_markers(self): + def test_commit_deletes_phantoms_and_dangling_null_markers(self): self._call_command(f"--project={self.project.pk}", "--commit") self.assertFalse(Occurrence.objects.filter(pk=self.phantom_occurrence.pk).exists()) self.assertFalse(Detection.objects.filter(pk=self.phantom_null.pk).exists()) @@ -6271,6 +6295,10 @@ def test_commit_deletes_phantoms_and_orphan_null_markers(self): Detection.objects.filter(pk=self.null_on_processed_image.pk).exists(), "Null markers on images with at least one real detection must be kept", ) + self.assertFalse( + Occurrence.objects.filter(pk=self.phantom_no_determination.pk).exists(), + "Occurrence with a real detection but no determination must be deleted as a phantom", + ) def test_commit_is_idempotent(self): self._call_command(f"--project={self.project.pk}", "--commit") From 8c019e43e64e6201d32a815e17b1bb0b709f60b6 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Mon, 22 Jun 2026 17:51:16 -0700 Subject: [PATCH 10/11] fix(main): scope cleanup to true #1310 phantoms; drop dead bbox=[] sentinel 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) --- .../commands/cleanup_null_only_occurrences.py | 19 +++++-- ami/main/models.py | 23 ++++---- ami/main/tests.py | 52 +++++++++---------- ami/ml/models/pipeline.py | 5 +- 4 files changed, 54 insertions(+), 45 deletions(-) diff --git a/ami/main/management/commands/cleanup_null_only_occurrences.py b/ami/main/management/commands/cleanup_null_only_occurrences.py index e96167f8b..d31c5233f 100644 --- a/ami/main/management/commands/cleanup_null_only_occurrences.py +++ b/ami/main/management/commands/cleanup_null_only_occurrences.py @@ -3,8 +3,8 @@ field bug, on a per-project basis. The bug created two categories of rows that should never have been persisted: -- Occurrence rows with no real detections (or with determination=NULL), surfaced as - ghost rows in the API. +- Occurrence rows with no real detections (their only detections are null-marker + sentinels, or they have none at all), surfaced as ghost rows in the API. - Detection rows that mark a SourceImage as "processed" while no real detections exist for it — these prevent filter_processed_images from re-yielding the image on the next ML run. @@ -47,8 +47,17 @@ def handle(self, *args, **options): raise CommandError(f"Project {project_id} does not exist") from err all_occs = Occurrence.objects.filter(project=project) - valid_occs = all_occs.valid() - phantom_occs = all_occs.exclude(pk__in=valid_occs.values("pk")) + # Phantom = an occurrence with NO real (valid) detection backing it: its only detections + # are null-marker sentinels, or it has none at all. This is the Issue #1310 debris. + # + # Deliberately narrower than Occurrence.valid(): valid() ALSO excludes occurrences whose + # determination is null, but an occurrence that has a real detection and merely a missing + # determination is a different (partial-write) shape, not #1310 debris. Deleting it would + # SET_NULL the real detection's occurrence FK (Detection.occurrence is on_delete=SET_NULL), + # stranding a classified detection on an image that filter_processed_images then skips + # forever. Those are left for a separate, targeted repair. + has_valid_detection = Exists(Detection.objects.valid().filter(occurrence_id=OuterRef("pk"))) + phantom_occs = all_occs.exclude(has_valid_detection) has_valid_detection = Detection.objects.valid().filter(source_image_id=OuterRef("source_image_id")) dangling_null_markers = ( @@ -62,7 +71,7 @@ def handle(self, *args, **options): null_count = dangling_null_markers.count() self.stdout.write(f"Project #{project.pk} ({project.name}):") - self.stdout.write(f" Phantom occurrences (no valid detection or null determination): {phantom_count}") + self.stdout.write(f" Phantom occurrences (no real detection backing them): {phantom_count}") self.stdout.write(f" Dangling null-marker detections on images with no real detections: {null_count}") if phantom_count == 0 and null_count == 0: diff --git a/ami/main/models.py b/ami/main/models.py index 9fc1615d9..2dba65525 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -97,12 +97,10 @@ class TaxonRank(OrderedEnum): ] ) -NULL_DETECTIONS_FILTER = Q(bbox__isnull=True) | Q(bbox=[]) - def bbox_is_null(bbox) -> bool: - """In-memory equivalent of NULL_DETECTIONS_FILTER for already-fetched bbox values.""" - return bbox is None or bbox == [] + """In-memory equivalent of null_detections_q() for an already-fetched bbox value.""" + return bbox is None def null_detections_q(prefix: str = "") -> Q: @@ -111,8 +109,16 @@ def null_detections_q(prefix: str = "") -> Q: for use across relations (e.g. null_detections_q("images__detections__") for an aggregate filter on a parent table). For Detection queries directly, prefer Detection.objects.null_markers() / .valid() instead. + + Null markers are stored as SQL NULL (bbox IS NULL); that is the only sentinel form. """ - return Q(**{f"{prefix}bbox__isnull": True}) | Q(**{f"{prefix}bbox": []}) + return Q(**{f"{prefix}bbox__isnull": True}) + + +# Single source of truth for "this Detection is a null marker", shared by +# DetectionQuerySet.valid() / .null_markers(). Defined via null_detections_q() so the +# constant and the helper cannot drift apart. +NULL_DETECTIONS_FILTER = null_detections_q() def get_media_url(path: str) -> str: @@ -3130,14 +3136,13 @@ class Detection(BaseModel): NULL_BBOX = None """Canonical bbox value for null markers (rows that record 'an algorithm ran but - found nothing'). Use Detection.build_null_marker() to construct them. The legacy - bbox=[] form is still recognised by .null_markers() / .is_null_marker for - backwards compatibility with historical rows.""" + found nothing'). Null markers are stored as SQL NULL; use Detection.build_null_marker() + to construct them.""" @property def is_null_marker(self) -> bool: """True for sentinel rows representing 'no detections found by this algorithm.'""" - return self.bbox is None or self.bbox == [] + return self.bbox is None @classmethod def build_null_marker(cls, source_image, detection_algorithm) -> "Detection": diff --git a/ami/main/tests.py b/ami/main/tests.py index e8231d73c..4c8b4b219 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -6038,14 +6038,6 @@ def test_is_null_marker_for_bbox_none(self): ) self.assertTrue(det.is_null_marker) - def test_is_null_marker_for_bbox_empty_list_legacy(self): - det = Detection.objects.create( - source_image=self.source_image, - bbox=[], - detection_algorithm=self.algorithm, - ) - self.assertTrue(det.is_null_marker) - def test_is_null_marker_false_for_real_detection(self): det = Detection.objects.create( source_image=self.source_image, @@ -6071,21 +6063,16 @@ def test_valid_and_null_markers_are_disjoint_and_complete(self): bbox=[0.0, 0.0, 1.0, 1.0], detection_algorithm=self.algorithm, ) - null_via_none = Detection.objects.create( + null_marker = Detection.objects.create( source_image=self.source_image, bbox=None, detection_algorithm=self.algorithm, ) - null_via_empty = Detection.objects.create( - source_image=self.source_image, - bbox=[], - detection_algorithm=self.algorithm, - ) scoped = Detection.objects.filter(source_image=self.source_image) valid_pks = set(scoped.valid().values_list("pk", flat=True)) null_pks = set(scoped.null_markers().values_list("pk", flat=True)) self.assertEqual(valid_pks, {real.pk}) - self.assertEqual(null_pks, {null_via_none.pk, null_via_empty.pk}) + self.assertEqual(null_pks, {null_marker.pk}) self.assertEqual(valid_pks & null_pks, set()) @@ -6244,28 +6231,29 @@ def setUp(self): occurrence=self.phantom_occurrence, ) - # Second phantom shape: a real detection but no determination. valid() - # excludes occurrences with determination=NULL, so the command must - # treat this as a phantom too — it exercises the other exclusion arm, - # distinct from phantom_occurrence (which is excluded for having no real - # detection). - self.img_no_determination = SourceImage.objects.create( + # A different (partial-write) shape: an occurrence with a real detection but a + # missing determination. Occurrence.valid() excludes it (determination IS NULL), + # but the cleanup command must NOT delete it — doing so would SET_NULL the real + # detection's occurrence FK and strand a classified detection on an image that + # filter_processed_images then skips forever. The command's phantom predicate is + # deliberately narrower than valid() to spare exactly this case. + self.img_real_no_determination = SourceImage.objects.create( deployment=self.deployment, project=self.project, event=self.event, - path="no-determination.jpg", + path="real-no-determination.jpg", ) - self.phantom_no_determination = Occurrence.objects.create( + self.occ_real_no_determination = Occurrence.objects.create( project=self.project, event=self.event, deployment=self.deployment, determination=None, ) self.real_detection_no_determination = Detection.objects.create( - source_image=self.img_no_determination, + source_image=self.img_real_no_determination, bbox=[0.0, 0.0, 1.0, 1.0], detection_algorithm=self.algorithm, - occurrence=self.phantom_no_determination, + occurrence=self.occ_real_no_determination, ) def _call_command(self, *args): @@ -6295,9 +6283,17 @@ def test_commit_deletes_phantoms_and_dangling_null_markers(self): Detection.objects.filter(pk=self.null_on_processed_image.pk).exists(), "Null markers on images with at least one real detection must be kept", ) - self.assertFalse( - Occurrence.objects.filter(pk=self.phantom_no_determination.pk).exists(), - "Occurrence with a real detection but no determination must be deleted as a phantom", + self.assertTrue( + Occurrence.objects.filter(pk=self.occ_real_no_determination.pk).exists(), + "Occurrence with a real detection but a missing determination is a partial-write " + "shape, not Issue #1310 debris — it must be preserved, not deleted", + ) + self.real_detection_no_determination.refresh_from_db() + self.assertEqual( + self.real_detection_no_determination.occurrence_id, + self.occ_real_no_determination.pk, + "The real detection must keep its occurrence FK — deleting the occurrence would " + "SET_NULL it and strand the detection", ) def test_commit_is_idempotent(self): diff --git a/ami/ml/models/pipeline.py b/ami/ml/models/pipeline.py index a9b5acbac..2b7227439 100644 --- a/ami/ml/models/pipeline.py +++ b/ami/ml/models/pipeline.py @@ -546,9 +546,8 @@ def get_or_create_detection( if serialized_bbox is None: # Null marker: algorithm-specific lookup so different pipelines don't share sentinels. - # Use .null_markers() so legacy bbox=[] sentinels from older runs are also matched and - # re-used instead of producing duplicate rows. Detection.NULL_BBOX is the canonical - # sentinel value used for new writes; .null_markers() recognises both forms. + # Narrow to .null_markers() (rather than a bare filter) so an existing sentinel for this + # image+algorithm is re-used instead of matching a real detection or creating a duplicate. assert detection_resp.algorithm, f"No detection algorithm was specified for detection {detection_repr}" try: detection_algo = algorithms_known[detection_resp.algorithm.key] From a87133deb7a0209fba6bc942150437de7cca1856 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Mon, 22 Jun 2026 19:24:32 -0700 Subject: [PATCH 11/11] docs(ml): explain what existing_detection holds in each get_or_create_detection branch [skip ci] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- ami/ml/models/pipeline.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/ami/ml/models/pipeline.py b/ami/ml/models/pipeline.py index 2b7227439..7940207ac 100644 --- a/ami/ml/models/pipeline.py +++ b/ami/ml/models/pipeline.py @@ -545,9 +545,11 @@ def get_or_create_detection( ), f"Detection belongs to a different source image: {detection_repr}" if serialized_bbox is None: - # Null marker: algorithm-specific lookup so different pipelines don't share sentinels. - # Narrow to .null_markers() (rather than a bare filter) so an existing sentinel for this - # image+algorithm is re-used instead of matching a real detection or creating a duplicate. + # existing_detection := the null-marker sentinel already recorded for this image+algorithm + # (or None if there is none yet). The lookup is algorithm-specific so different pipelines + # don't share sentinels, and narrowed to .null_markers() rather than a bare filter: a null + # response has no bbox to match on, so without .null_markers() the (image, algorithm) filter + # would also return real detections, and .first() could reuse one as if it were the sentinel. assert detection_resp.algorithm, f"No detection algorithm was specified for detection {detection_repr}" try: detection_algo = algorithms_known[detection_resp.algorithm.key] @@ -566,7 +568,10 @@ def get_or_create_detection( .first() ) else: - # Real detection: algorithm-agnostic — same bbox = same physical detection + # existing_detection := the detection with this exact bbox on this image (or None). The + # match is algorithm-agnostic — the same bounding box on the same image is the same physical + # detection regardless of which algorithm found it, so a bbox match is a duplicate to reuse. + # A specific bbox can't match a null marker (bbox IS NULL), so sentinels are excluded here. existing_detection = Detection.objects.filter( source_image=source_image, bbox=serialized_bbox,