-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add UV for project management, Ruff for formatting. #1391
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
base: master
Are you sure you want to change the base?
Conversation
c6fbdf7 to
1952c6c
Compare
|
So, turns out we're hitting a memory limit on the github action when compiling the docker images, so I am going to figure out what to do there. |
d5c573b to
e407e93
Compare
|
Weirdly this job hung forever waiting for something to happen. |
|
I think perhaps the |
bb9897a to
a9d9cb6
Compare
|
hi! Give me some time to take a look into this. I wasn't expeting a overhaul. I'll have to look into these new tools and confirm they are become the standard. Regarding the GH Action, I'll try to look into it. The k8s job probably is failing because you didn't update the k8s configuration files. I'll take a look. Secondly, the Postgres job is useful for, as you said, testing (and documenting) how to dump and load the database. I think every functionality of PokeAPI should be tested and having some test files that document how to actually do something is the best. |
|
I've not set up and used k8s myself before so testing and debugging is slow going. RE: Postgres job. Does it have to be done via docker? If so, I think porting it to circle CI so I can provision a bigger runner is the best course? It keep hitting memory limits. GitHub runner options are limited. |
|
No need to run it on Docker |
|
Postgres job rewritten to use bare metal instead of docker, now works :) |
Context: - Far more modern, easier environment management - I have tested all makefile commands work
Context: - This commit includes automatic formatting additions
Context: - This is a big change, but it is functionality identical - Explicit imports for some files make the diff very large - I set line length to 120 which is standard in Python - Some files used tabs instead of spaces (!) - now fixed
Context: - We needed to explicilty include psycopg-binary here - Rest of the changes are just replacements of python -> uv run
Context: - With the Docker config we hit a memory limit in Github actions - This alternative approach uses "bare metal" and still works
|
@Naramsim All tests passing now. Reorganised the commits to be distinct. Best reviewed commit by commit. |
|
|
||
| dev-install: # Install developer requirements + base requirements | ||
| $(PIP) install -r test-requirements.txt | ||
| uv sync |
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'd like a target that checks if uv is installed and having it running before the other targets that requires it.
check-uv:
command -v uv &>/dev/null || { echo
'`uv` not detected'; }
setup: check-uv # Set up the project database
uv run manage.py migrate ${local_config}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.
That's a good idea!
|
|
||
| docker-migrate: # (Docker) Run any pending migrations | ||
| docker compose exec -T app python manage.py migrate ${docker_config} | ||
| docker compose --verbose exec -T app uv run manage.py migrate ${docker_config} |
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.
We can remove the --verbose
| @@ -0,0 +1,35 @@ | |||
| [project] | |||
| name = "pokeapi" | |||
| version = "0.1.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.
Let's go with 3.0.0. When this will be merged I'll draft a new release.
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.
Can do but it's not necessary - this is if we are publishing the project as a package, which we're not.
| version = "0.1.0" | ||
| description = "The Pokemon API" | ||
| readme = "README.md" | ||
| requires-python = ">=3.14" |
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.
Is it true? Can't python 3.10 be used?
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.
Can be, I'd prefer to be on the latest version - uv will "just work" for this.
| "drf-spectacular==0.29.0", | ||
| "gunicorn==23.0.0", | ||
| "psycopg==3.3.2", | ||
| "psycopg2-binary>=2.9.11", |
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.
We don't use psycopg2 anymore. Can you try removing it?
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.
OO this might be something I missed, good spot.
| ] | ||
|
|
||
| [tool.ruff] | ||
| line-length = 120 |
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.
Honestly I don't like having lines with a maximum length. Nowadays all editors split and wrap the lines automatically. I'd remove the property or set it to a greater value.
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.
80-90 characters is pretty standard in the python world. This is extending it from that default to 120 (which, I'd prefer to keep the default for readability, but 120 isn't the worst)
editor-wrapped lines are quite hard to read.
| - Download this source code into a working directory, be sure to use the flag `--recurse-submodules` to clone also our submodules. | ||
|
|
||
| - Install the requirements using pip: | ||
| - Install [uv](https://docs.astral.sh/uv/getting-started/installation/) for Python environment management. |
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 don't paricularily like adding new toold to projects. I don't see the benefits of using uv only the downside of forcing everyone to install it on their machines. It's true that it's a project with many stars and maybe a good community but for pokeapi I don't see any added benefits. If you @phalt or other contributors strongly advocate for using it, I'm ok with it, but if I could decide I wouldn't add it.
My argument is: Python officially already ships with uv-like features. It has venv, can install a pyprojet.toml, etc. uv does it faster from my understanding.
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.
One very good added benefit of uv is having a lock file.
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.
re: "forcing everyone to install it on their machines"
I'd recommend actually configuring the core tools via mise. That way, any contributor only needs mise, and any other tools (uv, python, docker, anything) is managed + locked by mise in local dev, in CI, in devcontainers, AI agent workspaces, everywhere in a reproducible way.
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.
Uv is becoming an industry standard, I use it at my company (3,000+ python engineers, one of the biggest Django projects in the world, we even have a few Python and Django core devs here).
Why uv?
- It manages python versions you don't need to install and manage multiple versions yourself.
- It wraps
virtualenvand eliminates the need to understand it, reduces barrier to entry. - It has lock files to enforce specific versions - previously pip didn't fully support this.
- It helps you run tools easily (in a future PR I am going to add
uv run pokeapi build_datacommand to eliminate all the manualimport data; data.buildstuff we have). - It is fast.
For years PokeAPI has been a good example of a great API project, and it has lagged behind in being a great example of a Python project, and it could be one with a bit of TLC - which is what my aim is this year.
I think providing up to date tools is a win win for us (easier to maintain) and also people browsing our code (they see what is standard).
| USER pokeapi | ||
|
|
||
| CMD ["gunicorn", "config.wsgi:application", "-c", "gunicorn.conf.py"] | ||
| CMD ["uv", "run", "gunicorn", "config.wsgi:application", "-c", "gunicorn.conf.py"] |
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.
Is uv also necessary for running the app? Why can't I just use gunicorn?
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.
This way it'll use the venv to run, without any activate step
| ENV PATH="/root/.local/bin:$PATH" | ||
|
|
||
| # Install runtime dependencies | ||
| RUN apk add --no-cache postgresql-libs libstdc++ |
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.
Are we sure we need this two packages? We didn't need them before. Only for building the app at line 9
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.
Will try and drop them and see if it works
| from rest_framework import status | ||
| from rest_framework.test import APITestCase | ||
| from pokemon_v2.models import * | ||
|
|
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.
Is it bad to import *?
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.
Yes, it is implicit, and not recommended in Python.
|
Can I chime in @sargunv @cmmartti @Indyandie especially for whether to adopt the astral-sh ecosystem or not. |
|
Yup, can confirm these are the new standard and for good reason. I use them all at work and they're great. |
This is the first part of #1390
UV package manager
uv is fast becoming the standard for managing an entire Python project. It handles package management, environment management, tool management etc. It's great.
The first commit introduces it. We also add
pyproject.tomlwhich is the new project dependency standard by Python. It replacesrequirements.txtand other configurations.Ruff auto formatting
Ruff is a Python code formatter. It is super fast, and like
uvbecoming the standard.The second commit is the "auto" formatted changes - there were very few.
Ruff manual formatting
This is the big commit. I had to manually go and update all the poorly formatted code in the entire project. Read the commit to see the summary of the changes.
Docker changes
I'm not the best at Docker, but I finally got it working. Changes are mostly around using
uvproperly.postgres job
This job used to run in Docker. For some reason it now hits memory limits. I've decided to just shift this to use "bare metal" and achieve the same result.
AI support
For the very very large files, I made 3-4 changes, then asked a coding agent to "copy the changes I did for the rest of the file to fix the import errors". Then I verified it by manually reading the diff
Proof it works
make build-allworks still