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

Update Python versions in the CI workflow. #590

Merged
merged 11 commits into from May 2, 2023
Merged

Conversation

@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #590 (b2d99a3) into main (77abdb0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #590   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          356       356           
  Branches        23        29    +6     
=========================================
  Hits           356       356           

@ezio-melotti
Copy link
Member Author

3.12 is failing with

==================================== ERRORS ====================================
__________________ ERROR collecting tests/test_backport_pr.py __________________
tests/test_backport_pr.py:12: in <module>
    from miss_islington import backport_pr
miss_islington/backport_pr.py:9: in <module>
    from . import tasks, util
miss_islington/tasks.py:9: in <module>
    from celery import bootsteps
/opt/hostedtoolcache/Python/3.12.0-alpha.2/x64/lib/python3.12/site-packages/celery/bootsteps.py:6: in <module>
    from kombu.common import ignore_errors
/opt/hostedtoolcache/Python/3.12.0-alpha.2/x64/lib/python3.12/site-packages/kombu/common.py:14: in <module>
    from .entity import Exchange, Queue
/opt/hostedtoolcache/Python/3.12.0-alpha.2/x64/lib/python3.12/site-packages/kombu/entity.py:7: in <module>
    from .serialization import prepare_accept_content
/opt/hostedtoolcache/Python/3.12.0-alpha.2/x64/lib/python3.12/site-packages/kombu/serialization.py:440: in <module>
    for ep, args in entrypoints('kombu.serializers'):  # pragma: no cover
/opt/hostedtoolcache/Python/3.12.0-alpha.2/x64/lib/python3.12/site-packages/kombu/utils/compat.py:82: in entrypoints
    for ep in importlib_metadata.entry_points().get(namespace, [])
E   AttributeError: 'EntryPoints' object has no attribute 'get'

entry_points() used to return a dict, but now returns an EntryPoints instance, that apparently doesn't have a .get method. See:

cc @jaraco

@hugovk
Copy link
Member

hugovk commented Nov 23, 2022

Suggestions for this PR or another.

We can also bump these to:

  • actions/checkout@v3
  • actions/cache@v3
  • codecov/codecov-action@v3

I've got a script to bump actions (works only on tags not branches, but that covers most), although dependabot can take care of it all later.

We can now also remove the actions/cache step and use the newish caching built in to actions/setup-python:

Something like:

      - uses: actions/setup-python@v4
        with:
          python-version: ${{ matrix.python-version }}
          cache: pip
          cache-dependency-path: pyproject.toml

@jaraco
Copy link
Member

jaraco commented Dec 2, 2022

Kombu is relying on interfaces removed in 3.12 (python/cpython#97785). Fixed in celery/kombu#1601.

@ewdurbin ewdurbin temporarily deployed to miss-islingt-update-ci--1xxuua December 2, 2022 14:55 Inactive
@jaraco
Copy link
Member

jaraco commented Dec 2, 2022

By the way, you may want to use this technique to avoid having to do two-phase commits for each Python version (see actions/setup-python#508).

@ewdurbin ewdurbin temporarily deployed to miss-islingt-update-ci--1xxuua December 4, 2022 10:41 Inactive
@ezio-melotti
Copy link
Member Author

Thanks @jaraco for looking into this!

By the way, you may want to use this technique to avoid having to do two-phase commits for each Python version (see actions/setup-python#508).

I'm not sure I understand what you mean here. AFAICS, you are adding -dev to the matrix and then use python-version: ${{ matrix.python }}${{ matrix.dev }} -- isn't the result equivalent to python-version: ["3.9-dev", "3.10-dev", "3.11-dev", "3.12-dev"]? Also doesn't the -dev suffix only apply to the latest version (i.e. 3.12)?

With the current PR, once 3.13 is released we will just need to do:

- python-version: ["3.9", "3.10", "3.11", "3.12-dev"]
+ python-version: ["3.9", "3.10", "3.11", "3.12", "3.13-dev"]

So, unless I'm missing something, I don't see a reason to handle the -dev separately.

@jaraco
Copy link
Member

jaraco commented Dec 4, 2022

isn't the result equivalent to python-version: ["3.9-dev", "3.10-dev", "3.11-dev", "3.12-dev"]?

Yes. Because -dev is meaningless after final is released and required before final is released, so it's just meaningless cruft enforced by the maintainers of the action.

So, unless I'm missing something, I don't see a reason to handle the -dev separately.

Except, within about a month of Python 3.12 going final, someone is going to notice that it has -dev attached and going to want to remove that cruft, so you'll make another commit or someone will send a PR to remove the -dev, which you'll probably accept. Then, when 3.13 comes around a few weeks later, you'll need to add it and have to remember to add -dev. If you're lucky, you might get away with just one commit, but in my experience, that approach doesn't generalize.

If instead you apply -dev to all versions, then you end up with (a) a consistent way to declare all supported Python versions and (b) the ability to test on pre-release Python versions. Of course, adding -dev explicitly to all Python versions makes the matrix noisy and also crufty, but by applying the dev as a separate matrix element, it gets applied mostly silently.

You're welcome to continue to manage it manually - I just wanted to share that I've found a better way that involves less toil.

@ewdurbin ewdurbin temporarily deployed to miss-islingt-update-ci--va2w54 December 25, 2022 05:33 Inactive
@ezio-melotti
Copy link
Member Author

I now updated the PR to include your suggestion.

@hugovk
Copy link
Member

hugovk commented Mar 1, 2023

Good news: actions/setup-python#414 was merged to fallback to a pre-release when there's no stable.

It's not been released yet, but when it is, we can use plain x.y across the board.

(Although we'd still need a temporary 3.12.0-alpha.4 pending those upstream dependencies.)

@Mariatta
Copy link
Member

Can we just update to 3.11 on this PR? the 3.12 is still failing.

@hugovk
Copy link
Member

hugovk commented Apr 19, 2023

Yeah, let's split 3.12 into another PR, those other things will take some time, hopefully they will be sorted during the beta. 🤞

Some other updates we can add to this PR:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 75a8d93..2e614cb 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -18,8 +18,8 @@ jobs:
         dev: ["-dev"]
 
     steps:
-      - uses: actions/checkout@v2
-      - uses: actions/cache@v1
+      - uses: actions/checkout@v3
+      - uses: actions/cache@v3
         with:
           path: ~/.cache/pip
           key: ${{ runner.os }}-pip-${{ hashFiles('pyproject.toml') }}
@@ -30,7 +30,7 @@ jobs:
           python-version: ${{ matrix.python-version }}${{ matrix.dev }}
       - run: python3 -m pip install -U -r dev-requirements.txt
       - run: pytest --cov=. --cov-report=xml
-      - uses: codecov/codecov-action@v2
+      - uses: codecov/codecov-action@v3
         if: always()
         with:
           token: ${{ secrets.CODECOV_TOKEN }}

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
ezio-melotti and others added 2 commits May 2, 2023 16:07
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@ezio-melotti ezio-melotti merged commit b0fdea6 into main May 2, 2023
4 checks passed
@ewdurbin ewdurbin temporarily deployed to miss-islingt-update-ci--q0goyf May 2, 2023 14:35 Inactive
@ezio-melotti ezio-melotti deleted the update-ci-py-versions branch May 2, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants