-
Notifications
You must be signed in to change notification settings - Fork 204
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
Update python version in API and attribution #5095
Conversation
1942168
to
9cff8cd
Compare
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.
A few notes before we consider merging this!
api/pyproject.toml
Outdated
"django-uuslug >=2.0.0, <3", | ||
"djangorestframework >=3.14.0, <4", | ||
"drf-spectacular >=0.27.1, <0.28", | ||
"elasticsearch==8.13.0", |
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.
It looks like this package and pook
were the only ones that were changed in this list, and both were pinned. Is there a specific version that should be set as the upper bound instead so these can still receive patch updates?
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.
Thank you for catching these! Initially, pdm automatically updated more packages, but some of the tests with pook were failing, so I reverted most unnecessary changes manually, and some reversals resulted in pinned versions :(
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.
@obulat if you noticed any issues with Pook, do you mind opening an issue upstream so I can fix it?
api/pyproject.toml
Outdated
"uvicorn[standard] >=0.30, <0.31", | ||
"openverse-attribution @ file:///${PROJECT_ROOT}/../packages/python/openverse-attribution", | ||
"structlog-sentry>=2.1.0", | ||
"pook==2.0.0", |
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.
Shouldn't this be in the dev-dependencies
section? Is it needed for core application code?
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.
Removed in c9a8fcb
I was going to suggest looking at going straight to 3.13 (it is GA since last week), but Django 5.1 is required before that's possible (so #4015). |
6d90269
to
c9a8fcb
Compare
Great to know. We'll go step by step here :) Some changes in this PR's initial commits caused test failures due to pook mocks not working as expected, so we might have to debug while updating these packages. |
c9a8fcb
to
a625836
Compare
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.
I recreated the image and everything seems to be working correctly 👍
env COMPOSE_PROFILES="api" just up --force-recreate --build
just catalog/up && just api/init
Fixes
Fixes #4940 by @sarayourfriend
Description
Updates python version to 3.12 in the API and python/openverse-attribution. Some packages also had to be updated to work with the new version of Python.
Testing Instructions
The CI should pass, and the API should work as usual
Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin