From 3e7d9dd0afe855ef4ae2c965e7b90d9aec3beee7 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 29 Apr 2026 07:49:41 -0700 Subject: [PATCH 01/13] fix(occurrences): prefetch detections/classifications in list view to remove N+1 Issue: https://github.com/RolnickLab/antenna/issues/1271 OccurrenceViewSet.list issued one query per occurrence to fetch detection paths, the best machine prediction, and the best identification, scaling roughly linearly with page size and producing the 34-40s cold-cache responses tracked in Sentry as AMI-PLATFORM-API-BMY. This change adds reusable Prefetch factories in ami/main/models_future/occurrence.py and wires them into the list path: - prefetch_detection_images() populates a `prefetched_detection_images` to_attr list of detections that have a path - prefetch_classifications_for_best_prediction() populates detections__classifications with select_related taxon and algorithm - best_prediction_from_prefetch / best_identification_from_prefetch pick the winners in Python without re-querying OccurrenceQuerySet.with_list_prefetches() bundles these prefetches and is called from OccurrenceViewSet.get_queryset only when action == 'list'. OccurrenceListSerializer reads from the prefetch caches when present and falls back to the existing model methods for non-list callers (admin, exports, signals). Local measurement on a project with 25 occurrences: page size | before | after | reduction limit=5 | 24 | 13 | 1.8x limit=25 | 64 | 7 | 9.1x A new TestOccurrenceListQueryCount guards against regression by asserting the query count does not grow with page size. Follow-ups (separate PR): - django-zen-queries QueriesDisabledViewMixin to fail loudly on lazy queries during list rendering - audit other large-list serializers (SourceImage, Taxon) for the same pattern Co-Authored-By: Claude --- ami/main/api/serializers.py | 53 +++++++++--- ami/main/api/views.py | 4 +- ami/main/models.py | 9 ++ ami/main/models_future/occurrence.py | 119 +++++++++++++++++++++++++++ ami/main/tests.py | 53 ++++++++++++ 5 files changed, 227 insertions(+), 11 deletions(-) create mode 100644 ami/main/models_future/occurrence.py diff --git a/ami/main/api/serializers.py b/ami/main/api/serializers.py index ec79603e7..aadc8a107 100644 --- a/ami/main/api/serializers.py +++ b/ami/main/api/serializers.py @@ -1318,6 +1318,7 @@ class OccurrenceListSerializer(DefaultSerializer): deployment = DeploymentNestedSerializer(read_only=True) event = EventNestedSerializer(read_only=True) # first_appearance = TaxonSourceImageNestedSerializer(read_only=True) + detection_images = serializers.SerializerMethodField() determination_details = serializers.SerializerMethodField() best_machine_prediction = serializers.SerializerMethodField() identifications = OccurrenceIdentificationSerializer(many=True, read_only=True) @@ -1364,27 +1365,59 @@ class Meta: "updated_at", ] - def get_determination_details(self, obj: Occurrence): - # @TODO convert this to query methods to avoid N+1 queries. - # Currently at 100+ queries per page of 10 occurrences. - # Add a reusable method to the OccurrenceQuerySet class and call it from the ViewSet. + def get_detection_images(self, obj: Occurrence) -> list[str]: + """Return media URLs for the occurrence's detection crops. - context = self.context + Reads from the `prefetched_detection_images` to_attr list populated by + `prefetch_detection_images()`; falls back to the model method for + callers that did not apply the prefetch (e.g. detail view). + """ + from ami.main.models_future.occurrence import detection_image_urls_from_prefetch + + if hasattr(obj, "prefetched_detection_images"): + return detection_image_urls_from_prefetch(obj) + return list(obj.detection_images()) + + def _best_identification(self, obj: Occurrence) -> Identification | None: + """Pick the best identification, preferring prefetched data when available.""" + from ami.main.models_future.occurrence import best_identification_from_prefetch + + # `with_identifications()` populates the relation cache; if it ran we + # can pick the best in Python without a fresh DB hit. + if "identifications" in getattr(obj, "_prefetched_objects_cache", {}): + return best_identification_from_prefetch(obj) + return obj.best_identification + def _best_prediction(self, obj: Occurrence): + """Pick the best machine prediction, preferring prefetched data when available.""" + from ami.main.models_future.occurrence import best_prediction_from_prefetch + + # `prefetch_classifications_for_best_prediction()` populates both the + # detections cache and the nested classifications cache. + prefetch_cache = getattr(obj, "_prefetched_objects_cache", {}) + if "detections" in prefetch_cache: + return best_prediction_from_prefetch(obj) + return obj.best_prediction + + def get_determination_details(self, obj: Occurrence): + context = self.context # Add this occurrence to the context so that the nested serializers can access it # the `parent` attribute is not available since we are manually instantiating the serializers context["occurrence"] = obj taxon = TaxonNestedSerializer(obj.determination, context=context).data if obj.determination else None - if obj.best_identification: - identification = OccurrenceIdentificationSerializer(obj.best_identification, context=context).data + + best_ident = self._best_identification(obj) + if best_ident: + identification = OccurrenceIdentificationSerializer(best_ident, context=context).data else: identification = None - if identification or not obj.best_prediction: + if identification: prediction = None else: - prediction = ClassificationNestedSerializer(obj.best_prediction, context=context).data + best_pred = self._best_prediction(obj) + prediction = ClassificationNestedSerializer(best_pred, context=context).data if best_pred else None return dict( taxon=taxon, @@ -1402,7 +1435,7 @@ def get_best_machine_prediction(self, obj: Occurrence) -> dict | None: context = self.context context["occurrence"] = obj - prediction = obj.best_prediction + prediction = self._best_prediction(obj) if not prediction: return None diff --git a/ami/main/api/views.py b/ami/main/api/views.py index 9f38a970b..9869fe996 100644 --- a/ami/main/api/views.py +++ b/ami/main/api/views.py @@ -1227,7 +1227,9 @@ def get_queryset(self) -> QuerySet["Occurrence"]: qs = qs.with_detections_count().with_timestamps() # type: ignore qs = qs.with_identifications() # type: ignore qs = qs.apply_default_filters(project, self.request) # type: ignore - if self.action != "list": + if self.action == "list": + qs = qs.with_list_prefetches() # type: ignore + else: qs = qs.prefetch_related( Prefetch( "detections", queryset=Detection.objects.order_by("-timestamp").select_related("source_image") diff --git a/ami/main/models.py b/ami/main/models.py index e91b395b7..c285f2a19 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -2932,6 +2932,15 @@ def with_identifications(self): "identifications__user", ) + def with_list_prefetches(self): + """Add prefetches the list serializer needs (detection paths, classifications). + + See `ami.main.models_future.occurrence` for the prefetch definitions. + """ + from ami.main.models_future.occurrence import prefetches_for_list_serializer + + return self.prefetch_related(*prefetches_for_list_serializer()) + def with_best_detection(self): """ Annotate the queryset with fields from the best detection. diff --git a/ami/main/models_future/occurrence.py b/ami/main/models_future/occurrence.py new file mode 100644 index 000000000..e30cfcb9e --- /dev/null +++ b/ami/main/models_future/occurrence.py @@ -0,0 +1,119 @@ +""" +Reusable Prefetch factories for Occurrence list-view rendering. + +Centralising these lets the queryset, serializer, and any future caller share +a single source of truth for what data the list view needs eagerly loaded. + +Tracking issue: https://github.com/RolnickLab/antenna/issues/1271 +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from django.db.models import Prefetch + +if TYPE_CHECKING: + from ami.main.models import Classification, Identification, Occurrence + + +def prefetch_detection_images() -> Prefetch: + """Detections with non-null `path`, ordered for stable image lists. + + Populates `Occurrence.prefetched_detection_images` so the serializer can + yield image URLs without a per-occurrence query. + """ + from ami.main.models import Detection + + return Prefetch( + "detections", + queryset=( + Detection.objects.exclude(path=None) + .only("id", "occurrence_id", "path", "frame_num", "timestamp") + .order_by("frame_num", "timestamp") + ), + to_attr="prefetched_detection_images", + ) + + +def prefetch_classifications_for_best_prediction() -> Prefetch: + """All classifications under each occurrence's detections. + + `taxon` and `algorithm` are joined so picking the best prediction in + Python (via BEST_MACHINE_PREDICTION_ORDER) does not lazy-load either. + + The default `detections` relation manager populates `obj.detections.all()` + which the caller can walk to reach `.classifications.all()`. + """ + from ami.main.models import Classification + + return Prefetch( + "detections__classifications", + queryset=Classification.objects.select_related("taxon", "algorithm"), + ) + + +def prefetches_for_list_serializer() -> list[Prefetch]: + """All prefetches `OccurrenceListSerializer` needs to render without N+1. + + Identifications are covered by `OccurrenceQuerySet.with_identifications()` + which is already applied in the list viewset; intentionally not duplicated + here. + """ + return [ + prefetch_detection_images(), + prefetch_classifications_for_best_prediction(), + ] + + +def best_prediction_from_prefetch(occurrence: Occurrence) -> Classification | None: + """Pick the best machine prediction from a prefetched occurrence in Python. + + Mirrors the ordering used by `BEST_MACHINE_PREDICTION_ORDER` + (`-terminal`, `-score`, `-pk`). + + Requires `prefetch_classifications_for_best_prediction()` to have been + applied; walks `obj.detections.all()` -> `det.classifications.all()` from + the prefetch cache. + """ + classifications: list = [] + for det in occurrence.detections.all(): + classifications.extend(det.classifications.all()) + if not classifications: + return None + classifications.sort( + key=lambda c: ( + 0 if getattr(c, "terminal", False) else 1, + -(c.score or 0.0), + -c.pk, + ) + ) + return classifications[0] + + +def best_identification_from_prefetch(occurrence: Occurrence) -> Identification | None: + """Pick the most recent non-withdrawn identification from prefetched data. + + Mirrors `Occurrence.best_identification` but reads from the prefetched + `identifications` relation rather than issuing a fresh query. + """ + best: Identification | None = None + for ident in occurrence.identifications.all(): + if ident.withdrawn: + continue + if best is None or (ident.created_at and best.created_at and ident.created_at > best.created_at): + best = ident + return best + + +def detection_image_urls_from_prefetch(occurrence: Occurrence, limit: int | None = None) -> list[str]: + """Return media URLs from the `prefetched_detection_images` to_attr list. + + Requires `prefetch_detection_images()` to have been applied. + """ + from ami.main.models import get_media_url + + detections = getattr(occurrence, "prefetched_detection_images", []) + if limit is not None: + detections = detections[:limit] + return [get_media_url(det.path) for det in detections] diff --git a/ami/main/tests.py b/ami/main/tests.py index 0e7672e1c..590a20734 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -2927,6 +2927,59 @@ def test_taxa_include_occurrence_determinations_not_directly_linked(self): ) +class TestOccurrenceListQueryCount(APITestCase): + """Guard against N+1 regressions in OccurrenceViewSet.list (issue #1271). + + The number of queries should be roughly constant regardless of page size: + a small handful of pagination/auth/permission queries plus a fixed set of + prefetches (occurrences, detections, classifications, identifications, etc.). + """ + + def setUp(self): + self.project, self.deployment = setup_test_project() + create_taxa(self.project) + create_captures(deployment=self.deployment, num_nights=1, images_per_night=10) + + # Use a low threshold so all occurrences are returned + self.project.default_filters_score_threshold = 0.0 + self.project.save() + + self.user = User.objects.create_user(email="qcount@insectai.org", is_staff=False, is_superuser=False) + self.client.force_authenticate(user=self.user) + + def _list_occurrences(self, limit: int) -> int: + from django.core.cache import caches + from django.test.utils import CaptureQueriesContext + + url = f"/api/v2/occurrences/?project_id={self.project.pk}&limit={limit}" + # Clear cachalot cache between runs so query counts reflect cold state + try: + caches["default"].clear() + except Exception: + pass + + with CaptureQueriesContext(connection) as ctx: + res = self.client.get(url) + self.assertEqual(res.status_code, status.HTTP_200_OK) + self.assertEqual(len(res.data["results"]), min(limit, res.data["count"])) + return len(ctx.captured_queries) + + def test_list_query_count_does_not_scale_with_page_size(self): + """Query count for a page of 25 should be very close to a page of 5.""" + create_occurrences(deployment=self.deployment, num=25, determination_score=0.9) + + small_count = self._list_occurrences(limit=5) + large_count = self._list_occurrences(limit=25) + + # Allow a small constant overhead (e.g. cachalot bookkeeping) but + # nowhere near 5x scaling. + self.assertLessEqual( + large_count, + small_count + 5, + f"Query count grew with page size: {small_count} -> {large_count} (likely N+1 regression)", + ) + + class TestProjectDefaultTaxaFilter(APITestCase): """ Tests for project default taxa filtering (include/exclude lists). From b62fb399cb738ae79c368668a0c587bc3abc8c05 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 29 Apr 2026 12:59:41 -0700 Subject: [PATCH 02/13] fix(occurrences): address PR #1274 review feedback - Prevent detail-path N+1: `_best_prediction` now requires both `detections` AND nested `classifications` to be in the prefetch cache before calling `best_prediction_from_prefetch`. Detail viewset only prefetches `detections`, so the previous gate still walked `det.classifications.all()` lazily (one query per detection). - Mirror `Occurrence.best_prediction` semantics in the prefetch helper: `Occurrence.predictions()` filters classifications to the per-algorithm max-score row before ordering by `-terminal, -score`. The helper now applies the same per-algorithm grouping in Python so list and detail return the same winner for occurrences whose top-scoring classification is non-terminal. - Add deterministic tiebreaker to `best_identification_from_prefetch`: tuple-keyed comparison (`created_at_is_set, created_at, pk`) treats `None` timestamps as lowest and breaks ties by `pk`, matching `order_by('-created_at', '-pk')`. - Collapse the duplicate `detections` prefetch: prior PR loaded detections twice (once via a `to_attr` filtered list, once via `detections__classifications`). Replace both with a single `prefetch_detections_for_list()` that nests classifications, and filter for non-null paths in Python. Saves one query per request. - Don't swallow cache-clear failures in the regression guard. A silent cache-clear failure would let the warm cache hide query-count regressions. Refs: #1271 PR: #1274 Co-Authored-By: Claude --- ami/main/api/serializers.py | 24 +++--- ami/main/models_future/occurrence.py | 110 ++++++++++++++++----------- ami/main/tests.py | 8 +- 3 files changed, 83 insertions(+), 59 deletions(-) diff --git a/ami/main/api/serializers.py b/ami/main/api/serializers.py index aadc8a107..09e5ecab9 100644 --- a/ami/main/api/serializers.py +++ b/ami/main/api/serializers.py @@ -1368,13 +1368,13 @@ class Meta: def get_detection_images(self, obj: Occurrence) -> list[str]: """Return media URLs for the occurrence's detection crops. - Reads from the `prefetched_detection_images` to_attr list populated by - `prefetch_detection_images()`; falls back to the model method for - callers that did not apply the prefetch (e.g. detail view). + Reads from the prefetched `detections` cache populated by + `prefetch_detections_for_list()`; falls back to the model method for + callers that did not apply the prefetch (e.g. signals/exports). """ from ami.main.models_future.occurrence import detection_image_urls_from_prefetch - if hasattr(obj, "prefetched_detection_images"): + if "detections" in getattr(obj, "_prefetched_objects_cache", {}): return detection_image_urls_from_prefetch(obj) return list(obj.detection_images()) @@ -1389,13 +1389,17 @@ def _best_identification(self, obj: Occurrence) -> Identification | None: return obj.best_identification def _best_prediction(self, obj: Occurrence): - """Pick the best machine prediction, preferring prefetched data when available.""" - from ami.main.models_future.occurrence import best_prediction_from_prefetch + """Pick the best machine prediction, preferring prefetched data when available. - # `prefetch_classifications_for_best_prediction()` populates both the - # detections cache and the nested classifications cache. - prefetch_cache = getattr(obj, "_prefetched_objects_cache", {}) - if "detections" in prefetch_cache: + The list path prefetches detections AND nested classifications via + `prefetch_detections_for_list()`. The detail path only prefetches + detections, so calling `best_prediction_from_prefetch()` there would + walk `det.classifications.all()` lazily and reintroduce an N+1. + Gate strictly on classifications also being prefetched. + """ + from ami.main.models_future.occurrence import best_prediction_from_prefetch, has_prefetched_classifications + + if has_prefetched_classifications(obj): return best_prediction_from_prefetch(obj) return obj.best_prediction diff --git a/ami/main/models_future/occurrence.py b/ami/main/models_future/occurrence.py index e30cfcb9e..b08fc5cb5 100644 --- a/ami/main/models_future/occurrence.py +++ b/ami/main/models_future/occurrence.py @@ -17,39 +17,30 @@ from ami.main.models import Classification, Identification, Occurrence -def prefetch_detection_images() -> Prefetch: - """Detections with non-null `path`, ordered for stable image lists. +def prefetch_detections_for_list() -> Prefetch: + """Single detections prefetch covering image URL listing AND best-prediction selection. - Populates `Occurrence.prefetched_detection_images` so the serializer can - yield image URLs without a per-occurrence query. + One pass loads detections (ordered for stable image lists) plus their + classifications with `taxon`/`algorithm` joined. The serializer derives + image URLs by filtering `path is not None` in Python and picks the best + machine prediction from the same cache. + + Replaces the previous pair (a `to_attr` filtered list for image paths + plus a separate `detections__classifications` prefetch) which loaded the + detections relation twice. """ - from ami.main.models import Detection + from ami.main.models import Classification, Detection return Prefetch( "detections", queryset=( - Detection.objects.exclude(path=None) - .only("id", "occurrence_id", "path", "frame_num", "timestamp") - .order_by("frame_num", "timestamp") + Detection.objects.prefetch_related( + Prefetch( + "classifications", + queryset=Classification.objects.select_related("taxon", "algorithm"), + ) + ).order_by("frame_num", "timestamp") ), - to_attr="prefetched_detection_images", - ) - - -def prefetch_classifications_for_best_prediction() -> Prefetch: - """All classifications under each occurrence's detections. - - `taxon` and `algorithm` are joined so picking the best prediction in - Python (via BEST_MACHINE_PREDICTION_ORDER) does not lazy-load either. - - The default `detections` relation manager populates `obj.detections.all()` - which the caller can walk to reach `.classifications.all()`. - """ - from ami.main.models import Classification - - return Prefetch( - "detections__classifications", - queryset=Classification.objects.select_related("taxon", "algorithm"), ) @@ -60,60 +51,91 @@ def prefetches_for_list_serializer() -> list[Prefetch]: which is already applied in the list viewset; intentionally not duplicated here. """ - return [ - prefetch_detection_images(), - prefetch_classifications_for_best_prediction(), - ] + return [prefetch_detections_for_list()] + + +def has_prefetched_classifications(occurrence: Occurrence) -> bool: + """Return True iff `detections` AND each detection's `classifications` are prefetched. + + The list path prefetches both via `prefetch_detections_for_list()`. The + detail path prefetches only `detections` — calling + `best_prediction_from_prefetch()` there would walk `det.classifications.all()` + and reintroduce an N+1 (one query per detection). + """ + cache = getattr(occurrence, "_prefetched_objects_cache", {}) + detections = cache.get("detections") + if detections is None: + return False + return all("classifications" in getattr(det, "_prefetched_objects_cache", {}) for det in detections) def best_prediction_from_prefetch(occurrence: Occurrence) -> Classification | None: """Pick the best machine prediction from a prefetched occurrence in Python. - Mirrors the ordering used by `BEST_MACHINE_PREDICTION_ORDER` - (`-terminal`, `-score`, `-pk`). + Mirrors `Occurrence.best_prediction`, which calls `Occurrence.predictions()` + (per-algorithm max-score filtering) and then orders by `-terminal, -score`. + Replicating that grouping in Python keeps list and detail responses + consistent for occurrences whose top-scoring classification is non-terminal. - Requires `prefetch_classifications_for_best_prediction()` to have been + Requires `prefetch_detections_for_list()` (or equivalent) to have been applied; walks `obj.detections.all()` -> `det.classifications.all()` from the prefetch cache. """ - classifications: list = [] - for det in occurrence.detections.all(): - classifications.extend(det.classifications.all()) + classifications = [c for det in occurrence.detections.all() for c in det.classifications.all()] if not classifications: return None - classifications.sort( + + max_score_per_algo: dict[object, float] = {} + for c in classifications: + score = c.score if c.score is not None else float("-inf") + existing = max_score_per_algo.get(c.algorithm_id) + if existing is None or score > existing: + max_score_per_algo[c.algorithm_id] = score + + candidates = [ + c + for c in classifications + if (c.score if c.score is not None else float("-inf")) == max_score_per_algo[c.algorithm_id] + ] + if not candidates: + return None + candidates.sort( key=lambda c: ( 0 if getattr(c, "terminal", False) else 1, -(c.score or 0.0), -c.pk, ) ) - return classifications[0] + return candidates[0] def best_identification_from_prefetch(occurrence: Occurrence) -> Identification | None: """Pick the most recent non-withdrawn identification from prefetched data. - Mirrors `Occurrence.best_identification` but reads from the prefetched - `identifications` relation rather than issuing a fresh query. + Mirrors `Occurrence.best_identification` (`order_by("-created_at")`), with + a `pk` tiebreaker so equal timestamps produce a deterministic winner. + `created_at=None` is treated as lower than any real timestamp. """ best: Identification | None = None + best_key: tuple[bool, object, int] | None = None for ident in occurrence.identifications.all(): if ident.withdrawn: continue - if best is None or (ident.created_at and best.created_at and ident.created_at > best.created_at): + ident_key: tuple[bool, object, int] = (ident.created_at is not None, ident.created_at, ident.pk) + if best_key is None or ident_key > best_key: best = ident + best_key = ident_key return best def detection_image_urls_from_prefetch(occurrence: Occurrence, limit: int | None = None) -> list[str]: - """Return media URLs from the `prefetched_detection_images` to_attr list. + """Return media URLs for the prefetched detections (filtering out `path=None`). - Requires `prefetch_detection_images()` to have been applied. + Requires `prefetch_detections_for_list()` to have been applied. """ from ami.main.models import get_media_url - detections = getattr(occurrence, "prefetched_detection_images", []) + detections = [det for det in occurrence.detections.all() if det.path] if limit is not None: detections = detections[:limit] return [get_media_url(det.path) for det in detections] diff --git a/ami/main/tests.py b/ami/main/tests.py index 590a20734..db1e5d30e 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -2952,11 +2952,9 @@ def _list_occurrences(self, limit: int) -> int: from django.test.utils import CaptureQueriesContext url = f"/api/v2/occurrences/?project_id={self.project.pk}&limit={limit}" - # Clear cachalot cache between runs so query counts reflect cold state - try: - caches["default"].clear() - except Exception: - pass + # Clear cachalot cache between runs so query counts reflect cold state. + # Failures must propagate — a warm cache hides query-scaling regressions. + caches["default"].clear() with CaptureQueriesContext(connection) as ctx: res = self.client.get(url) From 80d44040427884edf8c6b0db1d6409ad87a1a04c Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 29 Apr 2026 13:49:18 -0700 Subject: [PATCH 03/13] refactor(occurrences): centralize BEST_IDENTIFICATION_ORDER constant Mirror BEST_MACHINE_PREDICTION_ORDER: define the identification ordering once and use it everywhere a "best identification" is selected. - New constant `BEST_IDENTIFICATION_ORDER = ("-created_at", "-pk")` alongside `BEST_MACHINE_PREDICTION_ORDER`. - `Occurrence.best_identification` and `OccurrenceQuerySet.with_verification_info` now order by the constant. Adds a `-pk` deterministic tiebreaker; created_at is auto_now_add so equal timestamps are vanishingly rare in practice. - `best_identification_from_prefetch` references the constant in its docstring (the helper's tuple key already encodes the same ordering). Co-Authored-By: Claude --- ami/main/models.py | 13 +++++++++++-- ami/main/models_future/occurrence.py | 6 +++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/ami/main/models.py b/ami/main/models.py index c285f2a19..6b320eb5c 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -60,6 +60,11 @@ # over non-terminal, then highest score, with pk as the deterministic tiebreaker. BEST_MACHINE_PREDICTION_ORDER: Final = ("-terminal", "-score", "-pk") +# Ordering for "best identification" selection used by Occurrence.best_identification, +# OccurrenceQuerySet.with_verification_info(), and best_identification_from_prefetch(). +# Most recent non-withdrawn identification wins, with pk as the deterministic tiebreaker. +BEST_IDENTIFICATION_ORDER: Final = ("-created_at", "-pk") + class TaxonRank(OrderedEnum): KINGDOM = "KINGDOM" @@ -3023,7 +3028,7 @@ def with_verification_info(self): """ best_identification_subquery = Identification.objects.filter( occurrence=OuterRef("pk"), withdrawn=False - ).order_by("-created_at") + ).order_by(*BEST_IDENTIFICATION_ORDER) return self.annotate( verified_by_name=models.Subquery(best_identification_subquery.values("user__name")[:1]), @@ -3249,7 +3254,11 @@ def best_identification(self): @TODO this could use a confidence level chosen manually by the users/experts. """ - return Identification.objects.filter(occurrence=self, withdrawn=False).order_by("-created_at").first() + return ( + Identification.objects.filter(occurrence=self, withdrawn=False) + .order_by(*BEST_IDENTIFICATION_ORDER) + .first() + ) def get_determination_score(self) -> float | None: if not self.determination: diff --git a/ami/main/models_future/occurrence.py b/ami/main/models_future/occurrence.py index b08fc5cb5..a6ea55a9a 100644 --- a/ami/main/models_future/occurrence.py +++ b/ami/main/models_future/occurrence.py @@ -112,9 +112,9 @@ def best_prediction_from_prefetch(occurrence: Occurrence) -> Classification | No def best_identification_from_prefetch(occurrence: Occurrence) -> Identification | None: """Pick the most recent non-withdrawn identification from prefetched data. - Mirrors `Occurrence.best_identification` (`order_by("-created_at")`), with - a `pk` tiebreaker so equal timestamps produce a deterministic winner. - `created_at=None` is treated as lower than any real timestamp. + Mirrors `Occurrence.best_identification`, which uses + `BEST_IDENTIFICATION_ORDER = ("-created_at", "-pk")`. `created_at=None` is + treated as lower than any real timestamp. """ best: Identification | None = None best_key: tuple[bool, object, int] | None = None From 22995b3ae0b6ca4d4c9cd5d92c2a575d82b88b3f Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 29 Apr 2026 15:24:24 -0700 Subject: [PATCH 04/13] refactor(occurrences): require prefetch in serializer; drop fallback paths Per PR review: the cache-key-gated fallbacks in `OccurrenceListSerializer` exercised the same N+1 path the PR closed, and silently triggered on the detail endpoint via shared serializer inheritance. - Drop `_best_prediction`, `_best_identification`, `get_detection_images` fallbacks. Serializer trusts the prefetch contract. - Add `prefetch_detections_for_detail()` factory + `with_detail_prefetches()` queryset method. Detail viewset now applies it instead of an inline Prefetch. - Drop `has_prefetched_classifications()` helper (no longer needed). - Add `TestOccurrenceDetailQueryCount` regression guard. Detail endpoint measures 12 queries; list (warm test client) measures 6. Closes review threads on PR #1274 (mihow comments 3164296309/3164306519, CodeRabbit comment 3163836361). Co-Authored-By: Claude --- ami/main/api/serializers.py | 31 ++---------- ami/main/api/views.py | 6 +-- ami/main/models.py | 6 +++ ami/main/models_future/occurrence.py | 71 +++++++++++----------------- ami/main/tests.py | 43 +++++++++++++++++ 5 files changed, 82 insertions(+), 75 deletions(-) diff --git a/ami/main/api/serializers.py b/ami/main/api/serializers.py index 09e5ecab9..ef695eb7e 100644 --- a/ami/main/api/serializers.py +++ b/ami/main/api/serializers.py @@ -1366,42 +1366,19 @@ class Meta: ] def get_detection_images(self, obj: Occurrence) -> list[str]: - """Return media URLs for the occurrence's detection crops. - - Reads from the prefetched `detections` cache populated by - `prefetch_detections_for_list()`; falls back to the model method for - callers that did not apply the prefetch (e.g. signals/exports). - """ from ami.main.models_future.occurrence import detection_image_urls_from_prefetch - if "detections" in getattr(obj, "_prefetched_objects_cache", {}): - return detection_image_urls_from_prefetch(obj) - return list(obj.detection_images()) + return detection_image_urls_from_prefetch(obj) def _best_identification(self, obj: Occurrence) -> Identification | None: - """Pick the best identification, preferring prefetched data when available.""" from ami.main.models_future.occurrence import best_identification_from_prefetch - # `with_identifications()` populates the relation cache; if it ran we - # can pick the best in Python without a fresh DB hit. - if "identifications" in getattr(obj, "_prefetched_objects_cache", {}): - return best_identification_from_prefetch(obj) - return obj.best_identification + return best_identification_from_prefetch(obj) def _best_prediction(self, obj: Occurrence): - """Pick the best machine prediction, preferring prefetched data when available. - - The list path prefetches detections AND nested classifications via - `prefetch_detections_for_list()`. The detail path only prefetches - detections, so calling `best_prediction_from_prefetch()` there would - walk `det.classifications.all()` lazily and reintroduce an N+1. - Gate strictly on classifications also being prefetched. - """ - from ami.main.models_future.occurrence import best_prediction_from_prefetch, has_prefetched_classifications + from ami.main.models_future.occurrence import best_prediction_from_prefetch - if has_prefetched_classifications(obj): - return best_prediction_from_prefetch(obj) - return obj.best_prediction + return best_prediction_from_prefetch(obj) def get_determination_details(self, obj: Occurrence): context = self.context diff --git a/ami/main/api/views.py b/ami/main/api/views.py index 9869fe996..57654dde8 100644 --- a/ami/main/api/views.py +++ b/ami/main/api/views.py @@ -1230,11 +1230,7 @@ def get_queryset(self) -> QuerySet["Occurrence"]: if self.action == "list": qs = qs.with_list_prefetches() # type: ignore else: - qs = qs.prefetch_related( - Prefetch( - "detections", queryset=Detection.objects.order_by("-timestamp").select_related("source_image") - ) - ) + qs = qs.with_detail_prefetches() # type: ignore return qs diff --git a/ami/main/models.py b/ami/main/models.py index 6b320eb5c..0f73e9df1 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -2946,6 +2946,12 @@ def with_list_prefetches(self): return self.prefetch_related(*prefetches_for_list_serializer()) + def with_detail_prefetches(self): + """Add prefetches the detail serializer needs (detections + source_image + classifications).""" + from ami.main.models_future.occurrence import prefetches_for_detail_serializer + + return self.prefetch_related(*prefetches_for_detail_serializer()) + def with_best_detection(self): """ Annotate the queryset with fields from the best detection. diff --git a/ami/main/models_future/occurrence.py b/ami/main/models_future/occurrence.py index a6ea55a9a..8c8a6693f 100644 --- a/ami/main/models_future/occurrence.py +++ b/ami/main/models_future/occurrence.py @@ -1,8 +1,9 @@ """ -Reusable Prefetch factories for Occurrence list-view rendering. +Reusable Prefetch factories for Occurrence list/detail rendering. -Centralising these lets the queryset, serializer, and any future caller share -a single source of truth for what data the list view needs eagerly loaded. +The serializer trusts the prefetch contract — the viewset is the single place +that wires it up. Don't gate serializer methods on `_prefetched_objects_cache` +membership; require the prefetch. Tracking issue: https://github.com/RolnickLab/antenna/issues/1271 """ @@ -17,56 +18,40 @@ from ami.main.models import Classification, Identification, Occurrence -def prefetch_detections_for_list() -> Prefetch: - """Single detections prefetch covering image URL listing AND best-prediction selection. +def _detections_prefetch(*, ordering: tuple[str, ...], with_source_image: bool) -> Prefetch: + from ami.main.models import Classification, Detection - One pass loads detections (ordered for stable image lists) plus their - classifications with `taxon`/`algorithm` joined. The serializer derives - image URLs by filtering `path is not None` in Python and picks the best - machine prediction from the same cache. + qs = Detection.objects.prefetch_related( + Prefetch( + "classifications", + queryset=Classification.objects.select_related("taxon", "algorithm"), + ) + ).order_by(*ordering) + if with_source_image: + qs = qs.select_related("source_image") + return Prefetch("detections", queryset=qs) - Replaces the previous pair (a `to_attr` filtered list for image paths - plus a separate `detections__classifications` prefetch) which loaded the - detections relation twice. - """ - from ami.main.models import Classification, Detection - return Prefetch( - "detections", - queryset=( - Detection.objects.prefetch_related( - Prefetch( - "classifications", - queryset=Classification.objects.select_related("taxon", "algorithm"), - ) - ).order_by("frame_num", "timestamp") - ), - ) +def prefetch_detections_for_list() -> Prefetch: + """Detections + nested classifications, ordered for stable list image galleries.""" + return _detections_prefetch(ordering=("frame_num", "timestamp"), with_source_image=False) -def prefetches_for_list_serializer() -> list[Prefetch]: - """All prefetches `OccurrenceListSerializer` needs to render without N+1. +def prefetch_detections_for_detail() -> Prefetch: + """Detections + nested classifications + source_image, ordered most-recent-first. - Identifications are covered by `OccurrenceQuerySet.with_identifications()` - which is already applied in the list viewset; intentionally not duplicated - here. + Detail responses serialize each detection via `DetectionNestedSerializer`, + which dereferences `source_image` (as `capture`). """ - return [prefetch_detections_for_list()] + return _detections_prefetch(ordering=("-timestamp",), with_source_image=True) + +def prefetches_for_list_serializer() -> list[Prefetch]: + return [prefetch_detections_for_list()] -def has_prefetched_classifications(occurrence: Occurrence) -> bool: - """Return True iff `detections` AND each detection's `classifications` are prefetched. - The list path prefetches both via `prefetch_detections_for_list()`. The - detail path prefetches only `detections` — calling - `best_prediction_from_prefetch()` there would walk `det.classifications.all()` - and reintroduce an N+1 (one query per detection). - """ - cache = getattr(occurrence, "_prefetched_objects_cache", {}) - detections = cache.get("detections") - if detections is None: - return False - return all("classifications" in getattr(det, "_prefetched_objects_cache", {}) for det in detections) +def prefetches_for_detail_serializer() -> list[Prefetch]: + return [prefetch_detections_for_detail()] def best_prediction_from_prefetch(occurrence: Occurrence) -> Classification | None: diff --git a/ami/main/tests.py b/ami/main/tests.py index db1e5d30e..aa76a842f 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -2978,6 +2978,49 @@ def test_list_query_count_does_not_scale_with_page_size(self): ) +class TestOccurrenceDetailQueryCount(APITestCase): + """Guard against N+1 regressions on the detail endpoint. + + `OccurrenceSerializer` extends `OccurrenceListSerializer`, so any + cache-key gating in the shared base can silently regress the detail + path. This guard pins detail-endpoint query count. + """ + + def setUp(self): + self.project, self.deployment = setup_test_project() + create_taxa(self.project) + create_captures(deployment=self.deployment, num_nights=1, images_per_night=10) + + self.project.default_filters_score_threshold = 0.0 + self.project.save() + + self.user = User.objects.create_user(email="qcount-detail@insectai.org", is_staff=False, is_superuser=False) + self.client.force_authenticate(user=self.user) + + def _detail_query_count(self, occurrence_id: int) -> int: + from django.core.cache import caches + from django.test.utils import CaptureQueriesContext + + url = f"/api/v2/occurrences/{occurrence_id}/?project_id={self.project.pk}" + caches["default"].clear() + with CaptureQueriesContext(connection) as ctx: + res = self.client.get(url) + self.assertEqual(res.status_code, status.HTTP_200_OK) + return len(ctx.captured_queries) + + def test_detail_query_count_is_bounded(self): + """Detail endpoint query count must not scale with detections per occurrence.""" + create_occurrences(deployment=self.deployment, num=5, determination_score=0.9) + occurrence_id = Occurrence.objects.filter(project=self.project).values_list("pk", flat=True).first() + self.assertIsNotNone(occurrence_id) + + count = self._detail_query_count(occurrence_id) + # 7 list-path queries + a small allowance for detail-only (full detections, + # nested predictions, source_image select_related). Hard ceiling catches + # silent reintroduction of N+1 in the shared serializer base. + self.assertLess(count, 20, f"Detail endpoint took {count} queries (likely N+1 regression)") + + class TestProjectDefaultTaxaFilter(APITestCase): """ Tests for project default taxa filtering (include/exclude lists). From 454ec77b5e4c2056ac5eb46fdce9e4decdd93247 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 29 Apr 2026 16:13:12 -0700 Subject: [PATCH 05/13] refactor(occurrences): strict prefetch helpers + bounded list detection_images MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per PR review (mihow): adopt the 'disable model methods' safety pattern as strict prefetch contract. Helpers now raise RuntimeError when required relations are missing from `_prefetched_objects_cache` instead of silently slow-pathing through Django's lazy lookups. - `_require_prefetch()` gate on best_prediction / best_identification / detection_image_urls helpers. - Restore old behavior: list responses cap detection_images at 3 (`OccurrenceListSerializer.detection_images_limit`). Detail subclass overrides to None (unbounded) — TODO: bound when occurrence tracking enables thousands of detections per occurrence. - New tests: - TestOccurrenceResponseShape pins list + detail JSON keys + the list cap. - TestOccurrencePrefetchHelpersEdgeCases asserts safe defaults on empty prefetched relations, and RuntimeError when prefetch is missing. Co-Authored-By: Claude --- ami/main/api/serializers.py | 11 ++- ami/main/models_future/occurrence.py | 36 ++++++-- ami/main/tests.py | 124 +++++++++++++++++++++++++++ 3 files changed, 164 insertions(+), 7 deletions(-) diff --git a/ami/main/api/serializers.py b/ami/main/api/serializers.py index ef695eb7e..cfce78e05 100644 --- a/ami/main/api/serializers.py +++ b/ami/main/api/serializers.py @@ -1314,6 +1314,12 @@ class Meta: class OccurrenceListSerializer(DefaultSerializer): + # Cap detection_images on list responses (cover gallery; usually 1, kept at 3 + # for backward compatibility). Detail subclass overrides to None (unbounded). + # TODO: bound detail too once occurrence tracking lands — counts can reach + # thousands per occurrence and pagination is the right answer. + detection_images_limit: int | None = 3 + determination = CaptureTaxonSerializer(read_only=True) deployment = DeploymentNestedSerializer(read_only=True) event = EventNestedSerializer(read_only=True) @@ -1368,7 +1374,7 @@ class Meta: def get_detection_images(self, obj: Occurrence) -> list[str]: from ami.main.models_future.occurrence import detection_image_urls_from_prefetch - return detection_image_urls_from_prefetch(obj) + return detection_image_urls_from_prefetch(obj, limit=self.detection_images_limit) def _best_identification(self, obj: Occurrence) -> Identification | None: from ami.main.models_future.occurrence import best_identification_from_prefetch @@ -1435,6 +1441,9 @@ def get_best_machine_prediction(self, obj: Occurrence) -> dict | None: class OccurrenceSerializer(OccurrenceListSerializer): + # Detail returns all detection_images (TODO: bound when tracking enabled). + detection_images_limit: int | None = None + determination = CaptureTaxonSerializer(read_only=True) detections = DetectionNestedSerializer(many=True, read_only=True) identifications = OccurrenceIdentificationSerializer(many=True, read_only=True) diff --git a/ami/main/models_future/occurrence.py b/ami/main/models_future/occurrence.py index 8c8a6693f..b280a01d0 100644 --- a/ami/main/models_future/occurrence.py +++ b/ami/main/models_future/occurrence.py @@ -54,18 +54,33 @@ def prefetches_for_detail_serializer() -> list[Prefetch]: return [prefetch_detections_for_detail()] +def _require_prefetch(occurrence: Occurrence, *relations: str) -> None: + """Raise if any required relation is missing from the prefetch cache. + + Strict contract: callers in serializer code must go through a viewset that + applied the corresponding prefetch. Silent slow-pathing (the serializer + triggering its own DB queries) is the N+1 we are fixing. + """ + cache = getattr(occurrence, "_prefetched_objects_cache", {}) + missing = [r for r in relations if r not in cache] + if missing: + raise RuntimeError( + f"Occurrence {occurrence.pk} is missing prefetched relations {missing!r}. " + "Apply OccurrenceQuerySet.with_list_prefetches() / with_detail_prefetches() / " + "with_identifications() in the viewset's get_queryset()." + ) + + def best_prediction_from_prefetch(occurrence: Occurrence) -> Classification | None: """Pick the best machine prediction from a prefetched occurrence in Python. Mirrors `Occurrence.best_prediction`, which calls `Occurrence.predictions()` (per-algorithm max-score filtering) and then orders by `-terminal, -score`. - Replicating that grouping in Python keeps list and detail responses - consistent for occurrences whose top-scoring classification is non-terminal. - Requires `prefetch_detections_for_list()` (or equivalent) to have been - applied; walks `obj.detections.all()` -> `det.classifications.all()` from - the prefetch cache. + Strict: requires `detections` (and each detection's `classifications`) to be + prefetched. Raises if not. """ + _require_prefetch(occurrence, "detections") classifications = [c for det in occurrence.detections.all() for c in det.classifications.all()] if not classifications: return None @@ -100,7 +115,11 @@ def best_identification_from_prefetch(occurrence: Occurrence) -> Identification Mirrors `Occurrence.best_identification`, which uses `BEST_IDENTIFICATION_ORDER = ("-created_at", "-pk")`. `created_at=None` is treated as lower than any real timestamp. + + Strict: requires `identifications` to be prefetched (via + `OccurrenceQuerySet.with_identifications()`). """ + _require_prefetch(occurrence, "identifications") best: Identification | None = None best_key: tuple[bool, object, int] | None = None for ident in occurrence.identifications.all(): @@ -116,8 +135,13 @@ def best_identification_from_prefetch(occurrence: Occurrence) -> Identification def detection_image_urls_from_prefetch(occurrence: Occurrence, limit: int | None = None) -> list[str]: """Return media URLs for the prefetched detections (filtering out `path=None`). - Requires `prefetch_detections_for_list()` to have been applied. + Strict: requires `detections` to be prefetched. Pass `limit` to bound output — + list responses cap at a few cover images; detail responses currently return + all (TODO: bound when occurrence tracking lands; per-occurrence detection + counts can reach thousands). """ + _require_prefetch(occurrence, "detections") + from ami.main.models import get_media_url detections = [det for det in occurrence.detections.all() if det.path] diff --git a/ami/main/tests.py b/ami/main/tests.py index aa76a842f..107da175e 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -3021,6 +3021,130 @@ def test_detail_query_count_is_bounded(self): self.assertLess(count, 20, f"Detail endpoint took {count} queries (likely N+1 regression)") +class TestOccurrenceResponseShape(APITestCase): + """Pin list + detail response shapes so the prefetch refactor doesn't silently change JSON.""" + + def setUp(self): + self.project, self.deployment = setup_test_project() + create_taxa(self.project) + create_captures(deployment=self.deployment, num_nights=1, images_per_night=10) + create_occurrences(deployment=self.deployment, num=10, determination_score=0.9) + self.project.default_filters_score_threshold = 0.0 + self.project.save() + self.user = User.objects.create_user(email="shape@insectai.org") + self.client.force_authenticate(user=self.user) + + def test_list_response_shape(self): + res = self.client.get(f"/api/v2/occurrences/?project_id={self.project.pk}&limit=10") + self.assertEqual(res.status_code, status.HTTP_200_OK) + self.assertGreater(len(res.data["results"]), 0) + row = res.data["results"][0] + for key in ( + "id", + "details", + "event", + "deployment", + "first_appearance_timestamp", + "duration", + "determination", + "detections_count", + "detection_images", + "determination_score", + "determination_details", + "best_machine_prediction", + "identifications", + "created_at", + "updated_at", + ): + self.assertIn(key, row, f"list response missing key {key!r}") + self.assertIsInstance(row["detection_images"], list) + # List path caps at 3 (matches old behavior). + self.assertLessEqual(len(row["detection_images"]), 3) + + def test_detail_response_shape(self): + occurrence_id = Occurrence.objects.filter(project=self.project).values_list("pk", flat=True).first() + res = self.client.get(f"/api/v2/occurrences/{occurrence_id}/?project_id={self.project.pk}") + self.assertEqual(res.status_code, status.HTTP_200_OK) + for key in ( + "id", + "determination", + "detection_images", + "determination_details", + "best_machine_prediction", + "detections", + "predictions", + ): + self.assertIn(key, res.data, f"detail response missing key {key!r}") + self.assertIsInstance(res.data["detection_images"], list) + self.assertIsInstance(res.data["detections"], list) + + +class TestOccurrencePrefetchHelpersEdgeCases(APITestCase): + """Helpers in `models_future/occurrence.py` must handle empty prefetch caches gracefully. + + `.valid()` excludes zero-detection occurrences from the list endpoint, but + helpers can still be called on objects whose detections were deleted, or in + non-API contexts (admin, exports). Empty must return None / [] without 500. + """ + + def setUp(self): + self.project, self.deployment = setup_test_project() + create_taxa(self.project) + create_captures(deployment=self.deployment, num_nights=1, images_per_night=2) + create_occurrences(deployment=self.deployment, num=2, determination_score=0.9) + + def _empty_occurrence(self) -> "Occurrence": + """An occurrence with prefetch caches populated but all relations empty.""" + from ami.main.models_future.occurrence import prefetches_for_list_serializer + + occurrence = ( + Occurrence.objects.filter(project=self.project) + .prefetch_related(*prefetches_for_list_serializer(), "identifications") + .first() + ) + assert occurrence is not None + # Wipe relations through DB to leave the prefetch cache empty. + occurrence.detections.all().delete() + occurrence.identifications.all().delete() + # Re-fetch with prefetches so the cache reflects the empty state. + return ( + Occurrence.objects.filter(pk=occurrence.pk) + .prefetch_related(*prefetches_for_list_serializer(), "identifications") + .get() + ) + + def test_helpers_return_safe_defaults_on_empty(self): + from ami.main.models_future.occurrence import ( + best_identification_from_prefetch, + best_prediction_from_prefetch, + detection_image_urls_from_prefetch, + ) + + occurrence = self._empty_occurrence() + self.assertIsNone(best_prediction_from_prefetch(occurrence)) + self.assertIsNone(best_identification_from_prefetch(occurrence)) + self.assertEqual(detection_image_urls_from_prefetch(occurrence), []) + self.assertEqual(detection_image_urls_from_prefetch(occurrence, limit=3), []) + + def test_helpers_raise_when_prefetch_missing(self): + """Strict contract: missing prefetch must raise, not silently slow-path.""" + from ami.main.models_future.occurrence import ( + best_identification_from_prefetch, + best_prediction_from_prefetch, + detection_image_urls_from_prefetch, + ) + + # No prefetch applied + occurrence = Occurrence.objects.filter(project=self.project).first() + assert occurrence is not None + with self.assertRaises(RuntimeError): + best_prediction_from_prefetch(occurrence) + with self.assertRaises(RuntimeError): + best_identification_from_prefetch(occurrence) + with self.assertRaises(RuntimeError): + detection_image_urls_from_prefetch(occurrence) + + class TestProjectDefaultTaxaFilter(APITestCase): """ Tests for project default taxa filtering (include/exclude lists). From 38b04cb6c695f16063b4e808d49d5d31629fdb5f Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 29 Apr 2026 16:22:44 -0700 Subject: [PATCH 06/13] test(occurrences): trim test surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop TestOccurrenceResponseShape — most assertions are key-presence checks already enforced by serializer Meta. Fold the only valuable assertion (the detection_images per-row cap on list responses) into _list_occurrences in TestOccurrenceListQueryCount where the response data is already inspected. Co-Authored-By: Claude --- ami/main/tests.py | 61 +++-------------------------------------------- 1 file changed, 3 insertions(+), 58 deletions(-) diff --git a/ami/main/tests.py b/ami/main/tests.py index 107da175e..3519c9121 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -2960,6 +2960,9 @@ def _list_occurrences(self, limit: int) -> int: res = self.client.get(url) self.assertEqual(res.status_code, status.HTTP_200_OK) self.assertEqual(len(res.data["results"]), min(limit, res.data["count"])) + # List responses cap detection_images per row (prevents unbounded payload). + for row in res.data["results"]: + self.assertLessEqual(len(row["detection_images"]), 3) return len(ctx.captured_queries) def test_list_query_count_does_not_scale_with_page_size(self): @@ -3021,64 +3024,6 @@ def test_detail_query_count_is_bounded(self): self.assertLess(count, 20, f"Detail endpoint took {count} queries (likely N+1 regression)") -class TestOccurrenceResponseShape(APITestCase): - """Pin list + detail response shapes so the prefetch refactor doesn't silently change JSON.""" - - def setUp(self): - self.project, self.deployment = setup_test_project() - create_taxa(self.project) - create_captures(deployment=self.deployment, num_nights=1, images_per_night=10) - create_occurrences(deployment=self.deployment, num=10, determination_score=0.9) - self.project.default_filters_score_threshold = 0.0 - self.project.save() - self.user = User.objects.create_user(email="shape@insectai.org") - self.client.force_authenticate(user=self.user) - - def test_list_response_shape(self): - res = self.client.get(f"/api/v2/occurrences/?project_id={self.project.pk}&limit=10") - self.assertEqual(res.status_code, status.HTTP_200_OK) - self.assertGreater(len(res.data["results"]), 0) - row = res.data["results"][0] - for key in ( - "id", - "details", - "event", - "deployment", - "first_appearance_timestamp", - "duration", - "determination", - "detections_count", - "detection_images", - "determination_score", - "determination_details", - "best_machine_prediction", - "identifications", - "created_at", - "updated_at", - ): - self.assertIn(key, row, f"list response missing key {key!r}") - self.assertIsInstance(row["detection_images"], list) - # List path caps at 3 (matches old behavior). - self.assertLessEqual(len(row["detection_images"]), 3) - - def test_detail_response_shape(self): - occurrence_id = Occurrence.objects.filter(project=self.project).values_list("pk", flat=True).first() - res = self.client.get(f"/api/v2/occurrences/{occurrence_id}/?project_id={self.project.pk}") - self.assertEqual(res.status_code, status.HTTP_200_OK) - for key in ( - "id", - "determination", - "detection_images", - "determination_details", - "best_machine_prediction", - "detections", - "predictions", - ): - self.assertIn(key, res.data, f"detail response missing key {key!r}") - self.assertIsInstance(res.data["detection_images"], list) - self.assertIsInstance(res.data["detections"], list) - - class TestOccurrencePrefetchHelpersEdgeCases(APITestCase): """Helpers in `models_future/occurrence.py` must handle empty prefetch caches gracefully. From 8f7db770efd62cecf7172e0ce4ce55370c9032f5 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 29 Apr 2026 17:06:45 -0700 Subject: [PATCH 07/13] fix(occurrences): apply detail prefetches to JSON exporter; cap list cover image to 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exports invoke OccurrenceExportSerializer (subclasses OccurrenceSerializer), whose inherited get_determination_details / _best_prediction call the strict prefetch helpers in models_future/occurrence.py. JSONExporter.get_queryset() did not apply the prefetch, so two CI tests in ami.exports.tests.DataExportTest raised RuntimeError ("Occurrence X is missing prefetched relations ['detections']"). Add .with_detail_prefetches() to the exporter queryset. Cap OccurrenceListSerializer.detection_images_limit from 3 to 1 — list cards render a single cover image; payload stays tight without changing response shape. Document the nested-prefetch caveat in _require_prefetch: it only checks top-level relations, so a caller prefetching 'detections' without nested 'classifications' silently re-introduces per-detection queries. Tighter enforcement is deferred to the django-zen-queries follow-up (issue #1271), which catches per-row queries at the view boundary regardless of depth. Co-Authored-By: Claude --- ami/exports/format_types.py | 5 +++++ ami/main/api/serializers.py | 6 +++--- ami/main/models_future/occurrence.py | 10 ++++++++++ ami/main/tests.py | 4 ++-- 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/ami/exports/format_types.py b/ami/exports/format_types.py index 9c57b5c25..a3f4c82d0 100644 --- a/ami/exports/format_types.py +++ b/ami/exports/format_types.py @@ -44,6 +44,10 @@ def get_serializer_class(self): return get_export_serializer() def get_queryset(self): + # OccurrenceSerializer (the export base class) calls strict prefetch + # helpers in `_best_prediction` / `_best_identification` / + # `get_detection_images`. Apply detail prefetches so those helpers + # find detections + nested classifications in the cache. return ( Occurrence.objects.valid() # type: ignore[union-attr] Custom manager method .filter(project=self.project) @@ -55,6 +59,7 @@ def get_queryset(self): .with_timestamps() # type: ignore[union-attr] Custom queryset method .with_detections_count() .with_identifications() + .with_detail_prefetches() # type: ignore[union-attr] Custom queryset method ) def export(self): diff --git a/ami/main/api/serializers.py b/ami/main/api/serializers.py index cfce78e05..ce4430834 100644 --- a/ami/main/api/serializers.py +++ b/ami/main/api/serializers.py @@ -1314,11 +1314,11 @@ class Meta: class OccurrenceListSerializer(DefaultSerializer): - # Cap detection_images on list responses (cover gallery; usually 1, kept at 3 - # for backward compatibility). Detail subclass overrides to None (unbounded). + # List responses render a single cover image per occurrence card; cap at 1 + # to keep payload tight. Detail subclass overrides to None (unbounded). # TODO: bound detail too once occurrence tracking lands — counts can reach # thousands per occurrence and pagination is the right answer. - detection_images_limit: int | None = 3 + detection_images_limit: int | None = 1 determination = CaptureTaxonSerializer(read_only=True) deployment = DeploymentNestedSerializer(read_only=True) diff --git a/ami/main/models_future/occurrence.py b/ami/main/models_future/occurrence.py index b280a01d0..038b8ad04 100644 --- a/ami/main/models_future/occurrence.py +++ b/ami/main/models_future/occurrence.py @@ -60,6 +60,16 @@ def _require_prefetch(occurrence: Occurrence, *relations: str) -> None: Strict contract: callers in serializer code must go through a viewset that applied the corresponding prefetch. Silent slow-pathing (the serializer triggering its own DB queries) is the N+1 we are fixing. + + Caveat: only top-level relations are checked. Nested prefetch (e.g. + `detections__classifications`) is still required but not enforced here — + if a caller prefetches `detections` without nested `classifications`, + `best_prediction_from_prefetch` will silently re-introduce per-detection + queries. The list/detail prefetch factories pair them correctly; new + callers should use those factories rather than building Prefetch objects + by hand. Tighter enforcement is deferred to the django-zen-queries pass + (issue #1271 follow-up), which catches all per-row queries at the view + boundary regardless of depth. """ cache = getattr(occurrence, "_prefetched_objects_cache", {}) missing = [r for r in relations if r not in cache] diff --git a/ami/main/tests.py b/ami/main/tests.py index 3519c9121..1913d7a62 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -2960,9 +2960,9 @@ def _list_occurrences(self, limit: int) -> int: res = self.client.get(url) self.assertEqual(res.status_code, status.HTTP_200_OK) self.assertEqual(len(res.data["results"]), min(limit, res.data["count"])) - # List responses cap detection_images per row (prevents unbounded payload). + # List responses cap detection_images per row (1 cover image; prevents unbounded payload). for row in res.data["results"]: - self.assertLessEqual(len(row["detection_images"]), 3) + self.assertLessEqual(len(row["detection_images"]), 1) return len(ctx.captured_queries) def test_list_query_count_does_not_scale_with_page_size(self): From 1f5918a1ca436086f53bf84cf9adddfdea8231c4 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 29 Apr 2026 18:04:41 -0700 Subject: [PATCH 08/13] test(occurrence): cover multi-detection scaling + drop None scores best_prediction_from_prefetch: - Skip classifications with score=None (mirrors SQL NULL semantics: Max(NULL) = NULL, score IN (NULL) never matches). - Removes the asymmetry between -inf in max calc and 0.0 in sort key. - Document Python-vs-SQL semantic difference around per-algorithm max grouping (Python is the stricter interpretation). TestOccurrenceDetailQueryCount: - Add test_detail_query_count_does_not_scale_with_detections, inflating one occurrence to 9 detections x 4 classifications. The single-detection test alone could not catch a multi-detection N+1; this guards the actual property the test name asserts. Co-Authored-By: Claude --- ami/main/models_future/occurrence.py | 28 +++++++++++------ ami/main/tests.py | 46 ++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 10 deletions(-) diff --git a/ami/main/models_future/occurrence.py b/ami/main/models_future/occurrence.py index 038b8ad04..2a702c287 100644 --- a/ami/main/models_future/occurrence.py +++ b/ami/main/models_future/occurrence.py @@ -87,32 +87,40 @@ def best_prediction_from_prefetch(occurrence: Occurrence) -> Classification | No Mirrors `Occurrence.best_prediction`, which calls `Occurrence.predictions()` (per-algorithm max-score filtering) and then orders by `-terminal, -score`. + Null-score handling: classifications with `score=None` are skipped, which + matches the SQL behavior of `score__in=Subquery(...)` — `Max(score)` over + only-NULL rows returns NULL, and `score IN (NULL)` never matches in SQL. + + Subtle SQL/Python difference (intentional): `Occurrence.predictions()` uses + `score__in=Subquery(per-algo maxes)`. If algo_A's max is 0.9 and algo_B has + a non-max classification at 0.9, the SQL form includes both; this Python + form keeps only per-algorithm maxima (algo_A's 0.9, not algo_B's). The + Python form is the stricter interpretation of the docstring intent + ("max score for each algorithm"). + Strict: requires `detections` (and each detection's `classifications`) to be prefetched. Raises if not. """ _require_prefetch(occurrence, "detections") - classifications = [c for det in occurrence.detections.all() for c in det.classifications.all()] + classifications = [ + c for det in occurrence.detections.all() for c in det.classifications.all() if c.score is not None + ] if not classifications: return None max_score_per_algo: dict[object, float] = {} for c in classifications: - score = c.score if c.score is not None else float("-inf") existing = max_score_per_algo.get(c.algorithm_id) - if existing is None or score > existing: - max_score_per_algo[c.algorithm_id] = score + if existing is None or c.score > existing: + max_score_per_algo[c.algorithm_id] = c.score - candidates = [ - c - for c in classifications - if (c.score if c.score is not None else float("-inf")) == max_score_per_algo[c.algorithm_id] - ] + candidates = [c for c in classifications if c.score == max_score_per_algo[c.algorithm_id]] if not candidates: return None candidates.sort( key=lambda c: ( 0 if getattr(c, "terminal", False) else 1, - -(c.score or 0.0), + -c.score, -c.pk, ) ) diff --git a/ami/main/tests.py b/ami/main/tests.py index 1913d7a62..90c5e66b5 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -3023,6 +3023,52 @@ def test_detail_query_count_is_bounded(self): # silent reintroduction of N+1 in the shared serializer base. self.assertLess(count, 20, f"Detail endpoint took {count} queries (likely N+1 regression)") + def test_detail_query_count_does_not_scale_with_detections(self): + """Detail with many detections per occurrence must not multiply queries. + + `create_occurrences` produces 1 detection/classification per occurrence. + Inflate one occurrence to N detections × N classifications and confirm + query count stays in the same envelope as the single-detection case + (i.e. bounded by the ceiling above, not detection_count). + """ + import datetime + + from ami.main.models import Classification, Detection + + create_occurrences(deployment=self.deployment, num=3, determination_score=0.9) + occurrence = Occurrence.objects.filter(project=self.project).first() + self.assertIsNotNone(occurrence) + + seed_detection = occurrence.detections.first() + self.assertIsNotNone(seed_detection) + seed_classification = seed_detection.classifications.first() + self.assertIsNotNone(seed_classification) + source_image = seed_detection.source_image + + for i in range(8): + det = Detection.objects.create( + source_image=source_image, + occurrence=occurrence, + timestamp=source_image.timestamp, + bbox=[0.1, 0.1, 0.2, 0.2], + path=f"detections/inflated_{i}.jpg", + ) + for j in range(3): + Classification.objects.create( + detection=det, + taxon=seed_classification.taxon, + algorithm=seed_classification.algorithm, + score=0.8 + (j * 0.01), + timestamp=datetime.datetime.now(), + ) + + count = self._detail_query_count(occurrence.pk) + self.assertLess( + count, + 20, + f"Detail with 9 detections × 4 classifications took {count} queries — N+1 regression", + ) + class TestOccurrencePrefetchHelpersEdgeCases(APITestCase): """Helpers in `models_future/occurrence.py` must handle empty prefetch caches gracefully. From cbae3fcac59e9507bd99f6ee203fe3f8b91f06ce Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Wed, 29 Apr 2026 19:15:17 -0700 Subject: [PATCH 09/13] chore(scripts): add reproducible benchmark for occurrences list endpoint Reusable curl + xargs runner that probes the project's occurrence count, then issues N parallel requests at varied offsets to bypass cachalot. Reports wall, avg, min, p50, p95, p99, max, and per-status error counts. Used to A/B compare staging deployments (arctia vs serbia) and to characterize prod baseline for PR #1274. Re-run after deploy to verify the speedup against prod's pre-merge numbers in the PR thread. Co-Authored-By: Claude --- scripts/benchmark_occurrences_list.sh | 66 +++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100755 scripts/benchmark_occurrences_list.sh diff --git a/scripts/benchmark_occurrences_list.sh b/scripts/benchmark_occurrences_list.sh new file mode 100755 index 000000000..7903eb576 --- /dev/null +++ b/scripts/benchmark_occurrences_list.sh @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +# Benchmark the /api/v2/occurrences/ list endpoint under concurrent load. +# +# Used to A/B compare a deployment with PR #1274 (occurrence list N+1 fix) +# against a baseline. Each request uses a randomized offset to bypass +# django-cachalot. Reports avg / p50 / p95 / p99 / max plus error counts. +# +# Usage: +# ./scripts/benchmark_occurrences_list.sh +# +# Example (prod): +# ./scripts/benchmark_occurrences_list.sh https://api.antenna.insectai.org 18 10 30 25 +# +# Example (staging A/B): +# ./scripts/benchmark_occurrences_list.sh https://arctia.dev.antenna.insectai.org 5 10 30 25 +# ./scripts/benchmark_occurrences_list.sh https://serbia.dev.antenna.insectai.org 5 10 30 25 + +set -euo pipefail + +BASE_URL="${1:?base_url required, e.g. https://api.antenna.insectai.org}" +PROJECT_ID="${2:?project_id required}" +CONCURRENCY="${3:-10}" +TOTAL="${4:-30}" +LIMIT="${5:-25}" + +# Probe project size so the random offset stays within the dataset +COUNT=$(curl -s "${BASE_URL}/api/v2/occurrences/?project_id=${PROJECT_ID}&limit=1" | jq -r '.count') +if [[ -z "${COUNT}" || "${COUNT}" == "null" ]]; then + echo "Failed to probe occurrence count at ${BASE_URL} project ${PROJECT_ID}" >&2 + exit 1 +fi +MAX_OFFSET=$(( COUNT > LIMIT ? COUNT - LIMIT : 0 )) + +OUT=$(mktemp) +trap 'rm -f "${OUT}"' EXIT + +echo "Benchmark: ${BASE_URL} project=${PROJECT_ID} count=${COUNT} concurrency=${CONCURRENCY} total=${TOTAL} limit=${LIMIT}" + +START=$(date +%s.%N) +seq 1 "${TOTAL}" | xargs -n1 -P"${CONCURRENCY}" -I{} bash -c " + offset=\$(( RANDOM % ${MAX_OFFSET} )) + curl -s -o /dev/null -w '%{http_code} %{time_total}\n' \ + '${BASE_URL}/api/v2/occurrences/?project_id=${PROJECT_ID}&limit=${LIMIT}&offset='\${offset} +" > "${OUT}" +END=$(date +%s.%N) +WALL=$(awk "BEGIN {printf \"%.3f\", ${END} - ${START}}") + +ERRS=$(awk '$1!=200{print}' "${OUT}" | wc -l | tr -d ' ') +ERR_CODES=$(awk '$1!=200{print $1}' "${OUT}" | sort | uniq -c | tr '\n' ' ') + +OK_TIMES=$(awk '$1==200{print $2}' "${OUT}" | sort -n) +OK_COUNT=$(echo "${OK_TIMES}" | grep -c . || echo 0) + +if [[ "${OK_COUNT}" -eq 0 ]]; then + echo "wall=${WALL}s errors=${ERRS}/${TOTAL} (${ERR_CODES})" + exit 1 +fi + +P50=$(echo "${OK_TIMES}" | awk -v n="${OK_COUNT}" 'NR==int(n*0.5)+1 {printf "%.3f", $1}') +P95=$(echo "${OK_TIMES}" | awk -v n="${OK_COUNT}" 'NR==int(n*0.95) {printf "%.3f", $1}') +P99=$(echo "${OK_TIMES}" | awk -v n="${OK_COUNT}" 'NR==int(n*0.99) {printf "%.3f", $1}') +AVG=$(echo "${OK_TIMES}" | awk '{s+=$1} END{printf "%.3f", s/NR}') +MIN=$(echo "${OK_TIMES}" | head -1 | awk '{printf "%.3f", $1}') +MAX=$(echo "${OK_TIMES}" | tail -1 | awk '{printf "%.3f", $1}') + +echo "wall=${WALL}s avg=${AVG}s min=${MIN}s p50=${P50}s p95=${P95}s p99=${P99}s max=${MAX}s errors=${ERRS}/${TOTAL} (${ERR_CODES:-none})" From 80f36cbdb73d71d291a2dc3cba96601f137b19b3 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Thu, 30 Apr 2026 17:08:45 -0700 Subject: [PATCH 10/13] refactor(occurrence): drop empty wrappers, trim prefetch helper docstrings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify the prefetch contract surface introduced in this branch: - Drop _best_prediction / _best_identification thin wrappers in OccurrenceListSerializer; inline best_*_from_prefetch calls. - Drop prefetches_for_list_serializer / prefetches_for_detail_serializer 1-line list wrappers; with_list_prefetches / with_detail_prefetches now call the Prefetch factories directly. - Compress _require_prefetch and best_*_from_prefetch docstrings; keep the strict-prefetch behavior and the SQL/Python null-handling note as a single line each. - OccurrenceSerializer.detection_images_limit = 100 (was None) — bound detail responses from the start; pagination is the long-term answer. Net -47 lines, no behavior change beyond the detail detection_images cap. Co-Authored-By: Claude --- ami/main/api/serializers.py | 28 ++++--------- ami/main/models.py | 13 +++--- ami/main/models_future/occurrence.py | 61 ++++++---------------------- ami/main/tests.py | 6 +-- 4 files changed, 29 insertions(+), 79 deletions(-) diff --git a/ami/main/api/serializers.py b/ami/main/api/serializers.py index ce4430834..bbcba23e7 100644 --- a/ami/main/api/serializers.py +++ b/ami/main/api/serializers.py @@ -1314,10 +1314,7 @@ class Meta: class OccurrenceListSerializer(DefaultSerializer): - # List responses render a single cover image per occurrence card; cap at 1 - # to keep payload tight. Detail subclass overrides to None (unbounded). - # TODO: bound detail too once occurrence tracking lands — counts can reach - # thousands per occurrence and pagination is the right answer. + # List cards render one cover image; detail subclass raises this to 100. detection_images_limit: int | None = 1 determination = CaptureTaxonSerializer(read_only=True) @@ -1376,17 +1373,9 @@ def get_detection_images(self, obj: Occurrence) -> list[str]: return detection_image_urls_from_prefetch(obj, limit=self.detection_images_limit) - def _best_identification(self, obj: Occurrence) -> Identification | None: - from ami.main.models_future.occurrence import best_identification_from_prefetch - - return best_identification_from_prefetch(obj) - - def _best_prediction(self, obj: Occurrence): - from ami.main.models_future.occurrence import best_prediction_from_prefetch - - return best_prediction_from_prefetch(obj) - def get_determination_details(self, obj: Occurrence): + from ami.main.models_future.occurrence import best_identification_from_prefetch, best_prediction_from_prefetch + context = self.context # Add this occurrence to the context so that the nested serializers can access it # the `parent` attribute is not available since we are manually instantiating the serializers @@ -1394,7 +1383,7 @@ def get_determination_details(self, obj: Occurrence): taxon = TaxonNestedSerializer(obj.determination, context=context).data if obj.determination else None - best_ident = self._best_identification(obj) + best_ident = best_identification_from_prefetch(obj) if best_ident: identification = OccurrenceIdentificationSerializer(best_ident, context=context).data else: @@ -1403,7 +1392,7 @@ def get_determination_details(self, obj: Occurrence): if identification: prediction = None else: - best_pred = self._best_prediction(obj) + best_pred = best_prediction_from_prefetch(obj) prediction = ClassificationNestedSerializer(best_pred, context=context).data if best_pred else None return dict( @@ -1419,10 +1408,12 @@ def get_best_machine_prediction(self, obj: Occurrence) -> dict | None: Populated regardless of human verification status, so clients can always show the ML result alongside a human-set determination. """ + from ami.main.models_future.occurrence import best_prediction_from_prefetch + context = self.context context["occurrence"] = obj - prediction = self._best_prediction(obj) + prediction = best_prediction_from_prefetch(obj) if not prediction: return None @@ -1441,8 +1432,7 @@ def get_best_machine_prediction(self, obj: Occurrence) -> dict | None: class OccurrenceSerializer(OccurrenceListSerializer): - # Detail returns all detection_images (TODO: bound when tracking enabled). - detection_images_limit: int | None = None + detection_images_limit: int | None = 100 determination = CaptureTaxonSerializer(read_only=True) detections = DetectionNestedSerializer(many=True, read_only=True) diff --git a/ami/main/models.py b/ami/main/models.py index 0f73e9df1..b30b4e645 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -2938,19 +2938,16 @@ def with_identifications(self): ) def with_list_prefetches(self): - """Add prefetches the list serializer needs (detection paths, classifications). + """Add prefetches the list serializer needs (detection paths, classifications).""" + from ami.main.models_future.occurrence import prefetch_detections_for_list - See `ami.main.models_future.occurrence` for the prefetch definitions. - """ - from ami.main.models_future.occurrence import prefetches_for_list_serializer - - return self.prefetch_related(*prefetches_for_list_serializer()) + return self.prefetch_related(prefetch_detections_for_list()) def with_detail_prefetches(self): """Add prefetches the detail serializer needs (detections + source_image + classifications).""" - from ami.main.models_future.occurrence import prefetches_for_detail_serializer + from ami.main.models_future.occurrence import prefetch_detections_for_detail - return self.prefetch_related(*prefetches_for_detail_serializer()) + return self.prefetch_related(prefetch_detections_for_detail()) def with_best_detection(self): """ diff --git a/ami/main/models_future/occurrence.py b/ami/main/models_future/occurrence.py index 2a702c287..4592ec8c9 100644 --- a/ami/main/models_future/occurrence.py +++ b/ami/main/models_future/occurrence.py @@ -46,30 +46,13 @@ def prefetch_detections_for_detail() -> Prefetch: return _detections_prefetch(ordering=("-timestamp",), with_source_image=True) -def prefetches_for_list_serializer() -> list[Prefetch]: - return [prefetch_detections_for_list()] - - -def prefetches_for_detail_serializer() -> list[Prefetch]: - return [prefetch_detections_for_detail()] - - def _require_prefetch(occurrence: Occurrence, *relations: str) -> None: - """Raise if any required relation is missing from the prefetch cache. - - Strict contract: callers in serializer code must go through a viewset that - applied the corresponding prefetch. Silent slow-pathing (the serializer - triggering its own DB queries) is the N+1 we are fixing. - - Caveat: only top-level relations are checked. Nested prefetch (e.g. - `detections__classifications`) is still required but not enforced here — - if a caller prefetches `detections` without nested `classifications`, - `best_prediction_from_prefetch` will silently re-introduce per-detection - queries. The list/detail prefetch factories pair them correctly; new - callers should use those factories rather than building Prefetch objects - by hand. Tighter enforcement is deferred to the django-zen-queries pass - (issue #1271 follow-up), which catches all per-row queries at the view - boundary regardless of depth. + """Raise if any required top-level relation is missing from the prefetch cache. + + Only checks top-level relations; nested prefetch (e.g. `detections__classifications`) + is required by callers but not enforced here. Use the list/detail factories above + to keep the pairing correct. Tighter depth-checking is deferred to django-zen-queries + (#1271 follow-up). """ cache = getattr(occurrence, "_prefetched_objects_cache", {}) missing = [r for r in relations if r not in cache] @@ -84,22 +67,10 @@ def _require_prefetch(occurrence: Occurrence, *relations: str) -> None: def best_prediction_from_prefetch(occurrence: Occurrence) -> Classification | None: """Pick the best machine prediction from a prefetched occurrence in Python. - Mirrors `Occurrence.best_prediction`, which calls `Occurrence.predictions()` - (per-algorithm max-score filtering) and then orders by `-terminal, -score`. - - Null-score handling: classifications with `score=None` are skipped, which - matches the SQL behavior of `score__in=Subquery(...)` — `Max(score)` over - only-NULL rows returns NULL, and `score IN (NULL)` never matches in SQL. + Mirrors `Occurrence.best_prediction` (per-algorithm max-score, then `-terminal, -score`). + Skips `score=None` to match SQL semantics of `score__in=Subquery(...)`. - Subtle SQL/Python difference (intentional): `Occurrence.predictions()` uses - `score__in=Subquery(per-algo maxes)`. If algo_A's max is 0.9 and algo_B has - a non-max classification at 0.9, the SQL form includes both; this Python - form keeps only per-algorithm maxima (algo_A's 0.9, not algo_B's). The - Python form is the stricter interpretation of the docstring intent - ("max score for each algorithm"). - - Strict: requires `detections` (and each detection's `classifications`) to be - prefetched. Raises if not. + Strict: requires `detections` (and each detection's `classifications`) prefetched. """ _require_prefetch(occurrence, "detections") classifications = [ @@ -115,8 +86,6 @@ def best_prediction_from_prefetch(occurrence: Occurrence) -> Classification | No max_score_per_algo[c.algorithm_id] = c.score candidates = [c for c in classifications if c.score == max_score_per_algo[c.algorithm_id]] - if not candidates: - return None candidates.sort( key=lambda c: ( 0 if getattr(c, "terminal", False) else 1, @@ -130,12 +99,9 @@ def best_prediction_from_prefetch(occurrence: Occurrence) -> Classification | No def best_identification_from_prefetch(occurrence: Occurrence) -> Identification | None: """Pick the most recent non-withdrawn identification from prefetched data. - Mirrors `Occurrence.best_identification`, which uses - `BEST_IDENTIFICATION_ORDER = ("-created_at", "-pk")`. `created_at=None` is - treated as lower than any real timestamp. + Mirrors `Occurrence.best_identification` (BEST_IDENTIFICATION_ORDER = -created_at, -pk). - Strict: requires `identifications` to be prefetched (via - `OccurrenceQuerySet.with_identifications()`). + Strict: requires `identifications` prefetched (via `OccurrenceQuerySet.with_identifications()`). """ _require_prefetch(occurrence, "identifications") best: Identification | None = None @@ -153,10 +119,7 @@ def best_identification_from_prefetch(occurrence: Occurrence) -> Identification def detection_image_urls_from_prefetch(occurrence: Occurrence, limit: int | None = None) -> list[str]: """Return media URLs for the prefetched detections (filtering out `path=None`). - Strict: requires `detections` to be prefetched. Pass `limit` to bound output — - list responses cap at a few cover images; detail responses currently return - all (TODO: bound when occurrence tracking lands; per-occurrence detection - counts can reach thousands). + Strict: requires `detections` prefetched. Pass `limit` to bound output. """ _require_prefetch(occurrence, "detections") diff --git a/ami/main/tests.py b/ami/main/tests.py index 90c5e66b5..db5a8ee68 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -3086,11 +3086,11 @@ def setUp(self): def _empty_occurrence(self) -> "Occurrence": """An occurrence with prefetch caches populated but all relations empty.""" - from ami.main.models_future.occurrence import prefetches_for_list_serializer + from ami.main.models_future.occurrence import prefetch_detections_for_list occurrence = ( Occurrence.objects.filter(project=self.project) - .prefetch_related(*prefetches_for_list_serializer(), "identifications") + .prefetch_related(prefetch_detections_for_list(), "identifications") .first() ) assert occurrence is not None @@ -3100,7 +3100,7 @@ def _empty_occurrence(self) -> "Occurrence": # Re-fetch with prefetches so the cache reflects the empty state. return ( Occurrence.objects.filter(pk=occurrence.pk) - .prefetch_related(*prefetches_for_list_serializer(), "identifications") + .prefetch_related(prefetch_detections_for_list(), "identifications") .get() ) From 9b1c532cd71f66915cb38e58926c21871f637aee Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Mon, 11 May 2026 12:48:25 -0700 Subject: [PATCH 11/13] test: add N+1 regression guards for SourceImage and Taxon list endpoints Audit follow-up to PR #1274 (Occurrence list N+1 fix). Both viewsets already scale flat (4q at limit=25 with cachalot disabled and warm pool). Tests pin the current behaviour so future serializer changes can't silently reintroduce per-row queries. Co-Authored-By: Claude --- ami/main/tests.py | 111 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/ami/main/tests.py b/ami/main/tests.py index db5a8ee68..4eddb63f2 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -3136,6 +3136,117 @@ def test_helpers_raise_when_prefetch_missing(self): detection_image_urls_from_prefetch(occurrence) +@override_settings(CACHALOT_ENABLED=False) +class TestSourceImageListQueryCount(APITestCase): + """Audit SourceImageViewSet.list for N+1 (follow-up to PR #1274). + + Cachalot disabled so we measure cold query count, not warm cache. + """ + + def setUp(self): + self.project, self.deployment = setup_test_project() + create_taxa(self.project) + create_captures(deployment=self.deployment, num_nights=1, images_per_night=25) + create_occurrences(deployment=self.deployment, num=25, determination_score=0.9) + + self.project.default_filters_score_threshold = 0.0 + self.project.save() + + self.user = User.objects.create_user( + email="qcount-sourceimage@insectai.org", is_staff=False, is_superuser=False + ) + self.client.force_authenticate(user=self.user) + + def _list_query_count(self, limit: int, with_detections: bool) -> int: + from django.core.cache import caches + from django.test.utils import CaptureQueriesContext + + url = ( + f"/api/v2/captures/?project_id={self.project.pk}&limit={limit}" + f"&with_detections={'true' if with_detections else 'false'}" + ) + caches["default"].clear() + with CaptureQueriesContext(connection) as ctx: + res = self.client.get(url) + self.assertEqual(res.status_code, status.HTTP_200_OK) + return len(ctx.captured_queries) + + def test_list_query_count_does_not_scale_without_detections(self): + small = self._list_query_count(limit=5, with_detections=False) + large = self._list_query_count(limit=25, with_detections=False) + print(f"\n[AUDIT] SourceImage list (no detections): limit=5 -> {small}q, limit=25 -> {large}q") + self.assertLessEqual(large, small + 5, f"SourceImage list scaling: {small} -> {large} (likely N+1)") + + def test_list_query_count_does_not_scale_with_detections(self): + small = self._list_query_count(limit=5, with_detections=True) + large = self._list_query_count(limit=25, with_detections=True) + print(f"\n[AUDIT] SourceImage list (with_detections=true): limit=5 -> {small}q, limit=25 -> {large}q") + self.assertLessEqual( + large, small + 5, f"SourceImage list scaling with detections: {small} -> {large} (likely N+1)" + ) + + def test_list_query_with_counts_and_detections(self): + """Realistic UI call: with_counts=true & with_detections=true at limit=25.""" + from django.core.cache import caches + from django.test.utils import CaptureQueriesContext + + url_base = f"/api/v2/captures/?project_id={self.project.pk}&with_detections=true&with_counts=true" + # Two warmups: first request does session/auth bootstrap, second equalises pool state. + self.client.get(url_base + "&limit=1") + self.client.get(url_base + "&limit=1") + caches["default"].clear() + with CaptureQueriesContext(connection) as ctx_small: + self.client.get(url_base + "&limit=5") + caches["default"].clear() + with CaptureQueriesContext(connection) as ctx_large: + self.client.get(url_base + "&limit=25") + small, large = len(ctx_small.captured_queries), len(ctx_large.captured_queries) + print( + f"\n[AUDIT] SourceImage list (with_detections+with_counts): " f"limit=5 -> {small}q, limit=25 -> {large}q" + ) + self.assertLessEqual(large, small + 5, f"SourceImage list scaling: {small} -> {large} (likely N+1)") + + +@override_settings(CACHALOT_ENABLED=False) +class TestTaxonListQueryCount(APITestCase): + """Audit TaxonViewSet.list for N+1 (follow-up to PR #1274, task #9/#15). + + Cachalot disabled so we measure cold query count, not warm cache. + """ + + def setUp(self): + self.project, self.deployment = setup_test_project() + create_taxa(self.project) + create_captures(deployment=self.deployment, num_nights=1, images_per_night=25) + # Spread occurrences across many taxa so the taxon list has 25+ rows. + taxa = list(Taxon.objects.filter(projects=self.project)) + for taxon in taxa[:25]: + create_occurrences(deployment=self.deployment, num=2, taxon=taxon, determination_score=0.9) + + self.project.default_filters_score_threshold = 0.0 + self.project.save() + + self.user = User.objects.create_user(email="qcount-taxon@insectai.org", is_staff=False, is_superuser=False) + self.client.force_authenticate(user=self.user) + + def _list_query_count(self, limit: int) -> int: + from django.core.cache import caches + from django.test.utils import CaptureQueriesContext + + url = f"/api/v2/taxa/?project_id={self.project.pk}&limit={limit}" + caches["default"].clear() + with CaptureQueriesContext(connection) as ctx: + res = self.client.get(url) + self.assertEqual(res.status_code, status.HTTP_200_OK) + return len(ctx.captured_queries) + + def test_list_query_count_does_not_scale_with_page_size(self): + small = self._list_query_count(limit=5) + large = self._list_query_count(limit=25) + print(f"\n[AUDIT] Taxon list: limit=5 -> {small}q, limit=25 -> {large}q") + self.assertLessEqual(large, small + 5, f"Taxon list scaling: {small} -> {large} (likely N+1)") + + class TestProjectDefaultTaxaFilter(APITestCase): """ Tests for project default taxa filtering (include/exclude lists). From d41deb720b1a7b7f87ab0566b162830337f9f21f Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Mon, 11 May 2026 12:48:34 -0700 Subject: [PATCH 12/13] fix(api): make TaxonListSerializer.parents actually return parent chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Field was declared as TaxonNestedSerializer(read_only=True) with no source, but Taxon has no `parents` attribute (only `parents_json` JSONB cache and the `parent` FK). DRF resolved getattr(taxon, "parents") to nothing, so the field rendered null for every taxon in the list response. Match the detail serializer at TaxonSerializer.parents (line 865) by reading from parents_json with TaxonParentSerializer. UI in ui/src/data-services/models/taxa.ts already consumes this shape and falls back when missing — fix lets the list path populate ranks directly. Found during audit follow-up to PR #1274. Co-Authored-By: Claude --- ami/main/api/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ami/main/api/serializers.py b/ami/main/api/serializers.py index bbcba23e7..354df5459 100644 --- a/ami/main/api/serializers.py +++ b/ami/main/api/serializers.py @@ -591,7 +591,7 @@ def get_taxa(self, obj): class TaxonListSerializer(DefaultSerializer): # latest_detection = DetectionNestedSerializer(read_only=True) occurrences = serializers.SerializerMethodField() - parents = TaxonNestedSerializer(read_only=True) + parents = TaxonParentSerializer(many=True, read_only=True, source="parents_json") parent_id = serializers.PrimaryKeyRelatedField(queryset=Taxon.objects.all(), source="parent") tags = serializers.SerializerMethodField() From 054d331716bd8e622c2b00103879a352791ecabd Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Mon, 11 May 2026 13:47:33 -0700 Subject: [PATCH 13/13] chore: address CodeRabbit nits on test fixture and benchmark script - ami/main/tests.py: switch Detection.bbox in the inflated-detections fixture to pixel-space ints, matching the rest of the codebase's bbox conventions. - scripts/benchmark_occurrences_list.sh: guard against RANDOM % 0 when the project has fewer than LIMIT occurrences (MAX_OFFSET = 0). Co-Authored-By: Claude --- ami/main/tests.py | 2 +- scripts/benchmark_occurrences_list.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ami/main/tests.py b/ami/main/tests.py index 4eddb63f2..360384853 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -3050,7 +3050,7 @@ def test_detail_query_count_does_not_scale_with_detections(self): source_image=source_image, occurrence=occurrence, timestamp=source_image.timestamp, - bbox=[0.1, 0.1, 0.2, 0.2], + bbox=[10, 10, 20, 20], path=f"detections/inflated_{i}.jpg", ) for j in range(3): diff --git a/scripts/benchmark_occurrences_list.sh b/scripts/benchmark_occurrences_list.sh index 7903eb576..9373d9e53 100755 --- a/scripts/benchmark_occurrences_list.sh +++ b/scripts/benchmark_occurrences_list.sh @@ -38,7 +38,7 @@ echo "Benchmark: ${BASE_URL} project=${PROJECT_ID} count=${COUNT} concurrency=${ START=$(date +%s.%N) seq 1 "${TOTAL}" | xargs -n1 -P"${CONCURRENCY}" -I{} bash -c " - offset=\$(( RANDOM % ${MAX_OFFSET} )) + offset=\$(( ${MAX_OFFSET} > 0 ? RANDOM % ${MAX_OFFSET} : 0 )) curl -s -o /dev/null -w '%{http_code} %{time_total}\n' \ '${BASE_URL}/api/v2/occurrences/?project_id=${PROJECT_ID}&limit=${LIMIT}&offset='\${offset} " > "${OUT}"