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

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

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Dec 17, 2024

Copy link

⚠️ GitHub issue #45039 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added the awaiting review Awaiting review label Dec 17, 2024
@pitrou
Copy link
Member Author

pitrou commented Dec 17, 2024

@github-actions crossbow submit wheel-manylinux-2-28-cp313-cp313-amd64 wheel-manylinux-2-28-cp313-cp313t-amd64

Copy link

Revision: 3b6759e

Submitted crossbow builds: ursacomputing/crossbow @ actions-7af60c9692

Task Status
wheel-manylinux-2-28-cp313-cp313-amd64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-amd64 GitHub Actions

@pitrou pitrou marked this pull request as ready for review December 17, 2024 13:04
@pitrou
Copy link
Member Author

pitrou commented Dec 17, 2024

@kou @raulcd Does this seem ok?

@pitrou
Copy link
Member Author

pitrou commented Dec 17, 2024

@github-actions crossbow submit wheelmanylinux

Copy link

Revision: 9e6cdb8

Submitted crossbow builds: ursacomputing/crossbow @ actions-552bdb3274

Task Status
wheel-manylinux-2-28-cp310-cp310-amd64 GitHub Actions
wheel-manylinux-2-28-cp310-cp310-arm64 GitHub Actions
wheel-manylinux-2-28-cp311-cp311-amd64 GitHub Actions
wheel-manylinux-2-28-cp311-cp311-arm64 GitHub Actions
wheel-manylinux-2-28-cp312-cp312-amd64 GitHub Actions
wheel-manylinux-2-28-cp312-cp312-arm64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313-amd64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313-arm64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-amd64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-arm64 GitHub Actions
wheel-manylinux-2-28-cp39-cp39-amd64 GitHub Actions
wheel-manylinux-2-28-cp39-cp39-arm64 GitHub Actions
wheel-manylinux-2014-cp310-cp310-amd64 GitHub Actions
wheel-manylinux-2014-cp310-cp310-arm64 GitHub Actions
wheel-manylinux-2014-cp311-cp311-amd64 GitHub Actions
wheel-manylinux-2014-cp311-cp311-arm64 GitHub Actions
wheel-manylinux-2014-cp312-cp312-amd64 GitHub Actions
wheel-manylinux-2014-cp312-cp312-arm64 GitHub Actions
wheel-manylinux-2014-cp313-cp313-amd64 GitHub Actions
wheel-manylinux-2014-cp313-cp313-arm64 GitHub Actions
wheel-manylinux-2014-cp313-cp313t-amd64 GitHub Actions
wheel-manylinux-2014-cp313-cp313t-arm64 GitHub Actions
wheel-manylinux-2014-cp39-cp39-amd64 GitHub Actions
wheel-manylinux-2014-cp39-cp39-arm64 GitHub Actions

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

Thanks @pitrou , LGTM, I am happy to merge without actually pushing the image and just validate on the nightlies

dev/tasks/python-wheels/github.linux.yml Show resolved Hide resolved
@github-actions github-actions bot added awaiting merge Awaiting merge awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting merge Awaiting merge awaiting changes Awaiting changes labels Dec 17, 2024
@pitrou
Copy link
Member Author

pitrou commented Dec 17, 2024

@github-actions crossbow submit wheel-manylinux-2-28-cp313-cp313-amd64 wheel-manylinux-2-28-cp313-cp313t-amd64 wheel-manylinux-2014-cp312-cp312-amd64 wheel-manylinux-2014-cp313-cp313-arm64

Copy link

Revision: 3f3a068

Submitted crossbow builds: ursacomputing/crossbow @ actions-605028a7bb

Task Status
wheel-manylinux-2-28-cp313-cp313-amd64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-amd64 GitHub Actions
wheel-manylinux-2014-cp312-cp312-amd64 GitHub Actions
wheel-manylinux-2014-cp313-cp313-arm64 GitHub Actions

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Dec 17, 2024
@pitrou pitrou force-pushed the gh45039-wheel-push branch from 3f3a068 to 9e6cdb8 Compare December 17, 2024 17:21
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Dec 17, 2024
@pitrou pitrou merged commit 8f336c4 into apache:main Dec 17, 2024
18 checks passed
@pitrou pitrou removed the awaiting change review Awaiting change review label Dec 17, 2024
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.)

Comment on lines +22 to +31
# 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')
%}

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.

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

Successfully merging this pull request may close these issues.

3 participants