Skip to content
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

Evaluate feasibility of Django ASGI and ADRF conversion #2566

Closed
zackkrida opened this issue Jul 5, 2023 · 4 comments
Closed

Evaluate feasibility of Django ASGI and ADRF conversion #2566

zackkrida opened this issue Jul 5, 2023 · 4 comments
Assignees
Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: api Related to the Django API

Comments

@zackkrida
Copy link
Member

Problem

Openverse needs to make a determination about how to best handle thumbnail traffic, as discussed here:

#2498

Converting the entire API to work asynchronously is a compelling choice, as asynchronicity benefits all parts of the API. However, we are uncertain about the compatibility of switching to ASGI and ADRF with the existing codebase.

Description

A contributor should run a timeboxed (perhaps 4 development hours for initial experimentation, with an additional 4 if necessary to close the issue) in an attempt to determine if this conversion is possible.

Alternatives

Additional context

@zackkrida zackkrida added 🟨 priority: medium Not blocking but should be addressed soon 🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users python 🧱 stack: api Related to the Django API labels Jul 5, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Openverse Backlog Jul 5, 2023
@zackkrida zackkrida changed the title Evaluate ease of Djanjo ASGI and ADRF conversion Evaluate feasibility of Djanjo ASGI and ADRF conversion Jul 5, 2023
@zackkrida zackkrida moved this from 📋 Backlog to 📅 To do in Openverse Backlog Jul 5, 2023
@sarayourfriend
Copy link
Collaborator

I've tried this before in the past, when adrf did not exist, so I want to share some advice for whomever takes this on, if it isn't me:

  1. It might be easier to split the thumbnails into their own view and only declare that view to be async. I don't believe you can have mixed views, where some entry points are async and others not (because as_view() applies to all of them), but we can have entire views that are async and some that aren't. So, it might be possible to scope the changes to just the thumbnails route by splitting it into its own view in the route configuration rather than an additional action on the media views.
  2. If that isn't possible, you might be able to use sync_to_async to avoid needing to convert functions called by views (like search_controller or utilities etc) to async. Something like:
await sync_to_async(search_controller.search)(*args, **kwargs)

Would potentially help scope the changes just to the entry point rather than the entire downstream code of the views.

Keep in mind that there are only 2 places we need to switch to asynchronous calls to solve the issue of workers blocked on thumbnail requests: the head request for the file type inference and the get request to Photon/upstream image URL (in the case of SVGs). Our Redis usage is safe from race conditions/data integrity issues that could be caused by sync code in an async context and our database usage is limited to reads. As long as the request to the thumbnails is async and the app is running under uvicorn as an ASGI app, then we'll have succeeded in converting the app to the extent necessary to solve the problem with thumbnails blocking workers for too long.

@sarayourfriend sarayourfriend moved this from 📅 To do to 🏗 In progress in Openverse Backlog Jul 17, 2023
@sarayourfriend sarayourfriend mentioned this issue Jul 17, 2023
7 tasks
@sarayourfriend
Copy link
Collaborator

This PR demonstrates the feasibility of the ASGI Django approach.

@AetherUnbound Can you clarify what the next steps are for this? Is a single new issue to actually implement the changes sufficient or should we try to break the work up into different chunks? Up front, these are the overall steps I would suggest we take to convert the app to ASGI, including up-front work that makes downstream issues easier and smaller:

  • Embed the production run command in the dockerfile rather than relying on ECS to override it. Use docker-compose to configure the development run command (currently runserver).
  • Remove the WSGI Basic Auth middleware that is not currently used
  • Remove gevent
  • Use a shared aiohttp.ClientSession rather than creating a new one per-request (relevant now to the check_dead_links function)
    • Not sure if we can do this before the ASGI conversion, actually, only because there's no easy way to close the client session before the app shuts down. WSGI doesn't have lifecycle hooks like ASGI and Django itself doesn't have anything. Not sure if there's something we can hook into on the Python process level or something. Maybe something implemented in a Gunicorn worker subclass?
  • Convert the image proxy route to an async view running under WSGI. No immediate benefits here as the worker will still block for the entire request. But it gets this part of the implementation out of the way to reduce the scope of a subsequent PR that converts the app to ASGI.
  • Convert the app to ASGI.

The first four can be completed concurrently. The last two must be completed in order and should ideally only be started after the first four are complete.

Do you think these issues are enough for us to move forward or should we write a more fleshed out implementation plan describing the changes for each one? If we want to start on this soon then someone else will need to convert the tasks to issues unless we can wait until after my holiday.

@dhruvkb dhruvkb changed the title Evaluate feasibility of Djanjo ASGI and ADRF conversion Evaluate feasibility of Django ASGI and ADRF conversion Jul 25, 2023
@AetherUnbound
Copy link
Collaborator

This outline looks excellent, thanks for solidifying it into concrete steps! I think the best plan would be to make separate issues for it, for each of the individual pieces you describe as part of the conversion. Having separate issues will help make scoping out the work amongst our other TODOs a little easier. I think given the maintainers' upcoming support engagement and our existing TODOs, the issue creation can wait until after you're back!

@sarayourfriend
Copy link
Collaborator

Issues are created! https://github.com/WordPress/openverse/milestone/19

Closing this issue as completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: api Related to the Django API
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants