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

Add arm64v8, ppc64le, s390x images of ubuntu:focal using aptman/qus #98

Merged
merged 5 commits into from
Mar 12, 2021

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Dec 11, 2020

For python-pillow/Pillow#5028 (comment). Companion to python-pillow/Pillow#5088.

Add arm64v8, ppc64le, s390x images of ubuntu:focal using aptman/qus.
Images will fail to push until python-pillow/Pillow#5088 is merged due to new failures (libtiff was previously untested on big-endian, now exposes new failures).

These images are the same as ubuntu-20.04-focal-amd64 but on different architectures and with libimagequant-dev added.
More information is in python-pillow/Pillow#5088.

Building all images locally now requires running docker run --rm --privileged aptman/qus -s -- -p or similar first.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Couple of small suggestions

ubuntu-20.04-focal-arm64v8/Dockerfile Outdated Show resolved Hide resolved
ubuntu-20.04-focal-arm64v8/Dockerfile Outdated Show resolved Hide resolved
@nulano
Copy link
Contributor Author

nulano commented Dec 12, 2020

Dockerfiles updated to match the #96 version.

@hugovk
Copy link
Member

hugovk commented Dec 12, 2020

Thanks!

Images will fail to push until python-pillow/Pillow#5088 is merged due to new failures (libtiff was previously untested on big-endian, now exposes new failures).

So this is failing until python-pillow/Pillow#5088 is merged (needs test changes from there), but python-pillow/Pillow#5088 is also failing until this is merged (needs images pushed from here). Is that right?

Can we split things up so we can merge and keep things green?

@hugovk
Copy link
Member

hugovk commented Dec 12, 2020

From python-pillow/Pillow#5088:


Speed comparison (approximate)

Architecture Travis CI GHA / Create Docker image GHA / Test in Docker image GHA Total
arm64 7 min 15 min 50 sec 8 min 15 sec (1.18x Travis) 24 min 5 sec
ppc64le 4 min 20 sec 16 min (*) 9 min 15 sec (2.15x Travis) 25 min 15 sec (*)
s390x 2 min 15 sec 14 min (*) 11 min 45 sec (5.22x Travis) 25 min 45 sec (*)

*: The ppc64le and s390x image builds currently fail a bit early due to new test failures, the install time will be slightly longer.


Thanks for the timings. They're not too bad, per repo, and definitely worth trying out. And this repo CI doesn't need to run as often as the main Pillow one.

We could use a technique we used on Travis to help speed up the builds as a whole. The GHA docs say:

The order that you define a matrix matters. The first option you define will be the first job that runs in your workflow.

So we can kick off the slow ones first and make better use of idle parallel jobs for the quick ones, so we're not waiting around for the slow ones to finish.

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 0bc5563..46ad119 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -12,6 +12,11 @@ jobs:
       matrix:
         target: [ "stable", "latest" ]
         image:
+          # Run slower jobs first to give them a headstart and reduce waiting time
+          - "ubuntu-20.04-focal-arm64v8"
+          - "ubuntu-20.04-focal-ppc64le"
+          - "ubuntu-20.04-focal-s390x"
+          # Then run the remainder
           - "alpine"
           - "amazon-2-amd64"
           - "arch"
@@ -22,9 +27,6 @@ jobs:
           - "fedora-33-amd64"
           - "ubuntu-18.04-bionic-amd64"
           - "ubuntu-20.04-focal-amd64"
-          - "ubuntu-20.04-focal-arm64v8"
-          - "ubuntu-20.04-focal-ppc64le"
-          - "ubuntu-20.04-focal-s390x"

@nulano
Copy link
Contributor Author

nulano commented Dec 12, 2020

Thanks!

Images will fail to push until python-pillow/Pillow#5088 is merged due to new failures (libtiff was previously untested on big-endian, now exposes new failures).

So this is failing until python-pillow/Pillow#5088 is merged (needs test changes from there), but python-pillow/Pillow#5088 is also failing until this is merged (needs images pushed from here). Is that right?

That's what I thought, but I just remembered there is also another problem. The "stable" jobs here will continue failing until the next release, and since the image push here is conditional on "stable" success, python-pillow/Pillow#5088 won't succeed until then either. Would it make sense to change it to look at the "latest" run instead? That would also let the .ci/install_raqm.sh change take effect here earlier. Then the new failing stable jobs can be excluded from the matrix until the next release.

Can we split things up so we can merge and keep things green?

Sure, the main repo PR can be split into adding xfail markers first and adding the new jobs second.

Edit: See python-pillow/Pillow#5091 for the first part fixing the tests in the "latest" jobs.

Co-authored-by: Hugo van Kemenade <[email protected]>
@hugovk
Copy link
Member

hugovk commented Dec 13, 2020

Or we could update the Pillow submodule here to point to current Pillow master.

A more cautious alternative would be to patch the test changes onto 8.0.1 (in a temporary branch), and update the submodule to point to that instead.

@radarhere What do you think?

@radarhere
Copy link
Member

How about this - I've created a temporary branch here, https://github.com/python-pillow/docker-images/tree/qus (if I remember, I'll remove it sometime), to just push the new images to docker, leaving all the existing images unmodified. I've restarted python-pillow/Pillow#5088, so that now passes apart from coverage.

From there, the simplest thing would seem to be to wait three weeks until 8.1.0 to merge this in.

Sound good?

@nulano
Copy link
Contributor Author

nulano commented Dec 14, 2020

That seems reasonable to me, but see my comment on the commit 43e252e#r45093209

I would instead use if: always() for the push step.

@radarhere
Copy link
Member

Ok, I've updated Pillow in the branch to use the version from python-pillow/Pillow#5088 and restored tests. The docker images have been updated, and I've rerun the Pillow PR jobs.

@nulano
Copy link
Contributor Author

nulano commented Jan 3, 2021

Now that the stable builds have been updated to 8.1.0, there are no failures anymore.

@wiredfool
Copy link
Member

Are we good here?

@hugovk
Copy link
Member

hugovk commented Mar 12, 2021

We're good, thanks!

@hugovk hugovk merged commit 9e34ded into python-pillow:master Mar 12, 2021
@nulano nulano deleted the qus branch March 12, 2021 15:29
@radarhere radarhere mentioned this pull request Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants