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

Fix tox + uv caching #29

Merged
merged 13 commits into from
Dec 2, 2024
Merged

Fix tox + uv caching #29

merged 13 commits into from
Dec 2, 2024

Conversation

dfsnow
Copy link
Member

@dfsnow dfsnow commented Dec 2, 2024

This PR updates our GitHub Actions workflows to fix the caching integration between tox and uv. It also switches most workflows over to using uv's built-in Python setup rather than the setup-python GitHub Action.

@dfsnow dfsnow force-pushed the dfsnow/setup-tox-caching branch 3 times, most recently from aca597a to 4bcd774 Compare December 2, 2024 19:46
@dfsnow dfsnow force-pushed the dfsnow/setup-tox-caching branch from 4bcd774 to 31d5198 Compare December 2, 2024 19:50
@dfsnow dfsnow closed this Dec 2, 2024
@dfsnow dfsnow reopened this Dec 2, 2024
@dfsnow dfsnow force-pushed the dfsnow/setup-tox-caching branch from bc55777 to 9ff3c3f Compare December 2, 2024 20:44
@dfsnow dfsnow changed the title Split tox setup and run Fix tox + uv caching Dec 2, 2024
Comment on lines 23 to 24
shell: bash
run: uv venv
Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually faster than using setup-python and isolates things in a virtual env. One downside is that it just installs the latest Python version by default (3.13 currently). If we want to pin a version we could add one to this command or add a .python-version file to the root of our project.

Choose a reason for hiding this comment

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

[Praise] I like this! Nice that it's faster, and I also think it's helpful that it more closely matches the way we use uv for development.

Comment on lines 23 to +24
- name: Install uv
uses: astral-sh/setup-uv@v3
with:
enable-cache: true
cache-dependency-glob: pyproject.toml
cache-suffix: docs
uses: astral-sh/setup-uv@v4
Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually faster to run this without using the cache.

Choose a reason for hiding this comment

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

[Praise] Common uv W 🚀

Comment on lines -26 to -37
uses: astral-sh/setup-uv@v3
with:
enable-cache: true
cache-dependency-glob: pyproject.toml

- name: Setup Python
uses: actions/setup-python@v5
with:
python-version-file: pyproject.toml

- name: Install Python dependencies
run: uv pip install .
Copy link
Member Author

Choose a reason for hiding this comment

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

None of this stuff is actually necessary to build the package, so we can remove it.

with:
enable-cache: true
cache-dependency-glob: pyproject.toml
cache-suffix: tox
cache-suffix: ${{ matrix.python-version }}-tox
Copy link
Member Author

@dfsnow dfsnow Dec 2, 2024

Choose a reason for hiding this comment

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

This is the cause of the caching issue identified in ccao-data/ccao#33. Basically, uv disables caching in CI by default because it's slower to recover the cache than it is to just uv pip install (wild).

It's supposed to cache built-from-source wheels even when pruning is enabled. However, there's a race condition in the testing setup since the first-to-finish jobs are the ones to populate the cache, and those jobs won't have the built-from-source wheel.

The easy fix here is to just give each Python version its own cache.

Choose a reason for hiding this comment

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

[Praise] This is wild 🤯 Great find!

Comment on lines +89 to +91
passenv =
UV_CACHE_DIR
PYTHONUNBUFFERED
Copy link
Member Author

Choose a reason for hiding this comment

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

Not strictly necessary I don't think, but also can't hurt.

@dfsnow dfsnow marked this pull request as ready for review December 2, 2024 22:28
@dfsnow dfsnow requested a review from wrridgeway as a code owner December 2, 2024 22:28
@dfsnow dfsnow requested a review from jeancochrane December 2, 2024 22:28
Copy link

@jeancochrane jeancochrane left a comment

Choose a reason for hiding this comment

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

Awesome work! I'll add these learnings over to my PR on ccao too.

Comment on lines 23 to 24
shell: bash
run: uv venv

Choose a reason for hiding this comment

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

[Praise] I like this! Nice that it's faster, and I also think it's helpful that it more closely matches the way we use uv for development.


- name: Install dependencies
run: uv pip install . ruff

- name: Lint with ruff
run: ruff check --output-format=github .
run: uv run ruff check --output-format=github .

Choose a reason for hiding this comment

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

[Question, non-blocking] This is totally unrelated to this PR, so no need to resolve now, but I wonder if we really need this lint workflow at all given that pre-commit is running ruff anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thinking, another holdover from the R package. I removed it in 55e3225.

Comment on lines 23 to +24
- name: Install uv
uses: astral-sh/setup-uv@v3
with:
enable-cache: true
cache-dependency-glob: pyproject.toml
cache-suffix: docs
uses: astral-sh/setup-uv@v4

Choose a reason for hiding this comment

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

[Praise] Common uv W 🚀

with:
enable-cache: true
cache-dependency-glob: pyproject.toml
cache-suffix: tox
cache-suffix: ${{ matrix.python-version }}-tox

Choose a reason for hiding this comment

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

[Praise] This is wild 🤯 Great find!

@dfsnow dfsnow force-pushed the dfsnow/setup-tox-caching branch 2 times, most recently from 0d2737c to 55e3225 Compare December 2, 2024 23:34
@dfsnow dfsnow merged commit 494da48 into main Dec 2, 2024
16 checks passed
@dfsnow dfsnow deleted the dfsnow/setup-tox-caching branch December 2, 2024 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants