-
Notifications
You must be signed in to change notification settings - Fork 213
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
Make miscellaneous improvements to the API developer experience #4387
Conversation
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.
Our usage of uvicorn reload instead of Django's development mode causes some of Django's normal dev-mode features not to work by default, I guess this is one of them. Did you happen to try configuring uvicorn's reload watcher pattern instead of changing Django's template caching behaviour?
https://www.uvicorn.org/settings/#reloading-with-watchfiles
It'd be nice to actually keep template caching locally when the templates aren't changing, so that at least we would theoretically see an issue if template caching caused an issue when it ran. If we tell uvicorn to also reload when templates change, then the Django configuration wouldn't need to change between prod and local. This seems to work for me locally:
diff --git a/api/run.py b/api/run.py
index 0265ad680..521625007 100644
--- a/api/run.py
+++ b/api/run.py
@@ -18,6 +18,8 @@ if __name__ == "__main__":
".", # default, API directory
"../packages/python/",
],
+ # *.py is default, add *.html so template changes also cause reloads
+ reload_includes=["*.py", "*.html"],
log_level="debug",
access_log=False,
)
That should let us avoid needing to mess with the template loader configuration at all.
Can you confirm whether you've tried this, and if it doesn't work can you document in the PR why? This LGTM to me otherwise, and happy to approve it, but wanted to make sure there wasn't a potentially simpler solution that would also avoid introducing a meaningful difference in the way templates are processed locally vs in production.
This PR is exactly what Django used to do before version 4.1. In subsequent versions, they enabled caching of templates locally, and this PR just reverses their change. We could configure Uvicorn to track HTML files and reload if they've changed but I didn't even try that because this worked. I'll try out the Uvicorn change you have suggested. |
Good to know! In that case, the difference between local and production I suppose we don't need to worry about (though they changed it for some reason, I guess?). If the uvicorn settings approach is simpler, then great, otherwise this works too 👍 |
This PR has migrations. Please rebase it before merging to ensure that conflicting migrations are not introduced. |
The change to reload Uvicorn when templates works well and is much more minimal compared to changing the loaders, so I've updated the PR. One thing to note is that disabling caching by changing the loaders (the previous implementation of this PR) was slightly faster during development as it did not require a full reload of Uvicorn. It would continue the Python process and merely read the latest contents of the template file from disk each time. |
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.
Nice improvements, thank you for finding them!
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.
LGTM!
Description
This PR
While the addition of the verbose names does lead to a new migration, the migration is a no-op in SQL.
SQL
Checklist
Update index.md
).main
) or a parent feature branch.just catalog/generate-docs
for catalogPRs) or the media properties generator (
just catalog/generate-docs media-props
for the catalog or
just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin