From e829e8b86839a26d54f2bcc863ba244bed9b164a Mon Sep 17 00:00:00 2001 From: Omar Carpentiero Date: Mon, 28 Sep 2026 14:04:53 +0200 Subject: [PATCH] fix(brain): paginate the patient list before building its rows The list built a status row for every patient in the project and paginated the resulting Python list, so a 10-row page paid for the whole project: each row lazily loaded its project and folder, asked for the rerunnable steps and the delete permission, and looked up every modality's ProcessingStep. Rows are now built for the visible page only. With a status filter the whole match set still has to be evaluated, but only through the in-memory status; modality_status takes the requires-processing flag precomputed once per request instead of querying it per patient. Fixes #97. Co-Authored-By: Claude Opus 5.5 --- brain/tests_patient_list_upload.py | 74 +++++++++++++++- brain/views.py | 137 ++++++++++++++++++----------- common/modality_config.py | 10 ++- 3 files changed, 167 insertions(+), 54 deletions(-) diff --git a/brain/tests_patient_list_upload.py b/brain/tests_patient_list_upload.py index 47063820..ba42d8af 100644 --- a/brain/tests_patient_list_upload.py +++ b/brain/tests_patient_list_upload.py @@ -1,9 +1,11 @@ from django.contrib.auth.models import User +from django.db import connection from django.test import TestCase +from django.test.utils import CaptureQueriesContext from django.urls import reverse from brain.models import Folder, Patient -from common.models import FileRegistry, Modality, Project, ProjectAccess +from common.models import FileRegistry, Job, Modality, Project, ProjectAccess class BrainPatientListTests(TestCase): @@ -41,6 +43,76 @@ def test_defaults_to_ten_and_applies_modality_status_filter(self): self.assertEqual(len(response.context["page_obj"].object_list), 10) self.assertEqual([item["patient"] for item in filtered.context["page_obj"].object_list], [patients[0]]) + def _add_patients(self, count, processed=False): + patients = [] + for index in range(count): + patient = Patient.objects.create(name=f"Brain extra {index}", project=self.project, folder=self.folder) + if processed: + FileRegistry.objects.create( + brain_patient=patient, + domain="brain", + modality=self.modality, + file_type="braintumor_mri_t1_raw", + file_path=f"brain/tests/{patient.pk}/t1.nii.gz", + file_size=2, + file_hash=f"{patient.pk:064d}", + ) + Job.objects.create( + domain="brain", + brain_patient=patient, + modality_slug=self.modality.slug, + input_files={"input": f"brain/tests/{patient.pk}/t1.nii.gz"}, + status="completed", + ) + patients.append(patient) + return patients + + def _queries_for(self, params=None): + # The first request of a test also saves the session; keep that out of the count. + self.client.get(reverse("brain:patient_list"), params or {}) + with CaptureQueriesContext(connection) as captured: + response = self.client.get(reverse("brain:patient_list"), params or {}) + self.assertEqual(response.status_code, 200) + return len(captured.captured_queries) + + def test_page_cost_does_not_grow_with_the_project(self): + """#97: rows used to be built for every patient before paginating.""" + self._add_patients(10, processed=True) + small = self._queries_for() + self._add_patients(30, processed=True) + self.assertEqual(self._queries_for(), small) + + def test_status_filter_page_cost_does_not_grow_with_the_project(self): + """The filtered path still reads every candidate, but builds rows for the page only.""" + params = {"status_braintumor-mri-t1": "processed"} + self._add_patients(10, processed=True) + small = self._queries_for(params) + self._add_patients(30, processed=True) + self.assertEqual(self._queries_for(params), small) + + def test_second_page_without_filters(self): + self._add_patients(11) + response = self.client.get(reverse("brain:patient_list"), {"page": 2}) + page_obj = response.context["page_obj"] + self.assertEqual(page_obj.number, 2) + self.assertEqual(page_obj.paginator.num_pages, 2) + self.assertEqual(page_obj.paginator.count, 11) + self.assertEqual(len(page_obj.object_list), 1) + self.assertIn("modality_status_list", page_obj.object_list[0]) + + def test_status_filter_paginates_the_matches(self): + processed = self._add_patients(12, processed=True) + self._add_patients(5) + response = self.client.get( + reverse("brain:patient_list"), {"status_braintumor-mri-t1": "processed", "page": 2} + ) + page_obj = response.context["page_obj"] + self.assertEqual(page_obj.paginator.count, 12) + self.assertEqual(len(page_obj.object_list), 2) + for item in page_obj.object_list: + self.assertIn(item["patient"], processed) + self.assertEqual(item["modality_statuses"]["braintumor-mri-t1"], "processed") + def test_invalid_page_size_falls_back_to_ten(self): response = self.client.get(reverse("brain:patient_list"), {"per_page": "200"}) self.assertEqual(response.context["per_page"], 10) diff --git a/brain/views.py b/brain/views.py index 38720656..8e6a5532 100644 --- a/brain/views.py +++ b/brain/views.py @@ -17,6 +17,7 @@ from common.export_share import is_share_expired from common.file_access import exists as artifact_exists from common.modality_config import ( + modality_requires_processing, modality_status, rerun_step_labels, rerunnable_steps_for_patient, @@ -253,9 +254,67 @@ def patient_detail(request, patient_id): return render_with_fallback(request, "patient_detail", context) +def _modality_status_list(patient, allowed_modalities, requires_processing): + """Per-modality status for one list row, from the prefetched files and jobs only. + + ``requires_processing`` maps slug -> `modality_requires_processing`, looked up + once per request rather than once per patient. + """ + files_by_modality = {} + for file_obj in patient.files.all(): + if file_obj.modality and file_obj.modality.slug: + files_by_modality.setdefault(file_obj.modality.slug, []).append(file_obj) + jobs_by_modality = {} + for job in patient.jobs.all(): + jobs_by_modality.setdefault(job.modality_slug, []).append(job) + modality_status_list = [] + for modality in allowed_modalities: + slug = modality.slug or "" + if slug in {"rawzip", "voice"}: + continue + status = modality_status( + slug, + jobs_by_modality.get(slug, []), + bool(files_by_modality.get(slug)), + requires_processing=requires_processing.get(slug), + ) + modality_status_list.append({ + "slug": slug, + "name": modality.name, + "icon": modality.icon or "", + "label": modality.label or "", + "status": status, + }) + return modality_status_list + + +def _patient_row(request, patient, modality_status_list): + """The template's row for one patient. + + Unlike `_modality_status_list` this issues queries of its own (the rerun + picker's steps, the delete permission), so it is only ever run for the rows + on the page being shown -- see #97. + """ + voice_captions = list(patient.voice_captions.all()) + return { + "patient": patient, + "voice_caption_processing": any(vc.processing_status in ["pending", "processing"] for vc in voice_captions), + "voice_caption_processed": bool(voice_captions) and all(vc.processing_status == "completed" for vc in voice_captions), + "voice_caption_count": len(voice_captions), + "voice_annotators": list({vc.user.username for vc in voice_captions}), + "tags": patient.tag_names(), + "folder": patient.folder, + "available_modalities": [m.slug for m in patient.modalities.all()], + "modality_statuses": {item["slug"]: item["status"] for item in modality_status_list}, + "modality_status_list": modality_status_list, + "rerunnable_steps": rerunnable_steps_for_patient(list(patient.files.all()), modality_status_list, patient=patient), + "can_delete": bool(user_is_project_admin(request.user, patient.project) or user_can_delete_single_patient(request.user, patient.folder, patient.project)), + } + + @login_required def patient_list(request): - patients = Patient.objects.select_related("uploaded_by").prefetch_related( + patients = Patient.objects.select_related("uploaded_by", "project", "folder").prefetch_related( "voice_captions", "voice_captions__user", "tags", @@ -313,57 +372,8 @@ def patient_list(request): if value in {"processed", "processing", "failed"}: status_filters[slug] = value - patients_with_status = [] # UI flag for the project the list is showing; each row is checked on its own project. is_admin = user_is_project_admin(request.user, session_project(request)) - for patient in patients: - voice_captions = list(patient.voice_captions.all()) - patient_files = list(patient.files.all()) - patient_jobs = list(patient.jobs.all()) if hasattr(patient, "jobs") else [] - files_by_modality = {} - for file_obj in patient_files: - if file_obj.modality and file_obj.modality.slug: - files_by_modality.setdefault(file_obj.modality.slug, []).append(file_obj) - jobs_by_modality = {} - for job in patient_jobs: - jobs_by_modality.setdefault(job.modality_slug, []).append(job) - modality_status_list = [] - for modality in allowed_modalities: - slug = modality.slug or "" - if slug in {"rawzip", "voice"}: - continue - status = modality_status( - slug, - jobs_by_modality.get(slug, []), - bool(files_by_modality.get(slug)), - ) - modality_status_list.append({ - "slug": slug, - "name": modality.name, - "icon": modality.icon or "", - "label": modality.label or "", - "status": status, - }) - patients_with_status.append({ - "patient": patient, - "voice_caption_processing": any(vc.processing_status in ["pending", "processing"] for vc in voice_captions), - "voice_caption_processed": bool(voice_captions) and all(vc.processing_status == "completed" for vc in voice_captions), - "voice_caption_count": len(voice_captions), - "voice_annotators": list({vc.user.username for vc in voice_captions}), - "tags": patient.tag_names(), - "folder": patient.folder, - "available_modalities": [m.slug for m in patient.modalities.all()], - "modality_statuses": {item["slug"]: item["status"] for item in modality_status_list}, - "modality_status_list": modality_status_list, - "rerunnable_steps": rerunnable_steps_for_patient(patient_files, modality_status_list, patient=patient), - "can_delete": bool(user_is_project_admin(request.user, patient.project) or user_can_delete_single_patient(request.user, patient.folder, patient.project)), - }) - - if status_filters: - patients_with_status = [ - item for item in patients_with_status - if all(item["modality_statuses"].get(slug, "absent") == value for slug, value in status_filters.items()) - ] try: per_page = int(request.GET.get("per_page", 10)) @@ -371,7 +381,32 @@ def patient_list(request): per_page = 10 if per_page not in {10, 20, 50, 100}: per_page = 10 - page_obj = Paginator(patients_with_status, per_page).get_page(request.GET.get("page")) + page_number = request.GET.get("page") + requires_processing = {m.slug: modality_requires_processing(m.slug) for m in allowed_modalities if m.slug} + + # Paginate before building rows: a row costs queries of its own, so building + # one per patient in the project made every page as slow as the whole + # project (#97). A status filter is computed, not stored, so it still has to + # look at every matching patient -- but only through the in-memory status, + # and the full row is still built for the page alone. + if status_filters: + candidates = [] + for patient in patients: + modality_status_list = _modality_status_list(patient, allowed_modalities, requires_processing) + statuses = {item["slug"]: item["status"] for item in modality_status_list} + if all(statuses.get(slug, "absent") == value for slug, value in status_filters.items()): + candidates.append((patient, modality_status_list)) + page_obj = Paginator(candidates, per_page).get_page(page_number) + page_obj.object_list = [ + _patient_row(request, patient, modality_status_list) + for patient, modality_status_list in page_obj.object_list + ] + else: + page_obj = Paginator(patients, per_page).get_page(page_number) + page_obj.object_list = [ + _patient_row(request, patient, _modality_status_list(patient, allowed_modalities, requires_processing)) + for patient in page_obj.object_list + ] project_id = current_project_id folders = filter_folders_for_user( request.user, diff --git a/common/modality_config.py b/common/modality_config.py index f050cc85..a9518c13 100644 --- a/common/modality_config.py +++ b/common/modality_config.py @@ -88,7 +88,7 @@ def modality_is_blocking(modality_slug): return modality_requires_processing(modality_slug) -def modality_status(modality_slug, jobs, has_files): +def modality_status(modality_slug, jobs, has_files, requires_processing=None): """The patient-row status for one modality's pill. ``'failed' | 'processing' | 'pending' | 'processed' | 'absent'``, in that @@ -100,8 +100,14 @@ def modality_status(modality_slug, jobs, has_files): makes it green. That is also what clears the debris -- a patient uploaded while a step-less modality still spawned a Job carries a 'failed' row that no rerun can ever complete, because there is no algo to run. + + ``requires_processing`` is :func:`modality_requires_processing` for the slug, + for callers that status many patients at once: it is a query, and asking it + per patient made a list's cost grow with the project (#97). """ - if modality_requires_processing(modality_slug): + if requires_processing is None: + requires_processing = modality_requires_processing(modality_slug) + if requires_processing: statuses = {getattr(job, "status", "") for job in jobs or ()} if "failed" in statuses: return "failed"