diff --git a/.agents/AGENTS.md b/.agents/AGENTS.md index 1499fc174..fc240258b 100644 --- a/.agents/AGENTS.md +++ b/.agents/AGENTS.md @@ -19,6 +19,8 @@ Every call to the AI model API incurs a cost and requires electricity. Be smart - Focus on optimizing cold queries first before adding caching - When ordering by annotated fields, pagination COUNT queries include those annotations - use `.values('pk')` to strip them - For large tables (>10k rows), consider fuzzy counting using PostgreSQL's pg_class.reltuples +- **Run `EXPLAIN (ANALYZE)` before claiming anything about a query plan.** Do not describe a query as "uses the index", "stays off the table", or "only touches N rows" from reading the ORM expression — verify it. A filter that looks bounded can still trigger a `Seq Scan`: an anti-join such as `pipelines__isnull=True` hides the candidate ids at plan time, so Postgres scans the whole table despite an index being available. Materializing the ids first and filtering on `field__in=[literal ids]` is what lets the planner use the index. +- **Measure on the largest project in the local database, and survey several project sizes.** The local DB is a copy of production — use it. A small or empty project routinely hides the defect. If a query's cost does not move with project size, it is bound by total table size and will degrade as the platform grows regardless of tenant. Wrap timing loops in `cachalot_disabled()` and rebuild the queryset each iteration, or a reused queryset serves from `_result_cache` and reports a fake ~0 ms. **Git Commit Guidelines:** - Do NOT include "Generated with Claude Code" in commit messages diff --git a/ami/main/api/views.py b/ami/main/api/views.py index e245044a8..18670fdcf 100644 --- a/ami/main/api/views.py +++ b/ami/main/api/views.py @@ -35,6 +35,8 @@ from ami.main.api.schemas import limit_doc_param, project_id_doc_param from ami.main.api.serializers import TagSerializer from ami.main.models_future.occurrence import model_agreement_for_project, top_identifiers_for_project +from ami.ml.models.algorithm import Algorithm +from ami.ml.serializers import AlgorithmSerializer from ami.utils.requests import get_default_classification_threshold from ami.utils.storages import ConnectionTestResult @@ -1222,11 +1224,15 @@ def filter_queryset(self, request, queryset, view): class OccurrenceAlgorithmFilter(filters.BaseFilterBackend): """ - Filter occurrences by the detection algorithm that detected them. + Filter occurrences by any algorithm that produced a result on them. - Accepts a list of algorithm ids to filter by or exclude by. + Matches an occurrence when one of its detections was made by the given + algorithm (detectors) or one of its classifications came from it + (classifiers and post-processing algorithms), so every algorithm listed + by ``Algorithm.objects.used_in_project()`` can be filtered here. - This filter can be both inclusive and exclusive. + Accepts a list of algorithm ids to filter by (``algorithm``) or exclude + by (``not_algorithm``). Both are supported and may be combined. """ query_param = "algorithm" @@ -1237,9 +1243,9 @@ def filter_queryset(self, request, queryset, view): algorithm_ids_exclusive = request.query_params.getlist(self.query_param_exclusive) if algorithm_ids: - queryset = queryset.filter(detections__classifications__algorithm__in=algorithm_ids) + queryset = queryset.processed_by_algorithm(algorithm_ids) if algorithm_ids_exclusive: - queryset = queryset.exclude(detections__classifications__algorithm__in=algorithm_ids_exclusive) + queryset = queryset.not_processed_by_algorithm(algorithm_ids_exclusive) return queryset @@ -1467,6 +1473,33 @@ def get_queryset(self) -> QuerySet["Occurrence"]: def list(self, request, *args, **kwargs): return super().list(request, *args, **kwargs) + @extend_schema(parameters=[project_id_doc_param], responses=AlgorithmSerializer(many=True)) + @action(detail=False, methods=["get"], name="algorithms") + def algorithms(self, request: Request) -> Response: + """Choices for the occurrence algorithm filter: every algorithm that produced a result in the project. + + Includes superseded pipeline versions and standalone post-processing algorithms + (their output is still filterable), so it differs from the project's algorithm + list at ``/ml/algorithms/``, which shows what the project can run — its + enabled-pipeline algorithms. Serving choices from what actually ran means the + filter never offers a value with zero results. + """ + project = self.get_active_project() + if project is None: + raise api_exceptions.ValidationError({"project_id": "This parameter is required."}) + if not Project.objects.visible_for_user(request.user).filter(pk=project.pk).exists(): + raise NotFound("Project not found.") + + qs = ( + Algorithm.objects.all() + .with_category_count() # type: ignore[union-attr] # Custom queryset method + .used_in_project(project) + .order_by("name") + ) + page = self.paginate_queryset(qs) + serializer = AlgorithmSerializer(page, many=True, context=self.get_serializer_context()) + return self.get_paginated_response(serializer.data) + class OccurrenceStatsViewSet(viewsets.GenericViewSet, ProjectMixin): """Aggregate stats over Occurrences. Each @action == one stats kind. diff --git a/ami/main/models.py b/ami/main/models.py index 3662b4107..1b4ac3d4e 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -3353,6 +3353,33 @@ def valid(self): def with_detections_count(self): return self.annotate(detections_count=models.Count("detections", distinct=True)) + def _processed_by_algorithm_q(self, algorithm_ids) -> Exists: + """Subquery matching occurrences with any result from the given algorithms — + a detection made by one (detectors) or a classification from one (classifiers + and post-processing algorithms such as class masking or a size filter).""" + return Exists( + Detection.objects.filter(occurrence_id=OuterRef("pk")).filter( + models.Q(detection_algorithm__in=algorithm_ids) + | models.Q(classifications__algorithm__in=algorithm_ids) + ) + ) + + def processed_by_algorithm(self, algorithm_ids) -> "OccurrenceQuerySet": + """Occurrences with at least one result from the given algorithms. + + Matches detectors through Detection.detection_algorithm and classifiers or + post-processing algorithms through their classifications, so every algorithm + listed by ``Algorithm.objects.used_in_project()`` can match here. The EXISTS + form returns each occurrence once; a join through + ``detections__classifications`` returns one row per matching result, which + inflates pagination counts and duplicates rows across pages. + """ + return self.filter(self._processed_by_algorithm_q(algorithm_ids)) + + def not_processed_by_algorithm(self, algorithm_ids) -> "OccurrenceQuerySet": + """Occurrences with no result from any of the given algorithms.""" + return self.exclude(self._processed_by_algorithm_q(algorithm_ids)) + def with_timestamps(self): """ These are timestamps used for filtering and ordering in the UI. diff --git a/ami/main/tests.py b/ami/main/tests.py index a0d0253a1..1bc8568b8 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -6510,6 +6510,108 @@ def test_valid_returns_only_real_with_determination(self): self.assertNotIn(no_determination.pk, valid_pks) +class TestOccurrenceAlgorithmFilterQuerySet(TestCase): + """ + Covers OccurrenceQuerySet.processed_by_algorithm / not_processed_by_algorithm, + which back the ?algorithm= and ?not_algorithm= occurrence filters (PR #1368). + + The filter must match an algorithm by either role it can play: the detector that + made a detection, or the classifier / post-processing algorithm that authored a + classification. It must also count each occurrence once — the previous join form + (``detections__classifications__algorithm__in``) returned one row per matching + classification, inflating the paginator's COUNT and duplicating occurrences across + pages. + """ + + def setUp(self): + from ami.main.models import Taxon + from ami.ml.models.algorithm import Algorithm + + self.project = Project.objects.create(name="Occurrence Algo Filter 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="occ-algo-filter.jpg", + ) + self.taxon = Taxon.objects.create(name="Occurrence Algo Filter Taxon") + + self.detector = Algorithm.objects.create(name="Filter Detector", version=1, task_type="localization") + self.classifier = Algorithm.objects.create(name="Filter Classifier", version=1, task_type="classification") + self.other = Algorithm.objects.create(name="Filter Other", version=1, task_type="classification") + + # Detected by `detector`, classified once by `classifier`. + self.occ_classified = self._make_occurrence(detector=self.detector, classifiers=[self.classifier]) + # Same pair, but classified three times — the double-count trap. + self.occ_multi = self._make_occurrence( + detector=self.detector, classifiers=[self.classifier, self.classifier, self.classifier] + ) + # A different algorithm entirely, to prove filters are exclusive. + self.occ_other = self._make_occurrence(detector=self.other, classifiers=[self.other]) + + def _make_occurrence(self, detector, classifiers) -> Occurrence: + occ = Occurrence.objects.create( + project=self.project, + event=self.event, + deployment=self.deployment, + determination=self.taxon, + ) + detection = Detection.objects.create( + source_image=self.source_image, + bbox=[0.0, 0.0, 1.0, 1.0], + detection_algorithm=detector, + occurrence=occ, + ) + for classifier in classifiers: + detection.classifications.create( + taxon=self.taxon, + algorithm=classifier, + score=0.9, + timestamp=datetime.datetime.now(), + ) + return occ + + def _pks(self, queryset): + return set(queryset.values_list("pk", flat=True)) + + def test_filter_by_classifier_matches_its_occurrences(self): + matched = Occurrence.objects.filter(project=self.project).processed_by_algorithm([self.classifier.pk]) + self.assertEqual(self._pks(matched), {self.occ_classified.pk, self.occ_multi.pk}) + + def test_filter_by_detector_matches_occurrences_it_detected(self): + """A detector authors no Classification, so the old classification-join form + returned nothing for it. Matching through Detection.detection_algorithm is what + lets the user filter by a localizer at all.""" + matched = Occurrence.objects.filter(project=self.project).processed_by_algorithm([self.detector.pk]) + self.assertEqual(self._pks(matched), {self.occ_classified.pk, self.occ_multi.pk}) + + def test_count_not_inflated_by_multiple_classifications(self): + """The occurrence with three classifications by the same algorithm must count + once. The paginator calls this same COUNT, so an inflated value would report + four occurrences where there are two and repeat rows across pages.""" + matched = Occurrence.objects.filter(project=self.project).processed_by_algorithm([self.classifier.pk]) + self.assertEqual(matched.count(), 2) + + def test_exclude_removes_matching_occurrences(self): + remaining = Occurrence.objects.filter(project=self.project).not_processed_by_algorithm([self.other.pk]) + self.assertEqual(self._pks(remaining), {self.occ_classified.pk, self.occ_multi.pk}) + + def test_exclude_is_the_complement_of_include(self): + base = Occurrence.objects.filter(project=self.project) + ids = [self.classifier.pk] + included = self._pks(base.processed_by_algorithm(ids)) + excluded = self._pks(base.not_processed_by_algorithm(ids)) + self.assertEqual(included | excluded, self._pks(base)) + self.assertEqual(included & excluded, set()) + + class TestCleanupNullOnlyOccurrencesCommand(TestCase): """ Covers ami/main/management/commands/cleanup_null_only_occurrences.py. diff --git a/ami/ml/models/algorithm.py b/ami/ml/models/algorithm.py index 0e9df4609..605f90861 100644 --- a/ami/ml/models/algorithm.py +++ b/ami/ml/models/algorithm.py @@ -157,6 +157,46 @@ def with_category_count(self): """ return self.annotate(category_count=ArrayLength("category_map__labels")) + def used_in_project(self, project) -> AlgorithmQuerySet: + """Algorithms that produced results in the project, whether or not their + pipeline is still enabled there. + + "Used" means owning actual output rows: classifiers and post-processing + algorithms are found through their classifications, detectors through their + detections (detectors never author a Classification). Superseded pipeline + versions and standalone post-processing algorithms therefore stay listed as + long as their results exist, while a configured-but-never-run algorithm does + not appear. + + Cost note (EXPLAIN-verified against a production copy): neither lookup can be + answered from an index, because neither table has a project column — project is + reachable only through source_image. Both sides scan, so the call costs roughly + 0.1-0.6 s cold regardless of project size, growing with total table size. + Executes the two lookups immediately rather than lazily; the id lists are tiny + (one row per algorithm) and sorted so the SQL string, and therefore cachalot's + cache key, is stable. Making this index-fast requires a denormalised project + column on Classification/Detection. + """ + from ami.main.models import Classification, Detection + + # ``order_by()`` clears each model's default ordering before ``distinct()``; + # otherwise the ordering columns widen the DISTINCT back to one row per result. + classifier_ids = ( + Classification.objects.filter(detection__source_image__project=project) + .order_by() + .values_list("algorithm_id", flat=True) + .distinct() + ) + detector_ids = ( + Detection.objects.filter(source_image__project=project) + .order_by() + .values_list("detection_algorithm", flat=True) + .distinct() + ) + ids = set(classifier_ids) | set(detector_ids) + ids.discard(None) # detections without a detection_algorithm + return self.filter(pk__in=sorted(ids)) + # Task types enum for better type checking class AlgorithmTaskType(str, enum.Enum): diff --git a/ami/ml/post_processing/class_masking.py b/ami/ml/post_processing/class_masking.py index fc72b836a..2da2001b7 100644 --- a/ami/ml/post_processing/class_masking.py +++ b/ami/ml/post_processing/class_masking.py @@ -272,7 +272,7 @@ def _get_or_create_masking_algorithm( return algorithm def _scoped_classifications( - self, config: ClassMaskingConfig, source_algorithm: Algorithm + self, config: ClassMaskingConfig, source_algorithm: Algorithm, masking_algorithm: Algorithm ) -> tuple[QuerySet[Classification], str]: """Resolve the terminal classifications to re-score from the config's scope. @@ -284,13 +284,27 @@ def _scoped_classifications( arrays, which measured 208s versus 0.5s on a 55,530-row scope. Broadening either scope to match several collections or a whole project would fan out and need de-duplication again — restrict the columns first. See #1376. + + Sources already re-scored by ``masking_algorithm`` are excluded via the + ``applied_to`` lineage, so a source is masked at most once per masking + algorithm even if it becomes terminal again later. A taxa list is identified + by primary key, not contents — re-masking against an edited list needs a new + list. See #1368. """ - base = Classification.objects.filter( - terminal=True, - algorithm=source_algorithm, - scores__isnull=False, - logits__isnull=False, - ).select_related("detection", "detection__occurrence") + base = ( + Classification.objects.filter( + terminal=True, + algorithm=source_algorithm, + scores__isnull=False, + logits__isnull=False, + ) + # If another task needs this replay guard, hoist it to a + # ClassificationQuerySet method (e.g. not_derived_by(algorithm)) + # rather than copying the exclude. + .exclude(derived_classifications__algorithm=masking_algorithm).select_related( + "detection", "detection__occurrence" + ) + ) if config.occurrence_id is not None: if not Occurrence.objects.filter(pk=config.occurrence_id).exists(): @@ -327,7 +341,7 @@ def run(self) -> None: masking_algorithm = self._get_or_create_masking_algorithm( source_algorithm, taxa_list, reweight=config.reweight ) - classifications, scope_desc = self._scoped_classifications(config, source_algorithm) + classifications, scope_desc = self._scoped_classifications(config, source_algorithm, masking_algorithm) self.logger.info(f"Applying class masking on {scope_desc} using taxa list {taxa_list.pk}") def _on_setup(total: int) -> None: diff --git a/ami/ml/post_processing/tests/test_class_masking.py b/ami/ml/post_processing/tests/test_class_masking.py index 0e9b28efa..bf38decc9 100644 --- a/ami/ml/post_processing/tests/test_class_masking.py +++ b/ami/ml/post_processing/tests/test_class_masking.py @@ -253,6 +253,53 @@ def test_task_run_collection_scope_persists_masking_algorithm(self): occ.refresh_from_db() self.assertEqual(occ.determination, self.species_taxa[1], "Occurrence determination follows the masked result") + def test_rerun_does_not_duplicate_masked_classifications(self): + """Re-running the same mask must not create a second masked classification for + a source already re-scored, even if that source became terminal again in between. + + Idempotency is keyed on the ``applied_to`` lineage — a source is masked at most + once per masking algorithm — not on the terminal flag. This makes it safe to + finish a partially completed run (e.g. one the health-check reaper revoked) or + to re-run after a re-classification / dedup pass re-terminalized a source. + """ + logits = [0.5, 3.0, 3.5] # excluded index 2 is top; index 1 is the in-list winner + taxa_list = TaxaList.objects.create(name="Idempotency list") + taxa_list.taxa.set(self.species_taxa[:2]) + + det, _ = self._detection_with_occurrence() + original = self._create_classification_with_logits(det, self.species_taxa[2], _softmax(logits), logits) + + def run(): + ClassMaskingTask( + source_image_collection_id=self.collection.pk, + taxa_list_id=taxa_list.pk, + algorithm_id=self.algorithm.pk, + ).run() + + run() + masking_algo = Algorithm.objects.get( + key=f"{self.algorithm.key}_filtered_by_taxa_list_{taxa_list.pk}_reweighted" + ) + self.assertEqual( + Classification.objects.filter(algorithm=masking_algo, applied_to=original).count(), + 1, + "First run masks the source exactly once", + ) + + # Simulate the source becoming terminal again (a dedup or re-classification pass) + # while its masked child still exists. The terminal filter alone would re-select + # it; the applied_to guard must still skip it. + original.refresh_from_db() + original.terminal = True + original.save(update_fields=["terminal"]) + + run() + self.assertEqual( + Classification.objects.filter(algorithm=masking_algo, applied_to=original).count(), + 1, + "Re-run must not create a duplicate masked classification for an already-masked source", + ) + def test_reweight_modes_get_distinct_masking_algorithms(self): """The reweight mode is part of the masking algorithm's identity. @@ -516,7 +563,10 @@ def test_collection_scope_returns_each_classification_once(self): taxa_list_id=taxa_list.pk, algorithm_id=self.algorithm.pk, ) - scoped, _ = task._scoped_classifications(task.config, self.algorithm) + masking_algorithm = task._get_or_create_masking_algorithm( + self.algorithm, taxa_list, reweight=task.config.reweight + ) + scoped, _ = task._scoped_classifications(task.config, self.algorithm, masking_algorithm) self.assertEqual(list(scoped.filter(pk=classification.pk)), [classification]) self.assertEqual( @@ -542,7 +592,10 @@ def test_scope_query_does_not_deduplicate_rows(self): ): with self.subTest(**kwargs): task = ClassMaskingTask(taxa_list_id=taxa_list.pk, algorithm_id=self.algorithm.pk, **kwargs) - scoped, _ = task._scoped_classifications(task.config, self.algorithm) + masking_algorithm = task._get_or_create_masking_algorithm( + self.algorithm, taxa_list, reweight=task.config.reweight + ) + scoped, _ = task._scoped_classifications(task.config, self.algorithm, masking_algorithm) self.assertFalse(scoped.query.distinct) def test_scope_size_is_reported_before_the_first_batch(self): diff --git a/ami/ml/tests.py b/ami/ml/tests.py index 9366eef47..bd92bb02f 100644 --- a/ami/ml/tests.py +++ b/ami/ml/tests.py @@ -2068,10 +2068,13 @@ def test_deployment_counts_refresh_after_save_results(self): ) -class TestAlgorithmViewSetProjectFilter(APITestCase): - """ - The algorithm list endpoint is scoped to algorithms belonging to - pipelines enabled for the active project. +class AlgorithmProjectTestBase(APITestCase): + """Shared fixture for the two project-scoped algorithm listings. + + Project A has an enabled pipeline carrying one algorithm that ran ("Algo Used") + and one that never did ("Algo Configured Unused"), plus a disabled pipeline whose + algorithm's determinations survive ("Algo Superseded"). Project B has its own + used algorithm. "Algo Orphan" belongs to no pipeline and never ran anywhere. """ def setUp(self): @@ -2081,28 +2084,58 @@ def setUp(self): self.project = Project.objects.create(name="Algo Project A", create_defaults=False) self.other_project = Project.objects.create(name="Algo Project B", create_defaults=False) - # Project A: one enabled pipeline, one disabled pipeline - self.algo_enabled = Algorithm.objects.create(name="Algo Enabled", version=1) - self.algo_disabled = Algorithm.objects.create(name="Algo Disabled", version=1) - # Project B: a different pipeline/algorithm + # Project A: an enabled-pipeline algorithm that has run, and one that never did. + self.algo_used = Algorithm.objects.create(name="Algo Used", version=1) + self.algo_configured_unused = Algorithm.objects.create(name="Algo Configured Unused", version=1) + # Project A: an old version on a now-disabled pipeline, whose determinations survive. + self.algo_superseded = Algorithm.objects.create(name="Algo Superseded", version=1) + # Project B: a different algorithm, also used. self.algo_other_project = Algorithm.objects.create(name="Algo Other Project", version=1) - # Unrelated algorithm not attached to any pipeline + # Unrelated algorithm attached to no pipeline and never run. self.algo_orphan = Algorithm.objects.create(name="Algo Orphan", version=1) enabled_pipeline = Pipeline.objects.create(name="Enabled Pipeline") - enabled_pipeline.algorithms.add(self.algo_enabled) + enabled_pipeline.algorithms.add(self.algo_used, self.algo_configured_unused) ProjectPipelineConfig.objects.create(project=self.project, pipeline=enabled_pipeline, enabled=True) disabled_pipeline = Pipeline.objects.create(name="Disabled Pipeline") - disabled_pipeline.algorithms.add(self.algo_disabled) + disabled_pipeline.algorithms.add(self.algo_superseded) ProjectPipelineConfig.objects.create(project=self.project, pipeline=disabled_pipeline, enabled=False) other_pipeline = Pipeline.objects.create(name="Other Project Pipeline") other_pipeline.algorithms.add(self.algo_other_project) ProjectPipelineConfig.objects.create(project=self.other_project, pipeline=other_pipeline, enabled=True) + self._classify_in_project(self.algo_used, self.project) + self._classify_in_project(self.algo_superseded, self.project) + self._classify_in_project(self.algo_other_project, self.other_project) + self.client.force_authenticate(user=self.user) + def _classify_in_project(self, algorithm, project): + """Give ``algorithm`` a classification whose capture belongs to ``project``.""" + source_image = SourceImage.objects.create(project=project) + detection = Detection.objects.create(source_image=source_image) + return Classification.objects.create( + detection=detection, + algorithm=algorithm, + timestamp=datetime.datetime.now(datetime.timezone.utc), + ) + + +class TestAlgorithmViewSetProjectFilter(AlgorithmProjectTestBase): + """ + The algorithm list endpoint scoped to a project shows what the project can run: + the algorithms on its enabled pipelines. + + It reflects configuration, not history — a freshly configured project sees its + algorithms before anything has run, and an algorithm only on a disabled pipeline + is not offered even if it ran in the past. The algorithms that actually produced + results are served separately as occurrence filter choices (see + TestOccurrenceAlgorithmChoices), and detail pages stay reachable for any + algorithm through the unscoped detail endpoint. + """ + def _list_algorithm_names(self, project_id=None): params = {"project_id": project_id} if project_id is not None else {} url = reverse_with_params("api:algorithm-list", params=params) @@ -2110,9 +2143,19 @@ def _list_algorithm_names(self, project_id=None): self.assertEqual(response.status_code, 200) return {row["name"] for row in response.json()["results"]} - def test_lists_only_enabled_pipeline_algorithms_for_project(self): + def test_project_list_shows_enabled_pipeline_algorithms(self): + """The scoped list is exactly the enabled pipelines' algorithms — including + one that has never run, so a new project can see what it is set up to use + before any job has completed.""" + names = self._list_algorithm_names(project_id=self.project.pk) + self.assertEqual(names, {"Algo Used", "Algo Configured Unused"}) + + def test_disabled_pipeline_algorithm_is_not_listed(self): + """An algorithm only on a pipeline the project has disabled is not part of + what the project can run, even though its past determinations survive. Those + stay reachable through the occurrence filter choices and the detail endpoint.""" names = self._list_algorithm_names(project_id=self.project.pk) - self.assertEqual(names, {"Algo Enabled"}) + self.assertNotIn("Algo Superseded", names) def test_other_project_only_sees_its_own_algorithms(self): names = self._list_algorithm_names(project_id=self.other_project.pk) @@ -2121,18 +2164,127 @@ def test_other_project_only_sees_its_own_algorithms(self): def test_unscoped_request_returns_all_algorithms(self): """Without project_id, current behavior lists all algorithms (unchanged).""" names = self._list_algorithm_names() - self.assertIn("Algo Enabled", names) - self.assertIn("Algo Disabled", names) - self.assertIn("Algo Other Project", names) - self.assertIn("Algo Orphan", names) + for name in ( + "Algo Used", + "Algo Superseded", + "Algo Configured Unused", + "Algo Other Project", + "Algo Orphan", + ): + self.assertIn(name, names) def test_detail_endpoint_unscoped_even_with_project_id(self): - """Detail stays unscoped so historical classification links still resolve.""" + """Detail stays unscoped so a link from a historical classification — here an + algorithm outside the project's enabled set — still resolves to its details + and category map.""" url = reverse_with_params( "api:algorithm-detail", - kwargs={"pk": self.algo_disabled.pk}, + kwargs={"pk": self.algo_orphan.pk}, params={"project_id": self.project.pk}, ) response = self.client.get(url) self.assertEqual(response.status_code, 200) - self.assertEqual(response.json()["name"], "Algo Disabled") + self.assertEqual(response.json()["name"], "Algo Orphan") + + +class TestOccurrenceAlgorithmChoices(AlgorithmProjectTestBase): + """ + The occurrence filter's algorithm choices, served at /occurrences/algorithms/, + are exactly the algorithms that produced results in the project. + + An algorithm qualifies by owning output rows: a detection made by it (detectors, + which never author a Classification) or a classification from it (classifiers and + standalone post-processing algorithms such as class masking). A superseded + pipeline version stays a choice as long as its results survive, and a configured + algorithm that never ran is not offered — the filter never lists a value with + zero matching occurrences. + """ + + def _choice_names(self, project_id): + url = reverse_with_params("api:occurrence-algorithms", params={"project_id": project_id}) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + return {row["name"] for row in response.json()["results"]} + + def test_choices_are_exactly_the_algorithms_that_produced_results(self): + """The classifier that ran and the superseded version whose determinations + survive are choices; the enabled pipeline's never-run algorithm is not. + This pins the semantic that choices follow results, not setup.""" + names = self._choice_names(self.project.pk) + self.assertEqual(names, {"Algo Used", "Algo Superseded"}) + + def test_configured_but_never_run_algorithm_is_not_a_choice(self): + """Filtering by an algorithm that never produced a result would always return + zero occurrences, so configuration alone does not admit one.""" + names = self._choice_names(self.project.pk) + self.assertNotIn("Algo Configured Unused", names) + + def test_detector_that_ran_is_a_choice_although_it_never_classified(self): + """Detectors set ``Detection.detection_algorithm`` and never write a + Classification, so they are reachable only through their detections. This pins + the regression where scoping choices purely by classification authorship + dropped every localizer.""" + detector = Algorithm.objects.create(name="Algo Detector", version=1, task_type="localization") + + source_image = SourceImage.objects.create(project=self.project) + Detection.objects.create(source_image=source_image, detection_algorithm=detector) + self.assertFalse(Classification.objects.filter(algorithm=detector).exists()) + + self.assertIn("Algo Detector", self._choice_names(self.project.pk)) + + def test_post_processing_algorithm_with_classifications_is_a_choice(self): + """A post-processing algorithm has no pipeline but produces determinations in + the project, so it must be offered — otherwise the user cannot filter + occurrences by the masked result.""" + masked_algo = Algorithm.objects.create(name="Class Masked Classifier", version=1) + self._classify_in_project(masked_algo, self.project) + + self.assertIn("Class Masked Classifier", self._choice_names(self.project.pk)) + + def test_classifications_in_other_project_do_not_leak(self): + """An algorithm whose classifications live in another project must not appear.""" + other_masked_algo = Algorithm.objects.create(name="Other Project Masked", version=1) + self._classify_in_project(other_masked_algo, self.other_project) + + self.assertNotIn("Other Project Masked", self._choice_names(self.project.pk)) + + def test_project_id_is_required(self): + """Choices are relative to a project; without one the request is rejected + rather than listing every algorithm on the platform.""" + url = reverse_with_params("api:occurrence-algorithms") + response = self.client.get(url) + self.assertEqual(response.status_code, 400) + + def test_response_is_paginated_like_a_list_endpoint(self): + """The endpoint returns the standard ``{count, results}`` shape the + frontend's entity picker consumes.""" + url = reverse_with_params("api:occurrence-algorithms", params={"project_id": self.project.pk}) + data = self.client.get(url).json() + self.assertEqual(data["count"], 2) + self.assertEqual(len(data["results"]), 2) + + def test_used_lookup_is_deduplicated_in_the_database(self): + """``used_in_project`` matches one classification row per determination, so it + must deduplicate in SQL rather than in Python. + + Without that, the rows fetched grow with a project's classification count — + hundreds of thousands on a real masking run — to identify a handful of algorithms. + The listed names are correct either way, so this asserts the row count of the + underlying lookup rather than the endpoint's output. The ``.order_by()`` matters: + Classification's default ordering would otherwise widen the DISTINCT back to one + row per classification. + """ + masked_algo = Algorithm.objects.create(name="Chatty Masked Classifier", version=1) + for _ in range(5): + self._classify_in_project(masked_algo, self.project) + + lookup = Classification.objects.filter( + algorithm_id=masked_algo.pk, + detection__source_image__project=self.project, + ).values_list("algorithm_id", flat=True) + + self.assertEqual(len(list(lookup)), 5, "The lookup matches one row per classification") + self.assertEqual( + len(list(lookup.order_by().distinct())), 1, "Deduplicating collapses them to the one algorithm" + ) + self.assertIn("Chatty Masked Classifier", self._choice_names(self.project.pk)) diff --git a/ami/ml/views.py b/ami/ml/views.py index 7de502f4a..63e460af6 100644 --- a/ami/ml/views.py +++ b/ami/ml/views.py @@ -56,11 +56,15 @@ class AlgorithmViewSet(DefaultViewSet, ProjectMixin): def get_queryset(self) -> QuerySet["Algorithm"]: qs: QuerySet["Algorithm"] = super().get_queryset() qs = qs.with_category_count() # type: ignore[union-attr] # Custom queryset method - # Only scope list by project. Detail stays unscoped so links from historical + # Only scope the list by project. Detail stays unscoped so links from historical # classifications whose pipeline is no longer enabled still resolve. if getattr(self, "action", None) == "list": project = self.get_active_project() if project: + # The project-scoped list shows the algorithms available to the project — those + # on its enabled pipelines — so a freshly configured project sees what it can + # run before anything has run. The algorithms that actually produced results + # (including superseded versions) are served by /occurrences/algorithms/. qs = qs.filter( pipelines__project_pipeline_configs__project=project, pipelines__project_pipeline_configs__enabled=True, diff --git a/ui/src/components/filtering/filters/algorithm-filter.tsx b/ui/src/components/filtering/filters/algorithm-filter.tsx index b4cbe932e..0e487880c 100644 --- a/ui/src/components/filtering/filters/algorithm-filter.tsx +++ b/ui/src/components/filtering/filters/algorithm-filter.tsx @@ -7,7 +7,7 @@ export const AlgorithmFilter = ({ onAdd, }: FilterProps & { placeholder?: string }) => ( { if (value) { onAdd(value) diff --git a/ui/src/data-services/constants.ts b/ui/src/data-services/constants.ts index 135896386..7cfd6b33c 100644 --- a/ui/src/data-services/constants.ts +++ b/ui/src/data-services/constants.ts @@ -14,6 +14,7 @@ export const API_ROUTES = { LOGOUT: 'auth/token/logout', ME: 'users/me', MEMBERS: (projectId: string) => `projects/${projectId}/members`, + OCCURRENCE_ALGORITHMS: 'occurrences/algorithms', OCCURRENCES: 'occurrences', PAGES: 'pages', PIPELINES: 'ml/pipelines',