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.

Sign up for GitHub

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

GH-45039: [CI][Packaging][Python] Fix Docker push step for free-threaded wheel builds #45040

Merged
merged 2 commits into from
Dec 17, 2024
Merged

GH-45039: [CI][Packaging][Python] Fix Docker push step for free-threaded wheel builds #45040

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b6759e
GH-45039: [CI][Packaging][Python] Fix Docker push step for free-threa…
pitrou Dec 17, 2024
9e6cdb8
Fix step name
pitrou Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 14 additions & 16 deletions dev/tasks/python-wheels/github.linux.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,16 @@

{{ macros.github_header() }}

# Testing free-threaded wheels uses a different Docker setup
{% set test_imports_image = (
'python-free-threaded-wheel-manylinux-test-imports' if python_abi_tag == 'cp313t'
else 'python-wheel-manylinux-test-imports')
%}
{% set test_unittests_image = (
'python-free-threaded-wheel-manylinux-test-unittests' if python_abi_tag == 'cp313t'
else 'python-wheel-manylinux-test-unittests')
%}

Comment on lines +22 to +31
Copy link
Member

Choose a reason for hiding this comment

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

I think that using GitHub Actions features instead of using Jinja2 is easier to read/understand:

diff --git a/dev/tasks/python-wheels/github.linux.yml b/dev/tasks/python-wheels/github.linux.yml
index f083b7c0c8..eadb948339 100644
--- a/dev/tasks/python-wheels/github.linux.yml
+++ b/dev/tasks/python-wheels/github.linux.yml
@@ -50,6 +50,15 @@ jobs:
       {{ macros.github_install_archery()|indent }}
       {{ macros.github_login_dockerhub()|indent }}
 
+      - name: Prepare
+        run: |
+          if [ "${PYTHON_ABI_TAG}" = "cp313t" ]; then
+            test_image_prefix=python-free-threaded
+          else
+            test_image_prefix=python
+          fi
+          echo "TEST_IMAGE_PREFIX=${test_image_prefix}" >> ${GITHUB_ENV}
+
       - name: Build wheel
         shell: bash
         env:
@@ -72,23 +81,11 @@ jobs:
 
       # TODO(kszucs): auditwheel show
       - name: Test wheel
-        if: |
-          '{{ python_abi_tag }}' != 'cp313t'
-        shell: bash
-        run: |
-          source arrow/ci/scripts/util_enable_core_dumps.sh
-          archery docker run python-wheel-manylinux-test-imports
-          archery docker run python-wheel-manylinux-test-unittests
-
-      # Free-threaded wheels need to be tested using a different Docker Compose service
-      - name: Test free-threaded wheel
-        if: |
-          '{{ python_abi_tag }}' == 'cp313t'
         shell: bash
         run: |
           source arrow/ci/scripts/util_enable_core_dumps.sh
-          archery docker run python-free-threaded-wheel-manylinux-test-imports
-          archery docker run python-free-threaded-wheel-manylinux-test-unittests
+          archery docker run ${TEST_IMAGE_PREFIX}-wheel-manylinux-test-imports
+          archery docker run ${TEST_IMAGE_PREFIX}-wheel-manylinux-test-unittests
 
       - name: Test wheel on AlmaLinux 8
         shell: bash
@@ -145,5 +142,5 @@ jobs:
         shell: bash
         run: |
           archery docker push python-wheel-manylinux-{{ manylinux_version }}
-          archery docker push python-wheel-manylinux-test-unittests
+          archery docker push ${TEST_IMAGE_PREFIX}-wheel-manylinux-test-unittests
       {% endif %}

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I don't know, both are uncommon to me :) Feel free to open a fixup PR if you want.

Copy link
Member

Choose a reason for hiding this comment

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

#45077

permissions:
packages: write

Expand Down Expand Up @@ -72,23 +82,11 @@ jobs:

# TODO(kszucs): auditwheel show
- name: Test wheel
if: |
'{{ python_abi_tag }}' != 'cp313t'
shell: bash
run: |
source arrow/ci/scripts/util_enable_core_dumps.sh
archery docker run python-wheel-manylinux-test-imports
archery docker run python-wheel-manylinux-test-unittests

# Free-threaded wheels need to be tested using a different Docker Compose service
- name: Test free-threaded wheel
if: |
'{{ python_abi_tag }}' == 'cp313t'
shell: bash
run: |
source arrow/ci/scripts/util_enable_core_dumps.sh
archery docker run python-free-threaded-wheel-manylinux-test-imports
archery docker run python-free-threaded-wheel-manylinux-test-unittests
archery docker run {{ test_imports_image }}
archery docker run {{ test_unittests_image }}

- name: Test wheel on AlmaLinux 8
shell: bash
Expand Down Expand Up @@ -141,9 +139,9 @@ jobs:
{{ macros.github_upload_wheel_scientific_python("arrow/python/repaired_wheels/*.whl")|indent }}

{% if arrow.is_default_branch() %}
pitrou marked this conversation as resolved.
Show resolved Hide resolved
- name: Push Docker Image
- name: Push Docker images
shell: bash
run: |
archery docker push python-wheel-manylinux-{{ manylinux_version }}
archery docker push python-wheel-manylinux-test-unittests
archery docker push {{ test_unittests_image }}
Copy link
Member

Choose a reason for hiding this comment

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

We may want to push test_unittests_image too.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean test_imports_image? It's actually an upstream Python image.

  python-wheel-manylinux-test-imports:
    image: ${ARCH}/python:${PYTHON_IMAGE_TAG}

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry. Yes. I meant test_imports_image.
I didn't notice that python-wheel-manylinux-test-imports is python:XXX.
I just checked python-free-threaded-wheel-manylinux-test-imports:

arrow/docker-compose.yml

Lines 1207 to 1210 in 37eb3b5

# TODO: Remove this when the official Docker Python image supports the free-threaded build.
# See https://github.com/docker-library/python/issues/947 for more info.
python-free-threaded-wheel-manylinux-test-imports:
image: ${REPO}:${ARCH}-python-3.13-free-threaded-wheel-manylinux-test-imports

We don't need to cache it because it's a temporary image?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I don't know. But it's probably not very important, since the build step took 1 minute in https://github.com/ursacomputing/crossbow/actions/runs/12377700603/job/34547835928

Copy link
Member

Choose a reason for hiding this comment

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

OK. Let's keep the current configuration. (We don't cache the python-free-threaded-wheel-manylinux-test-imports image.)

{% endif %}
Loading