Skip to content

Conversation

kieran-ryan
Copy link
Member

@kieran-ryan kieran-ryan commented Sep 1, 2025

🤔 What's changed?

⚡️ What's your motivation?

  • Improve CI performance

    Metric Previous Now Delta % Change
    Total Runner Time 23m 55s
    (1435s)
    16m 47s
    (1007s)
    -7m 8s
    (-428s)
    -29.8%
    Total Python Test Runner Time 9m 49s
    (589s)
    2m 58s
    (178s)
    -6m 51s
    (-411s)
    -69.8%
    Python Test Workflow Duration 1m 59s
    (119s)
    40s -1m 19s
    (-79s)
    -66.4%
    Python Test Fastest Job 47s 12s 35s -74.5%
    Python Test Average Job 1m 6s
    (66s)
    19.8s -46.2s -70%
    Python Codecov Step 213s
    (avg. 23s)
    23s
    (avg. 2.6s)
    -190s
    (-3m 10s)
    -89.2%
  • Improve maintainability

  • Improve developer experience

    • Fixes mypy cache requiring deletion before subsequent tox runs
    • Fixes tox running tests against all Python versions with linux only - having been configured for GitHub Actions using tox-gh-actions rather than local development

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ Anything particular you want feedback on?

  • Codecov GitHub Action not uploading with "Token required because branch is protected" error (#)... which historically has been an intermittent issue (see Upload failed: {"message":"Token required because branch is protected"} codecov/codecov-action#1709); hitting a 429 rate limit with a comparable error message "Please upload with the Codecov repository upload token to resolve issue" before the change... though the same configuration is running on pyprojectsort. Assumption that the issue is pre-existing and that the change in error message is in consequence of migrating from the deprecated CLI to the GitHub Action. Under those circumstances, would suggest to tackle separately if not immediately resolvable through a token change or other means - with no material impact.
  • Would advocate for pre-commit.ci integration to autoupdate dependencies and improve linting performance; the caveat being it would trigger for a negligible ~2s on each run to check whether there are changed files to lint - compared to a GitHub workflow which would only trigger when there are Python changes. Increasingly desirable with wider adoption of polyglot linting hooks over time.
  • Having optimised the slowest, most expensive workflow, assessing whether the remaining bottlenecks to rapid CI feedback (cpp and javascript workflows) could be optimised, would be a worthwhile endeavour.
  • Windows runner performance appears inconsistent on dependency download. Further caching optimisation across uv, tox and pre-commit may mitigate.
  • Will evaluate ty type checking integration separately over mypy; and uv lock pre-commit hook in use on gherkin.

📋 Checklist:

@kieran-ryan kieran-ryan added the 🏦 debt Tech debt label Sep 1, 2025
- Matches gherkin repository implementation
- Explicit trusted publishing for uv
- Run pre-commit through uv backend
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (5cf3d14) to head (0e2731b).
⚠️ Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #324   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          200       200           
=========================================
  Hits           200       200           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Run mypy through pre-commit
- Run Python test workflow through uv
- Use Python dependency groups standard
@kieran-ryan kieran-ryan force-pushed the debt/standardise-py-tooling branch from 0e2731b to 570d4da Compare September 13, 2025 15:23
@kieran-ryan kieran-ryan force-pushed the debt/standardise-py-tooling branch from a6238d7 to e4da46b Compare September 13, 2025 17:49
@kieran-ryan kieran-ryan force-pushed the debt/standardise-py-tooling branch from 157bbba to 578f166 Compare September 13, 2025 18:49
@kieran-ryan kieran-ryan self-assigned this Sep 13, 2025
@kieran-ryan kieran-ryan force-pushed the debt/standardise-py-tooling branch from 392ca8b to f390853 Compare September 13, 2025 21:40
@kieran-ryan kieran-ryan force-pushed the debt/standardise-py-tooling branch 3 times, most recently from 0e77823 to 74134d2 Compare September 13, 2025 22:58
@kieran-ryan kieran-ryan force-pushed the debt/standardise-py-tooling branch from 74134d2 to 81568c3 Compare September 13, 2025 23:00
@kieran-ryan kieran-ryan changed the title Enhance Python tooling infrastructure Optimise Python tooling infrastructure Sep 14, 2025
@kieran-ryan kieran-ryan force-pushed the debt/standardise-py-tooling branch from 571d0e1 to aa297d0 Compare September 14, 2025 15:02
@kieran-ryan kieran-ryan marked this pull request as ready for review September 14, 2025 15:10
@mpkorstanje mpkorstanje self-requested a review September 14, 2025 15:15
@mpkorstanje
Copy link
Contributor

mpkorstanje commented Sep 14, 2025

Cheers! Again, much appreciated!

Legacy .egg-info metadata and tests removed from distribution

Is this a breaking change? If so, please put it in the CHANGELOG?

Codecov GitHub Action not uploading with "Token required because branch is protected" error

I'll have a look.

Would advocate for pre-commit.ci integration to autoupdate dependencies and improve linting performance; the caveat being it would trigger for a negligible ~2s on each run to check whether there are changed files to lint - compared to a GitHub workflow which would only trigger when there are Python changes. Increasingly desirable with wider adoption of polyglot linting hooks over time.

Not opposed in principle.

The biggest limitation to using any sort of pre-commit is that the tools triggered by pre-commit must be installed on a developers machine. This is a problem in a polyglot repo because not everyone will have all tools installed. At a glance it looks like pre-commit.ci avoids that problem.

Updating dependencies should preferably be done through Renovate. Just so we have one obvious way of doing things. 😉

@kieran-ryan
Copy link
Member Author

kieran-ryan commented Sep 14, 2025

Cheers! Again, much appreciated!

Welcome Rien... was pleasantly surprised by how much performance optimisation was available over a significant proportion of the pipeline. 🙌

Legacy .egg-info metadata and tests removed from distribution

Is this a breaking change? If so, please put it in the CHANGELOG?

Added. Early legacy Python distribution metadata; low impact probability though worth a shout.

Codecov GitHub Action not uploading with "Token required because branch is protected" error

I'll have a look.

Thank you! Based on the unrelated #334 - where some jobs succeeded where as others subsequently hit rate limits in the initial workflow; and all failed in subsequent workflows; it may be the case that we need to split coverage reporting into its own job with a dependency on collecting the coverage artefacts from the test jobs - to limit how often it runs. So potentially one to circle back on as a separate issue should you concur issue is pre-existing - possibly requiring deeper exploration with codecov.

The biggest limitation to using any sort of pre-commit is that the tools triggered by pre-commit must be installed on a developers machine. This is a problem in a polyglot repo because not everyone will have all tools installed. At a glance it looks like pre-commit.ci avoids that problem.

Yeah very fair. Right now its scoped to the Python directory, so wouldn't impact other implementations; though it might be worth experimenting with approaches to evaluate whether it could be used as a standardised polyglot trigger for linting - which would be powerful. The below should work as a simplified way to install and run - installing and running the specified tools without additional prerequisites or installation's on the developer's machine (to the best of my understanding) - for what we use right now that is - though other options would be worth exploring to understand impact on the contributing experience in any case.

$ curl -LsSf https://astral.sh/uv/install.sh | sh
$ uv tool install pre-commit --with pre-commit-uv --force-reinstall
$ pre-commit run --all-files

Updating dependencies should preferably be done through Renovate. Just so we have one obvious way of doing things. 😉

Standardisation would be ideal... was working off assumption that Renovate is without pre-commit support... though appears partial Renovate pre-commit support is in "beta" testing! Might be worth evaluating (#335). Assume would be without support for frozen hashes (introduced by this pull request) over tag names - though would happily revert that change to enable the capability. Unlikely to be as integrated, however would certainly preference a working standardised polyglot alternative (Renovate).

Edit: See your other comment now on hashes - sweet! Ya veramos... let's see how goes.

@mpkorstanje mpkorstanje merged commit c70a8ee into main Sep 15, 2025
37 checks passed
@mpkorstanje mpkorstanje deleted the debt/standardise-py-tooling branch September 15, 2025 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏦 debt Tech debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants