Add ADRF and make the thumbnail view async #3020

sarayourfriend · 2023-09-13T01:23:06Z

Fixes

Description

This is the first step to convert the image proxy route to async. It converts the overall view to async. I've split this into a separate PR because (a) it is complex enough on its own and (b) converting the actual image_proxy.get function is blocked on sharing the aiohttp client session (for which I'll have another PR up later today, it's already finished I just forgot to push it last night 🤦).

So this PR only converts the view side of things to async. It does this by abstracting the thumbnails route in a slightly different way than it was before, which allows the audio and image views to ignore most of the safe-sync-in-async compatibility issues. To make that interplay a little easier, I decided to refactor image_proxy.get to use two MRO objects for the input, rather than the one-by-one parameter definitions. Now that I'm writing this, I'm realising that I could have avoided the changes to image_proxy.get itself by creating a new response definition for get_image_proxy_media_info and then using asdict to spread that into image_proxy.get. If y'all want me to make that change, I think it's a fine one to simplify the overall PR. On the other hand, I think the new API is also a bit cleaner, if potentially too abstract. Let me know what y'all think because I'm two minds about it.

Testing Instructions

Checkout the PR and run just build web to get the new dependencies. Also keep in mind that this PR depends on #3011.

Run just api/up and make various search requests, thumbnail, and related requests. Generally: exercise the image and audio media views and their various routes. Everything should essentially work as it did before. The existing API integration tests should also continue to work, along will all unit tests. This change should be generally totally transparent and not result in any observable differences on the consumer side, which is why I didn't add any new tests for this: the existing tests already cover the media view and should continue to work without changes. The only tests I changed were to use pook a little more fluidly and to accommodate the changes to the image_proxy.get function's interface.

Checklist

My pull request has a descriptive title (not a vague title likeUpdate index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.
[N/A] I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

sarayourfriend · 2023-11-16T19:20:07Z

api/Pipfile

-django-split-settings = "*"
-psycopg = "~=3.1"


Just moving things into alphabetical order here. The only meaningful change is to add adrf at the top.

sarayourfriend · 2023-11-16T19:27:54Z

api/api/utils/image_proxy/__init__.py

-    is_full_size: bool = False,
-    is_compressed: bool = True,
+    media_info: ImageProxyMediaInfo,
+    proxy_config: ImageProxyConfig = ImageProxyConfig(),


This might be a bad name, maybe "image proxy request config"?

Yea I think request_proxy_config for the variable itself might be better 🤔

This does not convert the `image_proxy.get` to async to avoid making this initial PR more complicated than it needs to be, and to avoid being blocked on the aiohttp client session sharing which we will want to have in before we convert the image proxy to use aiohttp.

openverse-bot · 2023-11-21T00:00:10Z

Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR:

@krysal
@obulat
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was ready for review 2 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)².

@sarayourfriend, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Specifically, Saturday and Sunday. ↩
For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

dhruvkb

LGTM! All endpoints seem to be working normally. I did notice a lack of static files in my local server, but it could just be my setup so approving.

@sarayourfriend were you able to receive static files? I did a complete rebuild of my images and the static files were not being served.

sarayourfriend · 2023-11-21T21:17:50Z

I did notice a lack of static files in my local server, but it could just be my setup

I also had this and also assumed it was my local. Clearly a bad assumption from me, apologies. I'll have a fix up soon.

sarayourfriend · 2023-11-21T22:23:21Z

@dhruvkb Can you run just build web and try again? I've done that locally and the static files are working. Only thing I can think is that maybe one of the dependency changes caused a side-effect?

AetherUnbound

This looks good! I'm able to build and run it locally, and it appropriately serves the static files for me on localhost:50270 🚀

AetherUnbound · 2023-11-21T22:32:01Z

api/api/utils/image_proxy/__init__.py

-    is_full_size: bool = False,
-    is_compressed: bool = True,
+    media_info: ImageProxyMediaInfo,
+    proxy_config: ImageProxyConfig = ImageProxyConfig(),


Yea I think request_proxy_config for the variable itself might be better 🤔

AetherUnbound · 2023-11-21T22:37:38Z

api/api/views/media_views.py

+    thumbnail_action = action(
+        detail=True,
+        url_path="thumb",
+        url_name="thumb",
+        serializer_class=media_serializers.MediaThumbnailRequestSerializer,
+        throttle_classes=[AnonThumbnailRateThrottle, OAuth2IdThumbnailRateThrottle],
+    )


sarayourfriend · 2023-11-22T00:40:25Z

@AetherUnbound I updated the dataclass names to simplify them and make the request config role clearer.

sarayourfriend requested a review from a team as a code owner September 13, 2023 01:23

sarayourfriend requested review from krysal and obulat and removed request for a team September 13, 2023 01:23

github-actions bot added the 🧱 stack: api Related to the Django API label Sep 13, 2023

openverse-bot added the 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work label Sep 13, 2023

sarayourfriend force-pushed the add/asgi branch from e8a415c to 2d9485f Compare September 13, 2023 03:01

sarayourfriend marked this pull request as draft September 18, 2023 05:31

sarayourfriend force-pushed the add/asgi branch from 14d064c to f956f4e Compare September 21, 2023 05:59

sarayourfriend mentioned this pull request Nov 2, 2023

Django ASGI #2843

Closed

2 tasks

sarayourfriend force-pushed the add/asgi branch from f956f4e to 17802db Compare November 2, 2023 23:32

sarayourfriend force-pushed the add/async-image-proxy-route branch from 66c95b6 to dc44ee8 Compare November 3, 2023 00:22

Base automatically changed from add/asgi to main November 8, 2023 22:23

sarayourfriend force-pushed the add/async-image-proxy-route branch from dc44ee8 to f9c66bf Compare November 16, 2023 19:19

sarayourfriend marked this pull request as ready for review November 16, 2023 19:19

sarayourfriend commented Nov 16, 2023

View reviewed changes

sarayourfriend mentioned this pull request Nov 20, 2023

Production thumbnails memory leak #3353

Closed

sarayourfriend added 6 commits November 20, 2023 16:03

Fix bad merge

0ffeeec

Fix bad merge

4663dbc

Fix docstrings on api docs

7478f30

Remove unnecessary thread sensitivity

8d8e2d1

Fix bad subclassing of thumbnails endpoints

8897c78

sarayourfriend force-pushed the add/async-image-proxy-route branch from 6d61fd4 to 8897c78 Compare November 20, 2023 06:07

dhruvkb approved these changes Nov 21, 2023

View reviewed changes

AetherUnbound approved these changes Nov 21, 2023

View reviewed changes

Use simpler/clearer names for image proxy config objects

c082676

sarayourfriend merged commit b91cdc3 into main Nov 22, 2023
43 checks passed

sarayourfriend deleted the add/async-image-proxy-route branch November 22, 2023 00:47

sarayourfriend mentioned this pull request Nov 22, 2023

Convert entire image proxy route async #3388

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ADRF and make the thumbnail view async #3020

Add ADRF and make the thumbnail view async #3020

sarayourfriend commented Sep 13, 2023 •

edited

Loading

sarayourfriend Nov 16, 2023

sarayourfriend Nov 16, 2023

AetherUnbound Nov 21, 2023

openverse-bot commented Nov 21, 2023

dhruvkb left a comment •

edited

Loading

sarayourfriend commented Nov 21, 2023

sarayourfriend commented Nov 21, 2023

AetherUnbound left a comment

AetherUnbound Nov 21, 2023

AetherUnbound Nov 21, 2023

sarayourfriend commented Nov 22, 2023

		django-split-settings = "*"
		psycopg = "~=3.1"

Add ADRF and make the thumbnail view async #3020

Add ADRF and make the thumbnail view async #3020

Conversation

sarayourfriend commented Sep 13, 2023 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

sarayourfriend Nov 16, 2023

Choose a reason for hiding this comment

sarayourfriend Nov 16, 2023

Choose a reason for hiding this comment

AetherUnbound Nov 21, 2023

Choose a reason for hiding this comment

openverse-bot commented Nov 21, 2023

Footnotes

dhruvkb left a comment • edited Loading

Choose a reason for hiding this comment

sarayourfriend commented Nov 21, 2023

sarayourfriend commented Nov 21, 2023

AetherUnbound left a comment

Choose a reason for hiding this comment

AetherUnbound Nov 21, 2023

Choose a reason for hiding this comment

AetherUnbound Nov 21, 2023

Choose a reason for hiding this comment

sarayourfriend commented Nov 22, 2023

sarayourfriend commented Sep 13, 2023 •

edited

Loading

dhruvkb left a comment •

edited

Loading