-
Notifications
You must be signed in to change notification settings - Fork 0
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
Configure ruff and UV #23
Conversation
6a34bdc
to
574b892
Compare
uses: actions/setup-python@v5 | ||
with: | ||
python-version: "3.x" | ||
cache: "pip" | ||
python-version-file: pyproject.toml |
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 just install the min version specified in our pyproject.toml
, in this case 3.10
.
@@ -18,14 +18,24 @@ jobs: | |||
- name: Checkout |
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.
Note that usually we want to install Python packages into the system library with uv
by setting UV_SYSTEM_PYTHON=1
. However, in this case, we want uv
to install things into the tox-created virtualenv, so we leave that var unset.
deps = | ||
setuptools>=61.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.
This installs dependencies into each tox environment using uv
.
uv tool install tox --with tox-uv | ||
tox --version |
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.
The tox-uv
plugin sets up virtual environments using uv
that are compatible with tox
.
[gh] | ||
python = | ||
3.12 = py312 | ||
3.11 = py311 | ||
3.10 = py310 | ||
3.9 = py39 | ||
3.8 = py38 |
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 no longer need this environment mapping since we just explicitly specify the tox environment via these commands:
env=$(echo ${{ matrix.python-version }} | tr -d '.' | sed 's/^/py/')
tox r -v --notest -e "$env"
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 work here! A couple questions about Python log buffering and appropriate ways of running system-level tools with uv
, but nothing serious.
@@ -6,21 +6,30 @@ on: | |||
|
|||
name: lint | |||
|
|||
env: | |||
PYTHONUNBUFFERED: "1" |
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.
[Question, non-blocking] Should we be setting this more widely across our workflows? I've never noticed a buffering problem with Python scripts in GitHub workflow logs (except on our network, which blocks websockets and so prevents all real-time logs from displaying) but maybe I just haven't been watching out for the right issue.
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's purely defensive. I've run into a few cases over the past couple months where Python won't output full logs due to its buffered output.
.github/workflows/pages.yaml
Outdated
|
||
- name: Generate HTML | ||
run: | | ||
sphinx-build -d _build/doctrees docs/source _build/html | ||
uv run sphinx-build -d _build/doctrees docs/source _build/html |
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.
[Question, non-blocking] If we're installing packages into the global environment by setting UV_SYSTEM_PYTHON=1
, do we need to prefix the sphinx-build
call with uv
here?
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.
You're right, totally unneeded! I removed it in acafd68.
.github/workflows/test-coverage.yaml
Outdated
uv run pytest --doctest-modules \ | ||
--junitxml=junit/test-results-${{ matrix.python-version }}.xml |
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.
[Question, non-blocking] Same question here about using the uv
prefix when UV_SYSTEM_PYTHON=1
.
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.
Fixed in acafd68!
f1569e6
to
acafd68
Compare
This PR swaps our Python packaging toolchain to
uv
, similar to the switch performed in ccao-data/data-architecture#635. The main benefit here is simpler tooling and much faster builds.The diff here looks really messy, but most of it is reformatting from
ruff
. I recommend excluding 2a7f7c1 and only reviewing the tooling changes.Closes #22.