Skip to content
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

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

mayeut
Copy link
Contributor

@mayeut mayeut commented Jun 4, 2022

Description:
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)
Allowing 3.11 for pre-release also allows to simplify workflows in situations like in pypa/packaging#551 which is using the python-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:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@mayeut mayeut requested a review from a team June 4, 2022 17:39
@dmitry-shibanov
Copy link
Contributor

Hello @mayeut. Thank you for your pull request. I think the same behaviour can be achieved through specifying python-version input as ~3.10.0.alpha.x and adding the check-latest functionality. With this input for python-version it will handle unstable and stable versions.

@mayeut
Copy link
Contributor Author

mayeut commented Jun 4, 2022

@dmitry-shibanov,

Thank you for your pull request. I think the same behaviour can be achieved through specifying python-version input as ~3.10.0.alpha.x and adding the #406 functionality. With this input for python-version it will handle unstable and stable versions.

The syntax proposed will still require workarounds and bash string manipulation when using python-version in a matrix together with tools like nox/tox (c.f. . pypa/packaging#551 for example).

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.
If you don't want to pay this price, then we're back to modifying the workflow twice in order to remove check-latest.

PS: the correct syntax would be ~3.10.0-0 for 3.10
PS2: I reworked src/find-python.ts to use this syntax
PS3: check-latest is not needed, the syntax on its own works, it's just awkward to use that syntax.

@mayeut mayeut force-pushed the stable-or-prerelease branch 4 times, most recently from 05908a6 to b81f92e Compare June 10, 2022 03:14
@mayeut mayeut force-pushed the stable-or-prerelease branch 2 times, most recently from f977444 to b5e5876 Compare June 16, 2022 05:48
@mayeut mayeut force-pushed the stable-or-prerelease branch 2 times, most recently from aaac910 to 0711a89 Compare July 1, 2022 19:58
@mayeut mayeut force-pushed the stable-or-prerelease branch 2 times, most recently from e5b2793 to 0de7c57 Compare July 9, 2022 10:34
@mayeut mayeut force-pushed the stable-or-prerelease branch 2 times, most recently from 64f1e85 to 9a0529a Compare August 2, 2022 20:49
@brcrista
Copy link
Contributor

brcrista commented Aug 5, 2022

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 3.11 a match for 3.11-alpha:

prerelease versions frequently are updated very quickly, and contain many breaking changes that are (by the author's design) not yet fit for public consumption. Therefore, by default, they are excluded from range matching semantics.

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.

@ssbarnea
Copy link

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.

@brcrista
Copy link
Contributor

brcrista commented Sep 7, 2022

@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 setup- actions still stands.

@brcrista
Copy link
Contributor

brcrista commented Sep 7, 2022

@mayeut I've been giving this some more thought. In setup-node, there's a latest keyword you can use that does what it says on the tin. Since I imagine this fallback should only apply to the latest major/minor version, would that be a good fit for this use case?

@webknjaz
Copy link

I think the point I made about consistency with the NPM version spec syntax and the other setup- actions still stands.

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.
It's like coming to a restaurant and being handed chopsticks for eating a soup, just because the chef is also making a lot of sushi and needs consistency across all the meals.

@mayeut
Copy link
Contributor Author

mayeut commented Oct 1, 2022

Since I imagine this fallback should only apply to the latest major/minor version, would that be a good fit for this use case?

@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.
Second, it's not really what you want to do, you don't really want 3.11-dev which could install 3.11.1a1 later on (it's not the case at the moment, but I would expect it to be the case) so you'll need to revert this one to 3.11 later on.

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.

However, the amount of friction here is by design.

IMHO, X.Y & pypyX.Y should be considered as special citizens here & not involve that amount of friction. Since I wrote this PR, #213 has been closed and now, another issue has been created: #501.
I understand the whole generic idea behind the spec to only match GA version as a default but we're not in a generic case here. The user explicitly requested an X.Y version for which no GA exists yet, we should install the latest pre-release in this case.
I could rework this with another input to the action which would then still be easier (and once the new input has been added, it can stay there forever, no need to remove or re-add later) e.g.

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 }}

@EwoutH
Copy link

EwoutH commented Oct 5, 2022

I would just like to say that I support enabling to use preview Python versions without the -dev suffix, since it will reduce maintenance efforts. Thanks for the PR!

@aaugustin
Copy link

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.

@webknjaz
Copy link

It would be great to avoid having to repeat this next year for 3.12-dev -> 3.12 (and I'm not finished yet):

@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 ~ or -dev, it's currently possible to use these markers in the condition for continue-on-failure. If it's going to be just the version, one would have to introduce an experimental flag in the matrix. And as the version becomes stable, they'd still have to make a change to drop that flag.
So strictly speaking, not having to update the matrix factor in one place would just shift the same activity to another matrix factor and that's it.

@hugovk
Copy link
Contributor

hugovk commented Oct 27, 2022

I don't know, none of my ~25 PRs above needed that, 3.11-dev was just a normal matrix job 🤷

@aaugustin
Copy link

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!

@brcrista
Copy link
Contributor

@mayeut to move forward with this, let's add it as an optional input: allow-prereleases: true.

Rationale:

  1. Not matching prereleases by default follows SemVer and is how Pip and Poetry work. It's also consistent with the rest of the setup- actions.
  2. It avoids a breaking change to setup-python.

Thoughts?

@webknjaz
Copy link

I don't know, none of my ~25 PRs above needed that, 3.11-dev was just a normal matrix job 🤷

@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.

@mayeut
Copy link
Contributor Author

mayeut commented Dec 26, 2022

@brcrista, I updated the PR with an optional input: allow-prereleases: true. I still need to update the docs with the new input.

EDIT: the doc has been updated.
EDIT2: the 2 failing workflows have nothing to do with this PR and are failing in other PRs and on main.

@mayeut mayeut force-pushed the stable-or-prerelease branch 2 times, most recently from e8c355d to f6ec9e5 Compare December 27, 2022 10:54
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."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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."

@brcrista brcrista requested a review from a team January 4, 2023 17:58
@mayeut mayeut force-pushed the stable-or-prerelease branch 2 times, most recently from f56e6ab to 5808a6d Compare January 8, 2023 09:17
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`)
@mayeut
Copy link
Contributor Author

mayeut commented Jan 22, 2023

@brcrista, I think I answered all remarks. Anything I missed to get this merged ?

Copy link
Contributor

@brcrista brcrista left a 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 !

@hugovk
Copy link
Contributor

hugovk commented Mar 9, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet