Check that intended python version is being used #808
Conversation
dandavison
force-pushed
the
fix-python-version-in-ci
branch
2 times, most recently
from
940c592 to
c088cc2
Compare
dandavison
force-pushed
the
fix-python-version-in-ci
branch
from
87a747c to
3dc6d6d
Compare
There was a problem hiding this comment.
Not sure this PR has much value if it's just for "double checking" any more than we need to double check everything else GH actions install/use. I do think https://github.com/actions/setup-python will remain a reasonable/official way to install Python in GH actions even though uv (and apt and others) also offer a way.
| - name: Check python version | ||
| shell: bash | ||
| run: | | ||
| actual=$(uv run python -c "import sys; print('.'.join(map(str, sys.version_info[:2])))") | ||
| if [[ "$actual" != "${{ matrix.python }}" ]]; then | ||
| echo "Python version in use by uv ($actual) does not match intended version (${{ matrix.python }})" | ||
| exit 1 | ||
| fi | ||
| actual=$(python -c "import sys; print('.'.join(map(str, sys.version_info[:2])))") | ||
| if [[ "$actual" != "${{ matrix.python }}" ]]; then | ||
| echo "Python version on PATH ($actual) does not match intended version (${{ matrix.python }})" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Is this embedded GH workflow logic/check necessary? Can we not trust setup-python? Do we need to check Rust version, protoc version, CPU arch, etc too if we're concerned with the actions? I can't tell from the PR description whether something happened that made this not work and made us concerned.
| @@ -48,7 +48,20 @@ jobs: | |||
| repo-token: ${{ secrets.GITHUB_TOKEN }} | |||
| - uses: astral-sh/setup-uv@v5 | |||
| - run: uv tool install poethepoet | |||
| - run: uv sync --all-extras | |||
| - run: uv sync --all-extras --python ${{ matrix.python }} | |||
There was a problem hiding this comment.
What is the use case for --python instead of letting it use the existing installed one? (even if we change the install method)
|
Yes, it appears that this PR is not strictly necessary at this point in time. But, I'm still learning
I need to read the discussion at https://github.com/astral-sh/setup-uv?tab=readme-ov-file#enable-caching and discussion in actions/setup-python#822 again to see whether the idea is that But note that we are testing across multiple platforms -- Windows in particular differs a lot, and I've noticed that |
Yes, this is my understanding. I am just wary of embedding bash into our workflows to check for tool versions after we install them unless it's shown to be needed. Obviously it's not a big deal, just a bit abnormal compared to what we do elsewhere when installing language runtime. It does make sense that non-setup-python Python install approaches may not configure it for default use on GH runners. |
|
Let's close this one. I mainly wanted to check that we were currently correct, and we are. I've extracted out the README change to #811 |
This PR changes GitHub actions CI jobs as follows:
Where a specific python version is intended, specify the version when using
uv sync.We continue to use
setup-pythonin the github actions to set the python version on$PATH. A moreuv-native way to do this may become available but I think is not currently available. Perhapsuv python installin the future. Also in the future, we can investigate uv caching: see https://github.com/astral-sh/setup-uv?tab=readme-ov-file#enable-caching and discussion in Support foruvPackage Manager Caching actions/setup-python#822.One reason I made this PR was to double-check that we were indeed running tests against the intended python versions, despite not specifying the version in
uv sync. I've confirmed that we were (see #809). Presumably becauseuvuses the python interpreter installed by thesetup-pythonaction. Nevertheless, it seems good to be explicit in theuvcommands, especially since, as described above, I think we will eventually move away fromsetup-python.