Skip to content

GitHub Actions: Simplify requirements cache #10288

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Mar 19, 2025

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Why recreate a venv and keep reactivating that venv at each job step when actions/setup-python already automatically does so?

Why keep dependencies in both pyproject.toml and multiple requirements_*.txt files which make cache key calculation complex?

Remove the complex cache gymnastics to see if pytests can pass on PyPy 3.11.

Refs #XXXX

Closes #10287
Closes #10286

The one failing pytest on PyPy3.9 and PyPy3.10 seems to be a full path vs. partial path difference.

- Using config file /opt/hostedtoolcache/PyPy/3.9.19/x64/lib/pypy3.9/site-packages/pylint/testutils/testing_pylintrc
+ Using config file pylint/testutils/testing_pylintrc

Which is minor but the 48 other failing pytests on PyPy3.11 seem to require attention.

Copy link

codecov bot commented Mar 19, 2025

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.86%. Comparing base (a3e5bef) to head (73f17d0).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #10288   +/-   ##
=======================================
  Coverage   95.86%   95.86%           
=======================================
  Files         175      175           
  Lines       19074    19074           
=======================================
  Hits        18286    18286           
  Misses        788      788           
πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cclauss cclauss changed the title GitHub Actions: Fix failing PyPy tests GitHub Actions: 49 failing pytests on PyPy 3.11 Mar 19, 2025

This comment has been minimized.

@Pierre-Sassoulas
Copy link
Member

Let's separate pypy3.10 fix / caching changes / pypy 3.11 support. I can see the former being merged relatively fast but 49 failing tests is going to take a while (and afaik no maintainer is using pypy).

@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Mar 19, 2025
@Pierre-Sassoulas Pierre-Sassoulas added this to the 4.0.0 milestone Mar 19, 2025
@mbyrnepr2
Copy link
Member

The cache logic changes look a great improvement but do you need to explicitly switch on the caching for pip?

Supported package managers are pip, pipenv and poetry. The cache input is optional, and caching is turned off by default.

https://github.com/actions/setup-python?tab=readme-ov-file#caching-packages-dependencies

@cclauss
Copy link
Contributor Author

cclauss commented Mar 22, 2025

I find the blending of requirements in pyproject.toml AND in multiple requirements_*.txt files to be quite confusing but perhaps all the combinations and permutations of add-ons and plugins make that necessary. I will create a different PR to try to use matrix.os to reduce the complexity of .github/workflows/tests.yaml before I circle back to this one.

This comment has been minimized.

@Pierre-Sassoulas
Copy link
Member

I find the blending of requirements in pyproject.toml AND in multiple requirements_*.txt files to be quite confusing but perhaps all the combinations and permutations of add-ons and plugins make that necessary.

afair it's a choice we did because someone preferred to be able to have requirements.txt 2/3 years ago. Personally I add an extra called dev / test in the pyproject.toml and remove requirements.txt (but it does create an extra on pypi that no one is supposed to use). If you have a proposal that is better / simpler than what we currently have I'm open to it.

@cclauss cclauss changed the title GitHub Actions: 49 failing pytests on PyPy 3.11 GitHub Actions: Simplify requirements cache Mar 24, 2025
@cclauss cclauss marked this pull request as draft March 24, 2025 16:02
@Julfried
Copy link
Contributor

PEP 735 defines dependency groups, which do extactly what Pierre suggested here, without having to create extras in pyproject.toml. This will also be supported in the upcoming pip release: pypa/pip#13065. Other package managers, like uv already support it

@cclauss
Copy link
Contributor Author

cclauss commented Mar 26, 2025

For me, the confusion comes in here

-r requirements_test_min.txt

and
.[testutils,spelling]

where requirements.txt files can import other requirements files and even optional-dependencies from pyproject.toml.

I guess we will need to use dependency groups that specify all dependencies that they need.

@Julfried
Copy link
Contributor

PEPβ€―735 dependency groups are intended to be self-contained lists of dependencies, so one group cannot automatically include extras from another (e.g. using .[testutils, spelling]). I believe this kind of composition isn’t possible with dependency groups, though I'm not entirely sure. Before implementing any changes, it would be wise to wait until this feature is officially released in pip.

@cclauss
Copy link
Contributor Author

cclauss commented Mar 28, 2025

Or stop using pip and use uv instead.

@Julfried
Copy link
Contributor

While I am also a fan of uv, I do not think that forcing everyone to use it is the way to go here. Moving the github actions to uv makes sense though to improve speed.

@Pierre-Sassoulas
Copy link
Member

Agree with @Julfried here, we should not force uv on every potential contributor, let's wait for pip support for that and the github action can use uv in the meantime.

@Pierre-Sassoulas
Copy link
Member

Another thing though is that we're caching pylint editable install and we should not (it makes the doc generation and config file exemple generation checks fail unexpectedly)

This comment has been minimized.

@cclauss cclauss force-pushed the fix-pypy-tests branch 3 times, most recently from bf35eba to aa836fd Compare March 31, 2025 06:21

This comment has been minimized.

@DanielNoord
Copy link
Collaborator

Other idea: what if we run this new setup in parallel and check if it fixes all caching issues? Then we don't need to worry about porting everything and stuff potentially breaking. We can just make it a non required workflow and migrate once we deem it to be finished.

@cclauss
Copy link
Contributor Author

cclauss commented Mar 31, 2025

astral-sh/setup-uv@v5 is not allowed to be used in pylint-dev/pylint. Actions in this workflow must be: within a repository owned by pylint-dev, created by GitHub, or matching the following: codecov/codecov-action@, kanga333/comment-hider@, pypa/gh-action-pip-audit@, pypa/gh-action-pypi-publish@, sigstore/gh-action-sigstore-python@, softprops/action-gh-release@, tibdex/backport@*.

@cclauss cclauss closed this Mar 31, 2025
@cclauss cclauss deleted the fix-pypy-tests branch March 31, 2025 07:22
Copy link
Contributor

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit e6e4048

@Julfried
Copy link
Contributor

Julfried commented Mar 31, 2025

Other idea: what if we run this new setup in parallel and check if it fixes all caching issues? Then we don't need to worry about porting everything and stuff potentially breaking. We can just make it a non required workflow and migrate once we deem it to be finished.

I would agree here. Also I think we should first add dependency groups to pyproject.toml, so that people who use uv can use them and newly added actions aswell. Once they work properly and pip officially supports them remove all the requirements.txt files.

@cclauss
Copy link
Contributor Author

cclauss commented Mar 31, 2025

I think you folks have a clearer idea of what you want than I do, so please create the required PRs.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants