-
Notifications
You must be signed in to change notification settings - Fork 4
[ci] ensure correct Python version is used in CI, fix compatibility with older pre-3.12 patch versions #355
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
Conversation
.github/workflows/unit-tests.yml
Outdated
| shell: bash -l {0} | ||
| run: | | ||
| pip install -r ./requirements-tests.txt || exit 1 | ||
| python -m pip install -r ./requirements-tests.txt || exit 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.
This was an attempt to fix something weird I'm seeing, but it didn't work.
On the unit-tests (macOS-15-intel, Python-CPython 3.9) job, it looks like the correct Python is getting installed:
/Users/runner/hostedtoolcache/Python/3.9.25/x64/bin/pip
pip 25.3 from /Users/runner/hostedtoolcache/Python/3.9.25/x64/lib/python3.9/site-packages/pip (python 3.9)
/Users/runner/hostedtoolcache/Python/3.9.25/x64/bin/python
Python 3.9.25
/Users/runner/hostedtoolcache/Python/3.9.25/x64/bin/python3
Python 3.9.25
But pytest is picking up the Python 3.14 from the host
============================= test session starts ==============================
platform darwin -- Python 3.14.2, pytest-9.0.2, pluggy-1.6.0
For the unit-tests (windows-latest, Python-CPython 3.9) job, it appears to picking up the correct Python AND pytest is choosing the right version.
/c/hostedtoolcache/windows/Python/3.9.13/x64/Scripts/pip
pip 25.3 from C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pip (python 3.9)
/c/hostedtoolcache/windows/Python/3.9.13/x64/python
Python 3.9.13
/c/hostedtoolcache/windows/Python/3.9.13/x64/python3
Python 3.9.13
...
============================= test session starts =============================
platform win32 -- Python 3.9.13, pytest-8.4.2, pluggy-1.6.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.
Welp, seeing all these Python 3.14 paths in the "install" step of that Python 3.9 macOS job looks like a problem:
python -m pip install -v dist/*.whl
shell: /bin/bash -l {0}
env:
pythonLocation: /Users/runner/hostedtoolcache/Python/3.9.25/x64
PKG_CONFIG_PATH: /Users/runner/hostedtoolcache/Python/3.9.25/x64/lib/pkgconfig
Python_ROOT_DIR: /Users/runner/hostedtoolcache/Python/3.9.25/x64
Python2_ROOT_DIR: /Users/runner/hostedtoolcache/Python/3.9.25/x64
Python3_ROOT_DIR: /Users/runner/hostedtoolcache/Python/3.9.25/x64
Using pip 25.3 from /Library/Frameworks/Python.framework/Versions/3.14/lib/python3.14/site-packages/pip (python 3.14)
Processing ./dist/pydistcheck-0.11.0.99-py3-none-any.whl
Requirement already satisfied: click>=8.0 in /Library/Frameworks/Python.framework/Versions/3.14/lib/python3.14/site-packages (from pydistcheck==0.11.0.99) (8.3.1)
Installing collected packages: pydistcheck
changing mode of /Library/Frameworks/Python.framework/Versions/3.14/bin/pydistcheck to 755
Successfully installed pydistcheck-0.11.0.99
I honestly don't understand what's happening here. According to https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#using-update-environment-flag, the Python installed by actions/seutp-python should be added to PATH by default.
Maybe it's added to the end and therefore not being found? But if that's what is happening, I don't know how to reconcile that with which pip / pip --version looking like this:
/Users/runner/hostedtoolcache/Python/3.9.25/x64/bin/pip
pip 25.3 from /Users/runner/hostedtoolcache/Python/3.9.25/x64/lib/python3.9/site-packages/pip (python 3.9)
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.
There it is!
/Library/Frameworks/Python.framework/Versions/Current/bin shows up earlier in PATH than any of the stuff added by setup-python.
...
:/usr/local/opt/pipx_bin
:...
:/usr/local/bin:/usr/local/sbin:/Users/runner/bin
:...
:/Library/Frameworks/Python.framework/Versions/Current/bin
:...
:/Users/runner/hostedtoolcache/Python/3.9.25/x64/bin
:/Users/runner/hostedtoolcache/Python/3.9.25/x64
:...
:/usr/local/opt/pipx_bin
:...
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.
So it does look like setup-python is sticking its own paths on the beginning of PATH as expected
And even immediately before the "install" step
I think I see the problem... the install step is using shell: bash - l {0}. -l (login shell) is reloading .bashrc and that updates PATH with stuff, putting more of the GitHub Actions paths at the beginning of PATH
The default shell GitHub uses is bash -e {0}, or bash --noprofile --norc -eo pipefail {0} if you set it to bash... notice that --noprofile --norc!!
Honestly, there's no reason CI is using bash -l here. I'm certain I just copied that from somewhere else when I set up this repo. I'm gonna remove that everywhere.
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.
I pushed commits removing the -l and saw the correct Python versions being used!
|
|
||
|
|
||
| @lru_cache | ||
| def _tf_extractall_has_filter() -> bool: |
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.
After fixing the Python versions in CI here, I saw the following error in Windows Python 3.9 tests:
E + where 1 = <Result TypeError("extractall() got an unexpected keyword argument 'filter'")>.exit_code
It makes sense... the filter argument wasn't added to tarfile.Tarfile.extractall() until Python 3.12.
But why wasn't this test failing on any other operating system or older-than-3.12 Python version?
Looks like the filter argument was backported back to Python 3.8 (python/cpython#102950). The 3.9 backport was in 3.9.17 (python/cpython#104382) and actions/setup-python is setting up 3.9.13 for Windows:
...
Successfully set up CPython (3.9.13)
...
What a journey!
In #354, I happened to specify a version constraint that excluded a package from being installed in Python 3.9 environments. I was surprised to see errors like this:
(build link)
In a CI job that I thought was testing with Python 3.14.
Looks like due to a misconfiguration of
conda-incubator/miniconda, most unit test jobs have not been using the correct Python version, and instead have been falling back to the Python interpreter that comes installed on the GitHub Actions runners!!This fixes that by removing
condafrom those jobs completely... it was only being used to switch Python versions, theactions/setup-pythonofficial GitHub Action will be fine for that.That uncovered a bug!!
pydistcheckis unconditionally callingTarFile.extractall(..., filter="data"), butfilterwasn't added until Python 3.12 andactions/setup-pythonsets up a Python 3.9 patch version from earlier than the backport (#355 (comment)). This fixes that too.Other changes related to Python versions:
actions/pre-commitinstead ofconda-installingpre-commitpython -m pipin place ofpiporpipxbash -l(login shell) withbash(no profile) to avoid clobberingPATHmodifications made byactions/setup-python