Merge pull request #845 from ImageMarkup/misc-performance-enhancements

Miscellaneous performance improvements

isic/core/pagination.py

@@ -74,7 +74,9 @@ def decode_cursor(cls, encoded_cursor: str | None) -> Cursor:
 
     class Output(Schema):
         results: list[Any] = Field(description="The page of objects.")
-        count: int = Field(description="The total number of results across all pages.")
+        count: int | None = Field(
+            description="The total number of results across all pages. Only available on the first page."  # noqa: E501
+        )
         next: str | None = Field(description="URL of next page of results if there is one.")
         previous: str | None = Field(description="URL of previous page of results if there is one.")
 
@@ -92,7 +94,13 @@ def paginate_queryset(
             queryset = queryset.order_by(*self.default_ordering)
 
         order = queryset.query.order_by
-        total_count = queryset.count()
+
+        # only count the total number of results if a position is absent, usually indicating that
+        # we're on the first page. this improves performance for larger queries.
+        if pagination.cursor.position is None:
+            total_count = queryset.count()
+        else:
+            total_count = None
 
         base_url = request.build_absolute_uri()
         cursor = pagination.cursor

isic/core/views/collections.py

@@ -34,10 +34,9 @@ def collection_list(request):
     collections = get_visible_objects(
         request.user,
         "core.view_collection",
-        Collection.objects.annotate(num_images=Count("images", distinct=True)).order_by(
-            "-pinned", "name"
-        ),
+        Collection.objects.order_by("-pinned", "name"),
     )
+
     if request.user.is_authenticated:
         counts = collections.aggregate(
             pinned=Count("pk", filter=Q(pinned=True)),
@@ -55,6 +54,19 @@ def collection_list(request):
     paginator = Paginator(filter.qs, 25)
     page = paginator.get_page(request.GET.get("page"))
 
+    collection_counts = (
+        Collection.images.through.objects.filter(
+            collection_id__in=page.object_list.values_list("pk", flat=True)
+        )
+        .values("collection_id")
+        .annotate(num_images=Count("image_id"))
+    )
+
+    collection_counts = {c["collection_id"]: c["num_images"] for c in collection_counts}
+
+    for collection in page:
+        collection.num_images = collection_counts.get(collection.pk, 0)
+
     return render(
         request,
         "core/collection_list.html",

isic/ingest/templates/ingest/ingest_review.html

@@ -21,13 +21,11 @@
                 <div class="flex">
                   <p class="text-sm font-medium text-indigo-600 truncate">
                     {{ cohort.name }}
-                    {% with unreviewed_count=cohort.accessions.unreviewed.count %}
-                      {% if unreviewed_count %}
-                        <p class="px-2 ml-2 text-xs leading-5 font-semibold rounded-full bg-yellow-100 text-yellow-800">
-                          {{ unreviewed_count|intcomma }} unreviewed
-                        </p>
-                      {%endif%}
-                    {% endwith %}
+                    {% if cohort.unreviewed_count %}
+                      <p class="px-2 ml-2 text-xs leading-5 font-semibold rounded-full bg-yellow-100 text-yellow-800">
+                        {{ cohort.unreviewed_count|intcomma }} unreviewed
+                      </p>
+                    {%endif%}
                   </p>
                 </div>
                 <div class="sm:flex">

isic/ingest/tests/test_api_pagination.py

@@ -10,6 +10,7 @@ def test_pagination(image_factory, staff_client):
     resp = staff_client.get("/api/v2/images/", data={"limit": 1})
 
     assert resp.status_code == 200, resp.json()
+    assert resp.json()["count"] == 2
     assert len(resp.json()["results"]) == 1
     # default order is -created, so second image should be first
     assert resp.json()["results"][0]["isic_id"] == images[1].isic_id
@@ -21,6 +22,8 @@ def test_pagination(image_factory, staff_client):
     # follow next page link
     resp = staff_client.get(resp.json()["next"])
     assert resp.status_code == 200, resp.json()
+    # counts are not included in paginated responses
+    assert resp.json()["count"] is None
     assert len(resp.json()["results"]) == 1
     assert resp.json()["results"][0]["isic_id"] == images[0].isic_id
     assert resp.json()["next"] is None
@@ -31,6 +34,7 @@ def test_pagination(image_factory, staff_client):
     # make sure previous links also work
     resp = staff_client.get(resp.json()["previous"], data={"limit": 1})
     assert resp.status_code == 200, resp.json()
+    assert resp.json()["count"] == 2
     assert len(resp.json()["results"]) == 1
     assert resp.json()["results"][0]["isic_id"] == images[1].isic_id
     assert resp.json()["previous"] is None

isic/ingest/views/cohort.py

@@ -1,3 +1,4 @@
+from collections import defaultdict
 import itertools
 
 from django.contrib import messages
@@ -13,7 +14,7 @@
 from isic.core.permissions import needs_object_permission
 from isic.ingest.forms import MergeCohortForm
 from isic.ingest.models import Cohort
-from isic.ingest.models.accession import AccessionStatus
+from isic.ingest.models.accession import Accession, AccessionStatus
 from isic.ingest.models.contributor import Contributor
 from isic.ingest.services.cohort import cohort_merge, cohort_publish_initialize
 from isic.ingest.views import make_breadcrumbs
@@ -36,6 +37,13 @@ def cohort_counts(cohort: Cohort) -> dict[str, int]:
 def cohort_list(request):
     contributors = Contributor.objects.prefetch_related("cohorts").order_by("institution_name")
 
+    counts_by_cohort = Accession.objects.values("cohort_id").annotate(
+        lesion_count=Count("lesion", distinct=True),
+        patient_count=Count("patient", distinct=True),
+        accession_count=Count("pk"),
+    )
+    counts_by_cohort = {row["cohort_id"]: row for row in counts_by_cohort}
+
     rows = []
     for contributor in contributors.all():
         display_contributor = True
@@ -44,11 +52,7 @@ def cohort_list(request):
         ):
             display_attribution = True
             for cohort in cohorts:
-                aggregate_data = cohort.accessions.aggregate(
-                    lesion_count=Count("lesion", distinct=True),
-                    patient_count=Count("patient", distinct=True),
-                    accession_count=Count("pk"),
-                )
+                aggregate_data = counts_by_cohort.get(cohort.pk, defaultdict(int))
                 rows.append(
                     {
                         "contributor": contributor,

isic/ingest/views/review.py

@@ -9,7 +9,7 @@
 
 from isic.core.permissions import needs_object_permission
 from isic.ingest.models import Cohort
-from isic.ingest.models.accession import AccessionStatus
+from isic.ingest.models.accession import Accession, AccessionStatus
 
 from . import make_breadcrumbs
 
@@ -39,11 +39,27 @@ def ingest_review(request):
     cohorts = Cohort.objects.select_related("contributor", "creator").order_by("-created")
     paginator = Paginator(cohorts, 10)
     cohorts_page = paginator.get_page(request.GET.get("page"))
+    unreviewed_counts = (
+        Accession.objects.filter(cohort__in=cohorts_page)
+        .values("cohort_id")
+        .annotate(
+            unreviewed_count=Count("id", filter=Q(review=None, status=AccessionStatus.SUCCEEDED))
+        )
+    )
+    unreviewed_counts = {row["cohort_id"]: row["unreviewed_count"] for row in unreviewed_counts}
+
+    for cohort in cohorts_page:
+        cohort.unreviewed_count = unreviewed_counts.get(cohort.pk, 0)
 
     return render(
         request,
         "ingest/ingest_review.html",
-        {"cohorts": cohorts_page, "num_cohorts": paginator.count, "paginator": paginator},
+        {
+            "cohorts": cohorts_page,
+            "num_cohorts": paginator.count,
+            "paginator": paginator,
+            "unreviewed_counts": unreviewed_counts,
+        },
     )