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

Migrate to UV #2282

Merged
merged 29 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a7a6621
uv spike
chris48s Sep 26, 2024
7d853ad
set requires-python to python 3.12.x
chris48s Oct 16, 2024
915e49e
allow prereleases in config file
chris48s Oct 16, 2024
7528927
we don't use black anymore
chris48s Oct 16, 2024
c9972e5
project metadata
chris48s Oct 16, 2024
73ae578
bump dc_django_utils
chris48s Oct 16, 2024
c7e9d0c
bump django
chris48s Oct 16, 2024
8a69cf2
whitespace
chris48s Oct 16, 2024
6d5d0d1
remove unnecessary package declarations
chris48s Oct 16, 2024
f554139
move debug toolbar to dev-dependencies, remove 'local' optional group
chris48s Oct 16, 2024
eefd6f3
delete old requirements files
chris48s Oct 16, 2024
1b0e3a6
update install docs
chris48s Oct 16, 2024
7ad181a
delete mock package
chris48s Oct 17, 2024
4022a60
safety: upgrade gunicorn
chris48s Oct 23, 2024
a7d51c9
safety: remove packages we don't use anymore, extend jinja2 ignore
chris48s Oct 23, 2024
d847929
install pyyaml in cdk group
chris48s Oct 23, 2024
315ee6b
CI build
chris48s Oct 23, 2024
1e57ca1
move uv install to script
chris48s Oct 23, 2024
c85d8a3
install packages using uv in deploy, move gevent to prod optional group
chris48s Oct 23, 2024
ea89bad
install uv in the global env with pip instead of curl bashing
chris48s Oct 23, 2024
d32a8a0
use normalized package names
chris48s Oct 23, 2024
32f302a
set up a sensible-ish dependabot config
chris48s Oct 23, 2024
69c7155
add a workflow to re-generate uv.lock on dependabot PRs
chris48s Oct 23, 2024
f43426b
bump versions
chris48s Oct 23, 2024
c5b78be
uv 0.4.27, dependency-groups
chris48s Oct 31, 2024
f3d3075
add uv cache dir to circle cache
chris48s Oct 31, 2024
f2138a2
install and run coveralls in one-liner
chris48s Oct 31, 2024
bd91e5c
remove pyproject.toml hash from cache key
chris48s Oct 31, 2024
165b0fd
tell uv to only use the python we specify in circle image
chris48s Nov 4, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 16 additions & 23 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:
node-version: '18'

- restore_cache:
key: v3-dependencies-{{ checksum "requirements/base.txt" }}-{{ checksum "requirements/testing.txt" }}-{{ checksum "requirements/cdk.txt" }}
key: v3-dependencies-{{ checksum "pyproject.toml" }}-{{ checksum "uv.lock" }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. I know we're pinning exact versions in pyproject.toml now, but wouldn't the lock file be updated in this case too? What's your thinking in using both files for the cache key here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good question. I went back and forth on this in my own head.

My thinking on this was that because dependabot can't bump uv.lock it will try to push commits that change pyproject.toml but don't change the lockfile this might be sensible. Maybe it is irrelevant though. uv lock --locked should ensure we just fail the build early doors on those commits anyway.

I've removed it in 46d6de1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the only other consideration here is the python version. At the moment we tend to pump the v[n] prefix when we bump python versions, but including pyproject.toml would mean we bust the cash a lot more often. Not a big deal, I think what you currently have is reasonable 👍


- run:
name: Install node node_modules
Expand All @@ -35,22 +35,15 @@ jobs:
name: Install app dependencies
command: |
sudo apt update && sudo apt install -y gdal-bin python3-gdal python3-dev
pyenv local $(ls ~/.pyenv/versions/)
python -m venv .venv
deployscripts/install_uv.sh
uv venv --python 3.12
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

  1. This will download and install the python version if it's not found on the system. Neat locally, but not what we want in CI (easy to miss that UV is doing this for us!). Can you add --no-python-downloads or export UV_PYTHON_DOWNLOADS=never?
  2. Do we actually need to specify the python version here, if we have it in pyproject.toml / .python-version? And we're already using the cimg for the same version?

Copy link
Member Author

@chris48s chris48s Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we actually have opposite objectives here.

My thinking here was: Previously we were doing all sorts of pyenv faffing to make sure we were using the right global python version:

I did at one point work out what we were trying to do there, but I've forgotten again.

My thinking here is: Lets bin all that and invoke uv venv --python 3.12. If uv can use a version of python that is already installed, cool. If it needs to go and download one, also fine. Either way, we're running the right version in our venv. That is one of the core value propositions of UV imo. Tbh we could even consider switching the base image for the build to one that doesn't have python on it at all.

I don't mind trying what you suggested, but also I'd like to understand what advantage that gives us, or what problem what I've suggested causes.

Happy to just talk about this one on a call or whatever.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically want to ensure we don't drift between [Circle]CI image version and the Python version in the venv.

At the moment the build isn't going to fail if we use cimg/python3.8 and uv venv --python 3.12, but it will download a python (maybe for every CI run, depending on caching). If someone doesn't know this they might see the image version and assume that's what we're running. When we move to Python3.13 we might just bump the image, etc, etc.

I would like to be in a position where we either only specify the python version in one place (uv) or where we prevent uv from installing a version of Python that's not installed.

The pyenv faffing was due to Pyenv being too picky. If 3.12 is in python-version but you have 3.12.4 installed, it complains. You can't (or couldn't at the time) specify 3.12.* or anything like that. I don't think uv has this "feature" so it's moot here.

. .venv/bin/activate
pip install --upgrade pip
pip install coveralls wheel
pip install -r requirements/testing.txt
uv sync --all-extras
- run:
name: Install Playwright
command: |
. .venv/bin/activate
playwright install
- run:
name: Install CDK Python dependencies
command: |
. .venv/bin/activate
pip install -r requirements/cdk.txt
- run:
name: Pip safety check
command: |
Expand All @@ -60,7 +53,7 @@ jobs:
paths:
- ./.venv
- ./node_modules
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding $HOME/.cache/uv here will mean that uv doesn't need to re-download packages between runs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 9d49161

key: v3-dependencies-{{ checksum "requirements/base.txt" }}-{{ checksum "requirements/testing.txt" }}-{{ checksum "requirements/cdk.txt" }}
key: v3-dependencies-{{ checksum "pyproject.toml" }}-{{ checksum "uv.lock" }}

- run:
name: Pre-test checks
Expand All @@ -70,7 +63,8 @@ jobs:
python manage.py --version
python manage.py check
python manage.py makemigrations --check
pip check
uv lock --locked
chris48s marked this conversation as resolved.
Show resolved Hide resolved
uv pip check
ruff format . --check
ruff check .
git ls-files '*.html' | xargs djhtml --check
Expand All @@ -85,6 +79,7 @@ jobs:
name: Submit coverage
command: |
. .venv/bin/activate
uv pip install coveralls
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered moving this to a package in our manifest, but I can't really be bothered. I'm not sure how to meaningfully review a bump, for example.
Lets just stick with installing the latest version in CI and deal with it if it breaks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be collapsed into one line with uv run --with coveralls coverage

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well uv run --with coveralls coveralls worked, but first I copied and pasted what you said and then spent ages being confused about why the coverage report wasn't submitting :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coveralls

- store_artifacts:
Expand All @@ -107,20 +102,20 @@ jobs:
steps:
- checkout
- restore_cache:
key: v3-machine-dependencies-{{ checksum "requirements/base.txt" }}-{{ checksum "requirements/testing.txt" }}-{{ checksum "requirements/cdk.txt" }}
key: v3-machine-dependencies-{{ checksum "pyproject.toml" }}-{{ checksum "uv.lock" }}
- run:
name: Install CDK Python dependencies
command: |
pyenv local $(ls -1 /opt/circleci/.pyenv/versions | grep 3.12)
python -m venv .venv
deployscripts/install_uv.sh
uv venv --python 3.12
. .venv/bin/activate
pip install -r requirements/cdk.txt
uv sync --extra cdk --no-dev
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, uv sync --extra cdk --no-dev here will install all the main packages as well as the cdk extras.
Nothing bad will happen. We're just installing more stuff than we really need to.

Upstream feature request: astral-sh/uv#8292

Copy link
Member Author

@chris48s chris48s Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: We should now be able to upgrade to UV 0.4.27 which adds the --only-group flag and fix this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so there's a lot going on in c5b78be .

As of UV 0.4.27
https://github.com/astral-sh/uv/releases/tag/0.4.27

UV now has dependency-groups: https://docs.astral.sh/uv/concepts/dependencies/#dependency-groups
This is a distinct feature from optional-dependencies: https://docs.astral.sh/uv/concepts/dependencies/#optional-dependencies

..although there is obviously a lot of overlap between these two concepts.
I think if you're using uv as an application package manager, they basically just reduce to 2 overlapping ways to accomplish almost the same thing.
If you're using uv to build packages, optional-dependencies allow you to define optional groups that are "public" to your user so you can pip install package[optional], whereas dependency-groups are "private".

Switching from optional-dependencies to dependency-groups means we can now use the --only-group flag to just install CDK dependencies, which is good. There's no equivilent of the --all-extras flag though, so if we want everything we have to either do uv sync --group cdk --group production or add everything to our default groups and exclude the ones we don't want when we don't want them. I think what I've done here is probably reasonable.

The dev-dependencies feature that was current when I wrote this PR ~a week ago is now "legacy"
https://docs.astral.sh/uv/concepts/dependencies/#legacy-dev-dependencies

I think this is a pretty good microcosm of what we're getting involved in here. Both in terms of the rough and the smooth (or the ruff and the smooth, if you prefer :badpundog: ).

On the one hand, uv is moving quickly 🚀 A feature we wanted ended up in a release within a week or so and it was already something they were looking at before they triaged the issue.
On the other hand, uv is moving quickly 😱 A feature that was current a week ago is now considered legacy as of a patch release and there are several overlapping ways to do quite similar things.

I suspect this will all shake out by the time they're offering any sort of stability but I reckon there will be some more churn between now and then. I will also note that this whole thing is basically something poetry went through over a longer timeframe before they really got it right.

..anyhooo, in c5b78be I have

  • said we need uv>=0.4.27
  • converted our optional-dependencies and dev-dependencies
  • updated the relevant uv sync commands


- save_cache:
paths:
- ./.venv
- ./node_modules
key: v3-machine-dependencies-{{ checksum "requirements/base.txt" }}-{{ checksum "requirements/testing.txt" }}-{{ checksum "requirements/cdk.txt" }}
key: v3-machine-dependencies-{{ checksum "pyproject.toml" }}-{{ checksum "uv.lock" }}

- run:
name: CDK version
Expand Down Expand Up @@ -156,17 +151,16 @@ jobs:
steps:
- checkout
- restore_cache:
key: v3-machine-dependencies-{{ checksum "requirements/base.txt" }}-{{ checksum "requirements/testing.txt" }}-{{ checksum "requirements/cdk.txt" }}
key: v3-machine-dependencies-{{ checksum "pyproject.toml" }}-{{ checksum "uv.lock" }}
- run:
name: CDK deploy
command: |
pyenv local $(ls -1 /opt/circleci/.pyenv/versions | grep 3.12)
. .venv/bin/activate
npx cdk deploy --all --require-approval never --concurrency 3
- save_cache:
paths:
- .cdk.out
key: v3-machine-dependencies-{{ checksum "requirements/base.txt" }}-{{ checksum "requirements/testing.txt" }}-{{ checksum "requirements/cdk.txt" }}
key: v3-machine-dependencies-{{ checksum "pyproject.toml" }}-{{ checksum "uv.lock" }}
- slack/notify:
event: fail
template: basic_fail_1
Expand All @@ -192,11 +186,10 @@ jobs:
- node/install:
node-version: '18'
- restore_cache:
key: v3-dependencies-{{ checksum "requirements/base.txt" }}-{{ checksum "requirements/testing.txt" }}-{{ checksum "requirements/cdk.txt" }}
key: v3-dependencies-{{ checksum "pyproject.toml" }}-{{ checksum "uv.lock" }}
- run:
name: "Code Deploy: Create deployment group"
command: |
pyenv local $(ls ~/.pyenv/versions/)
. .venv/bin/activate
python deployscripts/create_deployment_group.py
- run:
Expand Down
22 changes: 14 additions & 8 deletions .github/dependabot.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,20 @@ updates:
- package-ecosystem: pip
directory: "/"
schedule:
interval: daily
open-pull-requests-limit: 10
interval: "daily"
groups:
cdk-libs:
patterns:
- "aws-cdk*"
- "constructs"
boto-libs:
patterns:
- "boto*"
chris48s marked this conversation as resolved.
Show resolved Hide resolved
ignore:
- dependency-name: boto3
- dependency-name: "boto*"
update-types: ["version-update:semver-patch"]
- dependency-name: botocore
update-types: ["version-update:semver-patch"]
- dependency-name: django
- dependency-name: "django"
versions:
- ">= 3.a"
- "< 4.2"
# We don't want notifications about Django 5
# until 5.2 LTS is released in 2025
- ">= 5.0"
29 changes: 29 additions & 0 deletions .github/workflows/uv-lock.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
name: UV Lock
chris48s marked this conversation as resolved.
Show resolved Hide resolved

on:
push:
branches:
- 'dependabot/**'

permissions:
contents: write

jobs:
build:
runs-on: ubuntu-latest

steps:
- name: Checkout code
uses: actions/checkout@v3

- uses: actions/setup-python@v5
with:
python-version: '3.12'

- run: ./deployscripts/install_uv.sh

- run: uv lock

- uses: stefanzweifel/git-auto-commit-action@v5
with:
commit_message: Update uv.lock
8 changes: 1 addition & 7 deletions .safety-policy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,7 @@ security: # configuration for the `safety check` command
65213: # vulnerability in the POLY1305 MAC algorithm on PowerPC CPUs
reason: we do not use the vulnerable function
expires: '2025-01-20' # Bump if still applies
66742: # Don't do stupid things with Black. We don't. Remove when we go to ruff
reason: we do not use loads of leading tabs
expires: '2025-01-20' # Bump if still applies
67599: # Apparently all versions of pip are bad.
reason: We need to be able to pip install things.
expires: '2024-08-20' # Bump if still applies
70612: # jinja2 version 3.1.4
reason: Only used in tests, no fixed version available, maintainer disputes it is a vuln
expires: '2024-10-20' # Bump if still applies
expires: '2025-01-20' # Bump if still applies
continue-on-vulnerability-error: False # Suppress non-zero exit codes when vulnerabilities are found. Enable this in pipelines and CI/CD processes if you want to pass builds that have vulnerabilities
8 changes: 5 additions & 3 deletions INSTALL.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ EveryElection requires Python 3.x and Postgres.

## Install Python dependencies

Every Election uses [uv](https://docs.astral.sh/uv/) to manage python packages.
chris48s marked this conversation as resolved.
Show resolved Hide resolved
[Install uv](https://docs.astral.sh/uv/getting-started/installation/) first if you don't already have it. Then

```
pip install -U pip
pip install -r requirements.txt
pip install -r requirements/local.txt
uv sync
```

Gotcha: If you're having trouble installing psycopg2-binary on Apple silicon, set your library path before retrying the install:
```commandline
export LIBRARY_PATH=$LIBRARY_PATH:/opt/homebrew/opt/openssl/lib
Expand Down
3 changes: 3 additions & 0 deletions appspec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ hooks:
# During the AfterInstall deployment lifecycle event, run the commands
# in the script specified in "location".
AfterInstall:
- location: deployscripts/install_uv.sh
timeout: 300
runas: root
- location: deployscripts/install_python_deps.sh
timeout: 300
runas: every_election
Expand Down
2 changes: 1 addition & 1 deletion cdk_settings.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"base_ami_id": "ami-0c40c7ef04b5ab961",
"recipe_version": "0.0.35"
"recipe_version": "0.0.41"
chris48s marked this conversation as resolved.
Show resolved Hide resolved
}
2 changes: 1 addition & 1 deletion cdk_stacks/components/add_user.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: AddUser
description: Adds a system user
schemaVersion: 1.0
component_version: 0.0.4
component_version: 0.0.5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What changed here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing.

When I pushed stuff to the dev AWS account to test it, the image builder threw errors saying there was already a different 0.0.4 of cdk_stacks/components/add_user.yml and already a different 0.0.2 of cdk_stacks/components/instance_connect.yml, so that's why I've bumped those two despite there being no other changes in those files.

I don't really know what a good pattern is for managing this stuff, but given I don't think there's really any downside to bumping a version even if nothing changed, keeping them aligned across AWS accounts seems sensible to me.

parameters:
- username:
type: string
Expand Down
9 changes: 4 additions & 5 deletions cdk_stacks/components/install_app.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: InstallApp
description: Installs the EE Django app
schemaVersion: 1.0
component_version: 0.0.22
component_version: 0.0.27
parameters:
- git_branch:
type: string
Expand Down Expand Up @@ -43,12 +43,11 @@ phases:
- git checkout {{ git_branch }}
- git pull --rebase origin {{ git_branch }}
- git branch
- python3 -m venv .venv
- ./deployscripts/install_uv.sh
chris48s marked this conversation as resolved.
Show resolved Hide resolved
- uv venv
- sudo chown -R {{ username }} /var/www/{{ username }}
- . .venv/bin/activate
- pip install -U pip
- pip install -r requirements/production.txt
- pip install gevent
- uv sync --extra production --no-dev
- npm ci
- name: CreateEnvWriterFile
action: CreateFile
Expand Down
2 changes: 1 addition & 1 deletion cdk_stacks/components/instance_connect.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: InstallInstanceConnect
description: Installs AWS EC2 instance Connect
schemaVersion: 1.0
component_version: 0.0.2
component_version: 0.0.3
phases:
- name: build
steps:
Expand Down
6 changes: 2 additions & 4 deletions deployscripts/install_python_deps.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ set -xeE

# should we delete the env and recreate?
cd /var/www/every_election/repo/
python3 -m venv .venv
uv venv
. .venv/bin/activate
pip install -U pip
pip install -r requirements/production.txt
pip install gevent
uv sync --extra production --no-dev
10 changes: 10 additions & 0 deletions deployscripts/install_uv.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/usr/bin/env bash
set -xeE

UV_CONSTRAINT=">=0.4.0,<0.5.0"

if [ "$CI" = "true" ]; then
pip install uv"$UV_CONSTRAINT"
else
sudo PIP_BREAK_SYSTEM_PACKAGES=1 pip install uv"$UV_CONSTRAINT"
fi
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of things to talk about in this file:

  1. Based on my experience with ruff, I think we probably don't want to just always install the latest uv, at least at this stage. Pinning a constraint and accepting we're going to need to bump it is probably a better call while this is still zero-versioned and offers no stability guarantees IMO. >=0.4.0,<0.5.0 seems pragmatic at this stage. Simultaneously, we'll probably want to bump it every so often. That's one of the costs we absorb of being early adopters. I abstracted this into this slightly clumsy script which I am calling in lots of places so that we've only got one place we have to bump the constraint. This is the reason why I'm not using the astral-sh/setup-uv action in .github/workflows/uv-lock.yml.

  2. One of the problems I was trying to solve here is that on the EC2 instances I wanted to install uv in such a way that regardless of whether we aws ec2-instance-connect ssh --os-user every_election or aws ec2-instance-connect ssh --os-user ubuntu, we've got uv installed and on the path. The simplest way to do that was smoosh it into the global python env with pip, rather than curl-bash it. There's more detail on this in the commit message for ea89bad

2 changes: 1 addition & 1 deletion every_election/apps/elections/tests/test_admin.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from unittest import mock
from unittest.mock import MagicMock

import mock
from django.test import TestCase
from elections import admin
from elections.models import ModerationHistory
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from io import StringIO
from unittest import mock

import mock
from django.test import TestCase
from elections.management.commands import attach_posts_per_election_from_csv
from elections.models import Election
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from unittest import mock

from data_provider import base_data
from django.test import TestCase
from mock import mock
from organisations.boundaries.boundary_bot.scraper import (
LgbceScraper,
ScraperException,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import mock
from unittest import mock

from django.contrib.gis.geos import MultiPolygon
from django.test import TestCase
from organisations.boundaries.osni import OsniLayer
Expand Down
2 changes: 1 addition & 1 deletion every_election/apps/organisations/tests/test_admin.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import os
from datetime import date
from unittest.mock import Mock, patch

import boto3
from botocore.exceptions import ClientError
from django.contrib.auth import get_user_model
from django.test import TestCase, override_settings
from django.urls import reverse
from elections.tests.factories import ElectedRoleFactory
from mock.mock import Mock, patch
from moto import mock_aws
from organisations.models import (
DivisionProblem,
Expand Down
Loading