-
Notifications
You must be signed in to change notification settings - Fork 551
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
feature: fallback to pre-release when no stable version is found #414
Conversation
Hello @mayeut. Thank you for your pull request. I think the same behaviour can be achieved through specifying python-version input as |
The syntax proposed will still require workarounds and bash string manipulation when using It's also not as easy as users expect it to be (actions/runner-images#5540 / actions/runner-images#3343). Furthermore, check-lastest will always download & install from manifest rather than using tool-cache on every stable python update (I'd say for about a week once a month, time for GitHub-hosted runners to get the new python & be deployed) which is a waste of time & bandwidth that's avoided with this PR. PS: the correct syntax would be |
05908a6
to
b81f92e
Compare
f977444
to
b5e5876
Compare
aaac910
to
0711a89
Compare
e5b2793
to
0de7c57
Compare
0de7c57
to
dcae6a4
Compare
64f1e85
to
9a0529a
Compare
Thanks for the suggestion @mayeut. I understand that this change would make certain use cases more ergonomic. However, the amount of friction here is by design. Actions uses NPM-style version patterns. Here's why it doesn't consider
So while it's a good fit for the use case of wanting to test a package against a pre-release version, it opens up the opposite problem of someone needing to use special syntax to make sure their workflow is running with a GA version. I think we should follow NPM's lead of defaulting to the GA case and requiring you to specify otherwise, rather than defaulting to the prerelease and requiring you to specify GA. |
9a0529a
to
59c0e91
Compare
I am not sure who is the product manager on GHA, but I hope that is not the same person as the one saying "the amount of friction here is by design.". The reality is that anyone wanting to test with multiple versions of python (or other tools) will want to be able to start building and testing with it before that version reaches GM. I would be really worries to find-out that, by design, GHA team makes the life harder for anyone trying to do this, forcing them to use more complex versioning or forcing them to have to change their pipelines the day one version of python reaches GM. My hopes were that GHA team to continue working on making its use easier for most users, not harder. I know that sometimes historical mistakes may slow down the progress, but not wanting to provide the best possible experience is a big red flag. It is bit hilarious to see someone from outside trying to help the team and write a pull-request, just to be refused with an answer like that. |
59c0e91
to
8156cbe
Compare
@ssbarnea your feedback is welcome, but please make your points respectfully and avoid criticizing anyone personally. I think the point I made about consistency with the NPM version spec syntax and the other |
@mayeut I've been giving this some more thought. In |
8156cbe
to
51a7a5a
Compare
OTOH, it's extremely weird to see an action for managing Python, that is expected to be used by pythonistas following versioning restrictions that are absolutely foreign to the Python ecosystem. |
@brcrista, no it doesn't, the purpose of this PR is to simplify workflows & interactions between the different tools using one version specifier consistently. I think most python library repos use a matrix of python version to test. Consider the following workflow (& I'll focus on CPython because PyPy is an entirely different animal): jobs:
test:
name: ${{ matrix.os }} / ${{ matrix.python_version }}
runs-on: ${{ matrix.os }}-latest
strategy:
fail-fast: false
matrix:
os: [Ubuntu, Windows, macOS]
python_version: ["3.7", "3.8", "3.9", "3.10", "pypy3.7", "pypy3.8", "pypy3.9"]
steps:
- uses: actions/checkout@v3
- uses: actions/setup-python@v4
name: Install Python ${{ matrix.python_version }}
with:
python-version: ${{ matrix.python_version }}
- name: Run nox
run: pipx run nox --error-on-missing-interpreters -s tests-${{ matrix.python_version }} Adding 3.11 is unnecessarily complex, here's one example: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 785c652..6490779 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -10,7 +10,7 @@ jobs:
fail-fast: false
matrix:
os: [Ubuntu, Windows, macOS]
- python_version: ["3.7", "3.8", "3.9", "3.10", "pypy3.7", "pypy3.8", "pypy3.9"]
+ python_version: ["3.7", "3.8", "3.9", "3.10", "3.11-dev", "pypy3.7", "pypy3.8", "pypy3.9"]
steps:
- uses: actions/checkout@v3
@@ -21,4 +21,9 @@ jobs:
python-version: ${{ matrix.python_version }}
- name: Run nox
- run: pipx run nox --error-on-missing-interpreters -s tests-${{ matrix.python_version }}
+ run: |
+ # Need to remove "-dev" suffix
+ INTERPRETER=${{ matrix.python_version }}
+ INTERPRETER=${INTERPRETER/-dev/}
+ pipx run nox --error-on-missing-interpreters -s tests-${INTERPRETER}
+ shell: bash First, you have to know how to implement the bash trick. This could also be done slightly differently: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 785c652..1e00652 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -10,7 +10,7 @@ jobs:
fail-fast: false
matrix:
os: [Ubuntu, Windows, macOS]
- python_version: ["3.7", "3.8", "3.9", "3.10", "pypy3.7", "pypy3.8", "pypy3.9"]
+ python_version: ["3.7", "3.8", "3.9", "3.10", "3.11", "pypy3.7", "pypy3.8", "pypy3.9"]
steps:
- uses: actions/checkout@v3
@@ -18,7 +18,7 @@ jobs:
- uses: actions/setup-python@v4
name: Install Python ${{ matrix.python_version }}
with:
- python-version: ${{ matrix.python_version }}
+ python-version: "${{ startsWith(matrix.python_version, 'pypy') && matrix.python_version || format('~{0}.0-0', matrix.python_version) }}"
- name: Run nox
run: pipx run nox --error-on-missing-interpreters -s tests-${{ matrix.python_version }} This one requires knowledge of GHA expressions (let's say intermediate level) and NPM version spec syntax and as @webknjaz said, the intended audience for this action is not the NPM users but Python users. Even if it were, the wrong answer was proposed by a maintainer for the version spec to use meaning it's not as trivial as it sounds. There are for sure other ways to do that but I'm almost certain they involve to do something more complex than should be.
IMHO, diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 785c652..1e00652 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -10,7 +10,7 @@ jobs:
fail-fast: false
matrix:
os: [Ubuntu, Windows, macOS]
- python_version: ["3.7", "3.8", "3.9", "3.10", "pypy3.7", "pypy3.8", "pypy3.9"]
+ python_version: ["3.7", "3.8", "3.9", "3.10", "3.11", "pypy3.7", "pypy3.8", "pypy3.9"]
steps:
- uses: actions/checkout@v3
@@ -18,7 +18,7 @@ jobs:
- uses: actions/setup-python@v4
name: Install Python ${{ matrix.python_version }}
with:
python-version: ${{ matrix.python_version }}
+ fallback-no-ga: "allow" # could be "forbid" (default) or "warn" ?
- name: Run nox
run: pipx run nox --error-on-missing-interpreters -s tests-${{ matrix.python_version }} |
I would just like to say that I support enabling to use preview Python versions without the |
I would also enjoy the ability to put "3.11" in my GHA config when it's still a prerelease and forgetting about it :-) The friction of having to set "3.11-dev" and then having to remember to come back to change it to "3.11" isn't improving my life as a GHA user. Not to mention the added friction of having to explain the situation to well-meaning contributors who aren't aware of these contraints. In addition, I'd like to minimize the amount of time I spend dealing with version updates. It isn't the most valuable use of my open-source maintenance time, to say the least. I understand that the proposed improvement here changes the behavior for a user who doesn't want to test on a given version before it's GA and accidentally configures it anyway. Instead of "crash - Python version not found", the behavior becomes "run on a pre-release". I would argue that a user who configures a given Python version probably wants to run that version. I wouldn't assume that they want to crash if that version is still in prerelease. I read the comments about creating friction on purpose but I'm not convinced by the benefits. If one doesn't want to test on a given Python version before it's GA, then one has no reason to add it in the GHA config until it's GA anyway. It looks like the current design degrades the experience of the majority to cater for the minority that makes a mistake by adding new Python versions earlier than intended, before GA. This doesn't sound like a good tradeoff. |
@hugovk I was reflecting on this for a while and realized that similar updates would probably still be necessary. Just think about it: usually, it's unstable at the beginning. So it's typical to allow failures for the dev version. With |
I don't know, none of my ~25 PRs above needed that, |
I don't need a two step process either. I can make one PR that says "3.12" (when it's in beta or RC) and makes required changes (there are usually few or none). No need for more! |
51a7a5a
to
59e5600
Compare
59e5600
to
985fbb6
Compare
985fbb6
to
d16e16d
Compare
@mayeut to move forward with this, let's add it as an optional input: Rationale:
Thoughts? |
d16e16d
to
34bc15e
Compare
@hugovk how many of those are Cython-based? For pure Python the problem described is indeed not as evident but if a project depends on a build tool that needs to make compatibility changes because CPython's ABI got updated, it adds a substantial amount of delay in the early stages of the release cycle. This is why we don't test against Cython stuff against the unreleased Python versions in @aio-libs, for example. This is the case I was thinking about. |
34bc15e
to
ec15bd7
Compare
@brcrista, I updated the PR with an optional input: EDIT: the doc has been updated. |
e8c355d
to
f6ec9e5
Compare
action.yml
Outdated
@@ -23,6 +23,9 @@ inputs: | |||
update-environment: | |||
description: "Set this option if you want the action to update environment variables." | |||
default: true | |||
allow-prereleases: | |||
description: "Set this option if you want the action to allow installing pre-releases python versions when no GA version matches." |
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.
description: "Set this option if you want the action to allow installing pre-releases python versions when no GA version matches." | |
description: "When 'true', a version range passed as to 'python-version' input will match prerelease versions if no GA versions are found." |
f56e6ab
to
5808a6d
Compare
This allows to specify version like `3.11` or `pypy3.10` in workflows before those versions are released. This lessen the burden for users of `setup-python` by not having to modify their workflow twice: once when a pre-release is available (e.g. `3.11-dev`) and once when the first stable release is published (e.g. `3.11`)
5808a6d
to
9c6f2fd
Compare
@brcrista, I think I answered all remarks. Anything I missed to get this merged ? |
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.
Thanks for the contribution @mayeut !
Thank you very much for this PR! It would be great if we can start using it, do you have an idea when it will be released? |
Description:
This allows to specify version like
3.11
orpypy3.10
in workflows before those versions are released.This lessen the burden for users of
setup-python
by not having to modify their workflow twice: once when a pre-release is available (e.g.3.11-dev
) and once when the first stable release is published (e.g.3.11
)Allowing
3.11
for pre-release also allows to simplify workflows in situations like in pypa/packaging#551 which is using thepython-version
specifier as an input to nox and would require some bash string manipulation otherwise.Related issue:
fixes #213
fixes #501
actions/runner-images#5540
actions/runner-images#3343
pypa/packaging#551
Check list: