-
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
Downgrade the API's python version back to 3.11 #5295
base: main
Are you sure you want to change the base?
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.
Good catch, the slope of the memory consumption definitely seems higher on the right side of the Python version bump (although it didn't get to as high of a peak as it does later on). I can get behind your reasoning for trying this out.
That said, this PR still updates a number of packages (even major versions in some cases). I think those will make it harder to pinpoint if it really was the Python version upgrade that caused the leak, and if it really was the Python version downgrade that fixed it.
In this PR, there are
- patch upgrade in some packages:
aiohappyeyeballs
,aiosignal
- major upgrade in some packages:
anyio
,asttokens
- introduction of some new packages:
async-timeout
Can we try downgrading the Python version, without changing any of the packages? I think PDM's --update-reuse
flag will avoid changes to the versions in the lock file wherever possible.
pdm lock --update-reuse
Good call! That was the reason I was removing Edit: Regarding |
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @obulat Excluding weekend1 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)2. @krysal, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
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 looked through both the lock files and it seems everything is good there. None of the IO package pins are changing so we'll be able to determine if the Python version itself was the cause of the trouble.
The attribution package has a number of updates to its packages but those should not be blocking because they're not related to IO and cannot be associated with the memory leak.
Description
This PR downgrades the Python version to the previous one, 3.11.*, partially reverting #5095, to see if the API memory utilization slows down. Ideally, it should return to acceleration levels previous to release from November 4th (below 50% after 2 weeks).
The highest peak on the graph coincides with the update of aiohttp (#5165) but we wanted that to fix #4901. It may have been drived by the Django v5 upgrade too, see release from 2024.11.17. So it's easier to test the Python version downgrade first. If that doesn't alleviate the problem, we can go back to version 3.12 (or even further, bump to 3.13) and revert the aiohttp update.
I restricted the versions allowed in some API packages to avoid doing many updates of other dependencies here, and also had to loosen the required python version of the
openverse-attribution
package to be able to downgrade the API.Testing Instructions
Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalog PRs) or the media properties generator (ov just catalog/generate-docs media-props
for the catalog orov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin