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.

Sign up for GitHub

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 the reusable tox workflow #1102

Merged
merged 2 commits into from
Nov 7, 2024
Merged

Migrate to the reusable tox workflow #1102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
48f38e2
Rename the test workflow to better reflect its purpose
kurtmckee Nov 6, 2024
8ab0996
Migrate to a reusable workflow
kurtmckee Nov 6, 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
90 changes: 0 additions & 90 deletions .github/workflows/build.yaml
View file
Open in desktop

This file was deleted.

80 changes: 80 additions & 0 deletions .github/workflows/test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
name: "🧪 Test"

on:
push:
branches:
- "main"
pull_request: null
# build weekly at 4:00 AM UTC
schedule:
- cron: '0 4 * * 1'

jobs:
test:
name: "${{ matrix.name }}"
strategy:
fail-fast: false
matrix:
# The `include` array below will match against these names
# and add additional keys and values to the JSON object.
name:
- "Linux"
- "macOS"
- "Windows"
- "Quality"

# The `include` array below will also inherit these values,
# which are critical for effective cache-busting.
# The nested list syntax ensures that the full array of values is inherited.
cache-key-hash-files:
-
- "requirements/*/*.txt"
- "pyproject.toml"
- "toxfile.py"
Copy link
Member

Choose a reason for hiding this comment

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

I have been wondering, over the past week, about whether or not tox-uv's faster venv building makes it unnecessary to cache the .tox dir contents. As long as the uv action's cache is populated, .tox/ can be quickly rebuilt.
One of the things I wonder is whether or not the balance between the two may, in fact, favor rebuilding over caching (since caching and hashing take some time).

I'm curious if you've given this any thought?

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 have. Caching the tarballs and wheels, instead of caching everything that was installed, hasn't previously been faster.

The numbers are borne out best on Windows, so I'll share from the feedparser logs, which tests the highest and lowest supported CPython versions (and which I recommend doing here, but didn't introduce in this PR).

Here's the timings reported by feedparser tests for Windows with a cache miss:

  py3.9-chardet: OK (45.76=setup[7.22]+cmd[38.55] seconds)
  py3.13-chardet: OK (41.80=setup[9.69]+cmd[32.11] seconds)
  congratulations :) (87.68 seconds)

and for a cache hit:

  py3.9-chardet: OK (42.22=setup[3.74]+cmd[38.48] seconds)
  py3.13-chardet: OK (31.66=setup[0.21]+cmd[31.45] seconds)
  congratulations :) (74.01 seconds)

(Note that the first tox environment always has the wheel build step counted in as a part of its setup.) Since the cmd times per tox environment are within ~0.5s of each other between the cache-miss and cache-hit executions, I'm more inclined to trust that the setup times aren't simply GitHub runner jitter.

So, my interpretation is that this is a win of ~13 seconds across 2 tox environments on Windows.

It took 1 second to look up the cache and miss, and then 5 seconds to upload the cache from the cache-miss job; it subsequently took 2 seconds to download the cache for the cache-hit job, which is an additional ~4 seconds won.

I have consistently found that it's faster to cache what's installed, rather than caching what needs to be installed. tox-uv makes environment creation and package installation fast, but I don't think it's fast enough.

You're welcome to try improving on this! It's mechanically trivial, but extremely time-consuming. Here's the steps:

  1. Create a branch off this project (or my own workflow repo)
  2. Point a second project with a "significant" test suite at the new branch
  3. Repeatedly push and force-push to the second project, possibly manually deleting the caches, and keep switching back to the workflow project to make and push changes.

Copy link
Member

Choose a reason for hiding this comment

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

I find this explanation 110% satisfactory. I'm probably not going to experiment with this at least within the next few days: my main question was about the comparison between time(cache miss + tox_uv setup + cache save) vs time(cache hit + tox_uv setup) and you've already provided numbers for that.

I am willing to accept some minor regressions in CI speeds if it gives us other improvements (e.g., workflow simplicity). In particular, I've been trying to track in the PRs as you've converted us over to the new workflow -- what exactly is being used for cache keys and is it "correct"?
The uv action cache carries all of the raw packages already (in the runner's homedir), so there's some interesting interplay there with the .tox dir.

Thanks for laying this all out for me!

Copy link
Member Author

@kurtmckee kurtmckee Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I think I see what you're referring to. This isn't using the uv GitHub action, so there's no side caching happening, and pip caching isn't enabled for the setup-python action.

For cache keys, here's the rule that I've generally been following:

Already-included files

These files are always included by the reusable workflow:

  • .python-identifiers

    (generated by the kurtmckee/detect-pythons action; ensures that the cache -- which contains symlinks to Python interpreter executables -- is invalidated if the Python versions change)

  • .workflow-config.json

    (ensures that changes to the requested configuration invalidates the cache)

  • tox.ini

    (ensures that changes to the tox configuration invalidates the cache)

Files you should use with cache-key-hash-files

In general, any files that contain tool configuration directives should be hashed for cache-busting.

  • pyproject.toml
  • mypy.ini
  • .flake8
  • .pre-commit-config.yaml
  • setup.cfg
  • requirements/*/*.txt
  • poetry.lock

If these files change, it can indicate that different dependencies should be installed, or that a tool like mypy should change how it's writing its own cache, or any number of other things that might make the workflow cache less useful.


include:
- name: "Linux"
runner: "ubuntu-latest"
cpythons:
- "3.8"
- "3.9"
- "3.10"
- "3.11"
- "3.12"
- "3.13"
tox-post-environments:
- "py3.8-mindeps"
- "coverage_report"

- name: "macOS"
runner: "macos-latest"
cpythons:
- "3.11"
tox-post-environments:
- "coverage_report"

- name: "Windows"
runner: "windows-latest"
cpythons:
- "3.11"
tox-post-environments:
- "coverage_report"

- name: "Quality"
runner: "ubuntu-latest"
cpythons:
- "3.13"
tox-environments:
- "check-min-python-is-tested"
- "mypy"
- "mypy-test"
- "poetry-check"
- "pylint"
- "test-lazy-imports"
- "twine-check"
cache-paths:
- ".mypy_cache/"

uses: "globus/workflows/.github/workflows/tox.yaml@04b4abd6fcb9b4be7263bc9d6994ae2ada220739" # v1.1
with:
config: "${{ toJSON(matrix) }}"
4 changes: 4 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ repos:
rev: 0.6.7
hooks:
- id: alphabetize-codeowners
- repo: https://github.com/rhysd/actionlint
rev: v1.7.4
hooks:
- id: actionlint

# custom local hooks
- repo: local
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ attr = "globus_sdk.__version__"
# non-packaging tool configs follow

[tool.pytest.ini_options]
addopts = "--no-success-flaky-report"
addopts = "--no-success-flaky-report --color=yes"
testpaths = ["tests"]
norecursedirs = ["tests/non-pytest"]
filterwarnings = [
Expand Down
26 changes: 13 additions & 13 deletions scripts/ensure_min_python_is_tested.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,23 @@
)
requires_python_version = proc.stdout.decode().strip()

with open(REPO_ROOT / ".github" / "workflows" / "build.yaml") as f:
with open(REPO_ROOT / ".github" / "workflows" / "test.yaml") as f:
workflow = YAML.load(f)
try:
test_mindeps_job = workflow["jobs"]["test-mindeps"]
except KeyError:
raise ValueError("Could not find the test-mindeps job. Perhaps it has moved?")

job_steps = test_mindeps_job["steps"]
for step in job_steps:
if "uses" in step and "actions/setup-python" in step["uses"]:
setup_python_step = step
includes = workflow["jobs"]["test"]["strategy"]["matrix"]["include"]
for include in includes:
if include["name"] == "Linux":
break
else:
raise ValueError("Could not find the setup-python step.")
raise ValueError("Could not find 'Linux' in the test matrix.")

python_version = setup_python_step["with"]["python-version"]
if python_version != requires_python_version:
for environment in include["tox-post-environments"]:
if environment.endswith("-mindeps"):
break
else:
raise ValueError("Could not find a '-mindeps' tox-post-environment.")

python_version, _, _ = environment.partition("-")
if python_version != f"py{requires_python_version}":
print("ERROR: ensure_min_python_is_tested.py failed!")
print(
f"\nPackage data sets 'Requires-Python: >={requires_python_version}', "
Expand Down
20 changes: 10 additions & 10 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ envlist =
pylint
test-lazy-imports
coverage_clean
py{313,312,311,310,39,38}
py38-mindeps
py{3.13,3.12,3.11,3.10,3.9,3.8}
py3.8-mindeps
coverage_report
docs
minversion = 4.22.0
labels =
freezedeps = freezedeps-print,freezedeps-py{313,312,311,310,39,38}
freezedeps = freezedeps-print,freezedeps-py{3.13,3.12,3.11,3.10,3.9,3.8}

[testenv]
# build a wheel, not a tarball, and use a common env to do it (so that the wheel is shared)
Expand All @@ -23,8 +23,8 @@ deps =
mindeps: -r requirements/py{py_dot_ver}/test-mindeps.txt
commands = coverage run -m pytest {posargs}
depends =
py{313,312,311,310,39,38}{-mindeps,}: coverage_clean, lint
coverage_report: py{313,312,311,310,39,38}{-mindeps,}
py{3.13,3.12,3.11,3.10,3.9,3.8}{-mindeps,}: coverage_clean, lint
coverage_report: py{3.13,3.12,3.11,3.10,3.9,3.8}{-mindeps,}

[testenv:coverage_clean]
dependency_groups = coverage
Expand Down Expand Up @@ -65,7 +65,7 @@ deps = pyright
commands = pyright src/ {posargs}

[testenv:docs]
# force use of py311 for doc builds so that we get the same behaviors as the
# force use of py3.11 for doc builds so that we get the same behaviors as the
# readthedocs doc build
basepython = python3.11
deps = -r requirements/py{py_dot_ver}/docs.txt
Expand Down Expand Up @@ -104,7 +104,7 @@ commands =
python -m dependency_groups typing -o requirements/.typing.in
python -m dependency_groups test-mindeps -o requirements/.test-mindeps.in
python -m dependency_groups docs -o requirements/.docs.in
[testenv:freezedeps-py{313,312,311,310,39,38}]
[testenv:freezedeps-py{3.13,3.12,3.11,3.10,3.9,3.8}]
description = freeze development dependencies using pip-compile
skip_install = true
setenv =
Expand All @@ -116,18 +116,18 @@ commands =
pip-compile --strip-extras -q -U --resolver=backtracking .typing.in -o py{py_dot_ver}/typing.txt

# Minimum dependencies are only tested against the lowest supported Python version.
py38: pip-compile --strip-extras -q -U --resolver=backtracking .test-mindeps.in -o py{py_dot_ver}/test-mindeps.txt
py3.8: pip-compile --strip-extras -q -U --resolver=backtracking .test-mindeps.in -o py{py_dot_ver}/test-mindeps.txt

# The docs requirements are only generated for Python 3.11.
py311: pip-compile --strip-extras -q -U --resolver=backtracking .docs.in -o py{py_dot_ver}/docs.txt
py3.11: pip-compile --strip-extras -q -U --resolver=backtracking .docs.in -o py{py_dot_ver}/docs.txt
depends = freezedeps-print

[testenv:check-min-python-is-tested]
description = Check the Requires-Python metadata against CI config
skip_install = true
deps =
ruamel.yaml<0.18
mddj==0.0.6
mddj==0.0.8
commands = python scripts/ensure_min_python_is_tested.py

[testenv:prepare-release]
Expand Down