diff --git a/.agents/AGENTS.md b/.agents/AGENTS.md index 458ae513b..710d72148 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..b499002b9 100644 --- a/ami/main/api/views.py +++ b/ami/main/api/views.py @@ -1222,11 +1222,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 +1241,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.detected_or_classified_by(algorithm_ids) if algorithm_ids_exclusive: - queryset = queryset.exclude(detections__classifications__algorithm__in=algorithm_ids_exclusive) + queryset = queryset.not_detected_or_classified_by(algorithm_ids_exclusive) return queryset diff --git a/ami/main/models.py b/ami/main/models.py index 3662b4107..017b51263 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 _machine_results_by(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).""" + 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 detected_or_classified_by(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._machine_results_by(algorithm_ids)) + + def not_detected_or_classified_by(self, algorithm_ids) -> "OccurrenceQuerySet": + """Occurrences with no result from any of the given algorithms.""" + return self.exclude(self._machine_results_by(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..fb8bb771f 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.detected_or_classified_by / not_detected_or_classified_by, + 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).detected_or_classified_by([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).detected_or_classified_by([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).detected_or_classified_by([self.classifier.pk]) + self.assertEqual(matched.count(), 2) + + def test_exclude_removes_matching_occurrences(self): + remaining = Occurrence.objects.filter(project=self.project).not_detected_or_classified_by([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.detected_or_classified_by(ids)) + excluded = self._pks(base.not_detected_or_classified_by(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 ffc77f346..512d2833c 100644 --- a/ami/ml/post_processing/class_masking.py +++ b/ami/ml/post_processing/class_masking.py @@ -263,19 +263,38 @@ 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. ``config_schema`` guarantees exactly one scope id is set, so the single ``else`` branch is sound. + + Sources already re-scored by ``masking_algorithm`` are excluded via the + ``applied_to`` lineage, so a source carries at most one masked classification + per masking algorithm however many times the mask is re-run. + + The ``terminal=True`` filter alone already handles an interrupted run, because + demoting a source and writing its masked child share one transaction: a resumed + run sees completed sources as non-terminal and skips them. The lineage guard + covers the case the flag cannot — a source restored to terminal by a later dedup + or re-classification pass, which the flag would re-select and mask a second time. + + A taxa list is identified here by primary key, not by its contents. Editing a + list in place and re-running the same mask therefore skips the sources it + already masked, leaving them scored against the list's earlier membership. + Re-masking against changed taxa needs a new taxa list. """ - 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, + ) + .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(): @@ -312,7 +331,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_batch(m: dict) -> None: diff --git a/ami/ml/post_processing/tests/test_class_masking.py b/ami/ml/post_processing/tests/test_class_masking.py index b0578272c..458018ca7 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. diff --git a/ami/ml/serializers.py b/ami/ml/serializers.py index e7e9e6aaf..4162b76db 100644 --- a/ami/ml/serializers.py +++ b/ami/ml/serializers.py @@ -30,6 +30,7 @@ class Meta: class AlgorithmSerializer(DefaultSerializer): category_map = MinimalCategoryMapNestedSerializer(read_only=True, source="category_map_id") + enabled_in_project = serializers.SerializerMethodField() class Meta: model = Algorithm @@ -45,10 +46,22 @@ class Meta: "task_type", "category_map", "category_count", + "enabled_in_project", "created_at", "updated_at", ] + def get_enabled_in_project(self, obj) -> bool | None: + """Whether the algorithm is on a pipeline the active project has enabled. + + The project algorithm list includes algorithms that produced results but are no + longer enabled — superseded versions, standalone post-processing algorithms — so + the UI can gray those out. Only the project-scoped list annotates this; it is + ``None`` on the unscoped list and on detail responses, where the flag has no + project to be relative to. + """ + return getattr(obj, "enabled_in_project", None) + class AlgorithmNestedSerializer(DefaultSerializer): class Meta: diff --git a/ami/ml/tests.py b/ami/ml/tests.py index cb88ee6fd..92f9f61d7 100644 --- a/ami/ml/tests.py +++ b/ami/ml/tests.py @@ -2037,8 +2037,15 @@ 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. + The algorithm list endpoint is scoped to the algorithms that actually produced + results in the active project, regardless of pipeline configuration. + + 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 listed as long as its results survive, and an algorithm configured on + an enabled pipeline that has never run does not appear at all — the list reflects + what happened, not what is set up. """ def setUp(self): @@ -2048,38 +2055,104 @@ 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 _list_algorithm_names(self, project_id=None): + 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), + ) + + def _list_rows(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) response = self.client.get(url) self.assertEqual(response.status_code, 200) - return {row["name"] for row in response.json()["results"]} + return {row["name"]: row for row in response.json()["results"]} + + def _list_algorithm_names(self, project_id=None): + return set(self._list_rows(project_id).keys()) + + def test_lists_only_algorithms_that_produced_results(self): + """The project list is exactly the algorithms with output here: the classifier + that ran and the superseded version whose determinations survive. The enabled + pipeline's never-run algorithm is absent, proving configuration alone does not + admit an algorithm.""" + names = self._list_algorithm_names(project_id=self.project.pk) + self.assertEqual(names, {"Algo Used", "Algo Superseded"}) + + def test_configured_but_never_run_algorithm_is_hidden(self): + """An algorithm on an enabled pipeline that never produced a result does not + appear. This pins the semantic that the list follows results, not setup.""" + names = self._list_algorithm_names(project_id=self.project.pk) + self.assertNotIn("Algo Configured Unused", names) + + def test_superseded_pipeline_version_with_results_is_listed(self): + """An algorithm whose pipeline is disabled for the project — an older model + version, for instance — stays listed as long as its determinations exist, so the + user can still filter occurrences by what an earlier run produced.""" + names = self._list_algorithm_names(project_id=self.project.pk) + self.assertIn("Algo Superseded", names) + + def test_enabled_in_project_flag_distinguishes_current_from_historical(self): + """Every listed algorithm carries `enabled_in_project`: True when it is on a + pipeline the project has enabled, False when it only ran historically (a + superseded version on a disabled pipeline). The UI grays out the False ones, + which is what lets the same list serve both the algorithms page and the + occurrence filter.""" + rows = self._list_rows(project_id=self.project.pk) + self.assertTrue(rows["Algo Used"]["enabled_in_project"]) + self.assertFalse(rows["Algo Superseded"]["enabled_in_project"]) + + def test_enabled_in_project_flag_is_null_when_unscoped(self): + """The flag is relative to a project, so the unscoped list reports it as null + rather than guessing a project to be enabled in.""" + rows = self._list_rows() + self.assertIsNone(rows["Algo Used"]["enabled_in_project"]) + + def test_detector_that_ran_is_listed_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 the list purely by classification authorship dropped + every localizer from the project's algorithm list.""" + 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()) - def test_lists_only_enabled_pipeline_algorithms_for_project(self): names = self._list_algorithm_names(project_id=self.project.pk) - self.assertEqual(names, {"Algo Enabled"}) + self.assertIn("Algo Detector", names) def test_other_project_only_sees_its_own_algorithms(self): names = self._list_algorithm_names(project_id=self.other_project.pk) @@ -2088,18 +2161,64 @@ 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 Used", names) + self.assertIn("Algo Superseded", names) + self.assertIn("Algo Configured Unused", names) self.assertIn("Algo Other Project", names) self.assertIn("Algo Orphan", names) def test_detail_endpoint_unscoped_even_with_project_id(self): - """Detail stays unscoped so historical classification links still resolve.""" + """Detail stays unscoped so links to an algorithm outside the project's used + set — here an orphan that never ran — still resolve.""" 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") + + def test_lists_post_processing_algorithm_with_classifications_in_project(self): + """A post-processing algorithm has no pipeline but produces determinations in + the project, so the list must include it — 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) + + names = self._list_algorithm_names(project_id=self.project.pk) + self.assertIn("Class Masked Classifier", names) + + 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._list_algorithm_names(project_id=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) + + names = self._list_algorithm_names(project_id=self.project.pk) + self.assertNotIn("Other Project Masked", names) diff --git a/ami/ml/views.py b/ami/ml/views.py index 7de502f4a..4a175a88a 100644 --- a/ami/ml/views.py +++ b/ami/ml/views.py @@ -1,7 +1,7 @@ import logging from django.db import transaction -from django.db.models import Prefetch +from django.db.models import Exists, OuterRef, Prefetch from django.db.models.query import QuerySet from django.utils.text import slugify from drf_spectacular.utils import extend_schema @@ -56,15 +56,29 @@ 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: - qs = qs.filter( - pipelines__project_pipeline_configs__project=project, - pipelines__project_pipeline_configs__enabled=True, - ).distinct() + # Scope the list to algorithms that actually produced results in the project — + # any superseded pipeline version or standalone post-processing algorithm whose + # output still exists, so the user can filter occurrences by anything that ran. + # Pipeline configuration is not consulted for membership; configured-but-never-run + # algorithms do not appear. See the method for cost characteristics. + qs = qs.used_in_project(project) # type: ignore[union-attr] # Custom queryset method + # Flag each one with whether it is still enabled for the project (on a pipeline + # the project has enabled). The list intentionally includes algorithms that ran + # but are no longer enabled, e.g. superseded versions; the UI grays those out. + qs = qs.annotate( + enabled_in_project=Exists( + ProjectPipelineConfig.objects.filter( + project=project, + enabled=True, + pipeline__algorithms=OuterRef("pk"), + ) + ) + ) return qs @extend_schema(parameters=[project_id_doc_param]) diff --git a/ui/src/data-services/models/algorithm.ts b/ui/src/data-services/models/algorithm.ts index c36d507c4..b4dc1871e 100644 --- a/ui/src/data-services/models/algorithm.ts +++ b/ui/src/data-services/models/algorithm.ts @@ -43,4 +43,12 @@ export class Algorithm extends Entity { ? this._algorithm.category_count : undefined } + + // Whether the algorithm is on a pipeline the active project has enabled. The + // project list also includes algorithms that only ran historically (superseded + // versions, post-processing algorithms), which come back false so the UI can gray + // them out. Undefined on the unscoped list, where there is no project to be enabled in. + get enabledInProject(): boolean | undefined { + return this._algorithm.enabled_in_project ?? undefined + } } diff --git a/ui/src/nova-ui-kit/components/table/table/table.tsx b/ui/src/nova-ui-kit/components/table/table/table.tsx index 987a21b4f..f61f86545 100644 --- a/ui/src/nova-ui-kit/components/table/table/table.tsx +++ b/ui/src/nova-ui-kit/components/table/table/table.tsx @@ -29,6 +29,7 @@ interface TableProps { items?: T[] onSelectedItemsChange?: (selectedItems: string[]) => void onSortSettingsChange?: (sortSettings?: TableSortSettings) => void + rowClassName?: (item: T) => string | undefined selectable?: boolean selectedItems?: string[] sortable?: boolean @@ -43,6 +44,7 @@ export const Table = ({ items = [], onSelectedItemsChange, onSortSettingsChange, + rowClassName, selectable, selectedItems = [], sortable, @@ -125,7 +127,7 @@ export const Table = ({ {items.map((item, rowIndex) => ( - + {selectable && ( diff --git a/ui/src/pages/project/algorithms/algorithms.tsx b/ui/src/pages/project/algorithms/algorithms.tsx index dc66fd17e..d4c24a141 100644 --- a/ui/src/pages/project/algorithms/algorithms.tsx +++ b/ui/src/pages/project/algorithms/algorithms.tsx @@ -61,6 +61,11 @@ export const Algorithms = () => { isLoading={isLoading} items={algorithms} onSortSettingsChange={setSort} + // Algorithms that ran in the project but are no longer enabled here (superseded + // versions, post-processing algorithms) are shown grayed rather than hidden. + rowClassName={(algorithm) => + algorithm.enabledInProject === false ? 'opacity-50' : undefined + } sortable sortSettings={sort} />