-
Notifications
You must be signed in to change notification settings - Fork 14
Thumbnails API for SourceImages #1306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 9 commits
fff2f04
a2c26f6
8077c2a
d11cfa3
33f42df
04cb3b3
e9abc52
1a150a2
f94b5c8
c7484ab
182c035
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,18 @@ | ||
| import datetime | ||
| import io | ||
| import logging | ||
| from statistics import mode | ||
|
|
||
| from django.conf import settings | ||
| from django.contrib.postgres.search import TrigramSimilarity | ||
| from django.core import exceptions | ||
| from django.core.files.storage import default_storage | ||
| from django.db import models | ||
| from django.db.models import Prefetch, Q | ||
| from django.db.models.functions import Coalesce | ||
| from django.db.models.query import QuerySet | ||
| from django.forms import BooleanField, CharField, IntegerField | ||
| from django.shortcuts import get_object_or_404 | ||
| from django.shortcuts import get_object_or_404, redirect | ||
| from django.utils import timezone | ||
| from django_filters.rest_framework import DjangoFilterBackend | ||
| from drf_spectacular.types import OpenApiTypes | ||
|
|
@@ -24,6 +27,7 @@ | |
| from rest_framework.response import Response | ||
| from rest_framework.views import APIView | ||
|
|
||
| import ami.utils.s3 | ||
| from ami.base.filters import NullsLastOrderingFilter, ThresholdFilter | ||
| from ami.base.models import BaseQuerySet | ||
| from ami.base.pagination import LimitOffsetPaginationWithPermissions | ||
|
|
@@ -52,6 +56,7 @@ | |
| Site, | ||
| SourceImage, | ||
| SourceImageCollection, | ||
| SourceImageThumbnail, | ||
| SourceImageUpload, | ||
| Tag, | ||
| TaxaList, | ||
|
|
@@ -709,6 +714,82 @@ def unstar(self, _request, pk=None) -> Response: | |
| raise api_exceptions.ValidationError(detail="Source image must be associated with a project") | ||
|
|
||
|
|
||
| class SourceImageThumbnailViewSet(DefaultReadOnlyViewSet, ProjectMixin): | ||
| """ | ||
| Endpoint for capture thumbnails | ||
| """ | ||
|
|
||
| queryset = SourceImage.objects.all() | ||
|
|
||
| permission_classes = [ObjectPermission] | ||
|
|
||
| def list(self, request): | ||
| raise api_exceptions.NotFound(detail=f"No collection of thumbnails") | ||
|
|
||
| def retrieve(self, request, pk=None): | ||
| _sizes = settings.THUMBNAILS["SIZES"] | ||
| _prefix = settings.THUMBNAILS["STORAGE_PREFIX"] | ||
|
|
||
| label = self.request.query_params.get("label", next(iter(_sizes))) | ||
| size = _sizes.get(label, None) | ||
|
loppear marked this conversation as resolved.
|
||
| if size is None: | ||
| raise api_exceptions.ValidationError( | ||
| detail=f"Invalid thumbnail size label provided: {label} not in {', '.join(_sizes.keys())}" | ||
| ) | ||
| obj: SourceImage = self.get_object() | ||
| try: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm tempted to say we don't need the SourceImageThumbnail model and can rely on the exists() check in the storage. But then I suppose we have to parse the height/width from the file name, and make sure we have a way to create those consistently. So I could go either way! This will be a big table eventually.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming files are on S3, I don't think we'll want to do filesystem name checks. Table size will be commensurate to SourceImage, could be pruned for age if that becomes an issue, though would lose the "we still have thumbnails if source files are removed" benefit. We don't join to the table just per-image lookups - should probably add a (source_image, label) index. |
||
| thumb = obj.thumbnails.get(label=label) | ||
| except SourceImageThumbnail.DoesNotExist: | ||
| thumb = None | ||
| if ( | ||
| not thumb | ||
| or thumb.width != size["width"] | ||
| or thumb.last_modified < obj.last_modified | ||
| or not default_storage.exists(thumb.path) | ||
| ): | ||
| if obj.path and obj.deployment and obj.deployment.data_source: | ||
| config = obj.deployment.data_source.config | ||
| # Get the file | ||
| try: | ||
| img = ami.utils.s3.read_image(config=config, key=obj.path) | ||
|
mihow marked this conversation as resolved.
Outdated
|
||
| except ami.utils.s3.botocore.exceptions.ClientError as e: | ||
| logger.error(f"Could not read image for {obj.path}: {e}") | ||
| raise api_exceptions.NotFound(detail=f"SourceImage with id {obj.id} media not found") | ||
|
loppear marked this conversation as resolved.
Outdated
|
||
| else: | ||
| # Make the thumbnail | ||
| orig_width, orig_height = img.size | ||
| width = size["width"] | ||
| height = size.get("height", None) | ||
| if not height: | ||
| height = int(orig_height * (width / float(orig_width))) | ||
| new_size = (width, height) | ||
| img.thumbnail(new_size) | ||
|
|
||
| buffer = io.BytesIO() | ||
| img.save(buffer, format="JPEG") | ||
| contents = buffer.getvalue() | ||
| file_size = len(contents) | ||
|
|
||
| # Write to storage | ||
| buffer.seek(0) | ||
| thumbnail_key = f"{_prefix}capture_{obj.id}/{label}.jpg" | ||
| thumbnail_path = default_storage.save(thumbnail_key, buffer) | ||
|
|
||
| # Save to DB | ||
| width, height = img.size | ||
| # Remove prior thumbnails for this size | ||
| for t in obj.thumbnails.filter(label=label): | ||
| default_storage.delete(t.path) | ||
| t.delete() | ||
| thumb = obj.thumbnails.create( | ||
| path=thumbnail_path, label=label, width=width, height=height, size=file_size | ||
| ) | ||
| else: | ||
| raise api_exceptions.NotFound(detail=f"SourceImage with id {obj.id} media config not found") | ||
|
|
||
| return redirect(default_storage.url(thumb.path), permanent=True) | ||
|
|
||
|
|
||
| class SourceImageCollectionViewSet(DefaultViewSet, ProjectMixin): | ||
| """ | ||
| Endpoint for viewing capture sets or samples of captures. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,39 @@ | ||||||||||||||||||||||||||||||||||||
| # Generated by Django 4.2.10 on 2026-05-13 10:17 | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| from django.db import migrations, models | ||||||||||||||||||||||||||||||||||||
| import django.db.models.deletion | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| class Migration(migrations.Migration): | ||||||||||||||||||||||||||||||||||||
| dependencies = [ | ||||||||||||||||||||||||||||||||||||
| ("main", "0084_revoke_delete_job_from_roles"), | ||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| operations = [ | ||||||||||||||||||||||||||||||||||||
| migrations.CreateModel( | ||||||||||||||||||||||||||||||||||||
| name="SourceImageThumbnail", | ||||||||||||||||||||||||||||||||||||
| fields=[ | ||||||||||||||||||||||||||||||||||||
| ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), | ||||||||||||||||||||||||||||||||||||
| ("created_at", models.DateTimeField(auto_now_add=True)), | ||||||||||||||||||||||||||||||||||||
| ("updated_at", models.DateTimeField(auto_now=True)), | ||||||||||||||||||||||||||||||||||||
| ("path", models.CharField(blank=True, max_length=255)), | ||||||||||||||||||||||||||||||||||||
| ("label", models.CharField(blank=True, max_length=255, null=True)), | ||||||||||||||||||||||||||||||||||||
| ("width", models.IntegerField(blank=True, null=True)), | ||||||||||||||||||||||||||||||||||||
| ("height", models.IntegerField(blank=True, null=True)), | ||||||||||||||||||||||||||||||||||||
| ("size", models.BigIntegerField(blank=True, null=True)), | ||||||||||||||||||||||||||||||||||||
| ("last_modified", models.DateTimeField(blank=True, null=True, auto_now_add=True)), | ||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||
| "source_image", | ||||||||||||||||||||||||||||||||||||
| models.ForeignKey( | ||||||||||||||||||||||||||||||||||||
| null=True, | ||||||||||||||||||||||||||||||||||||
| on_delete=django.db.models.deletion.SET_NULL, | ||||||||||||||||||||||||||||||||||||
| related_name="thumbnails", | ||||||||||||||||||||||||||||||||||||
| to="main.sourceimage", | ||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+25
to
+33
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A thumbnail is meaningless without its 🐛 Proposed fix (
"source_image",
models.ForeignKey(
- null=True,
- on_delete=django.db.models.deletion.SET_NULL,
+ on_delete=django.db.models.deletion.CASCADE,
related_name="thumbnails",
to="main.sourceimage",
),
),Note: this requires removing 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||
| options={ | ||||||||||||||||||||||||||||||||||||
| "abstract": False, | ||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,8 +1,10 @@ | ||||||
| import copy | ||||||
| import datetime | ||||||
| import logging | ||||||
| import typing | ||||||
| from io import BytesIO | ||||||
|
|
||||||
| from django.conf import settings | ||||||
| from django.contrib.auth.models import AnonymousUser | ||||||
| from django.core.files.uploadedfile import SimpleUploadedFile | ||||||
| from django.db import connection, models | ||||||
|
|
@@ -38,7 +40,13 @@ | |||||
| from ami.ml.models.pipeline import Pipeline | ||||||
| from ami.ml.models.processing_service import ProcessingService | ||||||
| from ami.ml.models.project_pipeline_config import ProjectPipelineConfig | ||||||
| from ami.tests.fixtures.main import create_captures, create_occurrences, create_taxa, setup_test_project | ||||||
| from ami.tests.fixtures.main import ( | ||||||
| create_captures, | ||||||
| create_captures_from_files, | ||||||
| create_occurrences, | ||||||
| create_taxa, | ||||||
| setup_test_project, | ||||||
| ) | ||||||
| from ami.tests.fixtures.storage import populate_bucket | ||||||
| from ami.users.models import User | ||||||
| from ami.users.roles import BasicMember, Identifier, MLDataManager, ProjectManager, create_roles_for_project | ||||||
|
|
@@ -210,6 +218,102 @@ def test_existing_processing_service_new_project(self): | |||||
| ) | ||||||
|
|
||||||
|
|
||||||
| NEW_THUMBNAIL_SETTINGS = copy.deepcopy(settings.THUMBNAILS) | ||||||
| NEW_THUMBNAIL_SETTINGS["SIZES"]["small"]["width"] = 300 | ||||||
|
|
||||||
|
|
||||||
| class TestImageThumbnailViews(TestCase): | ||||||
| base_url = "http://testserver/api/v2/captures/thumbnails/" | ||||||
|
|
||||||
| def setUp(self) -> None: | ||||||
| self.project, self.deployment = setup_test_project() | ||||||
|
|
||||||
| self.captures = create_captures_from_files(deployment=self.deployment) | ||||||
| self.first_capture = self.captures[0][0] | ||||||
|
|
||||||
| return super().setUp() | ||||||
|
|
||||||
| def test_thumbnail_no_list(self): | ||||||
| response = self.client.get(f"/api/v2/captures/thumbnails/") | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the no-op f-string to fix Ruff F541. Line 237 uses an f-string without placeholders, which triggers Ruff F541 and can fail CI. 🔧 Proposed fix- response = self.client.get(f"/api/v2/captures/thumbnails/")
+ response = self.client.get("/api/v2/captures/thumbnails/")📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.15.12)[error] 237-237: f-string without any placeholders Remove extraneous (F541) 🤖 Prompt for AI Agents |
||||||
| self.assertEqual(response.status_code, 404) | ||||||
|
|
||||||
| def test_thumbnail_new(self): | ||||||
| response = self.client.get(f"/api/v2/captures/thumbnails/{self.first_capture.pk}/") | ||||||
| self.assertEqual(response.status_code, 301) | ||||||
| thumb = self.first_capture.thumbnails.get(label="small") | ||||||
| self.assertEqual(thumb.width, 240) | ||||||
| self.assertEqual(thumb.height, 180) | ||||||
| self.assertEqual(response.headers["Location"], f"/media/{thumb.path}") | ||||||
|
|
||||||
| def test_thumbnail_new_with_size(self): | ||||||
| response = self.client.get(f"/api/v2/captures/thumbnails/{self.first_capture.pk}/?label=medium") | ||||||
| self.assertEqual(response.status_code, 301) | ||||||
| thumb = self.first_capture.thumbnails.get(label="medium") | ||||||
| self.assertEqual(thumb.width, 1024) | ||||||
| self.assertEqual(thumb.height, 768) | ||||||
| self.assertEqual(response.headers["Location"], f"/media/{thumb.path}") | ||||||
|
|
||||||
| def test_thumbnail_new_with_invalid_size(self): | ||||||
| response = self.client.get(f"/api/v2/captures/thumbnails/{self.first_capture.pk}/?label=typo") | ||||||
| self.assertEqual(response.status_code, 400) | ||||||
|
|
||||||
| def test_thumbnail_exists(self): | ||||||
| response = self.client.get(f"/api/v2/captures/thumbnails/{self.first_capture.pk}/") | ||||||
| self.assertEqual(response.status_code, 301) | ||||||
| t_id = self.first_capture.thumbnails.first().pk | ||||||
| response = self.client.get(f"/api/v2/captures/thumbnails/{self.first_capture.pk}/") | ||||||
| self.assertEqual(self.first_capture.thumbnails.count(), 1) | ||||||
| self.assertEqual(self.first_capture.thumbnails.first().pk, t_id) | ||||||
| thumb = self.first_capture.thumbnails.get(label="small") | ||||||
| self.assertEqual(response.status_code, 301) | ||||||
| self.assertEqual(response.headers["Location"], f"/media/{thumb.path}") | ||||||
|
|
||||||
| def test_thumbnail_exists_newer_modified_source(self): | ||||||
| response = self.client.get(f"/api/v2/captures/thumbnails/{self.first_capture.pk}/") | ||||||
| self.assertEqual(response.status_code, 301) | ||||||
| thumb = self.first_capture.thumbnails.first() | ||||||
| self.first_capture.last_modified = datetime.datetime.now() | ||||||
| self.first_capture.save() | ||||||
| self.assertTrue(self.first_capture.last_modified > thumb.last_modified) | ||||||
| response = self.client.get(f"/api/v2/captures/thumbnails/{self.first_capture.pk}/") | ||||||
| self.assertEqual(self.first_capture.thumbnails.count(), 1) | ||||||
| self.assertNotEqual(self.first_capture.thumbnails.first().pk, thumb.pk) | ||||||
| thumb = self.first_capture.thumbnails.get(label="small") | ||||||
| self.assertEqual(response.status_code, 301) | ||||||
| self.assertEqual(response.headers["Location"], f"/media/{thumb.path}") | ||||||
|
|
||||||
| @override_settings(THUMBNAILS=NEW_THUMBNAIL_SETTINGS) | ||||||
| def test_thumbnail_settings_change_regenerates(self): | ||||||
| # A pre-existing different size thumb | ||||||
| self.first_capture.thumbnails.create(path="thumbs/test", label="small", width=240, height=180, size=0) | ||||||
| response = self.client.get(f"/api/v2/captures/thumbnails/{self.first_capture.pk}/") | ||||||
| self.assertEqual(self.first_capture.thumbnails.count(), 1) | ||||||
| thumb = self.first_capture.thumbnails.get(label="small") | ||||||
| self.assertEqual(thumb.width, 300) | ||||||
| self.assertEqual(response.status_code, 301) | ||||||
| self.assertEqual(response.headers["Location"], f"/media/{thumb.path}") | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||
|
|
||||||
| def test_captures_response_includes_thumbnail_urls(self): | ||||||
| response = self.client.get(f"/api/v2/captures/{self.first_capture.pk}/?project_id={self.project.pk}") | ||||||
| self.assertEqual(response.status_code, 200) | ||||||
| rec = response.json() | ||||||
| self.assertIn("thumbnails", rec) | ||||||
| self.assertURLEqual(rec["thumbnails"]["small"], f"{self.base_url}{self.first_capture.pk}/?label=small") | ||||||
| self.assertURLEqual(rec["thumbnails"]["medium"], f"{self.base_url}{self.first_capture.pk}/?label=medium") | ||||||
|
|
||||||
| def test_captures_list_response_includes_thumbnail_urls(self): | ||||||
| response = self.client.get(f"/api/v2/captures/?project_id={self.project.pk}") | ||||||
| self.assertEqual(response.status_code, 200) | ||||||
| capture_json = response.json()["results"][0] | ||||||
| self.assertIn("thumbnails", capture_json) | ||||||
| self.assertURLEqual( | ||||||
| capture_json["thumbnails"]["small"], f"{self.base_url}{self.first_capture.pk}/?label=small" | ||||||
| ) | ||||||
| self.assertURLEqual( | ||||||
| capture_json["thumbnails"]["medium"], f"{self.base_url}{self.first_capture.pk}/?label=medium" | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| class TestImageGrouping(TestCase): | ||||||
| def setUp(self) -> None: | ||||||
| print(f"Currently active database: {connection.settings_dict}") | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny that this
thumbnailfield has been hanging out empty all along! Glad to retire it